Closed Bug 1190279 Opened 4 years ago Closed 4 years ago

Hamburger menu is misleading when user is unverified

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.3 - Aug 10
Tracking Status
firefox40 --- unaffected
firefox41 --- verified
firefox42 --- verified

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(2 files, 3 obsolete files)

After you create an account but before you verify it, the hamburger menu doesn't change from "Sign in to Sync". It should do what we do for migration, which is to display the string "%S not verified" (where %S is the email address.) For future reference, the string ID used for migration is |needVerifiedUserShort| and I don't see why we can't also use it here.

Ryan, you OK with that, or think something else would be better?
Flags: needinfo?(rfeeley)
*sigh* - so it turns out this *is* a regression - we fail to fetch the profile image with UNVERIFIED_ACCOUNT, then catch that error and update the UI to the default state.

So without that regression the hamburger menu looks exactly the same as if the user is verified. I'll leave the needinfo on Ryan to see if we should just restore that old behaviour, or should update the UI to reflect the fact the user isn't verified.
browser-fxaccounts badly needed tests, so here they are :) They demonstrate the regression (See the commented blocks in the new test)

Of note:

* The FxA "mock" story for browser tests doesn't really exist - we can't either replace the global fxAccounts object nor mock that existing one. So the only way I could see to have profile server tests was to have it do http requests to a local server.

* There's an unrelated profileInfoEnabled check - if our pref disables displaying a profile we should also avoid fetching it in the first place
Attachment #8642415 - Flags: feedback?(edouard.oger)
Attachment #8642415 - Flags: feedback?(ckarlof)
Attached patch 0003-fix-it.patch (obsolete) — Splinter Review
The actual fix (but without the potential string changes I mentioned earlier)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8642416 - Flags: feedback?(edouard.oger)
Comment on attachment 8642416 [details] [diff] [review]
0003-fix-it.patch

Review of attachment 8642416 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: browser/base/content/browser-fxaccounts.js
@@ +318,5 @@
>        updateWithUserData(userData);
> +      if (!userData || !profileInfoEnabled) {
> +        return null; // don't even try to grab the profile.
> +      }
> +      return fxAccounts.getSignedInUserProfile().catch(err => {

Maybe in the future, in order to put all error handling in one place, we could use the "global" catch bellow instead and test the |error| parameter.  I believe that we have a |ERROR_UNVERIFIED_ACCOUNT| const.
Attachment #8642416 - Flags: feedback?(edouard.oger) → feedback+
Comment on attachment 8642415 [details] [diff] [review]
0002-working-test-but-still-with-regression.patch

Review of attachment 8642415 [details] [diff] [review]:
-----------------------------------------------------------------

Much needed tests, I should have written them earlier I think.
Attachment #8642415 - Flags: feedback?(edouard.oger) → feedback+
Comment on attachment 8642415 [details] [diff] [review]
0002-working-test-but-still-with-regression.patch

Review of attachment 8642415 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm happy with this fix as is. It does seem like a good idea to signal the "needs verification" action in the hamburger menu, but I'm indifferent about whether it happens in this bug or not.
Attachment #8642415 - Flags: feedback?(ckarlof) → feedback+
Flags: firefox-backlog+
Priority: -- → P1
Iteration: --- → 42.3 - Aug 10
clearing ni? on rfeeley - let's fix the strings elsewhere if we decide that's what we want to do (but I now recall a discussion with ckarlof or rfkelly that we may end up loosening the restrictions on unverified users such that any such change wouldn't be desired)
Flags: needinfo?(rfeeley)
This patch the same as the 2 previously updated but with 2 changes:

* sync prefs now also listen for ONLOGIN, thus also fixing bug 1079909. Doing it here makes uplift a little bit easier.

* browser-fxaccounts doesn't attempt to fetch the profile for unverified users as that causes log spew with ERROR records, meaning the first sync for most users will end up generating a sync-error-*.log file - which will not be noticed by most, but it's still not great. Sync prefs already does this for the same reasons.

I didn't change the error handling as suggested in comment 4 as I prefer it to be more explicit about knowing exactly what failed when attempting to handle it.

try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a67d35f310
Attachment #8642415 - Attachment is obsolete: true
Attachment #8642416 - Attachment is obsolete: true
Attachment #8642770 - Flags: review?(edouard.oger)
Duplicate of this bug: 1079909
The "legacy" sync test was failing when run as part of the entire suite - it's a PITA to fix and given this is about FxA (which legacy sync doesn't use) I just deleted that test.

New try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=516410a05637
Attachment #8642770 - Attachment is obsolete: true
Attachment #8642770 - Flags: review?(edouard.oger)
Attachment #8642875 - Flags: review?(edouard.oger)
Comment on attachment 8642875 [details] [diff] [review]
0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

Review of attachment 8642875 [details] [diff] [review]:
-----------------------------------------------------------------

Tests looks OK!
Attachment #8642875 - Flags: review?(edouard.oger) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4070180&repo=fx-team
Flags: needinfo?(markh)
Sorry about that - I had another change to add a trailing .catch() in aboutaccounts that I didn't think was strictly necessary for this - but apparently it was. Re-pushed with this change.

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d92779c4acf
Flags: needinfo?(markh)
Pulsebot is a bit slow - relanded as https://hg.mozilla.org/integration/fx-team/rev/a4baa2a12eef
Comment on attachment 8642875 [details] [diff] [review]
0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

Approval Request Comment
[Feature/regressing bug #]: Firefox Accounts info in hamburger menu
[User impact if declined]: When signing up for a Firefox Account and before verifying their email, the state of both the Hamburger menu and Sync Preferences will be wrong, potentially misleading the user.
[Describe test coverage new/current, TreeHerder]: Existing tests pass, landed with new tests.
[Risks and why]: Low risk, limited to the FxA portion of the hamburger menu and the Sync preferences.
[String/UUID change made/needed]: None
Attachment #8642875 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a4baa2a12eef
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Comment on attachment 8642875 [details] [diff] [review]
0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

Approving for uplift to Aurora given that existing tests pass and new tests were added.
Attachment #8642875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting QE team to verify the fix and to ensure there were no unexpected fall outs from the patch (it is a pretty big change!).
Flags: qe-verify+
QA Contact: catalin.varga
Comment on attachment 8642875 [details] [diff] [review]
0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

This is going to miss the Aurora -> Beta uplift at this point. Please re-request Beta approval once you've rebased the patch and confirmed that it passes tests.
Attachment #8642875 - Flags: approval-mozilla-aurora+
This is a version of the patch for beta. The fix itself is the same, but some of the tests had to be neutered for Beta - but the test for this specific issue remains (for better or worse, the original patch introduced a new test that covered more than just this bug)

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a426d76ea0b0

Approval Request Comment
[Feature/regressing bug #]: FxA info in the hamburger menu
[User impact if declined]: The hamburger menu will show the wrong state for an unverified FxA user.
[Describe test coverage new/current, TreeHerder]: New tests and existing tests pass.
[Risks and why]: Very low, limited to the FxA area on the hamburger menu
[String/UUID change made/needed]: None
Attachment #8646143 - Flags: review+
Attachment #8646143 - Flags: approval-mozilla-beta?
Comment on attachment 8646143 [details] [diff] [review]
0001-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

Given that the existing and new automated tests pass, let's uplift to Beta.
Attachment #8646143 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Mark Hammond [:markh] from comment #0)
> It should do what we do for
> migration, which is to display the string "%S not verified" (where %S is the
> email address.)
Is this still expected? Now there is no difference before and after the email verification, the email address alone is displayed in the Hamburger menu.

(In reply to Mark Hammond [:markh] from comment #18)
> the state of both the Hamburger menu and Sync
> Preferences will be wrong
What Sync preferences? The Menu Bar/Tools option remains as "Set Up Sync" until the email is verified.
Tested on FF 41b2, 42.0a2 (2015-08-20) Win 7.
Flags: needinfo?(markh)
(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> (In reply to Mark Hammond [:markh] from comment #0)
> > It should do what we do for
> > migration, which is to display the string "%S not verified" (where %S is the
> > email address.)
> Is this still expected? Now there is no difference before and after the
> email verification, the email address alone is displayed in the Hamburger
> menu.

UX decided to not have a different state between verified and unverified. Before this bug the "unverified" state was the same as "not signed in" - now it should be the same as "verified".

> (In reply to Mark Hammond [:markh] from comment #18)
> > the state of both the Hamburger menu and Sync
> > Preferences will be wrong
> What Sync preferences? The Menu Bar/Tools option remains as "Set Up Sync"
> until the email is verified.
> Tested on FF 41b2, 42.0a2 (2015-08-20) Win 7.

about:preferences#sync before this bug also shows the same for "unverified" as for "not signed in" - now it has a distinct state for unverified which includes the ability to resend the verification mail.
Flags: needinfo?(markh)
(In reply to Mark Hammond [:markh] from comment #29)
> about:preferences#sync before this bug also shows the same for "unverified"
> as for "not signed in" - now it has a distinct state for unverified which
> includes the ability to resend the verification mail.
Note that the new about:preferences#sync UI didn't go in FF 41 beta. It only applies to FF 42 and up.
Also note the Menu Bar/Tools option that shows the same ("Set Up Sync") for "unverified" as for "not signed in".
Except these, we can mark this as verified fixed. Feel free to reopen if the above statements are not expected.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.