Merge pull request solidusio#6080 from SuperGoodSoft/refactor-line-item-total-updates

Refactor Line Item Total Calculations

Author: jarednorman

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

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::Config.item_total_class.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/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.

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

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?).