1428415 - Add a "Save to Firefox" toggle to the Add Payment Card page

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

Allow the user to choose to not save the debit/credit card into persistent Firefox storage.

If the user doesn't check the box then the data should only be stored in memory for its useful duration (e.g. until the Payment Request dialog closes).

In a private window the toggle should default to being unchecked, otherwise it should default to being checked.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

* WIP: Need to figure out how to get the new card into the state, but not saved to the profile
* Tests for basic-card-form.js in private/non-private windows

Review commit: https://reviewboard.mozilla.org/r/235392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235392/

Comment 2

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235392/diff/1-2/

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235392/diff/2-3/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review242812


Code analysis found 4 defects in this patch:
 - 4 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/content/paymentDialogWrapper.js:390
(Diff revision 3)
> +
>      this.sendMessageToContent("showPaymentRequest", {
>        request: requestSerialized,
>        savedAddresses: this.fetchSavedAddresses(),
>        savedBasicCards: this.fetchSavedPaymentCards(),
> +      isPrivate

Error: Missing trailing comma. [eslint: comma-dangle]

::: toolkit/components/payments/res/components/labelled-checkbox.js:50
(Diff revision 3)
> +  get checked() {
> +    return this.hasAttribute("checked");
> +  }
> +
> +  set checked(isChecked) {
> +    console.log(`checked setter: ${isChecked}, this._checkbox.checked: ${this._checkbox.checked}`);

Error: Unexpected console statement. [eslint: no-console]

::: toolkit/components/payments/res/components/labelled-checkbox.js:63
(Diff revision 3)
> +    return isChecked;
> +  }
> +
> +  handleEvent(event) {
> +    if (event.type == "input" && event.target == this._checkbox) {
> +      console.log("Got checkbox input event, checked: ", this._checkbox.checked);

Error: Unexpected console statement. [eslint: no-console]

::: toolkit/components/payments/res/containers/payment-method-picker.js:47
(Diff revision 3)
>      this.appendChild(this.editLink);
>      super.connectedCallback();
>    }
>  
>    render(state) {
> -    let {savedBasicCards} = state;
> +    let basicCards = paymentRequest.getBasicCards(state);

Error: 'paymentrequest' is not defined. [eslint: no-undef]

Comment 5

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235392/diff/3-4/

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review243888

I didn't do a full review yet but found enough to get started for now.

::: toolkit/components/payments/res/components/labelled-checkbox.js:32
(Diff revision 4)
> +
> +  connectedCallback() {
> +    this.appendChild(this._label);
> +    this._label.appendChild(this._checkbox);
> +    this._label.appendChild(this._labelSpan);
> +    this._checkbox.checked = this._checkbox.checked || this.hasAttribute("checked");

This should be handled in the `render` method which gets called two lines down. Is there a reason for similar logic to be in connectedCallback?

::: toolkit/components/payments/res/components/labelled-checkbox.js:41
(Diff revision 4)
> +  render() {
> +    this._labelSpan.textContent = this.label;
> +    this.checked = this.hasAttribute("checked");
> +  }
> +
> +  attributeChangedCallback(attr, oldValue, newValue) {
> +    super.attributeChangedCallback(attr, oldValue, newValue);
> +    if (attr == "checked") {
> +      this._checkbox.checked = this.checked;
> +    }
> +  }
> +
> +  handleEvent(event) {
> +    if (event.type == "input" && event.target == this._checkbox) {
> +      this.checked = this._checkbox.checked;
> +    }
> +  }

I'm a bit concerned that the checkededness (attributes and properties) are going to become out-of-sync and as a result I question the need for the `checked` attribute on `LabelledCheckbox` (and partially the need for `LabelledCheckbox` at all).

How about having `checked` be a setter and getter which refers to the property on `this._checkbox` and removing the attribute?

::: toolkit/components/payments/res/containers/basic-card-form.js:29
(Diff revision 4)
>      this.backButton.addEventListener("click", this);
>  
>      this.saveButton = document.createElement("button");
>      this.saveButton.addEventListener("click", this);
>  
> +    this.persistCheckbox = document.createElement("labelled-checkbox");

Please prefer `new LabelledCheckbox()` style in new code for custom elements so that we get static analysis if the name changes or it gets deleted. In this case I can see that the "labelled-checkbox.js" import should really be in this file instead of payment-dialog.

::: toolkit/components/payments/res/containers/basic-card-form.js:86
(Diff revision 4)
>        page,
>        savedAddresses,
> -      savedBasicCards,
>        selectedShippingAddress,
>      } = state;
> +    let basicCards =  paymentRequest.getBasicCards(state);

Nit: there are two spaces after the `=`

::: toolkit/components/payments/res/containers/basic-card-form.js:100
(Diff revision 4)
> +      this.persistCheckbox.setAttribute("hidden", "hidden");
> +      this.persistCheckbox.checked = !record.isTemporary;
> +    } else {
> +      if (selectedShippingAddress) {
> -      record.billingAddressGUID = selectedShippingAddress;
> +        record.billingAddressGUID = selectedShippingAddress;
> -    }
> +      }
> +      // Adding a new record: default persistence to checked when in a not-private session
> +      this.persistCheckbox.removeAttribute("hidden");

Why aren't you using the property for `hidden` in both places? If there is a good reason then it should be documented in a comment

::: toolkit/components/payments/res/containers/basic-card-form.js:181
(Diff revision 4)
> +    } else {
> +      // Only persist state, don't update the autofill store.
> +      // This record will never get inserted into the store so we generate a faux-guid.
> +      record.guid = page.guid || "temp-" + Math.abs(Math.random() * 0xffffffff|0);
> +
> +      log.debug("BasicCardForm: saving temporary record: ", record.guid, JSON.stringify(record));

Nit: don't include spaces at the end of the quote since the logging module already inserts spaces between arguments.

Also, I get nervous logging things like credit card records and passwords as they can persist for a while. I think we should remove the `JSON.stringify(record)` portion.

::: toolkit/components/payments/res/containers/basic-card-form.js:183
(Diff revision 4)
> +      if (!tempBasicCards) {
> +        tempBasicCards = {};
> +      }

You shouldn't need this with the change to PaymentStateSubscriberMixin.js

::: toolkit/components/payments/res/containers/basic-card-form.js:190
(Diff revision 4)
> +      tempBasicCards[record.guid] = record;
> +      this.requestStore.setState({
> +        tempBasicCards,
> +      });

You shouldn't mutate `tempBasicCards` here, use Object.assign so that the existing state is only mutated by `setState` e.g.:
```js
this.requestStore.setState({
  tempBasicCards: Object.assign({}, tempBasicCards, {
    [record.guid]: record,
  }),
});
```

::: toolkit/components/payments/res/containers/payment-dialog.js:166
(Diff revision 4)
>      // Ensure `selectedPaymentCard` never refers to a deleted payment card and refers
>      // to a payment card if one exists.
> -    if (!savedBasicCards[selectedPaymentCard]) {
> +    let basicCards = paymentRequest.getBasicCards(state);
> +    if (!basicCards[selectedPaymentCard]) {
>        this.requestStore.setState({
> -        selectedPaymentCard: Object.keys(savedBasicCards)[0] || null,
> +        selectedPaymentCard: Object.keys(basicCards)[0] || null,

I wonder if `[0]` the right thing still UX-wise?

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review243888

> This should be handled in the `render` method which gets called two lines down. Is there a reason for similar logic to be in connectedCallback?

There is apparently a lot of fluff left in this patch from my flailing trying to track down the cause of a test failure I was having.

> I'm a bit concerned that the checkededness (attributes and properties) are going to become out-of-sync and as a result I question the need for the `checked` attribute on `LabelledCheckbox` (and partially the need for `LabelledCheckbox` at all).
> 
> How about having `checked` be a setter and getter which refers to the property on `this._checkbox` and removing the attribute?

I know the value of the LabelledCheckbox is approaching marginal. It might become entirely redundant if we start generate the markup for the form using a template. As it stands, the component helps keep the label and the checkbox together in a way that is flexible from the point of view of styling, has good accessibility and is convenient to manage from js. 

I had implemented .checked in exactly the way you described originally. Having a getter and setter for checked that just reflects the state of the checkbox works well and may be better here. We don't have visual specs for this yet, but it might turn out we need the checked attribute for styling. We can easily add it if so.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review243888

> I wonder if `[0]` the right thing still UX-wise?

I think the user expectation would be that the card just added would be selected. I think we'll need to sort by last-modified when rendering in the payment-method-picker or the getBasicCards helper to ensure that is the case. 

Aside: I'm not sure if the UX pattern we decide on for auto-selecting shipping addresses should also apply here?

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review243888

> I think the user expectation would be that the card just added would be selected. I think we'll need to sort by last-modified when rendering in the payment-method-picker or the getBasicCards helper to ensure that is the case. 
> 
> Aside: I'm not sure if the UX pattern we decide on for auto-selecting shipping addresses should also apply here?

I've amended the latest patch to explictly set the newly-added card as the selectedPaymentCard. This is the same behavior as the other branch where we go through updateAutofillRecord. 

The question of what to select as default when there is no selectedPaymentCard property, or the previous one has been removed, is linked to the same question we are grappling with in the shipping address case. AIUI, there is a proposal for an event for payment method change - equivalent to shippingaddresschange - see https://github.com/w3c/payment-request/pull/695 so I think we should just aim for consistency in our approach for now so we can more easily make any changes in the future.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235392/diff/4-5/

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review243888

> I've amended the latest patch to explictly set the newly-added card as the selectedPaymentCard. This is the same behavior as the other branch where we go through updateAutofillRecord. 
> 
> The question of what to select as default when there is no selectedPaymentCard property, or the previous one has been removed, is linked to the same question we are grappling with in the shipping address case. AIUI, there is a proposal for an event for payment method change - equivalent to shippingaddresschange - see https://github.com/w3c/payment-request/pull/695 so I think we should just aim for consistency in our approach for now so we can more easily make any changes in the future.

I think we should have a bug on file to ensure we decide on the sorting and default selected with UX before shipping as I don't think we have anything to track that yet but I could be misremembering. Can you file this please? I guess it can go in the reserve (M4)?

Comment 12

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #11)
> I think we should have a bug on file to ensure we decide on the sorting and
> default selected with UX before shipping as I don't think we have anything
> to track that yet but I could be misremembering. Can you file this please? I
> guess it can go in the reserve (M4)?

I filed bug 1455789.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review244502

::: toolkit/components/payments/content/paymentDialogWrapper.js:383
(Diff revision 5)
> +    let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +    let isPrivate = PrivateBrowsingUtils.isWindowPrivate(chromeWindow);

Just noting that we discussed on IRC that this can suffer from races with window activations but that problem already exists in the paymentUIservice and will get addressed once we finalize on our dialog wrapper/UI.

::: toolkit/components/payments/content/paymentDialogWrapper.js:390
(Diff revision 5)
> +
>      this.sendMessageToContent("showPaymentRequest", {
>        request: requestSerialized,
>        savedAddresses: this.fetchSavedAddresses(),
>        savedBasicCards: this.fetchSavedPaymentCards(),
> +      isPrivate

Nit: missing comma

::: toolkit/components/payments/res/components/labelled-checkbox.js:8
(Diff revision 5)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +import ObservedPropertiesMixin from "../mixins/ObservedPropertiesMixin.js";
> +
> +/**
> + *  <labelled-checkbox></labelled-checkbox>

Nit: I would include the two required attributes in this example.

::: toolkit/components/payments/res/components/labelled-checkbox.js:36
(Diff revision 5)
> +    this.checked = this._checkbox.checked || this.hasAttribute("checked");
> +  }
> +
> +  get checked() {
> +    return this._checkbox.checked;
> +  }
> +
> +  set checked(value) {
> +    if (value) {
> +      this.setAttribute("checked", "checked");
> +    } else {
> +      this.removeAttribute("checked");
> +    }

If the `checked` attribute on this element doesn't cause a reaction (it doesn't now) then I don't think we should have it as it will introduce bugs. I think we should remove support for the attribute on this element and we can add it later if we need it for styling. i.e. remove all the `*Attribute("checked"…)` code

::: toolkit/components/payments/res/containers/basic-card-form.js:6
(Diff revision 5)
>  import PaymentStateSubscriberMixin from "../mixins/PaymentStateSubscriberMixin.js";
> +import LabelledCheckbox from "../components/labelled-checkbox.js";

Nit: sort these 2 alphabetically

::: toolkit/components/payments/res/containers/basic-card-form.js:179
(Diff revision 5)
> +    } else {
> +      // Only persist state, don't update the autofill store.
> +      // This record will never get inserted into the store so we generate a faux-guid.
> +      record.guid = page.guid || "temp-" + Math.abs(Math.random() * 0xffffffff|0);

I wish we still had front-end JS assertions to assert that `record.guid` doesn't already have a value. (We could potentially use console.assert though I don't think that causes test failures yet and we'd have to allow it with eslint [which is easy]).

Both times I looked at the patch I feel nervous about how the saving is tied to the hidden checkbox.

::: toolkit/components/payments/res/containers/payment-dialog.js:163
(Diff revision 5)
> +    if (!basicCards[selectedPaymentCard]) {
>        this.requestStore.setState({
> -        selectedPaymentCard: Object.keys(savedBasicCards)[0] || null,
> +        selectedPaymentCard: Object.keys(basicCards)[0] || null,

Please mention the bug number you filed here above the [0] line.

::: toolkit/components/payments/res/containers/payment-method-picker.js:8
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  import BasicCardOption from "../components/basic-card-option.js";
>  import PaymentStateSubscriberMixin from "../mixins/PaymentStateSubscriberMixin.js";
>  import RichSelect from "../components/rich-select.js";
> +/* import-globals-from ../paymentRequest.js */

This will need to be changed to an import (please keep them sorted) upon rebasing on bug 1427939 (just a reminder).

::: toolkit/components/payments/res/paymentRequest.js:118
(Diff revision 5)
>      // Handle getting called before the DOM is ready.
>      log.debug("onShowPaymentRequest:", detail);
>      await this.domReadyPromise;
>  
>      log.debug("onShowPaymentRequest: domReadyPromise resolved");
> +    log.debug("onShowPaymentRequest, isPrivate? ", detail.isPrivate);

Nit: remove the space before the closing quote

::: toolkit/components/payments/res/paymentRequest.xhtml:30
(Diff revision 5)
>    <!ENTITY orderDetailsLabel          "Order Details">
>    <!ENTITY orderTotalLabel            "Total">
>    <!ENTITY basicCardPage.error.genericSave    "There was an error saving the payment card.">
>    <!ENTITY basicCardPage.backButton.label     "Back">
>    <!ENTITY basicCardPage.saveButton.label     "Save">
> +  <!ENTITY basicCardPage.persistCheckbox.label     "Save credit card to Firefox (Security code will not be saved)">

It seems like the "(Security code will not be saved)" may get removed depending on where we show the CVV field but it's probably fine for now.

::: toolkit/components/payments/test/browser/browser_card_edit.js:194
(Diff revision 5)
> +      return state.page.id == "basic-card-page" && !state.page.guid;
> +    },
> +                                                          "Check add page state");
> +
> +    let savedCardCount = Object.keys(state.savedBasicCards).length;
> +    let tempCardCount = Object.keys(state.tempBasicCards || {}).length;

Nit: remove the `|| {}` since it shouldn't be necessary anymore and would indicate a problem if it was needed.

::: toolkit/components/payments/test/browser/head.js:247
(Diff revision 5)
> +  let privateWin = await BrowserTestUtils.openNewBrowserWindow({private: true});
> +  registerCleanupFunction(async function() {
> +    await BrowserTestUtils.closeWindow(privateWin);
> +  });

`registerCleanupFunction` is only called between test files, not test tasks so you should be closing the window at the bottom of this function or it can interfere with later tests getting the most recent window.

::: toolkit/components/payments/test/mochitest/test_payer_address_picker.html:14
(Diff revision 5)
> +  <script src="../../res/unprivileged-fallbacks.js"></script>
> +  <script src="../../res/paymentRequest.js"></script>

Now that I landed bug 1427939 you'll need to revert these before landing or you'll get an error.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

https://reviewboard.mozilla.org/r/235392/#review244502

> If the `checked` attribute on this element doesn't cause a reaction (it doesn't now) then I don't think we should have it as it will introduce bugs. I think we should remove support for the attribute on this element and we can add it later if we need it for styling. i.e. remove all the `*Attribute("checked"…)` code

I mostly agree, but I would like a way to set the initial checked state from markup. Having the checkbox attribute drive checkedness during render, but to not be kept in sync with the checked state seemed bad.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8966689 [details]
Bug 1428415 Add a checkbox for persisting new cards to the Add Payment Card screen.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235392/diff/5-6/

Comment 16

[Sam Foster [:sfoster] (he/him)]

I've refactored the saveRecord method a little to hopefully eliminate a bit of the indirection, and make the logic a bit clearer. The persistCheckbox is ignored now for the editing case, and the isTemporary property on the temporary records is done - in favor of just looking it up in the temporary cards collection.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6711b0df641
Add a checkbox for persisting new cards to the Add Payment Card screen. r=MattN

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c6711b0df641