fix(metrics): log locale instead of accept languages on flow events

#1702

r=vbudhram,vladikoff

Author: philbooth

bin/key_server.js

@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+'use strict'
+
 // Only `require()` the newrelic module if explicity enabled.
 // If required, modules will be instrumented.
 require('../lib/newrelic')()
@@ -74,7 +76,12 @@ function main() {
     log.stat(Password.stat())
   }
 
-  require('../lib/senders')(config, log)
+  let translator
+  require('../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+    .then(result => {
+      translator = result
+      return require('../lib/senders')(log, config, translator)
+    })
     .then(
       function(result) {
         senders = result
@@ -108,7 +115,7 @@ function main() {
                 config,
                 customs
               )
-              server = Server.create(log, error, config, routes, db)
+              server = Server.create(log, error, config, routes, db, translator)
 
               server.start(
                 function (err) {

lib/metrics/events.js

@@ -144,7 +144,7 @@ module.exports = log => {
 
     return request.gatherMetricsContext({
       event: event,
-      locale: coalesceLocale(optionalData, request),
+      locale: marshallLocale(request),
       uid: coalesceUid(optionalData, request),
       userAgent: request.headers['user-agent']
     }).then(data => {
@@ -175,12 +175,10 @@ function optionallySetService (data, request) {
     (request.query && request.query.service)
 }
 
-function coalesceLocale (data, request) {
-  if (data && data.locale) {
-    return data.locale
+function marshallLocale (request) {
+  if (request.app && request.app.locale) {
+    return `${request.app.locale}${request.app.isLocaleAcceptable ? '' : '.default'}`
   }
-
-  return request.app && request.app.acceptLanguage
 }
 
 function coalesceUid (data, request) {

lib/routes/account.js

@@ -221,8 +221,7 @@ module.exports = function (
               account = result
 
               return request.emitMetricsEvent('account.created', {
-                uid: account.uid.toString('hex'),
-                locale: account.locale
+                uid: account.uid.toString('hex')
               })
             }
           )

lib/senders/email.js

@@ -118,7 +118,7 @@ module.exports = function (log) {
     this.supportUrl = config.supportUrl
     this.syncUrl = config.syncUrl
     this.templates = templates
-    this.translator = translator
+    this.translator = translator.getTranslator
     this.verificationUrl = config.verificationUrl
     this.verifyLoginUrl = config.verifyLoginUrl
   }

lib/senders/index.js

@@ -8,21 +8,19 @@ var P = require('../promise')
 // This indirection exists to accommodate different config properties
 // in the old auth mailer. If/when the two config files are merged and
 // there's nothing left that imports mailer/config, it is safe to merge
-// legacy_index.js and this file into one. Be careful not to mix the args
-// up when you do that, they expect config and log in a different order.
+// legacy_index.js and this file into one.
 var createSenders = require('./legacy_index')
 
-module.exports = function (config, log, sender) {
+module.exports = function (log, config, translator, sender) {
   var defaultLanguage = config.i18n.defaultLanguage
 
   return createSenders(
     log,
     {
-      locales: config.i18n.supportedLanguages,
-      defaultLanguage: defaultLanguage,
       mail: config.smtp,
       sms: config.sms
     },
+    translator,
     sender
   )
   .then(

lib/senders/legacy_index.js

@@ -6,27 +6,17 @@
 // Those files should import this module rather than its sibling index.js.
 // If/when we eliminate the old mailer config and everything is importing
 // index.js, we can merge this into there and get rid of the indirection.
-// Be careful when doing that btw, they expect the log and config arguments
-// in a different order.
 
-var P = require('../promise')
 var createMailer = require('./email')
 var createSms = require('./sms')
 
-module.exports = function (log, config, sender) {
+module.exports = function (log, config, translator, sender) {
   var Mailer = createMailer(log)
-  return P.all(
-    [
-      require('./translator')(config.locales, config.defaultLanguage),
-      require('./templates')()
-    ]
-  )
-  .spread(
-    function (translator, templates) {
+  return require('./templates')()
+    .then(function (templates) {
       return {
         email: new Mailer(translator, templates, config.mail, sender),
         sms: createSms(log, translator, templates, config.sms)
       }
-    }
-  )
+    })
 }

lib/senders/sms.js

@@ -97,7 +97,7 @@ module.exports = function (log, translator, templates, smsConfig) {
 
     return template({
       link: smsConfig[templateName + 'Link'],
-      translator: translator(acceptLanguage)
+      translator: translator.getTranslator(acceptLanguage)
     }).text
   }
 }

lib/senders/translator.js

@@ -43,11 +43,18 @@ module.exports = function (locales, defaultLanguage) {
         languageTranslations[language] = translator
       }
 
-      return function translator(acceptLanguage) {
-        var languages = i18n.parseAcceptLanguage(acceptLanguage)
-        var best = i18n.normalizeLanguage(i18n.bestLanguage(languages, supportedLanguages, defaultLanguage))
+      return {
+        getTranslator: function (acceptLanguage) {
+          return languageTranslations[getLocale(acceptLanguage)]
+        },
+
+        getLocale: getLocale
+      }
 
-        return languageTranslations[best]
+      function getLocale (acceptLanguage) {
+        var languages = i18n.parseAcceptLanguage(acceptLanguage)
+        var bestLanguage = i18n.bestLanguage(languages, supportedLanguages, defaultLanguage)
+        return i18n.normalizeLanguage(bestLanguage)
       }
     }
   )

lib/server.js

@@ -43,7 +43,7 @@ function logEndpointErrors(response, log) {
   }
 }
 
-function create(log, error, config, routes, db) {
+function create(log, error, config, routes, db, translator) {
 
   // Hawk needs to calculate request signatures based on public URL,
   // not the local URL to which it is bound.
@@ -284,7 +284,11 @@ function create(log, error, config, routes, db) {
       request.app.remoteAddressChain = xff
       request.app.clientAddress = xff[clientAddressIndex]
 
-      request.app.acceptLanguage = trimLocale(request.headers['accept-language'])
+      const acceptLanguage = trimLocale(request.headers['accept-language'])
+      request.app.acceptLanguage = acceptLanguage
+      const locale = translator.getLocale(acceptLanguage)
+      request.app.locale = locale
+      request.app.isLocaleAcceptable = isLocaleAcceptable(locale, acceptLanguage)
 
       if (request.headers.authorization) {
         // Log some helpful details for debugging authentication problems.
@@ -341,6 +345,10 @@ function create(log, error, config, routes, db) {
   return server
 }
 
+function isLocaleAcceptable (locale, acceptLanguage) {
+  return RegExp(`^(?:.+, *)*${locale}(?:[,-].+)*$`, 'i').test(acceptLanguage)
+}
+
 module.exports = {
   create: create,
   // Functions below exported for testing

scripts/sms/balance.js

@@ -15,7 +15,10 @@ if (config.sms.apiKey === NOT_SET || config.sms.apiSecret === NOT_SET) {
 
 const log = require('../../lib/log')(config.log.level, 'sms-balance')
 
-require('../../lib/senders')(config, log)
+require('../../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return require('../../lib/senders')(log, config, translator)
+  })
   .then(senders => {
     return senders.sms.balance()
   })

scripts/sms/send.js

@@ -16,7 +16,10 @@ if (config.sms.apiKey === NOT_SET || config.sms.apiSecret === NOT_SET) {
 const args = parseArgs()
 const log = require('../../lib/log')(config.log.level, 'send-sms')
 
-require('../../lib/senders')(config, log)
+require('../../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return require('../../lib/senders')(log, config, translator)
+  })
   .then(senders => {
     return senders.sms.send.apply(null, args)
   })

scripts/write-emails-to-disk.js

@@ -27,7 +27,7 @@
  */
 
 var P = require('bluebird')
-var config = require('../config')
+const config = require('../config').getProperties()
 const createSenders = require('../lib/senders')
 var fs = require('fs')
 const log = require('../lib/senders/legacy_log')(require('../lib/senders/log')('server'))
@@ -52,8 +52,10 @@ var mailSender = {
   close: function () {}
 }
 
-
-createSenders(config.getProperties(), log, mailSender)
+require('../lib/senders/translator')(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+  .then(translator => {
+    return createSenders(log, config, translator, mailSender)
+  })
   .then((senders) => {
     const mailer = senders.email
     checkMessageType(mailer, messageToSend)

test/local/lib/mailer_locales.js

@@ -4,15 +4,20 @@
 
 'use strict'
 
+const ROOT_DIR = '../../..'
+
 const assert = require('insist')
-var config = require('../../../config/index').getProperties()
-var log = {}
+const config = require(`${ROOT_DIR}/config/index`).getProperties()
+const log = {}
 
 describe('mailer locales', () => {
 
   let mailer
   before(() => {
-    return require('../../../lib/senders')(config, log)
+    return require(`${ROOT_DIR}/lib/senders/translator`)(config.i18n.supportedLanguages, config.i18n.defaultLanguage)
+      .then(translator => {
+        return require(`${ROOT_DIR}/lib/senders`)(log, config, translator)
+      })
       .then(result => {
         mailer = result.email
       })

test/local/lib/senders/translator.js

@@ -2,25 +2,37 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+'use strict'
+
 const assert = require('insist')
 
 require('../../../../lib/senders/translator')(['en', 'pt_br', 'DE', 'ES_AR', 'ES_cl'], 'en')
-.then(
-  function (translator) {
-    it(
-      'translator works with upper and lowercase languages',
-      function (t) {
-        var x = translator('PT-br,DE')
-        assert.equal(x.language, 'pt-BR')
-        x = translator('bu-ll,es-ar')
-        assert.equal(x.language, 'es-AR')
-        x = translator('es-CL')
-        assert.equal(x.language, 'es-CL')
-        x = translator('en-US')
-        assert.equal(x.language, 'en')
-        x = translator('db-LB') // a locale that does not exist
-        assert.equal(x.language, 'en')
-      }
-    )
-  }
-)
+.then(translator => {
+  it('returns the correct interface', () => {
+    assert.equal(typeof translator, 'object')
+    assert.equal(Object.keys(translator).length, 2)
+    assert.equal(typeof translator.getTranslator, 'function')
+    assert.equal(typeof translator.getLocale, 'function')
+  })
+
+  it('getTranslator works with upper and lowercase languages', () => {
+    let x = translator.getTranslator('PT-br,DE')
+    assert.equal(x.language, 'pt-BR')
+    x = translator.getTranslator('bu-ll,es-ar')
+    assert.equal(x.language, 'es-AR')
+    x = translator.getTranslator('es-CL')
+    assert.equal(x.language, 'es-CL')
+    x = translator.getTranslator('en-US')
+    assert.equal(x.language, 'en')
+    x = translator.getTranslator('db-LB') // a locale that does not exist
+    assert.equal(x.language, 'en')
+  })
+
+  it('getLocale works with upper and lowercase languages', () => {
+    assert.equal(translator.getLocale('PT-br,DE'), 'pt-BR')
+    assert.equal(translator.getLocale('bu-ll,es-ar'), 'es-AR')
+    assert.equal(translator.getLocale('es-CL'), 'es-CL')
+    assert.equal(translator.getLocale('en-US'), 'en')
+    assert.equal(translator.getLocale('db-LB'), 'en')
+  })
+})