From 837272969467a2af62a295b06f72606d281c987a Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Tue, 26 May 2020 22:33:51 +0200 Subject: [PATCH] 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_creation: false).perform! The two extra callbacks are needed to: - set_perform_after_creation_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. - clear_perform_after_creation: this callback is needed to clean the instance, after this game ends, otherwise each call of perform! after the callbacks execution could not be executed if the instance was created with perform_after_creations: 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 --- core/app/models/spree/payment/cancellation.rb | 2 +- core/app/models/spree/refund.rb | 38 +++++ .../reimbursement_helpers.rb | 3 +- .../factories/refund_factory.rb | 1 + .../models/spree/payment/cancellation_spec.rb | 3 +- core/spec/models/spree/refund_spec.rb | 136 ++++++++++++++---- 6 files changed, 150 insertions(+), 33 deletions(-) diff --git a/core/app/models/spree/payment/cancellation.rb b/core/app/models/spree/payment/cancellation.rb index 2fd7aa429b1..1228ed43d89 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_creation: 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..3c1e030602f 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_creation + after_create :set_perform_after_creation_default + after_create :perform! + after_create :clear_perform_after_creation + 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 + + 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 9440120ba3a..a02e61d9113 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_creation: false }) simulate ? refund.readonly! : refund.perform! diff --git a/core/lib/spree/testing_support/factories/refund_factory.rb b/core/lib/spree/testing_support/factories/refund_factory.rb index d65ebf58970..ff1f1ad3ebe 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_creation { 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..d34d44f7b85 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_creation: false ).perform! end diff --git a/core/spec/models/spree/refund_spec.rb b/core/spec/models/spree/refund_spec.rb index 6edfa0179df..6ed66bbe71f 100644 --- a/core/spec/models/spree/refund_spec.rb +++ b/core/spec/models/spree/refund_spec.rb @@ -25,7 +25,20 @@ 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_creation) { false } + + let(:refund) do + create(:refund, + payment: payment, + amount: amount, + reason: refund_reason, + transaction_id: transaction_id, + perform_after_creation: perform_after_creation + ) + end before do allow(payment.payment_method) @@ -35,7 +48,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 +62,81 @@ it "does not attempt to process a transaction" do expect(subject.transaction_id).to be_nil end + + context "passing perform_after_creation" do + context "when it is false" do + let(:perform_after_creation) { false } + + it "does not print deprecation warnings, does not run perform! and reset the perform_after_creation 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_creation).to be_nil + end + end + + context "when it is true" do + let(:perform_after_creation) { true } + + it "prints a deprecation warning, runs perform! and reset the perform_after_creation 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_creation: true" do + let(:perform_after_creation) { 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_creation: false" do + let(:perform_after_creation) { 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 +149,23 @@ let(:transaction_id) { nil } context "processing is successful" do - let(:gateway_response_options) { { authorization: authorization } } - - it 'should create a refund' do + it 'creates 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) - 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 +183,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_creation) { 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 +218,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