From c65f623ed8eeff6be6802b967c71f067b6974ab6 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 00:05:51 +0000
Subject: [PATCH 01/17] Memoize and simplify objects being checked in
 Order#provided_by_order_cycle?

---
 app/models/spree/order.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 900cf0fe76..77965c54b4 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -831,8 +831,8 @@ module Spree
     end
 
     def provided_by_order_cycle?(line_item)
-      order_cycle_variants = order_cycle.andand.variants || []
-      order_cycle_variants.include? line_item.variant
+      @order_cycle_variant_ids ||= order_cycle&.variants&.map(&:id) || []
+      @order_cycle_variant_ids.include? line_item.variant_id
     end
 
     def require_customer?

From e7866db7b100deef4ba519d53d1f7c6bf11e2f8e Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 00:07:34 +0000
Subject: [PATCH 02/17] Improve efficiency in applying enterprise fees in
 Order#update_distribution_charge!

---
 app/models/spree/order.rb                                 | 8 +++++---
 lib/open_food_network/enterprise_fee_calculator.rb        | 5 -----
 .../open_food_network/enterprise_fee_calculator_spec.rb   | 4 ++--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 77965c54b4..c8acfae5bb 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -669,17 +669,19 @@ module Spree
       with_lock do
         EnterpriseFee.clear_all_adjustments_on_order self
 
+        fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle)
+
         loaded_line_items =
-          line_items.includes(variant: :product, order: [:distributor, :order_cycle]).all
+          line_items.includes(variant: :product).all
 
         loaded_line_items.each do |line_item|
           if provided_by_order_cycle? line_item
-            OpenFoodNetwork::EnterpriseFeeCalculator.new.create_line_item_adjustments_for line_item
+            fee_calculator.create_line_item_adjustments_for line_item
           end
         end
 
         if order_cycle
-          OpenFoodNetwork::EnterpriseFeeCalculator.new.create_order_adjustments_for self
+          fee_calculator.create_order_adjustments_for self
         end
       end
     end
diff --git a/lib/open_food_network/enterprise_fee_calculator.rb b/lib/open_food_network/enterprise_fee_calculator.rb
index 08eec58899..47db702182 100644
--- a/lib/open_food_network/enterprise_fee_calculator.rb
+++ b/lib/open_food_network/enterprise_fee_calculator.rb
@@ -39,8 +39,6 @@ module OpenFoodNetwork
 
     def create_line_item_adjustments_for(line_item)
       variant = line_item.variant
-      @distributor = line_item.order.distributor
-      @order_cycle = line_item.order.order_cycle
 
       per_item_enterprise_fee_applicators_for(variant).each do |applicator|
         applicator.create_line_item_adjustment(line_item)
@@ -48,9 +46,6 @@ module OpenFoodNetwork
     end
 
     def create_order_adjustments_for(order)
-      @distributor = order.distributor
-      @order_cycle = order.order_cycle
-
       per_order_enterprise_fee_applicators_for(order).each do |applicator|
         applicator.create_order_adjustment(order)
       end
diff --git a/spec/lib/open_food_network/enterprise_fee_calculator_spec.rb b/spec/lib/open_food_network/enterprise_fee_calculator_spec.rb
index 0dc9b121b4..97068516f9 100644
--- a/spec/lib/open_food_network/enterprise_fee_calculator_spec.rb
+++ b/spec/lib/open_food_network/enterprise_fee_calculator_spec.rb
@@ -141,7 +141,7 @@ module OpenFoodNetwork
         it "creates adjustments for a line item" do
           exchange.enterprise_fees << enterprise_fee_line_item
 
-          EnterpriseFeeCalculator.new.create_line_item_adjustments_for line_item
+          EnterpriseFeeCalculator.new(distributor, order_cycle).create_line_item_adjustments_for line_item
 
           a = Spree::Adjustment.last
           expect(a.metadata.fee_name).to eq(enterprise_fee_line_item.name)
@@ -150,7 +150,7 @@ module OpenFoodNetwork
         it "creates adjustments for an order" do
           exchange.enterprise_fees << enterprise_fee_order
 
-          EnterpriseFeeCalculator.new.create_order_adjustments_for order
+          EnterpriseFeeCalculator.new(distributor, order_cycle).create_order_adjustments_for order
 
           a = Spree::Adjustment.last
           expect(a.metadata.fee_name).to eq(enterprise_fee_order.name)

From 785cdf9bdc9656a0c4fb2f5effd37dd5816276c3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 00:25:30 +0000
Subject: [PATCH 03/17] Extract order fees logic to service

---
 app/models/spree/order.rb                | 22 ++-----------
 app/services/order_fees_handler.rb       | 36 +++++++++++++++++++++
 spec/models/spree/order_spec.rb          | 16 +++++++--
 spec/services/order_fees_handler_spec.rb | 41 ++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 22 deletions(-)
 create mode 100644 app/services/order_fees_handler.rb
 create mode 100644 spec/services/order_fees_handler_spec.rb

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index c8acfae5bb..0b4d211c9f 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -669,20 +669,9 @@ module Spree
       with_lock do
         EnterpriseFee.clear_all_adjustments_on_order self
 
-        fee_calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle)
-
-        loaded_line_items =
-          line_items.includes(variant: :product).all
-
-        loaded_line_items.each do |line_item|
-          if provided_by_order_cycle? line_item
-            fee_calculator.create_line_item_adjustments_for line_item
-          end
-        end
-
-        if order_cycle
-          fee_calculator.create_order_adjustments_for self
-        end
+        fee_handler = OrderFeesHandler.new(self)
+        fee_handler.create_line_item_fees!
+        fee_handler.create_order_fees!
       end
     end
 
@@ -832,11 +821,6 @@ module Spree
       subscription.present? && order_cycle.orders_close_at.andand > Time.zone.now
     end
 
-    def provided_by_order_cycle?(line_item)
-      @order_cycle_variant_ids ||= order_cycle&.variants&.map(&:id) || []
-      @order_cycle_variant_ids.include? line_item.variant_id
-    end
-
     def require_customer?
       return true unless new_record? || state == 'cart'
     end
diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb
new file mode 100644
index 0000000000..f3514d4465
--- /dev/null
+++ b/app/services/order_fees_handler.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class OrderFeesHandler
+  attr_reader :order, :distributor, :order_cycle
+
+  def initialize(order)
+    @order = order
+    @distributor = order.distributor
+    @order_cycle = order.order_cycle
+  end
+
+  def create_line_item_fees!
+    order.line_items.includes(variant: :product).each do |line_item|
+      if provided_by_order_cycle? line_item
+        calculator.create_line_item_adjustments_for line_item
+      end
+    end
+  end
+
+  def create_order_fees!
+    return unless order_cycle
+
+    calculator.create_order_adjustments_for order
+  end
+
+  private
+
+  def calculator
+    @calculator ||= OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle)
+  end
+
+  def provided_by_order_cycle?(line_item)
+    @order_cycle_variant_ids ||= order_cycle&.variants&.map(&:id) || []
+    @order_cycle_variant_ids.include? line_item.variant_id
+  end
+end
diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index 73f2b228e1..af5d5d6b57 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -533,14 +533,24 @@ describe Spree::Order do
     end
   end
 
-  describe "updating the distribution charge" do
-    let(:order) { build(:order) }
+  describe "applying enterprise fees" do
+    let(:fee_handler) { ::OrderFeesHandler.new(subject) }
+
+    before do
+      allow(subject).to receive(:fee_handler) { fee_handler }
+    end
 
     it "clears all enterprise fee adjustments on the order" do
       expect(EnterpriseFee).to receive(:clear_all_adjustments_on_order).with(subject)
       subject.update_distribution_charge!
     end
 
+    it "creates line item and order fee adjustments via OrderFeesHandler" do
+      expect(fee_handler).to receive(:create_line_item_fees!)
+      expect(fee_handler).to receive(:create_order_fees!)
+      subject.recreate_all_fees!
+    end
+
     it "skips order cycle per-order adjustments for orders that don't have an order cycle" do
       allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
 
@@ -552,7 +562,7 @@ describe Spree::Order do
     it "ensures the correct adjustment(s) are created for order cycles" do
       allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
       line_item = create(:line_item, order: subject)
-      allow(subject).to receive(:provided_by_order_cycle?) { true }
+      allow(fee_handler).to receive(:provided_by_order_cycle?) { true }
 
       order_cycle = double(:order_cycle)
       expect_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).
diff --git a/spec/services/order_fees_handler_spec.rb b/spec/services/order_fees_handler_spec.rb
new file mode 100644
index 0000000000..e77f0bcc29
--- /dev/null
+++ b/spec/services/order_fees_handler_spec.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe OrderFeesHandler do
+  let(:order_cycle) { create(:order_cycle) }
+  let(:order) { create(:order_with_line_items, line_items_count: 1, order_cycle: order_cycle) }
+  let(:line_item) { order.line_items.first }
+
+  let(:service) { OrderFeesHandler.new(order) }
+  let(:calculator) {
+    double(OpenFoodNetwork::EnterpriseFeeCalculator, create_order_adjustments_for: true)
+  }
+
+  before do
+    allow(service).to receive(:calculator) { calculator }
+  end
+
+  describe "#create_line_item_fees!" do
+    it "creates per-line-item fee adjustments for line items in the order cycle" do
+      allow(service).to receive(:provided_by_order_cycle?) { true }
+      expect(calculator).to receive(:create_line_item_adjustments_for).with(line_item)
+
+      service.create_line_item_fees!
+    end
+  end
+
+  describe "#create_order_fees!" do
+    it "creates per-order adjustment for the order cycle" do
+      expect(calculator).to receive(:create_order_adjustments_for).with(order)
+      service.create_order_fees!
+    end
+
+    it "skips per-order fee adjustments for orders that don't have an order cycle" do
+      allow(service).to receive(:order_cycle) { nil }
+      expect(calculator).to_not receive(:create_order_adjustments_for)
+
+      service.create_order_fees!
+    end
+  end
+end

From 9abf6cdcdff1932fe081575a81c92c1c85b1b53d Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 00:31:25 +0000
Subject: [PATCH 04/17] Rename expensive method
 Order#update_distribution_charge!

This method is named "update distribution charge". What this method actually does is delete all of the fee adjustments on an order and all it's line items, then recreate them all from scratch. We call this from lots of different places all the time, and it's incredibly expensive. It even gets called from inside of transactions being run inside callbacks. Renaming it hopefully will add a bit of clarity.

This needs to be a lot more granular!
---
 .../admin/bulk_line_items_controller.rb            |  2 +-
 app/controllers/cart_controller.rb                 |  2 +-
 app/controllers/line_items_controller.rb           |  2 +-
 app/controllers/spree/admin/orders_controller.rb   |  8 ++++----
 app/controllers/spree/orders_controller.rb         |  2 +-
 app/models/spree/order.rb                          |  2 +-
 app/services/order_factory.rb                      |  2 +-
 .../admin/bulk_line_items_controller_spec.rb       |  2 +-
 spec/controllers/line_items_controller_spec.rb     |  2 +-
 .../spree/admin/orders_controller_spec.rb          |  4 ++--
 spec/controllers/spree/orders_controller_spec.rb   |  2 +-
 spec/factories/order_factory.rb                    |  2 +-
 spec/features/admin/reports_spec.rb                |  2 +-
 spec/features/consumer/shopping/cart_spec.rb       |  2 +-
 spec/models/spree/adjustment_spec.rb               | 14 +++++++-------
 spec/models/spree/order_spec.rb                    | 10 +++++-----
 spec/models/tag_rule/discount_order_spec.rb        |  2 +-
 .../services/order_tax_adjustments_fetcher_spec.rb |  2 +-
 spec/support/request/shop_workflow.rb              |  2 +-
 19 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb
index fff22ea718..aca34f988f 100644
--- a/app/controllers/admin/bulk_line_items_controller.rb
+++ b/app/controllers/admin/bulk_line_items_controller.rb
@@ -33,7 +33,7 @@ module Admin
       # and https://www.postgresql.org/docs/current/static/sql-select.html#SQL-FOR-UPDATE-SHARE
       order.with_lock do
         if @line_item.update(line_item_params)
-          order.update_distribution_charge!
+          order.recreate_all_fees!
           render nothing: true, status: :no_content # No Content, does not trigger ng resource auto-update
         else
           render json: { errors: @line_item.errors }, status: :precondition_failed
diff --git a/app/controllers/cart_controller.rb b/app/controllers/cart_controller.rb
index 4be930cc12..18fe32c866 100644
--- a/app/controllers/cart_controller.rb
+++ b/app/controllers/cart_controller.rb
@@ -13,7 +13,7 @@ class CartController < BaseController
 
       cart_service.populate(params.slice(:products, :variants, :quantity), true)
       if cart_service.valid?
-        order.update_distribution_charge!
+        order.recreate_all_fees!
         order.cap_quantity_at_stock!
         order.update!
 
diff --git a/app/controllers/line_items_controller.rb b/app/controllers/line_items_controller.rb
index 28b8d4a324..5aa41927ed 100644
--- a/app/controllers/line_items_controller.rb
+++ b/app/controllers/line_items_controller.rb
@@ -42,7 +42,7 @@ class LineItemsController < BaseController
       item.destroy
       order.update_shipping_fees!
       order.update_payment_fees!
-      order.update_distribution_charge!
+      order.recreate_all_fees!
       order.create_tax_charge!
     end
   end
diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb
index 38da5e812e..de6f24f8b6 100644
--- a/app/controllers/spree/admin/orders_controller.rb
+++ b/app/controllers/spree/admin/orders_controller.rb
@@ -17,8 +17,8 @@ module Spree
       # Currently, adding or deleting line items does not trigger updating the
       # fees! This is a quick fix for that.
       # TODO: update fees when adding/removing line items
-      # instead of the update_distribution_charge method.
-      after_action :update_distribution_charge, only: :update
+      # instead of the recreate_all_fees method.
+      after_action :recreate_all_fees, only: :update
 
       before_action :require_distributor_abn, only: :invoice
 
@@ -103,8 +103,8 @@ module Spree
         render template: "spree/admin/orders/ticket", layout: false
       end
 
-      def update_distribution_charge
-        @order.update_distribution_charge!
+      def recreate_all_fees
+        @order.recreate_all_fees!
       end
 
       private
diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb
index 48e65ddbb1..96373103bb 100644
--- a/app/controllers/spree/orders_controller.rb
+++ b/app/controllers/spree/orders_controller.rb
@@ -77,7 +77,7 @@ module Spree
         discard_empty_line_items
         with_open_adjustments { update_totals_and_taxes }
 
-        @order.update_distribution_charge!
+        @order.recreate_all_fees!
 
         respond_with(@order) do |format|
           format.html do
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 0b4d211c9f..3b54897af2 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -661,7 +661,7 @@ module Spree
       end
     end
 
-    def update_distribution_charge!
+    def recreate_all_fees!
       # `with_lock` acquires an exclusive row lock on order so no other
       # requests can update it until the transaction is commited.
       # See https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/locking/pessimistic.rb#L69
diff --git a/app/services/order_factory.rb b/app/services/order_factory.rb
index bb2a99d018..3d8510a0df 100644
--- a/app/services/order_factory.rb
+++ b/app/services/order_factory.rb
@@ -78,7 +78,7 @@ class OrderFactory
   end
 
   def create_payment
-    @order.update_distribution_charge!
+    @order.recreate_all_fees!
     @order.payments.create(payment_method_id: attrs[:payment_method_id], amount: @order.reload.total)
   end
 
diff --git a/spec/controllers/admin/bulk_line_items_controller_spec.rb b/spec/controllers/admin/bulk_line_items_controller_spec.rb
index d40e0682d2..378c83c5fd 100644
--- a/spec/controllers/admin/bulk_line_items_controller_spec.rb
+++ b/spec/controllers/admin/bulk_line_items_controller_spec.rb
@@ -210,7 +210,7 @@ describe Admin::BulkLineItemsController, type: :controller do
               .to receive(:find).with(line_item1.id.to_s).and_return(line_item1)
 
             expect(line_item1.order).to receive(:reload).with(lock: true)
-            expect(line_item1.order).to receive(:update_distribution_charge!)
+            expect(line_item1.order).to receive(:recreate_all_fees!)
 
             spree_put :update, params
           end
diff --git a/spec/controllers/line_items_controller_spec.rb b/spec/controllers/line_items_controller_spec.rb
index c2816fb8f3..b7c63b543b 100644
--- a/spec/controllers/line_items_controller_spec.rb
+++ b/spec/controllers/line_items_controller_spec.rb
@@ -136,7 +136,7 @@ describe LineItemsController, type: :controller do
         order = create(:completed_order_with_totals, user: user, distributor: distributor, order_cycle: order_cycle, line_items_count: 1)
         order.reload.line_items.first.update(variant_id: variant.id)
         while !order.completed? do break unless order.next! end
-        order.update_distribution_charge!
+        order.recreate_all_fees!
         order
       end
       let(:params) { { format: :json, id: order.line_items.first } }
diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb
index 817e1c3b3a..baccf0cbbd 100644
--- a/spec/controllers/spree/admin/orders_controller_spec.rb
+++ b/spec/controllers/spree/admin/orders_controller_spec.rb
@@ -56,7 +56,7 @@ describe Spree::Admin::OrdersController, type: :controller do
       end
 
       it "updates distribution charges and redirects to order details page" do
-        expect_any_instance_of(Spree::Order).to receive(:update_distribution_charge!)
+        expect_any_instance_of(Spree::Order).to receive(:recreate_all_fees!)
 
         spree_put :update, params
 
@@ -86,7 +86,7 @@ describe Spree::Admin::OrdersController, type: :controller do
 
         context "and no errors" do
           it "updates distribution charges and redirects to customer details page" do
-            expect_any_instance_of(Spree::Order).to receive(:update_distribution_charge!)
+            expect_any_instance_of(Spree::Order).to receive(:recreate_all_fees!)
 
             spree_put :update, params
 
diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb
index 982eb01bf5..ccb3aa2942 100644
--- a/spec/controllers/spree/orders_controller_spec.rb
+++ b/spec/controllers/spree/orders_controller_spec.rb
@@ -278,7 +278,7 @@ describe Spree::OrdersController, type: :controller do
         order = create(:completed_order_with_totals, line_items_count: 1, user: user, distributor: distributor, order_cycle: order_cycle)
         order.reload.line_items.first.update(variant_id: variant.id)
         while !order.completed? do break unless order.next! end
-        order.update_distribution_charge!
+        order.recreate_all_fees!
         order
       end
       let(:params) {
diff --git a/spec/factories/order_factory.rb b/spec/factories/order_factory.rb
index aec931b752..2c6ae21f28 100644
--- a/spec/factories/order_factory.rb
+++ b/spec/factories/order_factory.rb
@@ -90,7 +90,7 @@ FactoryBot.define do
       after(:create) do |order, evaluator|
         create(:payment, state: "checkout", order: order, amount: order.total,
                          payment_method: evaluator.payment_method)
-        order.update_distribution_charge!
+        order.recreate_all_fees!
         order.ship_address = evaluator.ship_address
         while !order.completed? do break unless a = order.next! end
         order.select_shipping_method(evaluator.shipping_method.id)
diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb
index fe5df92355..6123e14b24 100644
--- a/spec/features/admin/reports_spec.rb
+++ b/spec/features/admin/reports_spec.rb
@@ -185,7 +185,7 @@ feature '
 
       2.times { order1.next }
       order1.select_shipping_method shipping_method.id
-      order1.reload.update_distribution_charge!
+      order1.reload.recreate_all_fees!
       order1.finalize!
 
       login_as_admin_and_visit spree.admin_reports_path
diff --git a/spec/features/consumer/shopping/cart_spec.rb b/spec/features/consumer/shopping/cart_spec.rb
index d6c8a7909c..0994bd9dc6 100644
--- a/spec/features/consumer/shopping/cart_spec.rb
+++ b/spec/features/consumer/shopping/cart_spec.rb
@@ -132,7 +132,7 @@ feature "full-page cart", js: true do
 
           cart_service = CartService.new(order)
           cart_service.populate(variants: { product_with_fee.variants.first.id => 3, product_with_tax.variants.first.id => 3 })
-          order.update_distribution_charge!
+          order.recreate_all_fees!
 
           visit main_app.cart_path
         end
diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb
index de6bdb1f98..2ab0313882 100644
--- a/spec/models/spree/adjustment_spec.rb
+++ b/spec/models/spree/adjustment_spec.rb
@@ -307,7 +307,7 @@ module Spree
 
         context "when enterprise fees have a fixed tax_category" do
           before do
-            order.reload.update_distribution_charge!
+            order.reload.recreate_all_fees!
           end
 
           context "when enterprise fees are taxed per-order" do
@@ -326,7 +326,7 @@ module Spree
               before do
                 fee_tax_rate.update_attribute :included_in_price, false
                 order.reload.create_tax_charge! # Updating line_item or order has the same effect
-                order.update_distribution_charge!
+                order.recreate_all_fees!
               end
 
               it "records the tax on TaxRate adjustment on the order" do
@@ -339,7 +339,7 @@ module Spree
               before do
                 enterprise_fee.tax_category = nil
                 enterprise_fee.save!
-                order.update_distribution_charge!
+                order.recreate_all_fees!
               end
 
               it "records no tax as charged" do
@@ -361,7 +361,7 @@ module Spree
               before do
                 fee_tax_rate.update_attribute :included_in_price, false
                 order.reload.create_tax_charge! # Updating line_item or order has the same effect
-                order.update_distribution_charge!
+                order.recreate_all_fees!
               end
 
               it "records the tax on TaxRate adjustment on the order" do
@@ -382,7 +382,7 @@ module Spree
             variant.product.update_attribute(:tax_category_id, product_tax_category.id)
 
             order.create_tax_charge! # Updating line_item or order has the same effect
-            order.update_distribution_charge!
+            order.recreate_all_fees!
           end
 
           context "when enterprise fees are taxed per-order" do
@@ -402,7 +402,7 @@ module Spree
               before do
                 product_tax_rate.update_attribute :included_in_price, false
                 order.reload.create_tax_charge! # Updating line_item or order has the same effect
-                order.reload.update_distribution_charge!
+                order.reload.recreate_all_fees!
               end
 
               it "records the no tax on TaxRate adjustment on the order" do
@@ -432,7 +432,7 @@ module Spree
               before do
                 product_tax_rate.update_attribute :included_in_price, false
                 order.reload.create_tax_charge! # Updating line_item or order has the same effect
-                order.update_distribution_charge!
+                order.recreate_all_fees!
               end
 
               it "records the tax on TaxRate adjustment on the order" do
diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index af5d5d6b57..c18deed302 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -542,7 +542,7 @@ describe Spree::Order do
 
     it "clears all enterprise fee adjustments on the order" do
       expect(EnterpriseFee).to receive(:clear_all_adjustments_on_order).with(subject)
-      subject.update_distribution_charge!
+      subject.recreate_all_fees!
     end
 
     it "creates line item and order fee adjustments via OrderFeesHandler" do
@@ -556,7 +556,7 @@ describe Spree::Order do
 
       allow(subject).to receive(:order_cycle) { nil }
 
-      subject.update_distribution_charge!
+      subject.recreate_all_fees!
     end
 
     it "ensures the correct adjustment(s) are created for order cycles" do
@@ -571,7 +571,7 @@ describe Spree::Order do
       allow_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).to receive(:create_order_adjustments_for)
       allow(subject).to receive(:order_cycle) { order_cycle }
 
-      subject.update_distribution_charge!
+      subject.recreate_all_fees!
     end
 
     it "ensures the correct per-order adjustment(s) are created for order cycles" do
@@ -584,7 +584,7 @@ describe Spree::Order do
 
       allow(subject).to receive(:order_cycle) { order_cycle }
 
-      subject.update_distribution_charge!
+      subject.recreate_all_fees!
     end
   end
 
@@ -820,7 +820,7 @@ describe Spree::Order do
       order.add_variant v1
       order.add_variant v2
 
-      order.update_distribution_charge!
+      order.recreate_all_fees!
     end
 
     it "removes the variant's line item" do
diff --git a/spec/models/tag_rule/discount_order_spec.rb b/spec/models/tag_rule/discount_order_spec.rb
index 4ea426a278..1da5635c5e 100644
--- a/spec/models/tag_rule/discount_order_spec.rb
+++ b/spec/models/tag_rule/discount_order_spec.rb
@@ -56,7 +56,7 @@ describe TagRule::DiscountOrder, type: :model do
     let!(:order) { line_item.order }
 
     before do
-      order.update_distribution_charge!
+      order.recreate_all_fees!
       tag_rule.calculator.update_attribute(:preferred_flat_percent, -10.00)
       tag_rule.context = { subject: order }
     end
diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb
index 4319f17b4d..ed86a1f15e 100644
--- a/spec/services/order_tax_adjustments_fetcher_spec.rb
+++ b/spec/services/order_tax_adjustments_fetcher_spec.rb
@@ -81,7 +81,7 @@ describe OrderTaxAdjustmentsFetcher do
 
     before do
       order.create_tax_charge!
-      order.update_distribution_charge!
+      order.recreate_all_fees!
     end
 
     subject { OrderTaxAdjustmentsFetcher.new(order).totals }
diff --git a/spec/support/request/shop_workflow.rb b/spec/support/request/shop_workflow.rb
index 738d9f7113..d6859c2f9f 100644
--- a/spec/support/request/shop_workflow.rb
+++ b/spec/support/request/shop_workflow.rb
@@ -42,7 +42,7 @@ module ShopWorkflow
     cart_service.populate(variants: { product.variants.first.id => quantity })
 
     # Recalculate fee totals
-    order.update_distribution_charge!
+    order.recreate_all_fees!
   end
 
   # Add an item to the cart

From 3ecdfca9cf6467d591778201eb7bb3437c0a0000 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 01:25:36 +0000
Subject: [PATCH 05/17] Rename fee adjustment clear-all method

---
 app/models/enterprise_fee.rb       | 4 ++--
 app/models/spree/order.rb          | 2 +-
 spec/models/enterprise_fee_spec.rb | 6 +++---
 spec/models/spree/order_spec.rb    | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb
index f1161bb8ab..b7a3eeb6e2 100644
--- a/app/models/enterprise_fee.rb
+++ b/app/models/enterprise_fee.rb
@@ -40,8 +40,8 @@ class EnterpriseFee < ActiveRecord::Base
     joins(:calculator).where('spree_calculators.type IN (?)', PER_ORDER_CALCULATORS)
   }
 
-  def self.clear_all_adjustments_on_order(order)
-    order.adjustments.where(originator_type: 'EnterpriseFee').destroy_all
+  def self.clear_all_adjustments(order)
+    order.adjustments.enterprise_fee.destroy_all
   end
 
   private
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 3b54897af2..4dd471b5c7 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -667,7 +667,7 @@ module Spree
       # See https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/locking/pessimistic.rb#L69
       # and https://www.postgresql.org/docs/current/static/sql-select.html#SQL-FOR-UPDATE-SHARE
       with_lock do
-        EnterpriseFee.clear_all_adjustments_on_order self
+        EnterpriseFee.clear_all_adjustments self
 
         fee_handler = OrderFeesHandler.new(self)
         fee_handler.create_line_item_fees!
diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb
index b5065ecee7..9cf76958d5 100644
--- a/spec/models/enterprise_fee_spec.rb
+++ b/spec/models/enterprise_fee_spec.rb
@@ -110,7 +110,7 @@ describe EnterpriseFee do
       order_cycle.exchanges[0].enterprise_fees[0].create_adjustment('foo4', line_item2.order, line_item2, true)
 
       expect do
-        EnterpriseFee.clear_all_adjustments_on_order order
+        EnterpriseFee.clear_all_adjustments order
       end.to change(order.adjustments, :count).by(-4)
     end
 
@@ -121,7 +121,7 @@ describe EnterpriseFee do
       enterprise_fee_aplicator.create_order_adjustment(order)
 
       expect do
-        EnterpriseFee.clear_all_adjustments_on_order order
+        EnterpriseFee.clear_all_adjustments order
       end.to change(order.adjustments, :count).by(-1)
     end
 
@@ -135,7 +135,7 @@ describe EnterpriseFee do
                                  label: 'hello' })
 
       expect do
-        EnterpriseFee.clear_all_adjustments_on_order order
+        EnterpriseFee.clear_all_adjustments order
       end.to change(order.adjustments, :count).by(0)
     end
   end
diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index c18deed302..0a495e6bf6 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -541,7 +541,7 @@ describe Spree::Order do
     end
 
     it "clears all enterprise fee adjustments on the order" do
-      expect(EnterpriseFee).to receive(:clear_all_adjustments_on_order).with(subject)
+      expect(EnterpriseFee).to receive(:clear_all_adjustments).with(subject)
       subject.recreate_all_fees!
     end
 
@@ -552,7 +552,7 @@ describe Spree::Order do
     end
 
     it "skips order cycle per-order adjustments for orders that don't have an order cycle" do
-      allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
+      allow(EnterpriseFee).to receive(:clear_all_adjustments)
 
       allow(subject).to receive(:order_cycle) { nil }
 
@@ -560,7 +560,7 @@ describe Spree::Order do
     end
 
     it "ensures the correct adjustment(s) are created for order cycles" do
-      allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
+      allow(EnterpriseFee).to receive(:clear_all_adjustments)
       line_item = create(:line_item, order: subject)
       allow(fee_handler).to receive(:provided_by_order_cycle?) { true }
 
@@ -575,7 +575,7 @@ describe Spree::Order do
     end
 
     it "ensures the correct per-order adjustment(s) are created for order cycles" do
-      allow(EnterpriseFee).to receive(:clear_all_adjustments_on_order)
+      allow(EnterpriseFee).to receive(:clear_all_adjustments)
 
       order_cycle = double(:order_cycle)
       expect_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).

From dffa4d4f39c59299c0881896c8156748ac558d86 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 01:37:59 +0000
Subject: [PATCH 06/17] Update order methods delegation

---
 app/models/spree/order.rb | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 4dd471b5c7..2022ecb37a 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -67,6 +67,8 @@ module Spree
     accepts_nested_attributes_for :shipments
 
     delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher
+    delegate :update_totals, to: :updater
+    delegate :create_line_item_fees!, :create_order_fees!, to: :fee_handler
 
     # Needs to happen before save_permalink is called
     before_validation :set_currency
@@ -267,8 +269,6 @@ module Spree
       updater.update
     end
 
-    delegate :update_totals, to: :updater
-
     def clone_billing_address
       if bill_address && ship_address.nil?
         self.ship_address = bill_address.clone
@@ -669,9 +669,8 @@ module Spree
       with_lock do
         EnterpriseFee.clear_all_adjustments self
 
-        fee_handler = OrderFeesHandler.new(self)
-        fee_handler.create_line_item_fees!
-        fee_handler.create_order_fees!
+        create_line_item_fees!
+        create_order_fees!
       end
     end
 
@@ -745,6 +744,10 @@ module Spree
 
     private
 
+    def fee_handler
+      @fee_handler ||= OrderFeesHandler.new(self)
+    end
+
     def process_each_payment
       raise Core::GatewayError, Spree.t(:no_pending_payments) if pending_payments.empty?
 

From b2b6d3ab876a1d750a0ef6e2838fef3521e02557 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Thu, 28 Jan 2021 23:11:54 +0000
Subject: [PATCH 07/17] Relocate specs for #provided_by_order_cycle? method
 extracted from Order class to service

---
 spec/models/spree/order_spec.rb          | 28 ------------------------
 spec/services/order_fees_handler_spec.rb | 20 +++++++++++++++++
 2 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index 0a495e6bf6..c886d5677d 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -588,34 +588,6 @@ describe Spree::Order do
     end
   end
 
-  describe "looking up whether a line item can be provided by an order cycle" do
-    it "returns true when the variant is provided" do
-      v = double(:variant)
-      line_item = double(:line_item, variant: v)
-      order_cycle = double(:order_cycle, variants: [v])
-      allow(subject).to receive(:order_cycle) { order_cycle }
-
-      expect(subject.send(:provided_by_order_cycle?, line_item)).to be true
-    end
-
-    it "returns false otherwise" do
-      v = double(:variant)
-      line_item = double(:line_item, variant: v)
-      order_cycle = double(:order_cycle, variants: [])
-      allow(subject).to receive(:order_cycle) { order_cycle }
-
-      expect(subject.send(:provided_by_order_cycle?, line_item)).to be false
-    end
-
-    it "returns false when there is no order cycle" do
-      v = double(:variant)
-      line_item = double(:line_item, variant: v)
-      allow(subject).to receive(:order_cycle) { nil }
-
-      expect(subject.send(:provided_by_order_cycle?, line_item)).to be false
-    end
-  end
-
   describe "getting the admin and handling charge" do
     let(:o) { create(:order) }
     let(:li) { create(:line_item, order: o) }
diff --git a/spec/services/order_fees_handler_spec.rb b/spec/services/order_fees_handler_spec.rb
index e77f0bcc29..f0358a49c9 100644
--- a/spec/services/order_fees_handler_spec.rb
+++ b/spec/services/order_fees_handler_spec.rb
@@ -38,4 +38,24 @@ describe OrderFeesHandler do
       service.create_order_fees!
     end
   end
+
+  context "checking if a line item can be provided by the order cycle" do
+    it "returns true when the variant is provided" do
+      allow(order_cycle).to receive(:variants) { [line_item.variant] }
+
+      expect(service.__send__(:provided_by_order_cycle?, line_item)).to be true
+    end
+
+    it "returns false otherwise" do
+      allow(order_cycle).to receive(:variants) { [] }
+
+      expect(service.__send__(:provided_by_order_cycle?, line_item)).to be false
+    end
+
+    it "returns false when there is no order cycle" do
+      allow(order).to receive(:order_cycle) { nil }
+
+      expect(service.__send__(:provided_by_order_cycle?, line_item)).to be false
+    end
+  end
 end

From 58c7c906248b4bb24d2f05d6af4f9c13157a1be8 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 29 Jan 2021 17:58:49 +0000
Subject: [PATCH 08/17] Refactor methods for updating closed adjustments

---
 app/models/spree/adjustment.rb | 4 ++--
 app/models/spree/order.rb      | 5 +----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb
index 942e7fc149..e171202e9f 100644
--- a/app/models/spree/adjustment.rb
+++ b/app/models/spree/adjustment.rb
@@ -106,8 +106,8 @@ module Spree
     # more than on line items at once via accepted_nested_attributes the order
     # object on the association would be in a old state and therefore the
     # adjustment calculations would not performed on proper values
-    def update!(calculable = nil)
-      return if immutable?
+    def update!(calculable = nil, force: false)
+      return if immutable? && !force
 
       # Fix for Spree issue #3381
       # If we attempt to call 'source' before the reload, then source is currently
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 2022ecb37a..f9f53edc08 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -857,11 +857,8 @@ module Spree
     def update_adjustment!(adjustment)
       return if adjustment.finalized?
 
-      state = adjustment.state
-      adjustment.state = 'open'
-      adjustment.update!
+      adjustment.update!(force: true)
       update!
-      adjustment.state = state
     end
 
     # object_params sets the payment amount to the order total, but it does this

From ce5f9a9a94074ebced538f160746c78b17a21b37 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 29 Jan 2021 18:01:40 +0000
Subject: [PATCH 09/17] Update existing closed order fees when deleting line
 items on completed orders in LineItemsController#delete

Whatever fee adjustments there are on other line items should be left alone (not recreated), and whatever fee adjustments are already on the order should just be updated.
---
 app/controllers/line_items_controller.rb       |  3 ++-
 app/models/spree/order.rb                      |  2 +-
 app/services/order_fees_handler.rb             |  6 ++++++
 spec/controllers/line_items_controller_spec.rb | 17 ++++++++++-------
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/app/controllers/line_items_controller.rb b/app/controllers/line_items_controller.rb
index 5aa41927ed..b4d8624134 100644
--- a/app/controllers/line_items_controller.rb
+++ b/app/controllers/line_items_controller.rb
@@ -42,7 +42,8 @@ class LineItemsController < BaseController
       item.destroy
       order.update_shipping_fees!
       order.update_payment_fees!
-      order.recreate_all_fees!
+      order.update_order_fees!
+      order.update!
       order.create_tax_charge!
     end
   end
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index f9f53edc08..044220759d 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -68,7 +68,7 @@ module Spree
 
     delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher
     delegate :update_totals, to: :updater
-    delegate :create_line_item_fees!, :create_order_fees!, to: :fee_handler
+    delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, to: :fee_handler
 
     # Needs to happen before save_permalink is called
     before_validation :set_currency
diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb
index f3514d4465..abd07bdeae 100644
--- a/app/services/order_fees_handler.rb
+++ b/app/services/order_fees_handler.rb
@@ -23,6 +23,12 @@ class OrderFeesHandler
     calculator.create_order_adjustments_for order
   end
 
+  def update_order_fees!
+    order.adjustments.enterprise_fee.where(source_type: 'Spree::Order').each do |fee|
+      fee.update!(order, force: true)
+    end
+  end
+
   private
 
   def calculator
diff --git a/spec/controllers/line_items_controller_spec.rb b/spec/controllers/line_items_controller_spec.rb
index b7c63b543b..473b261745 100644
--- a/spec/controllers/line_items_controller_spec.rb
+++ b/spec/controllers/line_items_controller_spec.rb
@@ -127,14 +127,17 @@ describe LineItemsController, type: :controller do
 
     context "on a completed order with enterprise fees" do
       let(:user) { create(:user) }
-      let(:variant) { create(:variant) }
+      let(:variant1) { create(:variant) }
+      let(:variant2) { create(:variant) }
       let(:distributor) { create(:distributor_enterprise, allow_order_changes: true) }
       let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) }
-      let(:enterprise_fee) { create(:enterprise_fee, calculator: build(:calculator_per_item) ) }
-      let!(:exchange) { create(:exchange, incoming: true, sender: variant.product.supplier, receiver: order_cycle.coordinator, variants: [variant], enterprise_fees: [enterprise_fee]) }
+      let(:calculator) { Calculator::PriceSack.new(preferred_minimal_amount: 15, preferred_normal_amount: 22, preferred_discount_amount: 11) }
+      let(:enterprise_fee) { create(:enterprise_fee, calculator: calculator) }
+      let!(:exchange) { create(:exchange, incoming: true, sender: variant1.product.supplier, receiver: order_cycle.coordinator, variants: [variant1, variant2], enterprise_fees: [enterprise_fee]) }
       let!(:order) do
-        order = create(:completed_order_with_totals, user: user, distributor: distributor, order_cycle: order_cycle, line_items_count: 1)
-        order.reload.line_items.first.update(variant_id: variant.id)
+        order = create(:completed_order_with_totals, user: user, distributor: distributor, order_cycle: order_cycle, line_items_count: 2)
+        order.reload.line_items.first.update(variant_id: variant1.id)
+        order.line_items.last.update(variant_id: variant2.id)
         while !order.completed? do break unless order.next! end
         order.recreate_all_fees!
         order
@@ -142,13 +145,13 @@ describe LineItemsController, type: :controller do
       let(:params) { { format: :json, id: order.line_items.first } }
 
       it "updates the fees" do
-        expect(order.reload.adjustment_total).to eq enterprise_fee.calculator.preferred_amount
+        expect(order.reload.adjustment_total).to eq calculator.preferred_discount_amount
 
         allow(controller).to receive_messages spree_current_user: user
         delete :destroy, params
         expect(response.status).to eq 204
 
-        expect(order.reload.adjustment_total).to eq 0
+        expect(order.reload.adjustment_total).to eq calculator.preferred_normal_amount
       end
     end
   end

From 793baca44f5191272b54efec13cd9cd5007d49ab Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 29 Jan 2021 19:20:31 +0000
Subject: [PATCH 10/17] Update fees on single line item and then order fees in
 LineItemsController#delete

Fees on other line items are left alone (not recreated), and whatever fees on the order are updated.
---
 app/controllers/admin/bulk_line_items_controller.rb       | 4 +++-
 app/models/spree/order.rb                                 | 3 ++-
 app/services/order_fees_handler.rb                        | 6 ++++++
 spec/controllers/admin/bulk_line_items_controller_spec.rb | 4 +++-
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb
index aca34f988f..f8667f69ec 100644
--- a/app/controllers/admin/bulk_line_items_controller.rb
+++ b/app/controllers/admin/bulk_line_items_controller.rb
@@ -33,7 +33,9 @@ module Admin
       # and https://www.postgresql.org/docs/current/static/sql-select.html#SQL-FOR-UPDATE-SHARE
       order.with_lock do
         if @line_item.update(line_item_params)
-          order.recreate_all_fees!
+          order.update_line_item_fees! @line_item
+          order.update_order_fees!
+          order.update!
           render nothing: true, status: :no_content # No Content, does not trigger ng resource auto-update
         else
           render json: { errors: @line_item.errors }, status: :precondition_failed
diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index 044220759d..c83023801a 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -68,7 +68,8 @@ module Spree
 
     delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher
     delegate :update_totals, to: :updater
-    delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, to: :fee_handler
+    delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!,
+             :update_line_item_fees!, to: :fee_handler
 
     # Needs to happen before save_permalink is called
     before_validation :set_currency
diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb
index abd07bdeae..ab176c4eee 100644
--- a/app/services/order_fees_handler.rb
+++ b/app/services/order_fees_handler.rb
@@ -23,6 +23,12 @@ class OrderFeesHandler
     calculator.create_order_adjustments_for order
   end
 
+  def update_line_item_fees!(line_item)
+    line_item.adjustments.enterprise_fee.each do |fee|
+      fee.update!(line_item, force: true)
+    end
+  end
+
   def update_order_fees!
     order.adjustments.enterprise_fee.where(source_type: 'Spree::Order').each do |fee|
       fee.update!(order, force: true)
diff --git a/spec/controllers/admin/bulk_line_items_controller_spec.rb b/spec/controllers/admin/bulk_line_items_controller_spec.rb
index 378c83c5fd..4d94591a0e 100644
--- a/spec/controllers/admin/bulk_line_items_controller_spec.rb
+++ b/spec/controllers/admin/bulk_line_items_controller_spec.rb
@@ -210,7 +210,9 @@ describe Admin::BulkLineItemsController, type: :controller do
               .to receive(:find).with(line_item1.id.to_s).and_return(line_item1)
 
             expect(line_item1.order).to receive(:reload).with(lock: true)
-            expect(line_item1.order).to receive(:recreate_all_fees!)
+            expect(line_item1.order).to receive(:update_line_item_fees!)
+            expect(line_item1.order).to receive(:update_order_fees!)
+            expect(line_item1.order).to receive(:update!).twice
 
             spree_put :update, params
           end

From 8466ab5675f1b41e939fa46eca84c34a5bcba72a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 29 Jan 2021 21:42:55 +0000
Subject: [PATCH 11/17] Extract more OFN fee-handling code from Spree::Order
 class

---
 app/models/spree/order.rb          | 15 +--------------
 app/services/order_fees_handler.rb | 13 +++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb
index c83023801a..93355035a5 100644
--- a/app/models/spree/order.rb
+++ b/app/models/spree/order.rb
@@ -69,7 +69,7 @@ module Spree
     delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher
     delegate :update_totals, to: :updater
     delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!,
-             :update_line_item_fees!, to: :fee_handler
+             :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler
 
     # Needs to happen before save_permalink is called
     before_validation :set_currency
@@ -662,19 +662,6 @@ module Spree
       end
     end
 
-    def recreate_all_fees!
-      # `with_lock` acquires an exclusive row lock on order so no other
-      # requests can update it until the transaction is commited.
-      # See https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/locking/pessimistic.rb#L69
-      # and https://www.postgresql.org/docs/current/static/sql-select.html#SQL-FOR-UPDATE-SHARE
-      with_lock do
-        EnterpriseFee.clear_all_adjustments self
-
-        create_line_item_fees!
-        create_order_fees!
-      end
-    end
-
     def set_order_cycle!(order_cycle)
       return if self.order_cycle == order_cycle
 
diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb
index ab176c4eee..3fd09bdb39 100644
--- a/app/services/order_fees_handler.rb
+++ b/app/services/order_fees_handler.rb
@@ -9,6 +9,19 @@ class OrderFeesHandler
     @order_cycle = order.order_cycle
   end
 
+  def recreate_all_fees!
+    # `with_lock` acquires an exclusive row lock on order so no other
+    # requests can update it until the transaction is commited.
+    # See https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/locking/pessimistic.rb#L69
+    # and https://www.postgresql.org/docs/current/static/sql-select.html#SQL-FOR-UPDATE-SHARE
+    order.with_lock do
+      EnterpriseFee.clear_all_adjustments order
+
+      create_line_item_fees!
+      create_order_fees!
+    end
+  end
+
   def create_line_item_fees!
     order.line_items.includes(variant: :product).each do |line_item|
       if provided_by_order_cycle? line_item

From 9303d61db11a58b7669063d1fee45fd771ac7de3 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 30 Jan 2021 17:49:10 +0000
Subject: [PATCH 12/17] Update specs

---
 spec/controllers/spree/orders_controller_spec.rb | 12 ------------
 spec/factories/order_factory.rb                  |  1 +
 spec/models/spree/adjustment_spec.rb             | 14 ++++++++++++++
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb
index ccb3aa2942..cf8ba1cfef 100644
--- a/spec/controllers/spree/orders_controller_spec.rb
+++ b/spec/controllers/spree/orders_controller_spec.rb
@@ -253,18 +253,6 @@ describe Spree::OrdersController, type: :controller do
         expect(order.adjustment_total).to eq((item_num - 1) * (shipping_fee + payment_fee))
         expect(order.shipment.adjustment.included_tax).to eq 0.6
       end
-
-      it "keeps the adjustments' previous state" do
-        spree_post :update,
-                   order: { line_items_attributes: {
-                     "0" => { id: line_item1.id, quantity: 1 },
-                     "1" => { id: line_item2.id, quantity: 0 }
-                   } }
-
-        # The second adjustment (shipping adjustment) is open before the update
-        #   so, restoring its state leaves it open.
-        expect(order.adjustments.map(&:state)).to eq(['closed', 'open'])
-      end
     end
 
     context "with enterprise fees" do
diff --git a/spec/factories/order_factory.rb b/spec/factories/order_factory.rb
index 2c6ae21f28..4f24d9b767 100644
--- a/spec/factories/order_factory.rb
+++ b/spec/factories/order_factory.rb
@@ -94,6 +94,7 @@ FactoryBot.define do
         order.ship_address = evaluator.ship_address
         while !order.completed? do break unless a = order.next! end
         order.select_shipping_method(evaluator.shipping_method.id)
+        order.save
       end
     end
   end
diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb
index 2ab0313882..ef2074b03a 100644
--- a/spec/models/spree/adjustment_spec.rb
+++ b/spec/models/spree/adjustment_spec.rb
@@ -45,6 +45,20 @@ module Spree
           expect(originator).to receive(:update_adjustment)
           adjustment.update!
         end
+
+        context "using the :force argument" do
+          it "should update adjustments without changing their state" do
+            expect(originator).to receive(:update_adjustment)
+            adjustment.update!(force: true)
+            expect(adjustment.state).to eq "open"
+          end
+
+          it "should update closed adjustments" do
+            adjustment.close
+            expect(originator).to receive(:update_adjustment)
+            adjustment.update!(force: true)
+          end
+        end
       end
 
       it "should do nothing when originator is nil" do

From 5c5d687c9b350cfb91618017d299c50d98f6b50b Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 30 Jan 2021 18:24:35 +0000
Subject: [PATCH 13/17] Remove hacks for working around closed adjustments

The enterprise fees are recreated and the shipping and payment fees are updated. The rest of the deleted code is not necessary (eg #with_open_adjustments). Everything else that needs to happen here is already done automatically (eg updating order totals).
---
 app/controllers/spree/orders_controller.rb | 35 ++++++----------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb
index 96373103bb..151eb0ec6e 100644
--- a/app/controllers/spree/orders_controller.rb
+++ b/app/controllers/spree/orders_controller.rb
@@ -73,11 +73,18 @@ module Spree
         redirect_to(main_app.root_path) && return
       end
 
+      # This action is called either from the cart page when the order is not yet complete, or from
+      # the edit order page (frontoffice) if the hub allows users to update completed orders.
+      # This line item updating logic should probably be handled through CartService#populate.
       if @order.update(order_params)
         discard_empty_line_items
-        with_open_adjustments { update_totals_and_taxes }
 
-        @order.recreate_all_fees!
+        @order.recreate_all_fees! # Enterprise fees on line items and on the order itself
+
+        if @order.complete?
+          @order.update_shipping_fees!
+          @order.update_payment_fees!
+        end
 
         respond_with(@order) do |format|
           format.html do
@@ -157,30 +164,6 @@ module Spree
 
     private
 
-    # Updates the various denormalized total attributes of the order and
-    # recalculates the shipment taxes
-    def update_totals_and_taxes
-      @order.updater.update_totals
-      @order.shipment&.ensure_correct_adjustment
-    end
-
-    # Sets the adjustments to open to perform the block's action and restores
-    # their state to whatever the they had. Note that it does not change any new
-    # adjustments that might get created in the yielded block.
-    def with_open_adjustments
-      previous_states = @order.adjustments.each_with_object({}) do |adjustment, hash|
-        hash[adjustment.id] = adjustment.state
-      end
-      @order.adjustments.each { |adjustment| adjustment.fire_events(:open) }
-
-      yield
-
-      @order.adjustments.each do |adjustment|
-        previous_state = previous_states[adjustment.id]
-        adjustment.update_attribute(:state, previous_state) if previous_state
-      end
-    end
-
     def discard_empty_line_items
       @order.line_items = @order.line_items.select { |li| li.quantity > 0 }
     end

From ba81bd8395934dd65825b69e0bd1961258765248 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Sat, 30 Jan 2021 21:41:13 +0000
Subject: [PATCH 14/17] Remove after_action callback in Admin:OrdersController

---
 app/controllers/spree/admin/orders_controller.rb | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb
index de6f24f8b6..76daa85014 100644
--- a/app/controllers/spree/admin/orders_controller.rb
+++ b/app/controllers/spree/admin/orders_controller.rb
@@ -12,14 +12,6 @@ module Spree
 
       # Ensure that the distributor is set for an order when
       before_action :ensure_distribution, only: :new
-
-      # After updating an order, the fees should be updated as well
-      # Currently, adding or deleting line items does not trigger updating the
-      # fees! This is a quick fix for that.
-      # TODO: update fees when adding/removing line items
-      # instead of the recreate_all_fees method.
-      after_action :recreate_all_fees, only: :update
-
       before_action :require_distributor_abn, only: :invoice
 
       respond_to :html, :json
@@ -43,6 +35,9 @@ module Spree
       end
 
       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?
             @order.errors.add(:line_items, Spree.t('errors.messages.blank'))
@@ -51,7 +46,6 @@ module Spree
                              flash: { error: @order.errors.full_messages.join(', ') })
         end
 
-        @order.update!
         if @order.complete?
           redirect_to spree.edit_admin_order_path(@order)
         else
@@ -103,10 +97,6 @@ module Spree
         render template: "spree/admin/orders/ticket", layout: false
       end
 
-      def recreate_all_fees
-        @order.recreate_all_fees!
-      end
-
       private
 
       def order_params

From d4750b9f26e1a9d94c0f27ec6cf5abfb990ecde4 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Wed, 3 Feb 2021 14:00:14 +0000
Subject: [PATCH 15/17] Improve clarity of orders controller test

---
 spec/controllers/spree/orders_controller_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb
index cf8ba1cfef..55cf94470f 100644
--- a/spec/controllers/spree/orders_controller_spec.rb
+++ b/spec/controllers/spree/orders_controller_spec.rb
@@ -242,15 +242,15 @@ describe Spree::OrdersController, type: :controller do
         allow(subject).to receive(:order_to_update) { order }
       end
 
-      it "updates the fees" do
+      it "updates the shipping and payment fees" do
         spree_post :update,
                    order: { line_items_attributes: {
                      "0" => { id: line_item1.id, quantity: 1 },
                      "1" => { id: line_item2.id, quantity: 0 }
                    } }
 
-        expect(order.line_items.count).to eq 1
-        expect(order.adjustment_total).to eq((item_num - 1) * (shipping_fee + payment_fee))
+        expect(order.reload.line_items.count).to eq 1
+        expect(order.adjustment_total).to eq(1 * (shipping_fee + payment_fee))
         expect(order.shipment.adjustment.included_tax).to eq 0.6
       end
     end

From cc55e9eeda2b2aea168853e4ce0e91928f945428 Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 19 Feb 2021 00:02:16 +0000
Subject: [PATCH 16/17] Improve coverage in orders_controller_spec

---
 .../spree/orders_controller_spec.rb           | 37 ++++++++++++++++---
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb
index 55cf94470f..2f77809ff1 100644
--- a/spec/controllers/spree/orders_controller_spec.rb
+++ b/spec/controllers/spree/orders_controller_spec.rb
@@ -257,16 +257,19 @@ describe Spree::OrdersController, type: :controller do
 
     context "with enterprise fees" do
       let(:user) { create(:user) }
-      let(:variant) { create(:variant) }
+      let(:variant1) { create(:variant) }
+      let(:variant2) { create(:variant) }
       let(:distributor) { create(:distributor_enterprise, allow_order_changes: true) }
       let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) }
       let(:enterprise_fee) { create(:enterprise_fee, calculator: build(:calculator_per_item) ) }
-      let!(:exchange) { create(:exchange, incoming: true, sender: variant.product.supplier, receiver: order_cycle.coordinator, variants: [variant], enterprise_fees: [enterprise_fee]) }
+      let!(:exchange) { create(:exchange, incoming: true, sender: variant1.product.supplier, receiver: order_cycle.coordinator, variants: [variant1, variant2], enterprise_fees: [enterprise_fee]) }
       let!(:order) do
-        order = create(:completed_order_with_totals, line_items_count: 1, user: user, distributor: distributor, order_cycle: order_cycle)
-        order.reload.line_items.first.update(variant_id: variant.id)
+        order = create(:completed_order_with_totals, line_items_count: 2, user: user, distributor: distributor, order_cycle: order_cycle)
+        order.reload.line_items.first.update(variant_id: variant1.id)
+        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) {
@@ -281,12 +284,34 @@ describe Spree::OrdersController, type: :controller do
       end
 
       it "updates the fees" do
-        expect(order.reload.adjustment_total).to eq enterprise_fee.calculator.preferred_amount
+        expect(order.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 2)
+        expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 2
 
         allow(controller).to receive_messages spree_current_user: user
         spree_post :update, params
 
-        expect(order.reload.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 2
+        expect(order.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 3)
+        expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 3
+      end
+
+      context "when a line item is removed" do
+        let(:params) {
+          { order: { line_items_attributes: {
+            "0" => { id: order.line_items.first.id, quantity: 0 },
+            "1" => { id: order.line_items.last.id, quantity: 1 }
+          } } }
+        }
+
+        it "updates the fees" do
+          expect(order.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 2)
+          expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 2
+
+          allow(controller).to receive_messages spree_current_user: user
+          spree_post :update, params
+
+          expect(order.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 1)
+          expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 1
+        end
       end
     end
   end

From f858fe3c685352f3f53bdf16e203114ebbda780c Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 19 Feb 2021 10:36:05 +0000
Subject: [PATCH 17/17] Improve coverage in admin_orders_controller_spec

---
 .../spree/admin/orders_controller_spec.rb     | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb
index baccf0cbbd..f9cae535e5 100644
--- a/spec/controllers/spree/admin/orders_controller_spec.rb
+++ b/spec/controllers/spree/admin/orders_controller_spec.rb
@@ -62,6 +62,42 @@ describe Spree::Admin::OrdersController, type: :controller do
 
         expect(response).to redirect_to spree.edit_admin_order_path(order)
       end
+
+      context "recalculating enterprise fees" do
+        let(:user) { create(:admin_user) }
+        let(:variant1) { create(:variant) }
+        let(:variant2) { create(:variant) }
+        let(:distributor) { create(:distributor_enterprise, allow_order_changes: true) }
+        let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) }
+        let(:enterprise_fee) { create(:enterprise_fee, calculator: build(:calculator_per_item) ) }
+        let!(:exchange) { create(:exchange, incoming: true, sender: variant1.product.supplier, receiver: order_cycle.coordinator, variants: [variant1, variant2], enterprise_fees: [enterprise_fee]) }
+        let!(:order) do
+          order = create(:completed_order_with_totals, line_items_count: 2, distributor: distributor, order_cycle: order_cycle)
+          order.reload.line_items.first.update(variant_id: variant1.id)
+          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
+
+        before do
+          allow(controller).to receive(:spree_current_user) { user }
+          allow(controller).to receive(:order_to_update) { order }
+        end
+
+        it "recalculates fees if the orders contents have changed" do
+          expect(order.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 2)
+          expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 2
+
+          order.contents.add(order.line_items.first.variant, 1)
+
+          spree_put :update, { id: order.number }
+
+          expect(order.reload.total).to eq order.item_total + (enterprise_fee.calculator.preferred_amount * 3)
+          expect(order.adjustment_total).to eq enterprise_fee.calculator.preferred_amount * 3
+        end
+      end
     end
 
     context "incomplete order" do