1109430 - Sync migrator module should show confirmation after resend verification request

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0007-Bug-XXXXXXX-Sync-migrator-module-should-show-confirm.patch

The sync prefs already display a confirmation after a request to resend a verification mail.  The new FxaMigrator module also offers the ability to resend and needs a way to display a confirmation.

This patch moves the strings from a prefs-specific file to the (yet to land!) accounts.properties file, and uses those strings to display a confirmation.  It would be ideal if we could avoid duplicating the boilerplate for putting the dialog together, but there's nowhere obvious to put it.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 8534131 [details] [diff] [review]
0007-Bug-XXXXXXX-Sync-migrator-module-should-show-confirm.patch

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

Looks fine, just a couple of comments.

There are weird rules around strings changes that I admit to not fully understanding.  Since you're moving these existing strings to a new file, do they need to be renamed?  I would hope not, but I don't know.

Now that these strings live in a file called accounts.properties, it would be nice to rename them anyhow to something less redundant, e.g., firefoxAccountsVerificationSentTitle -> verificationSentTitle.  Maybe not worth the localization churn, though, so I'll let you decide if you like that idea.

Comment 2

[Marco Mucci [:MarcoM]]

Hi Mark, should this bug be added to IT 37.2?

Comment 3

[Drew Willcoxon :adw]

I was looking at Ryan's SyncMigration.pdf, the one that shows the flow diagram for migration.  The verification part looks like this:

Pre-verified state --resend--> Email Sent (Web Hosted)

So he's saying that resending the verification email should show the web-hosted "A verification link has been sent" page.

Comment 4

[Drew Willcoxon :adw]

That "verification link has been sent" looks like this BTW: https://www.dropbox.com/s/eith1ey3i8f7295/Email%20Sent.pdf

Also, according to this other PDF, clicking the "[e-mail] not verified" menu panel button should open Sync preferences: https://www.dropbox.com/s/0l8aj6sjgjz0xbw/Sync%20Preverified.pdf

So that's two places where we're doing the wrong thing.

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Marco Mucci [:MarcoM] from comment #2)
> Hi Mark, should this bug be added to IT 37.2?

I guess so!

Comment 6

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: 0002-Bug-1109430-Sync-migrator-module-should-show-confirm.patch

(In reply to Drew Willcoxon :adw from comment #1)
> There are weird rules around strings changes that I admit to not fully
> understanding.  Since you're moving these existing strings to a new file, do
> they need to be renamed?  I would hope not, but I don't know.
> 
> Now that these strings live in a file called accounts.properties, it would
> be nice to rename them anyhow to something less redundant, e.g.,
> firefoxAccountsVerificationSentTitle -> verificationSentTitle.  Maybe not
> worth the localization churn, though, so I'll let you decide if you like
> that idea.

Yeah, I like that idea as it also by-passes the first consideration - so done!

(In reply to Drew Willcoxon :adw from comment #4)

> So that's two places where we're doing the wrong thing.

On IRC we agreed the only thing incorrect is that the hamburger menu should take us to prefs instead of showing the confirmation - that's fixed here.

One other addition is that we didn't do anything when the server failed to resende - in my case it was due to server-side throttling and I'd made too many recent resend requests.  Sadly we can't get a good message to display to the user (the message from the server isn't localized), so in that case the dialog shows with the generic strings:

+verificationNotSentTitle=Unable to Send Verification
+verificationNotSentHeading=We are unable to send a verification mail at this time
+verificationNotSentDescription=Please try again later.

This should obviously only be displayed in edge-cases.  Ryan, I'm flagging you for "review" on these strings (but no need to look at the patch - just signing-off on those strings is sufficient) - are you OK with that wording?

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8535279 [details] [diff] [review]
0002-Bug-1109430-Sync-migrator-module-should-show-confirm.patch

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

::: browser/locales/en-US/chrome/browser/accounts.properties
@@ +9,5 @@
> +# These strings are used in a dialog we display after the user requests we resend
> +# a verification email.
> +verificationSentTitle=Verification Sent
> +# LOCALIZATION NOTE: %S = Email address of user's Firefox Account
> +verificationSentHeading=A verification link has been sent to %S

Nit: The other lines in this file have spaces on either side of the =, so these new ones should too for consistency.

::: services/sync/modules/FxaMigrator.jsm
@@ +368,5 @@
> +    let heading;
> +    if (ok) {
> +      heading = sb.formatStringFromName("verificationSentHeading", [fxauser.email], 1);
> +    } else {
> +      heading = sb.GetStringFromName("verificationNotSentHeading");

Nit: I'd prefer a ternary:

let heading = ok ? sb.format... : sb.Get...

But that's a really picky style preference so I'm not asking you to change it, just offering it as a suggestion.

Comment 8

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Pushed with requested changes.

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

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/a76b8d5ee8dc - one of the two things in that push, or both, was leaking prefs windows in bc1 and bc3

Comment 10

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Repushed - it was bug 1098661 causing the leak.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ea0683e9e5

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22b602570b5b

Comment 12

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8535279 [details] [diff] [review]
0002-Bug-1109430-Sync-migrator-module-should-show-confirm.patch

Ryan said this was fine