WIP: Remove TaxRate.match

core/app/models/spree/tax_rate.rb

@@ -27,42 +27,13 @@ class TaxRate < Spree::Base
     validates :tax_category_id, presence: true
     validates_with DefaultTaxZoneValidator
 
-    scope :by_zone, ->(zone) { where(zone_id: zone) }
-
     # Gets the array of TaxRates appropriate for the specified order
-    def self.match(order_tax_zone)
-      return [] unless order_tax_zone
-      rates = includes(zone: { zone_members: :zoneable }).load.select do |rate|
-        # Why "potentially"?
-        # Go see the documentation for that method.
-        rate.potentially_applicable?(order_tax_zone)
-      end
-
-      # 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.
-
-      remaining_rates = rates.dup
-      rates.each do |rate|
-        if rate.included_in_price?
-          if remaining_rates.any?{|r| r != rate && r.tax_category == rate.tax_category }
-            remaining_rates.delete(rate)
+    scope :for_zone,
+          ->(zone) do
+            where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
           end
-        end
-      end
 
-      remaining_rates
-    end
+    scope :included_in_price, -> { where(included_in_price: true) }
 
     # Pre-tax amounts must be stored so that we can calculate
     # correct rate amounts in the future. For example:
@@ -81,35 +52,9 @@ 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 #potentially_applicable?
-    def self.adjust(order_tax_zone, items)
-      rates = self.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
-
     # Tax rates can *potentially* be applicable to an order.
-    # We do not know if they are/aren't 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,
-    # but then has a tax category that doesn't match against any of the line items
-    # inside of the order, then that tax rate will not be applicable to anything.
+    # We do not know if they are/aren't until we check their tax categories - if
+    # they match any of the line item's, they are applicable.
     # For instance:
     #
     # Zones:
@@ -144,11 +89,55 @@ def self.adjust(order_tax_zone, items)
     # 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 potentially_applicable?(order_tax_zone)
-      # If the rate's zone *contains* the order's tax zone, then it's applicable.
-      self.zone.contains?(order_tax_zone) ||
-      # The rate is a VAT and its zone contains the default zone, then it's applicable.
-      (self.included_in_price? && self.zone.contains?(Spree::Zone.default_tax))
+    def self.adjust(order_tax_zone, items)
+      # Early return to make sure nothing happens if there's no tax zone on
+      # the order.
+      return unless order_tax_zone
+
+      # Destroy all tax adjustments using destroy_all to ensure callbacks fire.
+      Spree::Adjustment.where(adjustable: items).tax.destroy_all
+
+      # TODO: The whole pre_tax_amount stuff is so unnecessary once prices are right.
+      items.each { |item| item.update_column(:pre_tax_amount, item.discounted_amount) }
+
+      # Find tax rates matching the order's tax zone
+      rates = for_zone(order_tax_zone)
+
+      # Imagine with me this scenario:
+      # You are living in Spain and you have a store which ships the US.
+      # Spain is therefore your default tax rate.
+      # When you ship to Spain, you want the Spanish rate to apply.
+      # When you ship to the US, you want your Spanish rate to be refunded.
+      # This little bit of code adds the default tax zone's VAT rates so #adjust
+      # knows what to refund.
+      #
+      # For further discussion, see #4397 and #4327.
+      if default_tax_zone && !default_tax_zone.contains?(order_tax_zone) && rates.included_in_price.empty?
+        rates += for_zone(default_tax_zone)
+      end
+
+      # Get all tax categories for which we have tax rates
+      tax_categories = rates.map(&:tax_category)
+      # Identify which items have to have a tax rate applied
+
+      relevant_items = items.select do |item|
+        tax_categories.include?(item.tax_category)
+      end
+
+      # For each item,
+      relevant_items.each do |item|
+        # Select the rates with the same tax category
+        relevant_rates = rates.select do |rate|
+          rate.tax_category == item.tax_category
+        end
+        # Store the pre_tax_amount on the item
+        # (incredibly inelegant, this line should go)
+        store_pre_tax_amount(item, relevant_rates)
+        # Have all the relevant rates adjust the item.
+        relevant_rates.each do |rate|
+          rate.adjust(order_tax_zone, item)
+        end
+      end
     end
 
     # Creates necessary tax adjustments for the order.
@@ -173,13 +162,9 @@ def adjust(order_tax_zone, item)
 
     # This method is used by Adjustment#update to recalculate the cost.
     def compute_amount(item)
-      if included_in_price
-        if default_zone_or_zone_match?(item.order.tax_zone)
-          calculator.compute(item)
-        else
-          # In this case, it's a refund.
-          calculator.compute(item) * - 1
-        end
+      if included_in_price && !default_zone_or_zone_match?(item.order.tax_zone)
+        # In this case, it's a refund.
+        calculator.compute(item) * - 1
       else
         calculator.compute(item)
       end
@@ -191,13 +176,16 @@ def default_zone_or_zone_match?(order_tax_zone)
 
     private
 
-      def create_label
-        label = ""
-        label << (name.present? ? name : tax_category.name) + " "
-        label << (show_rate_in_label? ? "#{amount * 100}%" : "")
-        label << " (#{Spree.t(:included_in_price)})" if included_in_price?
-        label
-      end
+    def self.default_tax_zone
+      @_default_tax_zone = Spree::Zone.default_tax
+    end
 
+    def create_label
+      label = ""
+      label << (name.present? ? name : tax_category.name) + " "
+      label << (show_rate_in_label? ? "#{amount * 100}%" : "")
+      label << " (#{Spree.t(:included_in_price)})" if included_in_price?
+      label
+    end
   end
 end

core/app/models/spree/zone.rb

@@ -15,6 +15,18 @@ class Zone < Spree::Base
     after_save :remove_defunct_members
     after_save :remove_previous_default
 
+    scope :with_member_ids,
+          ->(state_ids, country_ids) do
+            joins(:zone_members).where(
+              "(spree_zone_members.zoneable_type = 'Spree::State' AND
+                 spree_zone_members.zoneable_id IN (?))
+               OR (spree_zone_members.zoneable_type = 'Spree::Country' AND
+                 spree_zone_members.zoneable_id IN (?))",
+              state_ids,
+              country_ids
+            ).uniq
+          end
+
     alias :members :zone_members
     accepts_nested_attributes_for :zone_members, allow_destroy: true, reject_if: proc { |a| a['zoneable_id'].blank? }
 
@@ -24,12 +36,13 @@ def self.default_tax
       where(default_tax: true).first
     end
 
-    # Returns the matching zone with the highest priority zone type (State, Country, Zone.)
-    # Returns nil in the case of no matches.
+    # Returns the most specific matching zone for an address. Specific means:
+    # A State zone wins over a country zone, and a zone with few members wins
+    # over one with many members. If there is no match, returns nil.
     def self.match(address)
-      return unless address and matches = self.includes(:zone_members).
+      return unless address and matches = self.
+        with_member_ids(address.state_id, address.country_id).
         order(:zone_members_count, :created_at, :id).
-        where("(spree_zone_members.zoneable_type = 'Spree::Country' AND spree_zone_members.zoneable_id = ?) OR (spree_zone_members.zoneable_type = 'Spree::State' AND spree_zone_members.zoneable_id = ?)", address.country_id, address.state_id).
         references(:zones)
 
       ['state', 'country'].each do |zone_kind|
@@ -40,6 +53,18 @@ def self.match(address)
       matches.first
     end
 
+
+    # Returns all zones that contain any of the zone members of the zone passed
+    # in. This also includes any country zones that contain the state of the
+    # 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)
+      with_member_ids(
+        zone.states.pluck(:id),
+        zone.states.pluck(:country_id) + zone.countries.pluck(:id)
+      ).uniq
+    end
+
     def kind
       if members.any? && !members.any? { |member| member.try(:zoneable_type).nil? }
         members.last.zoneable_type.demodulize.underscore

core/spec/models/spree/reimbursement_spec.rb

@@ -50,7 +50,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

@@ -25,7 +25,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
@@ -38,13 +38,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