Merge pull request #7036 from Matt-Yorkley/adjustments-callbacks

[Adjustments] Reduce adjustments callbacks

app/controllers/cart_controller.rb

@@ -4,29 +4,23 @@ class CartController < BaseController
   def populate
     order = current_order(true)
 
-    # Without intervention, the Spree::Adjustment#update_adjustable callback is called many times
-    # during cart population, for both taxation and enterprise fees. This operation triggers a
-    # costly Spree::Order#update!, which only needs to be run once. We avoid this by disabling
-    # callbacks on Spree::Adjustment and then manually invoke Spree::Order#update! on success.
-    Spree::Adjustment.without_callbacks do
-      cart_service = CartService.new(order)
+    cart_service = CartService.new(order)
 
-      cart_service.populate(params.slice(:variants, :quantity), true)
-      if cart_service.valid?
-        order.recreate_all_fees!
-        order.cap_quantity_at_stock!
-        order.update!
+    cart_service.populate(params.slice(:variants, :quantity), true)
+    if cart_service.valid?
+      order.cap_quantity_at_stock!
+      order.recreate_all_fees!
 
-        variant_ids = variant_ids_in(cart_service.variants_h)
+      variant_ids = variant_ids_in(cart_service.variants_h)
 
-        render json: { error: false,
-                       stock_levels: VariantsStockLevels.new.call(order, variant_ids) },
-               status: :ok
-      else
-        render json: { error: cart_service.errors.full_messages.join(",") },
-               status: :precondition_failed
-      end
+      render json: { error: false,
+                     stock_levels: VariantsStockLevels.new.call(order, variant_ids) },
+             status: :ok
+    else
+      render json: { error: cart_service.errors.full_messages.join(",") },
+             status: :precondition_failed
     end
+
     populate_variant_attributes
   end
 

app/controllers/checkout_controller.rb

@@ -54,6 +54,8 @@ def update
   rescue StandardError => e
     flash[:error] = I18n.t("checkout.failed")
     action_failed(e)
+  ensure
+    @order.update!
   end
 
   # Clears the cached order. Required for #current_order to return a new order

app/controllers/spree/admin/orders_controller.rb

@@ -36,7 +36,6 @@ def edit
 
       def update
         @order.recreate_all_fees!
-        @order.update!
 
         unless order_params.present? && @order.update(order_params) && @order.line_items.present?
           if @order.line_items.empty?

app/models/spree/adjustment.rb

@@ -47,9 +47,6 @@ class Adjustment < ActiveRecord::Base
     validates :label, presence: true
     validates :amount, numericality: true
 
-    after_save :update_adjustable
-    after_destroy :update_adjustable
-
     state_machine :state, initial: :open do
       event :close do
         transition from: :open, to: :closed
@@ -144,29 +141,11 @@ def has_tax?
       included_tax.positive?
     end
 
-    def self.without_callbacks
-      skip_callback :save, :after, :update_adjustable
-      skip_callback :destroy, :after, :update_adjustable
-
-      result = yield
-    ensure
-      set_callback :save, :after, :update_adjustable
-      set_callback :destroy, :after, :update_adjustable
-
-      result
-    end
-
     # Allow accessing soft-deleted originator objects
     def originator
       return if originator_type.blank?
 
       originator_type.constantize.unscoped { super }
     end
-
-    private
-
-    def update_adjustable
-      adjustable.update! if adjustable.is_a? Order
-    end
   end
 end

app/models/spree/return_authorization.rb

@@ -95,6 +95,7 @@ def process_return
       credit.save
 
       order.return if inventory_units.all?(&:returned?)
+      order.update!
     end
 
     def allow_receive?

app/services/order_fees_handler.rb

@@ -20,6 +20,8 @@ def recreate_all_fees!
       create_line_item_fees!
       create_order_fees!
     end
+
+    order.update!
   end
 
   def create_line_item_fees!

spec/controllers/spree/admin/orders_controller_spec.rb

@@ -78,7 +78,6 @@
           order.line_items.last.update(variant_id: variant2.id)
           while !order.completed? do break unless order.next! end
           order.recreate_all_fees!
-          order.update!
           order
         end
 

spec/controllers/spree/orders_controller_spec.rb

@@ -313,7 +313,6 @@
         order.reload.line_items.last.update(variant_id: variant2.id)
         while !order.completed? do break unless order.next! end
         order.recreate_all_fees!
-        order.update!
         order
       end
       let(:params) {

spec/helpers/admin/orders_helper_spec.rb

@@ -7,13 +7,13 @@
     let(:order) { create(:order) }
 
     it "selects eligible adjustments" do
-      adjustment = create(:adjustment, adjustable: order, amount: 1)
+      adjustment = create(:adjustment, order: order, adjustable: order, amount: 1)
 
       expect(helper.order_adjustments_for_display(order)).to eq [adjustment]
     end
 
     it "filters shipping method adjustments" do
-      create(:adjustment, adjustable: order, amount: 1, originator_type: "Spree::ShippingMethod")
+      create(:adjustment, order: order, adjustable: order, amount: 1, originator_type: "Spree::ShippingMethod")
 
       expect(helper.order_adjustments_for_display(order)).to eq []
     end

spec/helpers/checkout_helper_spec.rb

@@ -40,6 +40,7 @@
     }
 
     before do
+      order.update!
       # Sanity check initial adjustments state
       expect(order.shipment_adjustments.count).to eq 1
       expect(order.adjustments.enterprise_fee.count).to eq 1

spec/models/spree/adjustment_spec.rb

@@ -63,18 +63,6 @@ module Spree
       end
     end
 
-    context "#save" do
-      it "should call order#update!" do
-        adjustment = Spree::Adjustment.new(
-          adjustable: order,
-          amount: 10,
-          label: "Foo"
-        )
-        expect(order).to receive(:update!)
-        adjustment.save
-      end
-    end
-
     context "adjustment state" do
       let(:adjustment) { create(:adjustment, state: 'open') }
 

spec/models/spree/order_spec.rb

@@ -525,6 +525,7 @@
 
     before do
       allow(subject).to receive(:fee_handler) { fee_handler }
+      allow(subject).to receive(:update!)
     end
 
     it "clears all enterprise fee adjustments on the order" do