feat(mailcheck): enabling mailcheck for all. #4703

divyabiyani · 2017-02-07T13:59:41Z

No description provided.

vladikoff · 2017-02-07T15:24:08Z

tests/functional/verification_experiments.js

-  'intern!object',
-  'tests/functional/lib/helpers'
-], function (intern, registerSuite, FunctionalHelpers) {
-  var EXP_MAILCHECK_URL = intern.config.fxaContentRoot + 'signup?forceExperiment=mailcheck&automatedBrowser=true';


@divyabiyani for this file, we should keep it, but rename it to mailcheck (make sure to rename it in the list of functional tests as well).

We can remove the 'signup?forceExperiment=mailcheck&automatedBrowser=true' query part and only leave the 'treatment works': function () { test. Delete the 'control works': function () { test though

vladikoff · 2017-02-07T15:27:06Z

app/scripts/lib/mailcheck.js

-              if (e.type === 'click' || e.which === KeyCodes.ENTER) {
-                element.val(suggestion.full);
-                // the user has used the suggestion
-                view.notifier.trigger('mailcheck.clicked');


@divyabiyani this used to log that the tooltip was clicked into the experiment file. Could you change this to logEvent, this way we still get metrics that users click on the tooltip.

We might have a unit test for the mailcheck click action, we need to update it to make sure that the correct event is logged

vladikoff · 2017-02-07T15:27:48Z

app/scripts/lib/mailcheck.js

@@ -55,37 +55,34 @@ define(function (require, exports, module) {

          // user got a suggestion to check their email input
          view.notifier.trigger('mailcheck.suggested');


same as https://github.com/mozilla/fxa-content-server/pull/4703/files#r99847017 , except this should logEvent that mailcheck was suggested

vladikoff · 2017-02-07T15:28:18Z

app/scripts/lib/mailcheck.js

+            if (e.type === 'click' || e.which === KeyCodes.ENTER) {
+              element.val(suggestion.full);
+              // the user has used the suggestion
+              view.notifier.trigger('mailcheck.clicked');


see https://github.com/mozilla/fxa-content-server/pull/4703/files#r99847017

vladikoff · 2017-02-09T16:39:11Z

unit tests need updating, see this mockView:

fxa-content-server/app/tests/spec/lib/mailcheck.js

Line 31 in 878b80c

mockView = {

https://github.com/mozilla/fxa-content-server/blob/master/app/tests/spec/lib/mailcheck.js#L36

vladikoff · 2017-02-09T16:41:25Z

Need to change the tests:

fxa-content-server/app/tests/spec/views/sign_up.js

Line 1190 in 61181e8

describe('suggestEmail', function () {

vladikoff · 2017-02-09T16:45:14Z

To fix issue with lib/experiment: either use the connectAnotherDevice for the test OR pass in allExperiments optionally for the test

vladikoff · 2017-02-09T16:50:11Z

See if we can fix corrected event here: https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/views/sign_up.js#L215 Also double if any other component uses signup.submit trigger, can we safely remove it?

vladikoff · 2017-02-09T16:52:20Z

Corrected event functionality: https://github.com/mozilla/fxa-content-server/blob/master/app/scripts/lib/experiments/mailcheck.js#L21

vladikoff · 2017-02-15T15:04:20Z

tests/spec/views/sign_up

check for isInExperiment for mailcheck, we can remove some of that

fxa-content-server/app/tests/spec/views/sign_up.js

Line 1286 in 313e69e

sinon.stub(view, 'isInExperiment', () => true);

(we still need that test, but without the experiment code)

The fail in does not show when able chooses control -> that whole test can be removed.
it('accepts window parameter override' can also be removed.
it('works when able chooses treatment' can also be removed.

vladikoff · 2017-02-15T15:05:24Z

it('accepts window parameter override' test can be removed, we no longer use the parameter override.

vladikoff · 2017-02-15T15:11:13Z

^ make that red, using an option to the Tooltip constructor. Maybe call the option, try isError: true. We might use that in other tooltips in the future.

vladikoff · 2017-02-15T15:12:13Z

tests/functional/mailcheck.js

@@ -34,22 +31,12 @@ define([
      var CORRECTED_EMAIL = 'something@gmail.com';


rename this from 'treatment works' - tooltip works

vladikoff · 2017-02-15T15:13:52Z

tests/functional.js -> needs a rename from './functional/verification_experiments', to mailcheck

vladikoff · 2017-02-15T15:19:38Z

functional tests

seeing issues with the password click, try using the following:

        .then(type('.email', BAD_EMAIL))
        .then(click('#fxa-signup-header'))
        .then(click('.tooltip-suggest > span:nth-child(1)'))

vladikoff · 2017-02-21T15:07:28Z

app/scripts/views/sign_up.js

+
+      if (emailValue.length > 0 && mailcheckValue === emailValue) {
+        this.logEvent('mailcheck.corrected');
+      }


@divyabiyani can we extract this logic into its own function?

call it _checkMailcheckResult maybe ?

vladikoff · 2017-02-21T15:09:08Z

app/tests/spec/lib/experiments/mailcheck.js

-          }
-        };
-        notifier.trigger('signup.submit', {}, mockView);
-        assert.isTrue(TestHelpers.isEventLogged(metrics, 'experiment.treatment.mailcheck.corrected'));


@divyabiyani related to https://github.com/mozilla/fxa-content-server/pull/4703/files#r102228528 let's make sure we have a unit test that checks that corrected has been logged. You can add this by unit testing the new "corrected" function (see linked comment).

@vladikoff : I think this function is checking it. https://github.com/divyabiyani/fxa-content-server/blob/mailcheck/app/tests/spec/views/sign_up.js#L1158

@divyabiyani ok perfect, then we can just clean up the code in (so it lives in its own function ) #4703 (comment) and it will already be tested 👍

vladikoff · 2017-02-21T15:42:23Z

tests/functional/mailcheck.js

        .then(type('.email', BAD_EMAIL))
-        .then(click('.password'))
+        .then(click('#age'))


@divyabiyani can you check if adding a .sleep(1000) helps this, maybe there is an issue with tooltip fade on CI, example:

.then(openPage(EXP_MAILCHECK_URL, '#fxa-signup-header')) .then(type('.email', BAD_EMAIL)) .then(click('#age')) // give time for the tooltip to fade in .sleep(1000) .then(click('.tooltip-suggest > span:nth-child(1)'))

vladikoff · 2017-02-23T00:43:33Z

Continued in #4751

rfk added the waffle:active label Feb 7, 2017

divyabiyani force-pushed the divyabiyani:mailcheck branch 2 times, most recently from 314a593 to 6a98c04 Feb 7, 2017

vladikoff reviewed Feb 7, 2017

View changes

vladikoff assigned divyabiyani and vladikoff Feb 7, 2017

divyabiyani force-pushed the divyabiyani:mailcheck branch 7 times, most recently from d664fff to cf001d3 Feb 9, 2017

divyabiyani force-pushed the divyabiyani:mailcheck branch 4 times, most recently from 115ab9d to 3afa544 Feb 13, 2017

vladikoff reviewed Feb 15, 2017

View changes

tests/functional/mailcheck.js

@@ -34,22 +31,12 @@ define([

var CORRECTED_EMAIL = 'something@gmail.com';

This comment has been minimized.

Sign in to view

vladikoff Feb 15, 2017 •

edited

Contributor

rename this from 'treatment works' - tooltip works

divyabiyani force-pushed the divyabiyani:mailcheck branch 5 times, most recently from a543feb to 3c4db31 Feb 20, 2017

vladikoff reviewed Feb 21, 2017

View changes

shane-tomlinson added the WIP label Feb 21, 2017

vladikoff reviewed Feb 21, 2017

View changes

divyabiyani force-pushed the divyabiyani:mailcheck branch 2 times, most recently from 98fbf4d to 556da64 Feb 21, 2017

feat(mailcheck): mailcheck removed from experiment

556da64

rfk added this to the FxA-79: mailcheck pt2 milestone Feb 21, 2017

vladikoff changed the title ~~[WIP]: Enabling mailcheck for all.~~ feat(mailcheck): enabling mailcheck for all. Feb 22, 2017

vladikoff closed this Feb 23, 2017

vladikoff removed the waffle:active label Feb 23, 2017

divyabiyani deleted the divyabiyani:mailcheck branch Feb 28, 2017

mozilla / fxa-content-server Archived

feat(mailcheck): enabling mailcheck for all. #4703

feat(mailcheck): enabling mailcheck for all. #4703

divyabiyani commented Feb 7, 2017 •

edited by rfk

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 15, 2017

vladikoff commented Feb 15, 2017

vladikoff commented Feb 15, 2017

This comment has been minimized.

vladikoff commented Feb 15, 2017

vladikoff commented Feb 15, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Feb 23, 2017

		@@ -55,37 +55,34 @@ define(function (require, exports, module) {

		// user got a suggestion to check their email input
		view.notifier.trigger('mailcheck.suggested');

		@@ -34,22 +31,12 @@ define([
		var CORRECTED_EMAIL = 'something@gmail.com';

mozilla / fxa-content-server Archived

feat(mailcheck): enabling mailcheck for all. #4703

feat(mailcheck): enabling mailcheck for all. #4703

Conversation

divyabiyani commented Feb 7, 2017 • edited by rfk

This comment has been minimized.

vladikoff Feb 7, 2017 Contributor

This comment has been minimized.

vladikoff Feb 7, 2017 Contributor

This comment has been minimized.

vladikoff Feb 7, 2017 Contributor

This comment has been minimized.

vladikoff Feb 7, 2017 Contributor

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 9, 2017

vladikoff commented Feb 15, 2017

tests/spec/views/sign_up

vladikoff commented Feb 15, 2017

vladikoff commented Feb 15, 2017

This comment has been minimized.

vladikoff Feb 15, 2017 • edited Contributor

vladikoff commented Feb 15, 2017

vladikoff commented Feb 15, 2017

functional tests

This comment has been minimized.

vladikoff Feb 21, 2017 Contributor

This comment has been minimized.

vladikoff Feb 21, 2017 Contributor

This comment has been minimized.

vladikoff Feb 21, 2017 Contributor

This comment has been minimized.

divyabiyani Feb 21, 2017 Author Member

This comment has been minimized.

vladikoff Feb 21, 2017 Contributor

This comment has been minimized.

vladikoff Feb 21, 2017 Contributor

vladikoff commented Feb 23, 2017

divyabiyani commented Feb 7, 2017 •

edited by rfk

vladikoff Feb 7, 2017
Contributor

vladikoff Feb 7, 2017
Contributor

vladikoff Feb 7, 2017
Contributor

vladikoff Feb 7, 2017
Contributor

vladikoff Feb 15, 2017 •

edited

Contributor

vladikoff Feb 21, 2017
Contributor

vladikoff Feb 21, 2017
Contributor

vladikoff Feb 21, 2017
Contributor

divyabiyani Feb 21, 2017
Author Member

vladikoff Feb 21, 2017
Contributor

vladikoff Feb 21, 2017
Contributor