From 47fca81ca77e87904030c4f5079fc4eb2662497d Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Wed, 18 Nov 2015 22:44:36 +0100
Subject: [PATCH 01/15] Remove default tax zone validation

With the new VAT taxation system, a default zone
is no longer mandatory. It is perfectly valid to have
taxes that are included in price only for specific foreign zones.
---
 core/app/models/spree/tax_rate.rb | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 88118f0c9a7..9d8f8e54bda 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -1,13 +1,3 @@
-module Spree
-  class DefaultTaxZoneValidator < ActiveModel::Validator
-    def validate(record)
-      if record.included_in_price
-        record.errors.add(:included_in_price, Spree.t(:included_price_validation)) unless Zone.default_tax
-      end
-    end
-  end
-end
-
 module Spree
   class TaxRate < Spree::Base
     acts_as_paranoid
@@ -25,7 +15,6 @@ class TaxRate < Spree::Base
 
     validates :amount, presence: true, numericality: true
     validates :tax_category_id, presence: true
-    validates_with DefaultTaxZoneValidator
 
     scope :by_zone, ->(zone) { where(zone_id: zone) }
 

From 87b5230d42ee14b0bb7218c94b427cbbece6bd76 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Fri, 20 Nov 2015 19:44:33 +0100
Subject: [PATCH 02/15] Add Spree::Zone.with_shared_members(zone)

This class method returns all those zones that have at least one member
in common (even via a state that is part of a country), as well as
itself.
---
 core/app/models/spree/zone.rb       |  23 +++++++
 core/spec/models/spree/zone_spec.rb | 102 ++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb
index 31e41e217d5..06c1b87f094 100644
--- a/core/app/models/spree/zone.rb
+++ b/core/app/models/spree/zone.rb
@@ -3,6 +3,11 @@ class Zone < Spree::Base
     has_many :zone_members, dependent: :destroy, class_name: "Spree::ZoneMember", inverse_of: :zone
     has_many :tax_rates, dependent: :destroy, inverse_of: :zone
 
+    with_options through: :zone_members, source: :zoneable do
+      has_many :countries, source_type: "Spree::Country"
+      has_many :states, source_type: "Spree::State"
+    end
+
     has_many :shipping_method_zones
     has_many :shipping_methods, through: :shipping_method_zones
 
@@ -35,6 +40,24 @@ def self.match(address)
       matches.first
     end
 
+    # The class method `.with_shared_members` returns all zones that also
+    # 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.
+    # It will also return itself.
+    def self.with_shared_members(zone)
+      joins('LEFT OUTER JOIN "spree_zone_members"
+              ON "spree_zone_members"."zone_id" = "spree_zones"."id"')
+        .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 (?))
+                OR (spree_zones.id = ?)",
+        zone.states.pluck(:id),
+        zone.states.pluck(:country_id) + zone.countries.pluck(:id),
+        zone.id
+      ).uniq
+    end
+
     def kind
       if members.any? && !members.any? { |member| member.try(:zoneable_type).nil? }
         members.last.zoneable_type.demodulize.underscore
diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb
index f9a76afcc81..396a3ef0a6c 100644
--- a/core/spec/models/spree/zone_spec.rb
+++ b/core/spec/models/spree/zone_spec.rb
@@ -302,4 +302,106 @@
       end
     end
   end
+
+  context ".with_shared_members" do
+    let!(:country)  { create(:country) }
+    let!(:country2) { create(:country, name: 'OtherCountry') }
+    let!(:country3) { create(:country, name: 'TaxCountry') }
+
+    subject(:zones_with_shared_members) { Spree::Zone.with_shared_members(zone) }
+
+    context 'when passing a zone with no members' do
+      let!(:zone) { create :zone }
+
+      it 'will return itself' do
+        expect(subject).to include(zone)
+      end
+    end
+
+    context "finding potential matches for a country zone" do
+      let!(:zone) do
+        create(:zone).tap do |z|
+          z.members.create(zoneable: country)
+          z.members.create(zoneable: country2)
+          z.save!
+        end
+      end
+
+      let!(:zone2) do
+        create(:zone).tap { |z| z.members.create(zoneable: country) && z.save! }
+      end
+
+      let!(:zone3) do
+        create(:zone).tap { |z| z.members.create(zoneable: country3) && z.save! }
+      end
+
+      it "will find all zones with countries covered by the passed in zone" do
+        expect(zones_with_shared_members).to include(zone, zone2)
+      end
+
+      it "will not return zones with countries not covered in the passed in zone" do
+        expect(zones_with_shared_members).not_to include(zone3)
+      end
+
+      it "only returns each zone once" do
+        expect(zones_with_shared_members.select { |z| z == zone }.size).to be 1
+      end
+    end
+
+    context "finding potential matches for a state zone" do
+      let!(:state)  { create(:state, country: country) }
+      let!(:state2) { create(:state, country: country2, name: 'OtherState') }
+      let!(:state3) { create(:state, country: country2, name: 'State') }
+      let!(:zone) do
+        create(:zone).tap do |z|
+          z.members.create(zoneable: state)
+          z.members.create(zoneable: state2)
+          z.save!
+        end
+      end
+      let!(:zone2) do
+        create(:zone).tap { |z| z.members.create(zoneable: state) && z.save! }
+      end
+      let!(:zone3) do
+        create(:zone).tap { |z| z.members.create(zoneable: state2) && z.save! }
+      end
+
+      it "will find all zones which share states covered by passed in zone" do
+        expect(zones_with_shared_members).to include(zone, zone2)
+      end
+
+      it "will find zones that share countries with any states of the passed in zone" do
+        expect(zones_with_shared_members).to include(zone3)
+      end
+
+      it "only returns each zone once" do
+        expect(zones_with_shared_members.select { |z| z == zone }.size).to be 1
+      end
+    end
+
+    context "state and country associations" do
+      let!(:country)  { create(:country) }
+
+      context "has countries associated" do
+        let!(:zone) do
+          create(:zone, countries: [country])
+        end
+
+        it "can access associated countries" do
+          expect(zone.countries).to include(country)
+        end
+      end
+
+      context "has states associated" do
+        let!(:state)  { create(:state, country: country) }
+        let!(:zone) do
+          create(:zone, states: [state])
+        end
+
+        it "can access associated states" do
+          expect(zone.states).to include(state)
+        end
+      end
+    end
+  end
 end

From 709d4b229dc8f3443d3be66f3a426ca3f1fd71bd Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Fri, 20 Nov 2015 20:10:08 +0100
Subject: [PATCH 03/15] Spree::Zone: Move SQL query into scope, linting, docs

Both Spree::Zone.match and Spree::Zone.with_shared_members employ almost
the same SQL for doing what they need to do. Moved into a named scope to
reduce understanding effort.

Improve documentation, follow Rubocop recommendations
---
 core/app/models/spree/zone.rb       | 40 +++++++++++++++++------------
 core/spec/models/spree/zone_spec.rb |  8 ------
 2 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb
index 06c1b87f094..7cb5fce0290 100644
--- a/core/app/models/spree/zone.rb
+++ b/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,21 +53,14 @@ def self.match(address)
       matches.first
     end
 
-    # The class method `.with_shared_members` returns all zones that also
-    # 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.
-    # It will also return itself.
+    # 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)
-      joins('LEFT OUTER JOIN "spree_zone_members"
-              ON "spree_zone_members"."zone_id" = "spree_zones"."id"')
-        .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 (?))
-                OR (spree_zones.id = ?)",
+      with_member_ids(
         zone.states.pluck(:id),
-        zone.states.pluck(:country_id) + zone.countries.pluck(:id),
-        zone.id
+        zone.states.pluck(:country_id) + zone.countries.pluck(:id)
       ).uniq
     end
 
diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb
index 396a3ef0a6c..dfbee1a8e1e 100644
--- a/core/spec/models/spree/zone_spec.rb
+++ b/core/spec/models/spree/zone_spec.rb
@@ -310,14 +310,6 @@
 
     subject(:zones_with_shared_members) { Spree::Zone.with_shared_members(zone) }
 
-    context 'when passing a zone with no members' do
-      let!(:zone) { create :zone }
-
-      it 'will return itself' do
-        expect(subject).to include(zone)
-      end
-    end
-
     context "finding potential matches for a country zone" do
       let!(:zone) do
         create(:zone).tap do |z|

From 643d334dcb336e12d1b7974d575a3c7fbe486b7f Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sat, 21 Nov 2015 20:53:42 +0100
Subject: [PATCH 04/15] Simplify Spree::TaxRate.match

TaxRate.match does some really hard-to-understand calculations at the
end, only so that TaxRate.adjust gets exactly the right array for other
really complex code. This only affects VAT taxation, and the code in
question is, unfortunately, somewhat questionable.

The tests for TaxRate.adjust test the same behaviour. They're xit'ed for
now, but will be added again once I'm done refactoring TaxRate.adjust.
---
 core/app/models/spree/tax_rate.rb       | 19 +++++++-----
 core/spec/models/spree/tax_rate_spec.rb | 39 +++++++------------------
 2 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 9d8f8e54bda..7fbeec2c6a0 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -18,14 +18,19 @@ class TaxRate < Spree::Base
 
     scope :by_zone, ->(zone) { where(zone_id: zone) }
 
+    scope :for_zone, -> (zone) do
+      where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
+    end
+
     # 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|
+      for_zone(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
+        # 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.
@@ -40,10 +45,10 @@ def self.match(order_tax_zone)
       # This little bit of code at the end stops the Spanish refund from appearing.
       #
       # For further discussion, see #4397 and #4327.
-      rates.delete_if do |rate|
-        rate.included_in_price? &&
-        (rates - [rate]).map(&:tax_category).include?(rate.tax_category)
-      end
+      # rates.delete_if do |rate|
+        # rate.included_in_price? &&
+        # (rates - [rate]).map(&:tax_category).include?(rate.tax_category)
+      # end
     end
 
     # Pre-tax amounts must be stored so that we can calculate
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index d28fef36860..805c8582746 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -1,15 +1,17 @@
 require 'spec_helper'
 
 describe Spree::TaxRate, :type => :model do
-  context "match" do
+  context ".match" do
     let(:order) { create(:order) }
     let(:country) { create(:country) }
     let(:tax_category) { create(:tax_category) }
     let(:calculator) { Spree::Calculator::FlatRate.new }
 
+    subject(:tax_rates_for_order) { Spree::TaxRate.match(order.tax_zone) }
+
     it "should return an empty array when tax_zone is nil" do
       allow(order).to receive_messages :tax_zone => nil
-      expect(Spree::TaxRate.match(order.tax_zone)).to eq([])
+      expect(tax_rates_for_order).to eq([])
     end
 
     context "when no rate zones match the tax zone" do
@@ -25,7 +27,7 @@
 
         it "should return an empty array" do
           allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to eq([])
+          expect(tax_rates_for_order).to eq([])
         end
 
         it "should return the rate that matches the rate zone" do
@@ -37,7 +39,7 @@
           )
 
           allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to eq([rate])
+          expect(tax_rates_for_order).to eq([rate])
         end
 
         it "should return all rates that match the rate zone" do
@@ -56,7 +58,7 @@
           )
 
           allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to match_array([rate1, rate2])
+          expect(tax_rates_for_order).to match_array([rate1, rate2])
         end
 
         context "when the tax_zone is contained within a rate zone" do
@@ -73,7 +75,7 @@
           end
 
           it "should return the rate zone" do
-            expect(Spree::TaxRate.match(order.tax_zone)).to eq([@rate])
+            expect(tax_rates_for_order).to eq([@rate])
           end
         end
       end
@@ -93,8 +95,6 @@
                                 :included_in_price => included_in_price)
         end
 
-        subject { Spree::TaxRate.match(order.tax_zone) }
-
         context "when the order has the same tax zone" do
           before do
             allow(order).to receive_messages :tax_zone => @zone
@@ -122,13 +122,6 @@
           context "when the order has a tax_address" do
             let(:tax_address) { stub_model(Spree::Address) }
 
-            context "when the tax is a VAT" do
-              let(:included_in_price) { true }
-              # The rate should match in this instance because:
-              # 1) It's the default rate (and as such, a negative adjustment should apply)
-              it { is_expected.to eq([rate]) }
-            end
-
             context "when the tax is not VAT" do
               it "returns no tax rate" do
                 expect(subject).to be_empty
@@ -139,16 +132,6 @@
           context "when the order does not have a tax_address" do
             let(:tax_address) { nil}
 
-            context "when the tax is a VAT" do
-              let(:included_in_price) { true }
-              # The rate should match in this instance because:
-              # 1) The order has no tax address by this stage
-              # 2) With no tax address, it has no tax zone
-              # 3) Therefore, we assume the default tax zone
-              # 4) This default zone has a default tax rate.
-              it { is_expected.to eq([rate]) }
-            end
-
             context "when the tax is not a VAT" do
               it { is_expected.to be_empty }
             end
@@ -252,7 +235,7 @@
         end
 
         context "when zone is contained by default tax zone" do
-          it "should create two adjustments, one for each tax rate" do
+          xit "should create two adjustments, one for each tax rate" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.count).to eq(1)
           end
@@ -270,7 +253,7 @@
             # Zone.stub_chain :default_tax, :contains? => false
             @zone.zone_members.delete_all
           end
-          it "should create an adjustment" do
+          xit "should create an adjustment" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.charge.count).to eq(1)
           end
@@ -295,7 +278,7 @@
             expect(line_item.adjustments.charge.count).to eq(0)
           end
 
-          it "should create a tax refund for each tax rate" do
+          xit "should create a tax refund for each tax rate" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.credit.count).to eq(1)
           end

From 08e9270373a8f82a66fb622d83eb830a171dd044 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sat, 21 Nov 2015 20:57:20 +0100
Subject: [PATCH 05/15] Remove TaxRate#potentially_applicable

Half this method is now covered by Spree::Zone.with_shared_members. The
other half has to come from refactoring Spree::TaxRate.adjust.
---
 core/app/models/spree/tax_rate.rb | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 7fbeec2c6a0..f20042f9ba5 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -131,14 +131,7 @@ 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 matches the order's tax zone, then it's applicable.
-      self.zone == order_tax_zone ||
-      # If the rate's zone *contains* the order's tax zone, then it's applicable.
-      self.zone.contains?(order_tax_zone) ||
-      # 1) The rate's zone is the default zone, then it's always applicable.
-      (self.included_in_price? && self.zone.default_tax)
-    end
+
 
     # Creates necessary tax adjustments for the order.
     def adjust(order_tax_zone, item)

From d18b1b8d92ca963efb7f08dc03b62cc8d67cf9ca Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sat, 21 Nov 2015 21:05:51 +0100
Subject: [PATCH 06/15] Refactor Spree::TaxRate.adjust - Step 1

---
 core/app/models/spree/tax_rate.rb | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index f20042f9ba5..f6b3f74defb 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -70,25 +70,25 @@ def self.store_pre_tax_amount(item, rates)
 
     # This method is best described by the documentation on #potentially_applicable?
     def self.adjust(order_tax_zone, items)
-      rates = self.match(order_tax_zone)
+      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.
+
+      # using destroy_all to ensure adjustment destroy callback fires.
+      Spree::Adjustment.where(adjustable: items).tax.destroy_all
+
+      relevant_items = items.select do |item|
+        tax_categories.include?(item.tax_category)
       end
+
       relevant_items.each do |item|
-        relevant_rates = rates.select { |rate| rate.tax_category == item.tax_category }
+        relevant_rates = rates.select do |rate|
+          rate.tax_category == item.tax_category
+        end
         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.

From 0b25daddf0dfdd85fa08406477e78f800f6f9d3b Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 13:16:26 +0100
Subject: [PATCH 07/15] Add commentary to TaxRate.adjust

Move the comments from TaxRate#potentially_applicable to TaxRate.adjust
and comment every line in the refactored adjust method.
---
 core/app/models/spree/tax_rate.rb | 52 +++++++++++++++++--------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index f6b3f74defb..e105dc74772 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -68,29 +68,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 #potentially_applicable?
-    def self.adjust(order_tax_zone, items)
-      rates = match(order_tax_zone)
-      tax_categories = rates.map(&:tax_category)
-
-      # using destroy_all to ensure adjustment destroy callback fires.
-      Spree::Adjustment.where(adjustable: items).tax.destroy_all
-
-      relevant_items = items.select do |item|
-        tax_categories.include?(item.tax_category)
-      end
-
-      relevant_items.each do |item|
-        relevant_rates = rates.select do |rate|
-          rate.tax_category == item.tax_category
-        end
-        store_pre_tax_amount(item, relevant_rates)
-        relevant_rates.each do |rate|
-          rate.adjust(order_tax_zone, item)
-        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.
@@ -131,7 +108,36 @@ 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 self.adjust(order_tax_zone, items)
+      # Destroy all tax adjustments using destroy_all to ensure adjustment destroy callback fires.
+      Spree::Adjustment.where(adjustable: items).tax.destroy_all
+      items.each { |item| item.update_column(:pre_tax_amount, 0) }
+
+      # Find tax rates matching the order's tax zone
+      rates = match(order_tax_zone)
+      # 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
+      # I think this could be done with an `items.join(:tax_category)` How?
+      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.
     def adjust(order_tax_zone, item)

From da8fc8bb16ca9108d55beeff8da7bdd5153dc163 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 15:39:48 +0100
Subject: [PATCH 08/15] Re-introduce and extract the bizarre tax rate exclusion
 thing

---
 core/app/models/spree/tax_rate.rb       | 62 +++++++++++++++----------
 core/spec/models/spree/tax_rate_spec.rb |  8 +++-
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index e105dc74772..6afcb2e3ff5 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -22,33 +22,16 @@ class TaxRate < Spree::Base
       where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
     end
 
+    scope :included_in_price, -> { where(included_in_price: true) }
+
     # Gets the array of TaxRates appropriate for the specified order
     def self.match(order_tax_zone)
       return [] unless order_tax_zone
-      for_zone(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 #4397 and #4327.
-      # rates.delete_if do |rate|
-        # rate.included_in_price? &&
-        # (rates - [rate]).map(&:tax_category).include?(rate.tax_category)
-      # end
+      if default_tax_zone
+        (for_zone(order_tax_zone) + for_zone(default_tax_zone).included_in_price).uniq
+      else
+        for_zone(order_tax_zone)
+      end
     end
 
     # Pre-tax amounts must be stored so that we can calculate
@@ -111,10 +94,17 @@ def self.store_pre_tax_amount(item, rates)
     def self.adjust(order_tax_zone, items)
       # Destroy all tax adjustments using destroy_all to ensure adjustment destroy callback fires.
       Spree::Adjustment.where(adjustable: items).tax.destroy_all
+      # TODO: Make sure items is always an AR relation and use `update_columns`
+      # I think the main thing to be done here is adapting the tests, which use arrays.
       items.each { |item| item.update_column(:pre_tax_amount, 0) }
 
       # Find tax rates matching the order's tax zone
       rates = match(order_tax_zone)
+
+      # This is a bizarre bit of code. I've extracted it for now so it doesn't
+      # clutter that badly.
+      rates = exclude_default_zone_vat_rates_if_vat_rate_present(rates) if default_tax_zone
+
       # 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
@@ -180,6 +170,30 @@ def default_zone_or_zone_match?(order_tax_zone)
 
     private
 
+    def self.default_tax_zone
+      @_default_tax_zone = Spree::Zone.default_tax
+    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 #4397 and #4327.
+    def self.exclude_default_zone_vat_rates_if_vat_rate_present(rates)
+      rates.delete_if do |rate|
+        rate.included_in_price? &&
+        (rates - [rate]).map(&:tax_category).include?(rate.tax_category)
+      end
+    end
+
       def create_label
         label = ""
         label << (name.present? ? name : tax_category.name) + " "
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 805c8582746..6f6c7a93a47 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -235,7 +235,7 @@
         end
 
         context "when zone is contained by default tax zone" do
-          xit "should create two adjustments, one for each tax rate" do
+          it "should create two adjustments, one for each tax rate" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.count).to eq(1)
           end
@@ -253,6 +253,10 @@
             # Zone.stub_chain :default_tax, :contains? => false
             @zone.zone_members.delete_all
           end
+
+          # The test setup for this one is just plain bizarre, and I suspect
+          # that's why it fails. Xited for now.
+          # TODO: Rewrite the test setup for all of these.
           xit "should create an adjustment" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.charge.count).to eq(1)
@@ -278,7 +282,7 @@
             expect(line_item.adjustments.charge.count).to eq(0)
           end
 
-          xit "should create a tax refund for each tax rate" do
+          it "should create a tax refund for each tax rate" do
             Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.credit.count).to eq(1)
           end

From 281e788233958b1a0d226fc2106a1c241f8f4250 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 17:40:31 +0100
Subject: [PATCH 09/15] Refactor tests for Spree::TaxRate#adjust

Especially the ones for prices that include tax (aka VAT) were rather
hard to work with (very long context lines, lots of instance variables
instead of `let`s etc.).

Changed that and `xit`ed those that fail. They fail because of bugs in
the VAT code. Before, they had the right descriptions and wrong
expectations.
---
 core/spec/models/spree/tax_rate_spec.rb | 275 +++++++++++++-----------
 1 file changed, 154 insertions(+), 121 deletions(-)

diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 6f6c7a93a47..cd9d1a6a270 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -187,176 +187,209 @@
   end
 
   context "#adjust" do
-    before do
-      @country = create(:country)
-      @zone = create(:zone, :name => "Country Zone", :default_tax => true, :zone_members => [])
-      @zone.zone_members.create(:zoneable => @country)
-      @category    = Spree::TaxCategory.create :name => "Taxable Foo"
-      @category2   = Spree::TaxCategory.create(:name => "Non Taxable")
-      @rate1        = Spree::TaxRate.create(
-        :amount => 0.10,
-        :calculator => Spree::Calculator::DefaultTax.create,
-        :tax_category => @category,
-        :zone => @zone
+    let!(:country) { create(:country) }
+    let!(:taxables_category) { create(:tax_category, name: "Taxable Foo") }
+    let!(:non_taxables_category) { create(:tax_category, name: "Non Taxable") }
+    let!(:zone) { create(:zone, countries: [country]) }
+    let!(:rate1) do
+      create(
+        :tax_rate,
+        tax_category: taxables_category,
+        zone: zone,
+        amount: 0.1
       )
-      @rate2       = Spree::TaxRate.create(
-        :amount => 0.05,
-        :calculator => Spree::Calculator::DefaultTax.create,
-        :tax_category => @category,
-        :zone => @zone
+    end
+    let!(:rate2) do
+      create(
+        :tax_rate,
+        tax_category: taxables_category,
+        zone: zone,
+        amount: 0.05
       )
-      @order       = Spree::Order.create!
-      @taxable     = create(:product, :tax_category => @category)
-      @nontaxable  = create(:product, :tax_category => @category2)
     end
+    let(:taxable) { create(:product, tax_category: taxables_category) }
+    let(:non_taxable) { create(:product, tax_category: non_taxables_category) }
+    let!(:order) { Spree::Order.create! }
+    let(:line_item) { order.line_items.last }
+
+    subject(:adjust_order_items) { Spree::TaxRate.adjust(order.tax_zone, order.line_items) }
 
     context "not taxable line item " do
-      let!(:line_item) { @order.contents.add(@nontaxable.master, 1) }
+      before { order.contents.add(non_taxable.master, 1) }
 
-      it "should not create a tax adjustment" do
-        Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-        expect(line_item.adjustments.tax.charge.count).to eq(0)
+      context "with order having no zone" do
+        before { allow(order).to receive(:tax_zone).and_return(nil) }
+
+        it "should not create a tax adjustment" do
+          expect(line_item.adjustments.tax.charge.count).to eq(0)
+          expect { adjust_order_items }.not_to change { line_item.adjustments.tax.charge.count }
+        end
+
+        it "should not create a refund" do
+          expect(line_item.adjustments.credit.count).to eq(0)
+          expect { adjust_order_items }.not_to change { line_item.adjustments.tax.charge.count }
+        end
       end
 
-      it "should not create a refund" do
-        Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-        expect(line_item.adjustments.credit.count).to eq(0)
+      context "with order tax zone being the tax rate's zone" do
+        before { allow(order).to receive(:tax_zone).and_return(zone) }
+
+        it "should not create a tax adjustment" do
+          expect(line_item.adjustments.tax.charge.count).to eq(0)
+          expect { adjust_order_items }.not_to change { line_item.adjustments.tax.charge.count }
+        end
+
+        it "should not create a refund" do
+          expect(line_item.adjustments.credit.count).to eq(0)
+          expect { adjust_order_items }.not_to change { line_item.adjustments.tax.charge.count }
+        end
       end
     end
 
     context "taxable line item" do
-      let!(:line_item) { @order.contents.add(@taxable.master, 1) }
+      before do
+        order.contents.add(taxable.master, 1)
+        # Delete all adjustments created when adding an item so we can observe
+        # changes.
+        line_item.adjustments.destroy_all
+      end
 
       context "when price includes tax" do
-        before do
-          @rate1.update_column(:included_in_price, true)
-          @rate2.update_column(:included_in_price, true)
-          Spree::TaxRate.store_pre_tax_amount(line_item, [@rate1, @rate2])
+        let!(:default_zone) { create(:zone, countries: [country], default_tax: true) }
+
+        let!(:rate1) do
+          create(
+            :tax_rate,
+            tax_category: taxables_category,
+            zone: zone,
+            amount: 0.1,
+            included_in_price: true
+          )
+        end
+        let!(:rate2) do
+          create(
+            :tax_rate,
+            tax_category: taxables_category,
+            zone: zone,
+            amount: 0.05,
+            included_in_price: true
+          )
         end
 
-        context "when zone is contained by default tax zone" do
-          it "should create two adjustments, one for each tax rate" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-            expect(line_item.adjustments.count).to eq(1)
+        context "when order's zone is the default zone" do
+          let!(:zone) { default_zone }
+
+          # This does not work properly. When you have two tax rates from the same
+          # category, one will be deleted. That is not right.
+          xit "should create two adjustments, one for each tax rate" do
+            expect(line_item.adjustments.credit.count).to eq(0)
+            expect { adjust_order_items }.to change { line_item.adjustments.tax.count }.by(2)
           end
 
           it "should not create a tax refund" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
             expect(line_item.adjustments.credit.count).to eq(0)
+            expect { adjust_order_items }.not_to change { line_item.adjustments.tax.credit.count }
           end
-        end
 
-        context "when order's zone is neither the default zone, or included in the default zone, but matches the rate's zone" do
-          before do
-            # With no zone members, this zone will not contain anything
-            # Previously:
-            # Zone.stub_chain :default_tax, :contains? => false
-            @zone.zone_members.delete_all
+          # This does not work either. Both tax rates should be taken into account.
+          xit "price adjustments should be accurate" do
+            included_tax = order.line_item_adjustments.sum(:amount)
+            expect(line_item.pre_tax_amount).to eq(17.38)
+            expect(line_item.pre_tax_amount + included_tax).to eq(line_item.price)
           end
+        end
 
-          # The test setup for this one is just plain bizarre, and I suspect
-          # that's why it fails. Xited for now.
-          # TODO: Rewrite the test setup for all of these.
-          xit "should create an adjustment" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-            expect(line_item.adjustments.charge.count).to eq(1)
+        context "when order's zone is nil" do
+          before do
+            allow(order).to receive(:tax_zone).and_return(nil)
           end
 
-          it "should not create a tax refund for each tax rate" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
+          it "should create no adjustments" do
             expect(line_item.adjustments.credit.count).to eq(0)
+            expect { adjust_order_items }.not_to change { line_item.adjustments.tax.count }
           end
         end
 
-        context "when order's zone does not match default zone, is not included in the default zone, AND does not match the rate's zone" do
-          before do
-            @new_zone = create(:zone, :name => "New Zone", :default_tax => false)
-            @new_country = create(:country, :name => "New Country")
-            @new_zone.zone_members.create(:zoneable => @new_country)
-            @order.ship_address = create(:address, :country => @new_country)
-            @order.save
-          end
-
-          it "should not create positive adjustments" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-            expect(line_item.adjustments.charge.count).to eq(0)
-          end
+        context "when order's zone is another zone" do
+          let(:foreign_country) { create(:country) }
+          let(:foreign_zone) { create(:zone, countries: [foreign_country]) }
 
-          it "should create a tax refund for each tax rate" do
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-            expect(line_item.adjustments.credit.count).to eq(1)
+          before do
+            allow(order).to receive(:tax_zone).and_return(foreign_zone)
           end
-        end
 
-        context "when price does not include tax" do
-          before do
-            allow(@order).to receive_messages :tax_zone => @zone
-            [@rate1, @rate2].each do |rate|
-              rate.included_in_price = false
-              rate.zone = @zone
-              rate.save
+          context "and the foreign zone has VAT rates" do
+            let!(:foreign_vat) do
+              create(
+              :tax_rate,
+              included_in_price: true,
+              tax_category: taxables_category,
+              zone: foreign_zone,
+              amount: 0.21
+              )
             end
-            Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-          end
 
-          it "should delete adjustments for open order when taxrate is deleted" do
-            @rate1.destroy!
-            @rate2.destroy!
-            expect(line_item.adjustments.count).to eq(0)
-          end
+            it "applies the foreign VAT" do
+              expect(line_item.adjustments.credit.count).to eq(0)
+              expect { adjust_order_items }.to change { line_item.adjustments.tax.count }.by(1)
+            end
 
-          it "should not delete adjustments for complete order when taxrate is deleted" do
-            @order.update_column :completed_at, Time.now
-            @rate1.destroy!
-            @rate2.destroy!
-            expect(line_item.adjustments.count).to eq(2)
+            it "does not refund the default zone's VAT" do
+              expect(line_item.adjustments.credit.count).to eq(0)
+              expect { adjust_order_items }.not_to change { line_item.adjustments.charge.count }
+            end
           end
 
-          it "should create an adjustment" do
-            expect(line_item.adjustments.count).to eq(2)
+          context "and the foreign zone does not have VAT rates" do
+            # This does not work. The code removes a VAT rate if
+            # there's more than one VAT of the same category.
+            xit "refunds both default zone's VATs" do
+              expect(line_item.adjustments.count).to eq(0)
+              expect { adjust_order_items }.to change { line_item.adjustments.tax.count }.by(2)
+            end
           end
+        end
+      end
 
-          it "should not create a tax refund" do
-            expect(line_item.adjustments.credit.count).to eq(0)
-          end
+      context "when price does not include tax" do
+        before do
+          allow(order).to receive(:tax_zone).and_return(zone)
+          adjust_order_items
+        end
 
-          describe 'tax adjustments' do
-            before { Spree::TaxRate.adjust(@order.tax_zone, @order.line_items) }
+        it "should delete adjustments for open order when taxrate is deleted" do
+          rate1.destroy!
+          rate2.destroy!
+          expect(line_item.adjustments.count).to eq(0)
+        end
 
-            it "should apply adjustments when a tax zone is present" do
-              expect(line_item.adjustments.count).to eq(2)
-            end
+        it "should not delete adjustments for complete order when taxrate is deleted" do
+          order.update_column :completed_at, Time.now
+          rate1.destroy!
+          rate2.destroy!
+          expect(line_item.adjustments.count).to eq(2)
+        end
 
-            describe 'when the tax zone is removed' do
-              before { allow(@order).to receive_messages :tax_zone => nil }
+        it "should create two adjustments" do
+          expect(line_item.adjustments.count).to eq(2)
+        end
 
-              it 'does not apply any adjustments' do
-                Spree::TaxRate.adjust(@order.tax_zone, @order.line_items)
-                expect(line_item.adjustments.count).to eq(0)
-              end
-            end
-          end
+        it "should not create a tax refund" do
+          expect(line_item.adjustments.credit.count).to eq(0)
         end
 
-        context "when two rates apply" do
-          before do
-            @price_before_taxes = line_item.price / (1 + @rate1.amount + @rate2.amount)
-            # Use the same rounding method as in DefaultTax calculator
-            @price_before_taxes = BigDecimal.new(@price_before_taxes).round(2, BigDecimal::ROUND_HALF_UP)
-            line_item.update_column(:pre_tax_amount, @price_before_taxes)
-            # Clear out any previously automatically-applied adjustments
-            @order.all_adjustments.delete_all
-            @rate1.adjust(@order.tax_zone, line_item)
-            @rate2.adjust(@order.tax_zone, line_item)
+        describe 'tax adjustments' do
+          it "should apply adjustments when a tax zone is present" do
+            expect(line_item.adjustments.count).to eq(2)
           end
 
-          it "should create two price adjustments" do
-            expect(@order.line_item_adjustments.count).to eq(2)
-          end
+          describe "when the order's tax zone is nil" do
+            before { allow(order).to receive(:tax_zone).and_return(nil) }
 
-          it "price adjustments should be accurate" do
-            included_tax = @order.line_item_adjustments.sum(:amount)
-            expect(@price_before_taxes + included_tax).to eq(line_item.price)
+            it 'does not apply any adjustments' do
+              Spree::TaxRate.adjust(order.tax_zone, order.line_items)
+              expect(line_item.adjustments.count).to eq(0)
+            end
           end
         end
       end

From 52c408b088dc4d2866a920e7772886eab1ee2576 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 18:22:50 +0100
Subject: [PATCH 10/15] Move shipment taxation tests

---
 core/spec/models/spree/tax_rate_spec.rb | 70 +++++++++----------------
 1 file changed, 25 insertions(+), 45 deletions(-)

diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index cd9d1a6a270..d5d00620d6f 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -142,51 +142,6 @@
   end
 
   context ".adjust" do
-    let(:order) { stub_model(Spree::Order) }
-    let(:tax_category_1) { stub_model(Spree::TaxCategory) }
-    let(:tax_category_2) { stub_model(Spree::TaxCategory) }
-    let(:rate_1) { stub_model(Spree::TaxRate, :tax_category => tax_category_1) }
-    let(:rate_2) { stub_model(Spree::TaxRate, :tax_category => tax_category_2) }
-
-    context "with line items" do
-      let(:line_item) do
-        stub_model(Spree::LineItem,
-          :price => 10.0,
-          :quantity => 1,
-          :tax_category => tax_category_1,
-          :variant => stub_model(Spree::Variant)
-        )
-      end
-
-      let(:line_items) { [line_item] }
-
-      before do
-        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
-      end
-
-      it "should apply adjustments for two tax rates to the order" do
-        expect(rate_1).to receive(:adjust)
-        expect(rate_2).not_to receive(:adjust)
-        Spree::TaxRate.adjust(order.tax_zone, line_items)
-      end
-    end
-
-    context "with shipments" do
-      let(:shipments) { [stub_model(Spree::Shipment, :cost => 10.0, :tax_category => tax_category_1)] }
-
-      before do
-        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
-      end
-
-      it "should apply adjustments for two tax rates to the order" do
-        expect(rate_1).to receive(:adjust)
-        expect(rate_2).not_to receive(:adjust)
-        Spree::TaxRate.adjust(order.tax_zone, shipments)
-      end
-    end
-  end
-
-  context "#adjust" do
     let!(:country) { create(:country) }
     let!(:taxables_category) { create(:tax_category, name: "Taxable Foo") }
     let!(:non_taxables_category) { create(:tax_category, name: "Non Taxable") }
@@ -214,6 +169,31 @@
 
     subject(:adjust_order_items) { Spree::TaxRate.adjust(order.tax_zone, order.line_items) }
 
+    context "with shipments" do
+      let(:shipments) do
+        [stub_model(Spree::Shipment, cost: 10.0, tax_category: taxables_category)]
+      end
+
+      let!(:rate2) do
+        create(
+          :tax_rate,
+          tax_category: non_taxables_category,
+          zone: zone,
+          amount: 0.05
+        )
+      end
+
+      before do
+        allow(Spree::TaxRate).to receive(:for_zone).and_return([rate1, rate2])
+      end
+
+      it "should apply adjustments for two tax rates to the order" do
+        expect(rate1).to receive(:adjust)
+        expect(rate2).not_to receive(:adjust)
+        Spree::TaxRate.adjust(zone, shipments)
+      end
+    end
+
     context "not taxable line item " do
       before { order.contents.add(non_taxable.master, 1) }
 

From 1a0b0203a939883d574500b126ea921a77e75d58 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 18:40:16 +0100
Subject: [PATCH 11/15] Reinstate and fix broken tests

Also refactor carefully and add much more commentary.

In order for refunds to work like they should, rather than always
loading the default tax zone's rates, and randomly discarding one, this
commit introduces a "Do we need to refund" check, and then adds the
default zone's VAT rates.
---
 core/app/models/spree/tax_rate.rb       | 47 +++++++++----------------
 core/spec/models/spree/tax_rate_spec.rb | 13 ++++---
 2 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 6afcb2e3ff5..4402b2c62d0 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -27,11 +27,7 @@ class TaxRate < Spree::Base
     # Gets the array of TaxRates appropriate for the specified order
     def self.match(order_tax_zone)
       return [] unless order_tax_zone
-      if default_tax_zone
-        (for_zone(order_tax_zone) + for_zone(default_tax_zone).included_in_price).uniq
-      else
-        for_zone(order_tax_zone)
-      end
+      for_zone(order_tax_zone)
     end
 
     # Pre-tax amounts must be stored so that we can calculate
@@ -97,13 +93,22 @@ def self.adjust(order_tax_zone, items)
       # TODO: Make sure items is always an AR relation and use `update_columns`
       # I think the main thing to be done here is adapting the tests, which use arrays.
       items.each { |item| item.update_column(:pre_tax_amount, 0) }
-
+      return unless order_tax_zone
       # Find tax rates matching the order's tax zone
-      rates = match(order_tax_zone)
-
-      # This is a bizarre bit of code. I've extracted it for now so it doesn't
-      # clutter that badly.
-      rates = exclude_default_zone_vat_rates_if_vat_rate_present(rates) if default_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)
@@ -174,26 +179,6 @@ def self.default_tax_zone
       @_default_tax_zone = Spree::Zone.default_tax
     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 #4397 and #4327.
-    def self.exclude_default_zone_vat_rates_if_vat_rate_present(rates)
-      rates.delete_if do |rate|
-        rate.included_in_price? &&
-        (rates - [rate]).map(&:tax_category).include?(rate.tax_category)
-      end
-    end
-
       def create_label
         label = ""
         label << (name.present? ? name : tax_category.name) + " "
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index d5d00620d6f..774491b5b3c 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -259,9 +259,7 @@
         context "when order's zone is the default zone" do
           let!(:zone) { default_zone }
 
-          # This does not work properly. When you have two tax rates from the same
-          # category, one will be deleted. That is not right.
-          xit "should create two adjustments, one for each tax rate" do
+          it "should create two adjustments, one for each tax rate" do
             expect(line_item.adjustments.credit.count).to eq(0)
             expect { adjust_order_items }.to change { line_item.adjustments.tax.count }.by(2)
           end
@@ -272,8 +270,9 @@
           end
 
           # This does not work either. Both tax rates should be taken into account.
-          xit "price adjustments should be accurate" do
-            included_tax = order.line_item_adjustments.sum(:amount)
+          it "price adjustments should be accurate" do
+            expect { adjust_order_items }.to change { line_item.adjustments.tax.count }
+            included_tax = line_item.adjustments.sum(:amount)
             expect(line_item.pre_tax_amount).to eq(17.38)
             expect(line_item.pre_tax_amount + included_tax).to eq(line_item.price)
           end
@@ -316,14 +315,14 @@
 
             it "does not refund the default zone's VAT" do
               expect(line_item.adjustments.credit.count).to eq(0)
-              expect { adjust_order_items }.not_to change { line_item.adjustments.charge.count }
+              expect { adjust_order_items }.not_to change { line_item.adjustments.credit.count }
             end
           end
 
           context "and the foreign zone does not have VAT rates" do
             # This does not work. The code removes a VAT rate if
             # there's more than one VAT of the same category.
-            xit "refunds both default zone's VATs" do
+            it "refunds both default zone's VATs" do
               expect(line_item.adjustments.count).to eq(0)
               expect { adjust_order_items }.to change { line_item.adjustments.tax.count }.by(2)
             end

From e4ddea5d09a67e53aae30eacefb90194dcab50fb Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 19:57:30 +0100
Subject: [PATCH 12/15] Fix resetting the pre_tax_amount

This broke the reimbursement tests.
---
 core/app/models/spree/tax_rate.rb | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 4402b2c62d0..d5444774a5c 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -48,11 +48,8 @@ def self.store_pre_tax_amount(item, rates)
     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:
@@ -88,12 +85,17 @@ def self.store_pre_tax_amount(item, rates)
     #
     # Those rates should never come into play at all and only the French rates should apply.
     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 adjustment destroy callback fires.
       Spree::Adjustment.where(adjustable: items).tax.destroy_all
       # TODO: Make sure items is always an AR relation and use `update_columns`
+      # TODO: Also: The whole pre_tax_amount stuff is so unnecessary once prices are right.
       # I think the main thing to be done here is adapting the tests, which use arrays.
-      items.each { |item| item.update_column(:pre_tax_amount, 0) }
-      return unless order_tax_zone
+      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)
 

From 323da68fb42743f9817876d334902cb6fead291c Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 21:18:43 +0100
Subject: [PATCH 13/15] Add :with_country trait for Zone factory

---
 core/lib/spree/testing_support/factories/zone_factory.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/core/lib/spree/testing_support/factories/zone_factory.rb b/core/lib/spree/testing_support/factories/zone_factory.rb
index db3c3e583dd..6b06f283d7e 100644
--- a/core/lib/spree/testing_support/factories/zone_factory.rb
+++ b/core/lib/spree/testing_support/factories/zone_factory.rb
@@ -13,5 +13,9 @@
   factory :zone, class: Spree::Zone do
     name { generate(:random_string) }
     description { generate(:random_string) }
+
+    trait :with_country do
+      countries { [create(:country)] }
+    end
   end
 end

From 3c7a34ecfd67775da1da88b1f600ddbc2f789fd3 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 21:19:13 +0100
Subject: [PATCH 14/15] Fix reimbursement specs

---
 core/spec/models/spree/reimbursement_spec.rb                | 2 +-
 core/spec/models/spree/reimbursement_tax_calculator_spec.rb | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/core/spec/models/spree/reimbursement_spec.rb b/core/spec/models/spree/reimbursement_spec.rb
index 60eebaa4410..5356fe8162d 100644
--- a/core/spec/models/spree/reimbursement_spec.rb
+++ b/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) }
diff --git a/core/spec/models/spree/reimbursement_tax_calculator_spec.rb b/core/spec/models/spree/reimbursement_tax_calculator_spec.rb
index 86f4a177c1a..2a050992774 100644
--- a/core/spec/models/spree/reimbursement_tax_calculator_spec.rb
+++ b/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

From 666bd3f809703a66247f0233feb86d1b0a2414ce Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Sun, 22 Nov 2015 21:22:46 +0100
Subject: [PATCH 15/15] Remove TaxRate.match

It does almost the same thing as TaxRate.for_zone, except for an early
return when the Argument is nil. Since it's only called from
TaxRate.adjust, at a point where we've already made sure that passed in
zone is not nil, the method is unnecessary.
---
 core/app/models/spree/tax_rate.rb       |  7 +------
 core/spec/models/spree/tax_rate_spec.rb | 14 ++++++--------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index d5444774a5c..1764f390cd3 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -18,18 +18,13 @@ class TaxRate < Spree::Base
 
     scope :by_zone, ->(zone) { where(zone_id: zone) }
 
+    # Gets the array of TaxRates appropriate for the specified order
     scope :for_zone, -> (zone) do
       where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
     end
 
     scope :included_in_price, -> { where(included_in_price: true) }
 
-    # Gets the array of TaxRates appropriate for the specified order
-    def self.match(order_tax_zone)
-      return [] unless order_tax_zone
-      for_zone(order_tax_zone)
-    end
-
     # Pre-tax amounts must be stored so that we can calculate
     # correct rate amounts in the future. For example:
     # https://github.com/spree/spree/issues/4318#issuecomment-34723428
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 774491b5b3c..9fa464bb818 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -1,18 +1,13 @@
 require 'spec_helper'
 
 describe Spree::TaxRate, :type => :model do
-  context ".match" do
+  context ".for_zone" do
     let(:order) { create(:order) }
     let(:country) { create(:country) }
     let(:tax_category) { create(:tax_category) }
     let(:calculator) { Spree::Calculator::FlatRate.new }
 
-    subject(:tax_rates_for_order) { Spree::TaxRate.match(order.tax_zone) }
-
-    it "should return an empty array when tax_zone is nil" do
-      allow(order).to receive_messages :tax_zone => nil
-      expect(tax_rates_for_order).to eq([])
-    end
+    subject(:tax_rates_for_order) { Spree::TaxRate.for_zone(order.tax_zone) }
 
     context "when no rate zones match the tax zone" do
       before do
@@ -363,7 +358,10 @@
           end
 
           describe "when the order's tax zone is nil" do
-            before { allow(order).to receive(:tax_zone).and_return(nil) }
+            before do
+              line_item.adjustments.tax.destroy_all
+              allow(order).to receive(:tax_zone).and_return(nil)
+            end
 
             it 'does not apply any adjustments' do
               Spree::TaxRate.adjust(order.tax_zone, order.line_items)