Closed Bug 1462779 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: prathiksha, Assigned: prathiksha)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

Attachments

(2 files, 1 obsolete file)

Bug 1462779 - Auto-select appropriate addresses in select dropdowns in on-boarding forms.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1462779 - Show the billing address page during on-boarding if requestShipping is false and there are no saved addresses.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
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.
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [webpayments]
Blocks: 1427935
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 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.
Attachment #8979412 - Flags: review?(MattN+bmo)
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
}
```
Attachment #8979412 - Flags: review?(MattN+bmo) → review+
Attachment #8979412 - Attachment is obsolete: true
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 = ["…"];
```
Attachment #8980467 - Flags: review?(MattN+bmo)
Attachment #8979412 - Attachment is obsolete: false
Attachment #8979412 - Attachment is patch: true
Attachment #8979412 - Attachment mime type: text/x-review-board-request → text/plain
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 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.
Attachment #8980467 - Flags: review?(MattN+bmo)
Attachment #8979412 - Attachment is obsolete: true
Attachment #8979412 - Attachment is patch: false
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"
Attachment #8981618 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8980467 - Flags: review?(MattN+bmo) → review+
Pushed by prathikshaprasadsuman@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/def9442b2a81
https://hg.mozilla.org/mozilla-central/rev/df80a62cc91f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.