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 1/5] 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 From 90712647b1ba374b4f26cd9c23c55ef32fac9a0f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 15:25:23 +0000 Subject: [PATCH 2/5] Update totals when processing a return --- app/models/spree/return_authorization.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index 8b762a19c4..fc5acb3f02 100644 --- a/app/models/spree/return_authorization.rb +++ b/app/models/spree/return_authorization.rb @@ -95,6 +95,7 @@ module Spree credit.save order.return if inventory_units.all?(&:returned?) + order.update! end def allow_receive? From 714e4f7896b68e646741f0ebf5ad8d984148816b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 15:30:57 +0000 Subject: [PATCH 3/5] Update test setup in checkout helper spec --- spec/helpers/checkout_helper_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/helpers/checkout_helper_spec.rb b/spec/helpers/checkout_helper_spec.rb index 076f1f9fb5..966569459a 100644 --- a/spec/helpers/checkout_helper_spec.rb +++ b/spec/helpers/checkout_helper_spec.rb @@ -40,6 +40,7 @@ describe CheckoutHelper, type: :helper do } 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 From c5a47b51a6a25895988a67a9c0422cdd9cf26264 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 16:11:10 +0000 Subject: [PATCH 4/5] Update order totals during CheckoutController#update --- app/controllers/checkout_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index ce04528410..4e3a22415b 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -54,6 +54,8 @@ class CheckoutController < ::BaseController 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 From 29c57031610b82c51771065c60d7fbd0c9e74a4a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 16:46:08 +0000 Subject: [PATCH 5/5] Improve OrdersHelper spec and delete dead code The removed test here was checking for adjustments that have an amount of zero and are eligible. If the amount is zero, it will already be marked as ineligible. --- spec/helpers/admin/orders_helper_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/helpers/admin/orders_helper_spec.rb b/spec/helpers/admin/orders_helper_spec.rb index 4556763d22..13079d4590 100644 --- a/spec/helpers/admin/orders_helper_spec.rb +++ b/spec/helpers/admin/orders_helper_spec.rb @@ -7,13 +7,13 @@ describe Admin::OrdersHelper, type: :helper do 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