feat(client): email first flow #5177

shane-tomlinson · 2017-06-29T14:36:07Z

The email first flow is available for Sync if the action=email query parameter is specified. The opt-in allows us to release and test without interfering w/ other views & flows. If action=email is not specified and no user is signed in, users should be sent to /signup. If a user is signed in, action is ignored, the user will be sent to /settings.

On the / screen, if the user enters an email that is registered, they will be sent to /signin. If the email isn't registered, they'll be sent to /signup. Note, an email that is registered but has not yet verified will be sent to /signin. The user can reset their password if they really want to re-register.

Once a user is on /signin or /signup, if the user decides they want to change their email, they'll be sent back to / to edit their email. This avoids having to deal with things like signin-from-signup or handling Account doesn't exist errors on the signin page.

I opted to use totally new views instead of the existing SignInView and SignUpView for a couple of reasons. First, this is an experiment. If the experiment fails, I want to be able to easily remove the code. Second, if the experiment succeeds, I want to be able to rip out the old views. There is so much complexity to handle corner cases that'll be avoided with this flow.

If the experiment succeeds, we'll need to update the new signin view to handle OAuth flows and the user's avatar, but that's in the future.

Note, I did not add an A/B test or metrics for this. That'll be a follow on PR, this portion is already large enough.

Note for testing: open https://127.0.0.1:3030/?service=sync&context=fx_firstrun_v2&action=email

fixes #5194

…itialContext. `serviceName` especially was set in all of the views that consumed the ServiceMixin. Might as well reduce duplicate effort and set the value in one place. Extraction from #5177

@philbooth

…itialContext. (#5250) r=@philbooth `serviceName` especially was set in all of the views that consumed the ServiceMixin. Might as well reduce duplicate effort and set the value in one place. Extraction from #5177

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

This extracts the sync suggestion logic from the sign up view into its own mixin so that it can be re-used on the /email page in the email-first flow. This is an extraction from #5177

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

Form elements that have a `name` or `id` attribute and do not have `data-novalue` are eligible for form-prefill. Values are prefilled if: * Above prerequisites are met. * Element does not have an `autocomplete="off"` attribute. * Form prefill model has a value for the field. Values are saved if: * Above prerequisites are met. Element name/id attributes are used as the key into the formPrefill model. Extraction from #5177

…ed. (#5253) r=philbooth The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

Extraction from #5177

Form elements that have a `name` or `id` attribute and do not have `data-novalue` are eligible for form-prefill. Values are prefilled if: * Above prerequisites are met. * Element does not have an `autocomplete="off"` attribute. * Form prefill model has a value for the field. Values are saved if: * Above prerequisites are met. Element name/id attributes are used as the key into the formPrefill model. Extraction from #5177

Extraction from #5177

Form elements that have a `name` or `id` attribute and do not have `data-novalue` are eligible for form-prefill. Values are prefilled if: * Above prerequisites are met. * Element does not have an `autocomplete="off"` attribute. * Form prefill model has a value for the field. Values are saved if: * Above prerequisites are met. Element name/id attributes are used as the key into the formPrefill model. Extraction from #5177

Extraction from #5177

Form elements that have a `name` or `id` attribute and do not have `data-novalue` are eligible for form-prefill. Values are prefilled if: * Above prerequisites are met. * Element does not have an `autocomplete="off"` attribute. * Form prefill model has a value for the field. Values are saved if: * Above prerequisites are met. Element name/id attributes are used as the key into the formPrefill model. Extraction from #5177

Extraction from #5177

@seanmonstar

…eanmonstar * refactor(client): Extract a form-prefill-mixin. Form elements that have a `name` or `id` attribute and do not have `data-novalue` are eligible for form-prefill. Values are prefilled if: * Above prerequisites are met. * Element does not have an `autocomplete="off"` attribute. * Form prefill model has a value for the field. Values are saved if: * Above prerequisites are met. Element name/id attributes are used as the key into the formPrefill model. Extraction from #5177 * fix(form-prefill): Cleanup based on @seanmonstar's feedback. * Remove extra `beforeDestroy` in sign_up.js * Remove extra `password` context value in sign_up.js * Remvoe extra `value={{password}}` in templates.

COPPA was turned into its own view because we had two variations which were A/B tested. This variant won. I want to re-use the COPPA logic in the email-first flow so initially created a mixin that creates the COPPA object. I realized we could do less housekeeping/delegating/event propagation between the consuming object and the COPPA object if the consuming class took care of event handlers itself. Instead of pushing all responsibilities to a separate object, I re-worked it so the consuming object takes over all of the responsibilities via a mixin. Extraction from #5177

shane-tomlinson · 2017-08-01T11:52:59Z

Pulling the WIP off of this. Ready for review. Going to add some test instructions in the description.

shane-tomlinson · 2017-08-01T12:10:31Z

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

@@ -43,7 +43,7 @@ define(function (require, exports, module) {
      },

      setInitialContext (context) {
-        const coppaHTML = this.renderTemplate(Template);
+        const coppaHTML = this.renderTemplate(Template, { required: config.required });


I missed this in the original coppa extraction PR. Added a test for the missing case too.

shane-tomlinson · 2017-08-01T12:11:17Z

app/scripts/views/sign_up.js

@@ -47,10 +47,6 @@ define(function (require, exports, module) {
    template: Template,
    className: 'sign-up',

-    initialize (options = {}) {


This is left over from a previous refactor and is no longer needed.

shane-tomlinson · 2017-08-01T12:12:11Z

app/tests/spec/models/reliers/sync.js

  const CONTEXT = 'fx_desktop_v1';
  const COUNTRY = 'RO';
  const SYNC_MIGRATION = 'sync11';
  const SYNC_SERVICE = 'sync';

-  describe('models/reliers/sync', () => {
+  describe.only('models/reliers/sync', () => {


Ditch the .only

shane-tomlinson · 2017-08-01T16:01:29Z

@mozilla/fxa-devs - r? I left some notes in the top about lack of metrics and experiments. That's the next PR.

vbudhram

@shane-tomlinson Couple of comments, which I don't believe are deal breakers for merge.

No Create account option in Sign In view. For some reason I thought I would see that somewhere.
Mistyped email? was a little confusing on first use since I didn't expect it to go back to original page.
Was surprised that clicking the email field takes you back to input email view.
Not part of this, but create account view doesn't have warning on strong passwords.

Otherwise, this LGTM 👍

vbudhram · 2017-08-01T17:52:34Z

app/tests/spec/models/reliers/sync.js

+          });
+        });
+
+        describe('emtpy', () => {


Seems like a lot of repeatable code, could possibly be replaced with something like this

const cases = [{"name": "invalid", action: "notvalid"}]; cases.forEach((test) => { describe(test.name, () => { //....test stuff } })

Or could do some test helper methods. Or all that might be over kill heh.

Seems like a lot of repeatable code, could possibly be replaced with something like this

Ahh, I like this suggestion. Will try.

vbudhram · 2017-08-01T18:18:10Z

tests/functional/sync_v3_email_first.js

+        // We do not expect the verification poll to occur. The poll
+        // will take a few seconds to complete if it erroneously occurs.
+        // Add an affordance just in case the poll happens unexpectedly.
+        .then(noPageTransition(selectors.CONFIRM_SIGNUP.HEADER, 5000));


Other areas in code use the default timeout when checking for confirm header. Is there a different reason to check for 5 seconds in here?

TBH, I don't remember why they were 5 seconds to begin with and I copied the tests. Shame on me.

shane-tomlinson · 2017-08-01T20:24:37Z

app/scripts/views/sign_up_password.js

+
+    showValidationErrorsEnd () {
+      if (! this._doPasswordsMatch()) {
+        this.displayError(AuthErrors.toError('PASSWORDS_DO_NOT_MATCH'));


Follow @seanmonstar's lead and attach the tooltip to the first password field.

shane-tomlinson · 2017-08-01T20:27:56Z

app/scripts/lib/router.js

+     * @private
+     */
+    _isEmailFirstFlow () {
+      const viewModel = this.getCurrentViewModel();


This makes the assumption that the view the user came from set the emailFirst field on the model. This assumption doesn't always hold, I got into a strange state today going to either /reset_password or /choose_what_to_sync, I'm not sure which one - one of them sent me back to the /signup page w/o specifying emailFirst and I had the old flow.

Once in the emailFirst flow, always in the emailFirst flow.

shane-tomlinson · 2017-08-01T20:30:22Z

app/tests/spec/models/reliers/sync.js

+          });
+        });
+
+        describe('emtpy', () => {


Seems like a lot of repeatable code, could possibly be replaced with something like this

Ahh, I like this suggestion. Will try.

shane-tomlinson · 2017-08-01T20:51:34Z

@shane-tomlinson Couple of comments, which I don't believe are deal breakers for merge.

No Create account option in Sign In view. For some reason I thought I would see that somewhere.

The simplicity is what I like, only show what is needed. We have already captured the user's email address and know that it has been registered before. If they want to sign up, they can change the address.

Mistyped email? was a little confusing on first use since I didn't expect it to go back to original page.

I was wondering about that text too, we use it elsewhere for similar purposes so seemed "good enough for now." Going back to the original screen to edit the email allows for a lot of code simplification, e.g., no "account not registered" and "signin from signup" logic.

Was surprised that clicking the email field takes you back to input email view.

I agree, it seems a little unnatural and part of me wonders if this is because we've been in this world for so long where you can edit the email address on any page. The email address itself used to look totally non-editable. @ryanfeeley suggested making it look like an input box but non-editable, and @davismtl suggested suggested sending users back to / when they click on the email field.

Not part of this, but create account view doesn't have warning on strong passwords.

Oooh, I totally didn't think of this. @ryanfeeley do we want to pull over the strong password warning?

Otherwise, this LGTM 👍

Keep these comments coming! I want this code & flow to be solid!

philbooth · 2017-08-02T12:44:20Z

I opted to use totally new views instead of the existing SignInView and SignUpView for a couple of reasons. First, this is an experiment. If the experiment fails, I want to be able to easily remove the code. Second, if the experiment succeeds, I want to be able to rip out the old views. There is so much complexity to handle corner cases that'll be avoided with this flow.

👍👍

philbooth · 2017-08-02T13:33:48Z

LGTM, r+

vladikoff · 2017-08-02T13:46:29Z

tests/intern_functional_circle.js

@@ -8,6 +8,8 @@ define([
 ], function (intern, selectCircleTests) {

  intern.functionalSuites = selectCircleTests([
+    'tests/functional/sync_v3_email_first',
+    'tests/functional/fx_firstrun_v2_email_first',


@shane-tomlinson we also need to add these tests to other functional files, not just circle ci (so it runs on team city)

@shane-tomlinson we also need to add these tests to other functional files

I totally forgot about the normal list. Thanks for catching this.

vladikoff · 2017-08-02T13:51:23Z

app/scripts/templates/index.mustache

+    <h1 id="fxa-enter-email-header">
+      {{#serviceName}}
+        <!-- L10N: For languages structured like English, the second phrase can read "to continue to %(serviceName)s" -->
+        {{#t}}Enter your email{{/t}} <span class="service">{{#t}}Continue to %(serviceName)s{{/t}}</span>


nit: this is interesting, I guess if I have an FxA account already, I should be entering the email I used with FxA, not one of my others emails. Could be confusing? I hope normal users just have one email or something, or remember the email they used with FxA originally...

@ryanfeeley - thoughts about this?

vladikoff · 2017-08-02T13:53:44Z

app/scripts/templates/index.mustache

+    <div class="error"></div>
+    <div class="success"></div>
+
+    {{{ syncSuggestionHTML }}}


If this action is only for Sync, do you need the sync suggestion here?

Good point. Until yesterday, this behavior wasn't limited to Sync, anybody that went to the /email page. I received some early feedback from @rfk that using a new endpoint didn't seem as awesome as it should be, so I migrated all the code into index.js and isolated the behavior to Sync. I suppose Sync needn't be a restriction, but it seems like an easy way to help us A/B test ensuring a homogeneous group.

I'll remove the Sync suggestion stuff until we need it.

vladikoff · 2017-08-02T13:55:31Z

app/scripts/templates/index.mustache

@@ -0,0 +1,34 @@
+<div id="main-content" class="card">


nit: not sure if index.mustache is a good name for this, could be confusing in the future

nit: not sure if index.mustache is a good name for this, could be confusing in the future

It was originally named email.mustache and I worried that having a different template name to the view (index.js) might be confusing. Think email.mustache is a better name? Think we should rename them both to email.js?

My initial reaction to both index.js and index.mustache was the same. That that name usually respresents the code directory. Then I saw it's had that name from before, and figured I'd just leave my reactions to myself.

vladikoff · 2017-08-02T14:10:15Z

app/scripts/templates/sign_up_password.mustache

+      </div>
+
+      <div class="input-row password-row">
+        <input id="password" type="password" class="password check-password tooltip-below" placeholder="{{#t}}Password{{/t}}" value="{{ password }}" pattern=".{8,}" required autofocus data-form-prefill="password" data-synchronize-show="true" />


I found that if I put a password here, then go back to email (re-render), put a different email in, go back to sign_up then the original password still stays. feature or bug?

@davismtl liked it. Originally I wanted it for users that went to the TOS/PP agreements and came back, or to password reset and came back. We could try clearing form prefill if the user modifies the email on /, I'd like to go for least user surprise TBH.

shane-tomlinson · 2017-08-02T20:45:21Z

There are a couple of problems I noticed while testing tonight:

If the two passwords are different, only edits to the #password field cause the tooltip to go away:

If the user is in the email-first flow on /signup and refreshes, the original SignUpView is used instead of the variant being tested:

* Simplify the unit tests for the Sync relier's `action` check. * Ensure a user that starts in email-first stays in email-first * Add `readySelector` to the `click` functional test method.

* Remove the Sync Suggestion from index.js * Add the new functional test suites to tests/functional.js

Getting the tooltip behavior just right was just turning into too much of a palava.

shane-tomlinson added the waffle:active label Jun 29, 2017

shane-tomlinson self-assigned this Jun 29, 2017

shane-tomlinson force-pushed the email-first branch from d8a13d2 to ed2d61d Compare July 6, 2017 12:21

This was referenced Jul 19, 2017

refactor(client): ServiceMixin sets service, serviceName in setInitialContext. #5250

Merged

feat(signup): add a Confirm Password field #5249

Merged

shane-tomlinson pushed a commit that referenced this pull request Jul 19, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared.

667b3cb

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 19, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared.

d92beaf

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson mentioned this pull request Jul 19, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared. #5253

Merged

shane-tomlinson pushed a commit that referenced this pull request Jul 19, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared.

56f6107

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared.

0c710f3

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shared.

d664a58

The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson mentioned this pull request Jul 20, 2017

refactor(client): Extract a form-prefill-mixin. #5258

Merged

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shar…

42a3219

…ed. (#5253) r=philbooth The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

feat(sync): Extract the Sync Suggestion logic into a mixin to be shar…

225bfda

…ed. (#5253) r=philbooth The Sync suggestion will be showed on the `/email` page in the Email-first flow. Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

refactor(coppa): Make the COPPA view a mixin for easier reuse.

68802a3

Extraction from #5177

shane-tomlinson pushed a commit that referenced this pull request Jul 20, 2017

refactor(coppa): Make the COPPA view a mixin for easier reuse.

8e8250f

Extraction from #5177

shane-tomlinson mentioned this pull request Jul 20, 2017

refactor(coppa): Make the COPPA view a mixin for easier reuse. #5261

Merged

shane-tomlinson pushed a commit that referenced this pull request Jul 24, 2017

refactor(coppa): Make the COPPA view a mixin for easier reuse.

56183d1

Extraction from #5177

shane-tomlinson force-pushed the email-first branch from ef7b3cc to eab66f3 Compare July 24, 2017 13:18

shane-tomlinson pushed a commit that referenced this pull request Jul 24, 2017

refactor(coppa): Make the COPPA view a mixin for easier reuse.

830d55b

Extraction from #5177

shane-tomlinson changed the title ~~[WIP] Email first~~ feat(client): email first flow Aug 1, 2017

shane-tomlinson force-pushed the email-first branch from f53564c to 7e2efd2 Compare August 1, 2017 11:52

shane-tomlinson force-pushed the email-first branch from 7e2efd2 to 353980f Compare August 1, 2017 12:13

shane-tomlinson commented Aug 1, 2017

View reviewed changes

shane-tomlinson force-pushed the email-first branch from 353980f to 79aefa3 Compare August 1, 2017 12:26

shane-tomlinson removed their assignment Aug 1, 2017

shane-tomlinson added waffle:review and removed waffle:active labels Aug 1, 2017

shane-tomlinson force-pushed the email-first branch from 79aefa3 to d08f00c Compare August 1, 2017 14:01

shane-tomlinson force-pushed the email-first branch from d08f00c to 1a65de4 Compare August 1, 2017 16:03

vbudhram reviewed Aug 1, 2017

View reviewed changes

shane-tomlinson commented Aug 1, 2017

View reviewed changes

vladikoff reviewed Aug 2, 2017

View reviewed changes

shane-tomlinson self-assigned this Aug 3, 2017

Shane Tomlinson added 6 commits August 3, 2017 15:20

feat(client): Email first.

f2257dd

fix(email-first): Fixes based on @vbudhram's feedback.

0743df5

* Simplify the unit tests for the Sync relier's `action` check. * Ensure a user that starts in email-first stays in email-first * Add `readySelector` to the `click` functional test method.

fix(email-first): Add the "Strong password" helper to /signup

962a802

fix(email-first): Fixes based on @vladikoff's feedback.

a89be38

* Remove the Sync Suggestion from index.js * Add the new functional test suites to tests/functional.js

fix(email-first): Remove the ENTRYPOINT straggler in index.js

c11b857

fix(email-first): Use the old style "passwords do not match" error.

9d6c54f

Getting the tooltip behavior just right was just turning into too much of a palava.

shane-tomlinson force-pushed the email-first branch from 1463925 to 9d6c54f Compare August 3, 2017 14:23

shane-tomlinson merged commit 5db6d32 into master Aug 3, 2017

shane-tomlinson deleted the email-first branch August 3, 2017 17:29

shane-tomlinson mentioned this pull request Aug 3, 2017

Add an A/B test for email-first #5335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): email first flow #5177

feat(client): email first flow #5177

shane-tomlinson commented Jun 29, 2017 •

edited

Loading

shane-tomlinson commented Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson commented Aug 1, 2017

vbudhram left a comment

vbudhram Aug 1, 2017

shane-tomlinson Aug 1, 2017

vbudhram Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson Aug 1, 2017

shane-tomlinson commented Aug 1, 2017

philbooth commented Aug 2, 2017

philbooth commented Aug 2, 2017

vladikoff Aug 2, 2017

shane-tomlinson Aug 2, 2017

vladikoff Aug 2, 2017

shane-tomlinson Aug 2, 2017

vladikoff Aug 2, 2017

shane-tomlinson Aug 2, 2017

vladikoff Aug 2, 2017

shane-tomlinson Aug 2, 2017

seanmonstar Aug 2, 2017

vladikoff Aug 2, 2017

shane-tomlinson Aug 2, 2017

shane-tomlinson commented Aug 2, 2017

feat(client): email first flow #5177

feat(client): email first flow #5177

Conversation

shane-tomlinson commented Jun 29, 2017 • edited Loading

shane-tomlinson commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-tomlinson commented Aug 1, 2017

vbudhram 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

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

shane-tomlinson commented Aug 1, 2017

philbooth commented Aug 2, 2017

philbooth commented Aug 2, 2017

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

shane-tomlinson commented Aug 2, 2017

shane-tomlinson commented Jun 29, 2017 •

edited

Loading