Revert "Update line items enterprise fee instead of deleting and recr…

…eating "

app/models/enterprise_fee.rb

@@ -42,8 +42,8 @@ class EnterpriseFee < ApplicationRecord
     joins(:calculator).where(spree_calculators: { type: PER_ORDER_CALCULATORS })
   }
 
-  def self.clear_order_adjustments(order)
-    order.all_adjustments.enterprise_fee.where.not(adjustable_type: "Spree::LineItem").destroy_all
+  def self.clear_all_adjustments(order)
+    order.all_adjustments.enterprise_fee.destroy_all
   end
 
   private

app/models/spree/order.rb

@@ -86,7 +86,7 @@ def states
 
     delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher
     delegate :update_totals, :update_totals_and_states, to: :updater
-    delegate :create_order_fees!, :update_order_fees!,
+    delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!,
              :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler
 
     validates :customer, presence: true, if: :require_customer?

app/services/orders/handle_fees_service.rb

@@ -16,30 +16,21 @@ def recreate_all_fees!
       # 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_order_adjustments order
+        EnterpriseFee.clear_all_adjustments order
 
-        # To prevent issue with fee being removed when a product is not linked to the order cycle
-        # anymore, we now create or update line item fees.
-        # Previously fees were deleted and recreated, like we still do for order fees.
-        create_or_update_line_item_fees!
+        create_line_item_fees!
         create_order_fees!
       end
 
       tax_enterprise_fees! unless order.before_payment_state?
       order.update_order!
     end
 
-    def create_or_update_line_item_fees!
-      order.line_items.includes(:variant).each do |line_item|
-        # No fee associated with the line item so we just create them
-        if line_item.enterprise_fee_adjustments.blank?
-          create_line_item_fees!(line_item)
-          next
+    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
-
-        create_or_update_line_item_fee!(line_item)
-
-        delete_removed_fees!(line_item)
       end
     end
 
@@ -75,34 +66,5 @@ 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 create_line_item_fees!(line_item)
-      return unless provided_by_order_cycle? line_item
-
-      calculator.create_line_item_adjustments_for(line_item)
-    end
-
-    def create_or_update_line_item_fee!(line_item)
-      fee_applicators(line_item.variant).each do |fee_applicator|
-        fee_adjustment = line_item.adjustments.find_by(originator: fee_applicator.enterprise_fee)
-
-        if fee_adjustment
-          fee_adjustment.update_adjustment!(line_item, force: true)
-        elsif provided_by_order_cycle? line_item
-          fee_applicator.create_line_item_adjustment(line_item)
-        end
-      end
-    end
-
-    def delete_removed_fees!(line_item)
-      order_cycle_fees = fee_applicators(line_item.variant).map(&:enterprise_fee)
-      removed_fees = line_item.enterprise_fee_adjustments.where.not(originator: order_cycle_fees)
-
-      removed_fees.each(&:destroy)
-    end
-
-    def fee_applicators(variant)
-      calculator.order_cycle_per_item_enterprise_fee_applicators_for(variant)
-    end
   end
 end

lib/open_food_network/enterprise_fee_calculator.rb

@@ -67,12 +67,13 @@ def per_item_enterprise_fee_applicators_for(variant)
 
       @order_cycle.exchanges_carrying(variant, @distributor).each do |exchange|
         exchange.enterprise_fees.per_item.each do |enterprise_fee|
-          fees << EnterpriseFeeApplicator.new(enterprise_fee, variant, exchange.role)
+          fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant,
+                                                               exchange.role)
         end
       end
 
       @order_cycle.coordinator_fees.per_item.each do |enterprise_fee|
-        fees << EnterpriseFeeApplicator.new(enterprise_fee, variant, 'coordinator')
+        fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, variant, 'coordinator')
       end
 
       fees
@@ -85,30 +86,12 @@ def per_order_enterprise_fee_applicators_for(order)
 
       @order_cycle.exchanges_supplying(order).each do |exchange|
         exchange.enterprise_fees.per_order.each do |enterprise_fee|
-          fees << EnterpriseFeeApplicator.new(enterprise_fee, nil, exchange.role)
+          fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, nil, exchange.role)
         end
       end
 
       @order_cycle.coordinator_fees.per_order.each do |enterprise_fee|
-        fees << EnterpriseFeeApplicator.new(enterprise_fee, nil, 'coordinator')
-      end
-
-      fees
-    end
-
-    def order_cycle_per_item_enterprise_fee_applicators_for(variant)
-      fees = []
-
-      return fees unless @order_cycle && @distributor
-
-      @order_cycle.exchanges.supplying_to(@distributor).each do |exchange|
-        exchange.enterprise_fees.per_item.each do |enterprise_fee|
-          fees << EnterpriseFeeApplicator.new(enterprise_fee, variant, exchange.role)
-        end
-      end
-
-      @order_cycle.coordinator_fees.per_item.each do |enterprise_fee|
-        fees << EnterpriseFeeApplicator.new(enterprise_fee, variant, 'coordinator')
+        fees << OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, nil, 'coordinator')
       end
 
       fees

spec/models/enterprise_fee_spec.rb

@@ -142,51 +142,47 @@
     end
   end
 
-  describe ".clear_order_adjustments" do
-    let(:order_cycle) { create(:order_cycle) }
-    let(:order) { create(:order, order_cycle:) }
+  describe "clearing all enterprise fee adjustments on an order" do
+    it "clears adjustments from many fees and on all line items" do
+      order_cycle = create(:order_cycle)
+      order = create(:order, order_cycle:)
+      line_item1 = create(:line_item, order:, variant: order_cycle.variants.first)
+      line_item2 = create(:line_item, order:, variant: order_cycle.variants.second)
+
+      order_cycle.coordinator_fees[0].create_adjustment('foo1', line_item1.order, true)
+      order_cycle.coordinator_fees[0].create_adjustment('foo2', line_item2.order, true)
+      order_cycle.exchanges[0].enterprise_fees[0].create_adjustment('foo3', line_item1, true)
+      order_cycle.exchanges[0].enterprise_fees[0].create_adjustment('foo4', line_item2, true)
+
+      expect do
+        EnterpriseFee.clear_all_adjustments order
+      end.to change { order.all_adjustments.count }.by(-4)
+    end
 
     it "clears adjustments from per-order fees" do
+      order = create(:order)
       enterprise_fee = create(:enterprise_fee)
       enterprise_fee_aplicator = OpenFoodNetwork::EnterpriseFeeApplicator.new(enterprise_fee, nil,
                                                                               'coordinator')
       enterprise_fee_aplicator.create_order_adjustment(order)
 
       expect do
-        described_class.clear_order_adjustments order
+        EnterpriseFee.clear_all_adjustments order
       end.to change { order.adjustments.count }.by(-1)
     end
 
     it "does not clear adjustments from another originator" do
+      order = create(:order)
       tax_rate = create(:tax_rate, calculator: build(:calculator))
       order.adjustments.create({ amount: 12.34,
                                  originator: tax_rate,
                                  state: 'closed',
                                  label: 'hello' })
 
       expect do
-        described_class.clear_order_adjustments order
+        EnterpriseFee.clear_all_adjustments order
       end.to change { order.adjustments.count }.by(0)
     end
-
-    it "doesn't clear adjustments from many fees and on all line items" do
-      line_item1 = create(:line_item, order:, variant: order_cycle.variants.first)
-      line_item2 = create(:line_item, order:, variant: order_cycle.variants.second)
-
-      # Order adjustment
-      fee1 = order_cycle.coordinator_fees[0].create_adjustment('foo1', line_item1.order, true)
-      fee2 = order_cycle.coordinator_fees[0].create_adjustment('foo2', line_item2.order, true)
-      # Line item adjustment
-      fee3 = order_cycle.exchanges[0].enterprise_fees[0].create_adjustment('foo3', line_item1, true)
-      fee4 = order_cycle.exchanges[0].enterprise_fees[0].create_adjustment('foo4', line_item2, true)
-
-      described_class.clear_order_adjustments order
-
-      adjustments = order.all_adjustments
-      # does not clear line item adjustments
-      expect(adjustments).not_to include(fee1, fee2)
-      expect(adjustments).to include(fee3, fee4)
-    end
   end
 
   describe "soft-deletion" do

spec/models/spree/order_spec.rb

@@ -614,6 +614,64 @@
     end
   end
 
+  describe "applying enterprise fees" do
+    subject { create(:order) }
+    let(:fee_handler) { Orders::HandleFeesService.new(subject) }
+
+    before do
+      allow(subject).to receive(:fee_handler) { fee_handler }
+      allow(subject).to receive(:update_order!)
+    end
+
+    it "clears all enterprise fee adjustments on the order" do
+      expect(EnterpriseFee).to receive(:clear_all_adjustments).with(subject)
+      subject.recreate_all_fees!
+    end
+
+    it "creates line item and order fee adjustments via Orders::HandleFeesService" 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)
+
+      allow(subject).to receive(:order_cycle) { nil }
+
+      subject.recreate_all_fees!
+    end
+
+    it "ensures the correct adjustment(s) are created for order cycles" do
+      allow(EnterpriseFee).to receive(:clear_all_adjustments)
+      line_item = create(:line_item, order: subject)
+      allow(fee_handler).to receive(:provided_by_order_cycle?) { true }
+
+      order_cycle = double(:order_cycle)
+      expect_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).
+        to receive(:create_line_item_adjustments_for).
+        with(line_item)
+      allow_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator)
+        .to receive(:create_order_adjustments_for)
+      allow(subject).to receive(:order_cycle) { order_cycle }
+
+      subject.recreate_all_fees!
+    end
+
+    it "ensures the correct per-order adjustment(s) are created for order cycles" do
+      allow(EnterpriseFee).to receive(:clear_all_adjustments)
+
+      order_cycle = double(:order_cycle)
+      expect_any_instance_of(OpenFoodNetwork::EnterpriseFeeCalculator).
+        to receive(:create_order_adjustments_for).
+        with(subject)
+
+      allow(subject).to receive(:order_cycle) { order_cycle }
+
+      subject.recreate_all_fees!
+    end
+  end
+
   describe "getting the admin and handling charge" do
     let(:o) { create(:order) }
     let(:li) { create(:line_item, order: o) }

spec/services/orders/handle_fees_service_spec.rb

@@ -9,177 +9,19 @@
 
   let(:service) { Orders::HandleFeesService.new(order) }
   let(:calculator) {
-    instance_double(OpenFoodNetwork::EnterpriseFeeCalculator, create_order_adjustments_for: true)
+    double(OpenFoodNetwork::EnterpriseFeeCalculator, create_order_adjustments_for: true)
   }
 
   before do
     allow(service).to receive(:calculator) { calculator }
   end
 
-  describe "#recreate_all_fees!" do
-    before do
-      allow(order).to receive(:update_order!)
-    end
-
-    it "clears order enterprise fee adjustments on the order" do
-      expect(EnterpriseFee).to receive(:clear_order_adjustments).with(order)
-
-      service.recreate_all_fees!
-    end
-
-    # both create_or_update_line_item_fees! and create_order_fees! are tested below,
-    # so it's enough to check they get called
-    it "creates line item and order fee adjustments" do
-      expect(service).to receive(:create_or_update_line_item_fees!)
-      expect(service).to receive(:create_order_fees!)
-
-      service.recreate_all_fees!
-    end
-
-    it "updates the order" do
-      expect(order).to receive(:update_order!)
-
-      service.recreate_all_fees!
-    end
-
-    it "doesn't create tax adjustment" do
-      expect(service).not_to receive(:tax_enterprise_fees!)
-
-      service.recreate_all_fees!
-    end
-
-    context "when after payment state" do
-      it "creates the tax adjustment for the fees" do
-        expect(service).to receive(:tax_enterprise_fees!)
-
-        order.update(state: "confirmation")
-        service.recreate_all_fees!
-      end
-    end
-  end
-
-  describe "#create_or_update_line_item_fees!" do
-    context "with no existing fee" do
-      it "creates per line item fee adjustments for line items in the order cylce" do
-        allow(service).to receive(:provided_by_order_cycle?) { true }
-        expect(calculator).to receive(:create_line_item_adjustments_for).with(line_item)
-
-        service.create_or_update_line_item_fees!
-      end
-
-      it "does create fee if variant not in Order Cyle" do
-        allow(service).to receive(:provided_by_order_cycle?) { false }
-        expect(calculator).not_to receive(:create_line_item_adjustments_for).with(line_item)
-
-        service.create_or_update_line_item_fees!
-      end
-    end
-
-    context "with existing line item fee" do
-      let(:fee) { order_cycle.exchanges[0].enterprise_fees.first }
-      let(:role) { order_cycle.exchanges[0].role }
-      let(:fee_applicator) {
-        OpenFoodNetwork::EnterpriseFeeApplicator.new(fee, line_item.variant, role)
-      }
-
-      it "updates the line item fee" do
-        allow(calculator).to receive(
-          :order_cycle_per_item_enterprise_fee_applicators_for
-        ).and_return([fee_applicator])
-        adjustment = fee.create_adjustment('foo', line_item, true)
-
-        expect do
-          service.create_or_update_line_item_fees!
-        end.to change { adjustment.reload.updated_at }
-      end
-
-      context "when enterprise fee is removed from the order cycle" do
-        it "removes the line item fee" do
-          adjustment = fee.create_adjustment('foo', line_item, true)
-          order_cycle.exchanges.first.enterprise_fees.destroy(fee)
-          allow(calculator).to receive(
-            :order_cycle_per_item_enterprise_fee_applicators_for
-          ).and_return([])
-
-          expect do
-            service.create_or_update_line_item_fees!
-          end.to change { line_item.adjustments.reload.enterprise_fee.count }.by(-1)
-        end
-      end
-
-      context "when an enterprise fee is deleted" do
-        before do
-          fee.create_adjustment('foo', line_item, true)
-          allow(calculator).to receive(
-            :order_cycle_per_item_enterprise_fee_applicators_for
-          ).and_return([])
-        end
-
-        context "soft delete" do
-          it "deletes the line item fee" do
-            fee.destroy
-
-            expect do
-              service.create_or_update_line_item_fees!
-            end.to change { line_item.adjustments.enterprise_fee.count }.by(-1)
-          end
-        end
-
-        context "hard delete" do
-          it "deletes the line item fee" do
-            fee.really_destroy!
-
-            expect do
-              service.create_or_update_line_item_fees!
-            end.to change { line_item.adjustments.enterprise_fee.count }.by(-1)
-          end
-        end
-      end
-
-      context "with a new enterprise fee added to the order cylce" do
-        let(:new_fee) { create(:enterprise_fee, enterprise: fee.enterprise) }
-        let(:fee_applicator2) {
-          OpenFoodNetwork::EnterpriseFeeApplicator.new(new_fee, line_item.variant, role)
-        }
-        let!(:adjustment) { fee.create_adjustment('foo', line_item, true) }
-
-        before do
-          allow(service).to receive(:provided_by_order_cycle?) { true }
-        end
-
-        it "creates a line item fee for the new fee" do
-          allow(calculator).to receive(
-            :order_cycle_per_item_enterprise_fee_applicators_for
-          ).and_return([fee_applicator2])
-
-          expect(fee_applicator2).to receive(:create_line_item_adjustment).with(line_item)
-
-          service.create_or_update_line_item_fees!
-        end
-
-        it "updates existing line item fee" do
-          allow(calculator).to receive(
-            :order_cycle_per_item_enterprise_fee_applicators_for
-          ).and_return([fee_applicator, fee_applicator2])
-
-          expect do
-            service.create_or_update_line_item_fees!
-          end.to change { adjustment.reload.updated_at }
-        end
-
-        context "with variant not included in the order cycle" do
-          it "doesn't create a new line item fee" do
-            allow(service).to receive(:provided_by_order_cycle?) { false }
-            allow(calculator).to receive(
-              :order_cycle_per_item_enterprise_fee_applicators_for
-            ).and_return([fee_applicator2])
-
-            expect(fee_applicator2).not_to receive(:create_line_item_adjustment).with(line_item)
+  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_or_update_line_item_fees!
-          end
-        end
-      end
+      service.create_line_item_fees!
     end
   end