1462779 - Show the billing address page on on-boarding when requestShipping is false and there are no saved addresses

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[:prathiksha]

When requestShipping is false and there are no saved addresses, we show the add_billing_address page before the basic card page on on-boarding. This way we can avoid having an empty address picker when the user enters the basic card page.

Comment 1

[Matthew N. [:MattN]]

Btw. bug 1462779 is adding:
> <!ENTITY billingAddress.addPage.title   "Add Billing Address">
> <!ENTITY billingAddress.editPage.title  "Edit Billing Address">

so depending upon which lands first this can be shared.

Comment 2

[:prathiksha]

Attachment: Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8979412 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

https://reviewboard.mozilla.org/r/245586/#review251626

::: browser/components/payments/res/paymentRequest.js:141
(Diff revision 1)
> +      } else {
> +        state.page.title = paymentDialog.dataset.billingAddressTitleAdd;

I think you forgot to take into account whether there are cards or not… we shouldn't show the billing address form if the user has a saved card.

Comment 4

[:prathiksha]

Comment on attachment 8979412 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8979412 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

https://reviewboard.mozilla.org/r/245586/#review251986

::: browser/components/payments/res/paymentRequest.js:119
(Diff revision 2)
> +    let noSavedAddresses = Object.keys(detail.savedAddresses).length == 0;
> +    let noSavedCards = Object.keys(detail.savedBasicCards).length == 0;

I think it would be easier to read if we avoid double-negatives by making these variables indicate that there is saved data.

::: browser/components/payments/res/paymentRequest.js
(Diff revision 2)
> -      page: {
> -        id: "payment-summary",
> -      },
>      };
>  
>      // Onboarding wizard flow.
> -    if (Object.keys(detail.savedAddresses).length == 0) {
> +    if ((!noSavedCards && !noSavedAddresses) || (!shippingRequested && !noSavedCards)) {
> +      state.page = {
> +        id: "payment-summary",

I think it was easier to understand if we leave the default page as payment-summary but override for the FTU in specific cases. e.g. (not necessarily accurate)
```js
if (!hasSavedAddress && (shippingRequested || !hasSavedCards) {
  // address-page
} else if (!hasSavedCards) {
  // basic-card-page
}
```

Comment 6

[:prathiksha]

Comment on attachment 8979412 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 7

[:prathiksha]

Attachment: Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

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

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

https://reviewboard.mozilla.org/r/246630/#review252756

Hello, I don't have time to manually test this yet but I have some comments for now.

::: commit-message-3dc6c:1
(Diff revision 1)
> +Bug 1462779 - Auto-select appropriate addresses in menulists on on-boarding forms. r?MattN

Nit: we don't call them menulists usually

::: browser/components/payments/res/containers/basic-card-form.js:162
(Diff revision 1)
> -      let addressGuid = basicCardPage.billingAddressGUID;
> -      let billingAddressSelect = this.form.querySelector("#billingAddressGUID");
> +    let billingAddressSelect = this.form.querySelector("#billingAddressGUID");
> -      billingAddressSelect.value = addressGuid;
> +    if (basicCardPage.billingAddressGUID) {
> +      billingAddressSelect.value = basicCardPage.billingAddressGUID;
> +    } else {
> +      billingAddressSelect.value = Object.keys(addresses)[0];

I think we shouldn't auto-select an address when editing as the user may not know that clicking back would mean that the billing address would not be associated with the card.

::: browser/components/payments/res/paymentRequest.js:148
(Diff revision 1)
> +        Object.assign(state.page, {selectedStateKey: ["selectedShippingAddress"]});
>        } else {
>          Object.assign(state["address-page"], {
>            title: paymentDialog.dataset.billingAddressTitleAdd,
>          });
> +        Object.assign(state.page, {selectedStateKey: ["basic-card-page", "billingAddressGUID"]});

Nit: These two places don't need Object.assign and would be easier to read as follows:
```js
state.page.selectedStateKey = ["…"];
```

Comment 9

[:prathiksha]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

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

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

https://reviewboard.mozilla.org/r/246630/#review253728

Please rebase the two commits for this bug onto latest mozilla-central since there are some conflicts with Jared's changes. Also, make sure when you push for review that you push both commits using `-r` (`-c` is only for one commit). e.g. `hg push review central -r 84567af75:985fed43`

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

https://reviewboard.mozilla.org/r/246630/#review253738

::: browser/components/payments/res/paymentRequest.js:153
(Diff revision 2)
> +        state.page.selectedStateKey = ["selectedShippingAddress"];
>        } else {
>          Object.assign(state["address-page"], {
>            title: paymentDialog.dataset.billingAddressTitleAdd,
>          });
> +        state.page.selectedStateKey = ["basic-card-page", "billingAddressGUID"];

Can you please add a test for this behaviour since I think it would be easy to regress the behaviour of the selected state key getting carried over to the next (card) page since it's in different code than what actually sets decides which page to show.

Comment 12

[:prathiksha]

Attachment: Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 13

[:prathiksha]

Comment on attachment 8981618 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 14

[:prathiksha]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

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

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8981618 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

https://reviewboard.mozilla.org/r/247730/#review253844

Thanks

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:45
(Diff revision 2)
>                 state["address-page"].selectedStateKey == "selectedShippingAddress";
>        }, "Address page is shown first during on-boarding if there are no saved addresses");
>  
> +      info("Checking if the address page has been rendered");
> +      let addressSaveButton = content.document.querySelector("address-form .save-button");
> +      ok(content.isVisible(addressSaveButton), "Address page is rendered");

The old message was better: "Address save button is rendered"

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

https://reviewboard.mozilla.org/r/246630/#review253846

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:342
(Diff revision 3)
>        ok(content.isVisible(cardSaveButton), "Basic card page is rendered");
>  
>        for (let [key, val] of Object.entries(PTU.BasicCards.JohnDoe)) {

What I mean to test is here that the billing address is selected by default in the dropdown.

Comment 17

[:prathiksha]

Comment on attachment 8981618 [details]
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.

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

Comment 18

[:prathiksha]

Comment on attachment 8980467 [details]
Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/def9442b2a81
Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses. r=MattN
https://hg.mozilla.org/integration/autoland/rev/df80a62cc91f
Auto-select appropriate addresses in select dropdowns in on-boarding forms. r=MattN

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/def9442b2a81
https://hg.mozilla.org/mozilla-central/rev/df80a62cc91f