[RFC] Cancel payments refunds correctly #2111

tvdeyen · 2017-07-25T08:25:26Z

The problem

When canceling payments Solidus handles all response as a void response.

That assumption is unfortunate. Lots of payment providers only allow voiding payments if they are not settled yet. Braintree is such a provider. After a transaction has settled you cannot void anymore but have to refund instead.

The solidus_braintree and solidus_paypal_braintree integrations handle this by checking the status of the transaction first and refund if the transaction cannot be voided anymore. Although this seems correct gateway wise, it is not for Solidus.

For a refunded payment we need to have a Spree::Refund object. Handling every response as a void and not as refund - if it should - is wrong and leads to incorrect payment state.

Solution

A new method Spree::PaymentMethod#try_void

Spree::Payment#cancel! has changed to tell the payment method to try a void. If that succeeds we handle the response as a void - like we do currently -, but if that fails we create a refund with the full amount and a "Order canceled" reason. Spree::Refund already talks with the gateway and credits accordingly.

Fixes #2104

kennyadsl

I just left a comment about naming a class but I'm fine with what has been done as well!

kennyadsl · 2017-07-27T12:50:29Z

core/app/models/spree/app_configuration.rb

+    # @return [Class] a class instance that responds to `cancel!(payment)`
+    attr_writer :payment_canceller
+    def payment_canceller
+      @payment_canceller ||= Spree::Payment::Cancellation.new(


I just have some doubt about the naming:

I expected this class to be named Spree::Payment::Canceller because it is the entity that performs the cancellation. Spree::Payment::Cancellation also makes sense but it's more an action, maybe I'd expect the cancellation to happen right when it is initialized, instead this will not perform anything until the cancel! method is called.

As discussed in private with @tvdeyen it is just a personal preference, both make sense, I just wanted to express mine 😸 .

kennyadsl · 2017-08-01T16:54:30Z

core/app/models/spree/payment/cancellation.rb

+
+      # @param reason [String] (DEFAULT_REASON) -
+      #   The reason used to create the Spree::RefundReason
+      def initialize(reason: DEFAULT_REASON)


Maybe this should not be an optional argument since we are passing it on initialization.

I want to be able to initialize without the reason given

jordan-brough · 2017-08-16T16:02:05Z

core/app/models/spree/payment/cancellation.rb

+          if response = payment.payment_method.try_void(payment)
+            payment.send(:handle_void_response, response)
+          else
+            payment.refunds.create!(amount: payment.amount, reason: refund_reason)


Might need to use the credit_allowed method here instead of amount?

Thanks, Jordan. Fixed

mamhoff

I like that there's a test for canceling a partially refunded payment, and that that works well now. I understand this is a pretty sensitive area of Solidus, but I do think this should get in.

tvdeyen · 2017-08-18T12:12:46Z

It is worth mentioning that a payment method that does not implement try_void yet will fall back to the current behavior. So, this is safe for existing stores.

tvdeyen · 2017-08-23T21:27:28Z

@jordan-brough I fixed the refund amount. It now uses credit_allowed

mamhoff · 2017-08-30T15:49:36Z

I am very much for this, but would like a dedicated changelog entry detailing that this is happening and what it means.

jhawthorn · 2017-08-30T15:49:27Z

core/app/models/spree/payment/cancellation.rb

+      #
+      # @param payment [Spree::Payment] - the payment that should be canceled
+      #
+      def cancel!(payment)


This should probably be just cancel (without the !)

jhawthorn · 2017-08-30T15:59:58Z

core/app/models/spree/payment_method/bogus_credit_card.rb

@@ -55,7 +55,7 @@ def void(_response_code, _credit_card, _options = {})
      ActiveMerchant::Billing::Response.new(true, 'Bogus Gateway: Forced success', {}, test: true, authorization: '12345')
    end

-    def cancel(_response_code)
+    def try_void(_payment)


A YARDoc comment would be helpful here, the return values are a little confusing:

ActiveMerchant::Billing::Response with true if the void succeeded

ActiveMerchant::Billing::Response with false if the void failed

Just false if it can't be voided at this time.

tvdeyen · 2017-08-30T16:01:49Z

I am very much for this, but would like a dedicated changelog entry detailing that this is happening and what it means.

It means nothing to existing stores as all existing payment methods do not implement try_void yet. Besides a deprecation notice stores will not see any difference to current behaviour.

But I will add a note about the deprecation

tvdeyen · 2017-08-31T09:02:17Z

@mamhoff @jhawthorn addressed your feedback

This new class handles cancellations of payments called by order cancel. This new class changes the way we cancel payments in that it asks the payment method to try a void. If that succeeds it handles the void as always. But if that fails - because the transaction is in a un-voidable state - it creates a refund about the amount instead.

In order to get a proper payment state - with refunds created when void is not possible - I needed to update the store credit spec in that it now tests that the payment handles the refund, in contrare to the former directly credit approach. Latter is not supported anymore.

tvdeyen force-pushed the try-void branch from 459f246 to b300aa3 Compare July 25, 2017 08:35

kennyadsl approved these changes Jul 27, 2017

View reviewed changes

kennyadsl reviewed Aug 1, 2017

View reviewed changes

jordan-brough reviewed Aug 16, 2017

View reviewed changes

tvdeyen force-pushed the try-void branch from b300aa3 to e3d89e4 Compare August 18, 2017 11:25

mamhoff approved these changes Aug 18, 2017

View reviewed changes

jhawthorn reviewed Aug 30, 2017

View reviewed changes

tvdeyen force-pushed the try-void branch 3 times, most recently from 71b4b84 to aa3ee8d Compare August 31, 2017 09:01

tvdeyen added 3 commits September 6, 2017 17:57

Configurable payment cancellation class

0d11f3f

tvdeyen force-pushed the try-void branch from aa3ee8d to 3ab6c84 Compare September 6, 2017 15:57

tvdeyen merged commit 1bb33ea into solidusio:master Sep 20, 2017

tvdeyen deleted the try-void branch September 20, 2017 16:13

gmacdougall mentioned this pull request Oct 4, 2017

Only credit back allowed amount #1603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Cancel payments refunds correctly #2111

[RFC] Cancel payments refunds correctly #2111

tvdeyen commented Jul 25, 2017

kennyadsl left a comment

kennyadsl Jul 27, 2017 •

edited

Loading

kennyadsl Aug 1, 2017

tvdeyen Aug 18, 2017

jordan-brough Aug 16, 2017

tvdeyen Aug 18, 2017

mamhoff left a comment

tvdeyen commented Aug 18, 2017

tvdeyen commented Aug 23, 2017

mamhoff commented Aug 30, 2017

jhawthorn Aug 30, 2017

tvdeyen Aug 31, 2017

jhawthorn Aug 30, 2017

tvdeyen Aug 31, 2017

tvdeyen commented Aug 30, 2017

tvdeyen commented Aug 31, 2017

[RFC] Cancel payments refunds correctly #2111

[RFC] Cancel payments refunds correctly #2111

Conversation

tvdeyen commented Jul 25, 2017

The problem

Solution

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

tvdeyen commented Aug 18, 2017

tvdeyen commented Aug 23, 2017

mamhoff commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Aug 30, 2017

tvdeyen commented Aug 31, 2017

kennyadsl Jul 27, 2017 •

edited

Loading