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.

Sign up for GitHub

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

fix #5829 (Voiding an initial payment (i.e. a full refund) after partially refunding the order is not possible with Stripe-SCA) #6453

Merged
merged 2 commits into from
Dec 14, 2020
Merged

fix #5829 (Voiding an initial payment (i.e. a full refund) after partially refunding the order is not possible with Stripe-SCA) #6453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9646232
fix #5829
andrewpbrett Dec 3, 2020
e866e98
add spec for partially refunded SCA orders
andrewpbrett Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/models/spree/gateway/stripe_sca.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def void(response_code, _creditcard, gateway_options)
payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id,
stripe_account: stripe_account_id)
gateway_options[:stripe_account] = stripe_account_id
provider.refund(payment_intent_response.amount_received, response_code, gateway_options)
provider.refund(refundable_amount(payment_intent_response), response_code, gateway_options)
end

# NOTE: the name of this method is determined by Spree::Payment::Processing
Expand All @@ -79,6 +79,11 @@ def create_profile(payment)

private

def refundable_amount(payment_intent_response)
payment_intent_response.amount_received -
payment_intent_response.charges.data.map(&:amount_refunded).sum
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are unsuccessful charges? Looking at the docs it seems that they might not even include them all?

This list only contains the latest charge, even if there were previously multiple unsuccessful charges. To view all previous charges for a PaymentIntent, you can filter the charges list using the payment_intent parameter.

source: https://stripe.com/docs/api/payment_intents/object#payment_intent_object-charges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this still works, because each time we try to make a payment for an order it creates a new PaymentIntent. I tried a couple things:

At first, I thought you meant "what if there are multiple partial refunds" so I tried that: I placed an order, then in the backend I made two adjustments to the order and issued a refund each time. The amount_refunded returns the sum across all refunds.

Then I realized you might be asking "what if the customer or an admin creates unsuccessful charges?" For the admin side, I placed an order through the front end and chose Cash. Then on the backend, I created two unsuccessful charges and one successful one. I was still able to do the partial refunds on the successful charge. I found the same when the unsuccessful charges were created by the customer on the front end.

The option to do the refund on the failed charge never appears in the first place, only on the successful one:
Screen Shot 2020-12-10 at 2 37 57 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

But does that mean that the payment_intent_response.charges includes only successful charges then? Sorry, I'm still wrapping my head around this Stripe API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm saying is that for our case, the PaymentIntent that we're querying Stripe about will only have the single successful charge. It's definitely complex! Especially since our mapping is not as direct as it could be if we were only dealing with Stripe as a provider.

end

# In this gateway, what we call 'secret_key' is the 'login'
def options
options = super
Expand Down
25 changes: 23 additions & 2 deletions spec/controllers/spree/admin/orders/payments/payments_controller_refunds_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,11 @@
before do
allow(Stripe).to receive(:api_key) { "sk_test_12345" }
allow(StripeAccount).to receive(:find_by) { stripe_account }

stub_payment_intent_get_request
end

context "where the request succeeds" do
before do
stub_payment_intent_get_request
# Issues the refund
stub_request(:post, "https://api.stripe.com/v1/charges/ch_1234/refunds").
with(basic_auth: ["sk_test_12345", ""]).
Expand All @@ -181,6 +180,7 @@

context "where the request fails" do
before do
stub_payment_intent_get_request
stub_request(:post, "https://api.stripe.com/v1/charges/ch_1234/refunds").
with(basic_auth: ["sk_test_12345", ""]).
to_return(status: 200, body: JSON.generate(error: { message: "Bup-bow!" }) )
Expand All @@ -198,6 +198,27 @@
expect(flash[:error]).to eq "Bup-bow!"
end
end

context "when a partial refund has already been issued" do
before do
stub_payment_intent_get_request(response: { amount_refunded: 200 })
stub_request(:post, "https://api.stripe.com/v1/charges/ch_1234/refunds").
with(basic_auth: ["sk_test_12345", ""]).
to_return(status: 200,
body: JSON.generate(id: 're_123', object: 'refund', status: 'succeeded') )
end

it "can still void the payment" do
order.reload
expect(order.payment_total).to_not eq 0
expect(order.outstanding_balance).to eq 0
spree_put :fire, params
expect(payment.reload.state).to eq 'void'
order.reload
expect(order.payment_total).to eq 0
expect(order.outstanding_balance).to_not eq 0
end
end
end
end

Expand Down
3 changes: 2 additions & 1 deletion spec/support/request/stripe_stubs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,15 @@ def stub_refund_request
private

def payment_intent_authorize_response_mock(options)
chargedata = [{ id: "ch_1234", amount: 2000, amount_refunded: options[:amount_refunded] || 0 }]
{ status: options[:code] || 200,
body: JSON.generate(id: "pi_123",
object: "payment_intent",
amount: 2000,
amount_received: 2000,
status: options[:intent_status] || "requires_capture",
last_payment_error: nil,
charges: { data: [{ id: "ch_1234", amount: 2000 }] }) }
charges: { data: chargedata }) }
end

def payment_intent_redirect_response_mock(redirect_url)
Expand Down