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

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

Assignee

Description

2 years ago
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.
Whiteboard: [form autofill] → [form autofill:MVP]
Assignee: nobody → schung
Assignee

Comment 1

2 years ago
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
Flags: needinfo?(mliang)
Flags: needinfo?(jhuang)
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.
Flags: needinfo?(mliang)
Flags: needinfo?(jhuang)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 4

2 years ago
mozreview-review
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?
Attachment #8925838 - Flags: review?(lchang)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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.
Attachment #8925838 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
Component: Form Manager → Form Autofill
Assignee

Comment 8

2 years ago
Nit addressed and thanks for the review!
Keywords: checkin-needed

Comment 9

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78acf48d63c3
Add credit card update doorhanger. r=lchang
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/78acf48d63c3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.