From c261747d5ceba381607ac6419494c47bcaf8e335 Mon Sep 17 00:00:00 2001 From: Jared Norman Date: Fri, 17 Jan 2025 14:17:33 -0800 Subject: [PATCH 1/2] Refactor line item total calculations While working on the in-memory updater in #5872, we found the need to change how item totals were being calculated, so that we could mark adjustments for destruction without actually destroying them, while still keeping tax adjustments intact. This change is completely backwards-compatible with the current OrderUpdater, so to reduce the scope of our PR, we wanted to make this change separately. Since the OrderUpdater is already very large, this helps reduce its responsibilities and makes it easier to test this behaviour. We don't see it as necessary to make this a configurable class, but this change leaves that option open in the future. Co-authored-by: Adam Mueller Co-authored-by: Alistair Norman Co-authored-by: Chris Todorov Co-authored-by: Harmony Bouvier Co-authored-by: Sofia Besenski Co-authored-by: benjamin wil --- core/app/models/spree/item_total.rb | 28 ++++++++ core/app/models/spree/order_updater.rb | 43 ++++-------- core/spec/models/spree/item_total_spec.rb | 67 +++++++++++++++++++ .../spree_order_updater_decorator.rb | 8 ++- 4 files changed, 115 insertions(+), 31 deletions(-) create mode 100644 core/app/models/spree/item_total.rb create mode 100644 core/spec/models/spree/item_total_spec.rb diff --git a/core/app/models/spree/item_total.rb b/core/app/models/spree/item_total.rb new file mode 100644 index 00000000000..c691700b6c5 --- /dev/null +++ b/core/app/models/spree/item_total.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class Spree::ItemTotal + def initialize(item) + @item = item + end + + def recalculate! + tax_adjustments = item.adjustments.select do |adjustment| + adjustment.tax? && !adjustment.marked_for_destruction? + end + + # Included tax adjustments are those which are included in the price. + # These ones should not affect the eventual total price. + # + # Additional tax adjustments are the opposite, affecting the final total. + item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) + item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) + + item.adjustment_total = item.adjustments.reject { |adjustment| + adjustment.marked_for_destruction? || adjustment.included? + }.sum(&:amount) + end + + private + + attr_reader :item +end diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 6c06b022efc..732b00d8177 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -113,7 +113,7 @@ def recalculate_adjustments # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ update_promotions - update_taxes + update_tax_adjustments update_item_totals end @@ -198,21 +198,8 @@ def update_promotions Spree::Config.promotions.order_adjuster_class.new(order).call end - def update_taxes + def update_tax_adjustments Spree::Config.tax_adjuster_class.new(order).adjust! - - [*line_items, *shipments].each do |item| - tax_adjustments = item.adjustments.select(&:tax?) - # Tax adjustments come in not one but *two* exciting flavours: - # Included & additional - - # Included tax adjustments are those which are included in the price. - # These ones should not affect the eventual total price. - # - # Additional tax adjustments are the opposite, affecting the final total. - item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) - item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) - end end def update_cancellations @@ -221,21 +208,17 @@ def update_cancellations def update_item_totals [*line_items, *shipments].each do |item| - # The cancellation_total isn't persisted anywhere but is included in - # the adjustment_total - item.adjustment_total = item.adjustments. - reject(&:included?). - sum(&:amount) - - if item.changed? - item.update_columns( - promo_total: item.promo_total, - included_tax_total: item.included_tax_total, - additional_tax_total: item.additional_tax_total, - adjustment_total: item.adjustment_total, - updated_at: Time.current, - ) - end + Spree::ItemTotal.new(item).recalculate! + + next unless item.changed? + + item.update_columns( + promo_total: item.promo_total, + included_tax_total: item.included_tax_total, + additional_tax_total: item.additional_tax_total, + adjustment_total: item.adjustment_total, + updated_at: Time.current, + ) end end end diff --git a/core/spec/models/spree/item_total_spec.rb b/core/spec/models/spree/item_total_spec.rb new file mode 100644 index 00000000000..3597c51860f --- /dev/null +++ b/core/spec/models/spree/item_total_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::ItemTotal do + describe "#recalculate!" do + subject { described_class.new(item).recalculate! } + + let!(:item) { create :line_item, adjustments: } + + let(:tax_rate) { create(:tax_rate) } + + let(:arbitrary_adjustment) { create :adjustment, amount: 1, source: nil } + let(:included_tax_adjustment) { create :adjustment, amount: 2, source: tax_rate, included: true } + let(:additional_tax_adjustment) { create :adjustment, amount: 3, source: tax_rate, included: false } + + context "with multiple types of adjustments" do + let(:marked_for_destruction_included_tax_adjustment) { create(:adjustment, amount: 5, source: tax_rate, included: true) } + let(:marked_for_destruction_additional_tax_adjustment) { create(:adjustment, amount: 7, source: tax_rate, included: false) } + + let(:adjustments) { + [ + arbitrary_adjustment, + included_tax_adjustment, + additional_tax_adjustment, + marked_for_destruction_included_tax_adjustment, + marked_for_destruction_additional_tax_adjustment + ] + } + + before do + [marked_for_destruction_included_tax_adjustment, marked_for_destruction_additional_tax_adjustment] + .each(&:mark_for_destruction) + end + + it "updates item totals" do + expect { + subject + }.to change(item, :adjustment_total).from(0).to(4). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end + end + + context "with only an arbitrary adjustment" do + let(:adjustments) { [arbitrary_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(1) + end + end + + context "with only tax adjustments" do + let(:adjustments) { [included_tax_adjustment, additional_tax_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(3). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end + end + end +end diff --git a/legacy_promotions/app/decorators/models/solidus_legacy_promotions/spree_order_updater_decorator.rb b/legacy_promotions/app/decorators/models/solidus_legacy_promotions/spree_order_updater_decorator.rb index e8e18ee9cff..eeaff71b3bf 100644 --- a/legacy_promotions/app/decorators/models/solidus_legacy_promotions/spree_order_updater_decorator.rb +++ b/legacy_promotions/app/decorators/models/solidus_legacy_promotions/spree_order_updater_decorator.rb @@ -17,8 +17,14 @@ def update_adjustment_total def update_item_totals [*line_items, *shipments].each do |item| + Spree::ItemTotal.new(item).recalculate! + # The cancellation_total isn't persisted anywhere but is included in - # the adjustment_total + # the adjustment_total. + # + # Core doesn't have "eligible" adjustments anymore, so we need to + # override the adjustment_total calculation to exclude them for legacy + # promotions. item.adjustment_total = item.adjustments. select(&:eligible?). reject(&:included?). From 7a3bfc26704562ce73c695d8f64104195a88ea96 Mon Sep 17 00:00:00 2001 From: Adam Mueller Date: Fri, 31 Jan 2025 08:23:00 -0800 Subject: [PATCH 2/2] Make ItemTotal class configurable I don't know if there's all that much customizing people will perform on this class, but it's easy enough to make it configurable just in case. --- core/app/models/spree/order_updater.rb | 2 +- core/lib/spree/app_configuration.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index 732b00d8177..d1f4106958e 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -208,7 +208,7 @@ def update_cancellations def update_item_totals [*line_items, *shipments].each do |item| - Spree::ItemTotal.new(item).recalculate! + Spree::Config.item_total_class.new(item).recalculate! next unless item.changed? diff --git a/core/lib/spree/app_configuration.rb b/core/lib/spree/app_configuration.rb index 6287c3818e6..cd7b8f0268c 100644 --- a/core/lib/spree/app_configuration.rb +++ b/core/lib/spree/app_configuration.rb @@ -430,6 +430,13 @@ def payment_canceller # as Spree::Wallet::AddPaymentSourcesToWallet. class_name_attribute :add_payment_sources_to_wallet_class, default: 'Spree::Wallet::AddPaymentSourcesToWallet' + # Allows providing your own class for recalculating totals on an item. + # + # @!attribute [rw] item_total_class + # @return [Class] a class with the same public interfaces as + # Spree::ItemTotal + class_name_attribute :item_total_class, default: 'Spree::ItemTotal' + # Allows providing your own class for calculating taxes on an order. # # This extension point is under development and may change in a future minor release.