1357026 - Should let user able to sign in to a Firefox account in the Firefox Sync tour

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Fischer [:Fischer]]

Should let user able to sing-in an Firefox account in the Firefox Sync tour

Comment 1

[[e-mail]]

As discussed yesterday UX come up with a new design contain the email input, and populate the real sign-up form with it.

We can fill the email section in about:accounts without any server-side change, ex:
about:accounts?action=signup&entrypoint=preferences&email=[e-mail]

UITour seems support open about:accounts as well. But can we do those in remote=true window?

@rex could you help check those feasibility? We need to find out if we can do this way so UX can provide the spec ASAP

Comment 2

[KM Lee [:rexboy] (inactivate after Sep. 22)]

See bug 1357023 comment 14.
Feasible with a few line changes on UITour-lib.js and UITour.jsm but we need to check with reviewers.

Comment 3

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1357026 - Open about:accounts with pre-filled email account from UITour.

Review commit: https://reviewboard.mozilla.org/r/149490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149490/

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

See https://bugzilla.mozilla.org/attachment.cgi?id=8877008 for the latest sync tour's spec. (from bug 1354707 comment 25)

After user clicks next, we use UITour to open about:accounts in another window with email address pre-filled.

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Matt seems to be very busy at this time.
Gijs would you help on reviewing this patch?
This patch adds a parameter in showFirefoxAccounts to enable us pre-fill email when opening about:accounts page. And it'll be used at onboarding sync tour's page later.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8878073 [details]
Bug 1357026 - Open about:accounts with pre-filled email account from UITour.

https://reviewboard.mozilla.org/r/149490/#review154110

Comment 7

[:Gijs (he/him)]

Comment on attachment 8878073 [details]
Bug 1357026 - Open about:accounts with pre-filled email account from UITour.

https://reviewboard.mozilla.org/r/149490/#review154180

::: browser/components/uitour/UITour-lib.js:580
(Diff revision 1)
> +   * Mozilla.UITour.showFirefoxAccounts(null, "[e-mail]");
>     */
> -  Mozilla.UITour.showFirefoxAccounts = function(extraURLCampaignParams) {
> +  Mozilla.UITour.showFirefoxAccounts = function(extraURLCampaignParams, email) {
>      _sendEvent("showFirefoxAccounts", {
>        extraURLCampaignParams: JSON.stringify(extraURLCampaignParams),
> +      email: email

Nit: don't need to duplicate this.

::: browser/components/uitour/test/browser_UITour_sync.js:68
(Diff revision 1)
>  });
>  
> +add_UITour_task(async function test_firefoxAccountsValidParams() {
> +  await gContentAPI.showFirefoxAccounts(null, "[e-mail]");
> +  await BrowserTestUtils.browserLoaded(gTestTab.linkedBrowser, false,
> +                                       "about:accounts?action=signup&entrypoint=uitour&email=foo%40bar.com");

It would be nicer if all of these didn't depend on the order in which these params were added, but given that this is a pre-existing problem, we don't need to fix this here...

Also, these tests don't actually check anything directly, only by implication - failure would present as a timeout, which is unfortunate. It would be helpful if here and elsewhere in this file, we at least added some info() statements to indicate what we are waiting for, so that logs will show where the test stalled.

Comment 8

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8878073 [details]
Bug 1357026 - Open about:accounts with pre-filled email account from UITour.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149490/diff/1-2/

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8878073 [details]
Bug 1357026 - Open about:accounts with pre-filled email account from UITour.

https://reviewboard.mozilla.org/r/149490/#review154180

> It would be nicer if all of these didn't depend on the order in which these params were added, but given that this is a pre-existing problem, we don't need to fix this here...
> 
> Also, these tests don't actually check anything directly, only by implication - failure would present as a timeout, which is unfortunate. It would be helpful if here and elsewhere in this file, we at least added some info() statements to indicate what we are waiting for, so that logs will show where the test stalled.

Yeah, I’ve thought about allowing {email: [e-mail]} in the first parameter, but seems in that way it need to couple the checking logic within promotion code’s one.

I just added some info() for tests waiting for browserLoaded(). Thanks for the suggestion!

Comment 10

[:Gijs (he/him)]

Though I did like the 'singing' variation, let's update the summary so we can find it later. ;-)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec461d56ce3a
Open about:accounts with pre-filled email account from UITour. r=Gijs,MattN

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

For verifying, since this patch contains no user interface, I guess we can test it only until bug 1357023 lands.

Comment 14

[Justin [:JW_SoftvisionQA]]

This issue has been verified on Win 10 x64.

Comment 15

[Nazir Ahmed Sabbir [:NaSb]]

I've managed to reproduce this bug on Nightly 55.0a1 (2017-04-17) (64-bit) from Linux x64!

This issue is now verified as fixed on Latest Firefox Nightly 56.0a1 (2017-07-06) (64-bit)

Build ID: 20170706130309
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
OS: Linux 4.8.0-58-generic