refactor(client): move flow model resposibilities out of the views #4718

philbooth · 2017-02-13T15:30:51Z

What's this?

A refactoring PR that includes work from the abandoned #4681 and tries to address the outstanding feedback from that PR.

What does it do?

It decouples the flow model from the views, instead moving responsibility for its management into the metrics object and leaning on events to communicate with it.

What outstanding feedback has been addressed?

In #4681 (comment), @shane-tomlinson said the metrics object should use the notifier view mixin instead of manually registering the event handlers for flow.initialize and flow.event itself.

Why didn't you just do that in the original PR?

Partly because of PR-fatigue and partly because I wasn't sure how to extract the view mixin for wider re-use away from the views.

And how did you extract that view mixin?

I moved it to lib/channels/notifier-mixin.js, so that it lives next to the notifier object on which it operates. Apart from that it's unchanged.

How come so many other files changed?

It turns out a lot of unit tests were creating the metrics object, and all those references had to be updated to include the notifier because the notifier mixin expects it to be present. The assertions themselves haven't changed.

Why are there two commits?

Because I wasn't sure about the mixin change and that is responsible for all the unit test changes. I wanted to be able to revert all those easily if it turns out I did it wrong. If/when it comes to merging this, the two commits can be squashed, no probs.

@mozilla/fxa-devs r?

shane-tomlinson · 2017-02-13T15:35:06Z

Assigning this to me, because I mention wanting to do this over in this PR that I opened today and forgot @philbooth was already on the case.

shane-tomlinson

This is moving in the right direction! No problems that I can see, just questions.

shane-tomlinson · 2017-02-14T14:04:08Z

app/scripts/lib/metrics.js

+     */
+    _logFlowEvent (data) {
+      if (! this._flowModel) {
+        // If there is no flow model, we're not in a recognised flow


How would "not being in a recognised flow" happen?

This condition dates from pre-#4676 actually, so the comment should probably be slightly different now. I think the only case now is if the flow model hasn't initialised for some reason (so the DOM attributes are missing). I'll test to make sure my understanding is correct, then update the comment.

Oh, this is also for the case where a user lands on a non-flow-initialising view. Only views that have flow-events-mixin initialise the flow model, which is what stops flow events from suddenly being emitted when a user lands on /settings, say. I've updated the comment to try and make this clearer.

shane-tomlinson · 2017-02-14T14:04:32Z

app/scripts/lib/metrics.js

+     *   @param {Boolean} [data.once]
+     *   If set, emit this event via the `logEventOnce` method.
+     *   Defaults to `false`.
+     */


nit: Can you add a @private JSDoc tag too?

shane-tomlinson · 2017-02-14T14:14:39Z

app/scripts/lib/metrics.js

+     * @param {Object} data
+     * Event data.
+     *
+     *   @param {String} data.event


We haven't used this format of JSDoc (with blank lines between the parameters), but I can see some advantages.

bike shed - I'm not convinced on introducing a new JSDoc format. It's more readable in ways, but also requires thought when scanning.

Okay, I'll revert this bit. Fwiw, I dislike our preferred style because the param name collides with the description text when you're viewing the code. I find it harder to read.

shane-tomlinson · 2017-02-14T14:16:13Z

app/scripts/views/confirm.js

@@ -36,7 +36,6 @@ define(function (require, exports, module) {
      // ephemeral properties like unwrapBKey and keyFetchToken
      // that need to be sent to the browser.
      this._account = this.user.initAccount(this.model.get('account'));
-      this.flow = this.model.get('flow');


Removals like this are awesome, there were times I scratched my head and asked myself "What's this here for?"

shane-tomlinson · 2017-02-14T14:19:40Z

app/scripts/views/mixins/flow-begin-mixin.js

-      const flowBegin = this.flow.get('flowBegin');
-
-      this.metrics.logFlowBegin(flowId, flowBegin);
+      this.logFlowEventOnce('begin');


I was just looking at the function signature of logFlowEventOnce - does it matter that no viewName is passed in? Should the viewName default to this.viewName in logFlowEventOnce and logFlowEvent?

The flow.begin event doesn't interpolate the view name by design, because it makes the funnel queries easier (and faster). It's possible some other flow events in the future may not interpolate either. Anyway, for that reason, viewName is optional.

shane-tomlinson · 2017-02-14T14:24:13Z

app/scripts/views/mixins/signin-mixin.js

-            account: account,
-            flow: this.flow
-          });
+          return this.navigate('confirm_signin', { account });


Just for my own understanding, the flow model is now taken care of by the metrics model, so no need to pass it from flow to flow?

What did the flow model provide to the confirmation screen? I was confused why confirm had a reference to it before, was it for the ResumeTokenMixin?

Yep, it was being propagated between views so that flow events would work everywhere we needed them to. That act of explicitly passing it between views is what led to this refactor. The changes were getting out of hand and, not sure if you remember, I tried to refactor it to the base view but couldn't get that to work. Then BANG! Flash of inspiration, I realised we could lean on events to make the pain go away.

shane-tomlinson · 2017-02-14T14:39:55Z

app/scripts/lib/metrics.js

+      }
+
+      const flowModel = new Flow({
+        sentryMetrics: this._sentryMetrics,


Not for this PR - I wonder if we can remove the _sentryMetrics reference from the Flow model and pass it a notifier instead, which would also mean the metrics model would no longer need a reference to _sentryMetrics. Whenever the Flow model needs to record an error, it could trigger a message on the notifier that makes its way to both the normal metrics module and Sentry. Lots of ✋ 👋 (hand-waving) here, but it seems feasible.

I already opened #4639 for exactly this reason. Didn't want to do it in this PR though.

shane-tomlinson · 2017-02-14T14:43:39Z

app/scripts/views/base.js

-    logFlowEvent (eventName, viewName) {
-      this.metrics.logFlowEvent(eventName, viewName);
+    logFlowEvent (eventName, viewName, data) {
+      this.notifier.trigger('flow.event', _.assign({}, data, {


Not for this PR, but it looks like logView, logEvent, logEventOnce, and logViewEvent could all use the same treatment.

philbooth · 2017-02-15T12:53:03Z

This is moving in the right direction!

A distinct lack of r+ though. Let me know what else is needed to make you happy! 😄

shane-tomlinson

Two minor changes requested, one to rename metrics.init to metrics.initialize, and one to use a consistent JSDoc format.

r+, this is going to allow a lot of further improvements @philbooth.

shane-tomlinson · 2017-02-16T11:20:13Z

app/scripts/lib/metrics.js

@@ -129,6 +133,8 @@ define(function (require, exports, module) {
    this._utmSource = options.utmSource || NOT_REPORTED_VALUE;
    this._utmTerm = options.utmTerm || NOT_REPORTED_VALUE;
    this._xhr = options.xhr || xhr;
+
+    this.initialize(options);


This module already has an init function that is called from app-start. Having both init and initialize is confusing. init is the odd one out compared to the rest of the repo. Going along with the never settle theme, instead of introducing a similar sounding method that's called from a different place, can you rename init to initialize and remove the call to metrics.init() in app-start? Cocktail should take care of resolving the conflicting methods by calling them both.

shane-tomlinson · 2017-02-16T11:24:43Z

app/scripts/lib/metrics.js

+     * @param {Object} data
+     * Event data.
+     *
+     *   @param {String} data.event


bike shed - I'm not convinced on introducing a new JSDoc format. It's more readable in ways, but also requires thought when scanning.

philbooth · 2017-02-16T13:28:12Z

Thanks @shane-tomlinson!

philbooth added the waffle:active label Feb 13, 2017

shane-tomlinson self-assigned this Feb 13, 2017

philbooth added waffle:review and removed waffle:active labels Feb 13, 2017

shane-tomlinson mentioned this pull request Feb 13, 2017

feat(firstrun): Add fx_firstrun_v2 events to support new firstrun flow. #4717

Merged

philbooth changed the title ~~refactor(client): move flow model resposibilities out of the views #4681~~ refactor(client): move flow model resposibilities out of the views Feb 13, 2017

philbooth requested a review from shane-tomlinson February 13, 2017 15:50

shane-tomlinson reviewed Feb 14, 2017

View reviewed changes

shane-tomlinson mentioned this pull request Feb 14, 2017

notifier has concerns all backwards, includes references to iframeChannel, webChannel, and tabChannel. #4724

Closed

philbooth force-pushed the phil/refactor-flow-model branch 2 times, most recently from 2b2d8dc to 85533a6 Compare February 15, 2017 12:08

shane-tomlinson approved these changes Feb 16, 2017

View reviewed changes

philbooth force-pushed the phil/refactor-flow-model branch from 85533a6 to c2df268 Compare February 16, 2017 12:12

refactor(client): move flow model resposibilities out of the views

baefe77

philbooth force-pushed the phil/refactor-flow-model branch from c2df268 to baefe77 Compare February 16, 2017 12:55

philbooth merged commit 4071291 into master Feb 16, 2017

philbooth removed the waffle:review label Feb 16, 2017

philbooth deleted the phil/refactor-flow-model branch February 16, 2017 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(client): move flow model resposibilities out of the views #4718

refactor(client): move flow model resposibilities out of the views #4718

philbooth commented Feb 13, 2017 •

edited

shane-tomlinson commented Feb 13, 2017

shane-tomlinson left a comment

shane-tomlinson Feb 14, 2017

philbooth Feb 14, 2017

philbooth Feb 14, 2017

shane-tomlinson Feb 14, 2017

shane-tomlinson Feb 14, 2017

shane-tomlinson Feb 16, 2017

philbooth Feb 16, 2017

shane-tomlinson Feb 14, 2017

shane-tomlinson Feb 14, 2017

philbooth Feb 14, 2017

shane-tomlinson Feb 14, 2017

philbooth Feb 14, 2017 •

edited

shane-tomlinson Feb 14, 2017

philbooth Feb 14, 2017

shane-tomlinson Feb 14, 2017

philbooth commented Feb 15, 2017

shane-tomlinson left a comment

shane-tomlinson Feb 16, 2017

shane-tomlinson Feb 16, 2017

philbooth commented Feb 16, 2017

refactor(client): move flow model resposibilities out of the views #4718

refactor(client): move flow model resposibilities out of the views #4718

Conversation

philbooth commented Feb 13, 2017 • edited

What's this?

What does it do?

What outstanding feedback has been addressed?

Why didn't you just do that in the original PR?

And how did you extract that view mixin?

How come so many other files changed?

Why are there two commits?

shane-tomlinson commented Feb 13, 2017

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Feb 15, 2017

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth commented Feb 16, 2017

philbooth commented Feb 13, 2017 •

edited

philbooth Feb 14, 2017 •

edited