1390816 - [Form Autofill] Add addresses sync checkbox in first time saving doorhanger

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Steve Chung [:steveck]]

We'll need to add this sync checkbox once the addresses sync is ready.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

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

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Hi Mark, do you think we can determine whether the sync engine is available by checking "services.sync.username"(Services.prefs.prefHasUserValue("services.sync.username")), or is there any other pref that I need to check as well(maybe like "services.sync.engine.addresses.available"? But I think user will not see that doorhanger if services.sync.engine.addresses.available is false)

Comment 3

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

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

That looks fine to me (checking that pref isn't ideal, but it's done in lots of places already). There should be no need to check the .available pref.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

https://reviewboard.mozilla.org/r/169102/#review174756

Comment 5

[Steve Chung [:steveck]]

Attachment: Bug 1390816 - Part 2: Add browser test for sync enabled case.

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

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

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

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

It would be better to also test there's no checkbox in the doorhanger when sync isn't logged in.

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

I already added in the first test (verify if the checkbox is hidden).

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

Oh I see! I misunderstood it. Sorry.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/839885df7077
Part 1: Add addresses sync checkbox in first time saving doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/95b1178330d5
Part 2: Add browser test for sync enabled case. r=lchang

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/839885df7077
https://hg.mozilla.org/mozilla-central/rev/95b1178330d5

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Approval Request Comment
[Feature/Bug causing the regression]: Address autofill with sync
[User impact if declined]: No indication that address sync is an option from the FTU doorhanger
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Verification would be good
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Uses existing checkbox doorhanger APIs
[String changes made/needed]: Yes but these strings are localized on pontoon and not part of the tree so they don't affect string freeze. We also aren't localizing at all for 56.

Comment 13

[Gerry Chang [:gchang]]

Hi flod,
Do you have any concerns about the string change part?

Comment 14

[Gerry Chang [:gchang]]

Hi Vance,
Can you help find a QA to verify this issue?

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Gerry Chang [:gchang] from comment #13)
> Do you have any concerns about the string change part?

No. As Matt explained, at the moment Autofill is not exposed to localization at all, and it will be as an external repo in the future.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Adding to the UI for the form autofill onboarding.
Let's uplift this for beta 7 and verify in beta once it lands.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2c15a6ed11f7
Sorry about the unfortunate landing order of these patches. You can thank our l10n commit hook for that.

https://hg.mozilla.org/releases/mozilla-beta/rev/02852b5a5b3c

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Verified that the check is correctly displayed in the doorhanger using Firefox 56 beta 12 across platforms (macOS 10.12.6, Ubuntu 16.04 32bit and Windows 64bit).