Merge pull request #6315 from SuperGoodSoft/avoid-unnecessary-adjustment-persistence-in-tax-calculations

Avoid unnecessary adjustment persistence in tax calculations

Author: tvdeyen

core/app/models/spree/line_item.rb

@@ -18,7 +18,7 @@ class LineItem < Spree::Base
 
     has_one :product, through: :variant
 
-    has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :destroy
+    has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :destroy, autosave: true
     has_many :inventory_units, inverse_of: :line_item
 
     before_validation :normalize_quantity

core/app/models/spree/order.rb

@@ -102,7 +102,7 @@ def states
     has_many :cartons, -> { distinct }, through: :inventory_units
 
     # Adjustments and promotions
-    has_many :adjustments, -> { order(:created_at) }, as: :adjustable, inverse_of: :adjustable, dependent: :destroy
+    has_many :adjustments, -> { order(:created_at) }, as: :adjustable, inverse_of: :adjustable, dependent: :destroy, autosave: true
     has_many :line_item_adjustments, through: :line_items, source: :adjustments
     has_many :shipment_adjustments, through: :shipments, source: :adjustments
     has_many :all_adjustments,

core/app/models/spree/order_taxation.rb

@@ -57,7 +57,7 @@ def update_adjustments(item, taxed_items)
 
       # Remove any tax adjustments tied to rates which no longer match.
       unmatched_adjustments = tax_adjustments - active_adjustments
-      item.adjustments.destroy(unmatched_adjustments)
+      unmatched_adjustments.each(&:mark_for_destruction)
     end
 
     # Update or create a new tax adjustment on an item.
@@ -77,7 +77,8 @@ def update_adjustment(item, tax_item)
         label: tax_item.label,
         included: tax_item.included_in_price
       )
-      tax_adjustment.update!(amount: tax_item.amount)
+
+      tax_adjustment.amount = tax_item.amount
       tax_adjustment
     end
   end

core/app/models/spree/order_updater.rb

@@ -157,9 +157,12 @@ def update_adjustment_total
       recalculate_adjustments
 
       all_items = line_items + shipments
-      order_tax_adjustments = adjustments.select(&:tax?)
+      # Ignore any adjustments that have been marked for destruction in our
+      # calculations. They'll get removed when/if we persist the order.
+      valid_adjustments = adjustments.reject(&:marked_for_destruction?)
+      order_tax_adjustments = valid_adjustments.select(&:tax?)
 
-      order.adjustment_total = all_items.sum(&:adjustment_total) + adjustments.sum(&:amount)
+      order.adjustment_total = all_items.sum(&:adjustment_total) + valid_adjustments.sum(&:amount)
       order.included_tax_total = all_items.sum(&:included_tax_total) + order_tax_adjustments.select(&:included?).sum(&:amount)
       order.additional_tax_total = all_items.sum(&:additional_tax_total) + order_tax_adjustments.reject(&:included?).sum(&:amount)
 

core/spec/models/spree/order_taxation_spec.rb

@@ -68,7 +68,7 @@
 
     it "creates a new tax adjustment", aggregate_failures: true do
       apply
-      expect(line_item.adjustments.count).to eq 1
+      expect(line_item.adjustments.size).to eq 1
 
       tax_adjustment = line_item.adjustments.first
       expect(tax_adjustment.label).to eq "Tax!"
@@ -132,12 +132,11 @@
         )
       end
 
-      it "removes the tax adjustment" do
-        expect {
-          taxation.apply(new_taxes)
-        }.to change {
-          line_item.adjustments.count
-        }.from(1).to(0)
+      it "marks the tax adjustment for destruction" do
+        order.save!
+        taxation.apply(new_taxes)
+
+        expect(line_item.adjustments.first).to be_marked_for_destruction
       end
     end
 
@@ -174,7 +173,7 @@
       end
 
       it "creates a new tax adjustment", aggregate_failures: true do
-        expect(order.adjustments.count).to eq 1
+        expect(order.adjustments.size).to eq 1
 
         tax_adjustment = order.adjustments.first
         expect(tax_adjustment.label).to eq "Order Tax!"

core/spec/models/spree/order_updater_spec.rb

@@ -69,28 +69,39 @@ module Spree
       end
 
       describe 'tax recalculation' do
-        let!(:ship_address) { create(:address) }
-        let!(:tax_zone) { create(:global_zone) } # will include the above address
-        let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) }
+        let(:tax_category) { create(:tax_category) }
+        let(:ship_address) { create(:address, state: new_york) }
+        let(:new_york) { create(:state, state_code: "NY") }
+        let(:tax_zone) { create(:zone, states: [new_york]) }
+
+        let!(:tax_rate) do
+          create(
+            :tax_rate,
+            name: "New York Sales Tax",
+            tax_categories: [tax_category],
+            zone: tax_zone,
+            included_in_price: false,
+            amount: 0.1
+          )
+        end
 
         let(:order) do
           create(
             :order_with_line_items,
-            line_items_attributes: [{ price: 10, variant: }],
-            ship_address:,
+            line_items_attributes: [{ price: 10, variant: variant }],
+            ship_address: ship_address,
           )
         end
         let(:line_item) { order.line_items[0] }
 
         let(:variant) { create(:variant, tax_category:) }
-        let(:tax_category) { create(:tax_category) }
 
         context 'when the item quantity has changed' do
           before do
             line_item.update!(quantity: 2)
           end
 
-          it 'updates the promotion amount' do
+          it 'updates the additional_tax_total' do
             expect {
               order.recalculate
             }.to change {
@@ -99,6 +110,60 @@ module Spree
           end
         end
 
+        context 'when the address has changed to a different state' do
+          let(:new_shipping_address) { create(:address) }
+
+          before do
+            order.ship_address = new_shipping_address
+          end
+
+          it 'removes the old taxes' do
+            expect {
+              order.recalculate
+            }.to change {
+              order.all_adjustments.tax.count
+            }.from(1).to(0)
+
+            expect(order.additional_tax_total).to eq 0
+            expect(order.adjustment_total).to eq 0
+          end
+        end
+
+        context "with an order-level tax adjustment" do
+          let(:colorado) { create(:state, state_code: "CO") }
+          let(:colorado_tax_zone) { create(:zone, states: [colorado]) }
+          let(:ship_address) { create(:address, state: colorado) }
+
+          let!(:colorado_delivery_fee) do
+            create(
+              :tax_rate,
+              amount: 0.27,
+              calculator: Spree::Calculator::FlatFee.new,
+              level: "order",
+              name: "Colorado Delivery Fee",
+              tax_categories: [tax_category],
+              zone: colorado_tax_zone
+            )
+          end
+
+          before { order.recalculate }
+
+          it "updates the order-level tax adjustment" do
+            expect {
+              order.ship_address = create(:address)
+              order.recalculate
+            }.to change { order.additional_tax_total }.from(0.27).to(0).
+                and change { order.adjustment_total }.from(0.27).to(0)
+          end
+
+          it "deletes the order-level tax adjustments when it persists the order" do
+            expect {
+              order.ship_address = create(:address)
+              order.recalculate
+            }.to change { order.all_adjustments.count }.from(1).to(0)
+          end
+        end
+
         context 'with a custom tax_calculator_class' do
           let(:custom_calculator_class) { double }
           let(:custom_calculator_instance) { double }