diff --git a/CHANGELOG.md b/CHANGELOG.md index fe362850457..63c3f065e2c 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/core/app/models/spree/calculator/default_tax.rb b/core/app/models/spree/calculator/default_tax.rb index c70b1dd60c7..24d1e2442c9 100644 --- a/core/app/models/spree/calculator/default_tax.rb +++ b/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 diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index ca82391338f..f9d05afe36b 100644 --- a/core/app/models/spree/line_item.rb +++ b/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? diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 9b68c99adc1..4b49c4c70d3 100644 --- a/core/app/models/spree/shipment.rb +++ b/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 diff --git a/core/app/models/spree/shipping_rate.rb b/core/app/models/spree/shipping_rate.rb index fcdb3656345..2369c6565a9 100644 --- a/core/app/models/spree/shipping_rate.rb +++ b/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 diff --git a/core/app/models/spree/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb index 73956472ae4..77fb3711f3f 100644 --- a/core/app/models/spree/tax/item_adjuster.rb +++ b/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 diff --git a/core/app/models/spree/tax/tax_helpers.rb b/core/app/models/spree/tax/tax_helpers.rb index a6d4f3f022f..a4e28f56426 100644 --- a/core/app/models/spree/tax/tax_helpers.rb +++ b/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 diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb index 1dc5ceb8692..9282cee5d05 100644 --- a/core/app/models/spree/tax_rate.rb +++ b/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) diff --git a/core/db/migrate/20160301103333_remove_pre_tax_amount_on_line_item_and_shipment.rb b/core/db/migrate/20160301103333_remove_pre_tax_amount_on_line_item_and_shipment.rb new file mode 100644 index 00000000000..7312ae7c9f6 --- /dev/null +++ b/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 diff --git a/core/lib/spree/testing_support/factories/line_item_factory.rb b/core/lib/spree/testing_support/factories/line_item_factory.rb index f82da720e63..c044443e100 100644 --- a/core/lib/spree/testing_support/factories/line_item_factory.rb +++ b/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 diff --git a/core/spec/models/spree/calculator/default_tax_spec.rb b/core/spec/models/spree/calculator/default_tax_spec.rb index cf8be2442fe..5ae8695e491 100644 --- a/core/spec/models/spree/calculator/default_tax_spec.rb +++ b/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 diff --git a/core/spec/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index e85461c7304..858f49e6d43 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/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 diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index 60be679ca05..60a7aedb2c9 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/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 diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 9731c5a4887..7c6a8e2c454 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/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! } diff --git a/core/spec/models/spree/shipping_rate_spec.rb b/core/spec/models/spree/shipping_rate_spec.rb index dadc78bddf4..43fbbcdbcb3 100644 --- a/core/spec/models/spree/shipping_rate_spec.rb +++ b/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 } diff --git a/core/spec/models/spree/tax/item_adjuster_spec.rb b/core/spec/models/spree/tax/item_adjuster_spec.rb index bc613178e19..f8bdfe9b0f1 100644 --- a/core/spec/models/spree/tax/item_adjuster_spec.rb +++ b/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