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 1/9] 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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 6/9] 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 7/9] 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 11a9b3d485a77021ae288a896d86e732d904c1b6 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Wed, 3 Feb 2016 11:43:19 +0100
Subject: [PATCH 8/9] Add YARD documentation to Tax Adjuster classes

---
 core/app/models/spree/tax/item_adjuster.rb  | 12 +++++++++++-
 core/app/models/spree/tax/order_adjuster.rb |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb
index 354e22e95cc..5431d9a863e 100644
--- a/core/app/models/spree/tax/item_adjuster.rb
+++ b/core/app/models/spree/tax/item_adjuster.rb
@@ -1,17 +1,27 @@
 module Spree
   module Tax
+    # Adjust a single taxable item (line item or shipment)
     class ItemAdjuster
       attr_reader :item, :order
 
       include TaxHelpers
 
+      # @param [Spree::LineItem,Spree::Shipment] item to adjust
+      # @param [Hash] options like already known tax rates for the order's zone
       def initialize(item, options = {})
         @item = item
         @order = @item.order
-        # set the instance variable so `TaxRate.match` is only called when necessary
+        # set instance variable so `TaxRate.match` is only called when necessary
         @rates_for_order_zone = options[:rates_for_order_zone]
       end
 
+      # Deletes all existing tax adjustments and creates new adjustments for all
+      # (geographically and category-wise) applicable tax rates.
+      #
+      # Creating the adjustments will also run the ItemAdjustments class and
+      # persist all taxation and promotion totals on the item.
+      #
+      # @return [Array<Spree::Adjustment>] newly created adjustments
       def adjust!
         return unless order_tax_zone
         # Using .destroy_all to make sure callbacks fire
diff --git a/core/app/models/spree/tax/order_adjuster.rb b/core/app/models/spree/tax/order_adjuster.rb
index 2a62bfe8475..1a9cc5a9e30 100644
--- a/core/app/models/spree/tax/order_adjuster.rb
+++ b/core/app/models/spree/tax/order_adjuster.rb
@@ -1,14 +1,18 @@
 module Spree
   module Tax
+    # Add tax adjustments to all line items and shipments in an order
     class OrderAdjuster
       attr_reader :order
 
       include TaxHelpers
 
+      # @param [Spree::Order] order to be adjusted
       def initialize(order)
         @order = order
       end
 
+      # Creates tax adjustments for all taxable items (shipments and line items)
+      # in the given order.
       def adjust!
         return unless order_tax_zone
 

From 52ca997b26bdd695ee04373a18e9440801f61e91 Mon Sep 17 00:00:00 2001
From: Martin Meyerhoff <mamhoff@gmail.com>
Date: Wed, 3 Feb 2016 12:04:19 +0100
Subject: [PATCH 9/9] Reintroduce TaxRate.adjust as a deprecated method

Add Spree::TaxRate.adjust as a deprecated method, and test for the deprecation
warnings. The deleted specs are covered in the specs for the ItemAdjuster.
---
 core/app/models/spree/tax_rate.rb       | 13 ++++++
 core/spec/models/spree/tax_rate_spec.rb | 57 +++++--------------------
 2 files changed, 24 insertions(+), 46 deletions(-)

diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb
index 15e982a7f36..c287e1f3689 100644
--- a/core/app/models/spree/tax_rate.rb
+++ b/core/app/models/spree/tax_rate.rb
@@ -70,6 +70,19 @@ 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) }
 
+    # Create tax adjustments for some items that have the same tax zone.
+    #
+    # @deprecated Please use Spree::Tax::OrderAdjuster or Spree::Tax::ItemAdjuster instead.
+    #
+    # @param [Spree::Zone] order_tax_zone is the smalles applicable zone to the order's tax address
+    # @param [Array<Spree::LineItem,Spree::Shipment>] items to be adjusted
+    def self.adjust(order_tax_zone, items)
+      ActiveSupport::Deprecation.warn("Please use Spree::Tax::OrderAdjuster or Spree::Tax::ItemAdjuster instead", caller)
+      items.map do |item|
+        Spree::Tax::ItemAdjuster.new(item, rates_for_order_zone: for_zone(order_tax_zone)).adjust!
+      end
+    end
+
     # Pre-tax amounts must be stored so that we can calculate
     # correct rate amounts in the future. For example:
     # https://github.com/spree/spree/issues/4318#issuecomment-34723428
diff --git a/core/spec/models/spree/tax_rate_spec.rb b/core/spec/models/spree/tax_rate_spec.rb
index edbc787ec2f..d0ad1c0f09f 100644
--- a/core/spec/models/spree/tax_rate_spec.rb
+++ b/core/spec/models/spree/tax_rate_spec.rb
@@ -86,64 +86,29 @@
 
   context ".adjust" do
     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) }
-    let(:rate_2) { stub_model(Spree::TaxRate, tax_category: tax_category_2) }
 
     context "with line items" do
-      let(:line_item) do
-        stub_model(Spree::LineItem,
-          price: 10.0,
-          quantity: 1,
-          tax_category: tax_category_1,
-          variant: stub_model(Spree::Variant),
-          order: order
-        )
-      end
+      let(:line_item) { stub_model(Spree::LineItem) }
 
-      let(:line_items) { [line_item] }
-
-      before do
-        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
-
-      it "should only apply adjustments for matching rates" do
-        expect(rate_1).to receive(:adjust)
-        expect(rate_2).not_to receive(:adjust)
-        Spree::Tax::OrderAdjuster.new(order).adjust!
+      it 'should emit a deprecation warning and call the item adjuster' do
+        expect(ActiveSupport::Deprecation).to receive(:warn)
+        expect(Spree::Tax::ItemAdjuster).to receive_message_chain(:new, :adjust!)
+        Spree::TaxRate.adjust(zone, [line_item])
       end
     end
 
     context "with shipments" do
-      let(:shipment) do
-        stub_model(
-          Spree::Shipment,
-          cost: 10.0,
-          tax_category: tax_category_1,
-          order: order
-        )
-      end
+      let(:shipment) { stub_model(Spree::Shipment) }
 
-      before do
-        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
-
-      it "should apply adjustments for matching rates" do
-        expect(rate_1).to receive(:adjust)
-        expect(rate_2).not_to receive(:adjust)
-        Spree::Tax::OrderAdjuster.new(order).adjust!
+      it 'should emit a deprecation warning and call the item adjuster' do
+        expect(ActiveSupport::Deprecation).to receive(:warn)
+        expect(Spree::Tax::ItemAdjuster).to receive_message_chain(:new, :adjust!)
+        Spree::TaxRate.adjust(zone, [shipment])
       end
     end
   end
 
-  # While the above test is nice and fast - let me tell you a story or two here.
-  context ".adjust" do
+  context "Taxation system integration tests" do
     let(:order) { create :order }
     let(:book_product) { create :product, price: 20, name: "Book", tax_category: books_category }
     let(:download_product) { create :product, price: 10, name: "Download", tax_category: digital_category }