1429198 - Account for supportedNetworks in payment modifiers

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

A site can specify `supportedTypes` and `supportedNetworks` for basic-card and the UI shouldn't allow the user to select a card which doesn't match those restrictions.

See:
https://w3c.github.io/payment-method-basic-card/#dom-basiccardrequest
https://www.w3.org/Payments/card-network-ids
https://w3c.github.io/payment-method-basic-card/#dom-basiccardtype

Comment 2

[Matthew N. [:MattN]]

https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/browser/components/payments/res/paymentRequest.js#253

Comment 3

[Matthew N. [:MattN]]

Line 247 should use this.getBasicCards to include temp. cards
Around line 257 we should compare the card's network to the supportedNetworks for the modifiers.

Spec issues at https://github.com/w3c/payment-request/issues/684#issuecomment-405612400 and https://github.com/w3c/payment-method-basic-card/issues/60 talk about first vs. last matching.

https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/browser/components/payments/res/paymentRequest.js#247,257

Comment 4

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

I modified `test_order_details.html` since it was failing. In the test, a card object didn't have a guid attribute, which is an expected attribute on calling `getBasicCards > _sortObjectsByTimeLastUsed`.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9026679 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9026679 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/components/payments/res/paymentRequest.js
@@ +257,4 @@
>      let modifier = modifiers.find(m => {
>        // take the first matching modifier
> +      return m.supportedMethods == "basic-card" &&
> +             acceptedNetworks.includes(method["cc-type"]);

I think there is a bit of confusion… You need to be matching the selected payment methods' cc-type to the supportedNetworks of modifier.data

Comment 6

[Matthew N. [:MattN]]

I don't see any WPT for supportedNetworks in modifiers, is this correct? It seems like a significant thing to test.

Comment 7

[Marcos Caceres [:marcosc]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #6)
> I don't see any WPT for supportedNetworks in modifiers, is this correct? 

Yeah, they were added before my time :( That's why "Modifiers and [[serializedModifierData]] not actually used anywhere": 
https://github.com/w3c/payment-request/issues/753

(and similarly with the ordering issue you raised)

> It seems like a significant thing to test.

I'll add them when I fix #735.

Comment 8

[Diego Pino]

I dumped a modifier at that point in code and the properties I see are:

```
additionalDisplayItems: [object Object]
supportedMethods: basic-card
total: [object Object]
```

There's no data, although PaymentDetailsModifier features that attribute https://searchfox.org/mozilla-central/source/dom/payments/PaymentRequestData.cpp#165.

Comment 9

[Matthew N. [:MattN]]

Attachment: Screen Shot showing request.paymentDetails.modifiers[1].data.supportedNetworks

https://rsolomakhin.github.io/pr/ is a test page with a mastercard discount (unfortunately it also tests supportedTypes which was dropped from the spec).

The following code works for me fine:
```js
    let modifier = modifiers.find(m => {
      // m.supportedMethods is a string despite the plural name kept for backwards-compat from when it was an sequence.                                                           
      if (m.supportedMethods && m.supportedMethods != method.methodName) {
        return false;
      }

      // XXX                                                                                                                                                                      
      if (m.data.supportedTypes) {
       	return false;
      }

      if (m.data.supportedNetworks && !m.data.supportedNetworks.includes(method["cc-type"])) {
        return false;
      }

      return true;
    });
```

We should also be applying the same technique as https://searchfox.org/mozilla-central/rev/8f89901f2d69d9783f946a7458a6d7ee70635a94/browser/components/payments/content/paymentDialogWrapper.js#395-397 to temporary cards in paymentDialogWrapper so that we don't need to hard-code "basic-card" in getModifierForPaymentMethod and can instead compare `m.supportedMethods == method.methodName`

Thanks

Comment 10

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Thanks for the feedback. I incorporated the changes suggested, although I dropped the check for `supportedTypes` (rsolomakhin/pr works either way without that check).

Regarding the missing `data` field I couldn't find, the reason was that I was testing `test_order_details.html` and that field was not defined in the data samples, so I further modified the test.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 9026992 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9026992 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/payments/res/paymentRequest.js
@@ +310,5 @@
> +    for (let card of Object.values(state.tempBasicCards)) {
> +      if (!card.methodName) {
> +        card.methodName = "basic-card";
> +      }
> +    }

As I mentioned in comment 9, I think this should be in paymentDialogWrapper so it's consistent with the persistent records

::: browser/components/payments/test/mochitest/test_order_details.html
@@ +186,4 @@
>      }],
>    });
>    requestStore.setState({request});
>    await asyncElementRendered();

Can you add test cases that ensure that supportedNetworks is actually being honored?

Comment 12

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

I addressed the comments in previous review:

* Added a method `fetchTempPaymentCards` in `paymentDialogWrapper` that ensures cards have a method name.
* Modified existing test `test_modified_total` adding only one card in `supportedNetworks`. That's necessary to pass the test otherwise the modifier is not applied.
* Added a new test `test_non_supported_network()` that tries to apply a modifier but cannot apply it because the card's network is not supported.
* Modified `test_constructor.html` adding `data.supportedNetworks`. Not strictly necessary for this patch.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 9027876 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9027876 [details] [diff] [review]:
-----------------------------------------------------------------

Marcos, FYI that I feel that how `supportedNetworks: []` is handled for modifiers isn't clearly defined in the specs.

::: browser/components/payments/content/paymentDialogWrapper.js
@@ +515,5 @@
>        isPrivate,
>      });
>    },
>  
> +  fetchTempPaymentCards() {

Nit: Can you move this near `fetchSavedPaymentCards`

::: browser/components/payments/res/paymentRequest.js
@@ +258,5 @@
> +      if (m.supportedMethods && m.supportedMethods != method.methodName) {
> +        return false;
> +      }
> +      if (m.data && m.data.supportedNetworks &&
> +          !m.data.supportedNetworks.includes(method["cc-type"])) {

I think you need to add a check (and test) for `supportedNetworks: []` (`… && m.data.supportedNetworks.length > 0`) since I think an empty array means that the modifier should apply.

https://w3c.github.io/payment-method-basic-card/#steps-to-check-if-an-instrument-is-supported

::: browser/components/payments/test/mochitest/test_order_details.html
@@ +178,5 @@
>          amount: { currency: "USD", value: "3.5" },
>        },
> +      data: {
> +        supportedNetworks: ["visa"],
> +      },

> * Modified existing test `test_modified_total` adding only one card in `supportedNetworks`. That's necessary to pass the test otherwise the modifier is not applied.

I don't understand why this is necessary. If there is no `data.supportedNetworks` on a modifier then I thought it applied to all networks and having test coverage for that is good.

Comment 14

[Marcos Caceres [:marcosc]]

Comment on attachment 9027876 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9027876 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. I'm still inching my way to fixing the modifier part of the spec (agree, that part is a mess).

Comment 15

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Addressed comments in the latest review.

Regarding the modified test, I wasn't aware that if supportedNetworks wasn't defined the modifier should apply anyway, but I see that's in line with:

https://www.w3.org/TR/payment-method-basic-card/#dfn-steps-to-check-if-an-instrument-is-supported

OTOH, I wonder if I should remove the changes in dom/payments/test/ConstructorChromeScript.js too.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 9031674 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9031674 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

(In reply to Diego Pino from comment #15)
> OTOH, I wonder if I should remove the changes in
> dom/payments/test/ConstructorChromeScript.js too.

Yeah, I think that makes sense to revert too.

::: browser/components/payments/res/paymentRequest.js
@@ +240,5 @@
>     * @returns {object?} the applicable modifier for the payment method
>     */
>    getModifierForPaymentMethod(state, methodID) {
> +    let basicCards = this.getBasicCards(state);
> +    let method = basicCards[methodID] || null;

Nit: could you rename `methodID` to `selectedMethodID` and `method` to `selectedMethod` in order to make the `.find` callback easier to read?

@@ +250,5 @@
>        return null;
>      }
>      let modifier = modifiers.find(m => {
>        // take the first matching modifier
> +      if (m.supportedMethods && m.supportedMethods != method.methodName) {

Since we have two relevant words starting with "m" here, could you also rename `m` to `modifier` to help readability.

Comment 17

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Addressed comments in last review.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 9032088 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032088 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fcc7597cd8d
Account for supportedNetworks in payment modifiers. r=MattN

Comment 20

[Eliza Balazs [:ebalazs_]]

Backed out changeset 3fcc7597cd8d (Bug 1429198) for ES lint failure in builds/worker/checkouts/gecko/browser/components/payments/res/paymentRequest.js. CLOSED TREE

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217673782&repo=mozilla-inbound&lineNumber=270

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&selectedJob=217673782&revision=3fcc7597cd8dc5b88ac288d809355b9865f7da41

Backout push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=9a5bde128fbe25ff1faca78dccbb10bac58f91e0

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 9032088 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032088 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/payments/res/paymentRequest.js
@@ +248,5 @@
>      let modifiers = state.request.paymentDetails.modifiers;
>      if (!modifiers || !modifiers.length) {
>        return null;
>      }
> +    let modifier = modifiers.find(modifier => {

I guess we should rename the `modifier` on the left to `appliedModifier`

Comment 22

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Comment 23

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb4ded086b87a5aa296bce54fa6f5f4c2593fa3

Comment 24

[Cristian Brindusan [:cbrindusan]]

Please note that on this push there are also some bc perma-failures on browser/components/payments/test/browser/browser_total.js

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3fcc7597cd8dc5b88ac288d809355b9865f7da41&selectedJob=217687072 
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217687072&repo=mozilla-inbound&lineNumber=3464

Comment 25

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Fixed lint error.

Comment 26

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d348e990993bf638ceaef23e855384b24a395c

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 9032387 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032387 [details] [diff] [review]:
-----------------------------------------------------------------

Note that Phabricator runs the linters for you but you can also run them locally.

You can mark checkin-needed without another review next with the fix below.

::: browser/components/payments/res/paymentRequest.js
@@ +251,5 @@
> +    let basicCards = this.getBasicCards(state);
> +    let selectedMethod = basicCards[selectedMethodID] || null;
> +    if (selectedMethod && selectedMethod.methodName !== "basic-card") {
> +      throw new Error(`${selectedMethod.methodName} (${selectedMethodID})` +
> +                      `is not a supported payment method`);

You're missing a space character between the two lines.

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 9032387 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032387 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, why did you revert to `tempBasicCards: this.temporaryStore.creditCards.getAll(),` in the one place?

Is that to address the test failure of comment 24?

Comment 29

[Diego Pino]

No, I think I didn't reintroduce `tempBasicCards: this.temporaryStore.creditCards.getAll()`. AFAICT, there are still two replacements of `this.temporaryStore.creditCards.getAll()` for `this.fetchTempPaymentCards()` in https://bugzilla.mozilla.org/attachment.cgi?id=9032387.

The only change I intended to introduce in https://bugzilla.mozilla.org/show_bug.cgi?id=1429198#c25 was the lint error pointed in https://bugzilla.mozilla.org/show_bug.cgi?id=1429198#c27.

Comment 30

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 9032440 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032440 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming you addressed the test failure from comment 24

Comment 32

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Fixes error in browser_total.js.

The issue was that for some tests `selectedMethod` can be null (https://searchfox.org/mozilla-central/source/browser/components/payments/test/browser/browser_total.js#18) so the existence of `selectedMethod` has to be checked before accessing its methodName or cc-type.

Comment 33

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78aca7e4b0b15ad945ad5008e89736d248270fe3

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 9032639 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032639 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/payments/res/paymentRequest.js
@@ +265,5 @@
> +        return false;
> +      }
> +      let supportedNetworks = modifier.data && modifier.data.supportedNetworks || [];
> +      return supportedNetworks.length == 0 ||
> +             selectedMethod && supportedNetworks.includes(selectedMethod["cc-type"]);

IIUC, this means that we would apply a method with no supportedNetworks even if no method is selected because we wouldn't return above. To make this simpler and play-it-safe, I think we should return null in the `if` statement above:
```js
if (!selectedMethod || !modifiers || !modifiers.length) {
  return null;
}
```

Comment 35

[Diego Pino]

Attachment: Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Return null if selectedMethod is null.

Comment 36

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9434b8c43820900e7759f8a705ec28ec6040b16a

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 9032693 [details] [diff] [review]
Bug-1429198-Account-for-supportedNetworks-in-payment.patch

Review of attachment 9032693 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming tests still pass locally: ./mach test browser/components/payments/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e83023e7e8
Account for supportedNetworks in payment modifiers. r=MattN

Comment 39

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/08e83023e7e8

Comment 40

[Daniel Bodea [:danibodea]]

Given the "qe-verify+" tag, can someone help with some way to verify this bug? Thanks.

Comment 41

[Hani Yacoub, Desktop QA]

Removing the "qe-verify+" tag since this feature is disabled.

Comment 42

[Hani Yacoub, Desktop QA]

Removing the "qe-verify+" tag since this feature is disabled.