From 3ff20c527f6dcc38a6cc3d00ea6ab5231a2c7d0a Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 29 Dec 2022 14:06:34 +0100 Subject: [PATCH 1/5] Fix indentation --- core/app/models/spree/payment/processing.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index 483cb96a1fe..95e76cf9e14 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -207,9 +207,9 @@ def record_response(response) end def protect_from_connection_error - yield + yield rescue ActiveMerchant::ConnectionError => error - gateway_error(error) + gateway_error(error) end def gateway_error(error) From 8859360e0ddbce73d060bdce73114206bc84ad95 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 29 Dec 2022 14:09:30 +0100 Subject: [PATCH 2/5] Avoid using a block for checking payment preconditions This turns a deeply nested set of `if`s into a list of guard clauses. --- core/app/models/spree/payment/processing.rb | 38 ++++++++++----------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index 95e76cf9e14..99670035839 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -35,12 +35,16 @@ def process! end def authorize! - handle_payment_preconditions { process_authorization } + return unless check_payment_preconditions! + + process_authorization end # Captures the entire amount of a payment. def purchase! - handle_payment_preconditions { process_purchase } + return unless check_payment_preconditions! + + process_purchase end # Takes the amount in cents to capture. @@ -151,26 +155,20 @@ def process_purchase capture_events.create!(amount: amount) end - def handle_payment_preconditions(&_block) - unless block_given? - raise ArgumentError.new("handle_payment_preconditions must be called with a block") - end - + # @raises Spree::Core::GatewayError + def check_payment_preconditions! return if payment_method.nil? - return if !payment_method.source_required? - - if source - if !processing? - if payment_method.supports?(source) - yield - else - invalidate! - raise Core::GatewayError.new(I18n.t('spree.payment_method_not_supported')) - end - end - else - raise Core::GatewayError.new(I18n.t('spree.payment_processing_failed')) + return unless payment_method.source_required? + unless source + gateway_error(I18n.t('spree.payment_processing_failed')) end + return if processing? + unless payment_method.supports?(source) + invalidate! + gateway_error(I18n.t('spree.payment_method_not_supported')) + end + + true end def gateway_action(source, action, success_state) From b1d3858da85717973b343dc96227e53f941dd819 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 29 Dec 2022 14:10:48 +0100 Subject: [PATCH 3/5] Avoid using `#send` during payment processing The internal helpers were adding more noise and indirection than clarity. Now the calls to `#purchase` and `#authorize` are clearly --- core/app/models/spree/payment/processing.rb | 45 ++++++++++----------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index 99670035839..d8487da8270 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -37,14 +37,34 @@ def process! def authorize! return unless check_payment_preconditions! - process_authorization + started_processing! + + protect_from_connection_error do + response = payment_method.authorize( + money.money.cents, + source, + gateway_options, + ) + handle_response(response, :pend, :failure) + end end # Captures the entire amount of a payment. def purchase! return unless check_payment_preconditions! - process_purchase + started_processing! + + protect_from_connection_error do + response = payment_method.purchase( + money.money.cents, + source, + gateway_options, + ) + handle_response(response, :complete, :failure) + end + + capture_events.create!(amount: amount) end # Takes the amount in cents to capture. @@ -143,18 +163,6 @@ def handle_void_response(response) private - def process_authorization - started_processing! - gateway_action(source, :authorize, :pend) - end - - def process_purchase - started_processing! - gateway_action(source, :purchase, :complete) - # This won't be called if gateway_action raises a GatewayError - capture_events.create!(amount: amount) - end - # @raises Spree::Core::GatewayError def check_payment_preconditions! return if payment_method.nil? @@ -171,15 +179,6 @@ def check_payment_preconditions! true end - def gateway_action(source, action, success_state) - protect_from_connection_error do - response = payment_method.send(action, money.money.cents, - source, - gateway_options) - handle_response(response, success_state, :failure) - end - end - def handle_response(response, success_state, failure_state) record_response(response) From 2b22cf0987ace5614caa9e96f1e778c9f0579c6e Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 29 Dec 2022 15:22:47 +0100 Subject: [PATCH 4/5] Reorder precondition checks in payment processing This way we group simple checks and checks that would raise exceptions in different groups for better readability. --- core/app/models/spree/payment/processing.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index d8487da8270..41ddad5c47d 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -165,12 +165,14 @@ def handle_void_response(response) # @raises Spree::Core::GatewayError def check_payment_preconditions! - return if payment_method.nil? + return if processing? + return unless payment_method return unless payment_method.source_required? + unless source gateway_error(I18n.t('spree.payment_processing_failed')) end - return if processing? + unless payment_method.supports?(source) invalidate! gateway_error(I18n.t('spree.payment_method_not_supported')) From a5ec43b237474c6e6c9aa13aff3e5ff32a03b8bd Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Mon, 2 Jan 2023 11:42:07 +0100 Subject: [PATCH 5/5] Streamline response handling in payment processing Remove some indirection and unnecessary meta-programming. Have it return either true or false leaving the "success" state call as a responsibility of the caller (the "failure_state" was always the same). Also remove one nesting level in the implementation. --- core/app/models/spree/payment/processing.rb | 38 ++++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/core/app/models/spree/payment/processing.rb b/core/app/models/spree/payment/processing.rb index 41ddad5c47d..5f60e6c54a9 100644 --- a/core/app/models/spree/payment/processing.rb +++ b/core/app/models/spree/payment/processing.rb @@ -45,7 +45,7 @@ def authorize! source, gateway_options, ) - handle_response(response, :pend, :failure) + pend! if handle_response(response) end end @@ -61,7 +61,7 @@ def purchase! source, gateway_options, ) - handle_response(response, :complete, :failure) + complete! if handle_response(response) end capture_events.create!(amount: amount) @@ -86,7 +86,7 @@ def capture!(capture_amount = nil) money = ::Money.new(capture_amount, currency) capture_events.create!(amount: money.to_d) update!(amount: captured_amount) - handle_response(response, :complete, :failure) + complete! if handle_response(response) end end @@ -181,24 +181,28 @@ def check_payment_preconditions! true end - def handle_response(response, success_state, failure_state) + # @returns true if the response is successful + # @returns false (and calls #failure) if the response is not successful + def handle_response(response) record_response(response) - if response.success? - unless response.authorization.nil? - self.response_code = response.authorization - self.avs_response = response.avs_result['code'] - - if response.cvv_result - self.cvv_response_code = response.cvv_result['code'] - self.cvv_response_message = response.cvv_result['message'] - end - end - send("#{success_state}!") - else - send(failure_state) + unless response.success? + failure gateway_error(response) + return false + end + + unless response.authorization.nil? + self.response_code = response.authorization + self.avs_response = response.avs_result['code'] + + if response.cvv_result + self.cvv_response_code = response.cvv_result['code'] + self.cvv_response_message = response.cvv_result['message'] + end end + + true end def record_response(response)