diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 78aca3479c..967f4aa8ac 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -146,10 +146,12 @@ class CheckoutController < Spree::StoreController end def valid_payment_intent_provided? - params["payment_intent"]&.starts_with?("pi_") && - @order.state == "payment" && - @order.payments.last.state == "pending" && - @order.payments.last.response_code == params["payment_intent"] + return false unless params["payment_intent"]&.starts_with?("pi_") + + last_payment = OrderPaymentFinder.new(@order).last_payment + @order.state == "payment" && + last_payment&.state == "pending" && + last_payment&.response_code == params["payment_intent"] end def handle_redirect_from_stripe diff --git a/app/helpers/order_helper.rb b/app/helpers/order_helper.rb new file mode 100644 index 0000000000..157ee212bb --- /dev/null +++ b/app/helpers/order_helper.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module OrderHelper + def last_payment_method(order) + OrderPaymentFinder.new(order).last_payment&.payment_method + end +end diff --git a/app/mailers/spree/order_mailer_decorator.rb b/app/mailers/spree/order_mailer_decorator.rb index bffc26ee46..fbf17013b8 100644 --- a/app/mailers/spree/order_mailer_decorator.rb +++ b/app/mailers/spree/order_mailer_decorator.rb @@ -2,6 +2,7 @@ Spree::OrderMailer.class_eval do helper HtmlHelper helper CheckoutHelper helper SpreeCurrencyHelper + helper OrderHelper include I18nHelper def cancel_email(order_or_order_id, resend = false) diff --git a/app/mailers/subscription_mailer.rb b/app/mailers/subscription_mailer.rb index e2216c01c9..0267e24ef4 100644 --- a/app/mailers/subscription_mailer.rb +++ b/app/mailers/subscription_mailer.rb @@ -1,6 +1,7 @@ class SubscriptionMailer < Spree::BaseMailer helper CheckoutHelper helper ShopMailHelper + helper OrderHelper include I18nHelper def confirmation_email(order) diff --git a/app/services/order_payment_finder.rb b/app/services/order_payment_finder.rb index c28d0e139a..54b19654a3 100644 --- a/app/services/order_payment_finder.rb +++ b/app/services/order_payment_finder.rb @@ -1,14 +1,28 @@ # frozen_string_literal: true -module OrderPaymentFinder - def self.last_payment_method(order) - # `max_by` avoids additional database queries when payments are loaded - # already. There is usually only one payment and this shouldn't cause - # any overhead compared to `order(:created_at).last`. Using `last` - # without order is not deterministic. - # - # We are not using `updated_at` because all payments are touched when the - # order is updated and then all payments have the same `updated_at` value. - order.payments.max_by(&:created_at)&.payment_method +class OrderPaymentFinder + def initialize(order) + @order = order + end + + def last_payment + last(@order.payments) + end + + def last_pending_payment + last(@order.pending_payments) + end + + private + + # `max_by` avoids additional database queries when payments are loaded + # already. There is usually only one payment and this shouldn't cause + # any overhead compared to `order(:created_at).last`. Using `last` + # without order is not deterministic. + # + # We are not using `updated_at` because all payments are touched when the + # order is updated and then all payments have the same `updated_at` value. + def last(payments) + payments.max_by(&:created_at) end end diff --git a/app/views/spree/order_mailer/_payment.html.haml b/app/views/spree/order_mailer/_payment.html.haml index 60fc1056f4..85ad234cda 100644 --- a/app/views/spree/order_mailer/_payment.html.haml +++ b/app/views/spree/order_mailer/_payment.html.haml @@ -8,7 +8,7 @@ = t :email_payment_summary %h4 = t :email_payment_method - %strong= OrderPaymentFinder.last_payment_method(@order)&.name + %strong= last_payment_method(@order)&.name %p - %em= OrderPaymentFinder.last_payment_method(@order)&.description + %em= last_payment_method(@order)&.description %p   diff --git a/app/views/spree/shared/_order_details.html.haml b/app/views/spree/shared/_order_details.html.haml index 22c8ccdf8f..f4861d8148 100644 --- a/app/views/spree/shared/_order_details.html.haml +++ b/app/views/spree/shared/_order_details.html.haml @@ -13,9 +13,9 @@ .pad .text-big = t :order_payment - %strong= OrderPaymentFinder.last_payment_method(order)&.name + %strong= last_payment_method(order)&.name %p.text-small.text-skinny.pre-line - %em= OrderPaymentFinder.last_payment_method(order)&.description + %em= last_payment_method(order)&.description .order-summary.text-small %strong diff --git a/engines/order_management/app/services/order_management/subscriptions/payment_setup.rb b/engines/order_management/app/services/order_management/subscriptions/payment_setup.rb index 1824247e37..6d90236466 100644 --- a/engines/order_management/app/services/order_management/subscriptions/payment_setup.rb +++ b/engines/order_management/app/services/order_management/subscriptions/payment_setup.rb @@ -18,7 +18,7 @@ module OrderManagement private def create_payment - payment = @order.pending_payments.last + payment = OrderPaymentFinder.new(@order).last_pending_payment return payment if payment.present? @order.payments.create( diff --git a/engines/order_management/app/services/order_management/subscriptions/stripe_payment_setup.rb b/engines/order_management/app/services/order_management/subscriptions/stripe_payment_setup.rb index 8bc7e4cb9e..37e3a097f3 100644 --- a/engines/order_management/app/services/order_management/subscriptions/stripe_payment_setup.rb +++ b/engines/order_management/app/services/order_management/subscriptions/stripe_payment_setup.rb @@ -5,7 +5,7 @@ module OrderManagement class StripePaymentSetup def initialize(order) @order = order - @payment = @order.pending_payments.last + @payment = OrderPaymentFinder.new(@order).last_pending_payment end def call! diff --git a/engines/order_management/app/services/order_management/subscriptions/stripe_sca_payment_authorize.rb b/engines/order_management/app/services/order_management/subscriptions/stripe_sca_payment_authorize.rb index da8830ce70..ea4fbe5ce7 100644 --- a/engines/order_management/app/services/order_management/subscriptions/stripe_sca_payment_authorize.rb +++ b/engines/order_management/app/services/order_management/subscriptions/stripe_sca_payment_authorize.rb @@ -5,7 +5,7 @@ module OrderManagement class StripeScaPaymentAuthorize def initialize(order) @order = order - @payment = @order.pending_payments.last + @payment = OrderPaymentFinder.new(@order).last_pending_payment end def call! diff --git a/spec/services/order_payment_finder_spec.rb b/spec/services/order_payment_finder_spec.rb new file mode 100644 index 0000000000..3d71d9a706 --- /dev/null +++ b/spec/services/order_payment_finder_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe OrderPaymentFinder do + let(:order) { create(:order_with_distributor) } + let(:finder) { OrderPaymentFinder.new(order) } + + context "when order has several non pending payments" do + let!(:failed_payment) { create(:payment, order: order, state: 'failed') } + let!(:complete_payment) { create(:payment, order: order, state: 'completed') } + + it "#last_payment returns the last payment" do + expect(finder.last_payment).to eq complete_payment + end + + it "#last_pending_payment returns nil" do + expect(finder.last_pending_payment).to be nil + end + end + + context "when order has a pending payment and a non pending payment" do + let!(:processing_payment) { create(:payment, order: order, state: 'processing') } + let!(:failed_payment) { create(:payment, order: order, state: 'failed') } + + it "#last_payment returns the last payment" do + expect(finder.last_payment).to eq failed_payment + end + + it "#last_pending_payment returns the pending payment" do + # a payment in the processing state is a pending payment + expect(finder.last_pending_payment).to eq processing_payment + end + + context "and an extra last pending payment" do + let!(:pending_payment) { create(:payment, order: order, state: 'pending') } + + it "#last_payment returns the last payment" do + expect(finder.last_payment).to eq pending_payment + end + + it "#last_pending_payment returns the pending payment" do + expect(finder.last_pending_payment).to eq pending_payment + end + end + end +end