From 3d6f51095c2b3868f880ca06c51345bc0a5a7331 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_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 --- .../spree/admin/refunds_controller.rb | 2 +- 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 | 139 ++++++++++++++---- 7 files changed, 153 insertions(+), 35 deletions(-) diff --git a/backend/app/controllers/spree/admin/refunds_controller.rb b/backend/app/controllers/spree/admin/refunds_controller.rb index 93004b7e321..7c183cbd0d2 100644 --- a/backend/app/controllers/spree/admin/refunds_controller.rb +++ b/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| 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..a92898ccdcb 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_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? 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..77a0f46fc5f 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_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