From a6abe8ca0899d89d7c044d682e014da491ac57ad Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 5 Nov 2020 10:53:54 +0100 Subject: [PATCH 1/2] Test refunds controller stubbing process! instead of perform! The first line of perform! avoid the method to be executed under certain circumstances (which are actually happening in the spec context). Due to this, we are not testing the current behavior since perform! should be skipped and in the spec it's raising an exception. To fix this, it's better to stub process!, which is where the simulated exception can happen. We now have a failing spec but it's reflecting what happens in the real world. --- backend/spec/controllers/spree/admin/refunds_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/spec/controllers/spree/admin/refunds_controller_spec.rb b/backend/spec/controllers/spree/admin/refunds_controller_spec.rb index 775ccf15ffe..db2a0564bc4 100644 --- a/backend/spec/controllers/spree/admin/refunds_controller_spec.rb +++ b/backend/spec/controllers/spree/admin/refunds_controller_spec.rb @@ -42,7 +42,7 @@ context "a Spree::Core::GatewayError is raised" do before do expect_any_instance_of(Spree::Refund). - to receive(:perform!). + to receive(:process!). and_raise(Spree::Core::GatewayError.new('An error has occurred')) end From ea7cbe03146c5a77d04202a0363b6824fa8b228f Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Thu, 5 Nov 2020 15:23:04 +0100 Subject: [PATCH 2/2] Avoid saving a refund when process! fails If we save unsuccessful refund on payment gateway errors we'll end up having a refund associated to the payment without the actual refund processed. Since at the moment refunds have no states to indicate whether is successful or not, the system is considering it as done and doesn't allow to perform a new one since the credit allowed amount for the payment becomes 0: payment.credit_allowed = payment total - refund amount and this formula is true also when the refund has not been succesful. This comes from our past behavior of creating and calling perform! on refunds at the same time. By just validating the refund we have the same result. The "perform_after_create: false" attribute setting has been moved into the perform! itself. When called on a record still unpersisted, it will set its value to false to avoid deprecation warnings and well.. to call perform! again! A more clean solution would be considering in that formula only refunds that are completed but at the moment there's no way to do that. This way we could also keep track of the failed attempts to perform refunds, saving all the information we get back from the gateway. --- backend/app/controllers/spree/admin/refunds_controller.rb | 5 +++-- core/app/models/spree/refund.rb | 8 +++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/backend/app/controllers/spree/admin/refunds_controller.rb b/backend/app/controllers/spree/admin/refunds_controller.rb index 7c183cbd0d2..10b6de39c40 100644 --- a/backend/app/controllers/spree/admin/refunds_controller.rb +++ b/backend/app/controllers/spree/admin/refunds_controller.rb @@ -11,8 +11,9 @@ 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! + @refund.attributes = refund_params + + if @refund.valid? && @refund.perform! flash[:success] = flash_message_for(@refund, :successfully_created) respond_with(@refund) do |format| format.html { redirect_to location_after_save } diff --git a/core/app/models/spree/refund.rb b/core/app/models/spree/refund.rb index 5ae7be4abfd..42b88ce2a6b 100644 --- a/core/app/models/spree/refund.rb +++ b/core/app/models/spree/refund.rb @@ -61,7 +61,13 @@ def perform! ) log_entries.build(details: perform_response.to_yaml) - update!(transaction_id: perform_response.authorization) + + self.transaction_id = perform_response.authorization + # This is needed otherwise set_perform_after_create_default callback + # will print a deprecation warning when save! creates a record. + self.perform_after_create = false unless persisted? + save! + update_order end