feat(metrics): metrics flow for iframeless flow #6227
Conversation
ghost
assigned 
|
Easier to view via https://github.com/mozilla/fxa-content-server/pull/6227/files?w=1 |
Looks perfect to me! |
|
|
||
| module.exports = function (config) { | ||
| const FLOW_ID_KEY = config.get('flow_id_key'); | ||
| const FLOW_EVENT_NAME = 'flow.begin'; |
There was a problem hiding this comment.
@philbooth what flow event name should we use here?
There was a problem hiding this comment.
flow.begin is correct (there's a whole bunch of post-processing that's keyed on it, so it's not really a choice at this point tbh)
There was a problem hiding this comment.
We'll also need to remove app/scripts/views/mixins/flow-begin-mixin.js from the client-side of course. Probably you already realised that, just making sure.
Any places that were mixing it in can mix in flow-events-mixin.js instead now, I think, and everything should just magically work.
There was a problem hiding this comment.
hmmm, We cannot just remove flow-begin from client-side because there are still iframe versions of the flow. So we either not record a begin if there was a query flow sent or some other solution
There was a problem hiding this comment.
Oh good point, lol. Yeah, maybe add some condition to the begin mixin so it doesn't emit the event if the query params are set?
vladikoff
force-pushed
the
i5882-v2
branch
2 times, most recently
from
b31fb0e
to
bd74c53
Compare
|
@philbooth r? added a change to avoid sending |
| createFlow(); | ||
|
|
||
| assert.equal(flow.get('flowId'), QUERY_FLOW_ID); | ||
| assert.equal(flow.get('flowBegin'), QUERY_FLOW_BEGIN); |
There was a problem hiding this comment.
Shouldn't this test also assert that markEventLogged is called correctly? (or if not here then somewhere)
| } else if (this.has('flowBegin')) { | ||
| this.logError(AuthErrors.toMissingResumeTokenPropertyError('flowId')); | ||
| } else if (urlParams.flow_begin_time && urlParams.flow_id) { | ||
| this.set('flowId', urlParams.flow_id); |
There was a problem hiding this comment.
Should flow_begin_time and flow_id be checked for validity? The schema below is even used when populating from the data attributes.
Fixes #5882
@philbooth feedback?