w3c / payment-request

Payment Request API
https://www.w3.org/TR/payment-request-1.1/
Other
482 stars 183 forks source link

Allow incremental request of billing and shipping address #873

Closed danyao closed 3 years ago

danyao commented 5 years ago

closes #842

Add the ability for the merchant to request fine-grained parts of billing address and shipping address. This is based on @adrianhopebailie's idea from https://github.com/w3c/payment-method-basic-card/issues/72#issuecomment-481275334.

We are making these changes because merchants (e.g. Netflix) has requested the ability to receive less information (e.g. Netflix).

At a high level, this pull request can be summarized as the following:

The following tasks have been completed:

Implementation commitment:


Preview | Diff

ianbjacobs commented 5 years ago

Thank you, @danyao! cc @samuelweiler

To summarize the proposal:

We have begun discussions [1] about the delegation of the request for shipping addresses to payment handlers. If the current proposal is adopted, that discussion will need to include a mechanism for the browser to request shipping address information from the payment handler in order to respond to the merchant's incremental request.

As @danyao notes, this proposal does not address billing addresses. Note that:

[1] https://github.com/w3c/payment-handler/issues/337

marcoscaceres commented 5 years ago

I'm not following why we need the incremental increase in granularity? Is that really necessary? It seems like it would make implementing an interface much more difficult, because you'd need to be constantly updating it to add, remove, different address parts... that might be really confusing for users.

marcoscaceres commented 5 years ago

to be clear, I don't follow why we need this:

To incrementally request address data, repeated calls to requestShippingAddress() can be chained with a final promise to compute new details using the address information. A call to updateWith() with the promise chain can be used to update the payment details.

Why not just ask up front?

aestes commented 5 years ago

@marcoscaceres asking for everything up front requires user agents to over-share in some cases. For instance, maybe you offer flat-rate shipping in countryA but variable-per-region shipping in countryB. To cover both cases up front you'd have to ask for country and region, even though if you had only asked for country and gotten back countryA then you didn't need to know the user's region.

So with incremental requests you can start at least-specific and drill down one level at a time (until you reach the redacted levels) until you have just enough to compute a details update and resolve your updateWith() promise.

marcoscaceres commented 5 years ago

Ah, I see. Thanks for clarifying @aestes.

samuelweiler commented 4 years ago

This architecture seems like a good approach for both shipping and billing address.

I would prefer if the "just ask for everything" option were removed - if only as a way to encourage the (admittedly harder to implement) incremental requests. Failing that, I want to see some specific text discouraging the use of the "just ask for everything". Perhaps as a sidebar in section 10? Point out that if potential customer disables it - assuming a UI has such a feature - the merchant will need to fall back to incremental requests anyway or lose the sale. And if they have to implement the incremental anyway, then why keep the "all"?

Whether that boolean is removed or not, there should be some specific text encouraging the use of the incremental approach. That could be in 18.4.2, perhaps as a sidebar. It should also be mentioned in the security considerations, in 20.6.

@aestes Thank you for reiterating the need for this. @joshkaroly, thank you for keeping the pressure on to do this with billing addresses, as well.

danyao commented 4 years ago

@samuelweiler I incorporated your comments:

I didn't add a note to the Security Considerations in Section 20.7 because that section currently focuses on non-normative recommendations to payment handler implementer to not over share user information (especially billing address) via PaymentMethodChangeEvent. I considered expanding it to highlight the user agent's responsibility to not over share shipping address in PaymentRequestUpdateEvent, but I felt it's redundant with the normative requirements encoded in the algorithm in 18.4.2. I also considered adding a non-normative note to compel the payee to use requestShippingAddress(), but that also seems redundant with the other notes in Section 10 and 18.4.2.

Would you mind taking another look? @marcoscaceres @ianbjacobs @aestes PTAL as well.

Also, as I was writing up the algorithm, I realize that the UX implementation may be tricky and would like to hear what others think. Say a payee initially requests "country", then request "region", should the user agent prompt the user for permission each time? This could be annoying. Should the user agent ask for a blanket permission upfront, e.g. "your shipping information will be shared"? But this is a bit misleading because user perceives more information is being shared than what the payee is actually asking for.

Currently this is only a problem for user agents with built-in payment methods (e.g. ApplePay in Safari and basic-card in Chrome). Soon, payment handlers will face this problem as well when they start to support delegated shipping and contact information (https://github.com/w3c/payment-handler/issues/337).

adrianhopebailie commented 4 years ago

@danyao thank you for this work, sorry you couldn't be at TPAC where we discussed the issue at length.

My read of the room was that there is wide support for this but that the group does not want to hold up the process by making this a normative requirement in v1.0 nor remove the existing APIs which are already widely used.

@marcoscaceres @aestes @rsolomakhin @ianbjacobs is there a way this can be marked as an "optional" feature for v1.0 of the spec?

If so, I propose we make this change and I will do a call for consensus from the WG to proceed with v1.0 including this PR and that amendment.

danyao commented 4 years ago

Hi all,

I significantly reworked this pull request to now cover both billing and shipping addresses, and clarified how the new fields affect both addresses returned in intermediate events and the final response. I also added specific language to delegate the responsibility of returning an appropriate version of the billing address to the payment handler. The Payment Handler API and Basic Card spec need to be updated to implement the other side of this stub, but I think the Payment Request API side is reasonably complete and stands on its own.

PTAL.

rsolomakhin commented 4 years ago

@marcoscaceres requested a review from @rsolomakhin 4 hours ago

Anything in particular you want me to take a look at or just the whole thing in general?

marcoscaceres commented 4 years ago

Anything in particular you want me to take a look at or just the whole thing in general?

There was some suggested changes to the state machine of the event (i.e., what happens if you call things out of order, what happens if one promise resolves before the other, etc... what happens if you requestAddress, it rejects, and but updateWith resolves? etc). Can you check if that makes sense and if we need it?

rsolomakhin commented 4 years ago

There was some suggested changes to the state machine of the event (i.e., what happens if you call things out of order, what happens if one promise resolves before the other, etc... what happens if you requestAddress, it rejects, and but updateWith resolves? etc). Can you check if that makes sense and if we need it?

I would prefer that requestShippingAddress() and requestBillingAddress() can be called only inside of the event.updateWith() method. Calling the two in parallel seems to introduce a lot of complexity. Is the parallel calling designed to satisfy a requirement that I'm missing?

// GOOD
paymentRequest.addEventListener('shippingaddressupdate', (event) => {
  event.updateWith(new Promise((updateAddressResolver) => {
    const country = paymentRequest.shippingAddress.country;
    if (country === 'US') {
      const address = await event.requestShippingAddress(['regionCode', 'country']);
      updateAddressResolver(calculatePriceForUsState(address.regionCode));
    } else {
      updateAddressResolver(calculatePriceForCountry(country));
    }
  }));
});

// BAD
paymentRequest.addEventListener('shippingaddressupdate', (event) => {
  event.updateWith(new Promise((updateAddressResolver) => {
    const country = paymentRequest.shippingAddress.country;
    updateAddressResolver(calculatePriceForCountry(country));
  }));
  const address = await event.requestShippingAddress([]);  // Too late!
});
marcoscaceres commented 4 years ago

Blocking on tests and implementation commitments.

danyao commented 4 years ago

Hi @marcoscaceres! Thanks for your patience with this pull request. I think I've addressed all your comments. Would you mind taking another pass?

marcoscaceres commented 4 years ago

Sure! will take a look soon!

marcoscaceres commented 4 years ago

As I was just reviewing this... I... um... had a thought...🤔 what if:

typedef (sequence<AddressParts> or boolean) AddressRequirement;

dictionary PaymentOptions {
    AddressRequirement requestBillingAddress = false;
    AddressRequirement requestShippingAddress = false; 
};

That would:

WDYT?

(AddressRequirement is a terrible name... we can change that.)

danyao commented 4 years ago
typedef (sequence<AddressParts> or boolean) AddressRequirement;

dictionary PaymentOptions {
    AddressRequirement requestBillingAddress = false;
    AddressRequirement requestShippingAddress = false; 
};

That would:

  • retain backwards compatibility with 1.0 (sequence just gets coerced to true in 1.0 implementations).
  • it avoids the { requestShippingAddress: false, requestShippingAddressParts: ["country"] } developer pitfall.
  • avoid adding adding extra members

WDYT?

I thought of two issues, but they seem solvable. First one is (old code + new browser) combination. If a website sets PaymentOptions.requestShippingAddress = true, a new browser needs to know how to translate that into a list. Does this mean we won't be able to leverage IDL type checking?

Second is how to differentiate between "address not requested" and "all address parts requested" if there is only a list. Currently with the boolean flag, we are treating {flag=true, list=[]} as "all". If there's only one list, I can think of three ways:

  1. list=null means "address not requested" and list=[] means "all parts requested". But because both null and [] are falsey values, it seems an easy pitfall.
  2. Introduce an "all" sentinel value to AddressParts to mean "all parts requested". All algorithms that take this list need to first translate "all" to a list of all parts. This feels a bit cumbersome to describe in spec language, but is probably easy enough in actual code.
  3. Make developer explicitly list all parts when they want all. This can be a bit annoying when introducing new address parts because every developer needs to update their code to explicitly request the new part. But maybe this is a privacy feature? Also address formats change slowly so this may not be a big deal in reality.
marcoscaceres commented 4 years ago

I thought of two issues, but they seem solvable. First one is (old code + new browser) combination. If a website sets PaymentOptions.requestShippingAddress = true, a new browser needs to know how to translate that into a list. Does this mean we won't be able to leverage IDL type checking?

According to WebIDL, we should be able to distinguish between them. So if a boolean is passed, and it's true, then we can substitute it with some kind of default list.

If there's only one list, I can think of three ways:

Oooh! I really like where you are going with 3!:

marcoscaceres commented 3 years ago

No more payment address with #955