1021523 - [Firefox Account] Log in with an existing FxA leads to verification link message

Status: VERIFIED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[Johan Lorenzo [:jlorenzo]]

Build Information

Device: Flame
Gaia      d2cfef555dabab415085e548ed44c48a99be5c32
Gecko     https://hg.mozilla.org/mozilla-central/rev/51b428be6213
BuildID   20140605040202
Version   32.0a1
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

Prerequisites:
Having an existing FxA account.

Steps to Reproduce
1. Go through FTU until FxA page.
2. Log in with your existing account.

Actual Results
After logged in "A verification link has been sent to your@email" message appears. Fortunately, no e-mail was sent.

Reproduction Frequency: 1/1

Comment 1

[Johan Lorenzo [:jlorenzo]]

Nominating as 2.0 blocker as this is a bad first user experience.

Comment 2

[Sam Penrose]

This is a duplicate of bug 994887.

We expect a tiny percentage of users to sign in with an existing account: the subset of sync users who purchase an FxOS device and enter their FxA creds on FTU. Respectfully suggest this should not be blocking-b2g. Could you please let me know whether you would like to reconsider?

Comment 4

[Jason Smith [:jsmith]]

(In reply to Sam Penrose from comment #2)
> This is a duplicate of bug 994887.
> 
> We expect a tiny percentage of users to sign in with an existing account:
> the subset of sync users who purchase an FxOS device and enter their FxA
> creds on FTU. Respectfully suggest this should not be blocking-b2g. Could
> you please let me know whether you would like to reconsider?

I think that's a risky assumption to make. Firefox OS devices can ship far later than when we're finished the release. How do you someone won't setup a FxAccount somewhere else by the time a certain 2.0 device ships in a target market?

To me, this is a basic use case that I think should work.

Comment 5

[Jason Smith [:jsmith]]

This also will fail a factory reset use case, which will be a certification blocker for the release.

Comment 6

[Sam Penrose]

(In reply to Jason Smith [:jsmith] from comment #5)
> This also will fail a factory reset use case, which will be a certification
> blocker for the release.

Thanks for weighing in. I think this last bit is conclusive, and we (Jared and I) should fix this issue because of it.

Comment 7

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

Adam, can you weigh in? Is this bug a release blocker?

Comment 8

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

Hmm, actually, clearing ni? for Adam, I think this has a simple fix.

It looks like we are expecting response to have a `registered` key if the user has an account, I'm not sure whether that's correct or not. I'll do some console.logging tomorrow to see what Gecko returns for existing vs new accounts. Setting ni? in case Sam has seen this in Gecko or knows of a bug in this part of the Gecko API.

- gaia/apps/system/fxa/js/screens/fxam_enter_email.js onNext method calls FxModuleServerRequest.checkEmail, and opens the sign in screen if response && response.registered.
  - This looks like the thing triggering the bug, response.registered seems to never be true.
- gaia/apps/system/fxa/js/fxam_server_request.js checkEmail method calls window.parent.FxAccountsClient.queryAccount
- gaia/apps/system/js/fxa_client.js queryAccount method sends the 'queryAccount` message to chrome

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #7)
> Adam, can you weigh in? Is this bug a release blocker?

Why would we bother exposing UX to sign into existing Firefox Account if we're not going to manage to do the UX flow correctly? To me, this is broken UX. I'd pref off the ability in the settings app to sign into existing account without this fixed.

Comment 11

[Sam Penrose]

Jared: you should be getting {registered: <boolean>} where <boolean> tells us whether the server says there is an acocunt keyed to the submitted email address. Here is the code:

  https://github.com/mozilla/gecko-dev/blob/master/services/fxaccounts/FxAccountsManager.jsm#L402

Comment 12

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

Oh, wait a minute. I misunderstood the bug. So the complaint is that we show the wrong string when you complete the login flow and go back to the fxa screen in ftu.

You go back to ftu and it says, "a verification email has been sent to [e-mail]".

OK, we'll reuse a string or something. Flod, can you flag as late-l10n?

Comment 13

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

Attachment: Github PR 20472

Comment 14

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

Comment on attachment 8439682 [details] [review]
Github PR 20472

Hey guys,

This is a pretty small fxa patch that is blocking 2.0. Do you have time to take a look at it?

The bug is that, when you sign in to firefox accounts in ftu, after the firefox account dialog goes away, we always show the same string in the fxa ftu screen: "A verification link has been sent to {{email}}." This doesn't make sense if you have already verified your email.

I've updated the FxAccountsIACHelper.openFlow success callback to get the current email and verified state (by calling getAccounts), do what we were doing before, but use a different string for verified accounts.

The string was lifted from the settings app, so it's been copy reviewed already.

Thanks!

Jared

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8439682 [details] [review]
Github PR 20472

String looks good to me.

P.S. It seems weird that you can't add a keyword to the bug.

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8439682 [details] [review]
Github PR 20472

Left a tiny comment on github, can we have unit tests for this change?

Thanks!

Comment 17

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

(In reply to Francesco Lodolo [:flod] from comment #15)
> P.S. It seems weird that you can't add a keyword to the bug.

lol, I thought it was one of those dropdown fields. I can definitely add it myself next time :-P

Comment 18

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

Comment on attachment 8439682 [details] [review]
Github PR 20472

Hey Francisco,

Updated, thanks for the review and feedback. I had to unroll the callbacks a bit to make testing easier, so the diff looks bigger than it really is.

Thanks!

Jared

Comment 19

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8439682 [details] [review]
Github PR 20472

Just left a tiny comment on github, but granting the r+ 

Also could you squash your comments in a single one and merge once travis green?

Thanks!

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8439682 [details] [review]
Github PR 20472

w00t!!

Forgot to add the r+ :D

Comment 21

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

Great! Thanks all for the reviews and feedback :-)

Comment 22

[bhavana bajaj [:bajaj]]

Jared,

can you help land this so this can get uplifted to 2.0 asap at it has string changes. Thanks!

Comment 23

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

Hi Bhavana - Yes, absolutely, I'll merge now.

I've verified that this works correctly for both unverified and verified accounts on my Flame.

Note, Travis has been buggy today, timing out during test runs, but I see that the gaia-try run only failed on known false negatives[1]. I am merging rather than continue to wait for Travis to get one all-green run.

[1] https://tbpl.mozilla.org/?tree=Gaia-Try&rev=f02c346b500f7e0f5b8414c7caef336278190da0

Comment 24

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

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/f9dc513538be691f7580b9a145cc4876b78c9782

Comment 26

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/3

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141202000201
Version         32.0

Flame v2.1 version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0