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

philbooth · 2017-01-27T10:21:34Z

This is the refactoring part of #4654, opened as a separate PR because I kind of like how it separated the responsibility for managing the flow model away from the views. It is minus the flow.clear part of that PR, which is no longer needed because of #4676 (if that gets merged).

Instead of the views holding a reference to the flow model and passing it as view data in calls to navigate, flow model access is now gated via the metrics object using events. This eliminates the need to pass the flow model around and makes the view code cleaner (imho).

Not sure how the rest of you feel about it though, so I leave it here for your assessment.

@mozilla/fxa-devs r?

shane-tomlinson

Another great PR @philbooth - all of my comments are nits/suggestions for furthering this line of thought.

shane-tomlinson · 2017-01-27T11:42:54Z

app/scripts/lib/metrics.js

@@ -153,6 +156,46 @@ define(function (require, exports, module) {
      this._clearInactivityFlushTimeout();
    },

+    _initializeFlowEvents (options) {


Can you add JSDocs to the new methods?

We have a view notifier-mixin that might be useful here, if we move the mixin to be usable by non-models too. .

Using that module, you can do something like:

init: function () { // lots of stuff already there .... // used by _initializeFlowModel` this._sentryMetrics = options.sentryMetrics; NotifierMixin.initialize(this); }, notifications: { 'flow.initialize': '_initializeFlowModel', 'flow.event': '_handleFlowEvent' },

Continuing with this line of thought and extending the work you start here, in a separate PR, we could add more notifications to handle logging views and events from views/base.js and possibly even stop passing views a direct reference to metrics. I could see this like:

notifications: { 'event': '_logEvent', 'flow.initialize': '_initializeFlowModel', 'flow.event': '_handleFlowEvent', 'view.displayed': '_logView', 'view.event': '_logViewEvent' },

shane-tomlinson · 2017-01-27T12:08:49Z

app/scripts/lib/metrics.js

@@ -503,23 +547,6 @@ define(function (require, exports, module) {
      return this._isSampledUser;
    },

-    logFlowBegin (flowId, flowBeginTime) {


shane-tomlinson · 2017-01-27T12:13:16Z

app/scripts/lib/metrics.js

+      const notifier = options.notifier;
+
+      if (notifier) {
+        notifier.on('flow.initialize', () => this._initializeFlowModel(options.sentryMetrics));


I find passing options.sentryMetrics a bit strange since _initializeFlowModel already accesses this._flowModel and this._window. Seems like it'd be easier to set this._sentryMetrics in the initializer then reference it within _initializeFlowModel

Just to explain the perceived strangeness, it was done at the same time I raised #4639. I'm anti-coupling generally and sentryMetrics is something I plan to replace with events. In this case it is used once during the initialisation of the object and then never referred to again. I explicitly didn't want to hold a reference lest it encourage further usage/coupling down the line.

But I'm not wedded to it, I'll change it to how you want.

shane-tomlinson · 2017-01-27T12:17:01Z

app/scripts/lib/metrics.js

-    logFlowEventOnce (eventName, viewName) {
-      this.logEventOnce(marshallFlowEvent(eventName, viewName));
-    },
-
    getFlowEventMetadata () {
      const metadata = (this._flowModel && this._flowModel.attributes) || {};


I realize this was already here, but it seems like this could be simplified to:

return this._flowModel && this._flowModel.pick('flowBegin', 'flowId');

And then where the data is consumed, you'd translate flowBegin to flowBeginTime

This method is public and that's not the only place it's called from. I just wanted to re-use the existing code when I call it further up.

shane-tomlinson · 2017-01-27T12:21:57Z

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

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


shane-tomlinson · 2017-01-27T12:22:54Z

app/tests/spec/views/force_auth.js

+      metrics = new Metrics({
+        notifier,
+        sentryMetrics: {
+          captureException () {}


Are exceptions being captured?

Not sure I understand the question.

I introduced it in these tests because the metrics object tries to parse the DOM attributes and calls captureException if it can't. So I could also have fixed the failing tests by adding appropriate DOM attributes, but defining sentryMetrics seemed easier.

shane-tomlinson · 2017-01-27T12:25:32Z

app/scripts/lib/app-start.js

@@ -214,7 +216,6 @@ define(function (require, exports, module) {
        const iframeChannel = new IframeChannel();

        iframeChannel.initialize({
-          metrics: this._metrics,


Was this just never used?

Yep. I only noticed because at first I thought I'd created a circular set of references in the initialisation code. But then when I traced it through I found metrics wasn't used by the channels so problem solved.

vbudhram · 2017-01-27T16:52:35Z

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

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


@philbooth Not really in the scope of this PR, but was trying to figure out the motivation for having flow-begin-mixin and if it was still relevant or if it could be combined with flow-events-mixin? Traced back to here but not sure if it still makes sense.

We still need them both.

flow-begin-mixin is mixed in to the views where we want to emit a flow.begin event. So sign-in, sign-up and force-auth, essentially. All that mixin does is emit the event, although it also needs flow-events-mixin in order to work.

flow-events-mixin just handles initialisation of the flow model. This is needed for the above, but it's also needed for other views, e.g. during password reset or sign-in confirmation, where we want to initialise the flow model from a resume token or after navigation has occurred. Mixing it in to those views means that flow events continue to be emitted with the correct flow id, but without a fresh flow.begin event being emitted.

Although also, I realise now, that navigation argument becomes moot with this change. But the resume token one doesn't, we still need it for that.

@philbooth Gotcha, thanks for clearing up!

philbooth · 2017-01-30T10:44:16Z

I'm going to kill this one for now. I might come back to it after the SMS install link work is finished, but that seems more important to work on at the moment.

I did make a start on the requested change to modify the notifier view mixin and use that to listen for events in lib/metrics, but that rippled through the tests quite a long way and unpicking them seemed like it was going to take a while.

I'll open an issue to point to this PR so I don't forget about it.

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

2eb432c

philbooth added the waffle:review label Jan 27, 2017

philbooth added this to the FxA-0: quality milestone Jan 27, 2017

philbooth self-assigned this Jan 27, 2017

shane-tomlinson suggested changes Jan 27, 2017

View reviewed changes

vbudhram reviewed Jan 27, 2017

View reviewed changes

Merge branch 'master' into phil/refactor-flow-model

487c821

philbooth added waffle:active and removed waffle:review labels Jan 29, 2017

philbooth closed this Jan 30, 2017

philbooth removed the waffle:active label Jan 30, 2017

philbooth deleted the phil/refactor-flow-model branch January 30, 2017 10:44

philbooth mentioned this pull request Jan 30, 2017

Refactor the responsibility for maintaining the flow model #4686

Closed

philbooth mentioned this pull request Feb 13, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

philbooth commented Jan 27, 2017

shane-tomlinson left a comment

shane-tomlinson Jan 27, 2017

shane-tomlinson Jan 27, 2017

shane-tomlinson Jan 27, 2017

philbooth Jan 27, 2017 •

edited

shane-tomlinson Jan 27, 2017

philbooth Jan 27, 2017

shane-tomlinson Jan 27, 2017

shane-tomlinson Jan 27, 2017

philbooth Jan 27, 2017

shane-tomlinson Jan 27, 2017

philbooth Jan 27, 2017

vbudhram Jan 27, 2017

philbooth Jan 27, 2017

philbooth Jan 27, 2017

vbudhram Jan 27, 2017

philbooth commented Jan 30, 2017

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

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

Conversation

philbooth commented Jan 27, 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

philbooth Jan 27, 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

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 commented Jan 30, 2017

philbooth Jan 27, 2017 •

edited