Merge pull request #941 from magiclabs/remove-pre-tax-amount-column

Calculate pre tax amounts on the fly

CHANGELOG.md

@@ -1,5 +1,29 @@
 ## Solidus 1.3.0 (unreleased)
 
+*   Removed `pre_tax_amount` column from line item and shipment tables
+
+    This column was previously used as a caching column in the process of
+    calculating VATs. Its value should have been (but wasn't) always the same as
+    `discounted_amount - included_tax_total`. It's been replaced with a method
+    that does just that. [#941](https://github.com/solidusio/solidus/pull/941)
+
+*   Renamed return item `pre_tax_amount` column to `amount`
+
+    The naming and functioning of this column was inconsistent with how
+    shipments and line items work: In those models, the base from which we
+    calculate everything is the `amount`. The ReturnItem now works just like
+    a line item.
+
+    Usability-wise, this change entails that for VAT countries, when creating
+    a refund for an order including VAT, you now have to enter the amount
+    you want to refund including VAT. This is what a backend user working
+    with prices including tax would expect.
+
+    For a non-VAT store, nothing changes except for the form field name, which
+    now says `Amount` instead of `Pre-tax-amount`. You might want to adjust the
+    i18n translation here, depending on your circumstances.
+    [#706](https://github.com/solidusio/solidus/pull/706)
+
 *   Removed Spree::BaseHelper#gem_available? and Spree::BaseHelper#current_spree_page?
 
     Both these methods were untested and not appropriate code to be in core. If you need these

core/app/models/spree/calculator/default_tax.rb

@@ -2,6 +2,8 @@
 
 module Spree
   class Calculator::DefaultTax < Calculator
+    include Spree::Tax::TaxHelpers
+
     def self.description
       Spree.t(:default_tax)
     end
@@ -14,7 +16,7 @@ def compute_order(order)
         line_item.tax_category == rate.tax_category
       end
 
-      line_items_total = matched_line_items.sum(&:total)
+      line_items_total = matched_line_items.sum(&:discounted_amount)
       if rate.included_in_price
         round_to_two_places(line_items_total - ( line_items_total / (1 + rate.amount) ) )
       else
@@ -25,7 +27,7 @@ def compute_order(order)
     # When it comes to computing shipments or line items: same same.
     def compute_shipment_or_line_item(item)
       if rate.included_in_price
-        deduced_total_by_rate(item.pre_tax_amount, rate)
+        deduced_total_by_rate(item, rate)
       else
         round_to_two_places(item.discounted_amount * rate.amount)
       end
@@ -36,8 +38,7 @@ def compute_shipment_or_line_item(item)
 
     def compute_shipping_rate(shipping_rate)
       if rate.included_in_price
-        pre_tax_amount = shipping_rate.cost / (1 + rate.amount)
-        deduced_total_by_rate(pre_tax_amount, rate)
+        deduced_total_by_rate(shipping_rate, rate)
       else
         with_tax_amount = shipping_rate.cost * rate.amount
         round_to_two_places(with_tax_amount)
@@ -54,8 +55,9 @@ def round_to_two_places(amount)
       BigDecimal.new(amount.to_s).round(2, BigDecimal::ROUND_HALF_UP)
     end
 
-    def deduced_total_by_rate(pre_tax_amount, rate)
-      round_to_two_places(pre_tax_amount * rate.amount)
+    def deduced_total_by_rate(item, rate)
+      unrounded_net_amount = item.discounted_amount / (1 + sum_of_included_tax_rates(item))
+      round_to_two_places(unrounded_net_amount * rate.amount)
     end
   end
 end

core/app/models/spree/line_item.rb

@@ -81,6 +81,10 @@ def money
     alias display_total money
     alias display_amount money
 
+    def pre_tax_amount
+      discounted_amount - included_tax_total
+    end
+
     # @return [Boolean] true when it is possible to supply the required
     #   quantity of stock of this line item's variant
     def sufficient_stock?

core/app/models/spree/shipment.rb

@@ -163,6 +163,10 @@ def line_items
       inventory_units.includes(:line_item).map(&:line_item).uniq
     end
 
+    def pre_tax_amount
+      discounted_amount - included_tax_total
+    end
+
     def ready_or_pending?
       ready? || pending?
     end

core/app/models/spree/shipping_rate.rb

@@ -9,6 +9,8 @@ class ShippingRate < Spree::Base
     delegate :code, to: :shipping_method, prefix: true
     alias_attribute :amount, :cost
 
+    alias_method :discounted_amount, :amount
+
     extend DisplayMoney
     money_methods :amount
 

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

@@ -29,15 +29,7 @@ def adjust!
         # 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(order), item) }
-      end
-
-      private
-
-      def rates_for_item
-        @rates_for_item ||= applicable_rates(order).select { |rate| rate.tax_category == item.tax_category }
+        rates_for_item(item).map { |rate| rate.adjust(order_tax_zone(order), item) }
       end
     end
   end

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

@@ -36,6 +36,14 @@ def rates_for_default_zone
       def order_tax_zone(order)
         @order_tax_zone ||= order.tax_zone
       end
+
+      def sum_of_included_tax_rates(item)
+        rates_for_item(item).map(&:amount).sum
+      end
+
+      def rates_for_item(item)
+        applicable_rates(item.order).select { |rate| rate.tax_category == item.tax_category }
+      end
     end
   end
 end

core/app/models/spree/tax_rate.rb

@@ -83,16 +83,6 @@ def self.adjust(order_tax_zone, items)
       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
-    def self.store_pre_tax_amount(item, rates)
-      sum_of_included_rates = rates.select(&:included_in_price).map(&:amount).sum
-      pre_tax_amount = item.discounted_amount / (1 + sum_of_included_rates)
-
-      item.update_column(:pre_tax_amount, pre_tax_amount)
-    end
-
     # Creates necessary tax adjustments for the order.
     def adjust(order_tax_zone, item)
       amount = compute_amount(item)

core/db/migrate/20160301103333_remove_pre_tax_amount_on_line_item_and_shipment.rb

@@ -0,0 +1,6 @@
+class RemovePreTaxAmountOnLineItemAndShipment < ActiveRecord::Migration
+  def change
+    remove_column :spree_line_items, :pre_tax_amount, :decimal, precision: 12, scale: 4, default: 0.0, null: false
+    remove_column :spree_shipments, :pre_tax_amount, :decimal, precision: 12, scale: 4, default: 0.0, null: false
+  end
+end

core/lib/spree/testing_support/factories/line_item_factory.rb

@@ -5,7 +5,6 @@
   factory :line_item, class: Spree::LineItem do
     quantity 1
     price { BigDecimal.new('10.00') }
-    pre_tax_amount { price }
     order
     transient do
       product nil

core/spec/models/spree/calculator/default_tax_spec.rb

@@ -1,13 +1,13 @@
 require 'spec_helper'
 
 describe Spree::Calculator::DefaultTax, type: :model do
-  let!(:country) { create(:country) }
-  let!(:zone) { create(:zone, name: "Country Zone", default_tax: true, zone_members: []) }
-  let!(:tax_category) { create(:tax_category, tax_rates: []) }
-  let!(:rate) { create(:tax_rate, tax_category: tax_category, amount: 0.05, included_in_price: included_in_price) }
+  let!(:address) { create(:address) }
+  let!(:zone) { create(:zone, name: "Country Zone", default_tax: true, countries: [address.country]) }
+  let!(:tax_category) { create(:tax_category) }
+  let!(:rate) { create(:tax_rate, tax_category: tax_category, amount: 0.05, included_in_price: included_in_price, zone: zone) }
   let(:included_in_price) { false }
   let!(:calculator) { Spree::Calculator::DefaultTax.new(calculable: rate ) }
-  let!(:order) { create(:order) }
+  let!(:order) { create(:order, ship_address: address) }
   let!(:line_item) { create(:line_item, price: 10, quantity: 3, tax_category: tax_category) }
   let!(:shipment) { create(:shipment, cost: 15) }
 
@@ -79,7 +79,6 @@
         context "when line item is discounted" do
           before do
             line_item.promo_total = -1
-            Spree::TaxRate.store_pre_tax_amount(line_item, [rate])
           end
 
           it "should be equal to the item's discounted total * rate" do
@@ -88,7 +87,6 @@
         end
 
         it "should be equal to the item's full price * rate" do
-          Spree::TaxRate.store_pre_tax_amount(line_item, [rate])
           expect(calculator.compute(line_item)).to eql 1.43
         end
       end

core/spec/models/spree/line_item_spec.rb

@@ -225,12 +225,4 @@ def copy_price
       expect(line_item.price).to eq 21.98
     end
   end
-
-  describe "precision of pre_tax_amount" do
-    let!(:line_item) { create :line_item, pre_tax_amount: 4.2051 }
-
-    it "keeps four digits of precision even when reloading" do
-      expect(line_item.reload.pre_tax_amount).to eq(4.2051)
-    end
-  end
 end

core/spec/models/spree/order_spec.rb

@@ -756,10 +756,10 @@ def merge!(other_order, user = nil)
   describe "#pre_tax_item_amount" do
     it "sums all of the line items' pre tax amounts" do
       subject.line_items = [
-        Spree::LineItem.new(price: 10, quantity: 2, pre_tax_amount: 5.0),
-        Spree::LineItem.new(price: 30, quantity: 1, pre_tax_amount: 14.0)
+        Spree::LineItem.new(price: 10, quantity: 2, included_tax_total: 15.0),
+        Spree::LineItem.new(price: 30, quantity: 1, included_tax_total: 16.0)
       ]
-
+      # (2*10)-15 + 30-16 = 5 + 14 = 19
       expect(subject.pre_tax_item_amount).to eq 19.0
     end
   end

core/spec/models/spree/shipment_spec.rb

@@ -24,14 +24,6 @@
   let(:variant) { mock_model(Spree::Variant) }
   let(:line_item) { mock_model(Spree::LineItem, variant: variant) }
 
-  describe "precision of pre_tax_amount" do
-    let!(:line_item) { create :line_item, pre_tax_amount: 4.2051 }
-
-    it "keeps four digits of precision even when reloading" do
-      expect(line_item.reload.pre_tax_amount).to eq(4.2051)
-    end
-  end
-
   # Regression test for https://github.com/spree/spree/issues/4063
   context "number generation" do
     before { allow(order).to receive :update! }

core/spec/models/spree/shipping_rate_spec.rb

@@ -3,8 +3,11 @@
 require 'spec_helper'
 
 describe Spree::ShippingRate, type: :model do
+  let(:address) { create(:address) }
+  let(:order) { create(:order, ship_address: address) }
+  let(:tax_category) { create(:tax_category) }
   let(:shipment) { create(:shipment) }
-  let(:shipping_method) { create(:shipping_method) }
+  let(:shipping_method) { create(:shipping_method, tax_category: tax_category) }
   let(:shipping_rate) {
     Spree::ShippingRate.new(shipment: shipment,
                                                 shipping_method: shipping_method,
@@ -15,13 +18,14 @@
     context "when tax included in price" do
       context "when the tax rate is from the default zone" do
         before { shipment.order.update_attributes!(ship_address_id: nil) }
-        let!(:zone) { create(:zone, default_tax: true) }
+        let!(:zone) { create(:zone, default_tax: true, countries: [address.country]) }
         let(:tax_rate) do
           create(:tax_rate,
             name: "VAT",
             amount: 0.1,
             included_in_price: true,
-            zone: zone)
+            zone: zone,
+            tax_category: tax_category)
         end
 
         before { shipping_rate.tax_rate = tax_rate }
@@ -41,15 +45,16 @@
         end
       end
 
-      context "when the tax rate is from a non-default zone" do
-        let!(:default_zone) { create(:zone, default_tax: true) }
-        let!(:non_default_zone) { create(:zone, default_tax: false) }
+      context "when shipping to a non-default zone" do
+        let!(:zone) { create(:zone, default_tax: true, countries: [address.country]) }
+        let!(:address) { create(:address, country_iso_code: "DE") }
         let(:tax_rate) do
           create(:tax_rate,
             name: "VAT",
             amount: 0.1,
             included_in_price: true,
-            zone: non_default_zone)
+            zone: zone,
+            tax_category: tax_category)
         end
         before { shipping_rate.tax_rate = tax_rate }
 

core/spec/models/spree/tax/item_adjuster_spec.rb

@@ -42,8 +42,6 @@
       let(:tax_zone) { build_stubbed(:zone, :with_country) }
 
       before do
-        expect(item).to receive(:update_column)
-
         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