Add a default implementation for PaymentMethod#try_void

[kennyadsl]

Summary

We use to replicate this behavior on all the payment extensions that add a payment integration. Still, the logic is pretty similar all the times.

By adding this default behavior we can save some time integrating new payment methods, unelss something special is needed.

The default try_void method added will just check if the void attempt is successful, otherwise it will return false. This will determine if Solidus will emit a refund along with an order cancellation.

The method is a bit complex because void can have different arity, based on if the payment method support payment profiles or not. See payment processing for more information.

This PR comes from this conversation on PayPal Commerce Platform.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

• I have written a thorough PR description.
• I have kept my commits small and atomic.
• I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

• I have added automated tests to cover my changes.
• I have attached screenshots to demo visual changes.
• I have opened a PR to update the guides.
• I have updated the README to account for my changes.

[elia]

For consistency with other placed deciding which arity to use I would make it dependent on the result of payment_profiles_supported? as in:

solidus/core/app/models/spree/payment/processing.rb

Lines 74 to 81 in 16d1e37

	if payment_method.payment_profiles_supported?
	# Gateways supporting payment profiles will need access to credit card object because this stores the payment profile information
	# so supply the authorization itself as well as the credit card, rather than just the authorization code
	response = payment_method.void(response_code, source, gateway_options)
	else
	# Standard ActiveMerchant void usage
	response = payment_method.void(response_code, gateway_options)
	end

Not sure there's a way to abstract it or maybe stop having two signatures, but for the time being I would suggest we stick to the same check, so it will be easier to refactor in the future.

[kennyadsl]

Done, looks cleaner for sure, and I think we will notice tests failing on the try_void method if that implementation changes.

[elia]

caveat: the class / module keywords will create a nesting scope related to the top-level, while the describe blocks won't create one. This means that TestPaymentMethodWithoutTryVoid, PaymentGateway etc. are defined on the top-level (aka Object).

A possible alternative is to use anonymous classes or a file-specific namespace in which to store these fixtures:

module SpreePaymentMethodSpec
  class TestPaymentMethodWithoutTryVoid < Spree::PaymentMethod
    attr_accessor :gateway_class, :payment_profiles_supported?
  end
  
  class PaymentGatewayWithoutSource
    def initialize(_options)
    end

    def void(_response_code, gateway_options = {})
      payment = gateway_options[:originator]

      if payment.completed?
        ActiveMerchant::Billing::Response.new(false, "Can't void a completed payment", {}, test: true)
      else
        ActiveMerchant::Billing::Response.new(true, "Payment correctly voided", {}, test: true)
      end
    end
  end
  
  class PaymentGatewayWithSource
    def initialize(_options)
    end

    def void(_response_code, _source, gateway_options = {})
      payment = gateway_options[:originator]

      if payment.completed?
        ActiveMerchant::Billing::Response.new(false, "Can't void a completed payment", {}, test: true)
      else
        ActiveMerchant::Billing::Response.new(true, "Payment correctly voided", {}, test: true)
      end
    end
  end
end

RSpec.describe Spree::PaymentMethod, type: :model do
  # ...
  
  describe "#try_void" do
    let(:payment) { create(:payment, payment_method: payment_method) }
    
    context "when the payment supports profiles" do
      let(:payment_method) { SpreePaymentMethodSpec::TestPaymentMethodWithoutTryVoid.new(
        gateway_class: SpreePaymentMethodSpec::PaymentGatewayWithSource, 
        payment_profiles_supported?: true
      ) }

      # ...

[kennyadsl]

@elia done a first pass with anonymous classes. I added a preliminary commit to refactor the rest of the file, which was using the same approach I used (that's why I went that route). I have the feeling we are losing a bit of readability in specs, at least now it seems harder to imagine what real Payment Method/Gateway classes look like. I'm open to giving it another shot if that's too cryptic.

[elia]

I think it's fine, any solution in this space has trade-offs and at least this isn't messing up with top-level constants 👍

[elia]

👍

[tvdeyen]

nice work

[waiting-for-dev]

👍