1190279 - Hamburger menu is misleading when user is unverified

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 1

[Mark Hammond [:markh] [:mhammond]]

*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.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-working-test-but-still-with-regression.patch

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

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-fix-it.patch

The actual fix (but without the potential string changes I mentioned earlier)

Comment 4

[Edouard Oger [:eoger]]

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.

Comment 5

[Edouard Oger [:eoger]]

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.

Comment 6

[Chris Karlof [:ckarlof]]

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.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

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)

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

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

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

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

Comment 11

[Edouard Oger [:eoger]]

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!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ad37329e81ce

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4070180&repo=fx-team

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a4baa2a12eef

Comment 15

[Mark Hammond [:markh] [:mhammond]]

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

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/0f5e2bbf8c1c

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Pulsebot is a bit slow - relanded as https://hg.mozilla.org/integration/fx-team/rev/a4baa2a12eef

Comment 18

[Mark Hammond [:markh] [:mhammond]]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4baa2a12eef

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!).

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0bbd19cd784a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_fxaccounts.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=1028303&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/a019592053c4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1190279-fix-UI-for-unverified-FxA-users-in-both-.patch

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

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8dfc0d4c01a3

Comment 28

[Paul Silaghi, QA [:pauly]]

(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.

Comment 29

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 30

[Paul Silaghi, QA [:pauly]]

(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.