feat(client): Initial Fennec integration work - the new *_complete screens. #3094

shane-tomlinson · 2015-09-22T12:04:17Z

rfk · 2015-09-22T19:38:29Z

Add the beginning states of capabilities.

Just to ensure situation awareness: are we planning to depend on some minimal capabilities stuff for this integration? I got the impression we were taking that out of the critical path.

shane-tomlinson · 2015-09-22T19:42:38Z

Just to ensure situation awareness: are we planning to depend on some minimal capabilities stuff for this integration? I got the impression we were taking that out of the critical path.

I am fleshing out how the internal API would look, where something akin to capabilities are still useful for determining whether things like the "open preferences" button is supported.

@ncalexan, @vladikoff and I are going to talk about the external/relier API on Thursday after the fxa-web meeting to see if it's necessary for v1. @vladikoff thinks yes. I don't have enough context yet and want more info.

shane-tomlinson · 2015-09-22T19:49:17Z

Add the beginning states of capabilities.

Ref #3077

shane-tomlinson · 2015-09-23T17:52:17Z

app/scripts/models/auth_brokers/fx-fennec-v1.js

+      }
+    }),
+
+    notifyOfSyncPreferences: function () {


Change this to openSyncPreferences and document it. Add a stub to the BaseAuthenticationBroker too.

shane-tomlinson · 2015-09-29T18:11:51Z

This PR breaks the "oauth force_auth with a registered force_email - allows the user to sign in" functional test, not quite ready.

shane-tomlinson · 2015-09-30T20:33:57Z

Removing the WIP, this is ready for review! Should be reviewed after #3118, which contains functional tests to ensure force_auth behavior does not regress for other brokers.

Should be reviewed before #3121, which is a follow on that adds the "choose what to Sync" screen.

vladikoff · 2015-10-01T20:24:44Z

app/scripts/models/auth_brokers/base.js

+       * Should the *_complete pages show a `Sync preferences` button
+       * if the relier is Firefox Sync?
+       */
+      syncPreferencesNotification: false


vladikoff · 2015-10-01T20:29:54Z

app/scripts/templates/ready.mustache

+    {{/isSignIn}}
+    {{#isSignUp}}
+      <h1 id="fxa-sign-up-complete-header">{{#t}}Account verified{{/t}}</h1>
+    {{/isSignUp}}


nit: we might consider moving this out of here into JS logic and just outputting the correct title, the template is looking a little insane.

What I really want to do, and the initial approach I used, is to create distinct templates for all of these things, then choose the correct template depending on the type passed in to initialize. This resulted in 5 nearly identical templates. Template partials would reduce the boilerplate, but we haven't gotten that working with compiled Mustache templates.

Still, maybe that's the correct way to go, because you are right, this is insanity.

vladikoff · 2015-10-01T20:30:40Z

app/scripts/templates/ready.mustache

+      {{/isSignIn}}
+      {{#isSignUp}}
+        <p class="account-ready-service">{{#t}}You are now ready to use %(serviceName)s{{/t}}</p>
+      {{/isSignUp}}


same as https://github.com/mozilla/fxa-content-server/pull/3094/files#r40962304

we should file an issue if you agree

vladikoff · 2015-10-01T20:34:12Z

app/scripts/views/ready.js

@@ -97,7 +119,7 @@ function (Cocktail, FormView, Template, Url, Constants, ServiceMixin,
        type: this.type
      };

-      // spring campaign scheduled to launch 6/2
+      var marketingSnippet;
      if (this._able.choose('springCampaign2015')) {
        marketingSnippet = new MarketingSnippetiOS(marketingSnippetOpts);


side-note: I guess we will need to update this very soon, whenever iOS gets shipped in U.S. 🇺🇸

vladikoff · 2015-10-01T20:44:29Z

Needs rebase

vladikoff · 2015-10-01T20:45:49Z

app/scripts/lib/router.js

@@ -129,6 +129,7 @@ function (
      'confirm_reset_password(/)': showView(ConfirmResetPasswordView),
      'cookies_disabled(/)': showView(CookiesDisabledView),
      'force_auth(/)': showView(ForceAuthView),
+      'force_auth_complete(/)': showView(ReadyView, { type: 'force_auth' }),


this view shows "Your account is ready!" no matter what (with or without existing sessions)

Do we need do any checking / error reporting or is that by design?

Not by design. A well caught overlook.

account_unlock_complete, force_auth_complete, reset_password_complete and signin_complete can all check the user's status since the user should have a sessionToken (or so we say at this point in time). signup_complete may not have a sessionToken if the user verifies in a second browser - the 1st browser shows signup_complete after confirm but has no sessionToken while the second browser receives the sessionToken.

Ok we should open an issue if needed for throwing on no-session access to http://localhost:3030/force_auth_complete

shane-tomlinson · 2015-10-02T16:55:31Z

app/scripts/views/ready.js

+
+  /*eslint-disable camelcase*/
+
+  // The "header" and "ready to sync" messages are generated


@vladikoff - is this any better? The template is definitely cleaner, but now the JS is smelly.

Other alternatives:

One template per screen - pro - fewer switches per screen. con w/o native template support for layout, lots of duplicate HTML across templates. My attempt at a homegrown layout solution was ugly.

One module per screen with the header HTML and ready to sync text as properties of the module. Pro - each module is relatively clean. Con - explosion of modules to take care of strings.

Pass the strings in from router.js as configuration. Pro - ready.js is clean. Con - router.js starts to become dirty.

Thoughts?

See note: https://github.com/mozilla/fxa-content-server/pull/3094/files#r41073115

One template per screen -

I like less HTML

One module per screen

this might work if we can extend "ready" properly

Pass the strings in from router.js

Not sold on doing that, seems like if we add a bit more it won't scale.

The current approach should be okay for now, just not a fan of generating that html in the JS file (per my comment)

vladikoff · 2015-10-02T22:13:31Z

app/scripts/templates/ready.mustache

-    {{#accountUnlock}}
-    <h1 id="fxa-account-unlock-complete-header">{{#t}}Account unlocked{{/t}}</h1>
-    {{/accountUnlock}}
+    {{{headerHTML}}}


@shane-tomlinson generating that html header looks strange, can we change
{{{headerHTML}}} to

<h1 id="{{headerId}}">{{headerTitle}}</h1>

I like this solution, it's a good middle ground. 👏

* Hook up support for the signin_complete and force_auth_complete screens. * Disable support for the signup marketing material. * Add a "Sync preferences" button to the *_complete screens. issue #2771

shane-tomlinson · 2015-10-05T12:45:09Z

@vladikoff - updated w/ your suggestion, thanks!

feat(client): Initial Fennec integration work - the new *_complete screens. r=vladikoff

shane-tomlinson added WIP waffle:wip waffle:review and removed WIP labels Sep 22, 2015

shane-tomlinson added this to the FxA-27: fennec web login milestone Sep 22, 2015

shane-tomlinson force-pushed the issue-2771-fennec branch from 6e00342 to a3666bc Sep 22, 2015

shane-tomlinson mentioned this pull request Sep 22, 2015

refactor(client): Tease appart the FxSync and FxDesktopV1 auth brokers. #3095

Merged

shane-tomlinson force-pushed the issue-2771-fennec branch from a3666bc to 80e897d Sep 23, 2015

shane-tomlinson reviewed Sep 23, 2015
View changes

app/scripts/models/auth_brokers/fx-fennec-v1.js

}

}),

notifyOfSyncPreferences: function () {

This comment has been minimized.

Sign in to view

shane-tomlinson Sep 23, 2015
Author Member

Change this to openSyncPreferences and document it. Add a stub to the BaseAuthenticationBroker too.

shane-tomlinson force-pushed the issue-2771-fennec branch 2 times, most recently from 06849c4 to 71d24e7 Sep 23, 2015

shane-tomlinson added waffle:progress WIP and removed waffle:review waffle:wip labels Sep 24, 2015

shane-tomlinson force-pushed the issue-2771-fennec branch from 71d24e7 to a8d7e85 Sep 29, 2015

shane-tomlinson mentioned this pull request Sep 29, 2015

Fennec integration tracking issue. #2771

Closed

shane-tomlinson mentioned this pull request Sep 30, 2015

feat(test): Add more /force_auth functional tests. #3118

Merged

shane-tomlinson force-pushed the issue-2771-fennec branch 3 times, most recently from eb88785 to fc87fcc Sep 30, 2015

vladikoff assigned shane-tomlinson Sep 30, 2015

shane-tomlinson mentioned this pull request Sep 30, 2015

fennec choose what to sync #3121

Merged

shane-tomlinson changed the title ~~WIP Issue 2771 fennec~~ feat(client): Initial Fennec integration work - the new *_complete screens. Sep 30, 2015

shane-tomlinson removed the WIP label Sep 30, 2015

shane-tomlinson added waffle:review and removed waffle:progress labels Oct 1, 2015

shane-tomlinson force-pushed the issue-2771-fennec branch from ac51c08 to 6eaf88a Oct 1, 2015

vladikoff reviewed Oct 1, 2015
View changes

app/scripts/models/auth_brokers/base.js

* Should the *_complete pages show a `Sync preferences` button

* if the relier is Firefox Sync?

*/

syncPreferencesNotification: false

This comment has been minimized.

Sign in to view

vladikoff Oct 1, 2015
Contributor

👍

vladikoff reviewed Oct 1, 2015
View changes

vladikoff assigned shane-tomlinson and unassigned vladikoff Oct 1, 2015

vladikoff added waffle:progress and removed waffle:review labels Oct 1, 2015

shane-tomlinson force-pushed the issue-2771-fennec branch 3 times, most recently from fdc9e03 to e7b1ab6 Oct 2, 2015

shane-tomlinson reviewed Oct 2, 2015
View changes

vladikoff reviewed Oct 2, 2015
View changes

shane-tomlinson force-pushed the issue-2771-fennec branch 2 times, most recently from 8d5925b to 449d0fa Oct 5, 2015

feat(client): Add support for new fennec screens.

8698f01

* Hook up support for the signin_complete and force_auth_complete screens. * Disable support for the signup marketing material. * Add a "Sync preferences" button to the *_complete screens. issue #2771

shane-tomlinson force-pushed the issue-2771-fennec branch from 449d0fa to 8698f01 Oct 5, 2015

shane-tomlinson assigned vladikoff and unassigned shane-tomlinson Oct 5, 2015

vladikoff added a commit that referenced this pull request Oct 5, 2015

Merge pull request #3094 from mozilla/issue-2771-fennec

5d85abc

feat(client): Initial Fennec integration work - the new *_complete screens. r=vladikoff

vladikoff removed the waffle:progress label Oct 5, 2015

vladikoff deleted the issue-2771-fennec branch Oct 5, 2015

shane-tomlinson mentioned this pull request Oct 5, 2015

Redirect ineligible users from the _complete screens to an auth screen. #3133

Closed

mozilla / fxa-content-server Archived

feat(client): Initial Fennec integration work - the new *_complete screens. #3094

feat(client): Initial Fennec integration work - the new *_complete screens. #3094

shane-tomlinson commented Sep 22, 2015

rfk commented Sep 22, 2015

shane-tomlinson commented Sep 22, 2015

shane-tomlinson commented Sep 22, 2015

This comment has been minimized.

shane-tomlinson commented Sep 29, 2015

shane-tomlinson commented Sep 30, 2015

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 Oct 1, 2015

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.

This comment has been minimized.

shane-tomlinson commented Oct 5, 2015


		/eslint-disable camelcase/

		// The "header" and "ready to sync" messages are generated

mozilla / fxa-content-server Archived

feat(client): Initial Fennec integration work - the new *_complete screens. #3094

feat(client): Initial Fennec integration work - the new *_complete screens. #3094

Conversation

shane-tomlinson commented Sep 22, 2015

rfk commented Sep 22, 2015

shane-tomlinson commented Sep 22, 2015

shane-tomlinson commented Sep 22, 2015

This comment has been minimized.

shane-tomlinson Sep 23, 2015 Author Member

shane-tomlinson commented Sep 29, 2015

shane-tomlinson commented Sep 30, 2015

This comment has been minimized.

vladikoff Oct 1, 2015 Contributor

This comment has been minimized.

vladikoff Oct 1, 2015 Contributor

This comment has been minimized.

shane-tomlinson Oct 2, 2015 Author Member

This comment has been minimized.

shane-tomlinson Oct 2, 2015 Author Member

This comment has been minimized.

vladikoff Oct 1, 2015 Contributor

This comment has been minimized.

vladikoff Oct 1, 2015 Contributor

vladikoff commented Oct 1, 2015

This comment has been minimized.

vladikoff Oct 1, 2015 Contributor

This comment has been minimized.

shane-tomlinson Oct 2, 2015 Author Member

This comment has been minimized.

vladikoff Oct 5, 2015 Contributor

This comment has been minimized.

shane-tomlinson Oct 2, 2015 Author Member

This comment has been minimized.

vladikoff Oct 2, 2015 Contributor

This comment has been minimized.

vladikoff Oct 2, 2015 Contributor

This comment has been minimized.

shane-tomlinson Oct 5, 2015 Author Member

shane-tomlinson commented Oct 5, 2015

shane-tomlinson Sep 23, 2015
Author Member

vladikoff Oct 1, 2015
Contributor

vladikoff Oct 1, 2015
Contributor

shane-tomlinson Oct 2, 2015
Author Member

shane-tomlinson Oct 2, 2015
Author Member

vladikoff Oct 1, 2015
Contributor

vladikoff Oct 1, 2015
Contributor

vladikoff Oct 1, 2015
Contributor

shane-tomlinson Oct 2, 2015
Author Member

vladikoff Oct 5, 2015
Contributor

shane-tomlinson Oct 2, 2015
Author Member

vladikoff Oct 2, 2015
Contributor

vladikoff Oct 2, 2015
Contributor

shane-tomlinson Oct 5, 2015
Author Member