-
-
Notifications
You must be signed in to change notification settings - Fork 66
Avoid duplicated Solidus refunds #281
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "solidus_stripe/money_to_stripe_amount_converter" | ||
|
|
||
| module SolidusStripe | ||
| # Synchronizes refunds from Stripe to Solidus. | ||
| # | ||
| # For our use case, Stripe has two ways to inform us about refunds initiated | ||
| # on their side: | ||
| # | ||
| # 1. The `charge.refunded` webhook event, which is triggered when a refund is | ||
| # explicitly created. | ||
| # 2. The `payment_intent.succeeded` webhook event, which is triggered when a | ||
| # payment intent is captured. If the payment intent is captured for less than | ||
| # the full amount, a refund is automatically created for the remaining amount. | ||
| # | ||
| # In both cases, Stripe doesn't tell us which refund was recently created, so | ||
| # we need to fetch all the refunds for the payment intent and check if any of | ||
| # them is missing on Solidus. We're using the `transaction_id` field on | ||
| # `Spree::Refund` to match refunds against Stripe refunds ids. We could think | ||
| # about only syncing the single refund not present on Solidus, but we need to | ||
| # acknowledge concurrent partial refunds. | ||
| # | ||
| # The `Spree::RefundReason` with `SolidusStripe::Config.refund_reason_name` | ||
| # as name is used as created refunds' reason. | ||
| # | ||
| # Besides, 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, otherwise we would end up | ||
| # with duplicate records. We're marking those refunds with a metadata field on | ||
| # Stripe, so we can filter them out (see {Gateway#credit}). | ||
| class RefundsSynchronizer | ||
| include MoneyToStripeAmountConverter | ||
|
|
||
| # Metadata key used to mark refunds that shouldn't be synced back to Solidus. | ||
| # @return [Symbol] | ||
| SKIP_SYNC_METADATA_KEY = :solidus_skip_sync | ||
|
|
||
| # Metadata value used to mark refunds that shouldn't be synced back to Solidus. | ||
| # @return [String] | ||
| SKIP_SYNC_METADATA_VALUE = 'true' | ||
|
|
||
| # @param payment_method [SolidusStripe::PaymentMethod] | ||
| def initialize(payment_method) | ||
| @payment_method = payment_method | ||
| end | ||
|
|
||
| # @param payment_intent_id [String] | ||
| def call(payment_intent_id) | ||
| payment = Spree::Payment.find_by!(response_code: payment_intent_id) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts on restricting to the payment method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An id collision is probably impossible, and the query will be slightly slower, but that won't hurt. |
||
|
|
||
| stripe_refunds(payment_intent_id) | ||
| .select(&method(:stripe_refund_needs_sync?)) | ||
| .map( | ||
| &method(:create_refund).curry[payment] | ||
| ) | ||
|
Comment on lines
+54
to
+56
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thoughts on just using a block here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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)) |
||
| end | ||
|
|
||
| private | ||
|
|
||
| def stripe_refunds(payment_intent_id) | ||
| @payment_method.gateway.request do | ||
| Stripe::Refund.list(payment_intent: payment_intent_id).data | ||
| end | ||
| end | ||
|
|
||
| def stripe_refund_needs_sync?(refund) | ||
| originated_outside_solidus = refund.metadata[SKIP_SYNC_METADATA_KEY] != SKIP_SYNC_METADATA_VALUE | ||
| not_already_synced = Spree::Refund.find_by(transaction_id: refund.id).nil? | ||
|
|
||
| originated_outside_solidus && not_already_synced | ||
| end | ||
|
|
||
| def create_refund(payment, refund) | ||
| Spree::Refund.create!( | ||
| payment: payment, | ||
| amount: refund_decimal_amount(refund), | ||
| transaction_id: refund.id, | ||
| reason: SolidusStripe::PaymentMethod.refund_reason | ||
| ).tap(&method(:log_refund).curry[payment]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here, maybe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| end | ||
|
|
||
| def log_refund(payment, refund) | ||
| SolidusStripe::LogEntries.payment_log( | ||
| payment, | ||
| success: true, | ||
| message: "Payment was refunded after Stripe event (#{refund.money})" | ||
| ) | ||
| end | ||
|
|
||
| def refund_decimal_amount(refund) | ||
| to_solidus_amount(refund.amount, refund.currency) | ||
| .then { |amount| solidus_subunit_to_decimal(amount, refund.currency) } | ||
| end | ||
| end | ||
| end | ||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.
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.