Keep calling perform! after refund creation with a deprecation message

We can't just stop calling this callback or we are going to break
applications when it's defined code like:

  Spree::Refund.create(attrs: ...)

since it currently relies on also calling perform! as well.

With this code, the old behavior is still there but we are asking
users to update their code to transition to the new behavior, which
is something like:

  Spree::Refund.create(attrs: ..., perform_after_create: false).perform!

The two extra callbacks are needed to:

- set_perform_after_create_default: prints the deprecation message only
  when creating the instance, otherwise it will be printed also when
  calling perform! on a good instance. Also, it sets the deafult to
  true when the attribute is not passed, which means that code has not
  been updated yet.
- clear_perform_after_create: this callback is needed to clean the
  instance, after this process ends, otherwise each call of perform! after
  the callbacks execution could not be executed if the instance was
  created with perform_after_create: false.

Specs needed some seriuos refactor to reflect this new architecture but
they look cleaner now.

Co-Authoring the commit since the initial code has been taken from #3181.

Co-authored-by: Angel Perez <aitbw@users.noreply.github.com>

backend/app/controllers/spree/admin/refunds_controller.rb

@@ -11,7 +11,7 @@ class RefundsController < ResourceController
       rescue_from Spree::Core::GatewayError, with: :spree_core_gateway_error
 
       def create
-        @refund.attributes = refund_params.merge(perform_after_creation: false)
+        @refund.attributes = refund_params.merge(perform_after_create: false)
         if @refund.save && @refund.perform!
           flash[:success] = flash_message_for(@refund, :successfully_created)
           respond_with(@refund) do |format|

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

@@ -31,7 +31,7 @@ def cancel(payment)
           if response = payment.payment_method.try_void(payment)
             payment.send(:handle_void_response, response)
           else
-            payment.refunds.create!(amount: payment.credit_allowed, reason: refund_reason).perform!
+            payment.refunds.create!(amount: payment.credit_allowed, reason: refund_reason, perform_after_create: false).perform!
           end
         else
           # For payment methods not yet implemeting `try_void`

core/app/models/spree/refund.rb

@@ -14,6 +14,11 @@ class Refund < Spree::Base
 
     validate :amount_is_less_than_or_equal_to_allowed_amount, on: :create
 
+    attr_accessor :perform_after_create
+    after_create :set_perform_after_create_default
+    after_create :perform!
+    after_create :clear_perform_after_create
+
     scope :non_reimbursement, -> { where(reimbursement_id: nil) }
 
     delegate :currency, to: :payment
@@ -38,6 +43,7 @@ def description
     # Attempts to perform the refund,
     # raises an error if the refund fails.
     def perform!
+      return true if perform_after_create == false
       return true if transaction_id.present?
 
       credit_cents = money.cents
@@ -51,6 +57,38 @@ def perform!
 
     private
 
+    # This callback takes care of setting the behavior that determines if it is needed
+    # to execute the perform! callback after_create.
+    # Existing code that creates refund without explicitely passing
+    #
+    # perform_after_create: false
+    #
+    # as attribute will still call perform! but a deprecation warning is emitted in order
+    # to ask users to change their code with the new supported behavior.
+    def set_perform_after_create_default
+      return true if perform_after_create == false
+
+      Spree::Deprecation.warn <<-WARN.strip_heredoc, caller
+        From Solidus v3.0 onwards, #perform! will need to be explicitly called when creating new
+        refunds. Please, change your code from:
+
+          Spree::Refund.create(your: attributes)
+
+        to:
+
+          Spree::Refund.create(your: attributes, perform_after_create: false).perform!
+      WARN
+
+      self.perform_after_create = true
+    end
+
+    # This is needed to avoid that when you create a refund with perform_after_create = false,
+    # it's not possibile to call perform! on that instance, since the value of this attribute
+    # will remain false until a reload of the instance.
+    def clear_perform_after_create
+      @perform_after_create = nil
+    end
+
     # return an activemerchant response object if successful or else raise an error
     def process!(credit_cents)
       response = if payment.payment_method.payment_profiles_supported?

core/app/models/spree/reimbursement_type/reimbursement_helpers.rb

@@ -34,7 +34,8 @@ def create_refund(reimbursement, payment, amount, simulate)
       refund = reimbursement.refunds.build({
         payment: payment,
         amount: amount,
-        reason: Spree::RefundReason.return_processing_reason
+        reason: Spree::RefundReason.return_processing_reason,
+        perform_after_create: false
       })
 
       if simulate

core/lib/spree/testing_support/factories/refund_factory.rb

@@ -13,6 +13,7 @@
 
     amount { 100.00 }
     transaction_id { generate(:refund_transaction_id) }
+    perform_after_create { false }
     payment do
       association(:payment, state: 'completed', amount: payment_amount)
     end

core/spec/models/spree/payment/cancellation_spec.rb

@@ -50,7 +50,8 @@
           before do
             payment.refunds.create!(
               amount: credit_amount,
-              reason: Spree::RefundReason.where(name: 'test').first_or_create
+              reason: Spree::RefundReason.where(name: 'test').first_or_create,
+              perform_after_create: false
             ).perform!
           end
 

core/spec/models/spree/refund_spec.rb

@@ -25,7 +25,21 @@
   let(:gateway_response_success) { true }
   let(:gateway_response_message) { "" }
   let(:gateway_response_params) { {} }
-  let(:gateway_response_options) { {} }
+  let(:gateway_response_options) { { authorization: authorization } }
+
+  let(:transaction_id) { nil }
+  let(:perform_after_create) { false }
+
+  let(:refund) do
+    create(
+      :refund,
+      payment: payment,
+      amount: amount,
+      reason: refund_reason,
+      transaction_id: transaction_id,
+      perform_after_create: perform_after_create
+    )
+  end
 
   before do
     allow(payment.payment_method)
@@ -35,7 +49,7 @@
   end
 
   describe 'create' do
-    subject { create(:refund, payment: payment, amount: amount, reason: refund_reason, transaction_id: nil) }
+    subject { refund }
 
     it "creates a refund record" do
       expect{ subject }.to change { Spree::Refund.count }.by(1)
@@ -49,25 +63,81 @@
     it "does not attempt to process a transaction" do
       expect(subject.transaction_id).to be_nil
     end
+
+    context "passing perform_after_create" do
+      context "when it is false" do
+        let(:perform_after_create) { false }
+
+        it "does not print deprecation warnings, does not run perform! and reset the perform_after_create value" do
+          expect(Spree::Deprecation).not_to receive(:warn)
+
+          expect { subject }.not_to change(Spree::LogEntry, :count)
+          expect(subject.transaction_id).to be_nil
+
+          expect(refund.perform_after_create).to be_nil
+        end
+      end
+
+      context "when it is true" do
+        let(:perform_after_create) { true }
+
+        it "prints a deprecation warning, runs perform! and reset the perform_after_create value" do
+          expect(Spree::Deprecation).to receive(:warn)
+
+          expect { subject }.to change(Spree::LogEntry, :count)
+          expect(subject.transaction_id).not_to be_nil
+
+          expect(refund.perform_after_create).to be_nil
+        end
+      end
+    end
   end
 
   describe "#perform!" do
-    subject do
-      refund = Spree::Refund.create!(
-        payment: payment,
-        amount: amount,
-        reason: refund_reason,
-        transaction_id: transaction_id
-      )
-      refund.perform!
-      refund
+    subject { refund.perform! }
+
+    context "with perform_after_create: true" do
+      let(:perform_after_create) { true }
+
+      it "does nothing, perform! already happened after create" do
+        expect(Spree::Deprecation).to receive(:warn)
+        refund
+        expect(refund.transaction_id).not_to be_nil
+
+        expect { subject }.not_to change(Spree::LogEntry, :count)
+      end
+    end
+
+    context "with perform_after_create: false" do
+      let(:perform_after_create) { false }
+
+      it "runs perform! without deprecation warnings" do
+        expect(Spree::Deprecation).not_to receive(:warn)
+        refund
+        expect(refund.transaction_id).to be_nil
+
+        expect { subject }.to change(Spree::LogEntry, :count)
+      end
+    end
+
+    context "without perform_after_create" do
+      let(:perform_after_create) { nil }
+
+      it "does nothing, perform! already happened after create" do
+        expect(Spree::Deprecation).to receive(:warn)
+        refund
+        expect(refund.transaction_id).not_to be_nil
+
+        expect { subject }.not_to change(Spree::LogEntry, :count)
+      end
     end
 
     context "when transaction_id exists" do
       let(:transaction_id) { "12kfjas0" }
 
       it "maintains the transaction id" do
-        expect(subject.reload.transaction_id).to eq transaction_id
+        subject
+        expect(refund.reload.transaction_id).to eq transaction_id
       end
 
       it "does not attempt to process a transaction" do
@@ -80,26 +150,23 @@
       let(:transaction_id) { nil }
 
       context "processing is successful" do
-        let(:gateway_response_options) { { authorization: authorization } }
-
-        it 'should create a refund' do
-          expect{ subject }.to change{ Spree::Refund.count }.by(1)
-        end
-
-        it 'returns the newly created refund' do
-          expect(subject).to be_a(Spree::Refund)
+        it 'creates a refund' do
+          expect{ subject }.to change(Spree::Refund, :count).by(1)
         end
 
-        it 'should save the returned authorization value' do
-          expect(subject.reload.transaction_id).to eq authorization
+        it 'saves the returned authorization value' do
+          subject
+          expect(refund.reload.transaction_id).to eq authorization
         end
 
-        it 'should save the passed amount as the refund amount' do
-          expect(subject.amount).to eq amount
+        it 'saves the passed amount as the refund amount' do
+          subject
+          expect(refund.reload.amount).to eq amount
         end
 
-        it 'should create a log entry' do
-          expect(subject.log_entries).to be_present
+        it 'creates a log entry' do
+          subject
+          expect(refund.reload.log_entries).to be_present
         end
 
         it "attempts to process a transaction" do
@@ -117,15 +184,26 @@
         let(:gateway_response_success) { false }
         let(:gateway_response_message) { "failure message" }
 
-        it 'should raise error and not create a refund' do
-          expect do
+        context 'without performing after create' do
+          it 'raises a GatewayError' do
             expect { subject }.to raise_error(Spree::Core::GatewayError, gateway_response_message)
-          end.to change{ Spree::Refund.count }
+          end
+        end
+
+        context 'calling perform! with after_create' do
+          let(:perform_after_create) { true }
+
+          it 'raises a GatewayError and does not create a refund' do
+            expect(Spree::Deprecation).to receive(:warn)
+
+            expect do
+              expect { subject }.to raise_error(Spree::Core::GatewayError, gateway_response_message)
+            end.not_to change(Spree::Refund, :count)
+          end
         end
       end
 
       context 'without payment profiles supported' do
-        let(:gateway_response_options) { { authorization: authorization } }
         before do
           allow(payment.payment_method).to receive(:payment_profiles_supported?) { false }
         end
@@ -141,7 +219,6 @@
       end
 
       context 'with payment profiles supported' do
-        let(:gateway_response_options) { { authorization: authorization } }
         before do
           allow(payment.payment_method).to receive(:payment_profiles_supported?) { true }
         end