Merge pull request #1463 from jordan-brough/tax-invocation-updates

Calculate taxes in OrderContents instead of in a LineItem callback

CHANGELOG.md

@@ -1,5 +1,12 @@
 ## Solidus 2.1.0 (master, unreleased)
 
+*   Remove callback `Spree::LineItem.after_create :update_tax_charge`
+
+    Any code that creates `LineItem`s outside the context of OrderContents
+    should ensure that it calls `order.create_tax_charge!` after doing so.
+
+*   Mark `Spree::Tax::ItemAdjuster` as api-private
+
 *   Analytics trackers were removed from the admin panel; the extension
     `solidus_trackers` provides the same functionality
 

core/app/models/spree/line_item.rb

@@ -31,8 +31,6 @@ class LineItem < Spree::Base
     validates :price, numericality: true
     validate :ensure_proper_currency
 
-    after_create :update_tax_charge
-
     after_save :update_inventory
 
     before_destroy :update_inventory
@@ -174,10 +172,6 @@ def destroy_inventory_units
       inventory_units.destroy_all
     end
 
-    def update_tax_charge
-      Spree::Tax::ItemAdjuster.new(self).adjust!
-    end
-
     def ensure_proper_currency
       if currency != order.currency
         Spree::Deprecation.warn "The line items currency is different from it's order currency. " \

core/app/models/spree/order_contents.rb

@@ -35,20 +35,8 @@ def remove_line_item(line_item, options = {})
     end
 
     def update_cart(params)
-      # We need old_tax_address / new_tax_address because we can't rely on methods
-      # offered by ActiveRecord::Dirty to determine if tax_address was updated
-      # because if we update the address, a new record will be created
-      # by the Address.factory instead of the old record being updated
-
-      old_tax_address = order.tax_address
-
       if order.update_attributes(params)
-
-        new_tax_address = order.tax_address
-
-        if should_recalculate_taxes?(old_tax_address, new_tax_address)
-          order.create_tax_charge!
-        end
+        order.create_tax_charge!
 
         unless order.completed?
           order.line_items = order.line_items.select { |li| li.quantity > 0 }
@@ -84,41 +72,12 @@ def approve(user: nil, name: nil)
 
     private
 
-    def should_recalculate_taxes?(old_address, new_address)
-      # Related to Solidus issue #894
-      # This is needed because if you update the shipping_address
-      # from the backend on an order that completed checkout,
-      # Taxes were not being recalculated if the Order tax zone
-      # was updated
-      #
-      # Possible cases:
-      #
-      # Case 1:
-      #
-      # If old_address is a TaxLocation it means that the order has not passed
-      # the address checkout state so taxes will be computed by the Order
-      # state machine, so we do not calculate taxes here.
-      #
-      # Case 2 :
-      # If new_address is a TaxLocation, but old_address is not, it means that
-      # an order has somehow lost his TaxAddress. Since it's not supposed to happen,
-      # we do not compute taxes.
-      #
-      # Case 3
-      # Both old_address and new_address are Spree::Address so the order
-      # has completed the checkout or that a registered user has updated his
-      # default addresses. We need to recalculate the taxes.
-
-      return if old_address.is_a?(Spree::Tax::TaxLocation) || new_address.is_a?(Spree::Tax::TaxLocation)
-
-      old_address.try!(:taxation_attributes) != new_address.try!(:taxation_attributes)
-    end
-
     def after_add_or_remove(line_item, options = {})
       reload_totals
       shipment = options[:shipment]
       shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments
       PromotionHandler::Cart.new(order, line_item).activate
+      order.create_tax_charge!
       reload_totals
       line_item
     end

core/app/models/spree/tax/item_adjuster.rb

@@ -1,3 +1,7 @@
+# @api private
+# @note This is a helper class for Tax::OrderAdjuster.  It is marked as api
+#   private because taxes should always be calculated on the entire order, so
+#   external code should call Tax::OrderAdjuster instead of Tax::ItemAdjuster.
 module Spree
   module Tax
     # Adjust a single taxable item (line item or shipment)

core/lib/spree/core/unreturned_item_charger.rb

@@ -24,6 +24,7 @@ def charge_for_items
       new_order.associate_user!(@original_order.user) if @original_order.user
 
       add_exchange_variants_to_order
+      new_order.create_tax_charge!
       set_shipment_for_new_order
 
       new_order.update!

core/lib/spree/testing_support/factories/order_factory.rb

@@ -45,6 +45,7 @@
           create(:line_item, attributes)
         end
         order.line_items.reload
+        order.create_tax_charge!
 
         create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, stock_location: evaluator.stock_location)
         order.shipments.reload

core/spec/models/spree/line_item_spec.rb

@@ -35,41 +35,6 @@
     end
   end
 
-  context "#create" do
-    let(:variant) { create(:variant) }
-
-    before do
-      create(:tax_rate, zone: order.tax_zone, tax_category: variant.tax_category)
-    end
-
-    context "when order has a tax zone" do
-      before do
-        expect(order.tax_zone).to be_present
-      end
-
-      it "creates a tax adjustment" do
-        order.contents.add(variant)
-        line_item = order.find_line_item_by_variant(variant)
-        expect(line_item.adjustments.tax.count).to eq(1)
-      end
-    end
-
-    context "when order does not have a tax zone" do
-      before do
-        order.bill_address = nil
-        order.ship_address = nil
-        order.save
-        expect(order.reload.tax_zone).to be_nil
-      end
-
-      it "does not create a tax adjustment" do
-        order.contents.add(variant)
-        line_item = order.find_line_item_by_variant(variant)
-        expect(line_item.adjustments.tax.count).to eq(0)
-      end
-    end
-  end
-
   describe 'line item creation' do
     let(:variant) { create :variant }
 

core/spec/models/spree/order_contents_spec.rb

@@ -2,12 +2,12 @@
 
 describe Spree::OrderContents, type: :model do
   let!(:store) { create :store }
-  let(:order) { Spree::Order.create }
+  let(:order) { create(:order) }
   let(:variant) { create(:variant) }
   let!(:stock_location) { variant.stock_locations.first }
   let(:stock_location_2) { create(:stock_location) }
 
-  subject { described_class.new(order) }
+  subject(:order_contents) { described_class.new(order) }
 
   context "#add" do
     context 'given quantity is not explicitly provided' do
@@ -96,6 +96,38 @@
         include_context "discount changes order total"
       end
     end
+
+    describe 'tax calculations' do
+      let!(:zone) { create(:global_zone) }
+      let!(:tax_rate) do
+        create(:tax_rate, zone: zone, tax_category: variant.tax_category)
+      end
+
+      context 'when the order has a tax zone' do
+        before do
+          expect(order.tax_zone).to be_present
+        end
+
+        it 'creates a tax adjustment' do
+          order_contents.add(variant)
+          line_item = order.find_line_item_by_variant(variant)
+          expect(line_item.adjustments.tax.count).to eq(1)
+        end
+      end
+
+      context 'when the order does not have a tax zone' do
+        before do
+          order.update_attributes!(ship_address: nil, bill_address: nil)
+          expect(order.tax_zone).to be_nil
+        end
+
+        it 'creates a tax adjustment' do
+          order_contents.add(variant)
+          line_item = order.find_line_item_by_variant(variant)
+          expect(line_item.adjustments.tax.count).to eq(0)
+        end
+      end
+    end
   end
 
   context "#remove" do
@@ -224,52 +256,9 @@
       }.to change { subject.order.total }
     end
 
-    context "given an order with existing addresses" do
-      let(:default_address) { create :address, state_code: "NY", zipcode: "17402" }
-      let(:order_with_address ) { create :order, ship_address: default_address, bill_address: default_address }
-
-      subject { described_class.new(order_with_address) }
-
-      context "when an address in a potentially different tax zone is supplied " do
-        let(:updated_address) { build :address, state_code: "AL",  zipcode: "64092" }
-
-        let(:params) do
-          { ship_address_attributes: updated_address.value_attributes, bill_address_attributes: updated_address.value_attributes }
-        end
-
-        it "updates tax adjustments" do
-          expect(subject.order).to receive(:create_tax_charge!)
-          subject.update_cart params
-        end
-      end
-
-      context "when an address in potentially the same tax zone is supplied" do
-        let(:updated_address) { build :address, state_code: "NY", zipcode: "17402", firstname: 'Robert' }
-
-        let(:params) do
-          { ship_address_attributes: updated_address.value_attributes, bill_address_attributes: updated_address.value_attributes }
-        end
-
-        it "does not updates tax adjustments" do
-          expect(subject.order).not_to receive(:create_tax_charge!)
-          subject.update_cart params
-        end
-      end
-    end
-
-    context "given an order with no existing addresses" do
-      context "when an address is supplied" do
-        let(:updated_address) { build :address, state_code: "CA", zipcode: "14902" }
-
-        let(:params) do
-          { ship_address_attributes: updated_address.attributes, bill_address_attributes: updated_address.value_attributes }
-        end
-
-        it "does not updates tax adjustments" do
-          expect(subject.order).not_to receive(:create_tax_charge!)
-          subject.update_cart params
-        end
-      end
+    it "updates tax adjustments" do
+      expect(subject.order).to receive(:create_tax_charge!)
+      subject.update_cart params
     end
 
     context "submits item quantity 0" do