From 46315c40457cef52ee3da7af50eb0f9ebb4fa037 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 12 Feb 2025 13:55:42 +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 | 48 +++++++-- .../orders/handle_fees_service_spec.rb | 99 +++++++++++++++++-- 3 files changed, 135 insertions(+), 14 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 66bfdb3db3..9fd75f2a8f 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -86,7 +86,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..ab46d95d16 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,15 @@ 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 - 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 + next create_line_item_fees!(line_item) if line_item.enterprise_fee_adjustments.blank? + + create_or_update_line_item_fee!(line_item) + + # update any fees removed from the Order Cycle + update_removed_fees!(line_item) end end @@ -66,5 +70,35 @@ module Orders @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 update_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 do |removed_fee| + removed_fee.update_adjustment!(line_item, force: true) + end + end + + def fee_applicators(variant) + calculator.order_cycle_per_item_enterprise_fee_applicators_for(variant) + end end end diff --git a/spec/services/orders/handle_fees_service_spec.rb b/spec/services/orders/handle_fees_service_spec.rb index f31ce21ce2..8bcf8d4273 100644 --- a/spec/services/orders/handle_fees_service_spec.rb +++ b/spec/services/orders/handle_fees_service_spec.rb @@ -9,19 +9,106 @@ RSpec.describe Orders::HandleFeesService do 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 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 "updates 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 { adjustment.reload.updated_at } + 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) + + service.create_or_update_line_item_fees! + end + end + end end end