1427945 - Implement a Payment Request Shipping Option Picker

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

* Render the shipping options provided by the web page (accessible in the state exposed via PaymentStateSubscriberMixin)
** The options can change based upon some events so this should use the mixin
* Save the selected shipping option in the state
* Handle the selected shipping option no longer being available (e.g. if the user changes their shipping address)

Comment 1

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

Attachment: Bug 1427945 - Implement a Payment Request Shipping Option Picker.

todo: browser_change_shipping.js is failing currently, because the shipping options rich-select isn't getting populated with the options that are passed in to the PaymentRequest constructor. Also, it doesn't appear that the code to change the selected shipping address is changing it.

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

Comment 2

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

Comment on attachment 8948811 [details]
Bug 1427945 - Implement a Payment Request Shipping Option Picker.

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

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8948811 [details]
Bug 1427945 - Implement a Payment Request Shipping Option Picker.

https://reviewboard.mozilla.org/r/218180/#review224648

Thanks. r=me with removal of `setState` from `render`

::: commit-message-2638a:3
(Diff revision 2)
> +Bug 1427945 - Implement a Payment Request Shipping Option Picker. r?mattn
> +
> +todo: browser_change_shipping.js is failing currently, because the shipping options rich-select isn't getting populated with the options that are passed in to the PaymentRequest constructor. Also, it doesn't appear that the code to change the selected shipping address is changing it.

I think you meant to remove this

::: toolkit/components/payments/res/components/shipping-option.js:20
(Diff revision 2)
> +  constructor() {
> +    super();
> +
> +    this._amount = document.createElement("currency-amount");

Maybe be should define `this.amount = null` in the constructor so it's clear that it's a thing. Normally with object literals we would define it as a property but that's not valid with a class.

::: toolkit/components/payments/res/components/shipping-option.js:36
(Diff revision 2)
> +    if (this.amount) {
> +      this._amount.currency = this.amount.currency;
> +      this._amount.value = this.amount.value;
> +    }

The problem with this approach is that it doesn't handle the amount getting cleared but I guess that shouldn't happen in practice. In that same vein though we could assume that this.amount is always truthy I think.

::: toolkit/components/payments/res/containers/shipping-option-picker.js:68
(Diff revision 2)
> +    }
> +    if (!selectedShippingOption) {
> +      return;
> +    }
> +    let selectedOptionEl = this.dropdown.getOptionByValue(selectedShippingOption);
> +    if (selectedShippingOption && !selectedOptionEl) {

`selectedShippingOption` is always truthy at this point so it can be removed from the condition

::: toolkit/components/payments/res/containers/shipping-option-picker.js:73
(Diff revision 2)
> +    if (!stateHadSelectedOption) {
> +      this.requestStore.setState({
> +        selectedShippingOption,
> +      });
> +    }

`render` functions generally shouldn't have side-effects outside of themselves as that can cause problems like infinite loops. I think if you move determination of `selectedShippingOption` to `setStateFromParent` like I did for `selectedShippingAddress` and `selectedPaymentCard` then you will be able to get rid of this.

::: toolkit/components/payments/res/paymentRequest.xhtml:55
(Diff revision 2)
>            <address-picker selected-state-key="selectedShippingAddress"></address-picker>
>          </section>
>  
> +        <section>
> +          <div><label>Shipping Options</label></div>
> +          <shipping-option-picker></shipping-option-picker>
> +        </section>

From working on the payment method picker, I think the current grid layout wants this inside the same <section>… I was initially confused by this as well. You will notice that the existing <section> has auto height and your new section would have the footer layout which isn't what you want. We can change the markup to a better version once we have the final visual spec.

::: toolkit/components/payments/test/PaymentTestUtils.jsm:63
(Diff revision 2)
> +      let select = content.document.querySelector("shipping-option-picker > rich-select");
> +      let popupBox = select.querySelector(".rich-select-popup-box");

In case you were wondering how to access the properties like .popupBox, you have to `Cu.waiveXrays`. The properties would be slightly less fragile if we change DOM layout.

::: toolkit/components/payments/test/PaymentTestUtils.jsm:89
(Diff revision 2)
> +      let option = select.querySelector(`[country="${country}"]`);
> +      select.click();
> +      option.click();
> +    },
> +
> +    selectShippingOptionByValue: value => {

Nit: `…ById` may be a bit clearer

::: toolkit/components/payments/test/PaymentTestUtils.jsm:155
(Diff revision 2)
>        total: {
>          label: "Total due",
>          amount: { currency: "USD", value: "60.00" },
>        },
>      },
> +    total60EUR: {

You should rename this to describe the the details… currently the total doesn't match the description but if you can figure out a good name to include other details about the `Details` in the name that would probably help to re-use them in the future. Even just `total75EUR_with2Shipping` or similar. IIRC from the spec it's supposed to be invalid to receive shipping options if requestShipping is false so knowing that a  details has shipping is relevant when testing (I filed bug 1436903 to fix the DOM code). This means `total60USD` should probably also be renamed. You should include `requestShipping` to ensure we don't break when the DOM starts enforcing this.

You should also consider re-using `twoShippingOptions` that Sam added instead of this new one.

::: toolkit/components/payments/test/browser/browser_change_shipping.js:114
(Diff revision 2)
> +    let methodDetails = result.methodDetails;
> +    is(methodDetails.cardholderName, "John Doe", "Check cardholderName");
> +    is(methodDetails.cardNumber, "9999999999", "Check cardNumber");
> +    is(methodDetails.expiryMonth, "01", "Check expiryMonth");
> +    is(methodDetails.expiryYear, "9999", "Check expiryYear");
> +    is(methodDetails.cardSecurityCode, "999", "Check cardSecurityCode");

FYI: This will change with a rebase on top of my commit

::: toolkit/components/payments/test/mochitest/test_shipping_option_picker.html:78
(Diff revision 2)
> +  await asyncElementRendered();
> +  let options = picker1.dropdown.popupBox.children;
> +  is(options.length, 2, "Check dropdown has both options");
> +  ok(options[0].textContent.includes("Carrier Pigeon"), "Check first option");
> +  is(options[0].querySelector(".amount").getAttribute("currency"), "USD", "Check currency");
> +  ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option");
> +});

Shouldn't you test which one is selected by default?

::: toolkit/components/payments/test/mochitest/test_shipping_option_picker.html:114
(Diff revision 2)
> +  await asyncElementRendered();
> +  let options = picker1.dropdown.popupBox.children;
> +  is(options.length, 2, "Check dropdown still has both options");
> +  ok(options[0].textContent.includes("Tortoise"), "Check updated first option");
> +  is(options[0].querySelector(".amount").getAttribute("currency"), "CAD", "Check currency");
> +  ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option is the same");

Same here. I would move this from the beginning of the next function as I think it makes more sense together with the setState

::: toolkit/components/payments/test/mochitest/test_shipping_option_picker.html:133
(Diff revision 2)
> +  options[0].click();
> +  await asyncElementRendered();
> +
> +  state = picker1.requestStore.getState();

You should use `promiseStateChange` (added in the payment method commit) before the `asyncElementRendered`. It can replace the `getState` line too.

::: toolkit/components/payments/test/mochitest/test_shipping_option_picker.html:164
(Diff revision 2)
> +  await asyncElementRendered();
> +  let options = picker1.dropdown.popupBox.children;
> +  is(options.length, 1, "Check dropdown has one remaining address");
> +  ok(options[0].textContent.includes("Tortoise"), "Check remaining address");
> +});

You should probably also check what is selected here.

Comment 4

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

Comment on attachment 8948811 [details]
Bug 1427945 - Implement a Payment Request Shipping Option Picker.

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

Comment 5

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

Comment on attachment 8948811 [details]
Bug 1427945 - Implement a Payment Request Shipping Option Picker.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1cc283d8485
Implement a Payment Request Shipping Option Picker. r=MattN

Comment 7

[Noemi Erli[:noemi_erli]]

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