Avoid duplicated Solidus refunds#281
Conversation
waiting-for-dev
force-pushed
the
waiting-for-dev/avoid_duplicated_refunds
branch
from
2a9a678 to
cd6d5d3
Compare
Codecov Report
@@ Coverage Diff @@
## master #281 +/- ##
==========================================
+ Coverage 99.58% 99.59% +0.01%
==========================================
Files 26 27 +1
Lines 477 489 +12
==========================================
+ Hits 475 487 +12
Misses 2 2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
kennyadsl
left a comment
kennyadsl
left a comment
There was a problem hiding this comment.
👍 Left a comment, but I'm fine with this, thanks @waiting-for-dev
Stripe doesn't guarantee that a webhook event will be delivered only once, so we need to be prepared to handle duplicated incoming refund events. Just creating a Solidus copy whenever we receive a refund event is not enough. On top of that, Stripe won't identify the refund that triggered the event. Both the `charge.refunded` and `payment_intent.succeeded` (for partial captures, which generates a refund for the remaining amount under the hood) webhooks only give us information to retrieve the list of all refunds associated with a charge or payment intent. Because of this, we are now syncing all the refunds associated with a payment intent at every webhook event. We keep track of the refunds already present on Solidus by leveraging the `transaction_id` field on `Spree::Refund`, copying the Stripe refund id as value. On top of that, we need to account for refunds created from Solidus admin panel, which calls the Stripe API. In this case, we need to avoid syncing the refund back to Solidus on the subsequent webhook. We're marking those refunds with a metadata field on Stripe, so we can exclude them from the sync. Closes #262
waiting-for-dev
force-pushed
the
waiting-for-dev/avoid_duplicated_refunds
branch
from
cd6d5d3 to
062e69a
Compare
|
|
||
| # @param payment_intent_id [String] | ||
| def call(payment_intent_id) | ||
| payment = Spree::Payment.find_by!(response_code: payment_intent_id) |
There was a problem hiding this comment.
Thoughts on restricting to the payment method? @payment_method.payments.find_by(…)
There was a problem hiding this comment.
An id collision is probably impossible, and the query will be slightly slower, but that won't hurt.
| Stripe::Refund.create( | ||
| amount: to_stripe_amount(amount_in_cents, currency), | ||
| payment_intent: payment_intent_id, | ||
| metadata: { | ||
| RefundsSynchronizer::SKIP_SYNC_METADATA_KEY => RefundsSynchronizer::SKIP_SYNC_METADATA_VALUE | ||
| } | ||
| ) | ||
| end |
There was a problem hiding this comment.
Thoughts on letting the synchronizer do this and avoid coupling the metadata key/value?
In other words: "if you go through the synchronizer we can guarantee you won't get duplicates"
There was a problem hiding this comment.
Not sure to understand. The synchronizer takes refunds from Stripe and copies them to Solidus if not present, while the gateway is responsible for creating stuff from Solidus to Stripe. 🤔
There was a problem hiding this comment.
Both SolidusStripe::PaymentIntent and SolidusStripe::Customer take charge of a specific part of the Stripe API, including creation. My suggestion is that maybe that should be the case for the RefundSynchronizer as well. Considering a smell this cross reference of metadata.
A less invasive alternative would be to simplify this part doing something like:
metadata: {
**RefundsSynchronizer.stripe_metadata
}There was a problem hiding this comment.
TBH, I think having API calls spread across the codebase is a bigger smell 🙂 Ideally, I'd like them to get closer to the gateway, wrapped within an adapter class, if that would be too much for it.
So I'll go with your latest suggestion, which I think best encapsulates details.
| .map( | ||
| &method(:create_refund).curry[payment] | ||
| ) |
There was a problem hiding this comment.
Thoughts on just using a block here .map { create_refund(payment, _1) } for the sake of readability?
There was a problem hiding this comment.
👍 I understand it's not the most standard Ruby, although when you get used to it readability is even better. What I would love is Ruby to support something like:
.map(&create_refund(payment))| amount: refund_decimal_amount(refund), | ||
| transaction_id: refund.id, | ||
| reason: SolidusStripe::PaymentMethod.refund_reason | ||
| ).tap(&method(:log_refund).curry[payment]) |
There was a problem hiding this comment.
Also here, maybe .tap { log_refund(payment, _1) } would be more readable.
|
@waiting-for-dev sorry if the review took long, maybe some of the comments can be addressed in a follow up if they make sense to you |
3 participants
Summary
Stripe doesn't guarantee that a webhook event will be delivered only once, so we need to be prepared to handle duplicated incoming refund events. Just creating a Solidus copy whenever we receive a refund event is not enough.
On top of that, Stripe won't identify the refund that triggered the event. Both the
charge.refundedandpayment_intent.succeeded(for partial captures, which generates a refund for the remaining amount under the hood) webhooks only give us information to retrieve the list of all refunds associated with a charge or payment intent.Because of this, we are now syncing all the refunds associated with a payment intent at every webhook event. We keep track of the refunds already present on Solidus by leveraging the
transaction_idfield onSpree::Refund, copying the Stripe refund id as value.On top of that, we need to account for refunds created from Solidus admin panel, which calls the Stripe API. In this case, we need to avoid syncing the refund back to Solidus on the subsequent webhook. We're marking those refunds with a metadata field on Stripe, so we can exclude them from the sync.
Closes #262
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: