From b5bc6b84d7c4480ec019e35514736a1abcb460cf Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 31 Mar 2025 12:55:35 +1100 Subject: [PATCH] Update Orders::HandleFeesService#recreate_all_fees! We now update or create line item fees instead of deleting them and recreating them. This is to cover the case when a product has been removed from an Order Cycle but we want to keep the fee already applied on existing order. This was an issue only if the existing order got updated after the product was removed. --- app/models/spree/order.rb | 2 +- app/services/orders/handle_fees_service.rb | 69 +++++++- .../orders/handle_fees_service_spec.rb | 163 +++++++++++++++++- 3 files changed, 220 insertions(+), 14 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 642b57678a..8ad700f3a3 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -88,7 +88,7 @@ module Spree delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher delegate :update_totals, :update_totals_and_states, to: :updater - delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, + delegate :create_order_fees!, :update_order_fees!, :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler validates :customer, presence: true, if: :require_customer? diff --git a/app/services/orders/handle_fees_service.rb b/app/services/orders/handle_fees_service.rb index 09ee04f1a2..16d0ffd0d1 100644 --- a/app/services/orders/handle_fees_service.rb +++ b/app/services/orders/handle_fees_service.rb @@ -16,9 +16,9 @@ module Orders # 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 + EnterpriseFee.clear_order_adjustments order - create_line_item_fees! + create_or_update_line_item_fees! create_order_fees! end @@ -26,11 +26,17 @@ module Orders order.update_order! 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 + 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 end + create_or_update_line_item_fee!(line_item) + + # delete any fees removed from the Order Cycle + delete_removed_fees!(line_item) end end @@ -58,6 +64,57 @@ module Orders private + 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) + applicators = calculator.per_item_enterprise_fee_applicators_for(line_item.variant) + + applicators.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 + + # Update any fees not already processed + fees_to_update = + order_cycle_per_item_enterprise_fee_for - applicators.map(&:enterprise_fee) + fees_to_update.each do |fee| + fee_adjustment = line_item.adjustments.find_by(originator: fee) + + fee_adjustment&.update_adjustment!(line_item, force: true) + end + end + + def delete_removed_fees!(line_item) + order_cycle_fees = order_cycle_per_item_enterprise_fee_for + + removed_fees = line_item.enterprise_fee_adjustments.where.not(originator: order_cycle_fees) + + removed_fees.each(&:destroy) + end + + def order_cycle_per_item_enterprise_fee_for + fees = [] + + return fees unless order_cycle && distributor + + order_cycle.exchanges.supplying_to(distributor).each do |exchange| + fees += exchange.enterprise_fees.per_item + end + + fees += order_cycle.coordinator_fees.per_item + + fees + end + def calculator @calculator ||= OpenFoodNetwork::EnterpriseFeeCalculator.new(distributor, order_cycle) end diff --git a/spec/services/orders/handle_fees_service_spec.rb b/spec/services/orders/handle_fees_service_spec.rb index f31ce21ce2..7018c662c7 100644 --- a/spec/services/orders/handle_fees_service_spec.rb +++ b/spec/services/orders/handle_fees_service_spec.rb @@ -4,24 +4,173 @@ require 'spec_helper' RSpec.describe Orders::HandleFeesService do let(:order_cycle) { create(:order_cycle) } - let(:order) { create(:order_with_line_items, line_items_count: 1, order_cycle:) } + let(:order) { + create(:order_with_line_items, line_items_count: 1, order_cycle:, + distributor: order_cycle.distributors.first) + } let(:line_item) { order.line_items.first } let(:service) { Orders::HandleFeesService.new(order) } let(:calculator) { - double(OpenFoodNetwork::EnterpriseFeeCalculator, create_order_adjustments_for: true) + instance_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) + 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_line_item_fees! + service.create_or_update_line_item_fees! + end + + it "does not 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.first.enterprise_fees.first } + let(:role) { order_cycle.exchanges.first.role } + let(:fee_applicator) { + OpenFoodNetwork::EnterpriseFeeApplicator.new(fee, line_item.variant, role) + } + + it "updates the line item fee" do + allow(calculator).to receive( + :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 the variant has been removed from the order cycle" do + it "updates the line item fee" do + allow(calculator).to receive( + :per_item_enterprise_fee_applicators_for + ).and_return([]) + 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 + 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( + :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 + + context "with coordinator fee" do + it "removes the coordinator fee" do + coordinator_fee = order_cycle.coordinator_fees.per_item.first + adjustment = coordinator_fee.create_adjustment('foo', line_item, true) + order_cycle.coordinator_fees.destroy(coordinator_fee) + allow(calculator).to receive( + :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 + end + + context "when an enterprise fee is deleted" do + before do + fee.create_adjustment('foo', line_item, true) + allow(calculator).to receive( + :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 } + # add the new fee to the order cycle + order_cycle.cached_outgoing_exchanges.first.enterprise_fees << new_fee + end + + it "creates a line item fee for the new fee" do + allow(calculator).to receive( + :per_item_enterprise_fee_applicators_for + ).and_return([fee_applicator, 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( + :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( + :per_item_enterprise_fee_applicators_for + ).and_return([]) + + expect(fee_applicator2).not_to receive(:create_line_item_adjustment).with(line_item) + + service.create_or_update_line_item_fees! + end + end + end end end