1402963 - Credit cards need deduplication

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P2)

Description

[Adam Roach [:abr]]

Currently, the credit card manager will ask to save a "new" credit card, even if the number matches an already-saved credit card number. In the case that a number matches, the existing credit card information should be updated (rather than a duplicate entry being created), subject to user confirmation.

Comment 1

[Steve Chung [:steveck]]

We will ignore the saving if user autofill the credit card profile without changing any field, but we didn't handle the credit card number dedup with manual input because of the masterpassword. It seems not possible if we want to merge the profile silently if user has master password set. We'll need to decrypt the number for checking the duplication, and it might trigger the masterpassword prompt if user doesn't login masterpassword yet. Hi Juwei, do you have suggestion about this case?

Comment 2

[Matthew N. [:MattN]]

Deduping using the expiration date, cardholder name, plus the last four digits (not encrypted) may be sufficient.

Comment 3

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #2)
> Deduping using the expiration date, cardholder name, plus the last four
> digits (not encrypted) may be sufficient.

The tricky part there, though, is that the most common update scenario is where the number and name are the same, but the expiration date has changed.

Comment 4

[Matthew N. [:MattN]]

But IIRC we don't have a credit card update doorhanger, only a save one so I don't think that will be a problem here. If any of the 3 fields I mentioned differ (which includes expiration) then we should show the Save doorhanger.

Comment 5

[Adam Roach [:abr]]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> But IIRC we don't have a credit card update doorhanger, only a save one so I
> don't think that will be a problem here. If any of the 3 fields I mentioned
> differ (which includes expiration) then we should show the Save doorhanger.

I think we need to look at a long-term solution here in addition to a short term fix. "We don't have a credit card update doorhanger" is an observation, not a constraint.

Comment 6

[Steve Chung [:steveck]]

Attachment: Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

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

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review189528

::: browser/extensions/formautofill/FormAutofillParent.jsm:421
(Diff revision 1)
>    },
>  
>    async _onCreditCardSubmit(creditCard, target, timeStartedFillingMS) {
>      // We'll show the credit card doorhanger if:
>      //   - User applys autofill and changed
> -    //   - User fills form manually
> +    //   - User fills form manually and the filling data is not duplicated to storage

nit: "... is not a duplicate of any record in storage."

::: browser/extensions/formautofill/FormAutofillParent.jsm:423
(Diff revision 1)
> -        Object.keys(creditCard.record).every(key => creditCard.untouchedFields.includes(key))) {
> +      if (creditCard.record[key] === "") {
> +        return true;
> +      }
> +      return creditCard.untouchedFields.includes(key);

A field with empty value might indicate that it was autofill'ed but cleared afterward. In this case we probably shouldn't treat it as "unchanged".

::: browser/extensions/formautofill/FormAutofillParent.jsm:444
(Diff revision 1)
>      } else {
>        Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_manual", 1);
>        this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
>      }
>  
> +    let isDuplicated = this.profileStorage.creditCards.getAll().some(cc => {

nit: `isDuplicate`

::: browser/extensions/formautofill/FormAutofillParent.jsm:445
(Diff revision 1)
>        Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_manual", 1);
>        this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
>      }
>  
> +    let isDuplicated = this.profileStorage.creditCards.getAll().some(cc => {
> +      if (cc["cc-number"].slice(-4) != creditCard.record["cc-number"].slice(-4)) {

Could you create a function like "getMaskedCCNumber" in "FormAutofillUtils" and use it to compare the entire masked numbers so the length can be taken into account as well?

BTW, I would recommend to leave some comments here explaining why we only compare masked numbers rather than decrypted numbers.

::: browser/extensions/formautofill/FormAutofillParent.jsm:449
(Diff revision 1)
> +    let isDuplicated = this.profileStorage.creditCards.getAll().some(cc => {
> +      if (cc["cc-number"].slice(-4) != creditCard.record["cc-number"].slice(-4)) {
> +        return false;
> +      }
> +      for (let key of ["cc-name", "cc-exp-month", "cc-exp-year"]) {
> +        if (cc[key] && creditCard.record[key] && (cc[key] != creditCard.record[key])) {

Looks like it will be treated as "duplicated" if every records in storage contain "cc-name", "cc-exp-month" and "cc-exp-year" but the incoming record doesn't contain any (or vice versa). Is that intentional?

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review189566

::: browser/extensions/formautofill/FormAutofillParent.jsm:444
(Diff revision 1)
>      } else {
>        Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_manual", 1);
>        this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
>      }
>  
> +    let isDuplicated = this.profileStorage.creditCards.getAll().some(cc => {

On a second thought, I think this function should be implemented in `ProfileStorage` because we should compare two records based on normalized results.

You might need to base on bug 1395122 so we won't get conflicts in the future.

Comment 9

[Steve Chung [:steveck]]

After some discussion with UX, we decide to prevent the duplicate card number saving in this bug, and create another bug for updating the existing credit card profile.

Hi Juwei, it's clear that we won't save the incoming profile if incoming one and existing one are exactly the same, but I'm not sure how we handle the case that the incoming profile has additional field data. Should we ignore the saving as well since the card number is the same, or still showing the notification and user can save as the new one? Just a reminder that we'll handle this case in the credit card update bug eventually, so ignore saving or create new one are just a temporary solution in this bug.

Comment 10

[Juwei Huang[:juwei]]

After discuss with Luke/Steve and consider the scope of MVP, I propose the idea as follows:

1. If users manually fill the cc form with exactly the same data as one of the saved cc profiles, don't pop out the save door hanger and dedupe the data.

2. If users manually fill the cc form and the card number is same as one of the saved cc profiles, pop out the save door hanger and merge the profile.

3. If users fill the form with autofill data and change or add any field, pop out the save door hanger.
   3.1 If the card number is the same as one of the saved cc profiles, merge the profile
   3.2 If the card number is different than any of the saved cc profiles, create a new profile

I believe the cases above covered all the situations we can think of so far.

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

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

Comment 12

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review195322

As our offline discuss, let's define the scope of this bug first.

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

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

Comment 14

[Steve Chung [:steveck]]

Attachment: Bug 1402963 - Part 2: Merge the credit card record into existing data.

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

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

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

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review198400

::: browser/extensions/formautofill/FormAutofillParent.jsm:435
(Diff revision 4)
>  
>    async _onCreditCardSubmit(creditCard, target, timeStartedFillingMS) {
>      // We'll show the credit card doorhanger if:
>      //   - User applys autofill and changed
> -    //   - User fills form manually
> -    if (creditCard.guid &&
> +    //   - User fills form manually and the filling data is not duplicated to storage
> +    // Add the probe to record credit card manual filling/autofill/autofill with modification.

It's a bit unclear to me when reading the comment here without the related code around. I prefer to comment them right aside `Services.telemetry.scalarAdd`.

::: browser/extensions/formautofill/FormAutofillParent.jsm:436
(Diff revision 4)
>    async _onCreditCardSubmit(creditCard, target, timeStartedFillingMS) {
>      // We'll show the credit card doorhanger if:
>      //   - User applys autofill and changed
> -    //   - User fills form manually
> -    if (creditCard.guid &&
> -        Object.keys(creditCard.record).every(key => creditCard.untouchedFields.includes(key))) {
> +    //   - User fills form manually and the filling data is not duplicated to storage
> +    // Add the probe to record credit card manual filling/autofill/autofill with modification.
> +    // Check duplication first since it will normalize credit card record as well.

A stale comment?

::: browser/extensions/formautofill/FormAutofillParent.jsm:439
(Diff revision 4)
> -    //   - User fills form manually
> -    if (creditCard.guid &&
> -        Object.keys(creditCard.record).every(key => creditCard.untouchedFields.includes(key))) {
> +    //   - User fills form manually and the filling data is not duplicated to storage
> +    // Add the probe to record credit card manual filling/autofill/autofill with modification.
> +    // Check duplication first since it will normalize credit card record as well.
> +    let unchanged;
> +    if (creditCard.guid) {
> +      let originalCcData = this.profileStorage.creditCards.get(creditCard.guid);

nit: `originalCCData`

::: browser/extensions/formautofill/FormAutofillParent.jsm:460
(Diff revision 4)
>      } else {
>        Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_manual", 1);
>        this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
>      }
>  
> +    // Early return if data is duplicated and:

nit: "Early return if it's a duplicate data and:"

::: browser/extensions/formautofill/FormAutofillParent.jsm:461
(Diff revision 4)
> +    // - Fill the form manually
> +    // - Autofill the form and form is unchanged
> +    if (this.profileStorage.creditCards.isDuplicate(creditCard.record)) {
> +      if (!creditCard.guid || (creditCard.guid && unchanged)) {

Hasn't the second case been addressed already in the above code?

::: browser/extensions/formautofill/ProfileStorage.jsm:1572
(Diff revision 4)
> +    this._normalizeRecord(targetCreditCard);
> +    return this.data.some(creditCard => {
> +      return this.VALID_FIELDS.every(field => {
> +        if (field == "cc-number") {
> +          return this._getMaskedCCNumber(targetCreditCard[field]) ==
> +                 this._getMaskedCCNumber(creditCard[field]);

`creditCard[field]` should be masked already.

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:108
(Diff revision 4)
> +        await new Promise(resolve => setTimeout(resolve, 1000));
> +        form.querySelector("input[type=submit]").click();
> +      });
> +
> +      await promiseShown;
> +      await clickDoorhangerButton(SECONDARY_BUTTON);

Shouldn't we verify that it will actually be saved?

Comment 17

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

https://reviewboard.mozilla.org/r/192880/#review198410

Looks like the second scenario in comment 10 is not addressed in your patch. Could you comment on bug that which following bugs will you address those scenarios not covered here?

::: browser/extensions/formautofill/ProfileStorage.jsm:1592
(Diff revision 1)
> +   *          Return true if credit card is merged into target with specific guid or false if not.
> +   */
> +  mergeIfPossible(guid, creditCard) {
> +    this.log.debug("mergeIfPossible:", guid, creditCard);
> +
> +    // Get cloned data for comparison

nit: "Query raw data for comparing the decrypted credit card number"

::: browser/extensions/formautofill/ProfileStorage.jsm:1616
(Diff revision 1)
> +    // Early return if the data is the same.
> +    let exactlyMatch = this.VALID_FIELDS.every((field) =>
> +      creditCardFound[field] === creditCardToMerge[field]
> +    );
> +    if (exactlyMatch) {
> +      return true;
> +    }

I can't think of any case that "not exactlyMatch" record will enter this check. Did you write this for the updating scenario?

::: browser/extensions/formautofill/ProfileStorage.jsm:1624
(Diff revision 1)
> +    creditCardFound = this._findByGUID(guid);
> +    for (let field in creditCardToMerge) {
> +      // No need to overwrite number field and encrypt again.
> +      if (this.VALID_FIELDS.includes(field) && field !== "cc-number") {
> +        creditCardFound[field] = creditCardToMerge[field];
> +      }
> +    }

`creditCardToMerge` should be exactly what we want. How about just replacing the record entirely?

::: browser/extensions/formautofill/ProfileStorage.jsm:1626
(Diff revision 1)
> +      return true;
> +    }
> +
> +    creditCardFound = this._findByGUID(guid);
> +    for (let field in creditCardToMerge) {
> +      // No need to overwrite number field and encrypt again.

Since you still need to recompute computed fields (which means the encrypted number will be updated anyway), you don't need to exclude "cc-number" here.

Therefore, following code is still needed.

```js
this._stripComputedFields(creditCardFound);
this._computeFields(creditCardFound);
```

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:291
(Diff revision 1)
> +        let name = form.querySelector("#cc-name");
> +        name.focus();
> +
> +        name.setUserInput("User 0");
> +        form.querySelector("#cc-number").setUserInput("1234123412341234");
> +        form.querySelector("#cc-exp-month").setUserInput("12");

This test fully depends on previous tests. I feel it might be a little bit fragile. Can we reset the storage in each test?

Comment 18

[Luke Chang [:lchang] (inactive)]

(In reply to Luke Chang [:lchang] from comment #17) 
> Looks like the second scenario in comment 10 is not addressed in your patch.
> Could you comment on bug that which following bugs will you address those
> scenarios not covered here?

I remember we had a discussion with UX that we should update the record automatically if the credit card number is identical. e.g.

(in storage) 4444333322221111, 12/2017, John Doe
(user manually typed) 4444333322221111, 4/2022   (note that the card holder name is omitted)

There won't be two records with the same card number. Instead, the incoming record will "merge" into the one in the storage. Therefore, we should have only "4444333322221111, 4/2022, John Doe" saved in the storage.

However, I can't find this scenario in spec now.

Mark, could you confirm this? Thanks.

Comment 19

[Mark Liang(:mark_liang)]

Yes, the example is correct. But I think there should still be a doorhanger as a confirmation for users to select either create a new profile or update the existing profile (if there are changes in other input fields)

Checkout the description here:
https://mozilla.invisionapp.com/share/56E59T04Q

Comment 20

[Luke Chang [:lchang] (inactive)]

Thanks. Let me try to clarify:


When a user manually fills in a record:

1. The incoming record is identical to any of saved records
     => no doorhanger

2. The credit card number of the incoming record is the same as any of saved records but there are some missing fields or additional fields. (e.g. incoming: 4444333322221111, 12/2017; storage: 4444333322221111, John Doe) 
     => show the save doorhanger [a] and merge if user clicks "Save Credit Card" (e.g. 4444333322221111, 12/2017, John Doe)

3. The credit card number of the incoming record is the same as any of saved records but there are some conflicting fields. (e.g. incoming: 4444333322221111, 12/2017, John Doe; storage: 4444333322221111, 4/2022) 
     => show the update doorhanger [b] and update if user clicks "Update Credit Card" (e.g. 4444333322221111, 4/2022, John Doe) or create a new record if user clicks "Create New Credit Card" (means there will be two records with the same numbers)


When a user uses Form Autofill to fill in a record:

4. The incoming record is identical to any of saved records (which means the user doesn't change any of filled fields) 
     => same as 1. (no doorhanger)

5. The credit card number of the incoming record is the same as any of saved records but there are some missing fields (filled by autofill but deleted by the user) or additional fields (not filled by autofill by added by the user manually). 
     => same as 3. (show the update doorhanger [b] and update if user clicks "Update Credit Card" or create a new record if user clicks "Create New Credit Card")

6. The credit card number of the incoming record is the same as any of saved records but there are some conflicting fields (filled by autofill but changed by the user). 
     => same as 3. (show the update doorhanger [b] and update if user clicks "Update Credit Card" or create a new record if user clicks "Create New Credit Card")


[a] the save doorhanger https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/215537981
[b] https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/256376971

Comment 21

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review198408

::: browser/extensions/formautofill/FormAutofillParent.jsm:461
(Diff revision 4)
> +    // - Fill the form manually
> +    // - Autofill the form and form is unchanged
> +    if (this.profileStorage.creditCards.isDuplicate(creditCard.record)) {
> +      if (!creditCard.guid || (creditCard.guid && unchanged)) {

Actually I want to avoid the issue that the form is changed but the masked number is the same, but unchanged is really unnecessary here. Maybe I should simply check the duplication only for manual case, or check the decrypted number for the autofill case.

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:108
(Diff revision 4)
> +        await new Promise(resolve => setTimeout(resolve, 1000));
> +        form.querySelector("input[type=submit]").click();
> +      });
> +
> +      await promiseShown;
> +      await clickDoorhangerButton(SECONDARY_BUTTON);

Maybe I should simply move this test to the 2nd part because it's mergeable(but not be able to be merged in 1st part).

Comment 22

[Luke Chang [:lchang] (inactive)]

(In reply to Luke Chang [:lchang] from comment #20)
> 2. The credit card number of the incoming record is the same as any of saved
> records but there are some missing fields or additional fields. (e.g.
> incoming: 4444333322221111, 12/2017; storage: 4444333322221111, John Doe) 
>      => show the save doorhanger [a] and merge if user clicks "Save Credit
> Card" (e.g. 4444333322221111, 12/2017, John Doe)

On a second thought, there are actually two choices here. One is to actually merge (result in 4444333322221111, 12/2017, John Doe). The other is to replace (result in 4444333322221111, 12/2017). Since we'll do the latter in case 5, they'll have different behaviors if we choose the former in case 2.

In addition, there is a more complicated case. In case 2 & 3, which record should we merge to or update if there are two or more records in storage, which all contain the same credit card numbers as the incoming one?

Comment 23

[Steve Chung [:steveck]]

Just discussed with Mark about other edge cases and he would like to spend more time to think about the proper solution. Here is some note during the discussion:
(In reply to Luke Chang [:lchang] from comment #20)
> Thanks. Let me try to clarify:
> 
> 
> When a user manually fills in a record:
> 
> 1. The incoming record is identical to any of saved records
>      => no doorhanger
> 
> 2. The credit card number of the incoming record is the same as any of saved
> records but there are some missing fields or additional fields. (e.g.
> incoming: 4444333322221111, 12/2017; storage: 4444333322221111, John Doe) 
>      => show the save doorhanger [a] and merge if user clicks "Save Credit
> Card" (e.g. 4444333322221111, 12/2017, John Doe)
> 
> 3. The credit card number of the incoming record is the same as any of saved
> records but there are some conflicting fields. (e.g. incoming:
> 4444333322221111, 12/2017, John Doe; storage: 4444333322221111, 4/2022) 
>      => show the update doorhanger [b] and update if user clicks "Update
> Credit Card" (e.g. 4444333322221111, 4/2022, John Doe) or create a new
> record if user clicks "Create New Credit Card" (means there will be two
> records with the same numbers)
Here we can either show "save" or "update" doorhanger. Update might have more option for user, but it would be am issue if there's 2 profile with same number is storage. We could not know which profile for update. If we decide to update both, it's possible to turn out 2 profile with same data. Create a new one here might not the best solution here, but it could reduce the possibility of duplication(although the possibility is quite low). Right now we will create a new cc profile.
> 
> 
> When a user uses Form Autofill to fill in a record:
> 
> 4. The incoming record is identical to any of saved records (which means the
> user doesn't change any of filled fields) 
>      => same as 1. (no doorhanger)
> 
> 5. The credit card number of the incoming record is the same as any of saved
> records but there are some missing fields (filled by autofill but deleted by
> the user) or additional fields (not filled by autofill by added by the user
> manually). 
>      => same as 3. (show the update doorhanger [b] and update if user clicks
> "Update Credit Card" or create a new record if user clicks "Create New
> Credit Card")
> 
> 6. The credit card number of the incoming record is the same as any of saved
> records but there are some conflicting fields (filled by autofill but
> changed by the user). 
>      => same as 3. (show the update doorhanger [b] and update if user clicks
> "Update Credit Card" or create a new record if user clicks "Create New
> Credit Card")
>
I think it's straightforward if user update the non-cc-number fields or fill a number that is not existed in storage, but there some edge about:
1) Updating the profile that is duplicated to other profile in storage. 
2) Number is duplicated to other profile. Should we update other profiles(we might have multiple profile with same number) instead and remove the current one, or still update the current profile? 

> 
> [a] the save doorhanger
> https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/215537981
> [b] https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/256376971

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183958/diff/4-5/

Comment 25

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

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

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

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

Comment 27

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review199416

r+ with comments.

::: browser/extensions/formautofill/ProfileStorage.jsm:1565
(Diff revision 5)
> +   *        The credit card for duplication checking.
> +   * @returns {boolean}
> +   *          Return true if storage has the same credit card and false otherwise.
> +   */
> +  isDuplicate(targetCreditCard) {
> +    this._normalizeRecord(targetCreditCard);

`targetCreditCard` should be cloned first in order not to expose the changes.

::: browser/extensions/formautofill/ProfileStorage.jsm:1571
(Diff revision 5)
> +        if (!targetCreditCard[field]) {
> +          return !creditCard[field];
> +        }

Although `cc-number` would never be absent, I prefer to check the existence of fields first by putting this check before `cc-number`'s check.

Comment 28

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review198408

> Maybe I should simply move this test to the 2nd part because it's mergeable(but not be able to be merged in 1st part).

I'm fine with that.

Comment 29

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review199470

::: browser/extensions/formautofill/FormAutofillParent.jsm:461
(Diff revision 5)
>        this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
>      }
>  
> +    // Early return if it's a duplicate data
> +    if (this.profileStorage.creditCards.isDuplicate(creditCard.record)) {
> +      return;

We should `notifyUsed` here.

Comment 30

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

https://reviewboard.mozilla.org/r/192880/#review199474

::: browser/extensions/formautofill/ProfileStorage.jsm:1595
(Diff revision 3)
>        });
>      });
>    }
> +
> +  /**
> +   * Merge new credit card into the specified record if mergeable.

nit: We can just define "mergeable" here. e.g. "... if cc-number is identical."

::: browser/extensions/formautofill/ProfileStorage.jsm:1623
(Diff revision 3)
> +      if (!creditCardToMerge[field]) {
> +        creditCardToMerge[field] = existingField;
> +      }
> +
> +      let incomingField = creditCardToMerge[field];
> +      if (!!incomingField && !!existingField) {

nit: I think `if (incomingField && existingField) {` should be fine.

::: browser/extensions/formautofill/ProfileStorage.jsm:1631
(Diff revision 3)
> +    // Early return if the data is the same.
> +    let exactlyMatch = this.VALID_FIELDS.every((field) =>
> +      creditCardFound[field] === creditCardToMerge[field]
> +    );
> +    if (exactlyMatch) {
> +      return true;
> +    }

Do we still need this check?

::: browser/extensions/formautofill/ProfileStorage.jsm:1639
(Diff revision 3)
> +    creditCardFound = this._findByGUID(guid);
> +    this._computeFields(creditCardToMerge);
> +    for (let field in creditCardToMerge) {
> +      creditCardFound[field] = creditCardToMerge[field];
> +    }
> +
> +    creditCardFound.timeLastModified = Date.now();
> +
> +    this._store.saveSoon();
> +    let str = Cc["@mozilla.org/supports-string;1"]
> +                 .createInstance(Ci.nsISupportsString);
> +    str.data = guid;
> +    Services.obs.notifyObservers(str, "formautofill-storage-changed", "merge");

Please use `this.update(guid, creditCardToMerge)` so we can make sure sync fields are updated as well. However, `notifyObservers` may need changes.

BTW, address part might need the same behaviors.

Comment 31

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183958/diff/5-6/

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

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

Comment 33

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

https://reviewboard.mozilla.org/r/183958/#review199840

::: browser/extensions/formautofill/FormAutofillParent.jsm:446
(Diff revisions 5 - 6)
>          return creditCard.untouchedFields.includes(field);
>        });
>  
>        if (unchanged) {
>          // Add probe to record credit card autofill(without modification).
> +        this.profileStorage.creditCards.notifyUsed(creditCard.guid);

This line should be put above the comment of "Add probe".

::: browser/extensions/formautofill/ProfileStorage.jsm:1558
(Diff revisions 5 - 6)
>        }
>      }
>    }
>  
>    /**
> -   * Normailze the given record and check if storage has the same record.
> +   * Normailze the given record and retrun matched guid if storage has the same record.

nit: "... and return the first matched guid ..."

::: browser/extensions/formautofill/ProfileStorage.jsm:1562
(Diff revisions 5 - 6)
>    /**
> -   * Normailze the given record and check if storage has the same record.
> +   * Normailze the given record and retrun matched guid if storage has the same record.
>     * @param {Object} targetCreditCard
>     *        The credit card for duplication checking.
> -   * @returns {boolean}
> -   *          Return true if storage has the same credit card and false otherwise.
> +   * @returns {string|null}
> +   *          Return guid if storage has the same credit card and null otherwise.

nit: "Return the first guid if ..."

::: browser/extensions/formautofill/ProfileStorage.jsm:1565
(Diff revisions 5 - 6)
>     *        The credit card for duplication checking.
> -   * @returns {boolean}
> -   *          Return true if storage has the same credit card and false otherwise.
> +   * @returns {string|null}
> +   *          Return guid if storage has the same credit card and null otherwise.
>     */
> -  isDuplicate(targetCreditCard) {
> -    this._normalizeRecord(targetCreditCard);
> +  getDuplicateGuid(targetCreditCard) {
> +    let targetCreditCardClone = this._clone(targetCreditCard);

nit: `clonedTargetCreditCard` to align with others in ProfileStorage.

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183958/diff/6-7/

Comment 35

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192880/diff/4-5/

Comment 36

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

https://reviewboard.mozilla.org/r/192880/#review199906

The patch itself looks good. Clear r? because I think we should add some unit tests in "test_creditCardRecords.js". Will you add in this commit or a new one?

::: browser/extensions/formautofill/ProfileStorage.jsm:1629
(Diff revision 5)
> +      if (!creditCardToMerge[field]) {
> +        creditCardToMerge[field] = existingField;
> +      }

nit: I just noticed that these lines will be unnecessary if we call `this.update(guid, creditCardToMerge, true)` below. It's up to you though.

::: browser/extensions/formautofill/ProfileStorage.jsm:1634
(Diff revision 5)
> +      if (!creditCardToMerge[field]) {
> +        creditCardToMerge[field] = existingField;
> +      }
> +
> +      let incomingField = creditCardToMerge[field];
> +      if (incomingField && existingField) {

It occurs to me that a field can be a number. We should explicitly compare a field with `undefined` (just like what we did in `AutofillRecords`).

Comment 37

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183958/diff/7-8/

Comment 38

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192880/diff/5-6/

Comment 39

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

https://reviewboard.mozilla.org/r/192880/#review200092

::: browser/extensions/formautofill/FormAutofillParent.jsm:492
(Diff revision 6)
> +    if (creditCard.guid) {
> +      changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));
> +    } else {
> +      changedGUIDs = this.profileStorage.creditCards.mergeToStorage(creditCard.record);
> +      if (!changedGUIDs.length) {
> +        changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));

nit: it's a little bit weird to modify the returned array of `mergeToStorage`. If you use `push` for the other two cases, I prefer to use `changedGUIDs.push(...this.profileStorage.creditCards.mergeToStorage(creditCard.record));` here. Otherwise, simply using `changedGUIDs = [this.profileStorage.creditCards.add(creditCard.record)];` in the other cases can be another option.

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js:397
(Diff revision 6)
> +    if (!testcase.noNeedToUpdate) {
> +      Assert.notEqual(creditCards[0].timeLastModified, timeLastModified);
> +    } else {
> +      Assert.equal(creditCards[0].timeLastModified, timeLastModified);
> +    }
> +    do_check_credit_card_matches(creditCards[0], testcase.expectedCreditCard);

You may want to check the sync metadata here.

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js:420
(Diff revision 6)
> +  do_check_eq(profileStorage.creditCards.mergeIfPossible(guid, anotherCreditCard), false);
> +
> +  // Unable to merge because no credit card number
> +  anotherCreditCard = profileStorage.creditCards._clone(TEST_CREDIT_CARD_1);
> +  anotherCreditCard["cc-number"] = "";
> +  do_check_eq(profileStorage.creditCards.mergeIfPossible(guid, anotherCreditCard), false);

Ditto.

Comment 40

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192880/diff/6-7/

Comment 41

[Steve Chung [:steveck]]

Comment on attachment 8912632 [details]
Bug 1402963 - Part 1: Deduplicate credit card by checking credit card storage and untouched fields.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183958/diff/8-9/

Comment 42

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192880/diff/7-8/

Comment 43

[Steve Chung [:steveck]]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192880/diff/8-9/

Comment 44

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8921841 [details]
Bug 1402963 - Part 2: Merge the credit card record into existing data.

https://reviewboard.mozilla.org/r/192880/#review202134

Looks good. Thanks.

Comment 45

[Steve Chung [:steveck]]

Thanks for the review, the try run looks fine except the tire 2 l10n failure(but everyone got failure on this one as well...).

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3eef0753a078
Part 1: Deduplicate credit card by checking credit card storage and untouched fields. r=lchang
https://hg.mozilla.org/integration/autoland/rev/8dbc7c83b487
Part 2: Merge the credit card record into existing data. r=lchang

Comment 47

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3eef0753a078
https://hg.mozilla.org/mozilla-central/rev/8dbc7c83b487