1417032 - [Form Autofill] Credit card number is missing after choosing "update" or "create new" in the doorhanger

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[Luke Chang [:lchang] (inactive)]

[Steps:]

1. Create a credit card record with "cc-name" & "cc-number" in the preferences
2. Open any website with a credit card form (e.g. https://luke-chang.github.io/autofill-demo/basic_cc.html)
3. Click "Name" field and trigger autofill dropdown
4. Choose the previously-saved record to autofill it
5. Type a valid number of month (e.g. 3) to "Expiration month" field
6. Click "Submit"
7. Choose "Update Credit Card"
8. Repeat Step 2 ~ 4


[Actual Result:]

Only "cc-name" and "cc-exp-month" were filled. "cc-number" is missing.


[Expected Result:]

"cc-number" should be filled as well.

Comment 1

[Luke Chang [:lchang] (inactive)]

If we change the step 7 to choose "Create New Credit Card", the "cc-number" is still missing in the new created record.

Comment 2

[Luke Chang [:lchang] (inactive)]

The problem is caused by [1] because we use the masked number to replace the incoming one. 

There is also another problem that it will early return incorrectly if one field is marked touched. That causes the untouched fields after it not to be addressed.


[1] https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/browser/extensions/formautofill/FormAutofillParent.jsm#455

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1417032 - Avoid reverting the masked number while saving credit card.

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

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928404 [details]
Bug 1417032 - Avoid reverting the masked number while saving credit card.

https://reviewboard.mozilla.org/r/199628/#review204712

::: browser/extensions/formautofill/FormAutofillParent.jsm:448
(Diff revision 1)
>      if (creditCard.guid) {
>        // Indicate that the user has used Credit Card Autofill to fill in a form.
>        setUsedStatus(3);
>  
>        let originalCCData = this.profileStorage.creditCards.get(creditCard.guid);
>        let unchanged = Object.keys(creditCard.record).every(field => {

The early-return issue hasn't been fixed yet. Are you going to fix it as well?

::: browser/extensions/formautofill/FormAutofillParent.jsm:452
(Diff revision 1)
> -        // Avoid updating the fields that users don't modify.
> +        // Avoid updating the fields that users don't modify, but skip number field
> +        // reversion since it'll need decryption first.

nit: "... but skip number field because we don't want to trigger decryption here."

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:508
(Diff revision 1)
>    is(creditCards[0]["cc-name"], "User 1", "cc-name field is updated");
>    is(SpecialPowers.getIntPref(CREDITCARDS_USED_STATUS_PREF), 3, "User has used autofill");

Probably we should also verify "cc-number" is unchanged here.

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8928404 [details]
Bug 1417032 - Avoid reverting the masked number while saving credit card.

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

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928404 [details]
Bug 1417032 - Avoid reverting the masked number while saving credit card.

https://reviewboard.mozilla.org/r/199628/#review204782

Thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d02cebdf1b3
Avoid reverting the masked number while saving credit card. r=lchang

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7d02cebdf1b3

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8928404 [details]
Bug 1417032 - Avoid reverting the masked number while saving credit card.

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1402963
[User impact if declined]:The saved credit card profile might miss the number in certain situation.
[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]:
Yes

1. Create a credit card record with "cc-name" & "cc-number" in the preferences
2. Open any website with a credit card form (e.g. https://luke-chang.github.io/autofill-demo/basic_cc.html)
3. Click "Name" field and trigger autofill dropdown
4. Choose the previously-saved record to autofill it
5. Type a valid number of month (e.g. 3) to "Expiration month" field
6. Click "Submit"
7. Choose "Update Credit Card" or "Create New Credit Card"
8. Repeat Step 2 ~ 4
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]:Low
[Why is the change risky/not risky?]: It only changed the logic that skips the card number field restoring, and that's what we should do in the beginning.
[String changes made/needed]: N/A

Comment 10

[Gerry Chang [:gchang]]

Hi Gabi, could you help verify if this issue was fixed as expected on the latest Nightly build? Thanks!

Comment 11

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with 59.0a1 20171116100106 on Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8928404 [details]
Bug 1417032 - Avoid reverting the masked number while saving credit card.

Issue fixed and verified. Beta58+.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef08956b714d

Comment 14

[Gabi Cheta [:Gabi] Desktop QA]

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