From 7e0ba4277b014955480dd0ec47e4b8846af34477 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Fri, 23 Sep 2016 07:54:53 -0600
Subject: [PATCH 1/5] Merge tax updating code and deprecate
 Order#create_tax_charge!

Previously we had two different sets of tax updating code.  One was
Order#create_tax_charge! and the other was the code in OrderUpdater.
The effect of the former encompasses the effect of the latter, so
functionally we can merge these.

This may result in increased load since we're now using the more
encompassing method which was more heavyweight.
---
 CHANGELOG.md                                              | 2 +-
 core/app/models/spree/order.rb                            | 4 +++-
 core/app/models/spree/order/checkout.rb                   | 2 --
 core/app/models/spree/order_contents.rb                   | 3 ---
 core/app/models/spree/order_updater.rb                    | 4 ++--
 core/lib/spree/core/unreturned_item_charger.rb            | 1 -
 core/lib/spree/testing_support/factories/order_factory.rb | 1 -
 core/spec/lib/spree/core/unreturned_item_charger_spec.rb  | 1 -
 core/spec/models/spree/order_contents_spec.rb             | 5 -----
 9 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7fcf4faa6d8..ff9a4e8eea8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,7 +18,7 @@
 *   Remove callback `Spree::LineItem.after_create :update_tax_charge`
 
     Any code that creates `LineItem`s outside the context of OrderContents
-    should ensure that it calls `order.create_tax_charge!` after doing so.
+    should ensure that it calls `order.update!` after doing so.
 
 *   Mark `Spree::Tax::ItemAdjuster` as api-private
 
diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index faa851c8144..1c9755f62b8 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -17,7 +17,7 @@ module Spree
   # `checkout_allowed?` or `payment_required?`.
   #
   #  * Implements an interface for mutating the order with methods like
-  # `create_tax_charge!` and `fulfill!`.
+  # `empty!` and `fulfill!`.
   #
   class Order < Spree::Base
     ORDER_NUMBER_LENGTH  = 9
@@ -348,9 +348,11 @@ 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.
+    # @deprecated This now happens during #update!
     def create_tax_charge!
       Spree::Tax::OrderAdjuster.new(self).adjust!
     end
+    deprecate create_tax_charge!: :update!, deprecator: Spree::Deprecation
 
     def outstanding_balance
       # If reimbursement has happened add it back to total to prevent balance_due payment state
diff --git a/core/app/models/spree/order/checkout.rb b/core/app/models/spree/order/checkout.rb
index 9951978f66e..9e4d77ca253 100644
--- a/core/app/models/spree/order/checkout.rb
+++ b/core/app/models/spree/order/checkout.rb
@@ -78,7 +78,6 @@ def self.define_state_machine!
 
                 after_transition to: :complete, do: :add_payment_sources_to_wallet
                 before_transition to: :payment, do: :set_shipments_cost
-                before_transition to: :payment, do: :create_tax_charge!
                 before_transition to: :payment, do: :assign_default_credit_card
 
                 before_transition to: :confirm, do: :add_store_credit_payments
@@ -90,7 +89,6 @@ def self.define_state_machine!
               before_transition from: :cart, do: :ensure_line_items_present
 
               if states[:address]
-                before_transition from: :address, do: :create_tax_charge!
                 before_transition to: :address, do: :assign_default_addresses!
                 before_transition from: :address, do: :persist_user_address!
               end
diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb
index 35444630cc0..5997c9c9f53 100644
--- a/core/app/models/spree/order_contents.rb
+++ b/core/app/models/spree/order_contents.rb
@@ -36,8 +36,6 @@ def remove_line_item(line_item, options = {})
 
     def update_cart(params)
       if order.update_attributes(params)
-        order.create_tax_charge!
-
         unless order.completed?
           order.line_items = order.line_items.select { |li| li.quantity > 0 }
           # Update totals, then check if the order is eligible for any cart promotions.
@@ -77,7 +75,6 @@ def after_add_or_remove(line_item, options = {})
       shipment = options[:shipment]
       shipment.present? ? shipment.update_amounts : order.ensure_updated_shipments
       PromotionHandler::Cart.new(order, line_item).activate
-      order.create_tax_charge!
       reload_totals
       line_item
     end
diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index 38a7c70158a..c65d594c2a5 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -197,10 +197,10 @@ def update_order_promotions
     end
 
     def update_taxes
+      Spree::Tax::OrderAdjuster.new(order).adjust!
+
       [*line_items, *shipments].each do |item|
         tax_adjustments = item.adjustments.select(&:tax?)
-
-        tax_adjustments.each(&:update!)
         # Tax adjustments come in not one but *two* exciting flavours:
         # Included & additional
 
diff --git a/core/lib/spree/core/unreturned_item_charger.rb b/core/lib/spree/core/unreturned_item_charger.rb
index c94c406fb86..5041518c90f 100644
--- a/core/lib/spree/core/unreturned_item_charger.rb
+++ b/core/lib/spree/core/unreturned_item_charger.rb
@@ -24,7 +24,6 @@ def charge_for_items
       new_order.associate_user!(@original_order.user) if @original_order.user
 
       add_exchange_variants_to_order
-      new_order.create_tax_charge!
       set_shipment_for_new_order
 
       new_order.update!
diff --git a/core/lib/spree/testing_support/factories/order_factory.rb b/core/lib/spree/testing_support/factories/order_factory.rb
index cae21cfed32..722b31be90e 100644
--- a/core/lib/spree/testing_support/factories/order_factory.rb
+++ b/core/lib/spree/testing_support/factories/order_factory.rb
@@ -45,7 +45,6 @@
           create(:line_item, attributes)
         end
         order.line_items.reload
-        order.create_tax_charge!
 
         create(:shipment, order: order, cost: evaluator.shipment_cost, shipping_method: evaluator.shipping_method, stock_location: evaluator.stock_location)
         order.shipments.reload
diff --git a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
index eeb33db3cb2..adb5b7cc383 100644
--- a/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
+++ b/core/spec/lib/spree/core/unreturned_item_charger_spec.rb
@@ -53,7 +53,6 @@
 
       it "applies tax" do
         exchange_order = exchange_shipment.order
-        exchange_order.create_tax_charge!
         exchange_order.update!
         subject
         expect(new_order.additional_tax_total).to be > 0
diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb
index 4ecc9b23f61..ef2da601e80 100644
--- a/core/spec/models/spree/order_contents_spec.rb
+++ b/core/spec/models/spree/order_contents_spec.rb
@@ -256,11 +256,6 @@
       }.to change { subject.order.total }
     end
 
-    it "updates tax adjustments" do
-      expect(subject.order).to receive(:create_tax_charge!)
-      subject.update_cart params
-    end
-
     context "submits item quantity 0" do
       let(:params) do
         { line_items_attributes: {

From 86e7435c882c1850c16b39b6482ce37dc9c8e840 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Mon, 26 Sep 2016 13:51:37 -0600
Subject: [PATCH 2/5] Fix non-realistic specs for updated tax code

We had several specs that manually created tax adjustments rather than
setting up a real tax rate to be applied to the order.  This fixes
that.
---
 .../features/admin/orders/adjustments_spec.rb | 53 ++++++++++---------
 core/spec/models/spree/order/updating_spec.rb |  4 +-
 .../models/spree/order_cancellations_spec.rb  | 10 ++--
 core/spec/models/spree/order_updater_spec.rb  |  1 -
 core/spec/models/spree/shipment_spec.rb       | 19 +++++--
 5 files changed, 51 insertions(+), 36 deletions(-)

diff --git a/backend/spec/features/admin/orders/adjustments_spec.rb b/backend/spec/features/admin/orders/adjustments_spec.rb
index 4650a27de6c..3cae2732ac9 100644
--- a/backend/spec/features/admin/orders/adjustments_spec.rb
+++ b/backend/spec/features/admin/orders/adjustments_spec.rb
@@ -3,29 +3,26 @@
 describe "Adjustments", type: :feature do
   stub_authorization!
 
-  let!(:order) { create(:completed_order_with_totals, line_items_count: 5) }
-  let!(:line_item) do
-    line_item = order.line_items.first
-    # so we can be sure of a determinate price in our assertions
-    line_item.update_column(:price, 10)
-    line_item
+  let!(:ship_address) { create(:address) }
+  let!(:tax_zone) { create(:global_zone) } # will include the above address
+  let!(:tax_rate) { create(:tax_rate, amount: 0.20, zone: tax_zone, tax_category: tax_category) }
+
+  let!(:order) do
+    create(
+      :completed_order_with_totals,
+      line_items_attributes: [{ price: 10, variant: variant }] * 5,
+      ship_address: ship_address,
+    )
   end
+  let!(:line_item) { order.line_items[0] }
 
-  let!(:tax_adjustment) do
-    create(:tax_adjustment,
-      adjustable: line_item,
-      finalized: true,
-      order: order,
-      label: "VAT 5%",
-      amount: 10)
-  end
+  let(:tax_category) { create(:tax_category) }
+  let(:variant) { create(:variant, tax_category: tax_category) }
 
   let!(:adjustment) { order.adjustments.create!(order: order, label: 'Rebate', amount: 10) }
 
   before(:each) do
-    # To ensure the order totals are correct
-    order.update_totals
-    order.persist_totals
+    order.update!
 
     visit spree.admin_path
     click_link "Orders"
@@ -35,9 +32,9 @@
 
   context "admin managing adjustments" do
     it "should display the correct values for existing order adjustments" do
-      within_row(1) do
-        expect(column_text(2)).to eq("VAT 5%")
-        expect(column_text(3)).to eq("$10.00")
+      within first('table tr', text: 'Tax') do
+        expect(column_text(2)).to match(/TaxCategory - \d+ 20\.000%/)
+        expect(column_text(3)).to eq("$2.00")
       end
     end
 
@@ -76,7 +73,9 @@
 
   context "admin editing an adjustment" do
     before(:each) do
-      within_row(2) { click_icon :edit }
+      within('table tr', text: 'Rebate') do
+        click_icon :edit
+      end
     end
 
     context "successfully" do
@@ -106,15 +105,19 @@
   end
 
   context "deleting an adjustment" do
-    it "should not be possible if adjustment is closed" do
-      within_row(1) do
-        expect(page).not_to have_css('.fa-trash')
+    context 'when the adjustment is finalized' do
+      let!(:adjustment) { super().tap(&:finalize!) }
+
+      it 'should not be possible' do
+        within('table tr', text: 'Rebate') do
+          expect(page).not_to have_css('.fa-trash')
+        end
       end
     end
 
     it "should update the total", js: true do
       accept_alert do
-        within_row(2) do
+        within('table tr', text: 'Rebate') do
           click_icon(:trash)
         end
       end
diff --git a/core/spec/models/spree/order/updating_spec.rb b/core/spec/models/spree/order/updating_spec.rb
index 9a13be434ae..0772d4c35d3 100644
--- a/core/spec/models/spree/order/updating_spec.rb
+++ b/core/spec/models/spree/order/updating_spec.rb
@@ -1,11 +1,9 @@
 require 'spec_helper'
 
 describe Spree::Order, type: :model do
-  let(:order) { stub_model(Spree::Order) }
+  let(:order) { create(:order) }
 
   context "#update!" do
-    let(:line_items) { [mock_model(Spree::LineItem, amount: 5)] }
-
     context "when there are update hooks" do
       before { Spree::Order.register_update_hook :foo }
       after { Spree::Order.update_hooks.clear }
diff --git a/core/spec/models/spree/order_cancellations_spec.rb b/core/spec/models/spree/order_cancellations_spec.rb
index 4222631e094..b46961b53f7 100644
--- a/core/spec/models/spree/order_cancellations_spec.rb
+++ b/core/spec/models/spree/order_cancellations_spec.rb
@@ -134,17 +134,19 @@
       let(:line_item) { order.line_items.to_a.first }
       let(:inventory_unit_1) { line_item.inventory_units[0] }
       let(:inventory_unit_2) { line_item.inventory_units[1] }
+      let(:promotion) { create(:promotion, :with_line_item_adjustment) }
+      let(:promotion_action) { promotion.actions[0] }
 
       before do
         order.contents.add(line_item.variant)
 
         # make the total $1.67 so it divides unevenly
         line_item.adjustments.create!(
-          source_type: 'Spree::TaxRate',
           order: order,
           amount: 0.01,
-          label: 'some fake tax',
-          finalized: true
+          label: 'some promo',
+          source: promotion_action,
+          finalized: true,
         )
         order.update!
       end
@@ -154,7 +156,7 @@
         order.cancellations.short_ship([inventory_unit_2])
         expect(line_item.adjustments.map(&:amount)).to match_array(
           [
-            0.01, # tax adjustment
+            0.01,  # promo adjustment
             -0.84, # short ship 1
             -0.83, # short ship 2
           ]
diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb
index f6dc1dff4dd..887d9e3def0 100644
--- a/core/spec/models/spree/order_updater_spec.rb
+++ b/core/spec/models/spree/order_updater_spec.rb
@@ -276,7 +276,6 @@ def create_adjustment(label, amount)
         let(:order) do
           create(
             :order_with_line_items,
-            line_items_count: 1,
             line_items_attributes: [{ price: 10, variant: variant }],
             ship_address: ship_address,
           )
diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb
index 62df39207ae..70788cf5832 100644
--- a/core/spec/models/spree/shipment_spec.rb
+++ b/core/spec/models/spree/shipment_spec.rb
@@ -118,10 +118,23 @@
   end
 
   context "#item_cost" do
+    let(:shipment) { order.shipments[0] }
+
+    let(:order) do
+      create(
+        :order_ready_to_ship,
+        line_items_attributes: [{ price: 10, variant: variant }],
+        ship_address: ship_address,
+      )
+    end
+
+    let!(:ship_address) { create(:address) }
+    let!(:tax_zone) { create(:global_zone) } # will include the above address
+    let!(:tax_rate) { create(:tax_rate, amount: 0.1, zone: tax_zone, tax_category: tax_category) }
+    let(:tax_category) { create(:tax_category) }
+    let(:variant) { create(:variant, tax_category: tax_category) }
+
     it 'should equal line items final amount with tax' do
-      shipment = create(:shipment, order: create(:order_with_totals))
-      create :tax_adjustment, adjustable: shipment.order.line_items.first, order: shipment.order
-      shipment.order.update!
       expect(shipment.item_cost).to eql(11.0)
     end
   end

From edd95afe66e9026fadecbecb0b5de34c258be763 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Wed, 28 Sep 2016 06:28:41 -0600
Subject: [PATCH 3/5] Add `Spree::Config.tax_adjuster` extension point

To allow easier customization of tax calculation in extensions or
applications.
---
 core/app/models/spree/app_configuration.rb   | 10 ++++++++++
 core/app/models/spree/order.rb               |  2 +-
 core/app/models/spree/order_updater.rb       |  2 +-
 core/spec/models/spree/order_updater_spec.rb | 17 +++++++++++++++++
 4 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/core/app/models/spree/app_configuration.rb b/core/app/models/spree/app_configuration.rb
index d2d8fbb895e..1e0fb07feb3 100644
--- a/core/app/models/spree/app_configuration.rb
+++ b/core/app/models/spree/app_configuration.rb
@@ -371,6 +371,16 @@ def add_payment_sources_to_wallet_class
       @add_payment_sources_to_wallet_class ||= Spree::Wallet::AddPaymentSourcesToWallet
     end
 
+    # Allows providing your own class for calculating taxes on an order.
+    #
+    # @!attribute [rw] tax_adjuster_class
+    # @return [Class] a class with the same public interfaces as
+    #   Spree::Tax::OrderAdjuster
+    attr_writer :tax_adjuster_class
+    def tax_adjuster_class
+      @tax_adjuster_class ||= Spree::Tax::OrderAdjuster
+    end
+
     def static_model_preferences
       @static_model_preferences ||= Spree::Preferences::StaticModelPreferences.new
     end
diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb
index 1c9755f62b8..52a4149ff2a 100644
--- a/core/app/models/spree/order.rb
+++ b/core/app/models/spree/order.rb
@@ -350,7 +350,7 @@ def line_item_options_match(line_item, options)
     # include taxes then price adjustments are created instead.
     # @deprecated This now happens during #update!
     def create_tax_charge!
-      Spree::Tax::OrderAdjuster.new(self).adjust!
+      Spree::Config.tax_adjuster_class.new(self).adjust!
     end
     deprecate create_tax_charge!: :update!, deprecator: Spree::Deprecation
 
diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb
index c65d594c2a5..f14178ca254 100644
--- a/core/app/models/spree/order_updater.rb
+++ b/core/app/models/spree/order_updater.rb
@@ -197,7 +197,7 @@ def update_order_promotions
     end
 
     def update_taxes
-      Spree::Tax::OrderAdjuster.new(order).adjust!
+      Spree::Config.tax_adjuster_class.new(order).adjust!
 
       [*line_items, *shipments].each do |item|
         tax_adjustments = item.adjustments.select(&:tax?)
diff --git a/core/spec/models/spree/order_updater_spec.rb b/core/spec/models/spree/order_updater_spec.rb
index 887d9e3def0..779b4fa3655 100644
--- a/core/spec/models/spree/order_updater_spec.rb
+++ b/core/spec/models/spree/order_updater_spec.rb
@@ -298,6 +298,23 @@ def create_adjustment(label, amount)
             }.from(1).to(2)
           end
         end
+
+        context 'with a custom tax_adjuster_class' do
+          let(:custom_adjuster_class) { double }
+          let(:custom_adjuster_instance) { double }
+
+          before do
+            order # generate this first so we can expect it
+            Spree::Config.tax_adjuster_class = custom_adjuster_class
+          end
+
+          it 'uses the configured class' do
+            expect(custom_adjuster_class).to receive(:new).with(order).at_least(:once).and_return(custom_adjuster_instance)
+            expect(custom_adjuster_instance).to receive(:adjust!).at_least(:once)
+
+            order.update!
+          end
+        end
       end
     end
 

From 4dbf194fcfae272dd436f41514c171a677f8caa5 Mon Sep 17 00:00:00 2001
From: Jordan Brough <git.j@brgh.net>
Date: Sat, 1 Oct 2016 07:27:32 -0600
Subject: [PATCH 4/5] Add Changelog entry for Spree::Config.tax_adjuster_class

---
 CHANGELOG.md | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ff9a4e8eea8..092f09fbd21 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,12 @@
 ## Solidus 2.1.0 (master, unreleased)
 
+*   Added Spree::Config.tax_adjuster_class
+
+    To allow easier customization of tax calculation in extensions or
+    applications.
+
+    https://github.com/solidusio/solidus/pull/1479
+
 *   Add `Spree::Promotion#remove_from` and `Spree::PromotionAction#remove_from`
 
     This will allow promotions to be removed from orders and allows promotion
@@ -39,7 +46,7 @@
 *   Removals
 
     * Removed deprecated method `Spree::TaxRate.adjust` (not to be confused with
-      Spree::TaxRate#adjust) in favor of `Spree::Tax::OrderAdjuster`.
+      Spree::TaxRate#adjust) in favor of `Spree::Config.tax_adjuster_class`.
 
       https://github.com/solidusio/solidus/pull/1462
 

From b3c81cc7954814f37f1fe0dfeeecc62eb1fba466 Mon Sep 17 00:00:00 2001
From: John Hawthorn <john@stembolt.com>
Date: Mon, 31 Oct 2016 14:47:24 -0700
Subject: [PATCH 5/5] Note create_tax_charge! deprecation in CHANGELOG

---
 CHANGELOG.md | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 092f09fbd21..5b0930f5214 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,20 @@
 ## Solidus 2.1.0 (master, unreleased)
 
+*   The OrderUpdater (as used by `order.update!`) now fully updates taxes.
+
+    Previously there were two different ways taxes were calculated: a "full"
+    and a "quick" calculation. The full calculation was performed with
+    `order.create_tax_charge!` and would determine which tax rates applied and
+    add taxes to items. The "quick" calculation was performed as part of an
+    order update, and would only update the tax amounts on existing line items
+    with taxes.
+
+    Now `order.update!` will perform the full calculation every time.
+    `order.create_tax_charge!` is now deprecated and has been made equivalent
+    to `order.update!`.
+
+    https://github.com/solidusio/solidus/pull/1479
+
 *   Added Spree::Config.tax_adjuster_class
 
     To allow easier customization of tax calculation in extensions or