Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Commit

Permalink
fix(metrics): ensure email sent amplitude events include device id
Browse files Browse the repository at this point in the history
  • Loading branch information
@philbooth
philbooth committed Nov 29, 2018
1 parent 85c278a commit 03a2f2e
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/email/utils/helpers.js
View file
Expand Up @@ -124,14 +124,14 @@ function logAmplitudeEvent (log, message, eventInfo) {
query: {}, query: {},
payload: {} payload: {}
}, { }, {
device_id: message.deviceId || getHeaderValue('X-Device-Id', message),
email_domain: eventInfo.domain, email_domain: eventInfo.domain,
email_sender: message.emailSender || getHeaderValue('X-Email-Sender', message), email_sender: message.emailSender || getHeaderValue('X-Email-Sender', message),
email_service: message.emailService || getHeaderValue('X-Email-Service', message), email_service: message.emailService || getHeaderValue('X-Email-Service', message),
service: message.service || getHeaderValue('X-Service-Id', message), service: message.service || getHeaderValue('X-Service-Id', message),
templateVersion: eventInfo.templateVersion, templateVersion: eventInfo.templateVersion,
uid: message.uid || getHeaderValue('X-Uid', message) uid: message.uid || getHeaderValue('X-Uid', message)
}, { }, {
device_id: message.deviceId || getHeaderValue('X-Device-Id', message),
flowBeginTime: message.flowBeginTime || getHeaderValue('X-Flow-Begin-Time', message), flowBeginTime: message.flowBeginTime || getHeaderValue('X-Flow-Begin-Time', message),
flow_id: eventInfo.flow_id, flow_id: eventInfo.flow_id,
time: Date.now() time: Date.now()
Expand Down
6 changes: 4 additions & 2 deletions lib/routes/account.js
View file
Expand Up @@ -76,7 +76,7 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push)


request.validateMetricsContext() request.validateMetricsContext()


const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


return customs.check(request, email, 'accountCreate') return customs.check(request, email, 'accountCreate')
.then(deleteAccountIfUnverified) .then(deleteAccountIfUnverified)
Expand Down Expand Up @@ -272,6 +272,7 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push)
redirectTo: form.redirectTo, redirectTo: form.redirectTo,
resume: form.resume, resume: form.resume,
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down Expand Up @@ -677,14 +678,15 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push)
const geoData = request.app.geo const geoData = request.app.geo
const service = request.payload.service || request.query.service const service = request.payload.service || request.query.service
const ip = request.app.clientAddress const ip = request.app.clientAddress
const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


try { try {
await mailer.sendNewDeviceLoginNotification( await mailer.sendNewDeviceLoginNotification(
accountRecord.emails, accountRecord.emails,
accountRecord, accountRecord,
{ {
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down
9 changes: 6 additions & 3 deletions lib/routes/password.js
View file
Expand Up @@ -441,7 +441,7 @@ module.exports = function (
} }
request.setMetricsFlowCompleteSignal(flowCompleteSignal) request.setMetricsFlowCompleteSignal(flowCompleteSignal)


const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


let passwordForgotToken let passwordForgotToken


Expand Down Expand Up @@ -483,6 +483,7 @@ module.exports = function (
redirectTo: request.payload.redirectTo, redirectTo: request.payload.redirectTo,
resume: request.payload.resume, resume: request.payload.resume,
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down Expand Up @@ -538,7 +539,7 @@ module.exports = function (
var service = request.payload.service || request.query.service var service = request.payload.service || request.query.service
const ip = request.app.clientAddress const ip = request.app.clientAddress


const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


return P.all([ return P.all([
request.emitMetricsEvent('password.forgot.resend_code.start'), request.emitMetricsEvent('password.forgot.resend_code.start'),
Expand All @@ -564,6 +565,7 @@ module.exports = function (
redirectTo: request.payload.redirectTo, redirectTo: request.payload.redirectTo,
resume: request.payload.resume, resume: request.payload.resume,
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down Expand Up @@ -621,7 +623,7 @@ module.exports = function (
var code = request.payload.code var code = request.payload.code
const accountResetWithRecoveryKey = request.payload.accountResetWithRecoveryKey const accountResetWithRecoveryKey = request.payload.accountResetWithRecoveryKey


const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


let accountResetToken let accountResetToken


Expand Down Expand Up @@ -663,6 +665,7 @@ module.exports = function (
{ {
code, code,
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
uid: passwordForgotToken.uid uid: passwordForgotToken.uid
Expand Down
5 changes: 4 additions & 1 deletion lib/routes/utils/signin.js
View file
Expand Up @@ -182,7 +182,7 @@ module.exports = (log, config, customs, db, mailer) => {


let sessions let sessions


const { flowId, flowBeginTime } = await request.app.metricsContext const { deviceId, flowId, flowBeginTime } = await request.app.metricsContext


const mustVerifySession = sessionToken.mustVerify && ! sessionToken.tokenVerified const mustVerifySession = sessionToken.mustVerify && ! sessionToken.tokenVerified


Expand Down Expand Up @@ -278,6 +278,7 @@ module.exports = (log, config, customs, db, mailer) => {
redirectTo, redirectTo,
resume, resume,
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down Expand Up @@ -330,6 +331,7 @@ module.exports = (log, config, customs, db, mailer) => {
{ {
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
code: sessionToken.tokenVerificationId, code: sessionToken.tokenVerificationId,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down Expand Up @@ -367,6 +369,7 @@ module.exports = (log, config, customs, db, mailer) => {
{ {
acceptLanguage: request.app.acceptLanguage, acceptLanguage: request.app.acceptLanguage,
code: sessionToken.tokenVerificationCode, code: sessionToken.tokenVerificationCode,
deviceId,
flowId, flowId,
flowBeginTime, flowBeginTime,
ip, ip,
Expand Down
4 changes: 2 additions & 2 deletions test/local/email/utils.js
View file
Expand Up @@ -116,14 +116,14 @@ describe('email utils helpers', () => {
payload: {} payload: {}
}) })
assert.deepEqual(args[2], { assert.deepEqual(args[2], {
device_id: 'bbb',
email_domain: 'other', email_domain: 'other',
email_service: 'fxa-email-service', email_service: 'fxa-email-service',
email_sender: 'ses', email_sender: 'ses',
service: 'ddd', service: 'ddd',
templateVersion: 'eee', templateVersion: 'eee',
uid: 'fff' uid: 'fff'
}) })
assert.equal(args[3].device_id, 'bbb')
assert.equal(args[3].flow_id, 'ccc') assert.equal(args[3].flow_id, 'ccc')
assert.equal(args[3].flowBeginTime, 42) assert.equal(args[3].flowBeginTime, 42)
assert.ok(args[3].time > Date.now() - 1000) assert.ok(args[3].time > Date.now() - 1000)
Expand Down Expand Up @@ -164,14 +164,14 @@ describe('email utils helpers', () => {
payload: {} payload: {}
}) })
assert.deepEqual(args[2], { assert.deepEqual(args[2], {
device_id: 'b',
email_domain: 'gmail', email_domain: 'gmail',
email_sender: 'wibble', email_sender: 'wibble',
email_service: 'fxa-auth-server', email_service: 'fxa-auth-server',
service: 'd', service: 'd',
templateVersion: 42, templateVersion: 42,
uid: 'e' uid: 'e'
}) })
assert.equal(args[3].device_id, 'b')
assert.equal(args[3].flow_id, 'c') assert.equal(args[3].flow_id, 'c')
assert.equal(args[3].flowBeginTime, 1) assert.equal(args[3].flowBeginTime, 1)
}) })
Expand Down
7 changes: 7 additions & 0 deletions test/local/routes/account.js
View file
Expand Up @@ -299,6 +299,7 @@ describe('/account/create', () => {
authPW: hexString(32), authPW: hexString(32),
service: 'sync', service: 'sync',
metricsContext: { metricsContext: {
deviceId: 'wibble',
flowBeginTime: Date.now(), flowBeginTime: Date.now(),
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
utmCampaign: 'utm campaign', utmCampaign: 'utm campaign',
Expand Down Expand Up @@ -505,6 +506,7 @@ describe('/account/create', () => {
assert.equal(args[2].uaOS, 'iOS') assert.equal(args[2].uaOS, 'iOS')
assert.equal(args[2].uaOSVersion, '11') assert.equal(args[2].uaOSVersion, '11')
assert.strictEqual(args[2].uaDeviceType, 'tablet') assert.strictEqual(args[2].uaDeviceType, 'tablet')
assert.equal(args[2].deviceId, mockRequest.payload.metricsContext.deviceId)
assert.equal(args[2].flowId, mockRequest.payload.metricsContext.flowId) assert.equal(args[2].flowId, mockRequest.payload.metricsContext.flowId)
assert.equal(args[2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime) assert.equal(args[2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime)
assert.equal(args[2].service, 'sync') assert.equal(args[2].service, 'sync')
Expand Down Expand Up @@ -584,6 +586,7 @@ describe('/account/login', function () {
service: 'sync', service: 'sync',
reason: 'signin', reason: 'signin',
metricsContext: { metricsContext: {
deviceId: 'blee',
flowBeginTime: Date.now(), flowBeginTime: Date.now(),
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
utmCampaign: 'utm campaign', utmCampaign: 'utm campaign',
Expand Down Expand Up @@ -611,6 +614,7 @@ describe('/account/login', function () {
service: 'dcdb5ae7add825d2', service: 'dcdb5ae7add825d2',
reason: 'signin', reason: 'signin',
metricsContext: { metricsContext: {
deviceId: 'blee',
flowBeginTime: Date.now(), flowBeginTime: Date.now(),
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
service: 'dcdb5ae7add825d2' service: 'dcdb5ae7add825d2'
Expand All @@ -627,6 +631,7 @@ describe('/account/login', function () {
service: 'dcdb5ae7add825d2', service: 'dcdb5ae7add825d2',
reason: 'signin', reason: 'signin',
metricsContext: { metricsContext: {
deviceId: 'blee',
flowBeginTime: Date.now(), flowBeginTime: Date.now(),
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103' flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103'
} }
Expand Down Expand Up @@ -800,6 +805,7 @@ describe('/account/login', function () {
assert.equal(args[2].uaOS, 'Android') assert.equal(args[2].uaOS, 'Android')
assert.equal(args[2].uaOSVersion, '6') assert.equal(args[2].uaOSVersion, '6')
assert.equal(args[2].uaDeviceType, 'mobile') assert.equal(args[2].uaDeviceType, 'mobile')
assert.equal(args[2].deviceId, mockRequest.payload.metricsContext.deviceId)
assert.equal(args[2].flowId, mockRequest.payload.metricsContext.flowId) assert.equal(args[2].flowId, mockRequest.payload.metricsContext.flowId)
assert.equal(args[2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime) assert.equal(args[2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime)
assert.equal(args[2].service, 'sync') assert.equal(args[2].service, 'sync')
Expand Down Expand Up @@ -1058,6 +1064,7 @@ describe('/account/login', function () {
assert.equal(tokenData.tokenVerificationId, null, 'sessionToken was created verified') assert.equal(tokenData.tokenVerificationId, null, 'sessionToken was created verified')
assert.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyLoginEmail was not called') assert.equal(mockMailer.sendVerifyCode.callCount, 0, 'mailer.sendVerifyLoginEmail was not called')
assert.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 1, 'mailer.sendNewDeviceLoginNotification was called') assert.equal(mockMailer.sendNewDeviceLoginNotification.callCount, 1, 'mailer.sendNewDeviceLoginNotification was called')
assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].deviceId, mockRequest.payload.metricsContext.deviceId)
assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].flowId, mockRequest.payload.metricsContext.flowId) assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].flowId, mockRequest.payload.metricsContext.flowId)
assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime) assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].flowBeginTime, mockRequest.payload.metricsContext.flowBeginTime)
assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].service, 'sync') assert.equal(mockMailer.sendNewDeviceLoginNotification.args[0][2].service, 'sync')
Expand Down
6 changes: 6 additions & 0 deletions test/local/routes/password.js
View file
Expand Up @@ -87,6 +87,7 @@ describe('/password', () => {
payload: { payload: {
email: TEST_EMAIL, email: TEST_EMAIL,
metricsContext: { metricsContext: {
deviceId: 'wibble',
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
flowBeginTime: Date.now() - 1 flowBeginTime: Date.now() - 1
} }
Expand Down Expand Up @@ -127,6 +128,7 @@ describe('/password', () => {
assert.equal(args[2].location.country, 'United States') assert.equal(args[2].location.country, 'United States')
assert.equal(args[2].timeZone, 'America/Los_Angeles') assert.equal(args[2].timeZone, 'America/Los_Angeles')
assert.equal(args[2].uid, uid) assert.equal(args[2].uid, uid)
assert.equal(args[2].deviceId, 'wibble')
}) })
}) })


Expand Down Expand Up @@ -172,6 +174,7 @@ describe('/password', () => {
payload: { payload: {
email: TEST_EMAIL, email: TEST_EMAIL,
metricsContext: { metricsContext: {
deviceId: 'wibble',
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
flowBeginTime: Date.now() - 1 flowBeginTime: Date.now() - 1
} }
Expand All @@ -182,6 +185,7 @@ describe('/password', () => {
.then(function(response) { .then(function(response) {
assert.equal(mockMailer.sendRecoveryCode.callCount, 1, 'mailer.sendRecoveryCode was called once') assert.equal(mockMailer.sendRecoveryCode.callCount, 1, 'mailer.sendRecoveryCode was called once')
assert.equal(mockMailer.sendRecoveryCode.args[0][2].uid, uid) assert.equal(mockMailer.sendRecoveryCode.args[0][2].uid, uid)
assert.equal(mockMailer.sendRecoveryCode.args[0][2].deviceId, 'wibble')


assert.equal(mockRequest.validateMetricsContext.callCount, 0) assert.equal(mockRequest.validateMetricsContext.callCount, 0)
assert.equal(mockLog.flowEvent.callCount, 2, 'log.flowEvent was called twice') assert.equal(mockLog.flowEvent.callCount, 2, 'log.flowEvent was called twice')
Expand Down Expand Up @@ -242,6 +246,7 @@ describe('/password', () => {
payload: { payload: {
code: 'abcdef', code: 'abcdef',
metricsContext: { metricsContext: {
deviceId: 'wibble',
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
flowBeginTime: Date.now() - 1 flowBeginTime: Date.now() - 1
} }
Expand Down Expand Up @@ -275,6 +280,7 @@ describe('/password', () => {


assert.equal(mockMailer.sendPasswordResetNotification.callCount, 1, 'mailer.sendPasswordResetNotification was called once') assert.equal(mockMailer.sendPasswordResetNotification.callCount, 1, 'mailer.sendPasswordResetNotification was called once')
assert.equal(mockMailer.sendPasswordResetNotification.args[0][2].uid, uid, 'mailer.sendPasswordResetNotification was passed uid') assert.equal(mockMailer.sendPasswordResetNotification.args[0][2].uid, uid, 'mailer.sendPasswordResetNotification was passed uid')
assert.equal(mockMailer.sendPasswordResetNotification.args[0][2].deviceId, 'wibble')
}) })
} }
) )
Expand Down
5 changes: 5 additions & 0 deletions test/local/routes/utils/signin.js
View file
Expand Up @@ -451,6 +451,7 @@ describe('sendSigninNotifications', () => {
payload: { payload: {
service: 'testservice', service: 'testservice',
metricsContext: { metricsContext: {
deviceId: 'wibble',
flowBeginTime: Date.now(), flowBeginTime: Date.now(),
flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103', flowId: 'F1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF1031DF103',
utmCampaign: 'utm campaign', utmCampaign: 'utm campaign',
Expand Down Expand Up @@ -543,6 +544,7 @@ describe('sendSigninNotifications', () => {
assert.calledWithExactly(mailer.sendVerifyCode, [], accountRecord, { assert.calledWithExactly(mailer.sendVerifyCode, [], accountRecord, {
acceptLanguage: 'en-US', acceptLanguage: 'en-US',
code: 'emailVerifyCode', code: 'emailVerifyCode',
deviceId: request.payload.metricsContext.deviceId,
flowBeginTime: request.payload.metricsContext.flowBeginTime, flowBeginTime: request.payload.metricsContext.flowBeginTime,
flowId: request.payload.metricsContext.flowId, flowId: request.payload.metricsContext.flowId,
ip: CLIENT_ADDRESS, ip: CLIENT_ADDRESS,
Expand Down Expand Up @@ -590,6 +592,7 @@ describe('sendSigninNotifications', () => {
assert.calledWithExactly(mailer.sendVerifyCode, [], accountRecord, { assert.calledWithExactly(mailer.sendVerifyCode, [], accountRecord, {
acceptLanguage: 'en-US', acceptLanguage: 'en-US',
code: 'tokenVerifyCode', // the token verification code is used if available code: 'tokenVerifyCode', // the token verification code is used if available
deviceId: request.payload.metricsContext.deviceId,
flowBeginTime: request.payload.metricsContext.flowBeginTime, flowBeginTime: request.payload.metricsContext.flowBeginTime,
flowId: request.payload.metricsContext.flowId, flowId: request.payload.metricsContext.flowId,
ip: CLIENT_ADDRESS, ip: CLIENT_ADDRESS,
Expand Down Expand Up @@ -673,6 +676,7 @@ describe('sendSigninNotifications', () => {
assert.calledWithExactly(mailer.sendVerifyLoginEmail, accountRecord.emails, accountRecord, { assert.calledWithExactly(mailer.sendVerifyLoginEmail, accountRecord.emails, accountRecord, {
acceptLanguage: 'en-US', acceptLanguage: 'en-US',
code: 'tokenVerifyCode', code: 'tokenVerifyCode',
deviceId: request.payload.metricsContext.deviceId,
flowBeginTime: request.payload.metricsContext.flowBeginTime, flowBeginTime: request.payload.metricsContext.flowBeginTime,
flowId: request.payload.metricsContext.flowId, flowId: request.payload.metricsContext.flowId,
ip: CLIENT_ADDRESS, ip: CLIENT_ADDRESS,
Expand Down Expand Up @@ -721,6 +725,7 @@ describe('sendSigninNotifications', () => {
assert.calledWithExactly(mailer.sendVerifyLoginCodeEmail, accountRecord.emails, accountRecord, { assert.calledWithExactly(mailer.sendVerifyLoginCodeEmail, accountRecord.emails, accountRecord, {
acceptLanguage: 'en-US', acceptLanguage: 'en-US',
code: 'tokenVerifyShortCode', code: 'tokenVerifyShortCode',
deviceId: request.payload.metricsContext.deviceId,
flowBeginTime: request.payload.metricsContext.flowBeginTime, flowBeginTime: request.payload.metricsContext.flowBeginTime,
flowId: request.payload.metricsContext.flowId, flowId: request.payload.metricsContext.flowId,
ip: CLIENT_ADDRESS, ip: CLIENT_ADDRESS,
Expand Down

0 comments on commit 03a2f2e

Please sign in to comment.