From 89a0168bc400c7386e013b104c69f4f42059fed7 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 12 Jan 2016 11:10:12 -0600
Subject: [PATCH 01/13] Add POROs for creating tax adjustments

For the time being, these two POROs (one for orders, another for items) call
the already defined methods on `Spree::TaxRate`.

The idea is, though, that the responsibilities between `Spree::Tax::{Item,Order}Adjuster` and
`Spree::TaxRate` are divided as follows: `Spree::TaxRate` stores and selects rates,
while `Spree::Tax::{Item,Order}Adjuster` create tax adjustments.
---
 core/app/models/spree/line_item.rb            |  2 +-
 core/app/models/spree/order.rb                |  5 +---
 core/app/models/spree/tax/item_adjuster.rb    | 16 +++++++++++
 core/app/models/spree/tax/order_adjuster.rb   | 15 ++++++++++
 .../models/spree/tax/item_adjuster_spec.rb    | 28 +++++++++++++++++++
 .../models/spree/tax/order_adjuster_spec.rb   | 22 +++++++++++++++
 6 files changed, 83 insertions(+), 5 deletions(-)
 create mode 100644 core/app/models/spree/tax/item_adjuster.rb
 create mode 100644 core/app/models/spree/tax/order_adjuster.rb
 create mode 100644 core/spec/models/spree/tax/item_adjuster_spec.rb
 create mode 100644 core/spec/models/spree/tax/order_adjuster_spec.rb

diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb
index 2b0b815c631..13ad17e54bd 100644
--- a/core/app/models/spree/line_item.rb
+++ b/core/app/models/spree/line_item.rb
@@ -173,7 +173,7 @@ def recalculate_adjustments
     end
 
     def update_tax_charge
-      Spree::TaxRate.adjust(order.tax_zone, [self])
+      Spree::Tax::ItemAdjuster.new(self).adjust!
     end
 
     def ensure_proper_currency
diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 8ee9d9b17e1..0fab7786483 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -321,10 +321,7 @@ def line_item_options_match(line_item, options)
     # Creates new tax charges if there are any applicable rates. If prices already
     # include taxes then price adjustments are created instead.
     def create_tax_charge!
-      # We want to only look up the applicable tax zone once and pass it to TaxRate calculation to avoid duplicated lookups.
-      order_tax_zone = tax_zone
-      Spree::TaxRate.adjust(order_tax_zone, line_items)
-      Spree::TaxRate.adjust(order_tax_zone, shipments) if shipments.any?
+      Spree::Tax::OrderAdjuster.new(self).adjust!
     end
 
     def outstanding_balance
diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
new file mode 100644
index 00000000000..1cf5ab7ad0e
--- /dev/null
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -0,0 +1,16 @@
+module Spree
+  module Tax
+    class ItemAdjuster
+      attr_reader :item, :order
+
+      def initialize(item)
+        @item = item
+        @order = @item.order
+      end
+
+      def adjust!
+        Spree::TaxRate.adjust(order.tax_zone, [item])
+      end
+    end
+  end
+end
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
new file mode 100644
index 00000000000..9b94f7f01aa
--- /dev/null
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -0,0 +1,15 @@
+module Spree
+  module Tax
+    class OrderAdjuster
+      attr_reader :order
+
+      def initialize(order)
+        @order = order
+      end
+
+      def adjust!
+        Spree::TaxRate.adjust(order.tax_zone, order.line_items + order.shipments)
+      end
+    end
+  end
+end
diff --git a/core/spec/models/spree/tax/item_adjuster_spec.rb b/core/spec/models/spree/tax/item_adjuster_spec.rb
new file mode 100644
index 00000000000..15e7a006c11
--- /dev/null
+++ b/core/spec/models/spree/tax/item_adjuster_spec.rb
@@ -0,0 +1,28 @@
+require 'spec_helper'
+
+RSpec.describe Spree::Tax::ItemAdjuster do
+  subject(:adjuster) { described_class.new(item) }
+  let(:order) { Spree::Order.new }
+  let(:item) { Spree::LineItem.new(order: order) }
+
+  before do
+    allow(order).to receive(:tax_zone) { build(:zone) }
+  end
+
+  describe 'initialization' do
+    it 'sets order to item order' do
+      expect(adjuster.order).to eq(item.order)
+    end
+
+    it 'sets adjustable' do
+      expect(adjuster.item).to eq(item)
+    end
+  end
+
+  describe '#adjust!' do
+    it 'calls Spree::TaxRate.adjust' do
+      expect(Spree::TaxRate).to receive(:adjust)
+      adjuster.adjust!
+    end
+  end
+end
diff --git a/core/spec/models/spree/tax/order_adjuster_spec.rb b/core/spec/models/spree/tax/order_adjuster_spec.rb
new file mode 100644
index 00000000000..b565e8023c1
--- /dev/null
+++ b/core/spec/models/spree/tax/order_adjuster_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+RSpec.describe Spree::Tax::OrderAdjuster do
+  subject(:adjuster) { described_class.new(order) }
+
+  describe 'initialization' do
+    let(:order) { Spree::Order.new }
+
+    it 'sets order to adjustable' do
+      expect(adjuster.order).to eq(order)
+    end
+  end
+
+  describe '#adjust!' do
+    let(:order) { Spree::Order.new }
+
+    it 'calls the Spree::TaxRate.adjust' do
+      expect(Spree::TaxRate).to receive(:adjust)
+      adjuster.adjust!
+    end
+  end
+end

From f7edf4645283393e1e4cc424f33553df6a46cb37 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 19 Jan 2016 11:44:30 -0600
Subject: [PATCH 02/13] Replace `TaxRate.adjust` with
 {Item,Order}Adjuster.adjust!

The code in `Spree::TaxRate.adjust!` is hard to understand, and the
adjusting should happen in dedicated adjuster classes. This commit
ports the functionality of `TaxRate.match` over to two classes:

* An `OrderAdjuster` which iterates over all items (line items and shipments)
  in an order and runs the

* `ItemAdjuster`, which actually creates adjustments for every item.

The code should read a lot easier now.

In order to reduce database load, the tax rate array for an order can be passed
into the `ItemAdjuster.new`. Considering the performance benefits, I think
that's justifiable amount of shared knowledge between the two Adjuster classes.
---
 core/app/models/spree/tax/item_adjuster.rb    | 26 ++++++++-
 core/app/models/spree/tax/order_adjuster.rb   | 18 ++++++-
 core/app/models/spree/tax_rate.rb             | 23 --------
 .../models/spree/order/state_machine_spec.rb  |  2 +-
 .../models/spree/tax/item_adjuster_spec.rb    | 53 +++++++++++++++++--
 .../models/spree/tax/order_adjuster_spec.rb   | 18 +++++--
 core/spec/models/spree/tax_rate_spec.rb       | 29 ++++++----
 7 files changed, 127 insertions(+), 42 deletions(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 1cf5ab7ad0e..3cb01f8ac4f 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -3,13 +3,35 @@ module Tax
     class ItemAdjuster
       attr_reader :item, :order
 
-      def initialize(item)
+      def initialize(item, options = {})
         @item = item
         @order = @item.order
+        # set the instance variable so `TaxRate.match` is only called when necessary
+        @rates_for_order_zone = options[:rates_for_order_zone]
       end
 
       def adjust!
-        Spree::TaxRate.adjust(order.tax_zone, [item])
+        return unless order_tax_zone
+        # Using .destroy_all to make sure callbacks fire
+        item.adjustments.tax.destroy_all
+
+        TaxRate.store_pre_tax_amount(item, rates_for_item)
+
+        rates_for_item.map { |rate| rate.adjust(order_tax_zone, item) }
+      end
+
+      private
+
+      def rates_for_item
+        @rates_for_item ||= rates_for_order_zone.select { |rate| rate.tax_category == item.tax_category }
+      end
+
+      def rates_for_order_zone
+        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
+      end
+
+      def order_tax_zone
+        @order_tax_zone ||= order.tax_zone
       end
     end
   end
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
index 9b94f7f01aa..80dcac22a3f 100644
--- a/core/app/models/spree/tax/order_adjuster.rb
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -8,7 +8,23 @@ def initialize(order)
       end
 
       def adjust!
-        Spree::TaxRate.adjust(order.tax_zone, order.line_items + order.shipments)
+        return unless order_tax_zone
+
+        (order.line_items + order.shipments).each do |item|
+          ItemAdjuster.new(item, rates_for_order_zone: rates_for_order_zone).adjust!
+        end
+      end
+
+      private
+
+      # When adjusting multiple items, we don't want to
+      # look up the zone rates for each individual item
+      def rates_for_order_zone
+        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
+      end
+
+      def order_tax_zone
+        @order_tax_zone ||= order.tax_zone
       end
     end
   end
diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 116fbb3fefb..e00d683a173 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -115,29 +115,6 @@ def self.store_pre_tax_amount(item, rates)
       item.update_column(:pre_tax_amount, pre_tax_amount.round(2))
     end
 
-    # This method is best described by the documentation on .match
-    def self.adjust(order_tax_zone, items)
-      rates = match(order_tax_zone)
-      tax_categories = rates.map(&:tax_category)
-      relevant_items, non_relevant_items = items.partition { |item| tax_categories.include?(item.tax_category) }
-      unless relevant_items.empty?
-        Spree::Adjustment.where(adjustable: relevant_items).tax.destroy_all # using destroy_all to ensure adjustment destroy callback fires.
-      end
-      relevant_items.each do |item|
-        relevant_rates = rates.select { |rate| rate.tax_category == item.tax_category }
-        store_pre_tax_amount(item, relevant_rates)
-        relevant_rates.each do |rate|
-          rate.adjust(order_tax_zone, item)
-        end
-      end
-      non_relevant_items.each do |item|
-        if item.adjustments.tax.present?
-          item.adjustments.tax.destroy_all # using destroy_all to ensure adjustment destroy callback fires.
-          item.update_columns pre_tax_amount: 0
-        end
-      end
-    end
-
     # Creates necessary tax adjustments for the order.
     def adjust(order_tax_zone, item)
       amount = compute_amount(item)
diff --git a/core/spec/models/spree/order/state_machine_spec.rb b/core/spec/models/spree/order/state_machine_spec.rb
index aaf3fb76667..e7b7a9323fd 100644
--- a/core/spec/models/spree/order/state_machine_spec.rb
+++ b/core/spec/models/spree/order/state_machine_spec.rb
@@ -43,7 +43,7 @@
       end
 
       it "adjusts tax rates when transitioning to delivery" do
-        expect(Spree::TaxRate).to receive(:adjust).once.with(order.tax_zone, order.line_items)
+        expect(Spree::Tax::OrderAdjuster).to receive(:new).once.with(order).and_call_original
         order.next!
       end
     end
diff --git a/core/spec/models/spree/tax/item_adjuster_spec.rb b/core/spec/models/spree/tax/item_adjuster_spec.rb
index 15e7a006c11..7eee729c96c 100644
--- a/core/spec/models/spree/tax/item_adjuster_spec.rb
+++ b/core/spec/models/spree/tax/item_adjuster_spec.rb
@@ -20,9 +20,56 @@
   end
 
   describe '#adjust!' do
-    it 'calls Spree::TaxRate.adjust' do
-      expect(Spree::TaxRate).to receive(:adjust)
-      adjuster.adjust!
+    before do
+      expect(order).to receive(:tax_zone).and_return(tax_zone)
+    end
+
+    context 'when the order has no tax zone' do
+      let(:tax_zone) { nil }
+
+      before do
+        allow(order).to receive(:tax_zone).and_return(nil)
+        adjuster.adjust!
+      end
+
+      it 'returns nil early' do
+        expect(adjuster.adjust!).to be_nil
+      end
+    end
+
+    context 'when the order has a tax zone' do
+      let(:item) { build_stubbed :line_item, order: order }
+      let(:tax_zone) { build_stubbed(:zone, :with_country) }
+
+      before do
+        expect(item).to receive(:update_column)
+
+        expect(Spree::TaxRate).to receive(:match).with(tax_zone).and_return(rates_for_order_zone)
+      end
+
+      context 'when there are no matching rates' do
+        let(:rates_for_order_zone) { [] }
+
+        it 'returns no adjustments' do
+          expect(adjuster.adjust!).to eq([])
+        end
+      end
+
+      context 'when there are matching rates for the zone' do
+        context 'and all rates have the same tax category as the item' do
+          let(:item_tax_category) { build_stubbed(:tax_category) }
+          let(:rate_1) { create :tax_rate, tax_category: item_tax_category }
+          let(:rate_2) { create :tax_rate }
+          let(:rates_for_order_zone) { [rate_1, rate_2] }
+
+          before { allow(item).to receive(:tax_category).and_return(item_tax_category) }
+
+          it 'creates an adjustment for every matching rate' do
+            expect(rate_1).to receive_message_chain(:adjustments, :create!)
+            expect(adjuster.adjust!.length).to eq(1)
+          end
+        end
+      end
     end
   end
 end
diff --git a/core/spec/models/spree/tax/order_adjuster_spec.rb b/core/spec/models/spree/tax/order_adjuster_spec.rb
index b565e8023c1..d58b960e01e 100644
--- a/core/spec/models/spree/tax/order_adjuster_spec.rb
+++ b/core/spec/models/spree/tax/order_adjuster_spec.rb
@@ -12,10 +12,22 @@
   end
 
   describe '#adjust!' do
-    let(:order) { Spree::Order.new }
+    let(:zone) { build_stubbed(:zone) }
+    let(:line_items) { build_stubbed_list(:line_item, 2) }
+    let(:order) { build_stubbed(:order, line_items: line_items) }
+    let(:rates_for_order_zone) { [] }
+    let(:item_adjuster) { Spree::Tax::ItemAdjuster.new(line_items.first) }
+
+    before do
+      expect(order).to receive(:tax_zone).at_least(:once).and_return(zone)
+      expect(Spree::TaxRate).to receive(:match).and_return(rates_for_order_zone)
+    end
+
+    it 'calls the item adjuster with all line items' do
+      expect(Spree::Tax::ItemAdjuster).to receive(:new).with(line_items.first, rates_for_order_zone: rates_for_order_zone).and_return(item_adjuster)
+      expect(Spree::Tax::ItemAdjuster).to receive(:new).with(line_items.second, rates_for_order_zone: rates_for_order_zone).and_return(item_adjuster)
 
-    it 'calls the Spree::TaxRate.adjust' do
-      expect(Spree::TaxRate).to receive(:adjust)
+      expect(item_adjuster).to receive(:adjust!).twice
       adjuster.adjust!
     end
   end
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index c65289dfe70..80993b39005 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -159,7 +159,8 @@
   end
 
   context ".adjust" do
-    let(:order) { stub_model(Spree::Order) }
+    let(:zone) { stub_model(Spree::Zone)}
+    let(:order) { stub_model(Spree::Order, tax_zone: zone) }
     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) }
@@ -171,34 +172,44 @@
           price: 10.0,
           quantity: 1,
           tax_category: tax_category_1,
-          variant: stub_model(Spree::Variant)
+          variant: stub_model(Spree::Variant),
+          order: order
         )
       end
 
       let(:line_items) { [line_item] }
 
       before do
-        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
+        allow(order).to receive(:line_items).and_return([line_item])
       end
 
       it "should only apply adjustments for matching rates" do
         expect(rate_1).to receive(:adjust)
         expect(rate_2).not_to receive(:adjust)
-        Spree::TaxRate.adjust(order.tax_zone, line_items)
+        Spree::Tax::OrderAdjuster.new(order).adjust!
       end
     end
 
     context "with shipments" do
-      let(:shipments) { [stub_model(Spree::Shipment, cost: 10.0, tax_category: tax_category_1)] }
+      let(:shipment) do
+        stub_model(
+          Spree::Shipment,
+          :cost => 10.0,
+          :tax_category => tax_category_1,
+          order: order
+        )
+      end
 
       before do
-        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
+        allow(order).to receive(:shipments).and_return([shipment])
       end
 
       it "should apply adjustments for matching rates" do
         expect(rate_1).to receive(:adjust)
         expect(rate_2).not_to receive(:adjust)
-        Spree::TaxRate.adjust(order.tax_zone, shipments)
+        Spree::Tax::OrderAdjuster.new(order).adjust!
       end
     end
   end
@@ -265,7 +276,7 @@
       before do
         allow(order).to receive(:tax_zone) { tax_zone }
         order.contents.add(variant)
-        Spree::TaxRate.adjust(order.tax_zone, order.line_items)
+        Spree::Tax::OrderAdjuster.new(order).adjust!
       end
 
       let(:line_item) { order.line_items.first }
@@ -525,7 +536,7 @@
       before do
         allow(order).to receive(:tax_zone) { tax_zone }
         order.contents.add(variant)
-        Spree::TaxRate.adjust(order.tax_zone, order.line_items)
+        Spree::Tax::OrderAdjuster.new(order).adjust!
       end
 
       let(:line_item) { order.line_items.first }

From b24c9cbce9c108a9b98deca5685315413a67e643 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Sun, 24 Jan 2016 12:30:46 -0600
Subject: [PATCH 03/13] Add TaxRate.for_zone scope

TaxRate.for_zone will return all tax rates that are valid for a given zone.
It does not care about the default VAT zone, as that should be handled by the
Adjuster classes.
---
 core/app/models/spree/tax_rate.rb       |   6 +-
 core/spec/models/spree/tax_rate_spec.rb | 166 +++++++-----------------
 2 files changed, 49 insertions(+), 123 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index e00d683a173..4aab39adb43 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -27,9 +27,7 @@ class TaxRate < Spree::Base
     validates :tax_category_id, presence: true
     validates_with DefaultTaxZoneValidator
 
-    scope :by_zone, ->(zone) { where(zone_id: zone) }
-
-    # Finds geographically matching tax rates for an order's tax zone.
+    # Finds geographically matching tax rates for a tax zone.
     # We do not know if they are/aren't applicable until we attempt to apply these rates to
     # the items contained within the Order itself.
     # For instance, if a rate passes the criteria outlined in this method,
@@ -69,6 +67,8 @@ class TaxRate < Spree::Base
     # Under no circumstances should negative adjustments be applied for the Spanish tax rates.
     #
     # Those rates should never come into play at all and only the French rates should apply.
+    scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
+
     def self.match(order_tax_zone)
       return [] unless order_tax_zone
       all_rates = includes(zone: { zone_members: :zoneable }).load
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 80993b39005..2e3012be8de 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -1,107 +1,68 @@
 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]) }
@@ -109,57 +70,22 @@
 
           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 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 zone is outside the default zone" do
+          let(:zone) { create(:zone, :with_country) }
 
-            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
   end
 
   context ".adjust" do
-    let(:zone) { stub_model(Spree::Zone)}
+    let(:zone) { stub_model(Spree::Zone) }
     let(:order) { stub_model(Spree::Order, tax_zone: zone) }
     let(:tax_category_1) { stub_model(Spree::TaxCategory) }
     let(:tax_category_2) { stub_model(Spree::TaxCategory) }
@@ -180,7 +106,7 @@
       let(:line_items) { [line_item] }
 
       before do
-        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
         allow(order).to receive(:line_items).and_return([line_item])
       end
 
@@ -195,14 +121,14 @@
       let(:shipment) do
         stub_model(
           Spree::Shipment,
-          :cost => 10.0,
-          :tax_category => tax_category_1,
+          cost: 10.0,
+          tax_category: tax_category_1,
           order: order
         )
       end
 
       before do
-        allow(Spree::TaxRate).to receive_messages :match => [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
         allow(order).to receive(:shipments).and_return([shipment])
       end
 

From 49b8aa227baa5d430985684d08c0c4d01c268c5c Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Sun, 24 Jan 2016 16:55:07 -0600
Subject: [PATCH 04/13] Allow passing nil into Spree::Zone.with_shared_members

When using .with_shared_members, sometimes you pass nil - and that should just
yield an empty ActiveRecord relation.

Same goes for Spree::TaxRate.for_zone(nil).
---
 core/app/models/spree/zone.rb           | 2 ++
 core/spec/models/spree/tax_rate_spec.rb | 4 ++--
 core/spec/models/spree/zone_spec.rb     | 8 ++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/core/app/models/spree/zone.rb b/core/app/models/spree/zone.rb
index 4df41080e0c..96735ff702a 100644
--- a/core/app/models/spree/zone.rb
+++ b/core/app/models/spree/zone.rb
@@ -58,6 +58,8 @@ def self.match(address)
     # current zone, if it's a state zone. If the passed-in zone has members, it
     # will also be in the result set.
     def self.with_shared_members(zone)
+      return none unless zone
+
       states_and_state_country_ids = zone.states.pluck(:id, :country_id).to_a
       state_ids = states_and_state_country_ids.map(&:first)
       state_country_ids = states_and_state_country_ids.map(&:second)
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 2e3012be8de..6997a66eda7 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -7,8 +7,8 @@
     context "when zone is nil" do
       let(:zone) { nil }
 
-      it "raises an exception" do
-        expect { rates_for_zone }.to raise_error(NameError)
+      it "should return an empty array" do
+        expect(subject).to eq([])
       end
     end
 
diff --git a/core/spec/models/spree/zone_spec.rb b/core/spec/models/spree/zone_spec.rb
index 36b13c718f6..8cc4f0a5829 100644
--- a/core/spec/models/spree/zone_spec.rb
+++ b/core/spec/models/spree/zone_spec.rb
@@ -348,6 +348,14 @@
       end
     end
 
+    context 'when passing nil' do
+      let!(:zone) { nil }
+
+      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|

From 132f02f246ff2c189f6e070317ce23f7d2344e4c Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Sun, 24 Jan 2016 17:05:55 -0600
Subject: [PATCH 05/13] Use Spree::TaxRate.for_zone scope in
 Spree::TaxRate.match

This is really an intermediary step. I'm using for_zone already in
Spree::TaxRate.match, which should result in a constant eight DB calls per
.match call. Previously, the number of DB calls would depend on how many
tax rates there are, and how these are configured (as Spree::Zone.contains?
would be run for every tax rate in the system).

A side effect of this refactoring is that the reimbursement specs had to be
amended a little: The zones had to be created `:with_country`, because otherwise
there'd be no shared members and the tax rates wouldn't apply. This actually makes
the tests slightly more realistic (what is a zone without members, anyway?).

Also, I had to change an expectation: The included tax total of the reimbursement
was previously expected to be negative (which is something that IMO should never
happen, as negative included taxes at the moment become positive additional taxes).
---
 core/app/models/spree/tax_rate.rb                    | 12 ++++--------
 core/spec/models/spree/reimbursement_spec.rb         |  2 +-
 .../spree/reimbursement_tax_calculator_spec.rb       |  6 +++---
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 4aab39adb43..6f37e9e39af 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -68,13 +68,13 @@ class TaxRate < Spree::Base
     #
     # Those rates should never come into play at all and only the French rates should apply.
     scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
+    scope :included_in_price, -> { where(included_in_price: true) }
 
     def self.match(order_tax_zone)
       return [] unless order_tax_zone
-      all_rates = includes(zone: { zone_members: :zoneable }).load
 
-      rates_for_order_zone = all_rates.select { |rate| rate.zone.contains?(order_tax_zone) }
-      rates_for_default_zone = all_rates.select(&:default_vat?)
+      rates_for_order_zone = for_zone(order_tax_zone)
+      rates_for_default_zone = for_zone(Spree::Zone.default_tax).included_in_price
 
       # Imagine with me this scenario:
       # You are living in Spain and you have a store which ships to France.
@@ -91,7 +91,7 @@ def self.match(order_tax_zone)
       # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
 
       order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
-      rates_for_default_zone.delete_if do |default_rate|
+      rates_for_default_zone.to_a.delete_if do |default_rate|
         order_zone_tax_categories.include?(default_rate.tax_category)
       end
 
@@ -149,10 +149,6 @@ def default_zone_or_zone_match?(order_tax_zone)
       Zone.default_tax.try!(:contains?, order_tax_zone) || zone.contains?(order_tax_zone)
     end
 
-    def default_vat?
-      included_in_price && zone.contains?(Spree::Zone.default_tax)
-    end
-
     private
 
     def create_label
diff --git a/core/spec/models/spree/reimbursement_spec.rb b/core/spec/models/spree/reimbursement_spec.rb
index b21612d209e..a7a582f56b9 100644
--- a/core/spec/models/spree/reimbursement_spec.rb
+++ b/core/spec/models/spree/reimbursement_spec.rb
@@ -49,7 +49,7 @@
     let!(:adjustments)            { [] } # placeholder to ensure it gets run prior the "before" at this level
 
     let!(:tax_rate)               { nil }
-    let!(:tax_zone)               { create :zone, default_tax: true }
+    let!(:tax_zone)               { create :zone, :with_country, default_tax: true }
     let(:shipping_method)         { create :shipping_method, zones: [tax_zone] }
     let(:variant)                 { create :variant }
     let(:order)                   { create(:order_with_line_items, state: 'payment', line_items_attributes: [{ variant: variant, price: line_items_price }], shipment_cost: 0, shipping_method: shipping_method) }
diff --git a/core/spec/models/spree/reimbursement_tax_calculator_spec.rb b/core/spec/models/spree/reimbursement_tax_calculator_spec.rb
index 2ce2c78c5b5..c49e4ab99d4 100644
--- a/core/spec/models/spree/reimbursement_tax_calculator_spec.rb
+++ b/core/spec/models/spree/reimbursement_tax_calculator_spec.rb
@@ -24,7 +24,7 @@
 
   context 'with additional tax' do
     let!(:tax_rate) { create(:tax_rate, name: "Sales Tax", amount: 0.10, included_in_price: false, zone: tax_zone) }
-    let(:tax_zone) { create(:zone, default_tax: true) }
+    let(:tax_zone) { create(:zone, :with_country, default_tax: true) }
 
     it 'sets additional_tax_total on the return items' do
       subject
@@ -37,13 +37,13 @@
 
   context 'with included tax' do
     let!(:tax_rate) { create(:tax_rate, name: "VAT Tax", amount: 0.1, included_in_price: true, zone: tax_zone) }
-    let(:tax_zone) { create(:zone, default_tax: true) }
+    let(:tax_zone) { create(:zone, :with_country, default_tax: true) }
 
     it 'sets included_tax_total on the return items' do
       subject
       return_item.reload
 
-      expect(return_item.included_tax_total).to be < 0
+      expect(return_item.included_tax_total).to be > 0
       expect(return_item.included_tax_total).to eq line_item.included_tax_total
     end
   end

From 7a1117eb628c246c088e10769cccaa5456c838af Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Sun, 24 Jan 2016 17:17:38 -0600
Subject: [PATCH 06/13] Extract tax adjuster helpers into module

The order adjuster and the item adjuster share code, because I need to run
some methods on the order level and cache them. This extracts these methods
into a module.

Inheritance would've also worked, but in this case I find a mixin
more fitting as the behaviour of the two classes is still quite different.
---
 core/app/models/spree/tax/item_adjuster.rb  | 10 ++--------
 core/app/models/spree/tax/order_adjuster.rb | 14 ++------------
 core/app/models/spree/tax/tax_helpers.rb    | 15 +++++++++++++++
 3 files changed, 19 insertions(+), 20 deletions(-)
 create mode 100644 core/app/models/spree/tax/tax_helpers.rb

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 3cb01f8ac4f..32b30b5ec2e 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -3,6 +3,8 @@ module Tax
     class ItemAdjuster
       attr_reader :item, :order
 
+      include TaxHelpers
+
       def initialize(item, options = {})
         @item = item
         @order = @item.order
@@ -25,14 +27,6 @@ def adjust!
       def rates_for_item
         @rates_for_item ||= rates_for_order_zone.select { |rate| rate.tax_category == item.tax_category }
       end
-
-      def rates_for_order_zone
-        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
-      end
-
-      def order_tax_zone
-        @order_tax_zone ||= order.tax_zone
-      end
     end
   end
 end
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
index 80dcac22a3f..45610b4006c 100644
--- a/core/app/models/spree/tax/order_adjuster.rb
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -3,6 +3,8 @@ module Tax
     class OrderAdjuster
       attr_reader :order
 
+      include TaxHelpers
+
       def initialize(order)
         @order = order
       end
@@ -14,18 +16,6 @@ def adjust!
           ItemAdjuster.new(item, rates_for_order_zone: rates_for_order_zone).adjust!
         end
       end
-
-      private
-
-      # When adjusting multiple items, we don't want to
-      # look up the zone rates for each individual item
-      def rates_for_order_zone
-        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
-      end
-
-      def order_tax_zone
-        @order_tax_zone ||= order.tax_zone
-      end
     end
   end
 end
diff --git a/core/app/models/spree/tax/tax_helpers.rb b/core/app/models/spree/tax/tax_helpers.rb
new file mode 100644
index 00000000000..9fb43c135fc
--- /dev/null
+++ b/core/app/models/spree/tax/tax_helpers.rb
@@ -0,0 +1,15 @@
+module Spree
+  module Tax
+    module TaxHelpers
+      private
+
+      def rates_for_order_zone
+        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
+      end
+
+      def order_tax_zone
+        @order_tax_zone ||= order.tax_zone
+      end
+    end
+  end
+end

From d5e15d0a11f92f528c1a911dbe06eb24f9aefb25 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Sun, 24 Jan 2016 17:43:43 -0600
Subject: [PATCH 07/13] Move TaxRate.match logic to TaxHelpers

TaxRate.match does some pretty weird things, and I want to change them.
This commit moves that code to a temporary `applicable_rates` helper.
---
 core/app/models/spree/tax/item_adjuster.rb    |  2 +-
 core/app/models/spree/tax/order_adjuster.rb   |  2 +-
 core/app/models/spree/tax/tax_helpers.rb      | 28 ++++++++++++++++++-
 core/app/models/spree/tax_rate.rb             | 28 -------------------
 .../models/spree/tax/item_adjuster_spec.rb    |  3 +-
 .../models/spree/tax/order_adjuster_spec.rb   |  3 +-
 core/spec/models/spree/tax_rate_spec.rb       |  6 ++--
 7 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 32b30b5ec2e..354e22e95cc 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -25,7 +25,7 @@ def adjust!
       private
 
       def rates_for_item
-        @rates_for_item ||= rates_for_order_zone.select { |rate| rate.tax_category == item.tax_category }
+        @rates_for_item ||= applicable_rates.select { |rate| rate.tax_category == item.tax_category }
       end
     end
   end
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
index 45610b4006c..2a62bfe8475 100644
--- a/core/app/models/spree/tax/order_adjuster.rb
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -13,7 +13,7 @@ def adjust!
         return unless order_tax_zone
 
         (order.line_items + order.shipments).each do |item|
-          ItemAdjuster.new(item, rates_for_order_zone: rates_for_order_zone).adjust!
+          ItemAdjuster.new(item, rates_for_order_zone: applicable_rates).adjust!
         end
       end
     end
diff --git a/core/app/models/spree/tax/tax_helpers.rb b/core/app/models/spree/tax/tax_helpers.rb
index 9fb43c135fc..7e8985a999e 100644
--- a/core/app/models/spree/tax/tax_helpers.rb
+++ b/core/app/models/spree/tax/tax_helpers.rb
@@ -3,8 +3,34 @@ module Tax
     module TaxHelpers
       private
 
+      # Imagine with me this scenario:
+      # You are living in Spain and you have a store which ships to France.
+      # Spain is therefore your default tax rate.
+      # When you ship to Spain, you want the Spanish rate to apply.
+      # When you ship to France, you want the French rate to apply.
+      #
+      # Normally, Spree would notice that you have two potentially applicable
+      # tax rates for one particular item.
+      # When you ship to Spain, only the Spanish one will apply.
+      # When you ship to France, you'll see a Spanish refund AND a French tax.
+      # This little bit of code at the end stops the Spanish refund from appearing.
+      #
+      # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
+      def applicable_rates
+        order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
+        default_rates_with_unmatched_tax_category = rates_for_default_zone.to_a.delete_if do |default_rate|
+          order_zone_tax_categories.include?(default_rate.tax_category)
+        end
+
+        (rates_for_order_zone + default_rates_with_unmatched_tax_category).uniq
+      end
+
       def rates_for_order_zone
-        @rates_for_order_zone ||= Spree::TaxRate.match(order_tax_zone)
+        @rates_for_order_zone ||= Spree::TaxRate.for_zone(order_tax_zone)
+      end
+
+      def rates_for_default_zone
+        @rates_for_default_zone ||= Spree::TaxRate.for_zone(Spree::Zone.default_tax)
       end
 
       def order_tax_zone
diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 6f37e9e39af..15e982a7f36 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -70,34 +70,6 @@ class TaxRate < Spree::Base
     scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
     scope :included_in_price, -> { where(included_in_price: true) }
 
-    def self.match(order_tax_zone)
-      return [] unless order_tax_zone
-
-      rates_for_order_zone = for_zone(order_tax_zone)
-      rates_for_default_zone = for_zone(Spree::Zone.default_tax).included_in_price
-
-      # Imagine with me this scenario:
-      # You are living in Spain and you have a store which ships to France.
-      # Spain is therefore your default tax rate.
-      # When you ship to Spain, you want the Spanish rate to apply.
-      # When you ship to France, you want the French rate to apply.
-      #
-      # Normally, Spree would notice that you have two potentially applicable
-      # tax rates for one particular item.
-      # When you ship to Spain, only the Spanish one will apply.
-      # When you ship to France, you'll see a Spanish refund AND a French tax.
-      # This little bit of code at the end stops the Spanish refund from appearing.
-      #
-      # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
-
-      order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
-      rates_for_default_zone.to_a.delete_if do |default_rate|
-        order_zone_tax_categories.include?(default_rate.tax_category)
-      end
-
-      (rates_for_order_zone + rates_for_default_zone).uniq
-    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/item_adjuster_spec.rb b/core/spec/models/spree/tax/item_adjuster_spec.rb
index 7eee729c96c..bc613178e19 100644
--- a/core/spec/models/spree/tax/item_adjuster_spec.rb
+++ b/core/spec/models/spree/tax/item_adjuster_spec.rb
@@ -44,7 +44,8 @@
       before do
         expect(item).to receive(:update_column)
 
-        expect(Spree::TaxRate).to receive(:match).with(tax_zone).and_return(rates_for_order_zone)
+        expect(Spree::TaxRate).to receive(:for_zone).with(tax_zone).and_return(rates_for_order_zone)
+        expect(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
       end
 
       context 'when there are no matching rates' do
diff --git a/core/spec/models/spree/tax/order_adjuster_spec.rb b/core/spec/models/spree/tax/order_adjuster_spec.rb
index d58b960e01e..378d0540bd2 100644
--- a/core/spec/models/spree/tax/order_adjuster_spec.rb
+++ b/core/spec/models/spree/tax/order_adjuster_spec.rb
@@ -20,7 +20,8 @@
 
     before do
       expect(order).to receive(:tax_zone).at_least(:once).and_return(zone)
-      expect(Spree::TaxRate).to receive(:match).and_return(rates_for_order_zone)
+      expect(Spree::TaxRate).to receive(:for_zone).with(zone).and_return(rates_for_order_zone)
+      expect(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
     end
 
     it 'calls the item adjuster with all line items' do
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 6997a66eda7..edbc787ec2f 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -106,7 +106,8 @@
       let(:line_items) { [line_item] }
 
       before do
-        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive(:for_zone).with(zone).and_return([rate_1, rate_2])
+        allow(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
         allow(order).to receive(:line_items).and_return([line_item])
       end
 
@@ -128,7 +129,8 @@
       end
 
       before do
-        allow(Spree::TaxRate).to receive_messages match: [rate_1, rate_2]
+        allow(Spree::TaxRate).to receive(:for_zone).with(zone).and_return([rate_1, rate_2])
+        allow(Spree::TaxRate).to receive(:for_zone).with(Spree::Zone.default_tax).and_return([])
         allow(order).to receive(:shipments).and_return([shipment])
       end
 

From c66ee856221e99413630dc825367e958e7ae162d Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Mon, 25 Jan 2016 10:45:11 -0600
Subject: [PATCH 08/13] Handle default VATs and tax rates for order separately

Before this commit, the taxation system would handle default VATs and
rates that *actually* apply to the order's item with the same code.
However, they do fundamentally different things: For example, the
`pre_tax_amount` should only be calculated from default VAT rates.

This commit introduces a new private method on the ItemAdjuster to
handle "deductable" VAT rates. This fixes some of the taxation specs,
because now the pre tax amount is calculated correctly, and it opens the
path for a cleverer handling of default VATs (namely, changing the line
item's price).

Additionally, this commit introduces cached options for the entire order
to be passed into the item adjuster.
---
 core/app/models/spree/tax/item_adjuster.rb  | 38 +++++++++++++++++++--
 core/app/models/spree/tax/order_adjuster.rb | 14 +++++++-
 core/app/models/spree/tax/tax_helpers.rb    | 38 ++++++++-------------
 core/spec/models/spree/tax_rate_spec.rb     |  2 --
 4 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 354e22e95cc..6b67bd3a539 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -8,8 +8,13 @@ class ItemAdjuster
       def initialize(item, options = {})
         @item = item
         @order = @item.order
-        # set the instance variable so `TaxRate.match` is only called when necessary
+        # set caching instance variables so order-wide calculations are only done
+        # once per order
         @rates_for_order_zone = options[:rates_for_order_zone]
+        @rates_for_default_zone = options[:rates_for_default_zone]
+        @order_tax_zone = options[:order_tax_zone]
+        @default_tax_zone = options[:default_tax_zone]
+        @outside_default_vat_zone = options[:outside_default_vat_zone]
       end
 
       def adjust!
@@ -19,13 +24,42 @@ def adjust!
 
         TaxRate.store_pre_tax_amount(item, rates_for_item)
 
+        if outside_default_vat_zone?
+          handle_deductable_default_vats
+        end
+
         rates_for_item.map { |rate| rate.adjust(order_tax_zone, item) }
       end
 
       private
 
+      def handle_deductable_default_vats
+        # 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.
+        return if rates_for_item.any? { |rate| rate.included_in_price }
+
+        default_rates_for_item.each do |default_rate|
+          default_rate.adjust(order_tax_zone, item)
+        end
+      end
+
+      def default_rates_for_item
+        rates_for_default_zone.included_in_price.select { |rate| rate.tax_category == item.tax_category }
+      end
+
       def rates_for_item
-        @rates_for_item ||= applicable_rates.select { |rate| rate.tax_category == item.tax_category }
+        @rates_for_item ||= rates_for_order_zone.select { |rate| rate.tax_category == item.tax_category }
       end
     end
   end
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
index 2a62bfe8475..422f7fe9455 100644
--- a/core/app/models/spree/tax/order_adjuster.rb
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -13,9 +13,21 @@ def adjust!
         return unless order_tax_zone
 
         (order.line_items + order.shipments).each do |item|
-          ItemAdjuster.new(item, rates_for_order_zone: applicable_rates).adjust!
+          ItemAdjuster.new(item, order_wide_options).adjust!
         end
       end
+
+      private
+
+      def order_wide_options
+        {
+          rates_for_order_zone: rates_for_order_zone,
+          rates_for_default_zone: rates_for_default_zone,
+          order_tax_zone: order_tax_zone,
+          default_tax_zone: default_tax_zone,
+          outside_default_vat_zone: outside_default_vat_zone?
+        }
+      end
     end
   end
 end
diff --git a/core/app/models/spree/tax/tax_helpers.rb b/core/app/models/spree/tax/tax_helpers.rb
index 7e8985a999e..1085601136f 100644
--- a/core/app/models/spree/tax/tax_helpers.rb
+++ b/core/app/models/spree/tax/tax_helpers.rb
@@ -3,39 +3,31 @@ module Tax
     module TaxHelpers
       private
 
-      # Imagine with me this scenario:
-      # You are living in Spain and you have a store which ships to France.
-      # Spain is therefore your default tax rate.
-      # When you ship to Spain, you want the Spanish rate to apply.
-      # When you ship to France, you want the French rate to apply.
-      #
-      # Normally, Spree would notice that you have two potentially applicable
-      # tax rates for one particular item.
-      # When you ship to Spain, only the Spanish one will apply.
-      # When you ship to France, you'll see a Spanish refund AND a French tax.
-      # This little bit of code at the end stops the Spanish refund from appearing.
-      #
-      # For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
-      def applicable_rates
-        order_zone_tax_categories = rates_for_order_zone.map(&:tax_category)
-        default_rates_with_unmatched_tax_category = rates_for_default_zone.to_a.delete_if do |default_rate|
-          order_zone_tax_categories.include?(default_rate.tax_category)
-        end
-
-        (rates_for_order_zone + default_rates_with_unmatched_tax_category).uniq
-      end
-
       def rates_for_order_zone
         @rates_for_order_zone ||= Spree::TaxRate.for_zone(order_tax_zone)
       end
 
       def rates_for_default_zone
-        @rates_for_default_zone ||= Spree::TaxRate.for_zone(Spree::Zone.default_tax)
+        @rates_for_default_zone ||= Spree::TaxRate.for_zone(default_tax_zone)
       end
 
       def order_tax_zone
         @order_tax_zone ||= order.tax_zone
       end
+
+      def default_tax_zone
+        # Memoizing values that are potentially `false` can not use the `||=` shorthand
+        @default_tax_zone.nil? ? Spree::Zone.default_tax.presence : @default_tax_zone
+      end
+
+      def outside_default_vat_zone?
+        # Memoizing booleans can not use the `||=` shorthand
+        if @outside_default_vat_zone.nil?
+          @outside_default_vat_zone = default_tax_zone && !default_tax_zone.contains?(order_tax_zone)
+        else
+          @outside_default_vat_zone
+        end
+      end
     end
   end
 end
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index edbc787ec2f..83a67d93a0a 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -321,12 +321,10 @@
           end
 
           it 'has 2.02 of included tax' do
-            pending 'waiting for the MOSS refactoring'
             expect(line_item.included_tax_total).to eq(2.02)
           end
 
           it 'has a constant amount pre tax' do
-            pending 'but it changes to 8.06, because Spree thinks both VATs apply'
             expect(line_item.pre_tax_amount).to eq(8.40)
           end
         end

From a6b0e4ec08bc1e0823e5b9ab829befc41c15847a Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 26 Jan 2016 07:14:16 -0600
Subject: [PATCH 09/13] Add `initial_price` column to line items

The "initial price" is the price in the default tax zone. It will be used when
calculating net amounts and new prices for foreign VAT zones.

This has been discussed in https://github.com/solidusio/solidus/issues/532
---
 core/app/models/spree/line_item.rb            |  1 +
 ...6123300_add_initial_price_to_line_items.rb |  5 +++++
 core/spec/models/spree/line_item_spec.rb      | 21 +++++++++++++++++++
 3 files changed, 27 insertions(+)
 create mode 100644 core/db/migrate/20160126123300_add_initial_price_to_line_items.rb

diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb
index 13ad17e54bd..cc96247f22e 100644
--- a/core/app/models/spree/line_item.rb
+++ b/core/app/models/spree/line_item.rb
@@ -141,6 +141,7 @@ def set_pricing_attributes
       self.currency ||= variant.currency
       self.cost_price ||= variant.cost_price
       self.price ||= variant.price
+      self.initial_price ||= price
     end
 
     def handle_copy_price_override
diff --git a/core/db/migrate/20160126123300_add_initial_price_to_line_items.rb b/core/db/migrate/20160126123300_add_initial_price_to_line_items.rb
new file mode 100644
index 00000000000..36d986ae931
--- /dev/null
+++ b/core/db/migrate/20160126123300_add_initial_price_to_line_items.rb
@@ -0,0 +1,5 @@
+class AddInitialPriceToLineItems < ActiveRecord::Migration
+  def change
+    add_column :spree_line_items, :initial_price, :decimal, precision: 8, scale: 2
+  end
+end
diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb
index e85461c7304..c5148cec62d 100644
--- a/core/spec/models/spree/line_item_spec.rb
+++ b/core/spec/models/spree/line_item_spec.rb
@@ -4,6 +4,27 @@
   let(:order) { create :order_with_line_items, line_items_count: 1 }
   let(:line_item) { order.line_items.first }
 
+  describe 'initial_price' do
+    it 'is available' do
+      expect(line_item).to respond_to(:initial_price)
+    end
+
+    it 'returns a BigDecimal' do
+      expect(line_item.initial_price).to be_a(BigDecimal)
+    end
+
+    it 'is equal to the initial price' do
+      expect(line_item.initial_price).to eq(line_item.price)
+    end
+
+    it 'does not change even if the price is modified' do
+      new_item = create(:line_item, price: 15)
+      expect(new_item.initial_price).to eq(15)
+      new_item.update(price: 10)
+      expect(new_item.initial_price).to eq(15)
+    end
+  end
+
   context '#destroy' do
     it "fetches deleted products" do
       line_item.product.destroy

From 9cdf6db095b048508601691aea889fbd890e2d21 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 26 Jan 2016 07:34:25 -0600
Subject: [PATCH 10/13] Change line item prices depending on VAT in order tax
 zone

This is the simplest thing that could possibly work. It does.
---
 core/app/models/spree/tax/item_adjuster.rb | 29 ++++++----------------
 core/spec/models/spree/tax_rate_spec.rb    | 10 --------
 2 files changed, 7 insertions(+), 32 deletions(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 6b67bd3a539..b85080698a9 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -22,36 +22,21 @@ def adjust!
         # Using .destroy_all to make sure callbacks fire
         item.adjustments.tax.destroy_all
 
-        TaxRate.store_pre_tax_amount(item, rates_for_item)
-
         if outside_default_vat_zone?
-          handle_deductable_default_vats
+          adjust_item_price
         end
 
+        TaxRate.store_pre_tax_amount(@item, rates_for_item)
+
         rates_for_item.map { |rate| rate.adjust(order_tax_zone, item) }
       end
 
       private
 
-      def handle_deductable_default_vats
-        # 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.
-        return if rates_for_item.any? { |rate| rate.included_in_price }
-
-        default_rates_for_item.each do |default_rate|
-          default_rate.adjust(order_tax_zone, item)
-        end
+      def adjust_item_price
+        default_vat_amounts = default_rates_for_item.map(&:amount).sum
+        included_item_rate_amounts = rates_for_item.select(&:included_in_price).map(&:amount).sum
+        item.price = item.initial_price / (1 + default_vat_amounts) * (1 + included_item_rate_amounts)
       end
 
       def default_rates_for_item
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index 83a67d93a0a..f5fd083251a 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -312,7 +312,6 @@
           let(:variant) { download }
 
           it 'still has an adjusted price for romania' do
-            pending "waiting for the MOSS refactoring"
             expect(line_item.price).to eq(10.42)
           end
 
@@ -338,7 +337,6 @@
           let(:variant) { book }
 
           it 'should sell at the net price' do
-            pending "Prices have to be adjusted"
             expect(line_item.price).to eq(18.69)
           end
 
@@ -347,7 +345,6 @@
           end
 
           it 'has no tax adjustments' do
-            pending "Right now it gets a refund"
             expect(line_item.adjustments.tax.count).to eq(0)
           end
 
@@ -356,7 +353,6 @@
           end
 
           it 'has no additional tax' do
-            pending 'but there is a refund, still'
             expect(line_item.additional_tax_total).to eq(0)
           end
 
@@ -369,12 +365,10 @@
           let(:variant) { sweater }
 
           it 'should sell at the net price' do
-            pending 'but prices are not adjusted according to the zone yet'
             expect(line_item.price).to eq(25.21)
           end
 
           it 'has no tax adjustments' do
-            pending 'but it has a refund'
             expect(line_item.adjustments.tax.count).to eq(0)
           end
 
@@ -383,7 +377,6 @@
           end
 
           it 'has no additional tax' do
-            pending 'but it has a refund for included taxes wtf'
             expect(line_item.additional_tax_total).to eq(0)
           end
 
@@ -396,12 +389,10 @@
           let(:variant) { download }
 
           it 'should sell at the net price' do
-            pending 'but prices are not adjusted yet'
             expect(line_item.price).to eq(8.40)
           end
 
           it 'has no tax adjustments' do
-            pending 'but a refund is created'
             expect(line_item.adjustments.tax.count).to eq(0)
           end
 
@@ -410,7 +401,6 @@
           end
 
           it 'has no additional tax' do
-            pending 'but an tax refund that disguises as additional tax is created'
             expect(line_item.additional_tax_total).to eq(0)
           end
 

From 98bca71e6b3c7cba5584e8fd793f971e8662d35b Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 26 Jan 2016 07:44:00 -0600
Subject: [PATCH 11/13] Remove dead code from tax_rate.rb

Since now there can be no VAT refunds anymore, we can get rid
of all the code in tax_rate.rb that exclusively deals with refunds.
---
 core/app/models/spree/tax_rate.rb | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 15e982a7f36..002357a8a5e 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -88,37 +88,22 @@ def self.store_pre_tax_amount(item, rates)
     end
 
     # Creates necessary tax adjustments for the order.
-    def adjust(order_tax_zone, item)
+    def adjust(item)
       amount = compute_amount(item)
       return if amount == 0
 
-      included = included_in_price && default_zone_or_zone_match?(order_tax_zone)
-
-      if amount < 0
-        label = Spree.t(:refund) + ' ' + create_label
-      end
-
-      adjustments.create!({
+      adjustments.create!(
         adjustable: item,
         amount: amount,
         order_id: item.order_id,
-        label: label || create_label,
-        included: included
-      })
+        label: create_label,
+        included: included_in_price
+      )
     end
 
     # This method is used by Adjustment#update to recalculate the cost.
     def compute_amount(item)
-      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
-    end
-
-    def default_zone_or_zone_match?(order_tax_zone)
-      Zone.default_tax.try!(:contains?, order_tax_zone) || zone.contains?(order_tax_zone)
+      calculator.compute(item)
     end
 
     private

From c6b67a1e938dba7fb05d6745f8d61e05aacc402d Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Tue, 26 Jan 2016 10:25:50 -0600
Subject: [PATCH 12/13] Add TaxRate.applicable_for?(item)

Which tax rates are applicable is for the moment determined by the tax
category of the item. There is cases though where more things apply (the customer,
for example, might be a business customer). Besides, putting this into a method on
TaxRate unifies that check.
---
 core/app/models/spree/tax/item_adjuster.rb | 4 ++--
 core/app/models/spree/tax_rate.rb          | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index b85080698a9..53906ede006 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -40,11 +40,11 @@ def adjust_item_price
       end
 
       def default_rates_for_item
-        rates_for_default_zone.included_in_price.select { |rate| rate.tax_category == item.tax_category }
+        rates_for_default_zone.included_in_price.select { |rate| rate.applicable_for?(item) }
       end
 
       def rates_for_item
-        @rates_for_item ||= rates_for_order_zone.select { |rate| rate.tax_category == item.tax_category }
+        @rates_for_item ||= rates_for_order_zone.select { |rate| rate.applicable_for?(item) }
       end
     end
   end
diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 002357a8a5e..d2006f3ea6c 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -70,6 +70,10 @@ class TaxRate < Spree::Base
     scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
     scope :included_in_price, -> { where(included_in_price: true) }
 
+    def applicable_for?(item)
+      tax_category == item.tax_category
+    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

From 499652535cbcdfc4cd1e7a7d67f922bc1b6fd405 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@googlemail.com>
Date: Wed, 18 Nov 2015 22:44:36 +0100
Subject: [PATCH 13/13] 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 d2006f3ea6c..9c48a51697c 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
 
     # Finds geographically matching tax rates for a tax zone.
     # We do not know if they are/aren't applicable until we attempt to apply these rates to