fix(metrics): include template name in sms.sent event

#1843

r=shane-tomlinson

docs/metrics-events.md

@@ -264,6 +264,14 @@ to each of the table names mentioned above:
 
 ## Significant changes
 
+### Train 86
+
+* [The template name
+  in the `sms.${templateName}.sent` event
+  was fixed.
+  Previously the message id
+  was logged in its place](https://github.com/mozilla/fxa-auth-server/pull/1843).
+
 ### Train 84
 
 * [The `sms.region.${region}` event

lib/routes/sms.js

@@ -11,6 +11,9 @@ const PhoneNumberUtil = require('google-libphonenumber').PhoneNumberUtil
 const validators = require('./validators')
 
 const METRICS_CONTEXT_SCHEMA = require('../metrics/context').schema
+const TEMPLATE_NAMES = new Map([
+  [ 1, 'installFirefox' ]
+])
 
 module.exports = (log, config, customs, sms) => {
   if (! config.sms.enabled) {
@@ -44,7 +47,7 @@ module.exports = (log, config, customs, sms) => {
 
         const sessionToken = request.auth.credentials
         const phoneNumber = request.payload.phoneNumber
-        const messageId = request.payload.messageId
+        const templateName = TEMPLATE_NAMES.get(request.payload.messageId)
         const acceptLanguage = request.app.acceptLanguage
 
         let phoneNumberUtil, parsedPhoneNumber
@@ -83,11 +86,11 @@ module.exports = (log, config, customs, sms) => {
         }
 
         function sendMessage (senderId) {
-          return sms.send(phoneNumber, senderId, messageId, acceptLanguage)
+          return sms.send(phoneNumber, senderId, templateName, acceptLanguage)
         }
 
         function logSuccess () {
-          return request.emitMetricsEvent(`sms.${messageId}.sent`)
+          return request.emitMetricsEvent(`sms.${templateName}.sent`)
         }
 
         function createResponse () {

lib/senders/sms.js

@@ -9,10 +9,6 @@ var MockNexmo = require('../mock-nexmo')
 var P = require('bluebird')
 var error = require('../error')
 
-var TEMPLATE_NAMES = new Map([
-  [ 1, 'installFirefox' ]
-])
-
 module.exports = function (log, translator, templates, smsConfig) {
   var nexmo = smsConfig.useMock ? new MockNexmo(log, smsConfig.balanceThreshold) : new Nexmo({
     apiKey: smsConfig.apiKey,
@@ -26,17 +22,17 @@ module.exports = function (log, translator, templates, smsConfig) {
   ])
 
   return {
-    send: function (phoneNumber, senderId, messageId, acceptLanguage) {
+    send: function (phoneNumber, senderId, templateName, acceptLanguage) {
       log.trace({
         op: 'sms.send',
         senderId: senderId,
-        messageId: messageId,
+        templateName: templateName,
         acceptLanguage: acceptLanguage
       })
 
       return P.resolve()
         .then(function () {
-          var message = getMessage(messageId, acceptLanguage)
+          var message = getMessage(templateName, acceptLanguage)
 
           return sendSms(senderId, phoneNumber, message.trim())
         })
@@ -58,7 +54,7 @@ module.exports = function (log, translator, templates, smsConfig) {
             log.info({
               op: 'sms.send.success',
               senderId: senderId,
-              messageId: messageId,
+              templateName: templateName,
               acceptLanguage: acceptLanguage
             })
           } else {
@@ -88,12 +84,11 @@ module.exports = function (log, translator, templates, smsConfig) {
     return P.promisify(object[methodName], { context: object })
   }
 
-  function getMessage (messageId, acceptLanguage) {
-    var templateName = TEMPLATE_NAMES.get(messageId)
+  function getMessage (templateName, acceptLanguage) {
     var template = templates['sms.' + templateName]
 
     if (! template) {
-      log.error({ op: 'sms.getMessage.error', messageId: messageId, templateName: templateName })
+      log.error({ op: 'sms.getMessage.error', templateName: templateName })
       throw error.invalidMessageId()
     }
 

test/local/routes/sms.js

@@ -58,7 +58,7 @@ describe('/sms', () => {
       },
       log: log,
       payload: {
-        messageId: 42,
+        messageId: 1,
         metricsContext: {
           flowBeginTime: Date.now(),
           flowId: '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'
@@ -98,7 +98,7 @@ describe('/sms', () => {
         assert.equal(args.length, 4)
         assert.equal(args[0], '+18885083401')
         assert.equal(args[1], '15036789977')
-        assert.equal(args[2], 42)
+        assert.equal(args[2], 'installFirefox')
         assert.equal(args[3], 'en-US')
       })
 
@@ -112,7 +112,7 @@ describe('/sms', () => {
 
         args = log.flowEvent.args[1]
         assert.equal(args.length, 1)
-        assert.equal(args[0].event, 'sms.42.sent')
+        assert.equal(args[0].event, 'sms.installFirefox.sent')
         assert.equal(args[0].flow_id, request.payload.metricsContext.flowId)
       })
     })

test/local/senders/sms.js

@@ -91,7 +91,7 @@ describe('lib/senders/sms:', () => {
   })
 
   it('sends a valid sms', () => {
-    return sms.send('+442078553000', 'Firefox', 1, 'en')
+    return sms.send('+442078553000', 'Firefox', 'installFirefox', 'en')
       .then(() => {
         assert.equal(sendSms.callCount, 1, 'nexmo.message.sendSms was called once')
         const args = sendSms.args[0]
@@ -106,7 +106,7 @@ describe('lib/senders/sms:', () => {
         assert.deepEqual(log.trace.args[0][0], {
           op: 'sms.send',
           senderId: 'Firefox',
-          messageId: 1,
+          templateName: 'installFirefox',
           acceptLanguage: 'en'
         }, 'log.trace was passed the correct data')
 
@@ -115,7 +115,7 @@ describe('lib/senders/sms:', () => {
         assert.deepEqual(log.info.args[0][0], {
           op: 'sms.send.success',
           senderId: 'Firefox',
-          messageId: 1,
+          templateName: 'installFirefox',
           acceptLanguage: 'en'
         }, 'log.info was passed the correct data')
 
@@ -124,8 +124,8 @@ describe('lib/senders/sms:', () => {
       })
   })
 
-  it('fails to send an sms with an invalid message id', () => {
-    return sms.send('+442078553000', 'Firefox', 2, 'en')
+  it('fails to send an sms with an invalid template name', () => {
+    return sms.send('+442078553000', 'Firefox', 'wibble', 'en')
       .then(() => assert.fail('sms.send should have rejected'))
       .catch(error => {
         assert.equal(error.errno, 131, 'error.errno was set correctly')
@@ -137,8 +137,7 @@ describe('lib/senders/sms:', () => {
         assert.equal(log.error.callCount, 1, 'log.error was called once')
         assert.deepEqual(log.error.args[0][0], {
           op: 'sms.getMessage.error',
-          messageId: 2,
-          templateName: undefined
+          templateName: 'wibble'
         }, 'log.error was passed the correct data')
 
         assert.equal(sendSms.callCount, 0, 'nexmo.message.sendSms was not called')
@@ -147,7 +146,7 @@ describe('lib/senders/sms:', () => {
 
   it('fails to send an sms that is throttled by the network provider', () => {
     nexmoStatus = '1'
-    return sms.send('+442078553000', 'Firefox', 1, 'en')
+    return sms.send('+442078553000', 'Firefox', 'installFirefox', 'en')
       .then(() => assert.fail('sms.send should have rejected'))
       .catch(error => {
         assert.equal(error.errno, 114, 'error.errno was set correctly')
@@ -162,7 +161,7 @@ describe('lib/senders/sms:', () => {
 
   it('fails to send an sms that is rejected by the network provider', () => {
     nexmoStatus = '2'
-    return sms.send('+442078553000', 'Firefox', 1, 'en')
+    return sms.send('+442078553000', 'Firefox', 'installFirefox', 'en')
       .then(() => assert.fail('sms.send should have rejected'))
       .catch(error => {
         assert.equal(error.errno, 132, 'error.errno was set correctly')