Closed
Bug 1379588
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Implement credit card profile creation in FormAutofillHandler
Categories
(Toolkit :: Form Manager, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: steveck, Assigned: steveck)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M4][ETA:7/28])
Attachments
(2 files)
We leverage FormAutofillHandler.createProfile to generate valid address record for address saving/updating while form submitted. If we need to support credit card record, createProfile should be able to generate address and credit card record separately after bug 1379533 landed.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → schung
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/21]
Assignee | ||
Updated•7 years ago
|
Whiteboard: [form autofill:M4][ETA:7/21] → [form autofill:M4][ETA:7/28]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8889722 [details] Bug 1379588 - Part 2: Make createRecords support creditcard. https://reviewboard.mozilla.org/r/160790/#review166098 Looks good. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:405 (Diff revision 1) > * @returns {Object} The new profile that convert from details with trimmed result. > */ > - createProfile() { > - let profile = {}; > + createRecords() { > + let records = {}; > > - this.address.fieldDetails.forEach(detail => { > + ["address", "creditCard"].forEach((type) => { nit: no bracket when there's only one argument in arrow function. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:411 (Diff revision 1) > + let details = this[type].fieldDetails; > + if (!details || details.length == 0) { > + return; > + } > + > + records[`${type}Record`] = {}; nit: use a variable for `${type}Record`.
Attachment #8889722 -
Flags: review?(lchang) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8889721 [details] Bug 1379588 - Part 1: Add address/creditCard object in handler and refactor collectFormFields for less cross-module call. https://reviewboard.mozilla.org/r/160788/#review166672 ::: browser/extensions/formautofill/FormAutofillContent.jsm:479 (Diff revision 2) > + if (validDetails.length == 0) { > + this.log.debug("Do not cache the handler since there is no valid form inside it."); > + return; > + } > > this._formsDetails.set(formHandler.form.rootElement, formHandler); I think we still need to save the handler so we won't need to parse the entire form twice when the same field gets focused again. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:36 (Diff revision 2) > + /** > + * Similiar to `fieldDetails`, and `fieldDetails` contains the address > + * records only. > + */ nit: "Similar to the `fieldDetails` above but contains address fields only." and so does the credit card's. ::: browser/extensions/formautofill/test/unit/test_collectFormFields.js (Diff revision 2) > - Assert.equal(handler.isValidAddressForm, testcase.isValidForm.address, "Valid Address Form Checking"); > - Assert.equal(handler.isValidCreditCardForm, testcase.isValidForm.creditCard, "Valid Credit Card Form Checking"); We should keep these tests as the APIs are still available.
Attachment #8889721 -
Flags: review?(lchang)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8889721 [details] Bug 1379588 - Part 1: Add address/creditCard object in handler and refactor collectFormFields for less cross-module call. https://reviewboard.mozilla.org/r/160788/#review167260 ::: browser/extensions/formautofill/FormAutofillHandler.jsm:137 (Diff revision 3) > + let validAddressDetails = this.address.fieldDetails; > + let validCreditCardDetails = this.creditCard.fieldDetails; > + > + if (this.address.fieldDetails.length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) { > + log.debug("Ignoring address related fields since it has only", > + this.address.fieldDetails.length, > + "field(s)"); > + validAddressDetails = []; > + } > + > + if (!this.creditCard.fieldDetails.some(i => i.fieldName == "cc-number")) { > + log.debug("Ignoring credit card related fields since it's without credit card number field"); > + validCreditCardDetails = []; > + } I prefer to set `this.address.fieldDetails` and `this.creditCard.fieldDetails` to `null` when they are not valid so we'll know whether it's valid once we need it.
Attachment #8889721 -
Flags: review?(lchang) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8889721 [details] Bug 1379588 - Part 1: Add address/creditCard object in handler and refactor collectFormFields for less cross-module call. https://reviewboard.mozilla.org/r/160788/#review167840 LGTM ::: browser/extensions/formautofill/FormAutofillHandler.jsm:149 (Diff revision 4) > + this.creditCard.fieldDetails = null; > + } > + > + // Return null is there's no valid form > + if (!this.address.fieldDetails && !this.creditCard.fieldDetails) { > + return null; The returning value is an empty array without this check, so you don't even need to do the below check in `FomrAutofillContent.identifyAutofillFields()`. ```JS if (!validDetails) { this.log.debug("There is no valid form in the handler:", formHandler); return; } ```
Attachment #8889721 -
Flags: review?(selee) → review+
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8889721 [details] Bug 1379588 - Part 1: Add address/creditCard object in handler and refactor collectFormFields for less cross-module call. https://reviewboard.mozilla.org/r/160788/#review167840 > The returning value is an empty array without this check, so you don't even need to do the below check in `FomrAutofillContent.identifyAutofillFields()`. > ```JS > if (!validDetails) { > this.log.debug("There is no valid form in the handler:", formHandler); > return; > } > ``` The returning value will be null if both address details and credit card details are null, so it's still necessary in the `FomrAutofillContent.identifyAutofillFields`. I'll add more comment in the JSDoc returns part.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 20•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/02fd97e1217b Part 1: Add address/creditCard object in handler and refactor collectFormFields for less cross-module call. r=lchang,seanlee https://hg.mozilla.org/integration/autoland/rev/4c921604f392 Part 2: Make createRecords support creditcard. r=lchang
Keywords: checkin-needed
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/02fd97e1217b https://hg.mozilla.org/mozilla-central/rev/4c921604f392
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•