diff --git a/CHANGELOG.md b/CHANGELOG.md index 500f722378..d71e0e5b4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ ## Solidus 2.1.0 (master, unreleased) +* 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. + +* Mark `Spree::Tax::ItemAdjuster` as api-private + * Analytics trackers were removed from the admin panel; the extension `solidus_trackers` provides the same functionality diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index 8264040152..48e5bfaa90 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -31,8 +31,6 @@ class LineItem < Spree::Base validates :price, numericality: true validate :ensure_proper_currency - after_create :update_tax_charge - after_save :update_inventory before_destroy :update_inventory @@ -174,10 +172,6 @@ def destroy_inventory_units inventory_units.destroy_all end - def update_tax_charge - Spree::Tax::ItemAdjuster.new(self).adjust! - end - def ensure_proper_currency if currency != order.currency Spree::Deprecation.warn "The line items currency is different from it's order currency. " \ diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index 5c317dd935..35444630cc 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -35,20 +35,8 @@ def remove_line_item(line_item, options = {}) end def update_cart(params) - # We need old_tax_address / new_tax_address because we can't rely on methods - # offered by ActiveRecord::Dirty to determine if tax_address was updated - # because if we update the address, a new record will be created - # by the Address.factory instead of the old record being updated - - old_tax_address = order.tax_address - if order.update_attributes(params) - - new_tax_address = order.tax_address - - if should_recalculate_taxes?(old_tax_address, new_tax_address) - order.create_tax_charge! - end + order.create_tax_charge! unless order.completed? order.line_items = order.line_items.select { |li| li.quantity > 0 } @@ -84,41 +72,12 @@ def approve(user: nil, name: nil) private - def should_recalculate_taxes?(old_address, new_address) - # Related to Solidus issue #894 - # This is needed because if you update the shipping_address - # from the backend on an order that completed checkout, - # Taxes were not being recalculated if the Order tax zone - # was updated - # - # Possible cases: - # - # Case 1: - # - # If old_address is a TaxLocation it means that the order has not passed - # the address checkout state so taxes will be computed by the Order - # state machine, so we do not calculate taxes here. - # - # Case 2 : - # If new_address is a TaxLocation, but old_address is not, it means that - # an order has somehow lost his TaxAddress. Since it's not supposed to happen, - # we do not compute taxes. - # - # Case 3 - # Both old_address and new_address are Spree::Address so the order - # has completed the checkout or that a registered user has updated his - # default addresses. We need to recalculate the taxes. - - return if old_address.is_a?(Spree::Tax::TaxLocation) || new_address.is_a?(Spree::Tax::TaxLocation) - - old_address.try!(:taxation_attributes) != new_address.try!(:taxation_attributes) - end - def after_add_or_remove(line_item, options = {}) reload_totals 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/tax/item_adjuster.rb b/core/app/models/spree/tax/item_adjuster.rb index 067392ae96..1ddbf08dc7 100644 --- a/core/app/models/spree/tax/item_adjuster.rb +++ b/core/app/models/spree/tax/item_adjuster.rb @@ -1,3 +1,7 @@ +# @api private +# @note This is a helper class for Tax::OrderAdjuster. It is marked as api +# private because taxes should always be calculated on the entire order, so +# external code should call Tax::OrderAdjuster instead of Tax::ItemAdjuster. module Spree module Tax # Adjust a single taxable item (line item or shipment) diff --git a/core/lib/spree/core/unreturned_item_charger.rb b/core/lib/spree/core/unreturned_item_charger.rb index 5041518c90..c94c406fb8 100644 --- a/core/lib/spree/core/unreturned_item_charger.rb +++ b/core/lib/spree/core/unreturned_item_charger.rb @@ -24,6 +24,7 @@ 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 722b31be90..cae21cfed3 100644 --- a/core/lib/spree/testing_support/factories/order_factory.rb +++ b/core/lib/spree/testing_support/factories/order_factory.rb @@ -45,6 +45,7 @@ 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/models/spree/line_item_spec.rb b/core/spec/models/spree/line_item_spec.rb index 58008505ad..b5e56ea536 100644 --- a/core/spec/models/spree/line_item_spec.rb +++ b/core/spec/models/spree/line_item_spec.rb @@ -35,41 +35,6 @@ end end - context "#create" do - let(:variant) { create(:variant) } - - before do - create(:tax_rate, zone: order.tax_zone, tax_category: variant.tax_category) - end - - context "when order has a tax zone" do - before do - expect(order.tax_zone).to be_present - end - - it "creates a tax adjustment" do - order.contents.add(variant) - line_item = order.find_line_item_by_variant(variant) - expect(line_item.adjustments.tax.count).to eq(1) - end - end - - context "when order does not have a tax zone" do - before do - order.bill_address = nil - order.ship_address = nil - order.save - expect(order.reload.tax_zone).to be_nil - end - - it "does not create a tax adjustment" do - order.contents.add(variant) - line_item = order.find_line_item_by_variant(variant) - expect(line_item.adjustments.tax.count).to eq(0) - end - end - end - describe 'line item creation' do let(:variant) { create :variant } diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index 2dbf3d9474..4ecc9b23f6 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -2,12 +2,12 @@ describe Spree::OrderContents, type: :model do let!(:store) { create :store } - let(:order) { Spree::Order.create } + let(:order) { create(:order) } let(:variant) { create(:variant) } let!(:stock_location) { variant.stock_locations.first } let(:stock_location_2) { create(:stock_location) } - subject { described_class.new(order) } + subject(:order_contents) { described_class.new(order) } context "#add" do context 'given quantity is not explicitly provided' do @@ -96,6 +96,38 @@ include_context "discount changes order total" end end + + describe 'tax calculations' do + let!(:zone) { create(:global_zone) } + let!(:tax_rate) do + create(:tax_rate, zone: zone, tax_category: variant.tax_category) + end + + context 'when the order has a tax zone' do + before do + expect(order.tax_zone).to be_present + end + + it 'creates a tax adjustment' do + order_contents.add(variant) + line_item = order.find_line_item_by_variant(variant) + expect(line_item.adjustments.tax.count).to eq(1) + end + end + + context 'when the order does not have a tax zone' do + before do + order.update_attributes!(ship_address: nil, bill_address: nil) + expect(order.tax_zone).to be_nil + end + + it 'creates a tax adjustment' do + order_contents.add(variant) + line_item = order.find_line_item_by_variant(variant) + expect(line_item.adjustments.tax.count).to eq(0) + end + end + end end context "#remove" do @@ -224,52 +256,9 @@ }.to change { subject.order.total } end - context "given an order with existing addresses" do - let(:default_address) { create :address, state_code: "NY", zipcode: "17402" } - let(:order_with_address ) { create :order, ship_address: default_address, bill_address: default_address } - - subject { described_class.new(order_with_address) } - - context "when an address in a potentially different tax zone is supplied " do - let(:updated_address) { build :address, state_code: "AL", zipcode: "64092" } - - let(:params) do - { ship_address_attributes: updated_address.value_attributes, bill_address_attributes: updated_address.value_attributes } - end - - it "updates tax adjustments" do - expect(subject.order).to receive(:create_tax_charge!) - subject.update_cart params - end - end - - context "when an address in potentially the same tax zone is supplied" do - let(:updated_address) { build :address, state_code: "NY", zipcode: "17402", firstname: 'Robert' } - - let(:params) do - { ship_address_attributes: updated_address.value_attributes, bill_address_attributes: updated_address.value_attributes } - end - - it "does not updates tax adjustments" do - expect(subject.order).not_to receive(:create_tax_charge!) - subject.update_cart params - end - end - end - - context "given an order with no existing addresses" do - context "when an address is supplied" do - let(:updated_address) { build :address, state_code: "CA", zipcode: "14902" } - - let(:params) do - { ship_address_attributes: updated_address.attributes, bill_address_attributes: updated_address.value_attributes } - end - - it "does not updates tax adjustments" do - expect(subject.order).not_to receive(:create_tax_charge!) - subject.update_cart params - end - 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