From 4c64aaed777e8d05309e9246ea10dae429fcbfb0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 14:35:47 +0000 Subject: [PATCH] Remove callbacks in Adjustment that call order.update! --- app/controllers/cart_controller.rb | 32 ++++++++----------- .../spree/admin/orders_controller.rb | 1 - app/models/spree/adjustment.rb | 21 ------------ app/services/order_fees_handler.rb | 2 ++ .../spree/admin/orders_controller_spec.rb | 1 - .../spree/orders_controller_spec.rb | 1 - spec/models/spree/adjustment_spec.rb | 12 ------- spec/models/spree/order_spec.rb | 1 + 8 files changed, 16 insertions(+), 55 deletions(-) diff --git a/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb index 8d11580aa6..f4ab6abf29 100644 --- a/app/controllers/cart_controller.rb +++ b/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 diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb index 76daa85014..e5bc415bc5 100644 --- a/app/controllers/spree/admin/orders_controller.rb +++ b/app/controllers/spree/admin/orders_controller.rb @@ -36,7 +36,6 @@ module Spree 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? diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 84a7f603de..ef34d0eb9f 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -47,9 +47,6 @@ module Spree 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 @@ module Spree 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 diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb index 3fd09bdb39..55c2416136 100644 --- a/app/services/order_fees_handler.rb +++ b/app/services/order_fees_handler.rb @@ -20,6 +20,8 @@ class OrderFeesHandler create_line_item_fees! create_order_fees! end + + order.update! end def create_line_item_fees! diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index 5bfb20acfd..529743bd6c 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -78,7 +78,6 @@ describe Spree::Admin::OrdersController, type: :controller do 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 diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index 86283702a9..f136a51f64 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -313,7 +313,6 @@ describe Spree::OrdersController, type: :controller do 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) { diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 150e806c87..4c3cf7e687 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/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') } diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index afdbb112ed..4e789af564 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -525,6 +525,7 @@ describe Spree::Order do 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