feat(sms): Log why users are ineligible for the SMS experiment. (#5385) r=@philbooth
These are logged as flow evnets. Possible flow events: * flow.sms.ineligible.android * flow.sms.ineligible.control_group * flow.sms.ineligible.ios * flow.sms.ineligible.not_in_experiment * flow.sms.ineligible.no_session * flow.sms.ineligible.other_user_signed_in * flow.sms.ineligible.signin * flow.sms.ineligible.unsupported_country * flow.sms.ineligible.xhr_error In addition, the country returned from the auth-server is logged: * flow.sms.status.country.<TWO_LETTER_COUNTRY_CODE> fixes #5382
This commit is contained in:
Shane Tomlinson
GitHub
Родитель
345463bd97
Коммит
765b2b41dc
|
|
@ -29,6 +29,16 @@ define((require, exports, module) => {
|
|||
const UserAgentMixin = require('lib/user-agent-mixin');
|
||||
const VerificationReasonMixin = require('views/mixins/verification-reason-mixin');
|
||||
|
||||
const REASON_ANDROID = 'sms.ineligible.android';
|
||||
const REASON_CONTROL_GROUP = 'sms.ineligible.control_group';
|
||||
const REASON_IOS = 'sms.ineligible.ios';
|
||||
const REASON_NOT_IN_EXPERIMENT = 'sms.ineligible.not_in_experiment';
|
||||
const REASON_NO_SESSION = 'sms.ineligible.no_session';
|
||||
const REASON_OTHER_USER_SIGNED_IN = 'sms.ineligible.other_user_signed_in';
|
||||
const REASON_SIGNIN = 'sms.ineligible.signin';
|
||||
const REASON_UNSUPPORTED_COUNTRY = 'sms.ineligible.unsupported_country';
|
||||
const REASON_XHR_ERROR = 'sms.ineligible.xhr_error';
|
||||
|
||||
return {
|
||||
dependsOn: [
|
||||
ExperimentMixin,
|
||||
|
|
@ -66,6 +76,11 @@ define((require, exports, module) => {
|
|||
return p.reject(new Error('chooseConnectAnotherDeviceScreen can only be called if user is eligible to connect another device'));
|
||||
}
|
||||
|
||||
// Initialize the flow metrics so any flow events are logged.
|
||||
// The flow-events-mixin, even if it were mixed in, does this in
|
||||
// `afterRender` whereas this method can be called in `beforeRender`
|
||||
this.notifier.trigger('flow.initialize');
|
||||
|
||||
return this._isEligibleForSms(account)
|
||||
.then(({ ok, country }) => {
|
||||
if (ok) {
|
||||
|
|
@ -74,10 +89,12 @@ define((require, exports, module) => {
|
|||
const group = this.getExperimentGroup('sendSms', { account });
|
||||
this.createExperiment('sendSms', group);
|
||||
|
||||
if (group === 'treatment' || group === 'signinCodes') {
|
||||
this.navigate('sms', { account, country });
|
||||
} else { // there are only three groups, by default, this is the control
|
||||
if (group === 'control') {
|
||||
this.logFlowEvent(REASON_CONTROL_GROUP);
|
||||
this.navigate('connect_another_device', { account });
|
||||
} else {
|
||||
// all non-control groups go to the sms page.
|
||||
this.navigate('sms', { account, country });
|
||||
}
|
||||
} else {
|
||||
this.navigate('connect_another_device', { account });
|
||||
|
|
@ -108,22 +125,37 @@ define((require, exports, module) => {
|
|||
},
|
||||
|
||||
/**
|
||||
* Check if the requirements are met to send an SMS.
|
||||
* Check if the requirements are met to send an SMS, if not, log why.
|
||||
*
|
||||
* @param {Object} account
|
||||
* @returns {Boolean}
|
||||
* @private
|
||||
*/
|
||||
_areSmsRequirementsMet (account) {
|
||||
return this.isSignUp() &&
|
||||
let reason;
|
||||
|
||||
if (! this.isSignUp()) {
|
||||
reason = REASON_SIGNIN;
|
||||
} else if (this.getUserAgent().isAndroid()) {
|
||||
// If already on a mobile device, doesn't make sense to send an SMS.
|
||||
! this.getUserAgent().isAndroid() &&
|
||||
! this.getUserAgent().isIos() &&
|
||||
reason = REASON_ANDROID;
|
||||
} else if (this.getUserAgent().isIos()) {
|
||||
reason = REASON_IOS;
|
||||
} else if (! (account && account.get('sessionToken'))) {
|
||||
reason = REASON_NO_SESSION;
|
||||
} else if (this.user.isAnotherAccountSignedIn(account)) {
|
||||
// If a user is already signed in to Sync which is different to the
|
||||
// user that just verified, show them the old "Account verified!" screen.
|
||||
! this.user.isAnotherAccountSignedIn(account) &&
|
||||
// Does able say we are eligible for the experiment?
|
||||
this.isInExperiment('sendSms', { account });
|
||||
reason = REASON_OTHER_USER_SIGNED_IN;
|
||||
} else if (! this.isInExperiment('sendSms', { account })) {
|
||||
reason = REASON_NOT_IN_EXPERIMENT;
|
||||
}
|
||||
|
||||
if (reason) {
|
||||
this.logFlowEvent(reason);
|
||||
}
|
||||
|
||||
return ! reason;
|
||||
},
|
||||
|
||||
/**
|
||||
|
|
@ -147,11 +179,17 @@ define((require, exports, module) => {
|
|||
// that SMS is enabled for Romania, though it's only enabled
|
||||
// for testing and not for the public at large. Experiment choices are used
|
||||
// for this because it's the logic most likely to change.
|
||||
if (resp.country) {
|
||||
this.logFlowEvent(`sms.status.country.${resp.country}`);
|
||||
}
|
||||
|
||||
// If geo-lookup is disabled, no country is returned, assume US
|
||||
const country = resp.country || 'US';
|
||||
if (resp.ok && this.isInExperiment('sendSmsEnabledForCountry', { country })) {
|
||||
return country;
|
||||
} else {
|
||||
// It's a big assumption, but assume ok: false means an unsupported country.
|
||||
this.logFlowEvent(REASON_UNSUPPORTED_COUNTRY);
|
||||
}
|
||||
}, (err) => {
|
||||
// Add `.smsStatus` to the context so we can differentiate between errors
|
||||
|
|
@ -161,6 +199,7 @@ define((require, exports, module) => {
|
|||
// prevent verification from completing. Send the user to
|
||||
// /connect_another_device instead. See #5109
|
||||
this.logError(err);
|
||||
this.logFlowEvent(REASON_XHR_ERROR);
|
||||
});
|
||||
}
|
||||
};
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ define(function (require, exports, module) {
|
|||
const Constants = require('lib/constants');
|
||||
const ExperimentMixin = require('views/mixins/experiment-mixin');
|
||||
const Cocktail = require('cocktail');
|
||||
const Notifier = require('lib/channels/notifier');
|
||||
const p = require('lib/promise');
|
||||
const Relier = require('models/reliers/relier');
|
||||
const sinon = require('sinon');
|
||||
|
|
@ -38,29 +39,32 @@ define(function (require, exports, module) {
|
|||
|
||||
describe('views/mixins/connect-another-device-mixin', () => {
|
||||
let account;
|
||||
let notifier;
|
||||
let relier;
|
||||
let user;
|
||||
let view;
|
||||
|
||||
beforeEach(() => {
|
||||
notifier = new Notifier();
|
||||
relier = new Relier();
|
||||
user = new User();
|
||||
|
||||
view = new View({
|
||||
notifier,
|
||||
relier,
|
||||
user
|
||||
});
|
||||
|
||||
sinon.stub(view, 'logFlowEvent', () => {});
|
||||
|
||||
account = user.initAccount({
|
||||
email: 'a@a.com',
|
||||
sessionToken: 'foo',
|
||||
uid: VALID_UID
|
||||
});
|
||||
});
|
||||
|
||||
describe('isEligibleForConnectAnotherDevice', () => {
|
||||
beforeEach(() => {
|
||||
account = user.initAccount({
|
||||
email: 'a@a.com',
|
||||
sessionToken: 'foo',
|
||||
uid: VALID_UID
|
||||
});
|
||||
});
|
||||
|
||||
describe('user is completing sign-in', () => {
|
||||
beforeEach(() => {
|
||||
sinon.stub(user, 'getSignedInAccount', () => {
|
||||
|
|
@ -123,12 +127,6 @@ define(function (require, exports, module) {
|
|||
|
||||
describe('_isEligibleForSms', () => {
|
||||
beforeEach(() => {
|
||||
account = user.initAccount({
|
||||
email: 'a@a.com',
|
||||
sessionToken: 'foo',
|
||||
uid: VALID_UID
|
||||
});
|
||||
|
||||
relier.set('country', 'US');
|
||||
});
|
||||
|
||||
|
|
@ -259,6 +257,8 @@ define(function (require, exports, module) {
|
|||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.signin'));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -277,6 +277,8 @@ define(function (require, exports, module) {
|
|||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.android'));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -295,6 +297,29 @@ define(function (require, exports, module) {
|
|||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.ios'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('no session', () => {
|
||||
beforeEach(() => {
|
||||
sinon.stub(view, 'isSignUp', () => true);
|
||||
sinon.stub(view, 'isInExperiment', () => true);
|
||||
sinon.stub(view, 'getUserAgent', () => {
|
||||
return {
|
||||
isAndroid: () => false,
|
||||
isIos: () => false
|
||||
};
|
||||
});
|
||||
account.unset('sessionToken');
|
||||
sinon.stub(user, 'isAnotherAccountSignedIn', () => true);
|
||||
});
|
||||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.no_session'));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -313,10 +338,12 @@ define(function (require, exports, module) {
|
|||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.other_user_signed_in'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('user is not part of treatment group', () => {
|
||||
describe('user is not part of experiment', () => {
|
||||
beforeEach(() => {
|
||||
sinon.stub(view, 'isSignUp', () => true);
|
||||
sinon.stub(view, 'isInExperiment', () => false);
|
||||
|
|
@ -331,6 +358,8 @@ define(function (require, exports, module) {
|
|||
|
||||
it('returns `false', () => {
|
||||
assert.isFalse(view._areSmsRequirementsMet(account));
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.not_in_experiment'));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -353,17 +382,70 @@ define(function (require, exports, module) {
|
|||
});
|
||||
});
|
||||
|
||||
describe('navigateToConnectAnotherDeviceScreen', () => {
|
||||
let account;
|
||||
describe('_smsCountry', () => {
|
||||
it('resolves to the country on success', () => {
|
||||
sinon.stub(account, 'smsStatus', () => p({ country: 'GB', ok: true }));
|
||||
sinon.stub(view, 'isInExperiment', () => true);
|
||||
|
||||
beforeEach(() => {
|
||||
account = user.initAccount({
|
||||
email: 'a@a.com',
|
||||
sessionToken: 'foo',
|
||||
uid: VALID_UID
|
||||
});
|
||||
return view._smsCountry(account)
|
||||
.then((country) => {
|
||||
assert.equal(country, 'GB');
|
||||
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.status.country.GB'));
|
||||
});
|
||||
});
|
||||
|
||||
it('resolves to `undefined` if auth-server responds ok: false', () => {
|
||||
sinon.stub(account, 'smsStatus', () => p({ country: 'AZ', ok: false }));
|
||||
|
||||
return view._smsCountry(account)
|
||||
.then((country) => {
|
||||
assert.isUndefined(country);
|
||||
|
||||
assert.isTrue(view.logFlowEvent.calledTwice);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.status.country.AZ'));
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.unsupported_country'));
|
||||
});
|
||||
});
|
||||
|
||||
it('resolves to `undefined` if auth-server reported country is not supported', () => {
|
||||
sinon.stub(account, 'smsStatus', () => p({ country: 'AZ', ok: true }));
|
||||
sinon.stub(view, 'isInExperiment', () => false);
|
||||
|
||||
return view._smsCountry(account)
|
||||
.then((country) => {
|
||||
assert.isUndefined(country);
|
||||
|
||||
assert.isTrue(view.isInExperiment.calledOnce);
|
||||
assert.isTrue(view.isInExperiment.calledWith('sendSmsEnabledForCountry', { country: 'AZ' }));
|
||||
|
||||
assert.isTrue(view.logFlowEvent.calledTwice);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.status.country.AZ'));
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.unsupported_country'));
|
||||
});
|
||||
});
|
||||
|
||||
it('handles XHR errors', () => {
|
||||
const err = AuthErrors.toError('UNEXPECTED_ERROR');
|
||||
|
||||
sinon.stub(account, 'smsStatus', () => p.reject(err));
|
||||
sinon.stub(view, 'logError', () => {});
|
||||
|
||||
return view._smsCountry(account)
|
||||
.then((country) => {
|
||||
assert.isUndefined(country);
|
||||
|
||||
assert.isTrue(view.logError.calledOnce);
|
||||
assert.isTrue(view.logError.calledWith(err));
|
||||
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.xhr_error'));
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('navigateToConnectAnotherDeviceScreen', () => {
|
||||
describe('not eligible for CAD', () => {
|
||||
it('rejects with an error', () => {
|
||||
sinon.stub(view, 'isEligibleForConnectAnotherDevice', () => false);
|
||||
|
|
@ -379,6 +461,7 @@ define(function (require, exports, module) {
|
|||
sinon.stub(view, 'isEligibleForConnectAnotherDevice', () => true);
|
||||
sinon.stub(view, 'navigate', () => {});
|
||||
sinon.stub(view, 'createExperiment', () => {});
|
||||
sinon.spy(notifier, 'trigger');
|
||||
});
|
||||
|
||||
describe('not eligible for SMS', () => {
|
||||
|
|
@ -389,6 +472,9 @@ define(function (require, exports, module) {
|
|||
.then(() => {
|
||||
assert.isFalse(view.createExperiment.called);
|
||||
|
||||
assert.isTrue(notifier.trigger.calledOnce);
|
||||
assert.isTrue(notifier.trigger.calledWith('flow.initialize'));
|
||||
|
||||
assert.isTrue(view.navigate.calledOnce);
|
||||
assert.isTrue(view.navigate.calledWith('connect_another_device', { account }));
|
||||
});
|
||||
|
|
@ -424,6 +510,9 @@ define(function (require, exports, module) {
|
|||
|
||||
assert.isTrue(view.navigate.calledOnce);
|
||||
assert.isTrue(view.navigate.calledWith('connect_another_device', { account }));
|
||||
|
||||
assert.isTrue(view.logFlowEvent.calledOnce);
|
||||
assert.isTrue(view.logFlowEvent.calledWith('sms.ineligible.control_group'));
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче