Closed Bug 1370764 Opened 7 years ago Closed 7 years ago

[Form Autofill] Dialog to add/edit/view a credit card profile

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: lchang, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4] [ETA:8/18])

Attachments

(3 files)

Bug 1370764 - (Part 1) Rename editAddress files to editDialog.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
A single view with editable <input>s for an initial set of fields. The dialog would be passed an optional credit card identifier for the edit/view case. The dialog will initially do no validation and simply load/save credit cards.

Simple tests for loading/saving should be written.
Assignee: nobody → scwwu
Whiteboard: [form autofill:M4] → [form autofill:M4] [ETA:8/4]
Status: NEW → ASSIGNED
Whiteboard: [form autofill:M4] [ETA:8/4] → [form autofill:M4] [ETA:8/18]
Comment on attachment 8899756 [details]
Bug 1370764 - (Part 1) Rename editAddress files to editDialog.

https://reviewboard.mozilla.org/r/171098/#review176640
Attachment #8899756 - Flags: review?(lchang) → review+
Comment on attachment 8899758 [details]
Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card.

https://reviewboard.mozilla.org/r/171102/#review176664

We might need a test case to check that a profile with invalid (or omitted) cc-number won't be saved.

::: browser/extensions/formautofill/content/editDialog.js:37
(Diff revision 2)
>    async init() {
>      if (this._record) {
>        await this.loadInitialValues(this._record);
>      }
>      this.attachEventListeners();
> +    window.dispatchEvent(new CustomEvent("FormReady"));

Comment on this about its meaning and it's for test only.

::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:54
(Diff revision 2)
> +    }, {once: true});
> +  });
> +  let creditCards = await getCreditCards();
> +
> +  is(creditCards.length, 1, "only one credit card is in storage");
> +  is(Object.keys(TEST_CREDIT_CARD_2).length, 4, "Sanity check number of properties");

It seems not to make sense to check the number of properties of a test case.

::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:56
(Diff revision 2)
> +    if (fieldName === "cc-number") {
> +      fieldValue = "*".repeat(fieldValue.length - 4) + fieldValue.substr(-4);
> +    }

How about also checking the existence of "cc-number-encrypted"?

::: browser/extensions/formautofill/test/browser/head.js:69
(Diff revision 2)
> -  "cc-exp-month": 12,
> -  "cc-exp-year": 2022,
> +  "cc-exp-month": "12",
> +  "cc-exp-year": "2022",

They are actually numbers in the storage. We shouldn't change its type here.
Attachment #8899758 - Flags: review?(lchang)
Comment on attachment 8899758 [details]
Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card.

https://reviewboard.mozilla.org/r/171102/#review176664

Thanks for the suggestions. I've added a test case to check for invalid credit card entry.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review176782

::: browser/extensions/formautofill/content/editAddress.xhtml:71
(Diff revision 3)
> -    // Localize strings before DOMContentLoaded to prevent flash
> -    window.dialog.localizeDocument();
> +    /* global EditAddress */
> +    new EditAddress({

I don't see the benefit of passing the elements from xhtml instead of getting them in js directly. The amount of properties and their naming differ between address and credit card. The classes they use are also different.

::: browser/extensions/formautofill/content/editDialog.js:247
(Diff revision 3)
> +      this._elements.title.dataset.localization = "editCreditCardTitle";
> +    }
> +    FormAutofillUtils.localizeMarkup(AUTOFILL_BUNDLE_URI, document);
> +  }
> +
> +  isCcNumber(ccNumber = "") {

nit: `isCCNumber`.

Also, I prefer no default value and to check if `ccNumber` is empty inside the function.

::: browser/extensions/formautofill/content/editDialog.js:262
(Diff revision 3)
> +    super.loadInitialValues(Object.assign({}, creditCard, {"cc-number": decryptedCC}));
> +  }
> +
> +  async handleSubmit() {
> +    let creditCard = this.buildFormObject();
> +    // Show error on the cc-number field if it's empty

"... if it's empty or invalid."
Comment on attachment 8899758 [details]
Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card.

https://reviewboard.mozilla.org/r/171102/#review177168

::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:102
(Diff revision 3)
> +      setTimeout(() => {
> +        win.removeEventListener("unload", unloadHandler);
> +        win.close();
> +        resolve();
> +      }, 500);

Here is a lint error.
Attachment #8899758 - Flags: review?(lchang) → review+
Comment on attachment 8899758 [details]
Bug 1370764 - (Part 3) Add browser chrome test for adding and editing credit card.

https://reviewboard.mozilla.org/r/171102/#review177168

> Here is a lint error.

Thanks. Added `SimpleTest.requestFlakyTimeout`.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review176782

> I don't see the benefit of passing the elements from xhtml instead of getting them in js directly. The amount of properties and their naming differ between address and credit card. The classes they use are also different.

I think it's simpler to look at the markup and query them in the JS below, rather than in a separate file. If the markup is changed, it would be more obvious that the code below should also be changed.

> nit: `isCCNumber`.
> 
> Also, I prefer no default value and to check if `ccNumber` is empty inside the function.

Done.

> "... if it's empty or invalid."

Thanks.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review177206

::: browser/extensions/formautofill/content/editDialog.js:248
(Diff revision 4)
> +    }
> +    FormAutofillUtils.localizeMarkup(AUTOFILL_BUNDLE_URI, document);
> +  }
> +
> +  isCCNumber(ccNumber) {
> +    return ccNumber ? ccNumber.match(/(\s*\d){12,}$/) : false;

`ccNumber.replace(/\s/g, "").match(/^\d{12,}$/)`

On a second thought, could you put this function in FormAutofillUtils and leverage it in ProfileStorage as well so we won't make different rules in the future.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review176782

> I think it's simpler to look at the markup and query them in the JS below, rather than in a separate file. If the markup is changed, it would be more obvious that the code below should also be changed.

Ok, I'm fine with that.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review177206

> `ccNumber.replace(/\s/g, "").match(/^\d{12,}$/)`
> 
> On a second thought, could you put this function in FormAutofillUtils and leverage it in ProfileStorage as well so we won't make different rules in the future.

Good idea. Thanks.
Comment on attachment 8899757 [details]
Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry.

https://reviewboard.mozilla.org/r/171100/#review177228

Thanks.
Attachment #8899757 - Flags: review?(lchang) → review+
Thanks Luke!
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dd6b014dffbf -d 8d1350135a04: rebasing 415517:dd6b014dffbf "Bug 1370764 - (Part 1) Rename editAddress files to editDialog. r=lchang"
rebasing 415518:366a2aa68c1f "Bug 1370764 - (Part 2) Create a dialog to add/edit/view a credit card entry. r=lchang"
merging browser/extensions/formautofill/FormAutofillUtils.jsm
merging browser/extensions/formautofill/ProfileStorage.jsm
merging browser/extensions/formautofill/test/unit/test_creditCardRecords.js
warning: conflicts while merging browser/extensions/formautofill/ProfileStorage.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased to central.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c15e0df1c51e
(Part 1) Rename editAddress files to editDialog. r=lchang
https://hg.mozilla.org/integration/autoland/rev/5e9fefab66fb
(Part 2) Create a dialog to add/edit/view a credit card entry. r=lchang
https://hg.mozilla.org/integration/autoland/rev/6fb1b3230adf
(Part 3) Add browser chrome test for adding and editing credit card. r=lchang
Keywords: checkin-needed
Depends on: 1393674
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: