Closed Bug 1413487 Opened 7 years ago Closed 7 years ago

[Form Autofill] Split "cc-exp" into "cc-exp-month" and "cc-exp-year" in the storage

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 file)

When a webiste uses a single field for "cc-exp", we should be able to split it into "cc-exp-month" and "cc-exp-year" and save it in the storage.
Status: NEW → ASSIGNED
Comment on attachment 8924414 [details]
Bug 1413487 - [Form Autofill] Split "cc-exp" into "cc-exp-month" and "cc-exp-year" in the storage.

https://reviewboard.mozilla.org/r/195704/#review201822

::: browser/extensions/formautofill/ProfileStorage.jsm:1551
(Diff revision 1)
>          creditCard["cc-exp-year"] = expYear;
>        }
>      }
> +
> +    if (creditCard["cc-exp"] && (!creditCard["cc-exp-month"] || !creditCard["cc-exp-year"])) {
> +      let rules = [

Just a personal preference, `let rules = [{` for more compact codes.

::: browser/extensions/formautofill/ProfileStorage.jsm:1556
(Diff revision 1)
> +      let rules = [
> +        {
> +          regex: "(\\d{4})[-/](\\d{1,2})",
> +          yearIndex: 1,
> +          monthIndex: 2,
> +        },

`},{`

::: browser/extensions/formautofill/ProfileStorage.jsm:1570
(Diff revision 1)
> +        {
> +          regex: "(\\d{2})(\\d{2})",
> +        },
> +      ];
> +
> +      for (let rule of rules) {

It's an alternative version to compare and convert values in `comparator`.
BTW, the comparators of rules[2] and rules[3] are identical, so I think they can be merged as well.

```JS
      let isMonthAndYearPair = (month, year) => {
        let isYear = num => {
          if (num < 100) {
            num += 2000;
          }
          return num > 2012;
        };
        let isMonth = num => {
          return !(num > 12 || num < 1);
        };
        if (year < 100) {
          year += 2000;
        }
        return isMonth(month) && isYear(year) ? {year, month} : null;
      };
      let rules = [{
        regex: "(\\d{4})[-/](\\d{1,2})",
        comparator: (index1, index2) => isMonthAndYearPair(index2, index1),
      },{
        regex: "(\\d{1,2})[-/](\\d{4})",
        comparator: (index1, index2) => isMonthAndYearPair(index1, index2),
      },{
        regex: "(\\d{1,2})[-/](\\d{1,2})",
        comparator: (index1, index2) => {
          let c1 = isMonthAndYearPair(index1, index2);
          if (c1) {
            return c1;
          }
          let c2 = isMonthAndYearPair(index2, index1);
          if (c2) {
            return c2;
          }
          return null;
        },
      },{
        regex: "(\\d{2})(\\d{2})",
        comparator: (index1, index2) => {
          let c1 = isMonthAndYearPair(index1, index2);
          if (c1) {
            return c1;
          }
          let c2 = isMonthAndYearPair(index2, index1);
          if (c2) {
            return c2;
          }
          return null;
        },
      }];

      for (let rule of rules) {
        let result = new RegExp(`(?:^|\\D)${rule.regex}(?!\\d)`).exec(creditCard["cc-exp"]);
        if (!result) {
          continue;
        }

        let values = rule.comparator(parseInt(result[1], 10), parseInt(result[2], 10));
        if (!values) {
          continue;
        }

        let {year, month} = values;
        creditCard["cc-exp-month"] = month;
        creditCard["cc-exp-year"] = year;
        break;
      }
```

::: browser/extensions/formautofill/test/unit/test_transformFields.js:834
(Diff revision 1)
>      do_print("Verify testcase: " + CREDIT_CARD_COMPUTE_TESTCASES[i].description);
>      do_check_record_matches(CREDIT_CARD_COMPUTE_TESTCASES[i].expectedResult, creditCards[i]);
>    }
>  });
>  
>  add_task(async function test_normalizeCreditCardFields() {

Is there any reason to implement `for` loop in `add_task` block?

How about using the `for` loop to wrap `test_normalizeCreditCardFields`?
```JS
for (let testcase of CREDIT_CARD_NORMALIZE_TESTCASES) {
  add_task(async function test_normalizeCreditCardFields() {
  ...
  }
}
```

This makes the test case debugging easier than the original version when inserting some logs.
Attachment #8924414 - Flags: review?(selee) → review+
BTW, the patch looks good to me. :)
Thanks for the review.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #2)
> ::: browser/extensions/formautofill/ProfileStorage.jsm:1570
> 
> It's an alternative version to compare and convert values in `comparator`.
> BTW, the comparators of rules[2] and rules[3] are identical, so I think they
> can be merged as well.

It looks a bit complicated to me as it needs to trace two sub-functions to know what happens. I'd prefer a simple way.


> ::: browser/extensions/formautofill/test/unit/test_transformFields.js:834
> 
> Is there any reason to implement `for` loop in `add_task` block?

I guess there's no reason at that time. Since it's unrelated to this patch and four test cases will need to be changed, how about doing it in a separate bug?
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fe74041cd1c
[Form Autofill] Split "cc-exp" into "cc-exp-month" and "cc-exp-year" in the storage. r=seanlee
https://hg.mozilla.org/mozilla-central/rev/1fe74041cd1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: