Closed
Bug 976633
Opened 10 years ago
Closed 10 years ago
Clicking Cancel on merge warning should not initiate Firefox Account creation
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
Firefox 30
People
(Reporter: jgruen, Assigned: ckarlof)
References
Details
Attachments
(3 files, 2 obsolete files)
310.90 KB,
image/gif
|
Details | |
59.25 KB,
image/gif
|
Details | |
3.29 KB,
patch
|
markh
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Current Behavior: Clicking cancel on merge warning dialogue triggers FxA account creation Expected Behavior: Clicking Cancel on merge warning dialogue should close the dialogue and not change user state. attachments: account created in error.gif > shows bad behavior account created in error pt2.gif > shows same account already existing (this === bad)
Reporter | ||
Comment 1•10 years ago
|
||
Comment 2•10 years ago
|
||
To be clear, no account should be created, but we do briefly show the wrong UI before "resetting" the tab. Did you get an account confirmation email? I thought we had an existing bug about this but can't find it now...
Comment 3•10 years ago
|
||
Oh, second screenshot suggests yes :/
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #2) > To be clear, no account should be created, but we do briefly show the wrong > UI before "resetting" the tab. Did you get an account confirmation email? YES, verification email is sent > > I thought we had an existing bug about this but can't find it now...
Updated•10 years ago
|
Assignee: nobody → ckarlof
Priority: -- → P2
Assignee | ||
Comment 5•10 years ago
|
||
To fix this, we obviously need to do this check before we actually create the account. Proposal: Add an extra about:accounts <-> browser message that triggers this check and returns a success/fail message back to the about:accounts hosted page. If "success" then the page can continue with the creation. The browser side work would involve factoring out the relink warning to a separate call. The hosted page work would involve triggering this call before a creation. I think we can do this without reving the about:accounts <-> browser API version. We just need to land the content server changes first. Thoughts Gavin, Mark? If this approach is agreeable, I can arrange the necessary FxA work.
Flags: needinfo?(mhammond)
Flags: needinfo?(gavin.sharp)
Comment 6•10 years ago
|
||
I see no problem with this approach.
Flags: needinfo?(mhammond)
Flags: needinfo?(gavin.sharp)
Assignee | ||
Comment 7•10 years ago
|
||
I've opened a bug for the required work on the client side: https://github.com/mozilla/fxa-content-server/issues/682
Updated•10 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8390947 -
Flags: feedback?(mhammond)
Assignee | ||
Comment 9•10 years ago
|
||
Hosted code WIP at: https://github.com/mozilla/fxa-content-server/pull/726 I still need to investigate how to modify/add tests for this.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8390949 -
Flags: feedback?(mhammond)
Assignee | ||
Updated•10 years ago
|
Attachment #8390947 -
Attachment is obsolete: true
Attachment #8390947 -
Flags: feedback?(mhammond)
Comment 11•10 years ago
|
||
Comment on attachment 8390949 [details] [diff] [review] 0001-WIP-for-976633.patch Review of attachment 8390949 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +81,5 @@ > return pressed == 0; // 0 is the "continue" button > } > > +function shouldCancelRelink(acctName) { > + return !!(needRelinkWarning(acctName) && !promptForRelink(acctName)); I think it is safe to drop the !! here @@ +142,5 @@ > // but it's a little more seamless to do here, and sync is currently the > // only fxa consumer, so... > let newAccountEmail = accountData.email; > + // The hosted code may have already checked for the relink situation > + // by sending the maybe_show_relink_warning command. If it did, then I'd like to see "maybe_show_relink_warning" renamed to something like "will_accept_new_account" @@ +144,5 @@ > let newAccountEmail = accountData.email; > + // The hosted code may have already checked for the relink situation > + // by sending the maybe_show_relink_warning command. If it did, then > + // it will indicate we don't need to ask twice. > + if (!accountData.skipRelinkWarning && shouldCancelRelink(newAccountEmail)) { and maybe "skipRelinkWarning" to "verifiedWillAcceptNewAccount"? @@ +183,5 @@ > ); > }, > > + onMaybeShowRelinkWarning: function(accountData) { > + // If the last fxa account used for sync isn't this account, we display Instead of duplicating this comment, I'd suggest moving it to the shouldCancelRelink function, and add a single line comment here, and at the other place shouldCancelRelink is called, saying something like "We need to confirm a relink - see shouldCancelRelink for more". Or even just remove the duplicated comment here. But not a big deal if that ends up looking even clunkier.
Attachment #8390949 -
Flags: feedback?(mhammond) → feedback+
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8390949 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8391570 -
Flags: review?(mhammond)
Comment 13•10 years ago
|
||
Comment on attachment 8391570 [details] [diff] [review] 0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch Review of attachment 8391570 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/aboutaccounts/aboutaccounts.js @@ +150,4 @@ > // we need to tell the page we successfully received the message, but > // then bail without telling fxAccounts > this.injectData("message", { status: "login" }); > // and reload the page or else it remains in a "signed in" state. this comment is now stale @@ +186,5 @@ > > + onCanLinkAccount: function(accountData) { > + // We need to confirm a relink - see shouldCancelRelink for more > + let cancel = shouldCancelRelink(accountData.email); > + this.injectData("message", { status: "can_link_account", data: { cancel: cancel } }); It kinda sucks that shouldCancelRelink is named that way - it should be shouldAllowRelink() - but no need to propagate that to this new method - so can we change the 'cancel' value to, say 'ok: !cancel'? Or maybe better: let ok = !shouldCancelRelink... .... {ok: ok}
Attachment #8391570 -
Flags: review?(mhammond) → review+
Assignee | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8aa53970769c
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8aa53970769c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8391570 [details] [diff] [review] 0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): When user is in account creation flow for Sync and cancels out during merge warning, it creates an account for user anyway User impact if declined: Users who cancel will have account created anyway Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch:
Attachment #8391570 -
Flags: approval-mozilla-aurora?
Comment 17•10 years ago
|
||
Comment on attachment 8391570 [details] [diff] [review] 0001-Bug-976633-Clicking-Cancel-on-merge-warning-should-n.patch We merged aurora => beta. So, I guess that applies to beta too.
Attachment #8391570 -
Flags: approval-mozilla-beta+
Attachment #8391570 -
Flags: approval-mozilla-aurora?
Attachment #8391570 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Comment 18•10 years ago
|
||
This is still broken as reported on latest Nightly (20140319). On Cancel to the merge warning, I receive confirmation email and the account is setup. Is there additional client work that is supposed to be done here? question: Is it intended that you can sign in via the set up process? (because you can as long as you have the account and password correct)
Assignee | ||
Comment 19•10 years ago
|
||
The hosted code in about:accounts needs to be modified to support this patch. WIP here: https://github.com/mozilla/fxa-content-server/pull/726 train-07 at the earliest.
Assignee | ||
Comment 20•10 years ago
|
||
I agree the tracking is confusing here. We should have split this into two different Bugzilla bugs. One for the Desktop portion and one for the hosted portion.
Comment 22•10 years ago
|
||
Upon sync data merge cancel, the user is no longer redirected to the account creation form - the get started page is displayed instead. Unfortunately, on the latest Beta (Build ID: 20140324101726) the account is still created, a confirmation email is received. The latest Aurora (Build ID: 20140325004002) and the latest Nightly (Build ID: 20140325030201) are not affected by this issue. I'm also a bit confused about what this bug should actually fix, based on Comment 18 and Comment 20. Chris, could you please clarify this?
Flags: needinfo?(ckarlof)
Comment 23•10 years ago
|
||
Andrei, there is a server side fix to put the finishing touches on this behavior. That fix may be riding the next services train-07. I have my eye on this bug. :-)
Flags: needinfo?(ckarlof)
Assignee | ||
Comment 24•10 years ago
|
||
The server fix missed train-07. I'm pretty sure that Nightly and Aurora are also still affected. I view this as a low impact bug. It will only impact users creating an account in a profile that was previously connected to FxA sync and then deciding to cancel.
Comment 25•10 years ago
|
||
This bug is verified fixed on Firefox 29 (Build ID: 20140421221237) and Aurora 30 (2014-04-28, 20140428004000), using: * Windows 7 64-bit [1], * Ubuntu 13.10 32-bit [2], * Mac OS X 10.9 [3]. 1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 2. Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 3. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•