WIP MOSS refactoring

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
@@ -173,7 +174,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

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

core/app/models/spree/tax/item_adjuster.rb

@@ -0,0 +1,51 @@
+module Spree
+  module Tax
+    class ItemAdjuster
+      attr_reader :item, :order
+
+      include TaxHelpers
+
+      def initialize(item, options = {})
+        @item = item
+        @order = @item.order
+        # 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!
+        return unless order_tax_zone
+        # Using .destroy_all to make sure callbacks fire
+        item.adjustments.tax.destroy_all
+
+        if outside_default_vat_zone?
+          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 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
+        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.applicable_for?(item) }
+      end
+    end
+  end
+end

core/app/models/spree/tax/order_adjuster.rb

@@ -0,0 +1,33 @@
+module Spree
+  module Tax
+    class OrderAdjuster
+      attr_reader :order
+
+      include TaxHelpers
+
+      def initialize(order)
+        @order = order
+      end
+
+      def adjust!
+        return unless order_tax_zone
+
+        (order.line_items + order.shipments).each do |item|
+          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

core/app/models/spree/tax/tax_helpers.rb

@@ -0,0 +1,33 @@
+module Spree
+  module Tax
+    module TaxHelpers
+      private
+
+      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(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

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,11 +15,8 @@ class TaxRate < Spree::Base
 
     validates :amount, presence: true, numericality: true
     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,33 +56,11 @@ 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.
-    def self.match(order_tax_zone)
-      return [] unless order_tax_zone
-      all_rates = includes(zone: { zone_members: :zoneable }).load
+    scope :for_zone, ->(zone) { where(zone_id: Spree::Zone.with_shared_members(zone).pluck(:id)) }
+    scope :included_in_price, -> { where(included_in_price: true) }
 
-      rates_for_order_zone = all_rates.select { |rate| rate.zone.contains?(order_tax_zone) }
-      rates_for_default_zone = all_rates.select(&:default_vat?)
-
-      # 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.delete_if do |default_rate|
-        order_zone_tax_categories.include?(default_rate.tax_category)
-      end
-
-      (rates_for_order_zone + rates_for_default_zone).uniq
+    def applicable_for?(item)
+      tax_category == item.tax_category
     end
 
     # Pre-tax amounts must be stored so that we can calculate
@@ -115,65 +80,23 @@ 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)
+    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)
-    end
-
-    def default_vat?
-      included_in_price && zone.contains?(Spree::Zone.default_tax)
+      calculator.compute(item)
     end
 
     private

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)

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

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

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

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) }

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