Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow incremental request of billing and shipping address #873

Closed
wants to merge 52 commits into from

Conversation

danyao
Copy link
Collaborator

@danyao danyao commented Jun 28, 2019

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 w3c/payment-method-basic-card#72 (comment).

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 existing requestBillingAddress and requestShipping booleans of PaymentOptions will live on to provide backward compatibility:
    • The new requestBillingAddressParts and requestShippingAddressParts will only take effect when the respective boolean flag is true.
    • This provides backward compatibility:
      • old code + new browser: booleans are used, lists default to [] => old behavior
      • new code + old browser: booleans are used, lists are ignored => old behavior
      • new code + new browser: booleans and lists are both considered => new behavior
  • requestBillingAddressParts and requestShippingAddressParts affect both intermediate results (e.g. those returned in PaymentRequestUpdateEvent and PaymentMethodChangeEvent) and final results in PaymentResponse
    • For billing address, the responsibility is on the payment handler. The specifics is left to the payment method specific specs (e.g. Basic Card and Payment Handler API).
    • For shipping address, a new [[requestedShippingAddressParts]] internal slot is added to PaymentRequest to keep track of the cumulative address parts requested.
      • “Create a PaymentAddress from user-provided input” algorithm is updated to take an additional requestedAddressParts argument that callers will set using the new internal slot.
    • A payee that desires full address at the end must explicitly call requestBillingAddress([]) and/or requestShippingAddress([]).
  • Implication on event firing:
    • shippingaddresschange event (type: PaymentRequestUpdateEvent) must be fired at least once if requestShippingAddressParts is non-empty to give payee an opportunity to request full shipping address.
    • paymentmethodchange event (type: PaymentMethodChangeEvent) must be fired at least once if requestBillingAddressParts is non-empty to give payee an opportunity to request full billing address.

The following tasks have been completed:

  • Confirmed there are no ReSpec errors/warnings.
  • Modified Web platform tests (link)
  • Modified MDN Docs (link)
  • Has undergone security/privacy review (link)

Implementation commitment:

  • Safari (link to issue)
  • Chrome (link to issue)
  • Firefox (link to issue)
  • Edge (public signal)

Preview | Diff

@ianbjacobs
Copy link
Collaborator

Thank you, @danyao!
cc @samuelweiler

To summarize the proposal:

  • A new requestShippingAddress() method enables the merchant to request partial shipping address information.
  • 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.
  • For full shipping address information, the merchant can still use the requestShipping boolean. This also helps with backwards compatibility.

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:

  • By default, merchants do not receive billing address information; they must explicitly request it (via requestBillingAddress).
  • The Basic Card specification defines a redact list that intends to provide enough information for payment authorization.
  • This proposal does not require changes to Basic Card.

[1] w3c/payment-handler#337

index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@rsolomakhin rsolomakhin left a comment

Choose a reason for hiding this comment

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

Technically this should work.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

aestes commented Jul 2, 2019

@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
Copy link
Member

Ah, I see. Thanks for clarifying @aestes.

@samuelweiler
Copy link
Member

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 danyao force-pushed the danyao-requestShippingAddress branch from 4f5fc3e to 6884656 Compare September 13, 2019 14:45
@danyao danyao force-pushed the danyao-requestShippingAddress branch from 6884656 to 28f206c Compare September 13, 2019 15:38
@danyao
Copy link
Collaborator Author

danyao commented Sep 13, 2019

@samuelweiler I incorporated your comments:

  • Added a note in Section 10 to discourage the use of the boolean and encourage the use of the new requestShippingAddress() method. Also noting potential future deprecation of the boolean.
  • Added similar note to Section 18.4.2
  • Added the detailed algorithm to Section 18.4.2 to highlight now the incremental request interact with the concept of redactList.

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 (w3c/payment-handler#337).

@adrianhopebailie
Copy link
Collaborator

@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.

index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@danyao
Copy link
Collaborator Author

danyao commented Oct 28, 2019

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.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Thanks for continuing to work on this... found a couple of issues. The main think is around the state machine of the event that needs a few clarifications. Having multiple ongoing asynchronous operations (.updateWidth() + .requestBillingAddress()) is going to be quite challenging to implement... but I guess as long as the order is clear (i.e., await requestBillingAddress() first, then do updateWidth(), then it might be ok 😅).

I need to chew on this a bit more... the example makes me wonder if it could be simplified.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@rsolomakhin
Copy link
Collaborator

@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
Copy link
Member

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
Copy link
Collaborator

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!
});

danyao and others added 2 commits November 4, 2019 21:49
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
danyao and others added 9 commits November 12, 2019 19:27
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
@danyao danyao changed the title WIP - very rough sketch of requestShippingAddress() method Allow incremental request of billing and shipping address Nov 13, 2019
@danyao
Copy link
Collaborator Author

danyao commented Nov 13, 2019

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
Copy link
Member

Sure! will take a look soon!

@marcoscaceres
Copy link
Member

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:

  • 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?

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

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
@danyao
Copy link
Collaborator Author

danyao commented Nov 13, 2019

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.

danyao and others added 4 commits November 13, 2019 01:31
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
Co-Authored-By: Marcos Cáceres <[email protected]>
@marcoscaceres
Copy link
Member

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!:

  • It never over-shares if we add new address parts,
  • it forces the developer to have good think about what they need,
  • and it gives us a nice way to deprecate the boolean over the next few years, if we want.

@marcoscaceres
Copy link
Member

No more payment address with #955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Richer negotiation re: address redaction?
7 participants