From 4a77350fb9e71268a465b041cccb825f8cacf678 Mon Sep 17 00:00:00 2001 From: Andrew Stewart Date: Fri, 20 Dec 2024 13:34:06 -0800 Subject: [PATCH] Refactor item-total recalculation into Spree::ItemTotal This change more clearly splits up the logic for recalculating shipment/item adjustment totals from the logic for persisting updated values to the database. Continuing to colocating logic for recalculating item totals in one PORO makes it easier to reason about and helps to simplify the higher-level order updater. Co-Authored-By: Harmony Evangelina Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: benjamin wil --- TODO.md | 2 +- .../models/spree/in_memory_order_updater.rb | 49 ++++++++----------- core/app/models/spree/item_total.rb | 9 ++++ core/spec/models/spree/item_total_spec.rb | 46 +++++++++++++---- 4 files changed, 68 insertions(+), 38 deletions(-) diff --git a/TODO.md b/TODO.md index 5dcf61e09b..f83e2e289b 100644 --- a/TODO.md +++ b/TODO.md @@ -1,7 +1,7 @@ In-Memory Order Updater TODO === -- [ ] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) +- [x] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) - [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing - [ ] Add test coverage for `update_item_total` when line item totals change - [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index b91f0e3037..963257bdd1 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -120,8 +120,9 @@ def update_adjustments(persist:) # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ update_promotions(persist:) - update_taxes - update_item_totals(persist:) + update_tax_adjustments + recalculate_item_totals + persist_changes_to_items if persist end # Updates the following Order total values: @@ -163,40 +164,32 @@ def update_promotions(persist:) end.new(order).call end - def update_taxes + # TODO: split implementation based on 'persist' + 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 end deprecate :update_cancellations, deprecator: Spree.deprecator - def update_item_totals(persist:) + def recalculate_item_totals [*line_items, *shipments].each do |item| - Spree::ItemTotalUpdater.recalculate(item) - - if persist && 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! + end + end + + def persist_item_changes + [*line_items, *shipments].each do |item| + 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 diff --git a/core/app/models/spree/item_total.rb b/core/app/models/spree/item_total.rb index 8d13ccbeac..3ef666e39e 100644 --- a/core/app/models/spree/item_total.rb +++ b/core/app/models/spree/item_total.rb @@ -6,6 +6,15 @@ def initialize(item) end def recalculate! + tax_adjustments = item.adjustments.select(&:tax?) + + # 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(&:included?).sum(&:amount) end diff --git a/core/spec/models/spree/item_total_spec.rb b/core/spec/models/spree/item_total_spec.rb index 9d71cf9352..1f6a2433a6 100644 --- a/core/spec/models/spree/item_total_spec.rb +++ b/core/spec/models/spree/item_total_spec.rb @@ -6,18 +6,46 @@ describe "#recalculate!" do subject { described_class.new(item).recalculate! } - let(:item) { create :line_item, adjustments: [adjustment] } - let(:adjustment) { create :adjustment, amount: 19.99 } + let(:item) { create :line_item, adjustments: } - it "sets the adjustment total on the item" do - expect { subject } - .to change { item.adjustment_total } - .from(0).to(19.99) + 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(:adjustments) { [arbitrary_adjustment, included_tax_adjustment, additional_tax_adjustment] } + + 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 - it "does not factor in included adjustments" do - adjustment.update!(included: true) - expect { subject }.not_to change { item.adjustment_total }.from(0) + 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