Luke Chang [:lchang] (inactive) Reporter
	Description • 7 years ago

According to UX spec [1], there should be a checkbox to enable the credit card sync in its door hanger when a user has enabled sync feature. Note that "Don't Save" button in the door hanger should be disabled if a user turns on the credit card sync.


[1] https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/215537981

	Luke Chang [:lchang] (inactive) Reporter
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review183090

Looks good. Thanks.

	Matthew N. [:MattN]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review183860

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:122
(Diff revision 2)
> +        get checked() {
> +          return Services.prefs.getBoolPref("services.sync.engine.creditcards");
> +        },
> +        get label() {
> +          // If sync account is not set, return null label to hide checkbox
> +          return Services.prefs.prefHasUserValue("services.sync.username") ?
> +            GetStringFromName("creditCardSyncCheckbox") : null;
> +        },
> +        callback(event) {
> +          let {secondaryButton, menubutton} = event.rangeParent.parentNode.parentNode;
> +          let checked = event.target.checked;
> +          Services.prefs.setBoolPref("services.sync.engine.creditcards", checked);

I would like Mark to review this patch, specifically the parts dealing with the two prefs to ensure that this is the best way to do this and that it properly handles a user without sync setup or with sync disconnected.

	Mark Hammond [:markh] [:mhammond]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review184010

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:18
(Diff revision 2)
>  changeAutofillOptions = Change Form Autofill Options
>  autofillOptionsLinkOSX = Form Autofill Preferences
>  autofillSecurityOptionsLinkOSX = Form Autofill & Security Preferences
>  changeAutofillOptionsOSX = Change Form Autofill Preferences
>  addressesSyncCheckbox = Share addresses with synced devices
> +creditCardSyncCheckbox = Share credit card with synced devices

The grammar here seems wrong - it should probably be "Share credit cards with synced devices" or similar.

	Luke Chang [:lchang] (inactive) Reporter
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8907515 [details]
Bug 1395028 - Part 2: Add cc doorhanger sync checkbox mochitest.

https://reviewboard.mozilla.org/r/179220/#review184780

This patch reminds me that the sync checkbox seems not necessary if sync is already enabled. Could you confirm this with UX? Thanks.

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:100
(Diff revision 1)
> +      cb.click();
> +      is(SpecialPowers.getBoolPref(SYNC_CREDITCARDS_PREF), true,
> +         "creditCards sync should be enabled after checked");
> +      is(secondaryButton.disabled, true, "Not saving button should be disabled");
> +      is(menuButton.disabled, true, "Never saving menu button should be disabled");

Let's click it again to verify the reverting case.

	Luke Chang [:lchang] (inactive) Reporter
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8907515 [details]
Bug 1395028 - Part 2: Add cc doorhanger sync checkbox mochitest.

https://reviewboard.mozilla.org/r/179220/#review185838

Looks good. Thanks.

	Luke Chang [:lchang] (inactive) Reporter
	Comment 13 • 7 years ago

Please land it after all dependencies land. Thanks.

	Luke Chang [:lchang] (inactive) Reporter
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8922215 [details]
Bug 1395028 - Part 3: Hide the sync checkbox if credit card sync is unavailable.

https://reviewboard.mozilla.org/r/193232/#review198490

Thanks.

	Pulsebot
	Comment 24 • 7 years ago

Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a6350651948
Part 1: Add credit card sync checkbox in its doorhanger. r=lchang,markh
https://hg.mozilla.org/integration/autoland/rev/5ccb5a1de8f6
Part 2: Add cc doorhanger sync checkbox mochitest. r=lchang
https://hg.mozilla.org/integration/autoland/rev/1fc7705bb390
Part 3: Hide the sync checkbox if credit card sync is unavailable. r=lchang

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5a6350651948
https://hg.mozilla.org/mozilla-central/rev/5ccb5a1de8f6
https://hg.mozilla.org/mozilla-central/rev/1fc7705bb390

	Gabi Cheta [:Gabi] Desktop QA
	Comment 26 • 7 years ago

Verified as fixed on 58.0b5 with Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6