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)
Toolkit
Form Manager
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.
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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+
Comment 3•7 years ago
|
||
BTW, the patch looks good to me. :)
Assignee | ||
Comment 4•7 years ago
|
||
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?
Comment hidden (mozreview-request) |
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fe74041cd1c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•