1429180 - Option to use a new billing address when adding a new payment card

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

When a user chooses to use a new payment card (not from existing storage), provide an option for a user to create a new billing address for that credit/debit card.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review249364

::: browser/components/payments/res/containers/basic-card-form.js:201
(Diff revision 1)
>          throw new Error("Unexpected click target");
>        }
>      }
>    }
>  
> -  saveRecord() {
> +  saveRecord(nextPageState) {

Am I missing something or is this argument never passed?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review249364

> Am I missing something or is this argument never passed?

That was leftover from when I was saving before moving to the address page. Will remove it now.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review249758

Between the `preserveInputs` and the `page` stuff this took a bit to figure out how to proceed especially since it also interacts with Pratihksha's bugs.

::: browser/components/payments/content/paymentDialogWrapper.js:516
(Diff revision 2)
>  
> +      successStateChange.page.previousPageGuid = guid;
> +
>        this.sendMessageToContent("updateState", successStateChange);

This isn't great… ideally this should be part of the state change structure that is passed along but since we don't have a good way to handle nested properties I can see why you did it this way…

When I added the `selectedStateKey` logic just above this I wondered if maybe I should have instead had the form get a GUID response and have it then set the state rather than have the wrapper do the page changing. Maybe that would be cleaner but I wasn't sure how to handle it API-wise. Thoughts?

::: browser/components/payments/res/containers/address-form.js:118
(Diff revision 2)
> -        this.requestStore.setState({
> +        let {
> +          page,
> +        } = this.requestStore.getState();
> +        let nextState = {
>            page: {
> -            id: "payment-summary",
> +            id: page.previousPageId || "payment-summary",
> +            guid: page.previousPageGuid || undefined,
>            },
> -        });
> +        };
> +        if (page.previousPageId) {

After thinking about this approach for a while, I think I made a mistake by putting the `guid` property on `state.page` because it doesn't work well when going between two add/edit pages. I'm thinking a better model would be like so:
```js
"address-page": {
  guid: …,
  title: …,
},

"basic-card-page": {
  guid: …,
},

page: {
  id: "payment-summary",
  error: …, // Since the generic errors aren't specific to a page I'm putting it here but otherwise we could duplicate the string for each relevant page?
  previousPageId: "basic-card-page",
},
```

This approach avoids the need for storing the `previousPageGuid` and `previousPageTitle` since the previous page will still have that state. It will also avoid the issue I see in debugging that the one page tries to use the guid of the wrong type of page since they both use page.guid. It will also improve performance of not re-rendering the wrong type of page on every `state.page` change.

::: browser/components/payments/res/containers/basic-card-form.js:82
(Diff revision 2)
> +      let fragment = document.createDocumentFragment();
> +      fragment.append(this.addressAddLink);
> +      fragment.append(" ");
> +      fragment.append(this.addressEditLink);
> +      let billingAddressRow = this.form.querySelector(".billingAddressRow");
> +      billingAddressRow.insertBefore(fragment, billingAddressGUID);

Nit: put the add/edit links after the <select> for consistency and better clarity IMO.

::: browser/extensions/formautofill/content/autofillEditForms.js:200
(Diff revision 2)
>  
>      this.loadRecord(record, addresses);
>      this.attachEventListeners();
>    }
>  
> -  loadRecord(record, addresses) {
> +  loadRecord(record, addresses, preserveInputs) {

Nit: `preserveFieldValues` (not just <input> and clarify that the .value is the important part).

This solution isn't addressing the root problem, which is that any re-render will throw away unsaved changes in form fields. `preserveInputs` only fixes it for one case of hitting back but any other state change will cause the same problem (try clicking the re-render button in the debug console after typing) e.g. if the user changes any autofill record in preferences. Can you file a follow-up so we can figure out a deeper fix?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review250836

::: browser/components/payments/res/containers/address-form.js:131
(Diff revision 3)
>        case this.backButton: {
> -        this.requestStore.setState({
> -          page: {
> -            id: "payment-summary",
> -          },
> -        });
> +        let state = this.requestStore.getState();
> +        state.page.id = state.page.previousId || "payment-summary";
> +        const previousId = state.page.previousId;
> +        if (previousId) {
> +          state[previousId].preserveFieldValues = true;
> +          state.page.previousId = "address-page";

Hmm… I'm not sure I understand why we would set the previousId to our back when we're going back… I would think you would set the equivalent of forwardId if we needed such a concept.

If I go: summary -> edit card -> edit address
and I click "Back" twice then shouldn't I be on the summary page? IIUC with your patch I would end up back on the address page.

::: browser/components/payments/res/containers/address-form.js:173
(Diff revision 3)
>        preserveOldProperties: true,
>        selectedStateKey: page.selectedStateKey,
>        successStateChange: {
>          page: {
> -          id: "payment-summary",
> +          id: page.previousId || "payment-summary",
> +          previousId: "address-page",

I guess I have the same question here.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review250940

::: browser/components/payments/res/containers/basic-card-form.js:171
(Diff revision 3)
> +          "basic-card-page": {
> +            preserveFieldValues: true,
> +            guid: basicCardPage.guid,
> +            title: basicCardPage.title,

Please remove the `title` property from all `state["basic-card-page"]` as it's no longer necessary since bug 1432927.

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js:27
(Diff revision 3)
> +  "payment-summary": {
> +    guid: null,

`guid` doesn't apply to "payment-summary" AFAIK

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review250836

> Hmm… I'm not sure I understand why we would set the previousId to our back when we're going back… I would think you would set the equivalent of forwardId if we needed such a concept.
> 
> If I go: summary -> edit card -> edit address
> and I click "Back" twice then shouldn't I be on the summary page? IIUC with your patch I would end up back on the address page.

This was really only necessary to say that the address has been modified. I've introduced the addressModified property for this. We don't need a "forwardId" since that would only make sense for the wizard anyways. In all other cases a forwardId doesn't make sense.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251538

::: browser/components/payments/content/paymentDialogWrapper.js:585
(Diff revision 5)
> +      const pageId = collectionName == "creditCards" ?
> +                                   "basic-card-page" :
> +                                   "address-page";
> +      successStateChange[pageId].guid = guid;

I'm not sure this is correct e.g. saving the billing address page takes you back to the card page but the `guid` here is for the address but being passed as the card GUID, right?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251538

> I'm not sure this is correct e.g. saving the billing address page takes you back to the card page but the `guid` here is for the address but being passed as the card GUID, right?

Saving the billing address page takes you back to the card page. The GUID here is for the address, and it is being assigned to the address-page, since for the billing address the collectionName is address-page. So this will not be set as the card GUID.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251538

> Saving the billing address page takes you back to the card page. The GUID here is for the address, and it is being assigned to the address-page, since for the billing address the collectionName is address-page. So this will not be set as the card GUID.

Oh… I see, sorry, so you're updating the `guid` on the page you're leaving, not necessarily the page that matches `successStateChange.page.id`.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251538

> Oh… I see, sorry, so you're updating the `guid` on the page you're leaving, not necessarily the page that matches `successStateChange.page.id`.

Correct.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251560

Clearing review because I think `addressesModified` and `successStateChange[pageId].guid` could be done in a cleaner way.

::: browser/components/payments/content/paymentDialogWrapper.js:578
(Diff revision 5)
>        // Select the new record
>        if (selectedStateKey) {
>          Object.assign(successStateChange, {
>            [selectedStateKey]: guid,
>          });
>        }
>  
> +      const pageId = collectionName == "creditCards" ?
> +                                   "basic-card-page" :
> +                                   "address-page";
> +      successStateChange[pageId].guid = guid;

I still wonder if the `successStateChange` & `errorStateChange` arguments were a mistake and instead a message should have been sent for the form to listen to a do its own state change in the unprivileged context… 
but it seems like if we're going to keep going this way we could rename `selectedStateKey` and have it take an array[1] to give the path of where to set the GUID and that would replace both of these two ways of getting the GUID set in the state.


[1] Ideally something like https://lodash.com/docs/4.17.10#set would be part of standard JS to make this easier.

::: browser/components/payments/res/containers/address-form.js:102
(Diff revision 5)
> -    this.backButton.hidden = page.onboardingWizard;
> -    this.cancelButton.hidden = !page.onboardingWizard;
> +    this.backButton.hidden = addressPage.onboardingWizard;
> +    this.cancelButton.hidden = !addressPage.onboardingWizard;

Revert these too

::: browser/components/payments/res/containers/address-form.js:150
(Diff revision 5)
> -        this.requestStore.setState({
> -          page: {
> -            id: "payment-summary",
> +        let state = this.requestStore.getState();
> +        const previousId = state.page.previousId;
> +        state.page.id = previousId || "payment-summary";

You shouldn't write to the state returned by `getState()`, especially nested objects, treat it as immutable or it will mess with state change observers since you'll directly change the old object, `page` in this case. It seems like you should do:
```js
let currentState = this.requestStore.getState();
const previousId = currentState.page.previousId;
let state = {
  page: {
    id: previousId || "payment-summary",
  }
};
if (previousId) {
  state[previousId] = Object.assign({}, currentState[previousId], {
    preserveFieldValues: true,
  });
}
this.requestStore.setState(state);
```

::: browser/components/payments/res/containers/address-form.js:196
(Diff revision 5)
> -    if (page.onboardingWizard && !Object.keys(savedBasicCards).length) {
> +    if (addressPage.onboardingWizard && !Object.keys(savedBasicCards).length) {
>        state.successStateChange = {
>          page: {
>            id: "basic-card-page",
> -          onboardingWizard: true,

`previousId` should be set here

::: browser/components/payments/res/containers/address-form.js:210
(Diff revision 5)
> -    paymentRequest.updateAutofillRecord("addresses", record, page.guid, state);
> +    state.successStateChange["address-page"] = addressPage;
> +    state.successStateChange["basic-card-page"] = basicCardPage;

I think we should only need to do this for the `previousId` if we make the `successStateChange` change as we wouldn't modify address-page at all in the success case, we would be able to set `state["basic-card-page"].billingAddressGUID = ${guid from the save set in the wrapper}`

::: browser/components/payments/res/containers/basic-card-form.js:156
(Diff revision 5)
> +    if (basicCardPage.addressesModified) {
> +      let addressGuid = state["address-page"].guid;

To avoid the need for `basicCardPage.addressesModified` (which isn't very clear), I think we can set `basicCardPage.billingAddressGUID` with the other changes I proposed in the wrapper (and in successStateChange) and then have this code set the billingAddressGUID option if it's present.

::: browser/components/payments/res/containers/basic-card-form.js:159
(Diff revision 5)
> +      billingAddressSelect.selectedIndex =
> +        [...billingAddressSelect.children].findIndex(o => o.value == addressGuid);

`billingAddressSelect.value = addressGuid`

::: browser/components/payments/res/containers/payment-dialog.js:262
(Diff revision 5)
> -    this._header.hidden = !state.page.onboardingWizard && state.page.id != "payment-summary";
> +    this._header.hidden = !state["address-page"].onboardingWizard &&
> +                          state.page.id != "payment-summary";

Please revert this as `onboardingWizard` also applies to the basic-card-page. I think for now it makes sense to keep that property on `state.page` since it's shared.

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js:18
(Diff revision 5)
> +  "basic-card-page": {
> +    guid: null,
> +    // onboardingWizard: true,
> +  },

Nit: add `preserveFieldValues`

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js:27
(Diff revision 5)
> +  "payment-summary": {
> +    title: "",
> +  },

Please remove the unused `title` property.

::: browser/components/payments/res/paymentRequest.js
(Diff revision 5)
> -        onboardingWizard: true,
>          guid: null,
>        };
>      }
> +    if (Object.keys(detail.savedBasicCards).length == 0) {
> +      state["basic-card-page"] = {
> +        onboardingWizard: true,
> +      };
> +    }

These should be combined and the `guid` shouldn't be on `state.page`… maybe a rebase issue.

::: browser/extensions/formautofill/content/autofillEditForms.js:200
(Diff revision 5)
>  
>      this.loadRecord(record, addresses);
>      this.attachEventListeners();
>    }
>  
> -  loadRecord(record, addresses) {
> +  loadRecord(record, addresses, preserveInputs) {

rename this argument too

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

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

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

https://reviewboard.mozilla.org/r/245588/#review251652

::: browser/components/payments/content/paymentDialogWrapper.js:586
(Diff revision 1)
> +          let subObj = Object.assign({}, successStateChange[selectedStateKey[0]]);
> +          subObj[selectedStateKey[1]] = guid;
> +          Object.assign(successStateChange, {
> +            [selectedStateKey[0]]: subObj,

I'm not too happy with how this turned out but I couldn't find a better way to do this.

We could do something as simple as:
`successStateChange[selectedStateKey[0]][selectedStateKey[1] = guid;` but that would fail if the successStateChange object didn't have a [selectedStateKey[0]] member.

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

https://reviewboard.mozilla.org/r/245588/#review251652

> I'm not too happy with how this turned out but I couldn't find a better way to do this.
> 
> We could do something as simple as:
> `successStateChange[selectedStateKey[0]][selectedStateKey[1] = guid;` but that would fail if the successStateChange object didn't have a [selectedStateKey[0]] member.

Yeah, I think it's okay as-is though I still think we should rename this since it's different than @selectedStateKey. e.g. `savedGUIDPath` or `GUIDSavePath` (`guidSavePath`) or something else?

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

https://reviewboard.mozilla.org/r/245588/#review251662

::: browser/components/payments/content/paymentDialogWrapper.js:591
(Diff revision 1)
> +          let subObj = Object.assign({}, successStateChange[selectedStateKey[0]]);
> +          subObj[selectedStateKey[1]] = guid;
> +          Object.assign(successStateChange, {
> +            [selectedStateKey[0]]: subObj,
> -        });
> +          });
> -      }
> +        }

Maybe add an `} else {` and `throw new Error(…)` to catch bugs

::: browser/components/payments/res/containers/address-form.js:218
(Diff revision 1)
>          },
>        };
>      }
>  
> -    state.successStateChange["address-page"] = addressPage;
>      state.successStateChange["basic-card-page"] = basicCardPage;

This line seems out of place or incorrect. When you're just editing a shipping address from the summary page, we shouldn't be setting the "basic-card-page" state.

::: browser/components/payments/res/containers/address-picker.js:131
(Diff revision 1)
>      for (let option of desiredOptions) {
>        this.dropdown.popupBox.appendChild(option);
>      }
>  
>      // Update selectedness after the options are updated
> -    let selectedAddressGUID = state[this.selectedStateKey];
> +    let selectedAddressGUID = state[this.selectedStateKey[0]];

I was thinking of just changing the API for updating autofill records in the wrapper, not changing @selectedStateKey. It seems kinda wrong to ignore everything but the first index but I do kinda like how you used an array for state.page.selectedStateKey for the billing address case so up to you what we do here. Can we leave the attributes only supporting a single value while having the update API and state.page supporting an array?

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251668

Thanks

::: browser/components/payments/res/containers/address-form.js:193
(Diff revision 6)
>            id: "address-page",
> -          onboardingWizard: page.onboardingWizard,
>            error: this.dataset.errorGenericSave,

I think this should be reverted so that the `onboardingWizard` property is preserved (and the header remains visible) if there is an error.

::: browser/components/payments/res/containers/address-form.js:206
(Diff revision 6)
>  
>      if (page.onboardingWizard && !Object.keys(savedBasicCards).length) {
>        state.successStateChange = {
>          page: {
>            id: "basic-card-page",
> -          onboardingWizard: true,
> +          previousId: "address-page",

I think `onboardingWizard` should remain here too

::: browser/components/payments/res/containers/basic-card-form.js:30
(Diff revision 6)
> +    this.addressAddLink = document.createElement("a");
> +    this.addressAddLink.href = "javascript:void(0)";
> +    this.addressAddLink.addEventListener("click", this);
> +    this.addressEditLink = document.createElement("a");
> +    this.addressEditLink.href = "javascript:void(0)";
> +    this.addressEditLink.addEventListener("click", this);

Please add add-link and edit-link classes respectively like we've done on the other add/edit links and please use them in the tests you added instead of a:last-of-type and similar

::: browser/components/payments/res/containers/basic-card-form.js:257
(Diff revision 6)
>        successStateChange: {
>          page: {
>            id: "payment-summary",
>          },
> +        "address-page": addressPage,
> +        "basic-card-page": basicCardPage,

I don't see why we need these two properties set?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

https://reviewboard.mozilla.org/r/245588/#review251662

> I was thinking of just changing the API for updating autofill records in the wrapper, not changing @selectedStateKey. It seems kinda wrong to ignore everything but the first index but I do kinda like how you used an array for state.page.selectedStateKey for the billing address case so up to you what we do here. Can we leave the attributes only supporting a single value while having the update API and state.page supporting an array?

Okay, that's fine with me. I had thought it would be more consistent to use arrays everywhere but it's not necessary and can make the code harder to read.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

https://reviewboard.mozilla.org/r/243482/#review251668

> I don't see why we need these two properties set?

We need to set the state of the previousId if it's set. I've updated this to use the same approach that is used by address-form.js to include the page state of the previousId if previousId is set.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

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

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

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

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975119 [details]
Bug 1429180 - Option to use a new billing address when adding a new payment method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243482/diff/7-8/

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8979416 [details]
Bug 1429180 - Change selectedStateKey to an array that defines the path within the state object that should be updated.

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02cc06146058
Option to use a new billing address when adding a new payment method. r=MattN
https://hg.mozilla.org/integration/autoland/rev/88f302c4a1eb
Change selectedStateKey to an array that defines the path within the state object that should be updated. r=MattN

Comment 30

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/02cc06146058
https://hg.mozilla.org/mozilla-central/rev/88f302c4a1eb