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

VERIFIED FIXED in Firefox OS v2.0

Status

P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jlorenzo, Assigned: _6a68)

Tracking

({late-l10n})

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)
late-l10n

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

Details

(Whiteboard: [2.0-FL-bug-bash] [fxa4fxos2.0] [qa+])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Nominating as 2.0 blocker as this is a bad first user experience.
blocking-b2g: --- → 2.0?

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
Component: Gaia::First Time Experience → FxA

Updated

4 years ago
Whiteboard: [2.0-FL-bug-bash] → [2.0-FL-bug-bash] [fxa4fxos2.0]

Comment 2

4 years ago
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?
Flags: needinfo?(jlorenzo)
Flags: needinfo?(bbajaj)

Updated

4 years ago
Duplicate of this bug: 994887
(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.

Updated

4 years ago
Flags: needinfo?(jlorenzo)
This also will fail a factory reset use case, which will be a certification blocker for the release.

Comment 6

4 years ago
(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.
Flags: needinfo?(bbajaj)

Updated

4 years ago
Assignee: nobody → 6a68
Priority: -- → P2
(Assignee)

Comment 7

4 years ago
Adam, can you weigh in? Is this bug a release blocker?
Flags: needinfo?(arogers)
(Assignee)

Comment 8

4 years ago
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
Flags: needinfo?(arogers) → needinfo?(spenrose)
(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.

Updated

4 years ago
Flags: needinfo?(spenrose)

Updated

4 years ago
Duplicate of this bug: 1009889

Comment 11

4 years ago
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
(Assignee)

Comment 12

4 years ago
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 foo@bar.com".

OK, we'll reuse a string or something. Flod, can you flag as late-l10n?
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 13

4 years ago
Created attachment 8439682 [details] [review]
Github PR 20472
(Assignee)

Comment 14

4 years ago
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
Attachment #8439682 - Flags: review?(francisco)
Attachment #8439682 - Flags: feedback?(francesco.lodolo)
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.
Attachment #8439682 - Flags: feedback?(francesco.lodolo) → feedback+
Flags: needinfo?(francesco.lodolo)
Keywords: late-l10n
Comment on attachment 8439682 [details] [review]
Github PR 20472

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

Thanks!
Attachment #8439682 - Flags: review?(francisco)
(Assignee)

Comment 17

4 years ago
(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
(Assignee)

Comment 18

4 years ago
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
Attachment #8439682 - Flags: review?(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 on attachment 8439682 [details] [review]
Github PR 20472

w00t!!

Forgot to add the r+ :D
Attachment #8439682 - Flags: review?(francisco) → review+
(Assignee)

Comment 21

4 years ago
Great! Thanks all for the reviews and feedback :-)
Jared,

can you help land this so this can get uplifted to 2.0 asap at it has string changes. Thanks!
Flags: needinfo?(6a68)
(Assignee)

Comment 23

4 years ago
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
Flags: needinfo?(6a68)
(Assignee)

Comment 24

4 years ago
Master: https://github.com/mozilla-b2g/gaia/commit/8d50156379b40eaebf1e2a04f2500b52e87c95af
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Whiteboard: [2.0-FL-bug-bash] [fxa4fxos2.0] → [2.0-FL-bug-bash] [fxa4fxos2.0] [qa+]
v2.0: https://github.com/mozilla-b2g/gaia/commit/f9dc513538be691f7580b9a145cc4876b78c9782
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Target Milestone: --- → 2.0 S4 (20june)
Created attachment 8531533 [details]
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.