From e5a85caef6def18b96cf103a5e70e7b246afd56c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 9 Jun 2021 22:14:46 +0100 Subject: [PATCH 1/7] Refactor Stripe::PaymentIntentValidator This makes the interface a lot simpler and moves the fetching of the information it requires inside the service itself. --- app/models/spree/gateway/stripe_sca.rb | 3 +-- app/services/process_payment_intent.rb | 12 +----------- lib/stripe/payment_intent_validator.rb | 18 +++++++++++++++--- .../spree/orders_controller_spec.rb | 4 ++-- .../stripe/payment_intent_validator_spec.rb | 9 +++++++-- spec/services/process_payment_intent_spec.rb | 9 ++++----- 6 files changed, 30 insertions(+), 25 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index 8e77d3f258..e23a4ea479 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -129,8 +129,7 @@ module Spree payment = fetch_payment(creditcard, gateway_options) raise Stripe::StripeError, I18n.t(:no_pending_payments) unless payment&.response_code - payment_intent_response = Stripe::PaymentIntentValidator.new. - call(payment.response_code, stripe_account_id) + payment_intent_response = Stripe::PaymentIntentValidator.new.call(payment) raise_if_not_in_capture_state(payment_intent_response) diff --git a/app/services/process_payment_intent.rb b/app/services/process_payment_intent.rb index beb92e4869..e9456cfb0b 100644 --- a/app/services/process_payment_intent.rb +++ b/app/services/process_payment_intent.rb @@ -69,16 +69,6 @@ class ProcessPaymentIntent end def payment_intent_status - @payment_intent_status ||= Stripe::PaymentIntentValidator.new. - call(payment_intent, stripe_account_id). - status - end - - def stripe_account_id - StripeAccount.find_by(enterprise_id: preferred_enterprise_id).stripe_user_id - end - - def preferred_enterprise_id - payment.payment_method.preferred_enterprise_id + @payment_intent_status ||= Stripe::PaymentIntentValidator.new.call(payment).status end end diff --git a/lib/stripe/payment_intent_validator.rb b/lib/stripe/payment_intent_validator.rb index 879a49eac2..2deeb74584 100644 --- a/lib/stripe/payment_intent_validator.rb +++ b/lib/stripe/payment_intent_validator.rb @@ -3,9 +3,11 @@ # This class validates if a given payment intent ID is valid in Stripe module Stripe class PaymentIntentValidator - def call(payment_intent_id, stripe_account_id) - payment_intent_response = Stripe::PaymentIntent.retrieve(payment_intent_id, - stripe_account: stripe_account_id) + def call(payment) + payment_intent_response = Stripe::PaymentIntent.retrieve( + payment_intent_id(payment), + stripe_account: stripe_account_id(payment) + ) raise_if_last_payment_error_present(payment_intent_response) @@ -14,6 +16,16 @@ module Stripe private + def payment_intent_id(payment) + payment.response_code + end + + def stripe_account_id(payment) + enterprise_id = payment.payment_method&.preferred_enterprise_id + + StripeAccount.find_by(enterprise_id: enterprise_id)&.stripe_user_id + end + def raise_if_last_payment_error_present(payment_intent_response) return unless payment_intent_response.respond_to?(:last_payment_error) && payment_intent_response.last_payment_error.present? diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 879d663342..909d7d3609 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -115,7 +115,7 @@ describe Spree::OrdersController, type: :controller do before do allow_any_instance_of(Stripe::PaymentIntentValidator) .to receive(:call) - .with(payment_intent, kind_of(String)) + .with(payment) .and_return(payment_intent_response) allow(Spree::Order).to receive(:find_by!) { order } @@ -161,7 +161,7 @@ describe Spree::OrdersController, type: :controller do before do allow_any_instance_of(Stripe::PaymentIntentValidator) .to receive(:call) - .with(payment_intent, kind_of(String)) + .with(payment) .and_raise(Stripe::StripeError, "error message") end diff --git a/spec/lib/stripe/payment_intent_validator_spec.rb b/spec/lib/stripe/payment_intent_validator_spec.rb index 81a730ab2a..c4ff925b08 100644 --- a/spec/lib/stripe/payment_intent_validator_spec.rb +++ b/spec/lib/stripe/payment_intent_validator_spec.rb @@ -7,13 +7,18 @@ module Stripe describe PaymentIntentValidator do describe "#call" do let(:validator) { Stripe::PaymentIntentValidator.new } + let(:payment) { build(:payment, response_code: payment_intent_id) } let(:payment_intent_id) { "pi_123" } let(:stripe_account_id) { "abc123" } + let(:stripe_account_mock) { double(stripe_user_id: stripe_account_id) } let(:payment_intent_response_mock) { { status: 200, body: payment_intent_response_body } } before do Stripe.api_key = "sk_test_12345" + allow(payment).to receive_message_chain(:payment_method, :preferred_enterprise_id) { 1 } + allow(StripeAccount).to receive(:find_by) { stripe_account_mock } + stub_request(:get, "https://api.stripe.com/v1/payment_intents/#{payment_intent_id}") .with(headers: { 'Stripe-Account' => stripe_account_id }) .to_return(payment_intent_response_mock) @@ -26,7 +31,7 @@ module Stripe it "returns payment intent id and does not raise" do expect { - result = validator.call(payment_intent_id, stripe_account_id) + result = validator.call(payment) expect(result).to eq payment_intent_response_body }.to_not raise_error Stripe::StripeError end @@ -39,7 +44,7 @@ module Stripe it "raises Stripe error with payment intent last_payment_error as message" do expect { - validator.call(payment_intent_id, stripe_account_id) + validator.call(payment) }.to raise_error Stripe::StripeError, "No money" end end diff --git a/spec/services/process_payment_intent_spec.rb b/spec/services/process_payment_intent_spec.rb index aa14085650..01424570af 100644 --- a/spec/services/process_payment_intent_spec.rb +++ b/spec/services/process_payment_intent_spec.rb @@ -52,8 +52,7 @@ describe ProcessPaymentIntent do context "where the stripe payment intent validation responds with errors" do before do allow(validator) - .to receive(:call).with(intent, anything).and_raise(Stripe::StripeError, - "error message") + .to receive(:call).with(payment).and_raise(Stripe::StripeError, "error message") end it "returns returns the error message" do @@ -77,7 +76,7 @@ describe ProcessPaymentIntent do before do allow(order).to receive(:deliver_order_confirmation_email) - allow(validator).to receive(:call).with(intent, anything).and_return(intent_response) + allow(validator).to receive(:call).with(payment).and_return(intent_response) end it "validates the intent" do @@ -143,7 +142,7 @@ describe ProcessPaymentIntent do before do payment.update_attribute(:state, "failed") - allow(validator).to receive(:call).with(intent, anything).and_return(intent) + allow(validator).to receive(:call).with(payment).and_return(intent) end it "does not return any error message" do @@ -166,7 +165,7 @@ describe ProcessPaymentIntent do before do allow(order).to receive(:process_payments!) { nil } - allow(validator).to receive(:call).with(intent, anything).and_return(intent_response) + allow(validator).to receive(:call).with(payment).and_return(intent_response) end it "returns a failed result" do From 65d9325287bc73cb4610300ffbdcf6546faf06ff Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 9 Jun 2021 22:35:02 +0100 Subject: [PATCH 2/7] Add #stripe_status helper method to Payment This method could definitely be used elsewhere, but is currently most useful in debugging; at any time in the console you can bring up a payment object and call #stripe_status on it, and it'll make a live call to Stripe and tell you exactly what state the payment is in on the Stripe side. --- app/models/spree/payment.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 3bb4b60858..1990778eaa 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -153,6 +153,17 @@ module Spree I18n.t('payment_method_fee') end + # Returns the current payment status from a live call to the Stripe API. + # Returns nil if the payment is not a Stripe payment or does not have a payment intent. + # If the payment requires authorization the status will be "requires_action". + # If the payment has been captured the status will be "succeeded". + # https://stripe.com/docs/api/payment_intents/object#payment_intent_object-status + def stripe_status + return if response_code.blank? + + Stripe::PaymentIntentValidator.new.call(self).status + end + def clear_authorization_url update_attribute(:cvv_response_message, nil) end From 986d068e28f64eafde39bd3f688cfae2e99fdf3b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 10 Jun 2021 10:33:41 +0100 Subject: [PATCH 3/7] Add #stripe_captured? method to Payment --- app/models/spree/payment.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 1990778eaa..8ad498080d 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -164,6 +164,10 @@ module Spree Stripe::PaymentIntentValidator.new.call(self).status end + def stripe_captured? + stripe_status == "succeeded" + end + def clear_authorization_url update_attribute(:cvv_response_message, nil) end From dcc808a8b9c8af7af1450d72126105e1cb0955f3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 10 Jun 2021 10:39:38 +0100 Subject: [PATCH 4/7] Improve #stripe_status return value for failed actions --- app/models/spree/payment.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 8ad498080d..3ec6743e02 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -162,6 +162,10 @@ module Spree return if response_code.blank? Stripe::PaymentIntentValidator.new.call(self).status + rescue Stripe::StripeError + # The Stripe::PaymentIntentValidator will raise an error if the Stripe API call indicates + # the last attempted action on the payment intent failed. + "failed" end def stripe_captured? From e1846b1030e7e4b58743eb04ca2d85896150d7c0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 10 Jun 2021 10:45:41 +0100 Subject: [PATCH 5/7] Extract Stripe payment helper methods to a service --- app/models/spree/payment.rb | 19 ------- app/services/stripe_payment_status.rb | 32 ++++++++++++ spec/services/stripe_payment_status_spec.rb | 55 +++++++++++++++++++++ 3 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 app/services/stripe_payment_status.rb create mode 100644 spec/services/stripe_payment_status_spec.rb diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 3ec6743e02..3bb4b60858 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -153,25 +153,6 @@ module Spree I18n.t('payment_method_fee') end - # Returns the current payment status from a live call to the Stripe API. - # Returns nil if the payment is not a Stripe payment or does not have a payment intent. - # If the payment requires authorization the status will be "requires_action". - # If the payment has been captured the status will be "succeeded". - # https://stripe.com/docs/api/payment_intents/object#payment_intent_object-status - def stripe_status - return if response_code.blank? - - Stripe::PaymentIntentValidator.new.call(self).status - rescue Stripe::StripeError - # The Stripe::PaymentIntentValidator will raise an error if the Stripe API call indicates - # the last attempted action on the payment intent failed. - "failed" - end - - def stripe_captured? - stripe_status == "succeeded" - end - def clear_authorization_url update_attribute(:cvv_response_message, nil) end diff --git a/app/services/stripe_payment_status.rb b/app/services/stripe_payment_status.rb new file mode 100644 index 0000000000..49f5c998ac --- /dev/null +++ b/app/services/stripe_payment_status.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class StripePaymentStatus + def initialize(payment) + @payment = payment + end + + # Returns the current payment status from a live call to the Stripe API. + # Returns nil if the payment is not a Stripe payment or does not have a payment intent. + # If the payment requires authorization the status will be "requires_action". + # If the payment has been captured the status will be "succeeded". + # Docs: https://stripe.com/docs/api/payment_intents/object#payment_intent_object-status + def stripe_status + return if payment.response_code.blank? + + Stripe::PaymentIntentValidator.new.call(self).status + rescue Stripe::StripeError + # Stripe::PaymentIntentValidator will raise an error if the response from the Stripe API + # call indicates the last attempted action on the payment intent failed. + "failed" + end + + # If the payment is a Stripe payment and has been captured in the associated Stripe account, + # returns true, otherwise false. + def stripe_captured? + stripe_status == "succeeded" + end + + private + + attr_reader :payment +end diff --git a/spec/services/stripe_payment_status_spec.rb b/spec/services/stripe_payment_status_spec.rb new file mode 100644 index 0000000000..912c87e213 --- /dev/null +++ b/spec/services/stripe_payment_status_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe StripePaymentStatus do + subject { StripePaymentStatus.new(payment) } + let(:payment) { build(:payment) } + + describe '#stripe_status' do + context "when the payment is not a Stripe payment or does not have a payment intent" do + it "returns nil" do + expect(subject.stripe_status).to be_nil + end + end + + context "when the payment has a payment intent" do + before { allow(payment).to receive(:response_code) { "pi_1234" } } + + it "fetches the status with Stripe::PaymentIntentValidator" do + expect(Stripe::PaymentIntentValidator). + to receive_message_chain(:new, :call, :status) { true } + + subject.stripe_status + end + + context "and the last action on the Stripe payment failed" do + it "returns failed response" do + allow(Stripe::PaymentIntentValidator). + to receive_message_chain(:new, :call, :status).and_raise(Stripe::StripeError) + + expect(subject.stripe_status).to eq "failed" + end + end + end + end + + describe '#stripe_captured?' do + context "when the payment is not a Stripe payment or does not have a payment intent" do + it "returns false" do + expect(subject.stripe_captured?).to eq false + end + end + + context "when the Stripe payment has been captured" do + before { allow(payment).to receive(:response_code) { "pi_1234" } } + + it "returns true" do + allow(Stripe::PaymentIntentValidator). + to receive_message_chain(:new, :call, :status) { "succeeded" } + + expect(subject.stripe_captured?).to eq true + end + end + end +end From 32a2610e7d1daa6abc1a38250f877788b3acca95 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 17 Jun 2021 11:35:35 +0100 Subject: [PATCH 6/7] Refactor arguments for PaymentIntentValidator --- app/models/spree/gateway/stripe_sca.rb | 2 +- app/services/process_payment_intent.rb | 2 +- app/services/stripe_payment_status.rb | 2 +- lib/stripe/payment_intent_validator.rb | 16 +++++++++++----- spec/controllers/spree/orders_controller_spec.rb | 15 ++++++--------- spec/lib/stripe/payment_intent_validator_spec.rb | 6 +++--- spec/services/process_payment_intent_spec.rb | 10 +++++----- 7 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/models/spree/gateway/stripe_sca.rb b/app/models/spree/gateway/stripe_sca.rb index e23a4ea479..28f9e6db7b 100644 --- a/app/models/spree/gateway/stripe_sca.rb +++ b/app/models/spree/gateway/stripe_sca.rb @@ -129,7 +129,7 @@ module Spree payment = fetch_payment(creditcard, gateway_options) raise Stripe::StripeError, I18n.t(:no_pending_payments) unless payment&.response_code - payment_intent_response = Stripe::PaymentIntentValidator.new.call(payment) + payment_intent_response = Stripe::PaymentIntentValidator.new(payment).call raise_if_not_in_capture_state(payment_intent_response) diff --git a/app/services/process_payment_intent.rb b/app/services/process_payment_intent.rb index e9456cfb0b..b18319d83d 100644 --- a/app/services/process_payment_intent.rb +++ b/app/services/process_payment_intent.rb @@ -69,6 +69,6 @@ class ProcessPaymentIntent end def payment_intent_status - @payment_intent_status ||= Stripe::PaymentIntentValidator.new.call(payment).status + @payment_intent_status ||= Stripe::PaymentIntentValidator.new(payment).call.status end end diff --git a/app/services/stripe_payment_status.rb b/app/services/stripe_payment_status.rb index 49f5c998ac..a5ed2759d8 100644 --- a/app/services/stripe_payment_status.rb +++ b/app/services/stripe_payment_status.rb @@ -13,7 +13,7 @@ class StripePaymentStatus def stripe_status return if payment.response_code.blank? - Stripe::PaymentIntentValidator.new.call(self).status + Stripe::PaymentIntentValidator.new(self).call.status rescue Stripe::StripeError # Stripe::PaymentIntentValidator will raise an error if the response from the Stripe API # call indicates the last attempted action on the payment intent failed. diff --git a/lib/stripe/payment_intent_validator.rb b/lib/stripe/payment_intent_validator.rb index 2deeb74584..10f176af61 100644 --- a/lib/stripe/payment_intent_validator.rb +++ b/lib/stripe/payment_intent_validator.rb @@ -3,10 +3,14 @@ # This class validates if a given payment intent ID is valid in Stripe module Stripe class PaymentIntentValidator - def call(payment) + def initialize(payment) + @payment = payment + end + + def call payment_intent_response = Stripe::PaymentIntent.retrieve( - payment_intent_id(payment), - stripe_account: stripe_account_id(payment) + payment_intent_id, + stripe_account: stripe_account_id ) raise_if_last_payment_error_present(payment_intent_response) @@ -16,11 +20,13 @@ module Stripe private - def payment_intent_id(payment) + attr_accessor :payment + + def payment_intent_id payment.response_code end - def stripe_account_id(payment) + def stripe_account_id enterprise_id = payment.payment_method&.preferred_enterprise_id StripeAccount.find_by(enterprise_id: enterprise_id)&.stripe_user_id diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 909d7d3609..7776d02903 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -113,9 +113,8 @@ describe Spree::OrdersController, type: :controller do let(:payment_intent_response) { double(id: "pi_123", status: "requires_capture") } before do - allow_any_instance_of(Stripe::PaymentIntentValidator) - .to receive(:call) - .with(payment) + allow(Stripe::PaymentIntentValidator) + .to receive_message_chain(:new, :call) .and_return(payment_intent_response) allow(Spree::Order).to receive(:find_by!) { order } @@ -159,9 +158,8 @@ describe Spree::OrdersController, type: :controller do let(:payment_intent) { "pi_123" } before do - allow_any_instance_of(Stripe::PaymentIntentValidator) - .to receive(:call) - .with(payment) + allow(Stripe::PaymentIntentValidator) + .to receive_message_chain(:new, :call) .and_raise(Stripe::StripeError, "error message") end @@ -183,9 +181,8 @@ describe Spree::OrdersController, type: :controller do before do allow(payment).to receive(:response_code).and_return("invalid") allow(OrderPaymentFinder).to receive(:new).with(order).and_return(finder) - allow_any_instance_of(Stripe::PaymentIntentValidator) - .to receive(:call) - .with(payment_intent, kind_of(String)) + allow(Stripe::PaymentIntentValidator) + .to receive_message_chain(:new, :call) .and_return(payment_intent) stub_payment_intent_get_request(payment_intent_id: "valid") end diff --git a/spec/lib/stripe/payment_intent_validator_spec.rb b/spec/lib/stripe/payment_intent_validator_spec.rb index c4ff925b08..12287bafe6 100644 --- a/spec/lib/stripe/payment_intent_validator_spec.rb +++ b/spec/lib/stripe/payment_intent_validator_spec.rb @@ -6,7 +6,7 @@ require 'stripe/payment_intent_validator' module Stripe describe PaymentIntentValidator do describe "#call" do - let(:validator) { Stripe::PaymentIntentValidator.new } + let(:validator) { Stripe::PaymentIntentValidator.new(payment) } let(:payment) { build(:payment, response_code: payment_intent_id) } let(:payment_intent_id) { "pi_123" } let(:stripe_account_id) { "abc123" } @@ -31,7 +31,7 @@ module Stripe it "returns payment intent id and does not raise" do expect { - result = validator.call(payment) + result = validator.call expect(result).to eq payment_intent_response_body }.to_not raise_error Stripe::StripeError end @@ -44,7 +44,7 @@ module Stripe it "raises Stripe error with payment intent last_payment_error as message" do expect { - validator.call(payment) + validator.call }.to raise_error Stripe::StripeError, "No money" end end diff --git a/spec/services/process_payment_intent_spec.rb b/spec/services/process_payment_intent_spec.rb index 01424570af..515ee16065 100644 --- a/spec/services/process_payment_intent_spec.rb +++ b/spec/services/process_payment_intent_spec.rb @@ -51,8 +51,8 @@ describe ProcessPaymentIntent do context "where the stripe payment intent validation responds with errors" do before do - allow(validator) - .to receive(:call).with(payment).and_raise(Stripe::StripeError, "error message") + allow(validator).to receive(:call). + and_raise(Stripe::StripeError, "error message") end it "returns returns the error message" do @@ -76,7 +76,7 @@ describe ProcessPaymentIntent do before do allow(order).to receive(:deliver_order_confirmation_email) - allow(validator).to receive(:call).with(payment).and_return(intent_response) + allow(validator).to receive(:call).and_return(intent_response) end it "validates the intent" do @@ -142,7 +142,7 @@ describe ProcessPaymentIntent do before do payment.update_attribute(:state, "failed") - allow(validator).to receive(:call).with(payment).and_return(intent) + allow(validator).to receive(:call).and_return(intent) end it "does not return any error message" do @@ -165,7 +165,7 @@ describe ProcessPaymentIntent do before do allow(order).to receive(:process_payments!) { nil } - allow(validator).to receive(:call).with(payment).and_return(intent_response) + allow(validator).to receive(:call).and_return(intent_response) end it "returns a failed result" do From afa18611819d118f5deee7f427ff4469a076cd93 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 8 Sep 2021 12:26:56 -0700 Subject: [PATCH 7/7] pass payment to Stripe Validator instead of the status object --- app/services/stripe_payment_status.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/stripe_payment_status.rb b/app/services/stripe_payment_status.rb index a5ed2759d8..0cc03607c9 100644 --- a/app/services/stripe_payment_status.rb +++ b/app/services/stripe_payment_status.rb @@ -13,7 +13,7 @@ class StripePaymentStatus def stripe_status return if payment.response_code.blank? - Stripe::PaymentIntentValidator.new(self).call.status + Stripe::PaymentIntentValidator.new(payment).call.status rescue Stripe::StripeError # Stripe::PaymentIntentValidator will raise an error if the response from the Stripe API # call indicates the last attempted action on the payment intent failed.