Permalink
Browse files

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

#4718

r=shane-tomlinson
  • Loading branch information...
1 parent 313e69e commit 4071291774e72f3426190b54c37f3ecb874548f4 @philbooth philbooth committed on GitHub Feb 16, 2017
Showing with 575 additions and 516 deletions.
  1. +5 −5 app/scripts/lib/app-start.js
  2. 0 app/scripts/{views/mixins → lib/channels}/notifier-mixin.js
  3. +78 −25 app/scripts/lib/metrics.js
  4. +9 −1 app/scripts/lib/storage-metrics.js
  5. +1 −1 app/scripts/models/auth_brokers/fx-firstrun-v2.js
  6. +8 −4 app/scripts/views/base.js
  7. +0 −1 app/scripts/views/confirm.js
  8. +1 −4 app/scripts/views/mixins/flow-begin-mixin.js
  9. +1 −6 app/scripts/views/mixins/flow-events-mixin.js
  10. +6 −2 app/scripts/views/mixins/resume-token-mixin.js
  11. +3 −9 app/scripts/views/mixins/signin-mixin.js
  12. +1 −4 app/scripts/views/mixins/signup-mixin.js
  13. +1 −1 app/tests/spec/lib/app-start.js
  14. +2 −2 app/tests/spec/{views/mixins → lib/channels}/notifier-mixin.js
  15. +1 −1 app/tests/spec/lib/experiment.js
  16. +1 −1 app/tests/spec/lib/experiments/base.js
  17. +1 −1 app/tests/spec/lib/experiments/mailcheck.js
  18. +113 −72 app/tests/spec/lib/metrics.js
  19. +1 −1 app/tests/spec/lib/router.js
  20. +0 −1 app/tests/spec/lib/storage-metrics.js
  21. +1 −1 app/tests/spec/models/refresh-observer.js
  22. +10 −1 app/tests/spec/views/base.js
  23. +1 −1 app/tests/spec/views/choose_what_to_sync.js
  24. +10 −14 app/tests/spec/views/complete_reset_password.js
  25. +1 −1 app/tests/spec/views/complete_sign_up.js
  26. +1 −10 app/tests/spec/views/confirm.js
  27. +1 −1 app/tests/spec/views/confirm_reset_password.js
  28. +4 −2 app/tests/spec/views/coppa/coppa-age-input.js
  29. +10 −1 app/tests/spec/views/force_auth.js
  30. +4 −8 app/tests/spec/views/form.js
  31. +3 −3 app/tests/spec/views/marketing_snippet.js
  32. +1 −1 app/tests/spec/views/mixins/account-reset-mixin.js
  33. +1 −1 app/tests/spec/views/mixins/avatar-mixin.js
  34. +1 −1 app/tests/spec/views/mixins/experiment-mixin.js
  35. +4 −1 app/tests/spec/views/mixins/external-links-mixin.js
  36. +6 −13 app/tests/spec/views/mixins/flow-begin-mixin.js
  37. +136 −158 app/tests/spec/views/mixins/flow-events-mixin.js
  38. +4 −1 app/tests/spec/views/mixins/password-mixin.js
  39. +9 −2 app/tests/spec/views/mixins/resume-token-mixin.js
  40. +4 −2 app/tests/spec/views/mixins/settings-panel-mixin.js
  41. +2 −7 app/tests/spec/views/mixins/signin-mixin.js
  42. +2 −6 app/tests/spec/views/mixins/signup-mixin.js
  43. +5 −2 app/tests/spec/views/oauth_sign_in.js
  44. +4 −1 app/tests/spec/views/oauth_sign_up.js
  45. +1 −1 app/tests/spec/views/permissions.js
  46. +49 −49 app/tests/spec/views/reset_password.js
  47. +1 −1 app/tests/spec/views/settings.js
  48. +1 −1 app/tests/spec/views/settings/avatar_camera.js
  49. +1 −1 app/tests/spec/views/settings/avatar_change.js
  50. +1 −1 app/tests/spec/views/settings/avatar_crop.js
  51. +1 −1 app/tests/spec/views/settings/avatar_gravatar.js
  52. +4 −2 app/tests/spec/views/settings/change_password.js
  53. +1 −1 app/tests/spec/views/settings/client_disconnect.js
  54. +1 −1 app/tests/spec/views/settings/clients.js
  55. +8 −6 app/tests/spec/views/settings/communication_preferences.js
  56. +1 −1 app/tests/spec/views/settings/delete_account.js
  57. +1 −1 app/tests/spec/views/settings/display_name.js
  58. +8 −6 app/tests/spec/views/settings/gravatar_permissions.js
  59. +17 −29 app/tests/spec/views/sign_in.js
  60. +1 −1 app/tests/spec/views/sign_in_unblock.js
  61. +18 −30 app/tests/spec/views/sign_up.js
  62. +1 −1 app/tests/spec/views/sub_panels.js
  63. +1 −1 app/tests/test_start.js
@@ -110,15 +110,15 @@ define(function (require, exports, module) {
// both the metrics and router depend on the language
// fetched from config.
.then(_.bind(this.initializeRelier, this))
- // metrics depends on the relier.
- .then(_.bind(this.initializeMetrics, this))
- // iframe channel depends on the relier and metrics
+ // iframe channel depends on the relier.
.then(_.bind(this.initializeIframeChannel, this))
// fxaClient depends on the relier and
// inter tab communication.
.then(_.bind(this.initializeFxaClient, this))
// depends on iframeChannel and interTabChannel
.then(_.bind(this.initializeNotifier, this))
+ // metrics depends on relier and notifier
+ .then(_.bind(this.initializeMetrics, this))
// assertionLibrary depends on fxaClient
.then(_.bind(this.initializeAssertionLibrary, this))
// profileClient depends on fxaClient and assertionLibrary
@@ -195,8 +195,10 @@ define(function (require, exports, module) {
isSampledUser: isSampledUser,
lang: this._config.lang,
migration: relier.get('migration'),
+ notifier: this._notifier,
screenHeight: screenInfo.screenHeight,
screenWidth: screenInfo.screenWidth,
+ sentryMetrics: this._sentryMetrics,
service: relier.get('service'),
uniqueUserId: this._getUniqueUserId(),
utmCampaign: relier.get('utmCampaign'),
@@ -205,7 +207,6 @@ define(function (require, exports, module) {
utmSource: relier.get('utmSource'),
utmTerm: relier.get('utmTerm')
});
- this._metrics.init();
},
initializeIframeChannel () {
@@ -214,7 +215,6 @@ define(function (require, exports, module) {
const iframeChannel = new IframeChannel();
iframeChannel.initialize({
- metrics: this._metrics,
origin: parentOrigin,
window: this._window
});
@@ -19,10 +19,13 @@ define(function (require, exports, module) {
const $ = require('jquery');
const _ = require('underscore');
+ const Cocktail = require('cocktail');
const Constants = require('lib/constants');
const Backbone = require('backbone');
const Duration = require('duration');
const Environment = require('lib/environment');
+ const Flow = require('models/flow');
+ const NotifierMixin = require('lib/channels/notifier-mixin');
const p = require('lib/promise');
const speedTrap = require('speedTrap');
const Strings = require('lib/strings');
@@ -118,6 +121,7 @@ define(function (require, exports, module) {
this._referrer = this._window.document.referrer || NOT_REPORTED_VALUE;
this._screenHeight = options.screenHeight || NOT_REPORTED_VALUE;
this._screenWidth = options.screenWidth || NOT_REPORTED_VALUE;
+ this._sentryMetrics = options.sentryMetrics;
this._service = options.service || NOT_REPORTED_VALUE;
// if navigationTiming is supported, the baseTime will be from
// navigationTiming.navigationStart, otherwise Date.now().
@@ -129,12 +133,14 @@ 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);
}
_.extend(Metrics.prototype, Backbone.Events, {
ALLOWED_FIELDS: ALLOWED_FIELDS,
- init () {
+ initialize () {
this._flush = _.bind(this.flush, this, true);
$(this._window).on('unload', this._flush);
// iOS will not send events once the window is in the background,
@@ -153,6 +159,64 @@ define(function (require, exports, module) {
this._clearInactivityFlushTimeout();
},
+ notifications: {
+ /* eslint-disable sorting/sort-object-props */
+ 'flow.initialize': '_initializeFlowModel',
+ 'flow.event': '_logFlowEvent'
+ /* eslint-enable sorting/sort-object-props */
+ },
+
+ /**
+ * @private
+ * Initialize the flow model. If it's already been initalized, do nothing.
+ * Initialization may fail if the required flow properties can't be found,
+ * either in the DOM or the resume token.
+ */
+ _initializeFlowModel () {
+ if (this._flowModel) {
+ return;
+ }
+
+ const flowModel = new Flow({
+ sentryMetrics: this._sentryMetrics,
+ window: this._window
+ });
+
+ if (flowModel.has('flowId')) {
+ this._flowModel = flowModel;
+ }
+ },
+
+ /**
+ * @private
+ * Log a flow event. If there is no flow model, do nothing.
+ *
+ * @param {Object} data
+ * @param {String} data.event The name of the event.
+ * @param {String} [data.view] The name of the view, to be
+ * interpolated in the event name. If unset, the event is
+ * logged without a view name.
+ * @param {Boolean} [data.once] If set, emit this event via
+ * the `logEventOnce` method. Defaults to `false`.
+ */
+ _logFlowEvent (data) {
+ if (! this._flowModel) {
+ // If there is no flow model, we're not in a recognised flow and
+ // we should not emit the event. This would be the case if a user
+ // lands on `/settings`, for instance. Only views that mixin the
+ // `flow-events-mixin` will initialise the flow model.
+ return;
+ }
+
+ const eventName = marshallFlowEvent(data.event, data.view);
+
+ if (data.once) {
+ this.logEventOnce(eventName);
+ } else {
+ this.logEvent(eventName);
+ }
+ },
+
/**
* Send the collected data to the backend.
*
@@ -240,16 +304,17 @@ define(function (require, exports, module) {
* @returns {Object}
*/
getAllData () {
- var loadData = this._speedTrap.getLoad();
- var unloadData = this._speedTrap.getUnload();
+ const loadData = this._speedTrap.getLoad();
+ const unloadData = this._speedTrap.getUnload();
+ const flowData = this.getFlowEventMetadata();
- var allData = _.extend({}, loadData, unloadData, {
+ const allData = _.extend({}, loadData, unloadData, {
broker: this._brokerType,
context: this._context,
entrypoint: this._entrypoint,
experiments: flattenHashIntoArrayOfObjects(this._activeExperiments),
- flowBeginTime: this._flowBeginTime,
- flowId: this._flowId,
+ flowBeginTime: flowData.flowBeginTime,
+ flowId: flowData.flowId,
flushTime: Date.now(),
isSampledUser: this._isSampledUser,
lang: this._lang,
@@ -503,23 +568,6 @@ define(function (require, exports, module) {
return this._isSampledUser;
},
- logFlowBegin (flowId, flowBeginTime) {
- // Don't emit a new flow.begin event unless flowId has changed.
- if (flowId !== this._flowId) {
- this._flowId = flowId;
- this._flowBeginTime = flowBeginTime;
- this.logFlowEvent('begin');
- }
- },
-
- logFlowEvent (eventName, viewName) {
- this.logEvent(marshallFlowEvent(eventName, viewName));
- },
-
- logFlowEventOnce (eventName, viewName) {
- this.logEventOnce(marshallFlowEvent(eventName, viewName));
- },
-
getFlowEventMetadata () {
const metadata = (this._flowModel && this._flowModel.attributes) || {};
return {
@@ -528,8 +576,8 @@ define(function (require, exports, module) {
};
},
- setFlowModel (flowModel) {
- this._flowModel = flowModel;
+ getFlowModel (flowModel) {
+ return this._flowModel;
},
/**
@@ -542,5 +590,10 @@ define(function (require, exports, module) {
}
});
+ Cocktail.mixin(
+ Metrics,
+ NotifierMixin
+ );
+
module.exports = Metrics;
});
@@ -21,7 +21,15 @@ define(function (require, exports, module) {
// do nothing
}
- _.extend(StorageMetrics.prototype, new Metrics(), {
+ const notifier = {
+ off () {},
+ on () {},
+ trigger () {},
+ triggerAll () {},
+ triggerRemote () {}
+ };
+
+ _.extend(StorageMetrics.prototype, new Metrics({ notifier }), {
_send (data) {
var metrics = storage.get('metrics_all');
@@ -15,7 +15,7 @@ define(function (require, exports, module) {
const _ = require('underscore');
const Constants = require('lib/constants');
const FxFirstrunV1AuthenticationBroker = require('./fx-firstrun-v1');
- const NotifierMixin = require('views/mixins/notifier-mixin');
+ const NotifierMixin = require('lib/channels/notifier-mixin');
var proto = FxFirstrunV1AuthenticationBroker.prototype;
@@ -13,7 +13,7 @@ define(function (require, exports, module) {
const domWriter = require('lib/dom-writer');
const ErrorUtils = require('lib/error-utils');
const ExternalLinksMixin = require('views/mixins/external-links-mixin');
- const NotifierMixin = require('views/mixins/notifier-mixin');
+ const NotifierMixin = require('lib/channels/notifier-mixin');
const NullMetrics = require('lib/null-metrics');
const Logger = require('lib/logger');
const p = require('lib/promise');
@@ -713,9 +713,13 @@ define(function (require, exports, module) {
*
* @param {String} eventName
* @param {String} viewName
+ * @param {Object} data
*/
- logFlowEvent (eventName, viewName) {
- this.metrics.logFlowEvent(eventName, viewName);
+ logFlowEvent (eventName, viewName, data) {
+ this.notifier.trigger('flow.event', _.assign({}, data, {
+ event: eventName,
+ view: viewName
+ }));
},
/**
@@ -725,7 +729,7 @@ define(function (require, exports, module) {
* @param {String} viewName
*/
logFlowEventOnce (eventName, viewName) {
- this.metrics.logFlowEventOnce(eventName, viewName);
+ this.logFlowEvent(eventName, viewName, { once: true });
},
hideError () {
@@ -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');
},
getAccount () {
@@ -10,10 +10,7 @@ define(function (require, exports, module) {
module.exports = {
afterRender () {
- const flowId = this.flow.get('flowId');
- const flowBegin = this.flow.get('flowBegin');
-
- this.metrics.logFlowBegin(flowId, flowBegin);
+ this.logFlowEventOnce('begin');
}
};
});
@@ -8,16 +8,11 @@ define(function (require, exports, module) {
'use strict';
const $ = require('jquery');
- const Flow = require('models/flow');
const KEYS = require('lib/key-codes');
module.exports = {
afterRender () {
- this.flow = new Flow({
- sentryMetrics: this.sentryMetrics,
- window: this.window
- });
- this.metrics.setFlowModel(this.flow);
+ this.notifier.trigger('flow.initialize');
},
events: {
@@ -20,11 +20,15 @@ define(function (require, exports, module) {
*/
getResumeToken (account) {
var accountInfo = account.pickResumeTokenInfo();
- // flow is only available in views that mix in the flow-events-mixin
- var flowInfo = this.flow && this.flow.pickResumeTokenInfo();
var relierInfo = this.relier.pickResumeTokenInfo();
var userInfo = this.user.pickResumeTokenInfo();
+ let flowInfo;
+ const flowModel = this.metrics.getFlowModel();
+ if (flowModel) {
+ flowInfo = flowModel.pickResumeTokenInfo();
+ }
+
var resumeTokenInfo = _.extend(
{},
flowInfo,
@@ -105,16 +105,10 @@ define(function (require, exports, module) {
if (verificationReason === VerificationReasons.SIGN_IN &&
verificationMethod === VerificationMethods.EMAIL) {
- return this.navigate('confirm_signin', {
- account: account,
- flow: this.flow
- });
- } else {
- return this.navigate('confirm', {
- account: account,
- flow: this.flow
- });
+ return this.navigate('confirm_signin', { account });
}
+
+ return this.navigate('confirm', { account });
}
// If the account's uid changed, update the relier model or else
@@ -83,10 +83,7 @@ define(function (require, exports, module) {
return this.invokeBrokerMethod('afterSignUp', account)
.then(() => {
- this.navigate('confirm', {
- account: account,
- flow: this.flow
- });
+ this.navigate('confirm', { account });
});
},
@@ -274,7 +274,7 @@ define(function (require, exports, module) {
user: userMock,
window: windowMock
});
- appStart._metrics = new Metrics();
+ appStart._metrics = new Metrics({ notifier });
});
describe('fx-firstrun-v1', function () {
@@ -9,12 +9,12 @@ define(function (require, exports, module) {
const chai = require('chai');
const Cocktail = require('cocktail');
const Notifier = require('lib/channels/notifier');
- const NotifierMixin = require('views/mixins/notifier-mixin');
+ const NotifierMixin = require('lib/channels/notifier-mixin');
const sinon = require('sinon');
var assert = chai.assert;
- describe('views/mixins/notifier-mixin', function () {
+ describe('lib/channels/notifier-mixin', function () {
var data = { uid: 'foo' };
var functionHandlerSpy;
var notifier;
Oops, something went wrong.

0 comments on commit 4071291

Please sign in to comment.