Closed Bug 1428414 Opened 6 years ago Closed 6 years ago

Initial Payment Request Basic Credit Card Add/Edit page

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(5 files)

Share logic/UI with about:preferences (editCreditCard.xhtml[1]) from the management UI.

[1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/editCreditCard.xhtml
Whiteboard: [webpayments]
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1427954
Summary: Initial Payment Request Basic (Credit/Debit) Card Add/Edit page → Initial Payment Request Basic Credit Card Add/Edit page
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review234466


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/payments/test/mochitest/payments_common.js:5
(Diff revision 2)
>  "use strict";
>  
>  /* exported asyncElementRendered, promiseStateChange, deepClone */
>  
> +const PTU = SpecialPowers.Cu.import("resource://testing-common/PaymentTestUtils.jsm", {})

Error: 'ptu' is assigned a value but never used. [eslint: no-unused-vars]

::: toolkit/components/payments/test/mochitest/test_basic_card_form.html:71
(Diff revision 2)
> +  let form = document.createElement("basic-card-form");
> +  form.dataset.backButtonLabel = "Back";
> +  await form.requestStore.setState({
> +    page: {
> +      id: "test-page",
> +    }

Error: Missing trailing comma. [eslint: comma-dangle]
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review235006

::: commit-message-300eb:1
(Diff revision 4)
> +Bug 1428414 - Use the autofill credit card form in the Payment dialog. r=jaws

I'll try to split this up more when I get a chance but it'd be great to get at least a high-level review in the meantime.
Comment on attachment 8959929 [details]
Bug 1428414 - Add a PaymentRequest local dev. server.

https://reviewboard.mozilla.org/r/228688/#review235128
Attachment #8959929 - Flags: review?(jaws) → review+
Comment on attachment 8959930 [details]
Bug 1428414 - Support re-using autofill edit forms for different records.

https://reviewboard.mozilla.org/r/228690/#review235140

::: browser/extensions/formautofill/content/autofillEditForms.js:21
(Diff revision 2)
>    /**
>     * Fill the form with a record object.
>     * @param  {object} record
>     */
> -  loadInitialValues(record) {
> -    for (let field in record) {
> +  loadRecord(record) {
> +    this._record = record;

I think we should move `this._record = record` to EditAddress.loadRecord since with this patch EditCreditCard.loadRecord will end up setting `this._record` twice (once before calling `generateYears()` and a second time when calling `super.loadRecord`.
Attachment #8959930 - Flags: review?(jaws) → review+
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review235152

I can't find a way to save the changes made by the add/edit page. When I click on "Add", I can type in whatever I want but the only option is "Back". Clicking on "Back" shows the same empty form. If I choose "Edit" on a selected card, the edit form shows the correct card details (though it does obfuscate the number which I think would be incorrect for "edit") but also doesn't persist any of the changes made.

::: toolkit/components/payments/res/containers/basic-card-form.js:22
(Diff revision 4)
> +
> +class BasicCardForm extends PaymentStateSubscriberMixin(HTMLElement) {
> +  constructor() {
> +    super();
> +
> +    this.backButton = document.createElement("button");

I think it is odd that we use an <a> to go to the add/edit screen and then a <button> to return to the summary screen.

We should use <a> here to match the <a> for add/edit that took the user to this screen, especially if "back" is not supposed to persist the changes.

::: toolkit/components/payments/res/containers/payment-method-picker.js:25
(Diff revision 4)
>      this.spacerText = document.createTextNode(" ");
>      this.securityCodeInput = document.createElement("input");
>      this.securityCodeInput.autocomplete = "off";
>      this.securityCodeInput.size = 3;
>      this.securityCodeInput.addEventListener("change", this);
> +    this.addLink = document.createElement("a");

Please add `href="#"` attributes to addLink and editLink so they will be styled like links. Right now they don't have the underline or blue text color.

::: toolkit/components/payments/res/unprivileged-fallbacks.js:6
(Diff revision 4)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /**
> - * This file defines a fallback log object to be used during development outside
> + * This file defines a fallback objects to be used during development outside

s/defines a fallback/defines fallback/
Attachment #8959381 - Flags: review?(jaws) → review-
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review235152

Right, this commit wasn't implementing the buttons since I wanted to unblock other bugs.

> I think it is odd that we use an <a> to go to the add/edit screen and then a <button> to return to the summary screen.
> 
> We should use <a> here to match the <a> for add/edit that took the user to this screen, especially if "back" is not supposed to persist the changes.

Hmm… not sure I agree… forms usually use buttons for "cancel" which is what back basically is.

> Please add `href="#"` attributes to addLink and editLink so they will be styled like links. Right now they don't have the underline or blue text color.

FYI: That's actually not a good pattern to use because it will cause the page to scroll to the top when clicked. "javascript:void(0)" or an empty href with `evt.preventDefault();` is better but I get your point and I will add the href attribute.
As I kinda expected, saving the record isn't trivial if we want to handle errors with saving and if we want to auto-select the new records in the pickers since we need a way to know about the new record's guid.
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review236790

::: toolkit/components/payments/res/containers/basic-card-form.js:74
(Diff revisions 4 - 5)
>        selectedPaymentCard,
>        savedBasicCards,
>      } = state;
>  
> +    let editing = !!state.selectedPaymentCard;
> +    this.form.querySelector("#cc-number").disabled = editing;

Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number?

I think this might be trying to hard to prevent users from editing a single entry to use different cards.
Attachment #8959381 - Flags: review?(jaws) → review+
Comment on attachment 8961650 [details]
Bug 1428414 - Removed cleared record attributes on card and address options.

https://reviewboard.mozilla.org/r/230524/#review236792
Attachment #8961650 - Flags: review?(jaws) → review+
Comment on attachment 8959381 [details]
Bug 1428414 - Use the autofill credit card form in the Payment dialog.

https://reviewboard.mozilla.org/r/228208/#review236790

> Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number?
> 
> I think this might be trying to hard to prevent users from editing a single entry to use different cards.

Two reasons:
a) What you said… we don't want users editing to swap between unrelated cards as it will mess with our telemetry and metadata and also lead them to having a worse experience if they intended to save multiple.
b) The plan was to never give the decrypted credit card number to the unprivileged dialog to mitigate potential XSS damage so the user wouldn't be able to meaningfully edit the number… they would basically have to re-type it anyways and disabling seemed simpler for now.
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #23)
> Comment on attachment 8959381 [details]
> Bug 1428414 - Use the autofill credit card form in the Payment dialog.
> 
> https://reviewboard.mozilla.org/r/228208/#review236790
> 
> > Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number?
> > 
> > I think this might be trying to hard to prevent users from editing a single entry to use different cards.
> 
> Two reasons:
> a) What you said… we don't want users editing to swap between unrelated
> cards as it will mess with our telemetry and metadata and also lead them to
> having a worse experience if they intended to save multiple.
> b) The plan was to never give the decrypted credit card number to the
> unprivileged dialog to mitigate potential XSS damage so the user wouldn't be
> able to meaningfully edit the number… they would basically have to re-type
> it anyways and disabling seemed simpler for now.

I don't really think case (a) is going to be as frequent as you think, but case (b) is sufficient for doing this at least for now. But we might want to include a link there to edit the number, taking the user to the Preferences subdialog.
Comment on attachment 8961651 [details]
Bug 1428414 - Support saving credit card changes in the Payment Request dialog.

https://reviewboard.mozilla.org/r/230526/#review238416
Attachment #8961651 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #31)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #23)
> > Comment on attachment 8959381 [details]
> > Bug 1428414 - Use the autofill credit card form in the Payment dialog.
> > 
> > https://reviewboard.mozilla.org/r/228208/#review236790
> > 
> > > Why do we want to limit changing the cc-number when editing? What about users who need to fix a typo in the number?
> > > 
> > > I think this might be trying to hard to prevent users from editing a single entry to use different cards.
> > 
> > Two reasons:
> > a) What you said… we don't want users editing to swap between unrelated
> > cards as it will mess with our telemetry and metadata and also lead them to
> > having a worse experience if they intended to save multiple.
> > b) The plan was to never give the decrypted credit card number to the
> > unprivileged dialog to mitigate potential XSS damage so the user wouldn't be
> > able to meaningfully edit the number… they would basically have to re-type
> > it anyways and disabling seemed simpler for now.
> 
> I don't really think case (a) is going to be as frequent as you think, but
> case (b) is sufficient for doing this at least for now. But we might want to
> include a link there to edit the number, taking the user to the Preferences
> subdialog.

(a) also applies to the pref subdialog and at the time that was implemented the number wasn't supposed to be editable either.
Depends on: 1451016
Comment on attachment 8961650 [details]
Bug 1428414 - Removed cleared record attributes on card and address options.

https://reviewboard.mozilla.org/r/230524/#review238934


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/payments/res/containers/address-picker.js:42
(Diff revision 5)
>  
>    /**
>     * De-dupe and filter addresses for the given set of fields that will be visible
>     *
>     * @param {object} addresses
> -   * @param {array?} fieldNames - optional list of field names that be used when de-duping entries
> +   * @param {array?} fieldNames - optional list of field names that be used when de-duping and excluding entries

Error: Line 42 exceeds the maximum line length of 100. [eslint: max-len]
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/b4f0025c8303
Add a PaymentRequest local dev. server. r=jaws
https://hg.mozilla.org/integration/autoland/rev/4fa0e33621e3
Support re-using autofill edit forms for different records. r=jaws
https://hg.mozilla.org/integration/autoland/rev/b045a7779f4d
Use the autofill credit card form in the Payment dialog. r=jaws
https://hg.mozilla.org/integration/autoland/rev/6045035ad2ed
Removed cleared record attributes on card and address options. r=jaws
https://hg.mozilla.org/integration/autoland/rev/70eb77d5bd24
Support saving credit card changes in the Payment Request dialog. r=jaws
Depends on: 1451179
Depends on: 1451143
Depends on: 1455151
Product: Toolkit → Firefox
Target Milestone: mozilla61 → Firefox 61
You need to log in before you can comment on or make changes to this bug.