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)

56 Branch
enhancement
Not set
normal

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: nobody → schung
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/21]
Whiteboard: [form autofill:M4][ETA:7/21] → [form autofill:M4][ETA:7/28]
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 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 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 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+
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.
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/02fd97e1217b
https://hg.mozilla.org/mozilla-central/rev/4c921604f392
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.