Closed Bug 1427939 Opened 3 years ago Closed 2 years ago

Show additional list items (`additionalDisplayItems`) tied to payment methods in the UI

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Whiteboard: [webpayments])

Attachments

(4 files, 2 obsolete files)

If provided, additional display items can be added for specific `supportedMethods`. These should be shown in the UI according to the UX spec.


https://w3c.github.io/payment-request/#dom-paymentdetailsmodifier-additionaldisplayitems
Priority: P3 → P1
Assignee: nobody → sfoster
Comment on attachment 8949924 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/219226/#review225384

::: commit-message-c2cdd:5
(Diff revision 1)
> +Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method. r?MattN
> +
> +* _getTotalItem, _getModifier, _getAdditionalDisplayItems methods are a straw man. These could be static methods on the RequestStore, or in a new StoreHelpers library or something?
> +
> +* Default to simply using the first paymentMethod - in lieu of support any other types.

I think the DOM code will pass a request through to the UI as long as one of the methods in supported so the first one may not be basic-card. On IRC I was suggesting that we could have all of the credit card shipping methods indicate that they are the basic-card type and then reference that method when deciding which additional display items to show.

::: toolkit/components/payments/test/browser/browser_total.js:19
(Diff revision 1)
> +    // We expect the *first* payment method to be selected when applying modifiers
> +    is(content.document.querySelector("#total > currency-amount").textContent,
> +       "$3.50",
> +       "Check modified total currency amount");
> +  };
> +  const args = {
> +    methodData: [PTU.MethodData.bobPay, PTU.MethodData.basicCard],
> +    details: PTU.Details.bobPayPaymentModifier,
> +  };

Since we don't support bobpay I don't think this is the desired outcome. See my comment on the commit message also.
Attachment #8949924 - Flags: review?(MattN+bmo)
I just filed a spec issue (https://github.com/w3c/payment-request/issues/684) to get the spec to clarify how we should handle multiple applicable modifiers for the selected method. For now I think we can assume that doesn't happen and file a follow-up for it if we need to change behaviour.
Status: NEW → ASSIGNED
Lower the priority until we figure out whether we are going to ship modifiers.
Priority: P1 → P4
Comment on attachment 8949924 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/219226/#review229734

This is the WIP for handling modifiers for totalItem and additionalDisplayItems. 

* Tests are sketched in. The mochitests pass, browser tests may not be currently passing. 
* There's some code duplication across payment-dialog and order-details, as both need a way to look up and render the currently applicable total.
* The helper library is a placeholder location for the getModifierForPaymentMethod method. I'm not sure where this should live. It is DOM agnostic and doesnt really make sense as a component method. Could be a moot point if we end up not using modifiers. If we keep the helper lib, my plan was to add a few unit tests for it.
Comment on attachment 8949924 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/219226/#review229736

::: toolkit/components/payments/test/mochitest/test_order_details.html:98
(Diff revision 2)
>        amount: { currency: "USD", value: "7" },
>      },
>    ];
> -  requestStore.setState({ paymentDetails });
> +  Object.assign(request, { paymentDetails });
> +  requestStore.setState(state);
> +

This change is probably unnnecessary as I guess requestStore.setState already does exactly this. I was probably troubleshooting some other test issue when I made this change
Ditto above and below
Attachment #8949924 - Flags: review?(MattN+bmo)
Assignee: sfoster → nobody
Status: ASSIGNED → NEW
Priority: P4 → P2
Whiteboard: [webpayments]
It seems like we'll have to implement this for webcompat :(
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8968712 [details]
Bug 1427939 - Ensure payment methods (e.g. credit cards) always have a methodName property.

https://reviewboard.mozilla.org/r/237418/#review243248
Attachment #8968712 - Flags: review?(MattN+bmo) → review+
Attachment #8954620 - Attachment is obsolete: true
Attachment #8949924 - Attachment is obsolete: true
Comment on attachment 8968815 [details]
Bug 1427939 - Fix console logging line numbers for paymentRequest.xhtml over HTTP.

https://reviewboard.mozilla.org/r/237544/#review243438

Looks good.
Attachment #8968815 - Flags: review?(sfoster) → review+
Comment on attachment 8968814 [details]
Bug 1427939 - Convert paymentRequest.js to an ES module.

https://reviewboard.mozilla.org/r/237542/#review243450

::: browser/base/content/test/static/browser_parsable_script.js:13
(Diff revision 2)
>  const kWhitelist = new Set([
>    /browser\/content\/browser\/places\/controller.js$/,
>  ]);
>  
>  const kESModuleList = new Set([
>    /toolkit\/res\/payments\/(components|containers|mixins)\/.*.js$/,

Nit: I guess it doesnt matter, but these '.' could be escaped too as '.' just means any single character, which in this case happens to match a literal '.' in the filename.

::: toolkit/components/payments/res/paymentRequest.js
(Diff revision 2)
>   * Communicates with privileged code via DOM Events.
>   */
>  
>  /* import-globals-from unprivileged-fallbacks.js */
>  
> -"use strict";

FWIW I didn't get any complaint from eslint with or without this "use strict"; here.
Attachment #8968814 - Flags: review?(sfoster) → review+
Comment on attachment 8968713 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/237420/#review244258

::: toolkit/components/payments/res/paymentRequest.js:171
(Diff revision 4)
> +   * @param {string} methodID (GUID) uniquely identifying the selected payment method
> +   * @returns {object?} the applicable modifier for the payment method
> +   */
> +  getModifierForPaymentMethod(state, methodID) {
> +    let method = state.savedBasicCards[methodID] || null;
> +    if (method.methodName !== "basic-card") {

This is a null dereference if a card with that GUID doesn't exist. Since you're throwing with a helpful error message if the methodName doesn't match then you should also check that method is non-null here too.

::: toolkit/components/payments/res/paymentRequest.js:181
(Diff revision 4)
> +      return null;
> +    }
> +    let modifier = modifiers.find(m => {
> +      // take the first matching modifier
> +      // TODO: match on supportedTypes and supportedNetworks
> +      return (m.supportedMethods == "basic-card");

nit, no parens

::: toolkit/components/payments/test/browser/browser_total.js:25
(Diff revision 4)
> +       "$2.00",
> +       "Check unmodified total currency amount");
> +  };
> +  const args = {
> +    methodData: [PTU.MethodData.bobPay, PTU.MethodData.basicCard],
> +    details: PTU.Details.bobPayPaymentModifier,

bobPayPaymentModifier.total is $2 USD by default, but bobPayPaymentModifier.displayItems is only $1 USD. This doesn't cause any issues but we should fix displayItems to match the total at least so the testcase isn't unnecessarily confusing.

::: toolkit/components/payments/test/mochitest/test_order_details.html:51
(Diff revision 4)
> +  let johnDoeCard = deepClone(PTU.BasicCards.JohnDoe);
> +  johnDoeCard.methodName = "basic-card";

Can you update PTU to have the methodName specified on each of the BasicCards instead of doing it here?

::: toolkit/components/payments/test/mochitest/test_order_details.html:192
(Diff revision 4)
> +  is(orderDetails.totalAmountElem.value, "3.5", "total amount uses modifier total");
> +  is(orderDetails.totalAmountElem.currency, "USD", "total currency uses modifier currency");

Can you please add tests for the label ("Total due" vs "foo")?
Attachment #8968713 - Flags: review?(jaws) → review+
Comment on attachment 8968713 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/237420/#review244258

> Can you update PTU to have the methodName specified on each of the BasicCards instead of doing it here?

That won't work without causing other problems since autofill storage will complain that `methodName` isn't a supported field when tests try to add it to storage. I thought about this but it seemed like it was better as-is. Thoughts?
Comment on attachment 8968713 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/237420/#review244258

> That won't work without causing other problems since autofill storage will complain that `methodName` isn't a supported field when tests try to add it to storage. I thought about this but it seemed like it was better as-is. Thoughts?

Yeah this is fine then.
Comment on attachment 8968713 [details]
Bug 1427939 - Use total and any additionalDisplayItems when a modifier matches the payment method.

https://reviewboard.mozilla.org/r/237420/#review244258

> Can you please add tests for the label ("Total due" vs "foo")?

Usage of that string never got implemented (with or without modifiers; order-details or the summary page) and I'm not sure if UX wants us to… IIRC the spec doesn't require us to use it if we don't want. I filed bug 1456034 for this.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/d418568f8b26
Convert paymentRequest.js to an ES module. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/df6d1bec2d29
Fix console logging line numbers for paymentRequest.xhtml over HTTP. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/36ee9b56e84d
Ensure payment methods (e.g. credit cards) always have a methodName property. r=MattN
https://hg.mozilla.org/integration/autoland/rev/f3ad6a992444
Use total and any additionalDisplayItems when a modifier matches the payment method. r=jaws
Depends on: 1457316
Product: Toolkit → Firefox
Target Milestone: mozilla61 → Firefox 61
You need to log in before you can comment on or make changes to this bug.