Merge pull request #3641 from nebulab/kennyadsl/restore-1415

Stop calling perform! as Spree::Refund after_create callback

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

@@ -10,6 +10,21 @@ class RefundsController < ResourceController
 
       rescue_from Spree::Core::GatewayError, with: :spree_core_gateway_error
 
+      def create
+        @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|
+            format.html { redirect_to location_after_save }
+          end
+        else
+          flash.now[:error] = @refund.errors.full_messages.join(", ")
+          respond_with(@refund) do |format|
+            format.html { render action: 'new' }
+          end
+        end
+      end
+
       private
 
       def location_after_save
@@ -25,6 +40,10 @@ def refund_reasons
         @refund_reasons ||= Spree::RefundReason.active.all
       end
 
+      def refund_params
+        params.require(:refund).permit!
+      end
+
       def build_resource
         super.tap do |refund|
           refund.amount = refund.payment.credit_allowed

backend/spec/controllers/spree/admin/refunds_controller_spec.rb

@@ -6,22 +6,48 @@
   stub_authorization!
 
   describe "POST create" do
-    context "a Spree::Core::GatewayError is raised" do
-      let(:payment) { create(:payment) }
+    let(:refund_reason) { create(:refund_reason) }
+    let(:refund_amount) { 100.0 }
+
+    let(:payment) { create(:payment, amount: payment_amount) }
+    let(:payment_amount) { refund_amount * 2 }
 
-      subject do
-        post :create,
-          params: {
-          refund: { amount: "50.0", refund_reason_id: "1" },
+    subject do
+      post(
+        :create,
+        params: {
+          refund: {
+            amount: refund_amount,
+            refund_reason_id: refund_reason.id,
+            transaction_id: nil
+          },
           order_id: payment.order_id,
           payment_id: payment.id
         }
+      )
+    end
+
+    context "and no Spree::Core::GatewayError is raised" do
+      it "creates a refund record" do
+        expect{ subject }.to change(Spree::Refund, :count).by(1)
+      end
+
+      it "calls #perform!" do
+        subject
+        # transaction_id comes from Spree::Gateway::Bogus.credit
+        expect(Spree::Refund.last.transaction_id).to eq("12345")
+      end
+    end
+
+    context "a Spree::Core::GatewayError is raised" do
+      before do
+        expect_any_instance_of(Spree::Refund).
+          to receive(:perform!).
+          and_raise(Spree::Core::GatewayError.new('An error has occurred'))
       end
 
-      before(:each) do
-        def controller.create
-          raise Spree::Core::GatewayError.new('An error has occurred')
-        end
+      it "does not create a refund record" do
+        expect{ subject }.to_not change { Spree::Refund.count }
       end
 
       it "sets an error message with the correct text" do

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)
+            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

@@ -10,13 +10,14 @@ class Refund < Spree::Base
 
     validates :payment, presence: true
     validates :reason, presence: true
-    validates :transaction_id, presence: true, on: :update # can't require this on create because the before_create needs to run first
     validates :amount, presence: true, numericality: { greater_than: 0 }
 
     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 :create_log_entry
+    after_create :clear_perform_after_create
 
     scope :non_reimbursement, -> { where(reimbursement_id: nil) }
 
@@ -37,22 +38,57 @@ def description
       payment.payment_method.name
     end
 
-    private
-
-    # attempts to perform the refund.
+    # Must be called for the refund transaction to be processed.
+    #
+    # 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
 
-      @response = process!(credit_cents)
+      response = process!(credit_cents)
+      log_entries.build(details: response.to_yaml)
 
-      self.transaction_id = @response.authorization
-      update_columns(transaction_id: transaction_id)
+      update!(transaction_id: response.authorization)
       update_order
     end
 
+    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?
@@ -73,10 +109,6 @@ def process!(credit_cents)
       raise Core::GatewayError.new(I18n.t('spree.unable_to_connect_to_gateway'))
     end
 
-    def create_log_entry
-      log_entries.create!(details: @response.to_yaml)
-    end
-
     def amount_is_less_than_or_equal_to_allowed_amount
       if payment && amount > payment.credit_allowed
         errors.add(:amount, :greater_than_allowed)

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

@@ -34,10 +34,17 @@ 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
       })
 
-      simulate ? refund.readonly! : refund.save!
+      if simulate
+        refund.readonly!
+      else
+        refund.save!
+        refund.perform!
+      end
+
       refund
     end
 

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/order_spec.rb

@@ -127,7 +127,7 @@
       let(:payment) { create(:payment, order: order, amount: payment_amount, state: 'completed') }
 
       before do
-        create(:refund, payment: payment, amount: payment_amount)
+        create(:refund, payment: payment, amount: payment_amount).perform!
       end
 
       it "cancels the order" do
@@ -281,7 +281,7 @@
       order.cancellations.short_ship([order.inventory_units.first])
       expect(order.outstanding_balance).to be_negative
       expect(order.payment_state).to eq('credit_owed')
-      create(:refund, amount: order.outstanding_balance.abs, payment: payment, transaction_id: nil)
+      create(:refund, amount: order.outstanding_balance.abs, payment: payment, transaction_id: nil).perform!
       order.reload
       expect(order.outstanding_balance).to eq(0)
       expect(order.payment_state).to eq('paid')

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

@@ -50,8 +50,9 @@
           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
 
           it 'only refunds the allowed credit amount' do