Refactor line item total calculations

While working on the in-memory updater in solidusio#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 <[email protected]>
Co-authored-by: Alistair Norman <[email protected]>
Co-authored-by: Chris Todorov <[email protected]>
Co-authored-by: Harmony Bouvier <[email protected]>
Co-authored-by: Sofia Besenski <[email protected]>
Co-authored-by: benjamin wil <[email protected]>

Author: 7 people

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 { |adjustment|
+      adjustment.tax? && !adjustment.marked_for_destruction?
+    }
+
+    # 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

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

core/spec/models/spree/item_total_spec.rb

@@ -0,0 +1,65 @@
+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