diff --git a/app/scripts/lib/experiments/grouping-rules/send-sms-install-link.js b/app/scripts/lib/experiments/grouping-rules/send-sms-install-link.js index 44c766b3b..a211a612b 100644 --- a/app/scripts/lib/experiments/grouping-rules/send-sms-install-link.js +++ b/app/scripts/lib/experiments/grouping-rules/send-sms-install-link.js @@ -10,9 +10,9 @@ define((require, exports, module) => { const BaseGroupingRule = require('./base'); - const GROUPS = ['control', 'treatment']; + const GROUPS = ['control', 'treatment', 'signinCodes']; - function isEmailInTreatment (email) { + function isEmailInSigninCodesGroup (email) { return /@softvision\.(com|ro)$/.test(email) || /@mozilla\.(com|org)$/.test(email); } @@ -30,10 +30,10 @@ define((require, exports, module) => { let choice; - if (subject.forceExperimentGroup) { + if (subject.forceExperiment === this.name && subject.forceExperimentGroup) { choice = subject.forceExperimentGroup; - } else if (isEmailInTreatment(subject.account.get('email'))) { - choice = 'treatment'; + } else if (isEmailInSigninCodesGroup(subject.account.get('email'))) { + choice = 'signinCodes'; } else { choice = this.uniformChoice(GROUPS, subject.uniqueUserId); } diff --git a/app/scripts/models/reliers/sync.js b/app/scripts/models/reliers/sync.js index e579f904a..40d421a6a 100644 --- a/app/scripts/models/reliers/sync.js +++ b/app/scripts/models/reliers/sync.js @@ -32,16 +32,13 @@ define(function (require, exports, module) { // because users that open the verification link with an // invalid signinCode should still be able to sign in. // The error will be logged and the signinCode ignored. - signin: Vat.string().empty('').renameTo('signinCode'), - // allow relier to enable signinCodes, used for testing. - signinCodes: Vat.boolean().renameTo('enableSigninCodes') + signin: Vat.string().empty('').renameTo('signinCode') }; /*eslint-enable camelcase*/ module.exports = Relier.extend({ defaults: _.extend({}, Relier.prototype.defaults, { customizeSync: false, - enableSigninCodes: false, signinCode: undefined }), diff --git a/app/scripts/views/mixins/connect-another-device-mixin.js b/app/scripts/views/mixins/connect-another-device-mixin.js index 13fb59c3b..2a428468b 100644 --- a/app/scripts/views/mixins/connect-another-device-mixin.js +++ b/app/scripts/views/mixins/connect-another-device-mixin.js @@ -74,9 +74,9 @@ define((require, exports, module) => { const group = this.getExperimentGroup('sendSms', { account }); this.createExperiment('sendSms', group); - if (group === 'treatment') { + if (group === 'treatment' || group === 'signinCodes') { this.navigate('sms', { account, country }); - } else { // there are only two groups, by default, this is the control + } else { // there are only three groups, by default, this is the control this.navigate('connect_another_device', { account }); } } else { diff --git a/app/scripts/views/mixins/experiment-mixin.js b/app/scripts/views/mixins/experiment-mixin.js index 52110f952..933027b3d 100644 --- a/app/scripts/views/mixins/experiment-mixin.js +++ b/app/scripts/views/mixins/experiment-mixin.js @@ -2,15 +2,45 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +/** + * Mixin to provide experiment related functionality. + * + * Expects the view to expose: + * - either this.getAccount() or this._account + * - this.metrics + * - this.translator + * - this.window + * + * @mixin ExperimentMixin + */ define(function (require, exports, module) { 'use strict'; + const { isFunction } = require('underscore'); const ExperimentInterface = require('lib/experiment'); + function getAccount () { + // make no assumptions about the availability of this.getAccount. + return (isFunction(this.getAccount) && this.getAccount()) || this._account; + } + module.exports = { initialize (options = {}) { - this.experiments = options.experiments || new ExperimentInterface({ - account: this._account, + this.experiments = options.experiments || this._createExperimentInterface(options); + + this.experiments.chooseExperiments(); + }, + + /** + * Create an ExperimentInterface instance using `options` + * + * @param {Object} [options={}] + * @returns {Object} + * @private + */ + _createExperimentInterface (options = {}) { + return new ExperimentInterface({ + account: getAccount.call(this), experimentGroupingRules: options.experimentGroupingRules, metrics: this.metrics, notifier: options.notifier, @@ -18,8 +48,6 @@ define(function (require, exports, module) { user: options.user, window: this.window }); - - this.experiments.chooseExperiments(); }, /** diff --git a/app/scripts/views/mixins/sms-mixin.js b/app/scripts/views/mixins/sms-mixin.js index ee0d19e9a..5b57e272a 100644 --- a/app/scripts/views/mixins/sms-mixin.js +++ b/app/scripts/views/mixins/sms-mixin.js @@ -4,13 +4,18 @@ /** * Get the features that should be enabled when sending an SMS. - * Requires the SeachParamMixin + * + * @mixin SmsMixin */ define((require, exports, module) => { 'use strict'; + const ExperimentMixin = require('views/mixins/experiment-mixin'); + module.exports = { + dependsOn: [ ExperimentMixin ], + /** * Get the features that should be enabled when sending an SMS * @@ -18,7 +23,7 @@ define((require, exports, module) => { */ getSmsFeatures () { const features = []; - if (this.relier.get('enableSigninCodes')) { + if (this.isInExperimentGroup('sendSms', 'signinCodes')) { features.push('signinCodes'); } return features; diff --git a/app/tests/spec/lib/experiments/grouping-rules/send-sms-install-link.js b/app/tests/spec/lib/experiments/grouping-rules/send-sms-install-link.js index 5e6bed3b2..f15ad7ab1 100644 --- a/app/tests/spec/lib/experiments/grouping-rules/send-sms-install-link.js +++ b/app/tests/spec/lib/experiments/grouping-rules/send-sms-install-link.js @@ -25,23 +25,23 @@ define(function (require, exports, module) { assert.isFalse(experiment.choose({ uniqueUserId: 'user-id' })); }); - describe('forceExperimentGroup set', () => { + describe('forceExperiment, forceExperimentGroup set', () => { it('returns `treatment`', () => { account.set('email', 'testuser@testuser.com'); - assert.equal(experiment.choose({ account, forceExperimentGroup: 'treatment', uniqueUserId: 'user-id' }), 'treatment'); + assert.equal(experiment.choose({ account, forceExperiment: 'sendSms', forceExperimentGroup: 'treatment', uniqueUserId: 'user-id' }), 'treatment'); }); }); - describe('email forces treatment', () => { + describe('email forces `signinCodes`', () => { it('returns true', () => { account.set('email', 'testuser@softvision.com'); - assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'treatment'); + assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'signinCodes'); account.set('email', 'testuser@softvision.ro'); - assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'treatment'); + assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'signinCodes'); account.set('email', 'testuser@mozilla.com'); - assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'treatment'); + assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'signinCodes'); account.set('email', 'testuser@mozilla.org'); - assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'treatment'); + assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'signinCodes'); }); }); @@ -52,7 +52,7 @@ define(function (require, exports, module) { assert.equal(experiment.choose({ account, uniqueUserId: 'user-id' }), 'control'); assert.isTrue(experiment.uniformChoice.calledOnce); - assert.isTrue(experiment.uniformChoice.calledWith(['control', 'treatment'], 'user-id')); + assert.isTrue(experiment.uniformChoice.calledWith(['control', 'treatment', 'signinCodes'], 'user-id')); }); }); }); diff --git a/app/tests/spec/models/reliers/sync.js b/app/tests/spec/models/reliers/sync.js index 526b37ac5..34ce6ff92 100644 --- a/app/tests/spec/models/reliers/sync.js +++ b/app/tests/spec/models/reliers/sync.js @@ -50,8 +50,7 @@ define(function (require, exports, module) { customizeSync: 'true', migration: SYNC_MIGRATION, service: SYNC_SERVICE, - signin: 'signin-code', - signinCodes: 'true' + signin: 'signin-code' }); return relier.fetch() @@ -62,7 +61,6 @@ define(function (require, exports, module) { assert.equal(relier.get('service'), SYNC_SERVICE); assert.isTrue(relier.get('customizeSync')); assert.equal(relier.get('signinCode'), 'signin-code'); - assert.isTrue(relier.get('enableSigninCodes')); }); }); @@ -294,100 +292,6 @@ define(function (require, exports, module) { }); }); - describe('signinCodes query parameter', () => { - describe('missing', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT - }); - - return relier.fetch(); - }); - - it('succeeds', () => { - assert.isFalse(relier.get('enableSigninCodes')); - }); - }); - - describe('emtpy', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT, - signinCodes: '' - }); - - return fetchExpectError(); - }); - - it('errors correctly', () => { - assert.isTrue(AuthErrors.is(err, 'INVALID_PARAMETER')); - assert.equal(err.param, 'signinCodes'); - }); - }); - - describe('whitespace', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT, - signinCodes: ' ' - }); - - return fetchExpectError(); - }); - - it('errors correctly', () => { - assert.isTrue(AuthErrors.is(err, 'INVALID_PARAMETER')); - assert.equal(err.param, 'signinCodes'); - }); - }); - - describe('not a boolean', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT, - signinCodes: 'not a boolean' - }); - - return fetchExpectError(); - }); - - it('errors correctly', () => { - assert.isTrue(AuthErrors.is(err, 'INVALID_PARAMETER')); - assert.equal(err.param, 'signinCodes'); - }); - }); - - describe('true', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT, - signinCodes: 'true' - }); - - return relier.fetch(); - }); - - it('succeeds', () => { - assert.isTrue(relier.get('enableSigninCodes')); - }); - }); - - describe('false', () => { - beforeEach(() => { - windowMock.location.search = TestHelpers.toSearchString({ - context: CONTEXT, - signinCodes: 'false' - }); - - return relier.fetch(); - }); - - it('succeeds', () => { - assert.isFalse(relier.get('enableSigninCodes')); - }); - }); - }); - it('translates `service` to `serviceName`', () => { windowMock.location.search = TestHelpers.toSearchString({ context: CONTEXT, diff --git a/app/tests/spec/views/mixins/experiment-mixin.js b/app/tests/spec/views/mixins/experiment-mixin.js index f05f6a93c..3c3b753ad 100644 --- a/app/tests/spec/views/mixins/experiment-mixin.js +++ b/app/tests/spec/views/mixins/experiment-mixin.js @@ -24,7 +24,9 @@ define(function (require, exports, module) { describe('views/mixins/experiment-mixin', () => { let experiments; + let metrics; let notifier; + let translator; let view; let windowMock; @@ -43,14 +45,21 @@ define(function (require, exports, module) { isInExperimentGroup: sinon.spy() }; + metrics = {}; + notifier = { trigger: sinon.spy() }; + + translator = {}; + windowMock = new WindowMock(); view = new View({ experiments, + metrics, notifier, + translator, window: windowMock }); }); @@ -59,6 +68,56 @@ define(function (require, exports, module) { return view.destroy(); }); + describe('_createExperimentInterface', () => { + const experimentGroupingRules = {}; + const notifier = {}; + const user = {}; + + const getAccountAccount = { uid: 'get-account-account' }; + const _accountAccount = { uid: '_account-account' }; + + describe('view has `this.getAccount', () => { + it('creates ExperimentInterface with account returned by this.getAccount', () => { + view.getAccount = sinon.spy(() => getAccountAccount); + view._account = _accountAccount; + + const experimentInterface = view._createExperimentInterface({ + experimentGroupingRules, + notifier, + user + }); + + assert.strictEqual(experimentInterface.account, getAccountAccount); + assert.strictEqual(experimentInterface.experimentGroupingRules, experimentGroupingRules); + assert.strictEqual(experimentInterface.notifier, notifier); + assert.strictEqual(experimentInterface.metrics, metrics); + assert.strictEqual(experimentInterface.translator, translator); + assert.strictEqual(experimentInterface.user, user); + assert.strictEqual(experimentInterface.window, windowMock); + }); + }); + + describe('view does not have this.getAccount, has this._account', () => { + it('creates ExperimentInterface with this._account', () => { + view._account = _accountAccount; + + const experimentInterface = view._createExperimentInterface({ + experimentGroupingRules, + notifier, + user + }); + + assert.strictEqual(experimentInterface.account, _accountAccount); + assert.strictEqual(experimentInterface.experimentGroupingRules, experimentGroupingRules); + assert.strictEqual(experimentInterface.notifier, notifier); + assert.strictEqual(experimentInterface.metrics, metrics); + assert.strictEqual(experimentInterface.translator, translator); + assert.strictEqual(experimentInterface.user, user); + assert.strictEqual(experimentInterface.window, windowMock); + }); + }); + }); + describe('initialize', () => { it('chooses experiments', () => { assert.isTrue(experiments.chooseExperiments.calledOnce); diff --git a/app/tests/spec/views/mixins/sms-mixin.js b/app/tests/spec/views/mixins/sms-mixin.js index d51b1d81c..9c2b30b23 100644 --- a/app/tests/spec/views/mixins/sms-mixin.js +++ b/app/tests/spec/views/mixins/sms-mixin.js @@ -9,8 +9,7 @@ define(function (require, exports, module) { const { assert } = require('chai'); const BaseView = require('views/base'); const Cocktail = require('cocktail'); - const Relier = require('models/reliers/base'); - const SearchParamMixin = require('lib/search-param-mixin'); + const sinon = require('sinon'); const Template = require('stache!templates/test_template'); const SmsView = BaseView.extend({ @@ -18,34 +17,30 @@ define(function (require, exports, module) { }); Cocktail.mixin( SmsView, - SearchParamMixin, SmsMixin ); describe('views/mixins/sms-mixin', () => { - let relier; let view; beforeEach(() => { - relier = new Relier(); view = new SmsView({ - relier, windowMock: window }); }); describe('getSmsFeatures', () => { - describe('relier.enableSigninCodes=true', () => { + describe('user in `signinCodes` experiment group', () => { it('returns an array with `signinCodes`', () => { - relier.set('enableSigninCodes', true); + sinon.stub(view, 'isInExperimentGroup', () => true); assert.isTrue(view.getSmsFeatures().indexOf('signinCodes') > -1); }); }); - describe('relier.enableSigninCodes=false', () => { + describe('user not in `signinCodes` experiment group', () => { it('returns an empty array', () => { - relier.set('enableSigninCodes', false); + sinon.stub(view, 'isInExperimentGroup', () => false); assert.deepEqual(view.getSmsFeatures(), []); }); }); diff --git a/docs/query-params.md b/docs/query-params.md index 9dc5dff15..967db2a7d 100644 --- a/docs/query-params.md +++ b/docs/query-params.md @@ -316,10 +316,8 @@ Should not be used by reliers. Should only be used by functional tests. Force a particular AB test. #### Options -* `connectAnotherDevice` - the "connect another device" experiment that - encourages users to set up multiple devices. -* `mailcheck` - provide a tooltip to correct common domain name errors in the - email. +* `disabledButtonState` - Are the submit buttons on signin/signup disabled? +* `sendSms` - Allow users to send an SMS containing a Firefox Mobile installation link ### `forceExperimentGroup` Force the user into a particular AB test experiment group. @@ -327,6 +325,8 @@ Force the user into a particular AB test experiment group. #### Options * `control` - default behavior. * `treatment` - new behavior. +* `signinCodes` - a second treatment group, only used for the `sendSms` experiment. + When sending an SMS, the install link contains a signinCode that helps the user sign in more easily on the second device. ## Reset Password parameters @@ -357,7 +357,7 @@ if they perform a reset password. Shows the option to change a user's primary email address. #### Options -* `true` +* `true` * `false` (default) #### When to specify diff --git a/tests/functional/fx_desktop_handshake.js b/tests/functional/fx_desktop_handshake.js index 3ed8809d3..ed73a8715 100644 --- a/tests/functional/fx_desktop_handshake.js +++ b/tests/functional/fx_desktop_handshake.js @@ -28,7 +28,7 @@ define([ const SETTINGS_PAGE_URL = `${config.fxaContentRoot}settings?automatedBrowser=true&forceUA=${encodeURIComponent(userAgent)}`; const SYNC_SETTINGS_PAGE_URL = `${SETTINGS_PAGE_URL}&service=sync`; - const SYNC_SMS_PAGE_URL = `${config.fxaContentRoot}sms?automatedBrowser=true&service=sync&signinCodes=true&forceUA=${encodeURIComponent(userAgent)}`; + const SYNC_SMS_PAGE_URL = `${config.fxaContentRoot}sms?automatedBrowser=true&service=sync&forceExperiment=sendSms&forceExperimentGroup=signinCodes&forceUA=${encodeURIComponent(userAgent)}`; //eslint-disable-line max-len var browserSignedInEmail; let browserSignedInAccount; diff --git a/tests/functional/fx_fennec_v1_sign_in.js b/tests/functional/fx_fennec_v1_sign_in.js index 26dfc6de0..709874a94 100644 --- a/tests/functional/fx_fennec_v1_sign_in.js +++ b/tests/functional/fx_fennec_v1_sign_in.js @@ -13,7 +13,7 @@ define([ const config = intern.config; const SIGNIN_PAGE_URL = `${config.fxaContentRoot}signin?context=fx_fennec_v1&service=sync`; - const SMS_PAGE_URL = `${config.fxaContentRoot}sms?context=fx_desktop_v1&service=sync&signinCodes=true`; + const SMS_PAGE_URL = `${config.fxaContentRoot}sms?context=fx_desktop_v1&service=sync&forceExperiment=sendSms&forceExperimentGroup=signinCodes`; let email; const PASSWORD = '12345678'; diff --git a/tests/functional/fx_ios_v1_sign_in.js b/tests/functional/fx_ios_v1_sign_in.js index ee5c840f4..5b15dc3e9 100644 --- a/tests/functional/fx_ios_v1_sign_in.js +++ b/tests/functional/fx_ios_v1_sign_in.js @@ -16,7 +16,7 @@ define([ const config = intern.config; const SIGNIN_PAGE_URL = `${config.fxaContentRoot}signin?context=fx_ios_v1&service=sync`; - const SMS_PAGE_URL = `${config.fxaContentRoot}sms?context=fx_desktop_v1&service=sync&signinCodes=true`; + const SMS_PAGE_URL = `${config.fxaContentRoot}sms?context=fx_desktop_v1&service=sync&forceExperiment=sendSms&forceExperimentGroup=signinCodes`; let email; const PASSWORD = '12345678'; diff --git a/tests/functional/send_sms.js b/tests/functional/send_sms.js index 467e331b0..5404e40e4 100644 --- a/tests/functional/send_sms.js +++ b/tests/functional/send_sms.js @@ -27,7 +27,7 @@ const SEND_SMS_URL = `${config.fxaContentRoot}sms?service=sync&country=US`; - const SEND_SMS_SIGNIN_CODE_URL = `${SEND_SMS_URL}&signinCodes=true`; + const SEND_SMS_SIGNIN_CODE_URL = `${SEND_SMS_URL}&forceExperiment=sendSms&forceExperimentGroup=signinCodes`; const SEND_SMS_NO_QUERY_URL = `${config.fxaContentRoot}sms`;