From 23e7b4bb8efaf97ce2dbabe2657fedf5721a916b Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Fri, 20 Nov 2015 19:44:33 +0100
Subject: [PATCH 1/4] 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).
---
 core/app/models/spree/zone.rb       | 15 ++++++
 core/spec/models/spree/zone_spec.rb | 77 +++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb
index 68b51dd6456..b5ccab9fa20 100644
--- a/core/app/models/spree/zone.rb
+++ b/core/app/models/spree/zone.rb
@@ -40,6 +40,21 @@ def self.match(address)
       matches.first
     end
 
+    # Returns all zones that also contain any of the zone members of the zone
+    # passed in. This also includes any country zones that contain any states of
+    # the current zone, if it's a state zone. If the zone passed in has members,
+    # those will also be returned.
+    def self.with_shared_members(zone)
+      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 (?))",
+        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
diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb
index e6f7d43ce88..1850122f1f7 100644
--- a/core/spec/models/spree/zone_spec.rb
+++ b/core/spec/models/spree/zone_spec.rb
@@ -336,4 +336,81 @@
       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 an empty set' do
+        expect(subject).to eq([])
+      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
+  end
 end

From 1eca077f9913b73e141c5bad0e7c86adf611d763 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Fri, 20 Nov 2015 20:10:08 +0100
Subject: [PATCH 2/4] 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 | 36 ++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb
index b5ccab9fa20..dfd64d14324 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,16 +53,13 @@ def self.match(address)
       matches.first
     end
 
-    # Returns all zones that also contain any of the zone members of the zone
-    # passed in. This also includes any country zones that contain any states of
-    # the current zone, if it's a state zone. If the zone passed in has members,
-    # those will also be returned.
+
+    # 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(: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 (?))",
+      with_member_ids(
         zone.states.pluck(:id),
         zone.states.pluck(:country_id) + zone.countries.pluck(:id)
       ).uniq

From a7a93346f8d16370588c59864cc3fff16428c833 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Tue, 24 Nov 2015 22:48:32 +0100
Subject: [PATCH 3/4] Introduce TaxRate.for_zone

A scope that yields all tax rates that are valid for a particular zone.
This is intended to replace `Spree::TaxRate.match` in its entirety. I've
also refactored the specs for better readability and reduced scope of
the method.
---
 core/app/models/spree/tax_rate.rb       |   7 +-
 core/spec/models/spree/tax_rate_spec.rb | 158 +++++++-----------------
 2 files changed, 47 insertions(+), 118 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index bf24a60d5b7..bbdfda2e832 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -27,9 +27,12 @@ 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
+    scope :for_zone,
+          ->(zone) do
+            where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
+          end
+
     def self.match(order_tax_zone)
       return [] unless order_tax_zone
       rates = includes(zone: { zone_members: :zoneable }).load.select do |rate|
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index a2d57b9358f..fefa0e97f39 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -1,158 +1,84 @@
 require 'spec_helper'
 
 describe Spree::TaxRate, :type => :model do
-  context "match" do
-    let(:order) { create(:order) }
-    let(:country) { create(:country) }
-    let(:tax_category) { create(:tax_category) }
-    let(:calculator) { Spree::Calculator::FlatRate.new }
-
-    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([])
+  context ".for_zone" do
+    subject(:rates_for_zone) { Spree::TaxRate.for_zone(zone) }
+
+    context "when zone is nil" do
+      let(:zone) { nil }
+
+      it "raises an exception" do
+        expect { rates_for_zone }.to raise_error(NameError)
+      end
     end
 
     context "when no rate zones match the tax zone" do
-      before do
-        Spree::TaxRate.create(:amount => 1, :zone => create(:zone))
-      end
+      let(:rate_zone) { create(:zone, :with_country) }
+      let!(:rate) { create :tax_rate, zone: rate_zone }
 
       context "when there is no default tax zone" do
-        before do
-          @zone = create(:zone, :name => "Country Zone", :default_tax => false, :zone_members => [])
-          @zone.zone_members.create(:zoneable => country)
-        end
+        context "and the zone has no shared members with the rate zone" do
+          let(:zone) { create(:zone, :with_country) }
 
-        it "should return an empty array" do
-          allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to eq([])
+          it "should return an empty array" do
+            expect(subject).to eq([])
+          end
         end
 
-        it "should return the rate that matches the rate zone" do
-          rate = Spree::TaxRate.create(
-            :amount => 1,
-            :zone => @zone,
-            :tax_category => tax_category,
-            :calculator => calculator
-          )
+        context "and the zone has shared members with the rate zone" do
+          let(:zone) { create(:zone, countries: rate_zone.countries) }
 
-          allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to eq([rate])
+          it "should return the rate that matches the rate zone" do
+            expect(subject).to eq([rate])
+          end
         end
 
-        it "should return all rates that match the rate zone" do
-          rate1 = Spree::TaxRate.create(
-            :amount => 1,
-            :zone => @zone,
-            :tax_category => tax_category,
-            :calculator => calculator
-          )
-
-          rate2 = Spree::TaxRate.create(
-            :amount => 2,
-            :zone => @zone,
-            :tax_category => tax_category,
-            :calculator => Spree::Calculator::FlatRate.new
-          )
-
-          allow(order).to receive_messages :tax_zone => @zone
-          expect(Spree::TaxRate.match(order.tax_zone)).to match_array([rate1, rate2])
+        context "there is many rates that match the zone" do
+          let!(:rate2) { create :tax_rate, zone: rate_zone}
+          let(:zone) { create(:zone, countries: rate_zone.countries) }
+
+          it "should return all rates that match the rate zone" do
+            expect(subject).to match_array([rate, rate2])
+          end
         end
 
         context "when the tax_zone is contained within a rate zone" do
-          before do
-            sub_zone = create(:zone, :name => "State Zone", :zone_members => [])
-            sub_zone.zone_members.create(:zoneable => create(:state, :country => country))
-            allow(order).to receive_messages :tax_zone => sub_zone
-            @rate = Spree::TaxRate.create(
-              :amount => 1,
-              :zone => @zone,
-              :tax_category => tax_category,
-              :calculator => calculator
-            )
-          end
+          let(:country1) { create :country }
+          let(:country2) { create :country }
+          let(:rate_zone) { create(:zone, countries: [country1, country2]) }
+          let(:zone) { create(:zone, countries: [country1]) }
 
           it "should return the rate zone" do
-            expect(Spree::TaxRate.match(order.tax_zone)).to eq([@rate])
+            expect(subject).to eq([rate])
           end
         end
       end
 
       context "when there is a default tax zone" do
-        before do
-          @zone = create(:zone, :name => "Country Zone", :default_tax => true, :zone_members => [])
-          @zone.zone_members.create(:zoneable => country)
-        end
-
+        let(:default_zone) { create(:zone, :with_country, default_tax: true) }
         let(:included_in_price) { false }
         let!(:rate) do
-          Spree::TaxRate.create(:amount => 1,
-                                :zone => @zone,
-                                :tax_category => tax_category,
-                                :calculator => calculator,
-                                :included_in_price => included_in_price)
+          create(:tax_rate, zone: default_zone, 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
-            allow(order).to receive_messages :tax_address => tax_address
-          end
-
-          let(:tax_address) { stub_model(Spree::Address) }
-
+        context "when the zone is the default zone" do
+          let(:zone) { default_zone }
           context "when the tax is not a VAT" do
+
             it { is_expected.to eq([rate]) }
           end
 
           context "when the tax is a VAT" do
             let(:included_in_price) { true }
+
             it { is_expected.to eq([rate]) }
           end
         end
 
-        context "when the order has a different tax zone" do
-          before do
-            allow(order).to receive_messages :tax_zone => create(:zone, :name => "Other Zone")
-            allow(order).to receive_messages :tax_address => tax_address
-          end
-
-          context "when the order has a tax_address" do
-            let(:tax_address) { stub_model(Spree::Address) }
+        context "when the zone is outside the default zone" do
+          let(:zone) { create(:zone, :with_country)}
 
-            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
-              end
-            end
-          end
-
-          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
-          end
+          it { is_expected.to be_empty }
         end
       end
     end

From e8b6ffa42e6acb505b65714457e555d398f9b0cf Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 1 Dec 2015 23:20:07 +0100
Subject: [PATCH 4/4] Tax Rate refactoring

This looks like a very large commit, but it actually mostly removes lines.

The goal is to make somewhat more understandable what's currently going on in the Solidus
codebase: What rates apply to my order's zone? Those which have shared members with my zone.
When is there a refund? Whenever the store is sending stuff to a place outside the default zone
that does not have included taxes.

The refactoring now implies that a zone have members to be applicable. In the real world, that is
always the case.

There is an expectation for a negative included tax in the reimbursement tax calculator. That
expectation is IMO bogus, I have no idea why it should be there.
---
 core/app/models/spree/tax_rate.rb             | 145 ++++++++----------
 core/spec/models/spree/reimbursement_spec.rb  |   2 +-
 .../reimbursement_tax_calculator_spec.rb      |   6 +-
 core/spec/models/spree/tax_rate_spec.rb       |  20 +--
 4 files changed, 77 insertions(+), 96 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index bbdfda2e832..6617d0668d3 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -33,39 +33,7 @@ class TaxRate < Spree::Base
             where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id))
           end
 
-    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)
-          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:
@@ -84,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:
@@ -147,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.
@@ -176,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
@@ -194,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
diff --git a/core/spec/models/spree/reimbursement_spec.rb b/core/spec/models/spree/reimbursement_spec.rb
index e0591aeaf01..dee63b5b4ee 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
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index fefa0e97f39..fb7c35337e6 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -91,6 +91,11 @@
     let(:rate_1) { stub_model(Spree::TaxRate, :tax_category => tax_category_1) }
     let(:rate_2) { stub_model(Spree::TaxRate, :tax_category => tax_category_2) }
 
+    before do
+      allow(Spree::TaxRate).to receive_messages :for_zone => [rate_1, rate_2]
+      allow(order).to receive(:tax_zone).and_return(build(:zone))
+    end
+
     context "with line items" do
       let(:line_item) do
         stub_model(Spree::LineItem,
@@ -103,10 +108,6 @@
 
       let(:line_items) { [line_item] }
 
-      before do
-        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
-      end
-
       it "should only apply adjustments for matching rates" do
         expect(rate_1).to receive(:adjust)
         expect(rate_2).not_to receive(:adjust)
@@ -117,10 +118,6 @@
     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 matching rates" do
         expect(rate_1).to receive(:adjust)
         expect(rate_2).not_to receive(:adjust)
@@ -286,8 +283,7 @@
             expect(line_item.adjustments.tax.count).to eq(1)
           end
 
-          # This test fails intermittently - it's a matter of luck
-          xit 'has 4.79 of included tax' do
+          it 'has 4.79 of included tax' do
             expect(line_item.included_tax_total).to eq(4.79)
           end
 
@@ -308,8 +304,8 @@
             expect(line_item.adjustments.tax.count).to eq(1)
           end
 
-          # Fails intermittently - xit'ed for the time being
-          xit 'has 2.02 of included tax' do
+          it 'has 2.02 of included tax' do
+            pending 'but it calculates the tax base on the german gross price'
             expect(line_item.included_tax_total).to eq(2.02)
           end