diff --git a/core/app/models/spree/payment/cancellation.rb b/core/app/models/spree/payment/cancellation.rb index 2fd7aa429b1..8d32b132663 100644 --- a/core/app/models/spree/payment/cancellation.rb +++ b/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` diff --git a/core/app/models/spree/refund.rb b/core/app/models/spree/refund.rb index 096b54b0938..442bb999ba9 100644 --- a/core/app/models/spree/refund.rb +++ b/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_creation == 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_creation: 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_creation_default + return true if perform_after_creation == 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_creation: false).perform! + WARN + + self.perform_after_creation = true + end + + # This is needed to avoid that when you create a refund with perform_after_creation = 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_creation + @perform_after_creation = 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? diff --git a/core/app/models/spree/reimbursement_type/reimbursement_helpers.rb b/core/app/models/spree/reimbursement_type/reimbursement_helpers.rb index 88aa59e6591..abfc08efdc8 100644 --- a/core/app/models/spree/reimbursement_type/reimbursement_helpers.rb +++ b/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 diff --git a/core/lib/spree/testing_support/factories/refund_factory.rb b/core/lib/spree/testing_support/factories/refund_factory.rb index d65ebf58970..f33534f5b2c 100644 --- a/core/lib/spree/testing_support/factories/refund_factory.rb +++ b/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 diff --git a/core/spec/models/spree/payment/cancellation_spec.rb b/core/spec/models/spree/payment/cancellation_spec.rb index 386749aa31a..8dd99e12f06 100644 --- a/core/spec/models/spree/payment/cancellation_spec.rb +++ b/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 diff --git a/core/spec/models/spree/refund_spec.rb b/core/spec/models/spree/refund_spec.rb index 6edfa0179df..77d9b2a3ba1 100644 --- a/core/spec/models/spree/refund_spec.rb +++ b/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_creation).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_creation" do + let(:perform_after_creation) { 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