949051 - [User Story] FxA - Sign up for Firefox Accounts in Settings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[Adam Rogers (:arog)]

User Story:

As a user, I want to be able to set up a Firefox account from the Settings menu so that I can chose to do this after the First Run Experience, giving me the freedom of choice and making it easy to create an account when I chose.

Acceptance Criteria

1. I can create a new Firefox Account through the settings menu
2. After creating a new account, I am notified that a confirmation email has been sent to me and must configure email and follow the steps in the confirmation email to continue.
3. If my device drops connectivity while in the process of creating an account/signing in, I am made aware the of the lack of connectivity and asked to try again later.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: WIP firefox accounts additions to settings app

kaze, i'm not sure who would be the best to review this, feel free to hand off to whoever is best for settings reviews for 1.4 (maybe evelyn or arthur? not sure).

There will be additional changes landing in the next couple of weeks, as I get UX feedback and as the fxa server changes.

Comment 2

[Fabien Cazenave [:kaze]]

Hi Jared, is your patch ready for review or is it just a WIP for which you want some feedback?

Comment 3

[Fabien Cazenave [:kaze]]

(in both cases, see my comments on your pull request)

Comment 4

[Fabien Cazenave [:kaze]]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

I haven’t checked the JS code yet but I have a couple remarks regarding the CSS and l10n parts. Please check my comments on the pull request and re-flag me when ready — f? for a WIP, r? if your patch is ready.

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

thanks for taking a look, kaze! I'll fix and resubmit.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

kaze - I've updated the PR per your comments.

Let's try for a full review, I will have to make some UX updates, but they are relatively minor, and shouldn't affect the basic JS structure defined by this patch.

Thanks for the feedback, especially for the detailed explanation of how l10n works :-)

Comment 7

[Fabien Cazenave [:kaze]]

Thanks for addressing all my nitpicks! Looking.

Comment 8

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey again - Do you still have time to review this code? It's committed for 1.4 and I'd really like to get some feedback soon, it's my first large gaia commit, and I'd like to have plenty of time for any needed revisions. If you're busy next week, do you think one of the other settings peers might have time? Thanks!

Comment 9

[Maria Angeles Oteo (:oteo)]

sorry, I pressed the incorrect key :(
setting the ni to Kaze again

Comment 10

[Fabien Cazenave [:kaze]]

I’ve spent some time on your patch. It looks much better, though there are still a few CSS and l10n issues to address — see my remarks on your pull request.

My main worry regarding your code so far is that the JS scripts should not be loaded with the index.html file, but only when the “Firefox Accounts” panel is “loaded” (= uncommented and added to the DOM). We sure don’t want to increase the Settings startup time.

Another worry is that I couldn’t create a Firefox Account from the Settings app despite your patch. :-)
Following our discussion on IRC, I probably need a few custom prefs to enable this, please paste them here for further reference. Thanks!

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

kaze pointed out he couldn't get the patch to actually start the fxa flow. We need to document the needed custom prefs somewhere, but for right now, they are:

////// put this in gaia/build/config/custom-prefs.js:

// FxA-related prefs
user_pref("dom.identity.enabled", true);
user_pref("toolkit.identity.debug", true);
user_pref("dom.inter-app-communication-api.enabled", true);
user_pref("dom.identity.syntheticEventsOk", true);

// use the old version of the server until sam has fixed bug 943521
user_pref("identity.fxaccounts.auth.uri", "https://api-accounts-legacy.dev.lcip.org/v1");

Comment 12

[Fabien Cazenave [:kaze]]

To sum up our discussion on IRC and if I understood correctly, we need a simple way to know whether the main “Firefox Accounts” item in the main panel should be displayed or not.

Instead of loading all these JS scripts from the beginning, maybe we could use the `data-show-name` trick that Eitan is adding with bug 942312 to do this. WDYT?

Comment 13

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Well, yes and no :-)

One issue is that we want to be able to pref fxa off, that's correct. This is tracked by bug 964390. ferjm is working on that, and I think he is using precisely the approach you mentioned, the one from bug 942312.

A separate issue is that we want to be able to update the 'firefox accounts' item in the main panel before loading the firefox accounts panel: we need to check what the account state is, then update the menu item accordingly. I'll attach the mocks to this bug so you get an idea of what I mean :-)

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: fxa signed out state in main menu panel

Comment 15

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: fxa unverified state in main menu panel

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

So, if you look at the attachments, you can see that the small text under the menu item changes, depending on your firefox accounts state on the device. In order to show this when the settings app first opens, I need to hook into the same startup loop as wifi and the other components that display status immediately.

If this presents performance challenges, I'm sure we can find an optimization to mitigate it, just let me know how to measure reproducibly, and I"ll try some things :-)

Comment 18

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks for the partial review, do you think you'll have a chance to look at the JS code as well? I am concerned that I still haven't gotten feedback on that. Thanks!

Comment 19

[Fabien Cazenave [:kaze]]

(In reply to Jared Hirsch [:_6a68] from comment #16)
> So, if you look at the attachments, you can see that the small text under
> the menu item changes, depending on your firefox accounts state on the
> device. In order to show this when the settings app first opens, I need to
> hook into the same startup loop as wifi and the other components that
> display status immediately.
> 
> If this presents performance challenges, I'm sure we can find an
> optimization to mitigate it, just let me know how to measure reproducibly,
> and I"ll try some things :-)

The Settings app startup is already much to slow, so we’re going to be very picky about anything that causes any additional regression — especially for a feature that has no “blocking-b2g” flag.

I doubt we really need to load all these scripts just for displaying the FxA status; you should really lazy-load these scripts and maybe think of a way to cache the FxA status for the first startup. That’s what we do for the connectivity settings.

I expect this to be a major refactor your JS code. I can review your code later today but it’s going to be an r- because you’re loading too much JS along with index.html.

There’s a “show time to load” item in the developer settings panel that can help you measure the startup time (make sure you always kill the Settings app before starting it). Please test on one of our supported devices, e.g. Inari or Buri.

Comment 20

[Fabien Cazenave [:kaze]]

Here’s a link that might help seeing why I’m so worried by the regressions in the startup time:
https://datazilla.mozilla.org/b2g/?branch=master&device=hamachi&range=90&test=cold_load_time&app_list=settings&app=settings&gaia_rev=bbe0fd0ca135d089&gecko_rev=c8a8bf353a31f20c&plot=avg

Comment 21

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Kaze - Thanks for the feedback!

Even though my code will be r- for now, I'd still really like your feedback on the bulk of the JS--this is my first large gaia patch, and I'm sure there will be some things I've done incorrectly. I'd like to cover as much ground as possible with each review, so that we can get this thing finished quickly.

About your performance concerns:

I do have a hamachi, so I can measure startup time with and without my code included.

The simplest first step seems to be lazy-loading all the JS inside the panel, and simply not showing the fxa status on first view. This isn't ideal UX, but it's not technically broken. I'm going to take this approach for the current patch.

The next step (which I think should wait for a separate patch) is loading minimal JS and caching the status, as you've suggested, so that the first run displays the status correctly. I've opened bug 972523 to capture further discussion of this feature.

Comment 22

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

OK, forget what I said in the last comment. Refactoring to avoid hurting load time but still make the status work properly. Huge props to mhenretty who explained The Gaia Way to me in detail :-)

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi kaze - reopening for review.

Travis is green but the FxA dev server seems to be down, as I can't actually get the device status from the FxA Test Client app on the device (hitting getAccounts, the spinner just spins and spins). Similarly, I can't complete the signup flow if I start it in the Settings app. I've pinged the dev-fxacct list about this, but it's a holiday weekend in the US, I'm not sure how soon I'll get a response.

I avoided hurting load time by (1) waiting for mozL10n ready inside index.html, then (2) using LazyLoader to fetch JS after the page has settled. I have also refactored the code to load as little as possible inside the index.html context.

I also addressed the l10n bugs and other nits from your previous review.

Comment 24

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Clearing review--it turns out we are using the prod server now, I need to update my handset's gecko to pick up those changes.

Also, now that we don't need to use a special server with fxos devices, the minimal custom-prefs now needed are:
user_pref("dom.identity.enabled", true);
user_pref("toolkit.identity.debug", true);
user_pref("dom.inter-app-communication-api.enabled", true);
user_pref("dom.identity.syntheticEventsOk", true);

Once I verify this is *actually* working on device, I'll r? again.

Comment 25

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Confirmed working on handset, reopening for review.

Re-summarizing: I've fixed all the issues mentioned from the last review, including totally refactoring the patch to lazy-load code that runs inside the main settings panel.

Note: the settings menu item doesn't work right now, because of a race condition in the shared FxAccountsIACHelper. (If you attach a debugger inside the menu_loader.js LazyLoader callback, the delay is long enough that the callbacks attach properly, and you can verify that the DOM updates with correct strings.) ferjm is familiar with the helper code and sam suggested I assign the bug to him, so I have split that into bug 974108.

Comment 26

[Sam Penrose]

Leaving this User Story as is since we tied the PR to it, but marking it as a Gaia implementation blocker so we keep track.

Comment 27

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

hey evelyn - I think kaze might be a bit busy, do you have any time to help out with a pretty large review?

Comment 28

[Fabien Cazenave [:kaze]]

(In reply to Jared Hirsch [:_6a68] from comment #25)
> Note: the settings menu item doesn't work right now, because of a race
> condition in the shared FxAccountsIACHelper. (If you attach a debugger
> inside the menu_loader.js LazyLoader callback, the delay is long enough that
> the callbacks attach properly, and you can verify that the DOM updates with
> correct strings.) ferjm is familiar with the helper code and sam suggested I
> assign the bug to him, so I have split that into bug 974108.

In this case, we’ll probably need to land bug 974108 first. Right?

Comment 29

[Sam Penrose]

(In reply to Fabien Cazenave [:kaze] from comment #28)
> (In reply to Jared Hirsch [:_6a68] from comment #25)
> > Note: the settings menu item doesn't work right now, because of a race
> > condition in the shared FxAccountsIACHelper. (If you attach a debugger
> > inside the menu_loader.js LazyLoader callback, the delay is long enough that
> > the callbacks attach properly, and you can verify that the DOM updates with
> > correct strings.) ferjm is familiar with the helper code and sam suggested I
> > assign the bug to him, so I have split that into bug 974108.
> 
> In this case, we’ll probably need to land bug 974108 first. Right?

We might. In any case, we would be very happy with a conditional r+ (after any changes to this code that you identify). We will not attempt to land this bug with a broken dependency.

Comment 30

[Fabien Cazenave [:kaze]]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

This looks very good, thanks Jared! :-)

We need a fix for bug 974108 before we can test and land this patch, so let’s keep the r? for now. Marking f+ to remember I’m fine with this patch so far.

Comment 31

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

kaze: ferjm did some work to enable us to pref off firefox accounts at build time. i've cherry-picked his commit on top of the existing branch, it's very small and hopefully won't hold things up.

the bug that was tracking the pref was bug 964390, it's closed but contains the discussion.

Aside from that, 974108 is fixed and waiting for your review :-)

Comment 32

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

There's a problem with the fxa_iac_client unit tests. Removing r? until I figure out what's up.

Also clearing out the ni? fields which should be cleared anyway.

Comment 33

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

As suggested in 974108, I've separated the fxa_iac_client.js race condition pull request from this larger settings pull request.

Technically, I think that should make this code r+, but blocked by 974108... ;-)

Comment 39

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

kaze - I've added another commit to this pull request. Now there's the main patch, which you've f+, then a patch from ferjm that adds a UI pref to disable this code, and I've just added [1], a fix for a really little, closely related bug (974185). I'm happy to split this into a separate pull request if you'd rather.

[1] https://github.com/6a68/gaia/commit/976b5090a947f21ddf6fa90e231ec219715d6134

Comment 40

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

hey evelyn - I think kaze might be a bit busy, do you have any time to help out with a pretty large review? or maybe one of the other settings peers has some time?

Comment 41

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Jared, I'll help review the patch.

Comment 42

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Overall it looks good but I've noticed a few bugs. Please check my comments in github. Thanks!

Comment 43

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Hi Arthur!

Resubmitting. Please note that I've removed the 974108 (fxa_iac_client race condition fix) patches from this branch, so you'll need to apply those to test on device.

I've addressed your comments individually in github, thanks so much for the great review feedback. Once the branch is looking good, I'll squash all the commits into a single one.

Comment 44

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Thanks for the patch, we are almost there! There are a few nits that need to be addressed before merging. Please check my github comments.

Comment 45

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Hey Arthur! Thanks again for keeping with this review. Fixed the nits, looks good on device, travis is green. Fingers crossed this can land Monday :-)

Comment 46

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8365339 [details] [review]
WIP firefox accounts additions to settings app

Thank you for the effort!! r=me. Please squash the commits before merging.

Comment 47

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Arthur and kaze, thank you so much for helping me land this enormous patch! :beers:

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/8328ac15d589c213373c0849cfcff9a34e715a25

Comment 49

[Mark Mayo [:mmayo]]

(In reply to Fernando Jiménez Moreno [:ferjm]. from comment #48)
> https://github.com/mozilla-b2g/gaia/commit/
> 8328ac15d589c213373c0849cfcff9a34e715a25

Nice! Thanks :ferjm and :_6a68!

Comment 50

[Mark Mayo [:mmayo]]

(In reply to Mark Mayo [:mmayo] from comment #49)
> (In reply to Fernando Jiménez Moreno [:ferjm]. from comment #48)
> > https://github.com/mozilla-b2g/gaia/commit/
> > 8328ac15d589c213373c0849cfcff9a34e715a25
> 
> Nice! Thanks :ferjm and :_6a68!

Big thanks to :arthurcc and :kaze as well!  :)

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures.
https://github.com/mozilla-b2g/gaia/commit/d2651c13d6370d62bf833b31c7e2e5f054510136

https://tbpl.mozilla.org/php/getParsedLog.php?id=36284955&tree=B2g-Inbound

Comment 52

[Kevin Grandon :kgrandon]

I tried taking a look to see what's wrong with the tests, nothing too apparent. One thing I would try is to disable other tests in the file and ensure your individual tests have a clean setup/teardown. Something about the TBPL sandboxing may be causing this. Sorry I was not of more help.

Comment 53

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

:kgrandon no worries! Thanks a lot for taking a look--at least it's nothing super obvious that I should have caught in the review process :-)

Comment 54

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

RyanVM, I've finally gotten the push-to-try stuff totally sorted. I think.

What's the right incantation to use, to reproduce the test errors you found? I'm currently using:
  -b o -p linux64_gecko,linux32_gecko,macosx64_gecko -u all -t none

Also, I attempted a push last night but got a github timeout--did I miss something in the gaia.json syntax?  https://tbpl.mozilla.org/?tree=Try&rev=39c6a5179490&jobname=b2g_ubuntu64_vm%20try%20opt%20test%20gaia-unit

Comment 55

[Ryan VanderMeulen [:RyanVM]]

The Try syntax looks good to me. I'm not familiar with the needed gaia.json changes, though. Jonathan, do those look ok to you?

Comment 56

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Thanks, Ryan. Clearing ni: my gaia.json syntax looks like the wiki page[1], so I'm hoping it was just a transient infrastructure thing.

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia

Comment 57

[Jonathan Griffin (:jgriffin)]

The syntax is right, it looks like just a network problem.  I've retriggered the build on your try run, we'll see if it gets further this time.

Comment 58

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I've split the tests into individual files, in an effort to isolate the tests from each other. Have to head out for the evening, but hopefully the tests go green this time:

https://tbpl.mozilla.org/?tree=Try&rev=438a4ec8b139
https://github.com/6a68/gaia/tree/bug-949051-fixing-tests-by-splitting

Comment 59

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hmm, all the tests passed. That's odd. Retrying the unedited tests to see if they work:

https://tbpl.mozilla.org/?tree=Try&rev=e6bbe15eedd6

Comment 60

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

And another Try build with the unedited tests, rebased against today's master:

https://tbpl.mozilla.org/?tree=Try&rev=10446ce31b52

Comment 61

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Ugh, this is weird. Because the commit was reverted, the history looks odd.

Cherry-picked the old commit on top of today's master. Resubmitted again to try:

https://tbpl.mozilla.org/?tree=Try&rev=a4e5a852d347
https://github.com/6a68/gaia/tree/bug-949051-fxa-settings-app-resubmitted

Comment 62

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Latest Try build has the G tests green, though Gu are failing due to some timeout (maybe github DDoS drama, idk).

Re-marking as checkin-needed per convo with RyanVM in IRC

Comment 63

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Latest Try build has the G tests green, though Gu are failing due to some timeout (maybe github DDoS drama, idk):

https://tbpl.mozilla.org/?tree=Try&rev=f24d092bcfec

Re-marking as checkin-needed per convo with RyanVM in IRC

Comment 64

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Whoops! Forgot to link to the branch with the new code (because the old PR was closed):

https://github.com/mozilla-b2g/gaia/pull/17454

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/e7c75d3d41719ea5ac20184777874c230b61b3b7

Comment 66

[Francesco Lodolo [:flod]]

fxa-description=Firefox lets you use a single account to log in to services like Marketplace and Where’s My Fox.

Is the brand (Firefox) correct in the Firefox OS context?

Comment 67

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Actually, that string is definitely out of date, since Where's My Fox now seems to be called Find My Device.

There's a bug to cover string review, bug 960130. I'll do string fixes as a sub-bug of that bug.

Comment 69

[No-Jun Park [:njpark]]

Manual Tested Acceptance criteria (take #1), including:
- Does the setting show correct account?
- Can the user log in using existing account?
- Can the user set up a new account, notified of the email verification, and get into verified status once the user verifies?
- When the network connection is down, does it display proper message to the user?
- Does the workflow get stuck in any way?
- Can the user log out from Settings?
- Does the screen display properly?

Version Info:
Gaia      5827cb13b1a033141ce539e8fbd44cd0bf16593d
Gecko     https://hg.mozilla.org/mozilla-central/rev/690c810c8e3e
BuildID   20140410040201
Version   31.0a1

Comment 70

[No-Jun Park [:njpark]]

Test case is in MozTrap:
https://moztrap.mozilla.org/manage/case/12862/

Comment 72

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Screenshot of network offline error (acceptance criteria 3)

Hey Sam - Uploading a screenshot of the "network offline" error that might be difficult to demo using the emulator. Just setting ni? so you notice this in the bugmail flurry :-)

Comment 73

[Sam Penrose]

Thanks Jared!