-
Notifications
You must be signed in to change notification settings - Fork 58
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 confirmation of SetupIntents with a payment method ID #226
Support confirmation of SetupIntents with a payment method ID #226
Conversation
210f898
to
08a5437
Compare
Previously localstripe only supported confirmation of SetupIntents with payment_method_data, which is a dictionary containing the information needed to create a PaymentMethod. The Stripe SetupIntents API also allows the caller to pass in an already-created PaymentMethod, by ID. This change adds that capability to localstripe. I also added a few of the canonical test articles you can use as a payment method ID. These generally map 1:1 with the test card numbers.
08a5437
to
528e2b2
Compare
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.
Hi @bpcreech, thanks for this useful addition !
I've run our internal test suite against this change and everything looks good 👍 We'll merge and release the change once the PR gets a second approval from one of my colleagues.
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.
Nice, thanks @bpcreech!
I wonder why you removed:
if obj.payment_method is not None:
raise UserError(400, 'Bad request')
And also why you added:
self.payment_method_types = [pm.type]
I imagine that you have a good reason, and for us, it works as before, so I'm OK with it. But next time, don't hesitate to highlight every changes that are not obvious, either inside the commit message or by posting on the corresponding lines inside the opened PR, like that you will avoid questions from us.
PS: nice work 😎
I added the ability to add a payment method by ID on confirm here: adrienverge#226 I removed validation that there was payment_method_data. Per review comment on that PR, common sense, and testing with the actual Stripe API, we should still return a 400 error if neither payment_method nor payment_method_data are supplied to the confirm request. So I added that and changed an old test case that seemed to be asserting a behavior unlike what Stripe actually does. This also addresses another review comment, I was resetting payment_method_types within the confirm handler. Upon testing, this is not what the real Stripe API does, so I removed that recent addition.
@H--o-l good points! I added those in initial testing but now I see they're not quite right. We did need to change the input validation (you must specify a payment_method XOR payment_method_data when you call confirm) but there's still a case where we should throw 400. I created #229 to do that... and to remove the spurious override of payment_method_types which you spotted. |
Actually @H--o-l I tested this with the real Stripe API, and the change happens to have been correct. The old behavior: If you pass a payment_method or payment_method_data during SetupIntent creation, and then try and pass in another payment_method_data during confirmation, you get an error 400. The new behavior: If you pass a payment_method or payment_method_data during SetupIntent creation, and then try and pass in another payment_method_data during confirmation, the new payment_method or payment_method_data overrides the old. The new behavior is correct. To try this with the real Stripe API: HOST=https://api.stripe.com
SK=sk_test_...
curl -sSfg -u $SK: $HOST/v1/setup_intents -X POST -d payment_method=pm_card_visa
seti=.... # copy and paste the setup intent ID from the above
curl -sSfg -u $SK: "$HOST/v1/setup_intents/$seti/confirm" -d return_url=https://bogus -d payment_method=pm_card_chargeCustomerFail
# note it works! Stripe uses the new payment method. There is no documentation of this behavior on Stripe's site https://docs.stripe.com/api/setup_intents/confirm. However, conveniently, just removing the special logic adheres closer to the undocumented behavior, so I think we can keep it this way. |
I added the ability to add a payment method by ID on confirm here: adrienverge#226 Per testing with the actual Stripe API, and common sense, we should still return a 400 error if neither payment_method nor payment_method_data are supplied to the confirm request (the point of the confirm is to add a payment method, so it makes no sense to call confirm with no payment method). So I added validation and changed an old test case that seemed to be asserting a behavior unlike what Stripe actually does. This also addresses a one-liner review comment from this PR: adrienverge#226 I was resetting payment_method_types within the confirm handler. Upon testing, this is not what the real Stripe API does, so I removed that recent addition.
I added the ability to add a payment method by ID on confirm here: adrienverge#226 Per testing with the actual Stripe API, and common sense, we should return a 400 error if neither payment_method nor payment_method_data are supplied to the confirm request (the point of the confirm is to add a payment method, so it makes no sense to call confirm with no payment method). So I added validation and changed an old test case that seemed to be asserting a behavior unlike what Stripe actually does. This also addresses a one-liner review comment from this PR: adrienverge#226 I was resetting payment_method_types within the confirm handler. Upon testing, this is not what the real Stripe API does, so I removed that recent addition.
@bpcreech thanks for the double-check and the explanation, works for me 👍 |
I added the ability to add a payment method by ID on confirm here: #226 Per testing with the actual Stripe API, and common sense, we should return a 400 error if neither payment_method nor payment_method_data are supplied to the confirm request (the point of the confirm is to add a payment method, so it makes no sense to call confirm with no payment method). So I added validation and changed an old test case that seemed to be asserting a behavior unlike what Stripe actually does. This also addresses a one-liner review comment from this PR: #226 I was resetting payment_method_types within the confirm handler. Upon testing, this is not what the real Stripe API does, so I removed that recent addition.
Previously localstripe only supported confirmation of SetupIntents with payment_method_data, which is a dictionary containing the information needed to create a PaymentMethod. The Stripe SetupIntents API also allows the caller to pass in an already-created PaymentMethod, by ID. This change adds that capability to localstripe.
I also added a few of the canonical test articles you can use as a payment method ID. These generally map 1:1 with the test card numbers.