Merge pull request #1479 from jordan-brough/tax-extension-point

Add Tax extension point and merge tax updating code

CHANGELOG.md

@@ -13,6 +13,28 @@
     Warning: this change also deletes the `currency` database field (String)
     from the line items table, since it will not be used anymore.
 
+*   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
+    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
@@ -31,7 +53,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
 
@@ -52,7 +74,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
 

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

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

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!
+      Spree::Config.tax_adjuster_class.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

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

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

core/app/models/spree/order_updater.rb

@@ -197,10 +197,10 @@ def update_order_promotions
     end
 
     def update_taxes
+      Spree::Config.tax_adjuster_class.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
 

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!

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

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

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 }

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
           ]

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: {

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,
           )
@@ -299,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
 

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