Add a PORO for creating tax adjustments

core/app/models/spree/line_item.rb

@@ -173,7 +173,7 @@ def recalculate_adjustments
     end
 
     def update_tax_charge
-      Spree::TaxRate.adjust(order.tax_zone, [self])
+      Spree::Tax::ItemAdjuster.new(self).adjust!
     end
 
     def ensure_proper_currency

core/app/models/spree/order.rb

@@ -321,10 +321,7 @@ def line_item_options_match(line_item, options)
     # Creates new tax charges if there are any applicable rates. If prices already
     # include taxes then price adjustments are created instead.
     def create_tax_charge!
-      # We want to only look up the applicable tax zone once and pass it to TaxRate calculation to avoid duplicated lookups.
-      order_tax_zone = tax_zone
-      Spree::TaxRate.adjust(order_tax_zone, line_items)
-      Spree::TaxRate.adjust(order_tax_zone, shipments) if shipments.any?
+      Spree::Tax::OrderAdjuster.new(self).adjust!
     end
 
     def outstanding_balance

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

@@ -0,0 +1,42 @@
+module Spree
+  module Tax
+    # Adjust a single taxable item (line item or shipment)
+    class ItemAdjuster
+      attr_reader :item, :order
+
+      include TaxHelpers
+
+      # @param [Spree::LineItem,Spree::Shipment] item to adjust
+      # @param [Hash] options like already known tax rates for the order's zone
+      def initialize(item, options = {})
+        @item = item
+        @order = @item.order
+        # set instance variable so `TaxRate.match` is only called when necessary
+        @rates_for_order_zone = options[:rates_for_order_zone]
+      end
+
+      # Deletes all existing tax adjustments and creates new adjustments for all
+      # (geographically and category-wise) applicable tax rates.
+      #
+      # Creating the adjustments will also run the ItemAdjustments class and
+      # persist all taxation and promotion totals on the item.
+      #
+      # @return [Array<Spree::Adjustment>] newly created adjustments
+      def adjust!
+        return unless order_tax_zone
+        # Using .destroy_all to make sure callbacks fire
+        item.adjustments.tax.destroy_all
+
+        TaxRate.store_pre_tax_amount(item, rates_for_item)
+
+        rates_for_item.map { |rate| rate.adjust(order_tax_zone, item) }
+      end
+
+      private
+
+      def rates_for_item
+        @rates_for_item ||= applicable_rates.select { |rate| rate.tax_category == item.tax_category }
+      end
+    end
+  end
+end

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

@@ -0,0 +1,25 @@
+module Spree
+  module Tax
+    # Add tax adjustments to all line items and shipments in an order
+    class OrderAdjuster
+      attr_reader :order
+
+      include TaxHelpers
+
+      # @param [Spree::Order] order to be adjusted
+      def initialize(order)
+        @order = order
+      end
+
+      # Creates tax adjustments for all taxable items (shipments and line items)
+      # in the given order.
+      def adjust!
+        return unless order_tax_zone
+
+        (order.line_items + order.shipments).each do |item|
+          ItemAdjuster.new(item, rates_for_order_zone: applicable_rates).adjust!
+        end
+      end
+    end
+  end
+end

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

@@ -0,0 +1,41 @@
+module Spree
+  module Tax
+    module TaxHelpers
+      private
+
+      # Imagine with me this scenario:
+      # You are living in Spain and you have a store which ships to France.
+      # Spain is therefore your default tax rate.
+      # When you ship to Spain, you want the Spanish rate to apply.
+      # When you ship to France, you want the French rate to apply.
+      #
+      # Normally, Spree would notice that you have two potentially applicable
+      # tax rates for one particular item.
+      # When you ship to Spain, only the Spanish one will apply.
+      # When you ship to France, you'll see a Spanish refund AND a French tax.
+      # This little bit of code at the end stops the Spanish refund from appearing.
+      #
+      # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
+      def applicable_rates
+        order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
+        default_rates_with_unmatched_tax_category = rates_for_default_zone.to_a.delete_if do |default_rate|
+          order_zone_tax_categories.include?(default_rate.tax_category)
+        end
+
+        (rates_for_order_zone + default_rates_with_unmatched_tax_category).uniq
+      end
+
+      def rates_for_order_zone
+        @rates_for_order_zone ||= Spree::TaxRate.for_zone(order_tax_zone)
+      end
+
+      def rates_for_default_zone
+        @rates_for_default_zone ||= Spree::TaxRate.for_zone(Spree::Zone.default_tax)
+      end
+
+      def order_tax_zone
+        @order_tax_zone ||= order.tax_zone
+      end
+    end
+  end
+end

core/app/models/spree/tax_rate.rb

@@ -27,9 +27,7 @@ class TaxRate < Spree::Base
     validates :tax_category_id, presence: true
     validates_with DefaultTaxZoneValidator
 
-    scope :by_zone, ->(zone) { where(zone_id: zone) }
-
-    # Finds geographically matching tax rates for an order's tax zone.
+    # Finds geographically matching tax rates for a tax zone.
     # We do not know if they are/aren't applicable until we attempt to apply these rates to
     # the items contained within the Order itself.
     # For instance, if a rate passes the criteria outlined in this method,
@@ -69,33 +67,20 @@ class TaxRate < Spree::Base
     # Under no circumstances should negative adjustments be applied for the Spanish tax rates.
     #
     # Those rates should never come into play at all and only the French rates should apply.
-    def self.match(order_tax_zone)
-      return [] unless order_tax_zone
-      all_rates = includes(zone: { zone_members: :zoneable }).load
-
-      rates_for_order_zone = all_rates.select { |rate| rate.zone.contains?(order_tax_zone) }
-      rates_for_default_zone = all_rates.select(&:default_vat?)
-
-      # Imagine with me this scenario:
-      # You are living in Spain and you have a store which ships to France.
-      # Spain is therefore your default tax rate.
-      # When you ship to Spain, you want the Spanish rate to apply.
-      # When you ship to France, you want the French rate to apply.
-      #
-      # Normally, Spree would notice that you have two potentially applicable
-      # tax rates for one particular item.
-      # When you ship to Spain, only the Spanish one will apply.
-      # When you ship to France, you'll see a Spanish refund AND a French tax.
-      # This little bit of code at the end stops the Spanish refund from appearing.
-      #
-      # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
-
-      order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
-      rates_for_default_zone.delete_if do |default_rate|
-        order_zone_tax_categories.include?(default_rate.tax_category)
-      end
+    scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
+    scope :included_in_price, -> { where(included_in_price: true) }
 
-      (rates_for_order_zone + rates_for_default_zone).uniq
+    # Create tax adjustments for some items that have the same tax zone.
+    #
+    # @deprecated Please use Spree::Tax::OrderAdjuster or Spree::Tax::ItemAdjuster instead.
+    #
+    # @param [Spree::Zone] order_tax_zone is the smalles applicable zone to the order's tax address
+    # @param [Array<Spree::LineItem,Spree::Shipment>] items to be adjusted
+    def self.adjust(order_tax_zone, items)
+      ActiveSupport::Deprecation.warn("Please use Spree::Tax::OrderAdjuster or Spree::Tax::ItemAdjuster instead", caller)
+      items.map do |item|
+        Spree::Tax::ItemAdjuster.new(item, rates_for_order_zone: for_zone(order_tax_zone)).adjust!
+      end
     end
 
     # Pre-tax amounts must be stored so that we can calculate
@@ -115,29 +100,6 @@ def self.store_pre_tax_amount(item, rates)
       item.update_column(:pre_tax_amount, pre_tax_amount.round(2))
     end
 
-    # This method is best described by the documentation on .match
-    def self.adjust(order_tax_zone, items)
-      rates = match(order_tax_zone)
-      tax_categories = rates.map(&:tax_category)
-      relevant_items, non_relevant_items = items.partition { |item| tax_categories.include?(item.tax_category) }
-      unless relevant_items.empty?
-        Spree::Adjustment.where(adjustable: relevant_items).tax.destroy_all # using destroy_all to ensure adjustment destroy callback fires.
-      end
-      relevant_items.each do |item|
-        relevant_rates = rates.select { |rate| rate.tax_category == item.tax_category }
-        store_pre_tax_amount(item, relevant_rates)
-        relevant_rates.each do |rate|
-          rate.adjust(order_tax_zone, item)
-        end
-      end
-      non_relevant_items.each do |item|
-        if item.adjustments.tax.present?
-          item.adjustments.tax.destroy_all # using destroy_all to ensure adjustment destroy callback fires.
-          item.update_columns pre_tax_amount: 0
-        end
-      end
-    end
-
     # Creates necessary tax adjustments for the order.
     def adjust(order_tax_zone, item)
       amount = compute_amount(item)
@@ -172,10 +134,6 @@ def default_zone_or_zone_match?(order_tax_zone)
       Zone.default_tax.try!(:contains?, order_tax_zone) || zone.contains?(order_tax_zone)
     end
 
-    def default_vat?
-      included_in_price && zone.contains?(Spree::Zone.default_tax)
-    end
-
     private
 
     def create_label

core/app/models/spree/zone.rb

@@ -58,6 +58,8 @@ def self.match(address)
     # current zone, if it's a state zone. If the passed-in zone has members, it
     # will also be in the result set.
     def self.with_shared_members(zone)
+      return none unless zone
+
       states_and_state_country_ids = zone.states.pluck(:id, :country_id).to_a
       state_ids = states_and_state_country_ids.map(&:first)
       state_country_ids = states_and_state_country_ids.map(&:second)

core/spec/models/spree/order/state_machine_spec.rb

@@ -43,7 +43,7 @@
       end
 
       it "adjusts tax rates when transitioning to delivery" do
-        expect(Spree::TaxRate).to receive(:adjust).once.with(order.tax_zone, order.line_items)
+        expect(Spree::Tax::OrderAdjuster).to receive(:new).once.with(order).and_call_original
         order.next!
       end
     end

core/spec/models/spree/reimbursement_spec.rb

@@ -49,7 +49,7 @@
     let!(:adjustments)            { [] } # placeholder to ensure it gets run prior the "before" at this level
 
     let!(:tax_rate)               { nil }
-    let!(:tax_zone)               { create :zone, default_tax: true }
+    let!(:tax_zone)               { create :zone, :with_country, default_tax: true }
     let(:shipping_method)         { create :shipping_method, zones: [tax_zone] }
     let(:variant)                 { create :variant }
     let(:order)                   { create(:order_with_line_items, state: 'payment', line_items_attributes: [{ variant: variant, price: line_items_price }], shipment_cost: 0, shipping_method: shipping_method) }

core/spec/models/spree/reimbursement_tax_calculator_spec.rb

@@ -24,7 +24,7 @@
 
   context 'with additional tax' do
     let!(:tax_rate) { create(:tax_rate, name: "Sales Tax", amount: 0.10, included_in_price: false, zone: tax_zone) }
-    let(:tax_zone) { create(:zone, default_tax: true) }
+    let(:tax_zone) { create(:zone, :with_country, default_tax: true) }
 
     it 'sets additional_tax_total on the return items' do
       subject
@@ -37,13 +37,13 @@
 
   context 'with included tax' do
     let!(:tax_rate) { create(:tax_rate, name: "VAT Tax", amount: 0.1, included_in_price: true, zone: tax_zone) }
-    let(:tax_zone) { create(:zone, default_tax: true) }
+    let(:tax_zone) { create(:zone, :with_country, default_tax: true) }
 
     it 'sets included_tax_total on the return items' do
       subject
       return_item.reload
 
-      expect(return_item.included_tax_total).to be < 0
+      expect(return_item.included_tax_total).to be > 0
       expect(return_item.included_tax_total).to eq line_item.included_tax_total
     end
   end

core/spec/models/spree/tax/item_adjuster_spec.rb

@@ -0,0 +1,76 @@
+require 'spec_helper'
+
+RSpec.describe Spree::Tax::ItemAdjuster do
+  subject(:adjuster) { described_class.new(item) }
+  let(:order) { Spree::Order.new }
+  let(:item) { Spree::LineItem.new(order: order) }
+
+  before do
+    allow(order).to receive(:tax_zone) { build(:zone) }
+  end
+
+  describe 'initialization' do
+    it 'sets order to item order' do
+      expect(adjuster.order).to eq(item.order)
+    end
+
+    it 'sets adjustable' do
+      expect(adjuster.item).to eq(item)
+    end
+  end
+
+  describe '#adjust!' do
+    before do
+      expect(order).to receive(:tax_zone).and_return(tax_zone)
+    end
+
+    context 'when the order has no tax zone' do
+      let(:tax_zone) { nil }
+
+      before do
+        allow(order).to receive(:tax_zone).and_return(nil)
+        adjuster.adjust!
+      end
+
+      it 'returns nil early' do
+        expect(adjuster.adjust!).to be_nil
+      end
+    end
+
+    context 'when the order has a tax zone' do
+      let(:item) { build_stubbed :line_item, order: order }
+      let(:tax_zone) { build_stubbed(:zone, :with_country) }
+
+      before do
+        expect(item).to receive(:update_column)
+
+        expect(Spree::TaxRate).to receive(:for_zone).with(tax_zone).and_return(rates_for_order_zone)
+        expect(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
+      end
+
+      context 'when there are no matching rates' do
+        let(:rates_for_order_zone) { [] }
+
+        it 'returns no adjustments' do
+          expect(adjuster.adjust!).to eq([])
+        end
+      end
+
+      context 'when there are matching rates for the zone' do
+        context 'and all rates have the same tax category as the item' do
+          let(:item_tax_category) { build_stubbed(:tax_category) }
+          let(:rate_1) { create :tax_rate, tax_category: item_tax_category }
+          let(:rate_2) { create :tax_rate }
+          let(:rates_for_order_zone) { [rate_1, rate_2] }
+
+          before { allow(item).to receive(:tax_category).and_return(item_tax_category) }
+
+          it 'creates an adjustment for every matching rate' do
+            expect(rate_1).to receive_message_chain(:adjustments, :create!)
+            expect(adjuster.adjust!.length).to eq(1)
+          end
+        end
+      end
+    end
+  end
+end

core/spec/models/spree/tax/order_adjuster_spec.rb

@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+RSpec.describe Spree::Tax::OrderAdjuster do
+  subject(:adjuster) { described_class.new(order) }
+
+  describe 'initialization' do
+    let(:order) { Spree::Order.new }
+
+    it 'sets order to adjustable' do
+      expect(adjuster.order).to eq(order)
+    end
+  end
+
+  describe '#adjust!' do
+    let(:zone) { build_stubbed(:zone) }
+    let(:line_items) { build_stubbed_list(:line_item, 2) }
+    let(:order) { build_stubbed(:order, line_items: line_items) }
+    let(:rates_for_order_zone) { [] }
+    let(:item_adjuster) { Spree::Tax::ItemAdjuster.new(line_items.first) }
+
+    before do
+      expect(order).to receive(:tax_zone).at_least(:once).and_return(zone)
+      expect(Spree::TaxRate).to receive(:for_zone).with(zone).and_return(rates_for_order_zone)
+      expect(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
+    end
+
+    it 'calls the item adjuster with all line items' do
+      expect(Spree::Tax::ItemAdjuster).to receive(:new).with(line_items.first, rates_for_order_zone: rates_for_order_zone).and_return(item_adjuster)
+      expect(Spree::Tax::ItemAdjuster).to receive(:new).with(line_items.second, rates_for_order_zone: rates_for_order_zone).and_return(item_adjuster)
+
+      expect(item_adjuster).to receive(:adjust!).twice
+      adjuster.adjust!
+    end
+  end
+end