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