Make OrderTaxation mark adjustments for removal

Instead of actually destroying adjustments in the OrderTaxation class, we can just mark them for removal. This will allow them to be removed when the order is persisted, instead of being removed during the recalculate process.

This does have the side effect of needing to check for tax adjustments that are marked for destruction when calculating tax totals elsewhere in the OrderUpdaters

Co-authored-by: Chris Todorov <chris@super.gd>
Co-authored-by: Adam Mueller <adam@super.gd>
Co-authored-by: Alistair Norman <alistair@super.gd>
Co-authored-by: benjamin wil <benjamin@super.gd>
Co-authored-by: Harmony Bouvier <harmony@super.gd>
Co-authored-by: Kendra Riga <kendra@super.gd>
Co-authored-by: Senem <senem@super.gd>

Author: 8 people

TODO.md

@@ -6,6 +6,7 @@ In-Memory Order Updater TODO
 - [x] Add test coverage for `recalculate_item_total` when line item totals change
 - [ ] Handle persistence in `update_promotions`
 - [ ] Handle persistence in `update_taxes`
+- [ ] Scope handling of tax adjustments in `InMemoryOrderUpdater` and `OrderUpdater` to *not* marked for destruction
 - [ ] Write the `InMemoryOrderAdjuster` (also, should we rename this to `InMemoryOrderPromotionAdjuster`)
 
 - [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation)

core/app/models/spree/in_memory_order_updater.rb

@@ -148,6 +148,7 @@ def update_adjustment_total(persist:)
       update_adjustments(persist:)
 
       all_items = line_items + shipments
+      # FIXME: We will need to ignore tax adjustments marked for destruction here and maybe elsewhere
       order_tax_adjustments = adjustments.select(&:tax?)
 
       order.adjustment_total = all_items.sum(&:adjustment_total) + adjustments.sum(&:amount)

core/app/models/spree/order_taxation.rb

@@ -56,9 +56,8 @@ def update_adjustments(item, taxed_items)
 
       # Remove any tax adjustments tied to rates which no longer match.
       unmatched_adjustments = tax_adjustments - active_adjustments
-      # FIXME: Find a way to not have to destroy for in-memory recalculating
-      # https://api.rubyonrails.org/classes/ActiveRecord/AutosaveAssociation.html#method-i-mark_for_destruction
-      item.adjustments.destroy(unmatched_adjustments)
+
+      unmatched_adjustments.each(&:mark_for_destruction)
     end
 
     # Update or create a new tax adjustment on an item.

core/app/models/spree/order_updater.rb

@@ -202,6 +202,7 @@ def update_taxes
       Spree::Config.tax_adjuster_class.new(order).adjust!
 
       [*line_items, *shipments].each do |item|
+        # FIXME: We will need to ignore tax adjustments marked for destruction here and maybe elsewhere
         tax_adjustments = item.adjustments.select(&:tax?)
         # Tax adjustments come in not one but *two* exciting flavours:
         # Included & additional

core/spec/models/spree/in_memory_order_updater_spec.rb

@@ -147,9 +147,21 @@ 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.05
+        )
+      end
 
       let(:order) do
         create(
@@ -161,22 +173,37 @@ module Spree
       let(:line_item) { order.line_items[0] }
 
       let(:variant) { create(:variant, tax_category: 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 {
-            line_item.additional_tax_total
+            line_item.reload.additional_tax_total
           }.from(1).to(2)
         end
 
         context "when recalculating the order in memory" do
+          it "does not persist the tax totals" do
+            expect {
+              described_class.new(order).recalculate(persist: false)
+            }.to change {
+              line_item.additional_tax_total
+            }.from(1).to(2)
+
+            expect(line_item.reload.additional_tax_total).to eq(1)
+          end
+
+          it 'does not make manipulative queries' do
+            expect {
+              described_class.new(order).recalculate(persist: false)
+            }.not_to make_database_queries(manipulative: true)
+          end
+
           it "raises an error" do
             order_adjuster = double
             allow(order_adjuster).to receive(:call) { raise NotImplementedError }
@@ -188,6 +215,30 @@ 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 {
+            described_class.new(order).recalculate(persist: true)
+          }.to change {
+            order.all_adjustments.tax.count
+          }.from(1).to(0)
+        end
+
+        context "when the persist flag is set to 'false'" do
+          it "doesn't make manipulative database queries" do
+            expect {
+              described_class.new(order).recalculate(persist: false)
+            }.not_to make_database_queries(manipulative: true)
+          end
+        end
+      end
+
       context 'with a custom tax_calculator_class' do
         let(:custom_calculator_class) { double }
         let(:custom_calculator_instance) { double }

core/spec/models/spree/order_taxation_spec.rb

@@ -116,12 +116,11 @@
         )
       end
 
-      it "removes the tax adjustment" do
-        expect {
-          taxation.apply(new_taxes)
-        }.to change {
-          line_item.adjustments.size
-        }.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