Closed Bug 949051 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox OS Graveyard :: FxA, defect)

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.0, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: arogers, Assigned: jhirsch)

References

Details

(Whiteboard: [ucid:FxA48, 2.0:p1, ft:FirefoxAccounts][qa+][dependency:Marketplace])

Attachments

(4 files)

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.
Blocks: 941723
Depends on: 936281
Blocks: 936281
No longer depends on: 936281
Depends on: 930074
Depends on: 955945
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.
Attachment #8365339 - Flags: review?(kaze)
Flags: needinfo?(kaze)
Depends on: 952063
Hi Jared, is your patch ready for review or is it just a WIP for which you want some feedback?
Flags: needinfo?(kaze)
(in both cases, see my comments on your pull request)
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.
Attachment #8365339 - Flags: review?(kaze) → review-
thanks for taking a look, kaze! I'll fix and resubmit.
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 :-)
Attachment #8365339 - Flags: review- → review?(kaze)
Thanks for addressing all my nitpicks! Looking.
Blocks: 965492
No longer blocks: 965492
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!
Flags: needinfo?(kaze)
Flags: needinfo?(kaze)
sorry, I pressed the incorrect key :(
setting the ni to Kaze again
Flags: needinfo?(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!
Flags: needinfo?(kaze)
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");
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?
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 :-)
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 :-)
Duplicate of this bug: 964390
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!
Flags: needinfo?(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.
Flags: needinfo?(kaze)
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.
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 :-)
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 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.
Attachment #8365339 - Flags: review?(kaze)
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.
Attachment #8365339 - Flags: review?(kaze)
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.
Blocks: 955952
hey evelyn - I think kaze might be a bit busy, do you have any time to help out with a pretty large review?
Flags: needinfo?(ehung)
(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?
(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 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.
Attachment #8365339 - Flags: feedback+
Depends on: 974108
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 :-)
Flags: needinfo?(kaze)
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.
Attachment #8365339 - Flags: review?(kaze)
Flags: needinfo?(kaze)
Flags: needinfo?(ehung)
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... ;-)
Attachment #8365339 - Flags: review?(kaze)
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
Assignee: nobody → 6a68
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?
Flags: needinfo?(ehung)
Hi Jared, I'll help review the patch.
Flags: needinfo?(ehung)
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!
Attachment #8365339 - Flags: review?(kaze)
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.
Attachment #8365339 - Flags: review?(arthur.chen)
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.
Attachment #8365339 - Flags: review?(arthur.chen)
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 :-)
Attachment #8365339 - Flags: review?(arthur.chen)
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.
Attachment #8365339 - Flags: review?(arthur.chen) → review+
Arthur and kaze, thank you so much for helping me land this enormous patch! :beers:
https://github.com/mozilla-b2g/gaia/commit/8328ac15d589c213373c0849cfcff9a34e715a25
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(In reply to Fernando Jiménez Moreno [:ferjm]. from comment #48)
> https://github.com/mozilla-b2g/gaia/commit/
> 8328ac15d589c213373c0849cfcff9a34e715a25

Nice! Thanks :ferjm and :_6a68!
Component: FxA → Gaia
(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!  :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
: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 :-)
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
Flags: needinfo?(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?
Flags: needinfo?(ryanvm) → needinfo?(jgriffin)
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
Flags: needinfo?(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.
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
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
And another Try build with the unedited tests, rebased against today's master:

https://tbpl.mozilla.org/?tree=Try&rev=10446ce31b52
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
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
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
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
Master: https://github.com/mozilla-b2g/gaia/commit/e7c75d3d41719ea5ac20184777874c230b61b3b7
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
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?
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.
Whiteboard: [ucid:FxA48, 1.4:p1, ft:FirefoxAccounts] → [ucid:FxA48, 1.4:p1, ft:FirefoxAccounts][qa+]
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
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 :-)
Flags: needinfo?(spenrose)
Thanks Jared!
Flags: needinfo?(spenrose)
Whiteboard: [ucid:FxA48, 1.4:p1, ft:FirefoxAccounts][qa+] → [ucid:FxA48, 2.0:p1, ft:FirefoxAccounts][qa+]
Whiteboard: [ucid:FxA48, 2.0:p1, ft:FirefoxAccounts][qa+] → [ucid:FxA48, 2.0:p1, ft:FirefoxAccounts][qa+][dependency:Marketplace]
Flags: in-moztrap?(npark)
Flags: in-moztrap?(npark) → in-moztrap+
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.