From 75355b035947f31b3ca9ca7cac83cbe992c431c7 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 6 Feb 2024 16:47:18 +1100
Subject: [PATCH 1/5] Add test to cover updating pending payments

---
 .../order_management/order/updater_spec.rb    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb
index 921c4cc18a..127b678ece 100644
--- a/engines/order_management/spec/services/order_management/order/updater_spec.rb
+++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb
@@ -128,6 +128,46 @@ module OrderManagement
           expect(updater).not_to receive(:update_shipments).with(order)
           updater.update
         end
+
+        context "with pending payments" do
+          let!(:payment) { create(:payment, order: , amount: order.total) }
+
+          context "with order in payment state" do
+            let(:order) { create(:order_with_totals, state: "payment") }
+
+            it "updates pending payments" do
+              # update order so the order total will change
+              update_order_quantity(order)
+              order.payments.reload
+
+              expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
+            end
+          end
+
+          context "with order in confirmation state" do
+            let(:order) { create(:order_with_totals, state: "confirmation") }
+
+            it "updates pending payments" do
+              # update order so the order total will change
+              update_order_quantity(order)
+              order.payments.reload
+
+              expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
+            end
+
+          end
+
+          context "with order in cart" do
+            let(:order) { create(:order_with_totals) }
+
+            it "doesn't update pending payments" do
+              # update order so the order total will change
+              update_order_quantity(order)
+
+              expect { updater.update }.not_to change { payment.reload.amount }
+            end
+          end
+        end
       end
 
       it "updates totals once" do
@@ -377,6 +417,10 @@ module OrderManagement
           order.updater.update_payment_state
         end
       end
+
+      def update_order_quantity(order)
+        order.line_items.first.update_attribute(:quantity, 2)
+      end
     end
   end
 end

From e6fba74a875ba103a22f6e129a5ced9faa168c09 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 6 Feb 2024 16:49:08 +1100
Subject: [PATCH 2/5] Make `update_pending_payment` private

It's not used anywhere in the code, there is no reason for it to be
public
---
 .../app/services/order_management/order/updater.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb
index 6c311dade7..497991d18f 100644
--- a/engines/order_management/app/services/order_management/order/updater.rb
+++ b/engines/order_management/app/services/order_management/order/updater.rb
@@ -38,13 +38,6 @@ module OrderManagement
         update_pending_payment
       end
 
-      def update_pending_payment
-        return unless order.state.in? ["payment", "confirmation"]
-        return unless order.pending_payments.any?
-
-        order.pending_payments.first.update_attribute :amount, order.total
-      end
-
       # Updates the following Order total values:
       #
       # - payment_total - total value of all finalized Payments (excludes non-finalized Payments)
@@ -239,6 +232,13 @@ module OrderManagement
       def requires_authorization?
         payments.requires_authorization.any? && payments.completed.empty?
       end
+
+      def update_pending_payment
+        return unless order.state.in? ["payment", "confirmation"]
+        return unless order.pending_payments.any?
+
+        order.pending_payments.first.update_attribute :amount, order.total
+      end
     end
   end
 end

From 7da516b637f7bee3da5ddcd2bb0e26943f798fee Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 6 Feb 2024 16:56:29 +1100
Subject: [PATCH 3/5] Update pending payment for completed order

Update pending payment for cash order, so we can take into account any
changes affecting order toral (shipments, line item quantity etc...).
This is to allow payment capture to cover the order total, not just
the amount due at checkout.
---
 .../app/services/order_management/order/updater.rb |  5 ++++-
 .../order_management/order/updater_spec.rb         | 14 ++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb
index 497991d18f..97b77ace28 100644
--- a/engines/order_management/app/services/order_management/order/updater.rb
+++ b/engines/order_management/app/services/order_management/order/updater.rb
@@ -234,7 +234,10 @@ module OrderManagement
       end
 
       def update_pending_payment
-        return unless order.state.in? ["payment", "confirmation"]
+        # We only want to update complete order pending payment when it's a cash payment. We assume
+        # that if the payment was a credit card it would alread have been processed, so we don't
+        # bother checking the payment type
+        return unless order.state.in? ["payment", "confirmation", "complete"]
         return unless order.pending_payments.any?
 
         order.pending_payments.first.update_attribute :amount, order.total
diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb
index 127b678ece..3dab32c37e 100644
--- a/engines/order_management/spec/services/order_management/order/updater_spec.rb
+++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb
@@ -105,6 +105,20 @@ module OrderManagement
           expect(shipment).to receive(:update!).with(order)
           updater.update_shipments
         end
+
+        context "whith pending payments" do
+          let(:order) { create(:completed_order_with_totals) }
+
+          it "updates pending payments" do
+            payment = create(:payment, order: , amount: order.total)
+
+            # update order so the order total will change
+            update_order_quantity(order)
+            order.payments.reload
+
+            expect { updater.update }.to change { payment.reload.amount }.from(50).to(60)
+          end
+        end
       end
 
       context "incompleted order" do

From c11d30eb13ec16baa8825f29c6dc5f2cfead528a Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Wed, 7 Feb 2024 14:36:03 +1100
Subject: [PATCH 4/5] Refactor order updater spec

Re organise the specs based on the method we are testing, it makes
the spec file more readable.

Best viewed with whitespace hidden
---
 .../order_management/order/updater_spec.rb    | 317 +++++++++---------
 1 file changed, 166 insertions(+), 151 deletions(-)

diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb
index 3dab32c37e..0447e84cb8 100644
--- a/engines/order_management/spec/services/order_management/order/updater_spec.rb
+++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb
@@ -8,7 +8,7 @@ module OrderManagement
       let(:order) { create(:order) }
       let(:updater) { OrderManagement::Order::Updater.new(order) }
 
-      context "updating order totals" do
+      describe "#update_totals" do
         before do
           2.times { create(:line_item, order:, price: 10) }
         end
@@ -19,11 +19,23 @@ module OrderManagement
           updater.update_totals
           expect(order.payment_total).to eq(10)
         end
+      end
+
+      describe "#update_item_total" do
+        before do
+          2.times { create(:line_item, order:, price: 10) }
+        end
 
         it "updates item total" do
           updater.update_item_total
           expect(order.item_total).to eq(20)
         end
+      end
+
+      describe "#update_adjustment_total" do
+        before do
+          2.times { create(:line_item, order:, price: 10) }
+        end
 
         it "updates adjustment totals" do
           allow(order).to receive_message_chain(:all_adjustments, :additional, :eligible,
@@ -40,7 +52,7 @@ module OrderManagement
         end
       end
 
-      context "updating shipment state" do
+      describe "#update_shipment_state" do
         let(:shipment) { build(:shipment) }
 
         before do
@@ -85,19 +97,111 @@ module OrderManagement
         order.state_changed('shipment')
       end
 
-      context "completed order" do
+      describe "#update" do
+        it "updates totals once" do
+          expect(updater).to receive(:update_totals).once
+          updater.update
+        end
+
+        it "updates all adjustments" do
+          expect(updater).to receive(:update_all_adjustments)
+          updater.update
+        end
+
+        context "completed order" do
+          before { allow(order).to receive(:completed?) { true } }
+
+          it "updates payment state" do
+            expect(updater).to receive(:update_payment_state)
+            updater.update
+          end
+
+          it "updates shipment state" do
+            expect(updater).to receive(:update_shipment_state)
+            updater.update
+          end
+
+          context "whith pending payments" do
+            let(:order) { create(:completed_order_with_totals) }
+
+            it "updates pending payments" do
+              payment = create(:payment, order:, amount: order.total)
+
+              # update order so the order total will change
+              update_order_quantity(order)
+              order.payments.reload
+
+              expect { updater.update }.to change { payment.reload.amount }.from(50).to(60)
+            end
+          end
+        end
+
+        context "incompleted order" do
+          before { allow(order).to receive_messages completed?: false }
+
+          it "doesnt update payment state" do
+            expect(updater).not_to receive(:update_payment_state)
+            updater.update
+          end
+
+          it "doesnt update shipment state" do
+            expect(updater).not_to receive(:update_shipment_state)
+            updater.update
+          end
+
+          it "doesnt update the order shipment" do
+            shipment = build(:shipment)
+            allow(order).to receive_messages shipments: [shipment]
+
+            expect(shipment).not_to receive(:update!).with(order)
+            expect(updater).not_to receive(:update_shipments).with(order)
+            updater.update
+          end
+
+          context "with pending payments" do
+            let!(:payment) { create(:payment, order:, amount: order.total) }
+
+            context "with order in payment state" do
+              let(:order) { create(:order_with_totals, state: "payment") }
+
+              it "updates pending payments" do
+                # update order so the order total will change
+                update_order_quantity(order)
+                order.payments.reload
+
+                expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
+              end
+            end
+
+            context "with order in confirmation state" do
+              let(:order) { create(:order_with_totals, state: "confirmation") }
+
+              it "updates pending payments" do
+                # update order so the order total will change
+                update_order_quantity(order)
+                order.payments.reload
+
+                expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
+              end
+            end
+
+            context "with order in cart" do
+              let(:order) { create(:order_with_totals) }
+
+              it "doesn't update pending payments" do
+                # update order so the order total will change
+                update_order_quantity(order)
+
+                expect { updater.update }.not_to change { payment.reload.amount }
+              end
+            end
+          end
+        end
+      end
+
+      describe "#update_shipments" do
         before { allow(order).to receive(:completed?) { true } }
 
-        it "updates payment state" do
-          expect(updater).to receive(:update_payment_state)
-          updater.update
-        end
-
-        it "updates shipment state" do
-          expect(updater).to receive(:update_shipment_state)
-          updater.update
-        end
-
         it "updates the order shipment" do
           shipment = build(:shipment)
           allow(order).to receive_messages shipments: [shipment]
@@ -105,93 +209,6 @@ module OrderManagement
           expect(shipment).to receive(:update!).with(order)
           updater.update_shipments
         end
-
-        context "whith pending payments" do
-          let(:order) { create(:completed_order_with_totals) }
-
-          it "updates pending payments" do
-            payment = create(:payment, order: , amount: order.total)
-
-            # update order so the order total will change
-            update_order_quantity(order)
-            order.payments.reload
-
-            expect { updater.update }.to change { payment.reload.amount }.from(50).to(60)
-          end
-        end
-      end
-
-      context "incompleted order" do
-        before { allow(order).to receive_messages completed?: false }
-
-        it "doesnt update payment state" do
-          expect(updater).not_to receive(:update_payment_state)
-          updater.update
-        end
-
-        it "doesnt update shipment state" do
-          expect(updater).not_to receive(:update_shipment_state)
-          updater.update
-        end
-
-        it "doesnt update the order shipment" do
-          shipment = build(:shipment)
-          allow(order).to receive_messages shipments: [shipment]
-
-          expect(shipment).not_to receive(:update!).with(order)
-          expect(updater).not_to receive(:update_shipments).with(order)
-          updater.update
-        end
-
-        context "with pending payments" do
-          let!(:payment) { create(:payment, order: , amount: order.total) }
-
-          context "with order in payment state" do
-            let(:order) { create(:order_with_totals, state: "payment") }
-
-            it "updates pending payments" do
-              # update order so the order total will change
-              update_order_quantity(order)
-              order.payments.reload
-
-              expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
-            end
-          end
-
-          context "with order in confirmation state" do
-            let(:order) { create(:order_with_totals, state: "confirmation") }
-
-            it "updates pending payments" do
-              # update order so the order total will change
-              update_order_quantity(order)
-              order.payments.reload
-
-              expect { updater.update }.to change { payment.reload.amount }.from(10).to(20)
-            end
-
-          end
-
-          context "with order in cart" do
-            let(:order) { create(:order_with_totals) }
-
-            it "doesn't update pending payments" do
-              # update order so the order total will change
-              update_order_quantity(order)
-
-              expect { updater.update }.not_to change { payment.reload.amount }
-            end
-          end
-        end
-      end
-
-      it "updates totals once" do
-        expect(updater).to receive(:update_totals).once
-        updater.update
-      end
-
-      it "updates all adjustments" do
-        expect(updater).to receive(:update_all_adjustments)
-        updater.update
       end
 
       describe "#update_payment_state" do
@@ -331,9 +348,28 @@ module OrderManagement
             end
           end
         end
+
+        context "when unused payments records exist which require authorization, " \
+                "but the order is fully paid" do
+          let!(:cash_payment) {
+            build(:payment, state: "completed", amount: order.new_outstanding_balance)
+          }
+          let!(:stripe_payment) { build(:payment, state: "requires_authorization") }
+          before do
+            order.payments << cash_payment
+            order.payments << stripe_payment
+          end
+
+          it "cancels unused payments requiring authorization" do
+            expect(stripe_payment).to receive(:void_transaction!)
+            expect(cash_payment).to_not receive(:void_transaction!)
+
+            order.updater.update_payment_state
+          end
+        end
       end
 
-      context '#shipping_address_from_distributor' do
+      describe '#shipping_address_from_distributor' do
         let(:distributor) { build(:distributor_enterprise) }
         let(:shipment) {
           create(:shipment_with, :shipping_method, shipping_method:)
@@ -367,71 +403,50 @@ module OrderManagement
         end
       end
 
-      describe "updating order totals" do
-        describe "#update_totals_and_states" do
-          it "deals with legacy taxes" do
-            expect(updater).to receive(:handle_legacy_taxes)
+      describe "#update_totals_and_states" do
+        it "deals with legacy taxes" do
+          expect(updater).to receive(:handle_legacy_taxes)
 
-            updater.update_totals_and_states
+          updater.update_totals_and_states
+        end
+      end
+
+      describe "#handle_legacy_taxes" do
+        context "when the order is incomplete" do
+          it "doesn't touch taxes" do
+            allow(order).to receive(:completed?) { false }
+
+            expect(order).to_not receive(:create_tax_charge!)
+            updater.__send__(:handle_legacy_taxes)
           end
         end
 
-        describe "#handle_legacy_taxes" do
-          context "when the order is incomplete" do
-            it "doesn't touch taxes" do
-              allow(order).to receive(:completed?) { false }
+        context "when the order is complete" do
+          before { allow(order).to receive(:completed?) { true } }
+
+          context "and the order has legacy taxes" do
+            let!(:legacy_tax_adjustment) {
+              create(:adjustment, order:, adjustable: order, included: false,
+                                  originator_type: "Spree::TaxRate")
+            }
+
+            it "re-applies order taxes" do
+              expect(order).to receive(:create_tax_charge!)
 
-              expect(order).to_not receive(:create_tax_charge!)
               updater.__send__(:handle_legacy_taxes)
             end
           end
 
-          context "when the order is complete" do
-            before { allow(order).to receive(:completed?) { true } }
+          context "and the order has no legacy taxes" do
+            it "leaves taxes untouched" do
+              expect(order).to_not receive(:create_tax_charge!)
 
-            context "and the order has legacy taxes" do
-              let!(:legacy_tax_adjustment) {
-                create(:adjustment, order:, adjustable: order, included: false,
-                                    originator_type: "Spree::TaxRate")
-              }
-
-              it "re-applies order taxes" do
-                expect(order).to receive(:create_tax_charge!)
-
-                updater.__send__(:handle_legacy_taxes)
-              end
-            end
-
-            context "and the order has no legacy taxes" do
-              it "leaves taxes untouched" do
-                expect(order).to_not receive(:create_tax_charge!)
-
-                updater.__send__(:handle_legacy_taxes)
-              end
+              updater.__send__(:handle_legacy_taxes)
             end
           end
         end
       end
 
-      context "when unused payments records exist which require authorization, " \
-              "but the order is fully paid" do
-        let!(:cash_payment) {
-          build(:payment, state: "completed", amount: order.new_outstanding_balance)
-        }
-        let!(:stripe_payment) { build(:payment, state: "requires_authorization") }
-        before do
-          order.payments << cash_payment
-          order.payments << stripe_payment
-        end
-
-        it "cancels unused payments requiring authorization" do
-          expect(stripe_payment).to receive(:void_transaction!)
-          expect(cash_payment).to_not receive(:void_transaction!)
-
-          order.updater.update_payment_state
-        end
-      end
-
       def update_order_quantity(order)
         order.line_items.first.update_attribute(:quantity, 2)
       end

From 5fd36e7eedf70f1bc7a062f2faadd79b29a7a0f0 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Wed, 7 Feb 2024 15:27:51 +1100
Subject: [PATCH 5/5] Fix SubscriptionConfirmJob spec

Fix the payment stubbing, payment `process_offline!` was stubbed to
return true and payment `completed?` was also stubbed to return true.
`process_offline!` did not change the payment status in the database,
meaning that any call to 'order.pending_payments' after that would
return the payment which should have been processed offline, and
therefore completed. This created some unwanted side effect, resulting
in the test breaking.
---
 spec/jobs/subscription_confirm_job_spec.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/spec/jobs/subscription_confirm_job_spec.rb b/spec/jobs/subscription_confirm_job_spec.rb
index 237659e876..ff96e97a60 100644
--- a/spec/jobs/subscription_confirm_job_spec.rb
+++ b/spec/jobs/subscription_confirm_job_spec.rb
@@ -239,8 +239,11 @@ describe SubscriptionConfirmJob do
 
         context "when payments are processed without error" do
           before do
-            expect(payment).to receive(:process_offline!) { true }
-            expect(payment).to receive(:completed?) { true }
+            allow(payment).to receive(:process_offline!) do
+              # Mark payment as complete like it would be if sucessfully processed offline
+              payment.state = "complete"
+              true
+            end
           end
 
           it "sends only a subscription confirm email, no regular confirmation emails" do