Merge pull request #5257 from luisramos0/stripe_sca_payments_last

StripeSCA - reuse better method to fetch last payment of an order to avoid nasty bugs in the future

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

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

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)

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)

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

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  

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

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(

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!

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!

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