1403881 - [Form Autofill] Prompt a door hanger to ask users whether to update a credit card profile when users submit a modified profile

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Steve Chung [:steveck]]

Per discussion in bug 1402963, we'll overwrite data if user submit a modified profile. But ideally we should another update doorhanger for credit card update scenario. Since it'll take additional security/string review process, we create this bug for complete credit card update and then we can land bug 1402963 as early as possible.

Comment 1

[Steve Chung [:steveck]]

Hi UX folks,

Based on the bug 1402963 comment 10 and spec[1], it seems we didn't define the credit card update doorhanger yet. Do you think it's the must for MVP, or it's fine that we can simply leverage the original save doorhanger? If the update spec or string is still pending, we would prefer to move this one out of MVP scope since the deadline is close.

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

Comment 2

[Mark Liang(:mark_liang)]

I think Juwei already updated the spec here: https://mozilla.invisionapp.com/share/GAE8HNL2M
Not sure what's the progress on string review. Since the string is basically the same as the address form update doorhanger I think the current copy should be fine.

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1403881 - Add credit card update doorhanger.

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

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8925838 [details]
Bug 1403881 - Add credit card update doorhanger.

https://reviewboard.mozilla.org/r/197038/#review202754

Looks good. However, we may need another test for: "no doorhanger if an modified autofill'ed record is a duplicate of other records in the storag".

::: browser/extensions/formautofill/FormAutofillParent.jsm:472
(Diff revision 1)
> -    let state = await FormAutofillDoorhanger.show(target, "creditCard");
> +    let state = creditCard.guid ? await FormAutofillDoorhanger.show(target, "updateCreditCard") :
> +                                  await FormAutofillDoorhanger.show(target, "addCreditCard");

nit: I'd prefer `let state = await FormAutofillDoorhanger.show(target, creditCard.guid ? "updateCreditCard" : "addCreditCard");`.

::: browser/extensions/formautofill/FormAutofillParent.jsm:493
(Diff revision 1)
>      let changedGUIDs = [];
> -    // TODO: Autofill(with guid) case should show update doorhanger with update/create new.
> -    // It'll be implemented in bug 1403881 and only avoid mergering for now.
>      if (creditCard.guid) {
> +      if (state == "update") {
> +        this.profileStorage.creditCards.update(creditCard.guid, creditCard.record);

Shouldn't we set `preserveOldProperties` here?

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8925838 [details]
Bug 1403881 - Add credit card update doorhanger.

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

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8925838 [details]
Bug 1403881 - Add credit card update doorhanger.

https://reviewboard.mozilla.org/r/197038/#review202868

::: browser/extensions/formautofill/FormAutofillParent.jsm:473
(Diff revisions 1 - 2)
> -                                  await FormAutofillDoorhanger.show(target, "addCreditCard");
> +                                                  creditCard.guid ?
> +                                                  "updateCreditCard" :
> +                                                  "addCreditCard");

Since we have maxlength=140, I think we can concatenate these lines into one line. What do you think?

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:508
(Diff revisions 1 - 2)
> +      await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
> +      await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
> +      await ContentTask.spawn(browser, null, async function() {
> +        let form = content.document.getElementById("form");
> +        let number = form.querySelector("#cc-number");
> +        number.setUserInput("1111222233334444");

nit: Can we test it's really "1234123412341234" before changed? It also helps people to understand the purpose of this test.

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8925838 [details]
Bug 1403881 - Add credit card update doorhanger.

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

Comment 8

[Steve Chung [:steveck]]

Nit addressed and thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78acf48d63c3
Add credit card update doorhanger. r=lchang

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/78acf48d63c3