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

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

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.

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 "foo@example.com 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.

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

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

Pushed with requested changes. https://hg.mozilla.org/integration/fx-team/rev/1b8f23ce8a7a

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

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 on attachment 8535279 [details] [diff] [review] 0002-Bug-1109430-Sync-migrator-module-should-show-confirm.patch Ryan said this was fine