Avoid methods other than update! on OrderUpdater

core/app/models/spree/adjustment.rb

@@ -105,7 +105,7 @@ def update!
         self.amount = source.compute_amount(adjustable)
 
         if promotion?
-          self.eligible = source.promotion.eligible?(adjustable, promotion_code: promotion_code)
+          self.eligible = calculate_eligibility
         end
 
         # Persist only if changed
@@ -116,6 +116,18 @@ def update!
       amount
     end
 
+    # Calculates based on attached promotion (if this is a promotion
+    # adjustment) whether this promotion is still eligible.
+    # @api private
+    # @return [true,false] Whether this adjustment is eligible
+    def calculate_eligibility
+      if !finalized? && source && promotion?
+        source.promotion.eligible?(adjustable, promotion_code: promotion_code)
+      else
+        eligible?
+      end
+    end
+
     private
 
     def require_promotion_code?

core/app/models/spree/order.rb

@@ -472,12 +472,10 @@ def merge!(*args)
 
     def empty!
       line_items.destroy_all
-      updater.update_item_count
       adjustments.destroy_all
       shipments.destroy_all
 
-      update_totals
-      persist_totals
+      update!
     end
 
     alias_method :has_step?, :has_checkout_step?
@@ -570,9 +568,9 @@ def shipping_eq_billing_address?
 
     def set_shipments_cost
       shipments.each(&:update_amounts)
-      updater.update_shipment_total
-      persist_totals
+      update!
     end
+    deprecate set_shipments_cost: :update!, deprecator: Spree::Deprecation
 
     def is_risky?
       payments.risky.count > 0
@@ -796,9 +794,12 @@ def ensure_inventory_units
     end
 
     def ensure_promotions_eligible
-      updater.update_adjustment_total
-      if promo_total_changed?
+      adjustment_changed = all_adjustments.eligible.promotion.any? do |adjustment|
+        !adjustment.calculate_eligibility
+      end
+      if adjustment_changed
         restart_checkout_flow
+        update!
         errors.add(:base, Spree.t(:promotion_total_changed_before_complete))
       end
       errors.empty?

core/app/models/spree/order/checkout.rb

@@ -79,7 +79,6 @@ def define_state_machine!
               end
 
               after_transition to: :complete, do: :add_payment_sources_to_wallet
-              before_transition to: :payment, do: :set_shipments_cost
               before_transition to: :payment, do: :add_default_payment_from_wallet
 
               before_transition to: :confirm, do: :add_store_credit_payments
@@ -99,7 +98,6 @@ def define_state_machine!
               before_transition to: :delivery, do: :ensure_shipping_address
               before_transition to: :delivery, do: :create_proposed_shipments
               before_transition to: :delivery, do: :ensure_available_shipping_rates
-              before_transition to: :delivery, do: :set_shipments_cost
               before_transition from: :delivery, do: :apply_free_shipping_promotions
             end
 
@@ -125,8 +123,7 @@ def define_state_machine!
             after_transition to: :canceled, do: :after_cancel
 
             after_transition from: any - :cart, to: any - [:confirm, :complete] do |order|
-              order.update_totals
-              order.persist_totals
+              order.update!
             end
 
             after_transition do |order, transition|

core/app/models/spree/order_update_attributes.rb

@@ -17,12 +17,7 @@ def apply
       assign_order_attributes
       assign_payments_attributes
 
-      if order.save
-        order.set_shipments_cost if order.shipments.any?
-        true
-      else
-        false
-      end
+      order.save
     end
 
     private

core/app/models/spree/order_updater.rb

@@ -17,6 +17,7 @@ def initialize(order)
     def update
       @order.transaction do
         update_item_count
+        update_shipment_amounts
         update_totals
         if order.completed?
           update_payment_state
@@ -32,6 +33,65 @@ def run_hooks
       update_hooks.each { |hook| order.send hook }
     end
 
+    # Updates 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"
+    #           or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment.
+    # ready     when all Shipments are in the "ready" state
+    # backorder when there is backordered inventory associated with an order
+    # pending   when all Shipments are in the "pending" state
+    #
+    # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
+    def update_shipment_state
+      if order.backordered?
+        order.shipment_state = 'backorder'
+      else
+        # get all the shipment states for this order
+        shipment_states = shipments.states
+        if shipment_states.size > 1
+          # multiple shiment states means it's most likely partially shipped
+          order.shipment_state = 'partial'
+        else
+          # will return nil if no shipments are found
+          order.shipment_state = shipment_states.first
+          # TODO: inventory unit states?
+          # if order.shipment_state && order.inventory_units.where(:shipment_id => nil).exists?
+          #   shipments exist but there are unassigned inventory units
+          #   order.shipment_state = 'partial'
+          # end
+        end
+      end
+
+      order.state_changed('shipment')
+      order.shipment_state
+    end
+
+    # Updates the +payment_state+ attribute according to the following logic:
+    #
+    # paid          when +payment_total+ is equal to +total+
+    # balance_due   when +payment_total+ is less than +total+
+    # credit_owed   when +payment_total+ is greater than +total+
+    # failed        when most recent payment is in the failed state
+    #
+    # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
+    def update_payment_state
+      last_state = order.payment_state
+      if payments.present? && payments.valid.size == 0 && order.outstanding_balance != 0
+        order.payment_state = 'failed'
+      elsif order.state == 'canceled' && order.payment_total == 0
+        order.payment_state = 'void'
+      else
+        order.payment_state = 'balance_due' if order.outstanding_balance > 0
+        order.payment_state = 'credit_owed' if order.outstanding_balance < 0
+        order.payment_state = 'paid' if !order.outstanding_balance?
+      end
+      order.state_changed('payment') if last_state != order.payment_state
+      order.payment_state
+    end
+
+    private
+
     # This will update and select the best promotion adjustment, update tax
     # adjustments, update cancellation adjustments, and then update the total
     # fields (promo_total, included_tax_total, additional_tax_total, and
@@ -64,13 +124,16 @@ def update_totals
       update_adjustment_total
     end
 
+    def update_shipment_amounts
+      shipments.each do |shipment|
+        shipment.update_amounts
+      end
+    end
+
     # give each of the shipments a chance to update themselves
     def update_shipments
       shipments.each do |shipment|
-        next unless shipment.persisted?
         shipment.update!(order)
-        shipment.refresh_rates
-        shipment.update_amounts
       end
     end
 
@@ -114,65 +177,6 @@ def persist_totals
       order.save!(validate: false)
     end
 
-    # Updates 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"
-    #           or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment.
-    # ready     when all Shipments are in the "ready" state
-    # backorder when there is backordered inventory associated with an order
-    # pending   when all Shipments are in the "pending" state
-    #
-    # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
-    def update_shipment_state
-      if order.backordered?
-        order.shipment_state = 'backorder'
-      else
-        # get all the shipment states for this order
-        shipment_states = shipments.states
-        if shipment_states.size > 1
-          # multiple shiment states means it's most likely partially shipped
-          order.shipment_state = 'partial'
-        else
-          # will return nil if no shipments are found
-          order.shipment_state = shipment_states.first
-          # TODO: inventory unit states?
-          # if order.shipment_state && order.inventory_units.where(shipment_id: nil).exists?
-          #   shipments exist but there are unassigned inventory units
-          #   order.shipment_state = 'partial'
-          # end
-        end
-      end
-
-      order.state_changed('shipment')
-      order.shipment_state
-    end
-
-    # Updates the +payment_state+ attribute according to the following logic:
-    #
-    # paid          when +payment_total+ is equal to +total+
-    # balance_due   when +payment_total+ is less than +total+
-    # credit_owed   when +payment_total+ is greater than +total+
-    # failed        when most recent payment is in the failed state
-    #
-    # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention.
-    def update_payment_state
-      last_state = order.payment_state
-      if payments.present? && payments.valid.size == 0 && order.outstanding_balance != 0
-        order.payment_state = 'failed'
-      elsif order.state == 'canceled' && order.payment_total == 0
-        order.payment_state = 'void'
-      else
-        order.payment_state = 'balance_due' if order.outstanding_balance > 0
-        order.payment_state = 'credit_owed' if order.outstanding_balance < 0
-        order.payment_state = 'paid' if !order.outstanding_balance?
-      end
-      order.state_changed('payment') if last_state != order.payment_state
-      order.payment_state
-    end
-
-    private
-
     def round_money(n)
       (n * 100).round / 100.0
     end

core/app/models/spree/promotion_handler/coupon.rb

@@ -95,8 +95,7 @@ def determine_promotion_application_result
         discount ||= order.adjustments.promotion.detect(&detector)
 
         if discount && discount.eligible
-          order.update_totals
-          order.persist_totals
+          order.update!
           set_success_code :coupon_code_applied
         elsif order.promotions.with_coupon_code(order.coupon_code)
           # if the promotion exists on an order, but wasn't found above,

core/spec/models/spree/order/checkout_spec.rb

@@ -254,11 +254,6 @@ def assert_state_changed(order, from, to)
           allow(order).to receive(:ensure_available_shipping_rates) { true }
         end
 
-        it 'should invoke set_shipment_cost' do
-          expect(order).to receive(:set_shipments_cost)
-          order.next!
-        end
-
         it 'should update shipment_total' do
           expect { order.next! }.to change{ order.shipment_total }.by(10.00)
         end
@@ -300,7 +295,6 @@ def assert_state_changed(order, from, to)
         end
 
         it "transitions to payment" do
-          expect(order).to receive(:set_shipments_cost)
           order.next!
           assert_state_changed(order, 'delivery', 'payment')
           expect(order.state).to eq('payment')
@@ -333,7 +327,7 @@ def assert_state_changed(order, from, to)
         context "with a shipment that has a price" do
           before do
             shipment.shipping_rates.first.update_column(:cost, 10)
-            order.set_shipments_cost
+            order.update!
           end
 
           it "transitions to payment" do
@@ -345,7 +339,6 @@ def assert_state_changed(order, from, to)
         context "with a shipment that is free" do
           before do
             shipment.shipping_rates.first.update_column(:cost, 0)
-            order.set_shipments_cost
           end
 
           it "skips payment, transitions to confirm" do

core/spec/models/spree/order_spec.rb

@@ -107,10 +107,11 @@
 
     it "update and persist totals" do
       expect(shipment).to receive :update_amounts
-      expect(order.updater).to receive :update_shipment_total
-      expect(order.updater).to receive :persist_totals
+      expect(order.updater).to receive :update
 
-      order.set_shipments_cost
+      Spree::Deprecation.silence do
+        order.set_shipments_cost
+      end
     end
   end
 

core/spec/models/spree/order_update_attributes_spec.rb

@@ -39,44 +39,5 @@ module Spree
         end
       end
     end
-
-    context 'when changing shipping method' do
-      let!(:order) { create(:order_with_line_items, shipping_method: shipping_method1) }
-      let(:shipment){ order.shipments.first }
-      let!(:zone) { create(:zone) }
-      let!(:shipping_method1){ create(:shipping_method, cost: 10, zones: [zone]) }
-      let!(:shipping_method2){ create(:shipping_method, cost: 20, zones: [zone]) }
-
-      let(:attributes) do
-        {
-          shipments_attributes: {
-            0 => { selected_shipping_rate_id: shipping_method2, id: shipment.id }
-          }
-        }
-      end
-
-      it "updates shipment costs" do
-        zone.zone_members.create!(zoneable: order.ship_address.country)
-        order.create_proposed_shipments
-        order.set_shipments_cost
-
-        shipping_rate2 = shipment.shipping_rates.find_by(shipping_method_id: shipping_method2.id)
-
-        expect(order.shipment_total).to eq(10)
-
-        # We need an order which doesn't have shipping_rates loaded
-        order.reload
-
-        described_class.new(
-          order,
-          shipments_attributes: {
-            0 => { selected_shipping_rate_id: shipping_rate2.id, id: shipment.id }
-          }
-        ).apply
-
-        expect(order.shipment_total).to eq(20)
-        expect(order.shipments.first.cost).to eq(20)
-      end
-    end
   end
 end