search

solidusio / solidus_stripe

πŸ’³ Integrate Solidus with Stripe
https://stripe.com
BSD 3-Clause "New" or "Revised" License
36 stars 62 forks source link

Allow the use of non-card Stripe payment methods #295

Closed rainerdema closed 1 year ago

rainerdema commented 1 year ago

Summary

The goal of these changes is to see the supported and enabled Stripe payment methods in the checkout payment section and be able to use them (even without a dedicated configuration like dedicated partial views for payment method types).

Please look at the attached issues and all the commit details for more context.

Testing

https://user-images.githubusercontent.com/19948291/234878333-9c177079-a25f-4f57-850f-7f71700ac5f3.mov

In case of manual tests, be sure to:

  1. Activate the payment methods you want to test from the Stripe Dashboard.
  2. Set the Auto Capture mode to Yes in your Solidus Stripe Payment Method: This is needed to allow the display of non-card payment methods that don't support capture_mode set to false: Most of the Stripe payment methods have this behavior.
  3. Have the correct currency based on your Stripe Country account: For example, if my Stripe account is configured in "IT" I will probably need an order with "EUR" currency to display some of the activated payment methods.
  4. Try to use a Stripe Payment Method that is supported in the country (otherwise, it won't be displayed).

Please refer to the Stripe documentation to better understand all the settings needed to display and use a specific Stripe payment method.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

codecov[bot] commented 1 year ago

Codecov Report

Merging #295 (55fcde3) into main (4bcc604) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #295      +/-   ##
==========================================
+ Coverage   99.63%   99.64%   +0.01%     
==========================================
  Files          30       31       +1     
  Lines         546      563      +17     
==========================================
+ Hits          544      561      +17     
  Misses          2        2
Impacted Files Coverage Ξ”
app/models/solidus_stripe/payment_intent.rb 100.00% <100.00%> (ΓΈ)
...nts/source_forms/existing_payment/_stripe.html.erb 100.00% <100.00%> (ΓΈ)
.../views/checkouts/existing_payment/_stripe.html.erb 100.00% <100.00%> (ΓΈ)
...lates/app/views/checkouts/payment/_stripe.html.erb 100.00% <100.00%> (ΓΈ)
...tes/app/views/orders/payment_info/_stripe.html.erb 100.00% <100.00%> (ΓΈ)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

rainerdema commented 1 year ago

@waiting-for-dev I will remove the commit as:

Anyway, here are some explanations of why I made the mentioned changes πŸ‘€

It's unrelated and unnecessary to the intention of this PR.

The main problem about why I started this change was while implementing the sepa_debit payment method, I had some problems figuring out what was wrong with my webhook configuration and the reason for receiving 400 errors from the webhook controller (I was waiting for a "payment_intent.succeeded" from Stripe but I simply had a wrong signature configured).

We're changing to use exceptions to control the flow of execution. That's less performant, and the indirection makes it more difficult to reason about.

Yep, I can agree about the performance. But not raising anything in case of an exception still makes it difficult to understand what is wrong in case of problems. Maybe some kind of debug mode could be the "solution"?

We're moving business logic (how to load a payment method from a slug) to the controller layer. We're diluting the separation of concerns. slug is a request thing, so it was ok to be provided to the .from_request method.

I don't have much to add here, conceptually, I agree πŸ‘

More complex. We're now dealing with the unhappy path in two places.

What do you mean with more complex?

We already discussed that in https://github.com/solidusio/solidus_stripe/pull/213#discussion_r1131408586, so if we decide to change it I'd like it to have proper visibility so that we can discuss about it properly πŸ™

I completely missed this πŸ™

Note

Since the SolidusStripe::IntentsController.load_payment_method inspired me to load the resource like this, I think we should decide sooner or later: as they are implemented inconsistently with each other currently.

Update

Since there was a force push to remove the commit, you can find the changes mentioned here.

waiting-for-dev commented 1 year ago

The main problem about why I started this change was while implementing the sepa_debit payment method, I had some problems figuring out what was wrong with my webhook configuration and the reason for receiving 400 errors from the webhook controller (I was waiting for a "payment_intent.succeeded" from Stripe but I simply had a wrong signature configured).

We're changing to use exceptions to control the flow of execution. That's less performant, and the indirection makes it more difficult to reason about.

Yep, I can agree about the performance. But not raising anything in case of an exception still makes it difficult to understand what is wrong in case of problems. Maybe some kind of debug mode could be the "solution"?

Yeah, I agree. The reason I think it's not a good idea to get back to Stripe with 5xx error codes is that I'd like to differentiate "expected" failure responses to Stripe (the ones we're handling), vs. unexpected ones (something going wrong in the subscribers). We should have some logging for the expected failures.

What do you mean with more complex?

With the other solution, both the controller and the .from_request method from the Webhook class are controlling the unhappy path by raising an exception.

I completely missed this pray

No worries!

Note

Since the SolidusStripe::IntentsController.load_payment_method inspired me to load the resource like this, I think we should decide sooner or later: as they are implemented inconsistently with each other currently.

Yeah, I already shared my concerns in this comment. I'd like to see that encapsulated.

I also want to make clear that raising exceptions vs. where we load the payment method are two unrelated things. We can still load the payment method from the Webhook class, and raise instead of returning nil.

I'm happy to follow the conversation in a dedicated PR!! :slightly_smiling_face: