Use Shipment#recalculate_state

We don't want our in-memory order updater to have to persist shipment
state.

Because we no longer rely on `Shipment#update_state` which uses
`update_column`, we are now logging any additional `Shipment#state`
changes when the order is saved. The changes to the specs reflect this
change in behaviour.

Co-authored-by: Jared Norman <jared@super.gd>
Co-authored-by: Noah Silvera <noah@super.gd>
Co-authored-by: Alistair Norman <alistair@super.gd>

Author: 4 people

core/app/models/spree/in_memory_order_updater.rb

@@ -48,7 +48,6 @@ def recalculate(persist: true)
         monitor.call do
           if order.completed?
             recalculate_payment_state
-            update_shipments
             recalculate_shipment_state
           end
         end
@@ -61,7 +60,8 @@ def recalculate(persist: true)
     alias_method :update, :recalculate
     deprecate update: :recalculate, deprecator: Spree.deprecator
 
-    # Recalculates the +shipment_state+ attribute according to the following logic:
+    # Recalculates the state on all of them shipments, then recalculates the
+    # +shipment_state+ attribute according to the following logic:
     #
     # shipped   when all Shipments are in the "shipped" state
     # partial   when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped"
@@ -72,6 +72,8 @@ def recalculate(persist: true)
     #
     # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
     def recalculate_shipment_state
+      shipments.each(&:recalculate_state)
+
       log_state_change('shipment') do
         order.shipment_state = determine_shipment_state
       end
@@ -217,11 +219,6 @@ def persist_item_totals
       end
     end
 
-    # give each of the shipments a chance to update themselves
-    def update_shipments
-      shipments.each(&:update_state)
-    end
-
     def recalculate_payment_total
       order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) }
     end

core/spec/models/spree/in_memory_order_updater_spec.rb

@@ -544,7 +544,7 @@ module Spree
         order.shipments = [shipment]
         allow(order.shipments).to receive_messages(states: [], ready: [], pending: [], shipped: [])
         allow(updater).to receive(:update_totals) # Otherwise this gets called and causes a scene
-        expect(updater).not_to receive(:update_shipments)
+        expect(updater).not_to receive(:recalculate_shipment_state)
         updater.recalculate
       end
     end

core/spec/models/spree/shipment_spec.rb

@@ -983,14 +983,18 @@
   it_behaves_like "customer and admin metadata fields: storage and validation", :shipment
 
   describe "state change tracking" do
+    # Ensure that we're only testing tracking jobs created by the change in
+    # state.
+    before { shipment }
+
     it "enqueues a StateChangeTrackingJob when state changes" do
       expect {
         shipment.update!(state: 'shipped')
       }.to have_enqueued_job(Spree::StateChangeTrackingJob).with(
         shipment,
         'pending',
         'shipped',
-        kind_of(Time)
+        be_within(1.second).of(Time.current)
       )
     end
 
@@ -1000,20 +1004,6 @@
       }.not_to have_enqueued_job(Spree::StateChangeTrackingJob)
     end
 
-    it "captures the transition timestamp accurately" do
-      before_time = Time.current
-
-      shipment.update!(state: 'shipped')
-
-      # Check that a job was enqueued with a timestamp close to when we made the change
-      expect(Spree::StateChangeTrackingJob).to have_been_enqueued.with do |shipment_id, prev_state, next_state, timestamp|
-        expect(shipment_id).to eq(shipment.id)
-        expect(prev_state).to eq('pending')
-        expect(next_state).to eq('shipped')
-        expect(timestamp).to be_within(1.second).of(before_time)
-      end
-    end
-
     it "creates multiple state transitions" do
       clear_enqueued_jobs
 
@@ -1027,7 +1017,7 @@
       perform_enqueued_jobs do
         expect {
           shipment.update!(state: 'shipped')
-        }.to change(Spree::StateChange, :count).by(1)
+        }.to change(shipment.state_changes, :count).by(1)
       end
 
       state_change = Spree::StateChange.last