Skip to content
This repository has been archived by the owner on Aug 27, 2021. It is now read-only.

User changes payment method #53

Merged
merged 9 commits into from
Sep 20, 2018
Merged

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Apr 30, 2018

  • defines "user changes payment method" hook
  • Adds BasicCardChangeDetails dictionary
  • Web platform tests

Preview | Diff

@ianbjacobs
Copy link
Contributor

Hi @marcoscaceres,

What is the connection between "type" and "supportedTypes" (and same for "network" and "supportedNetworks")? Is type necessarily an element of supportedTypes (and same for "network")?

Ian

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from integration perspective with the other PR.

@marcoscaceres
Copy link
Member Author

@ianbjacobs

What is the connection between "type" and "supportedTypes" (and same for "network" and "supportedNetworks")? Is type necessarily an element of supportedTypes (and same for "network")?

Ah, good question. I'll make sure to do the supported check. However, I need #52 to land. I can take the r+ from @rsolomakhin.

@marcoscaceres marcoscaceres force-pushed the change-payment-method branch 3 times, most recently from a383244 to 1813ed5 Compare May 9, 2018 05:18
@marcoscaceres marcoscaceres requested a review from ianbjacobs May 9, 2018 05:38
@marcoscaceres
Copy link
Member Author

@ianbjacobs, I tried to address your question about types and networks by clarifying that there is a presupposition that the payment sheet is only presenting "supported cards" to the end user.... a "supported card" is one that meets the PaymentRequest's supportedTypes and supportedNetworks requirement.

It's a little hand wavy, because I don't want to get into the whole "but what if the user wants to add a new card"... but the general idea is there:

  1. user selects a card (or user adds one, so long as it's a "supported card").
  2. The Selected card becomes a BasicCardChangeDetails dictionary.
  3. Run Payment Request's "payment method changed algorithm", which fires the "paymentmethodchanged" event.

@marcoscaceres
Copy link
Member Author

@domenic, apologies, I made normative changes to this part of the API (see #53 (comment)).

Would you mind reviewing this again? I'll put up some tests tomorrow to complement this and w3c/payment-request#695.

@marcoscaceres marcoscaceres requested a review from domenic May 9, 2018 08:24
index.html Outdated
</li>
<li>If the <var>card</var> is part of a <a>network</a>, set
<var>methodDetails</var>["<a>network</a>"] to the network identifier
that represents <var>card</var>'s <a>network</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After dealing with the privacy issues related to shippingaddresschange, I'm no longer sure that this event is a good idea because of the same impact on the UA selecting a payment method by default. If the merchant wants to do anything meaningful with this data (e.g. to change the total and replace the need for modifiers) then the UA would have to dispatch this event as soon as the PaymentRequest opens and that would leak the user's default-selected type and network to the merchant. It sounds like Apple didn't send the network with their applepaypaymentmethodchanged event either so I'm not sure the network is needed. The two PRs also seem to forget about @rsolomakhin's request for the event to be optional for basic-card.

Because of the increased fingerprinting potential (type + network) from a user-interaction showing a PR without the user completing it, I no longer like the idea of the event, especially if the network is leaked (since it provides more entropy). Modifiers don't have these privacy issue and while they don't address the store card case, that's not something that we're focused on at Mozilla at the moment and it doesn't apply to regular debit/credit basic cards.

/cc @stpeter

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with @mnoorenberghe. As @aestes noted when opening w3c/payment-request#662, the applepaypaymentmethodchanged event "allows merchants to add additional display items based on the type of card selected by the user." I'm not sure what these display items are, but presumably they are not new items in the cart but actually payment modifiers (e.g., take $5 off if you pay with debit or 10% if you use your store card). If so, a PaymentDetailsModifier sequence can be provided in the initial payment request and the user agent can apply the modifiers locally if the user selects a card of a different type. Does the user agent really need to fire an event off to the merchant here (e.g., so that the merchant can dynamically determine what modifiers will be applied)? That feels a bit far-fetched and not worth the privacy leak.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnoorenberghe, wrote:

If the merchant wants to do anything meaningful with this data (e.g. to change the total and replace the need for modifiers) then the UA would have to dispatch this event as soon as the PaymentRequest opens and that would leak the user's default-selected type and network to the merchant.

We discussed this at the F2F also. The solution is to apply the modifiers on show(), then no event is fired on the default card - the modified gets applied, so no leakage occurs immediately.

When the user then changes card, by actually clicking on it, then we would fire the event.

I'll clarify that the event should only fire on explicit user action.

It sounds like Apple didn't send the network with their applepaypaymentmethodchanged event either so I'm not sure the network is needed.

I was reaching the same conclusion... why I started null'ing it out :) I'll remove it.

Because of the increased fingerprinting potential (type + network) from a user-interaction showing a PR without the user completing it, I no longer like the idea of the event, especially if the network is leaked (since it provides more entropy).

With the removal of network, and not firing on default card, I think the above is sufficient to mitigate the privacy risks, no?

(note also that you can .canMakePayment() this information already, but we can deal with that separately.)

@stpeter wrote:

I'm not sure what these display items are, but presumably they are not new items in the cart but actually payment modifiers

The idea is that if you know the card type, you don't need the modifiers on subsequent changes. So, instead of having a modifier, the developer just updates the display items directly.

@marcoscaceres
Copy link
Member Author

@mnoorenberghe wrote:

Modifiers don't have these privacy issue and while they don't address the store card case, that's not something that we're focused on at Mozilla at the moment and it doesn't apply to regular debit/credit basic cards.

Mozilla aside, I think this does raise a larger issue around "store" cards. I deliberately did not add the details member needed to support store cards yet, as we don't have a model for how to make those work.

I'm ok for us to not move on this until we understand how store cards would actually work - but think it's valid to add the event itself on the Payment Request API, as it's something Apple is relying on being there (that can't be done with modifiers).

@stpeter
Copy link
Collaborator

stpeter commented May 10, 2018

@marcoscaceres wrote:

I'm ok for us to not move on this until we understand how store cards would actually work - but think it's valid to add the event itself on the Payment Request API, as it's something Apple is relying on being there (that can't be done with modifiers).

IMHO we're still not clear on what "it" is here - perhaps @aestes could clarify for us. :-)

index.html Outdated
card by performing some user action (e.g., by selecting a different
card explicitly from a list of cards). For cards that are preselected
by default by the user agent, any matching <code><a data-cite=
"payment-request#dom-paymentdetailsmodifier">PaymentDetailsModifier</a></code>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "s" after PaymentDetailsModifier?

index.html Outdated
<li>Run the payment request's <a data-cite=
"!payment-request#dfn-payment-method-changed-algorithm">payment
method changed algorithm</a> with <var>methodDetails</var> and
"basic-card".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm as defined in https://github.com/w3c/payment-request/pull/695/files does not take a second argument, but it probably should.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was deriving the "method name" in a hand-wavy way in the Payment Request spec. I agree this explicit way is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated w3c/payment-request#695 to take the second argument.

@marcoscaceres marcoscaceres changed the title add: User changes payment method section User changes payment method Aug 21, 2018
@marcoscaceres marcoscaceres force-pushed the change-payment-method branch from c3f6916 to 955ba2c Compare August 24, 2018 05:48
@marcoscaceres marcoscaceres merged commit 3a4aba7 into gh-pages Sep 20, 2018
@marcoscaceres marcoscaceres deleted the change-payment-method branch September 20, 2018 05:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants