1371145 - [Form Autofill] Properly fill cc-exp/cc-exp-month/cc-exp-year into a select element

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Luke Chang [:lchang] (inactive)]

People usually use <select> elements to implement "cc-exp/cc-exp-month/cc-exp-year" fields in practice so we need to handle inexact matches here.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 2

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review173268

Are you going to handle months without numbers? BTW, do you think if we need to handle `cc-exp` that uses "01/25" or "0125"?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:223
(Diff revision 1)
> +    if (!("cc-exp" in profile) && ("cc-exp-month" in profile) && ("cc-exp-year" in profile)) {
> +      fieldNames.push("cc-exp");
> +    }

To align with others, I would prefer to compute `cc-exp` in `ProfileStorage` (in `CreditCards._computeFields()`).

::: browser/extensions/formautofill/FormAutofillUtils.jsm:369
(Diff revision 1)
>    },
>  
> +  findCreditCardSelectOption(selectEl, creditCard, fieldName) {
> +    let oneDigitMonth = parseInt(creditCard["cc-exp-month"], 10).toString();
> +    let twoDigitsMonth = oneDigitMonth.length < 2 ? "0" + oneDigitMonth : oneDigitMonth;
> +    let expYearInt = parseInt(creditCard["cc-exp-year"], 10);

`cc-exp-year` will be normalized to 4 digits in storage upon saving so you should treat it as 4 digits here.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:378
(Diff revision 1)
> +
> +    switch (fieldName) {
> +      case "cc-exp-month": {
> +        for (let pattern of [twoDigitsMonth, oneDigitMonth]) {
> +          let matchedOption = options.find(o => {
> +            return [o.text, o.label, o.value].some(s => s.startsWith(pattern));

`startsWith` isn't accurate as it matches "1" with "12".

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:21
(Diff revision 1)
>    "country": "US",
>  };
>  
> +const DEFAULT_CREDITCARD_RECORD = {
> +  "guid": "123",
> +  "cc-exp-month": "09",

No leading zero.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:22
(Diff revision 1)
>  };
>  
> +const DEFAULT_CREDITCARD_RECORD = {
> +  "guid": "123",
> +  "cc-exp-month": "09",
> +  "cc-exp-year": "25",

It is expected to be 4 digits.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:297
(Diff revision 1)
> +      "cc-exp-month": "option-cc-exp-month-09",
> +      "cc-exp-year": "option-cc-exp-year-25",
> +    }],
> +  },
> +  {
> +    description: "Credit Card form with matching options which contain labels",

Not sure your purpose of this task since it should pass without `label`, right?

Comment 3

[Luke Chang [:lchang] (inactive)]

(In reply to Luke Chang [:lchang] from comment #2)
> ::: browser/extensions/formautofill/FormAutofillHandler.jsm:223
> (Diff revision 1)
> > +    if (!("cc-exp" in profile) && ("cc-exp-month" in profile) && ("cc-exp-year" in profile)) {
> > +      fieldNames.push("cc-exp");
> > +    }
> 
> To align with others, I would prefer to compute `cc-exp` in `ProfileStorage`
> (in `CreditCards._computeFields()`).

Also, you need to address the normalization of `cc-exp` (split it into `cc-exp-month` and `cc-exp-year`) upon saving.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review173268

> To align with others, I would prefer to compute `cc-exp` in `ProfileStorage` (in `CreditCards._computeFields()`).

I would like to do this here because the cc-exp format really depends on the web sites. That means the calcuated result is useless for another web sites.
Even we only support this format: 12/2025 currently, it's still possible to handle the other cases (e.g. 12/25, 12-25, 12-2025) for other sites.

> `startsWith` isn't accurate as it matches "1" with "12".

This won't happen if we compare twoDigitsMonth first then oneDigitMonth.

> Not sure your purpose of this task since it should pass without `label`, right?

hmm, I agree. I copied this case from `third_party/HomeDepot/Checkout_ShippingPayment.html`, and I will refine it to make the coverage more clear.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review173268

> This won't happen if we compare twoDigitsMonth first then oneDigitMonth.

I use another approach to handle this issue.
The strings from `<option>` can be parsed then compare to the month value in integer.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review173268

> I would like to do this here because the cc-exp format really depends on the web sites. That means the calcuated result is useless for another web sites.
> Even we only support this format: 12/2025 currently, it's still possible to handle the other cases (e.g. 12/25, 12-25, 12-2025) for other sites.

You're right about this matching function. Though what I was saying is that we should have an entry of "cc-exp" in profile directly rather than appending it here. Otherwise, we need to append it everytime when we need `Object.keys(profile)`.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 12

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review175704

::: browser/extensions/formautofill/FormAutofillUtils.jsm:368
(Diff revision 6)
>      return null;
>    },
>  
> +  findCreditCardSelectOption(selectEl, creditCard, fieldName) {
> +    let expMonthInt = parseInt(creditCard["cc-exp-month"], 10);
> +    let oneDigitMonth = expMonthInt.toString();

nit: `creditCard["cc-exp-month"]` is one-digit format already.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:369
(Diff revision 6)
>    },
>  
> +  findCreditCardSelectOption(selectEl, creditCard, fieldName) {
> +    let expMonthInt = parseInt(creditCard["cc-exp-month"], 10);
> +    let oneDigitMonth = expMonthInt.toString();
> +    let twoDigitsMonth = oneDigitMonth.length < 2 ? "0" + oneDigitMonth : oneDigitMonth;

nit: `oneDigitMonth.padStart(2, "0")`

::: browser/extensions/formautofill/FormAutofillUtils.jsm:371
(Diff revision 6)
> +  findCreditCardSelectOption(selectEl, creditCard, fieldName) {
> +    let expMonthInt = parseInt(creditCard["cc-exp-month"], 10);
> +    let oneDigitMonth = expMonthInt.toString();
> +    let twoDigitsMonth = oneDigitMonth.length < 2 ? "0" + oneDigitMonth : oneDigitMonth;
> +    let expYearInt = parseInt(creditCard["cc-exp-year"], 10);
> +    let twoDigitsYear = (expYearInt - 2000).toString();

Although it's unlikely to happen in the near future, I prefer using `.substr(2, 2)` instead of the magic number "2000".

::: browser/extensions/formautofill/FormAutofillUtils.jsm:379
(Diff revision 6)
> +
> +    switch (fieldName) {
> +      case "cc-exp-month": {
> +        for (let option of options) {
> +          if ([option.text, option.label, option.value].some(
> +            s => parseInt(s, 10) == expMonthInt

Recommend using `/\d+/` so we can handle "Sep-09" well.

Moreover, I would write like this:

```js
let result = /[1-9]\d*/.exec(s);
return result && result[0] == oneDigitMonth;
```

(And you no longer need `expMonthInt`.)

::: browser/extensions/formautofill/FormAutofillUtils.jsm:397
(Diff revision 6)
> +        let patterns = [
> +          oneDigitMonth + "/" + twoDigitsYear,    // 8/22
> +          oneDigitMonth + "/" + fourDigitsYear,   // 8/2022
> +          twoDigitsMonth + "/" + twoDigitsYear,   // 08/22
> +          twoDigitsMonth + "/" + fourDigitsYear,  // 08/2022
> +          oneDigitMonth + "-" + twoDigitsYear,    // 8-22
> +          oneDigitMonth + "-" + fourDigitsYear,   // 8-2022
> +          twoDigitsMonth + "-" + twoDigitsYear,   // 08-22
> +          twoDigitsMonth + "-" + fourDigitsYear,  // 08-2022
> +          twoDigitsYear + "-" + twoDigitsMonth,   // 22-08
> +          fourDigitsYear + "-" + twoDigitsMonth,  // 2022-08
> +          fourDigitsYear + "/" + oneDigitMonth,   // 2022/8
> +        ];

Not sure if "0822" is the case we need to handle as well?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:413
(Diff revision 6)
> +          fourDigitsYear + "/" + oneDigitMonth,   // 2022/8
> +        ];
> +
> +        for (let option of options) {
> +          if ([option.text, option.label, option.value].some(
> +            str => patterns.some(pattern => pattern == str)

Maybe `str.includes(pattern)` would be better?

::: browser/extensions/formautofill/ProfileStorage.jsm:1473
(Diff revision 6)
>        creditCard["cc-family-name"] = nameParts.family;
>        hasNewComputedFields = true;
>      }
>  
> +    if (!("cc-exp" in creditCard) && "cc-exp-month" in creditCard && "cc-exp-year" in creditCard) {
> +      creditCard["cc-exp"] = creditCard["cc-exp-year"] + "-" + creditCard["cc-exp-month"];

A valid month string needs two-digit month number where `cc-exp-month` isn't.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:23
(Diff revision 6)
>  
> +const DEFAULT_CREDITCARD_RECORD = {
> +  "guid": "123",
> +  "cc-exp-month": "1",
> +  "cc-exp-year": "2025",
> +  "cc-exp": "1/2025",

`2025-01`

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 15

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

https://reviewboard.mozilla.org/r/168092/#review176126

Thanks.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:330
(Diff revision 8)
>  
>      return null;
>    },
>  
> +  findCreditCardSelectOption(selectEl, creditCard, fieldName) {
> +    let oneDigitMonth = creditCard["cc-exp-month"];

`cc-exp-month` is a number so it will fail when calling `padStart` below.

```js
let oneDigitMonth = creditCard["cc-exp-month"].toString();
```

::: browser/extensions/formautofill/FormAutofillUtils.jsm:332
(Diff revision 8)
> +    let expYearInt = parseInt(creditCard["cc-exp-year"], 10);
> +    let twoDigitsYear = expYearInt.toString().substr(2, 2);
> +    let fourDigitsYear = expYearInt.toString();

nit: Not sure if we'll need `expYearInt` eventually but how about:

```js
let fourDigitsYear = creditCard["cc-exp-year"].toString();
let twoDigitsYear = fourDigitsYear.substr(2, 2);
```

::: browser/extensions/formautofill/ProfileStorage.jsm:1475
(Diff revision 8)
>        creditCard["cc-family-name"] = nameParts.family;
>        hasNewComputedFields = true;
>      }
>  
> +    if (!("cc-exp" in creditCard) && "cc-exp-month" in creditCard && "cc-exp-year" in creditCard) {
> +      creditCard["cc-exp"] = creditCard["cc-exp-year"] + "-" + creditCard["cc-exp-month"].toString().padStart(2, "0");

`creditCard["cc-exp-year"].toString()`

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:21
(Diff revision 8)
> +  "cc-exp-month": "1",
> +  "cc-exp-year": "2025",

Use number type.

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 17

[Sean Lee [:seanlee][:weilonge]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4698bbf85ff3d8860b3d39278e22e700a48ae553

Comment 18

[Sean Lee [:seanlee][:weilonge]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a42900caf112f5b12442c72c20b93a12f8fcbb

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8896803 [details]
Bug 1371145 - Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1a379e6da52
Calculate the possible option of cc-exp/cc-exp-year/cc-exp-month for filling and previewing. r=lchang

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1a379e6da52