-
Notifications
You must be signed in to change notification settings - Fork 135
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
Support requesting billing address #749
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All nits
index.html
Outdated
"PaymentOptions.requestPayerAddress">requestPayerAddress</a> member | ||
was set to true in the <a>PaymentOptions</a> passed to the | ||
<a>PaymentRequest</a> constructor, then <a>payerAddress</a> will be | ||
the payer's address chosen by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph seems like it could be more similar to the shipping address one. E.g., mentioning the payer address changed algorithm.
index.html
Outdated
the payer's address chosen by the user. | ||
</p> | ||
<p class="note"> | ||
On some user agents, the some attributes of the <a>payerAddress</a>'s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the some" -> "some"
"the payerAddress's" -> "payerAddress"
index.html
Outdated
</p> | ||
<p class="note"> | ||
On some user agents, the some attributes of the <a>payerAddress</a>'s | ||
might be redacted until the <a>user accepts the payment request</a> - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hyphen minus -> either em dash (—) or just use a comma
index.html
Outdated
<p class="note"> | ||
On some user agents, the some attributes of the <a>payerAddress</a>'s | ||
might be redacted until the <a>user accepts the payment request</a> - | ||
at which point all the user provided details of <a>payerAddress</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"user provided" -> "user-provided"
index.html
Outdated
</li> | ||
<li>Set the <a data-lt= | ||
"PaymentRequest.payerAddress">payerAddress</a> attribute value of | ||
<var>request</var> to <var>payerAddress</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be worth testing that UAs create distinct objects here and change payerAddress to the new PaymentAddress object. That is, they can't mutate the existing object; per this spec they have to create a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make sure to test that.
Hmm, substantive concerns at #27 (comment); I'd appreciate your (or others') thoughts there before merging this. |
Closing, due to lack of consensus. |
@adrianhopebailie, @ianbjacobs, @rsolomakhin: Ok, so given feedback in #27 and teleconferences we've had. How about I do:
Agree? |
This looks good to me as is. |
@rsolomakhin, thanks for the feedback. The issue that come up previously is that billing address is always bound to an instrument, so it might make more sense to just attach the billing address to the "paymentmethodchange" event. Basically, the model that is currently specified stays the same... the deck chairs just get rearranged a bit. |
Ok, now matches #749 (comment) and I've also updated w3c/payment-method-basic-card#53 |
I do prefer a |
@domenic, the updated PR is basically a rewrite (smaller tho:)). Do you have cycles to re-review it? @rsolomakhin, do you want to give it a good look over and see if the text matches how you would implement this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I have a question and a nitpick suggestion.
index.html
Outdated
@@ -3881,9 +3904,16 @@ <h2> | |||
</li> | |||
<li>Set <var>event</var>.<a>[[\waitForUpdate]]</a> to true. | |||
</li> | |||
<li>Let <var>identifer</var> be undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcoscaceres I would prefer to name this methodIdentifier
to avoid confusion with the Payment Request identifier. paymentMethodIdentifier
and pmi
would also be OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, "identifer" -> "identifier"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsolomakhin, so I totally agree: problem is we have methodName
attributes on both PaymentResponse
and PaymentMethodChangeEvent
, both standing for PMI. Changing this one to be pmi
or paymentMethodIdentifier
would make this one the odd one out 😢
Although methodName
is a terrible name, I'd prefer we stay consistent. Is that ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsolomakhin sorry, I misread this out of context! You meant the local variable in the algorithm. Sure, I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've lost track of the plot on what feature this implements and how, but at least editorially it seems reasonable, with only a few nits.
I guess my biggest substantive question is why we're adding requestBillingAddress to PaymentOptions when it sounds like we're keeping billing address as a payment method-specific thing. (E.g., it is not returned in any property of PaymentResponse.) If that's the case, shouldn't whether or not to request the billing address be part of the payment method data?
index.html
Outdated
@@ -3881,9 +3904,16 @@ <h2> | |||
</li> | |||
<li>Set <var>event</var>.<a>[[\waitForUpdate]]</a> to true. | |||
</li> | |||
<li>Let <var>identifer</var> be undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, "identifer" -> "identifier"
index.html
Outdated
<var>detailsPromise</var> settling. If <var>detailsPromise</var> | ||
<var>detailsPromise</var>, a <a>PaymentRequest</a> | ||
<var>request</var>, and optionally a DOMString <var>identifer</var> | ||
(a <a>payment method identifier</a>). The steps are conditional on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be optional. It should just either be a string or null. Spec-ese doesn't have the nice JavaScript thing where undefined = missing, so the below is actually broken when the third argument is not passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, true that. Will fix.
index.html
Outdated
<var>identifer</var>, then <a data-cite= | ||
"!WEBIDL#dfn-convert-ecmascript-to-idl-value">convert</a> <a> | ||
paymentMethodErrors</a> to an IDL value. Otherwise, | ||
<a data-cite= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already an object, so no need to convert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah. I thought it was still a JS object and it was being converted into an IDL object. We might need to check other parts of the spec, as we do the same thing in other places.
billingAddress should be part of the payment method data. The thing is that in some cases, the Total depends on the billingAddress (e.g., VAT computation). The merchant wants to know about the billingAddress while the sheet is open to display a correct total. I believe the solution of this pull request will work for those payment methods build into the browser (e.g., basic card). However it will not work for some payment methods supported by payment handlers (e.g., for push payments or real-time credit financing) until we do something else. I'm synthesizing what I'm hearing here: Ian |
@domenic wrote:
Yes, you are absolutely right! It should be part of the method data. I'll fix that. @ianbjacobs, wrote:
I really need your help understanding what "something else" could be. I've read over everything provided thus far, and tried to make sure the updated PR addresses the push payment use case (as I understand it). If it doesn't address the use case, then I want to fix that - but I need your help showing me that what I've proposed here is actually unable to address the use case. Ian, would it be best to have a call about it? We've successfully resolved issues like this before when we've chatted (or you've successfully explained to me how wrong I am 😅). We only get one real shot at this, so we gotta make sure we get it right. |
@marcoscaceres : I think @ianbjacobs wants to make changes to Payment Handler, but this PR should work. @ianbjacobs : Did I understand you correctly? Please add me to the call, if you have one at a convenient time. |
After chatting with @marcoscaceres and @rsolomakhin we would like clarification on one point: if a merchant needs a billing address (e.g., for VAT computation), will that merchant need the billing address for EVERY payment method the merchant would accept? The answer to this question affects the design, roughly as follows:
As an example, would a merchant ever need billing address for a basic card payment, and not need a billing address for payment by credit transfer, or cryptocurrency, or real-time financing, etc.? @lyverovski, @Krystosterone, @michelle-stripe, @asolove-stripe, @mattsaxon, @vkuntz, @frank-hoffmann we welcome your input here. |
I think there are probably some cases where you only need billing address for one method. I could imagine wanting it for card so you do AVS on a card, but don't care about it for other payment methods. That said, the main use-case here is still where the billing address affects tax or other calculations controlling the total. For that use-case, it's really the same across all payment methods. |
Thanks @asolove-stripe! As @adrianhopebailie mentioned on a call right now, if billing address is needed for VAT then it would be needed no matter what the payment method might be. I still worry a bit about non-VAT use cases where a user might be asked for a billing address when that's unexpected for their chosen payment method, but maybe we don't have a good handle on those yet. |
Ok, we can go back to having it as a "global" In the future, if we need to, we can go more fine grained by adding |
This reverts commit 5191fe0.
c7d80a8
to
e91e6b4
Compare
@rsolomakhin @aestes, @zouhir just wanted to confirm if you are also planning to support this ( Firefox's plan is always return |
Yes, we plan to support this in Chrome, but the exact behavior needs to be thought through and fine-tuned on our end. The Firefox decision seems reasonable is in the realm of possibilities for us. |
I am currently using Stripe's javascript API with the paymentRequestButton, and I have found the billing address to be available in the returned json object when the customer selects a card. I.e. the billing address registered with the specific card comes back as part of the card details json object. Not sure if the underlying implementation is something that Stripe has done, or if it is part of the paymentRequest API, but it solves the problem in my case. |
Hi @snez, you wrote:
If I may ask, could you help us understand what you are using the billing address for? |
Put up a test for review: web-platform-tests/wpt#14255 |
Just a quick update: Firefox patches for |
File chrome bug, based on @rsolomakhin comment #749 (comment) |
The billing address is a minimum requirement for many large eCommerce systems in order to place an order (in my case Magento). I wouldn't be able to programmatically create an order without it, and thus the whole idea of accepting payments from the product page could not be implemented (see https://magento2.cryozonic.com/radiant-tee.html for an example), which is a perfect use for the paymentRequest API, and something that many merchants are trying to implement to improve their conversions. |
Thanks, @snez, for the example and details. That’s super helpful. |
Part 1 for #27 (part 2 is #750)
The following tasks have been completed:
Implementation commitment:
Impact on Payment Handler spec?
Preview | Diff