Skip to content

Commit

Permalink
Merge pull request #1543 from mamhoff/deprecate-order-tax-zone
Browse files Browse the repository at this point in the history
Deprecate order tax zone
  • Loading branch information
jhawthorn authored Nov 2, 2016
2 parents eb9abe7 + d5320e3 commit 9e31d79
Show file tree
Hide file tree
Showing 17 changed files with 106 additions and 101 deletions.
12 changes: 6 additions & 6 deletions core/app/models/spree/calculator/default_tax.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def compute_order(order)
line_items_total = matched_line_items.sum(&:discounted_amount)
if rate.included_in_price
order_tax_amount = round_to_two_places(line_items_total - ( line_items_total / (1 + rate.amount) ) )
refund_if_necessary(order_tax_amount, order.tax_zone)
refund_if_necessary(order_tax_amount, order.tax_address)
else
round_to_two_places(line_items_total * rate.amount)
end
Expand Down Expand Up @@ -48,20 +48,20 @@ def deduced_total_by_rate(item, rate)
unrounded_net_amount = item.discounted_amount / (1 + sum_of_included_tax_rates(item))
refund_if_necessary(
round_to_two_places(unrounded_net_amount * rate.amount),
item.order.tax_zone
item.order.tax_address
)
end

def refund_if_necessary(amount, order_tax_zone)
if default_zone_or_zone_match?(order_tax_zone)
def refund_if_necessary(amount, order_tax_address)
if default_zone_or_zone_match?(order_tax_address)
amount
else
amount * -1
end
end

def default_zone_or_zone_match?(order_tax_zone)
Zone.default_tax.try!(:contains?, order_tax_zone) || rate.zone.contains?(order_tax_zone)
def default_zone_or_zone_match?(order_tax_address)
Zone.default_tax.try!(:include?, order_tax_address) || rate.zone.include?(order_tax_address)
end
end
end
9 changes: 3 additions & 6 deletions core/app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,10 @@ def backordered?
# Returns the relevant zone (if any) to be used for taxation purposes.
# Uses default tax zone unless there is a specific match
def tax_zone
@tax_zone ||= Zone.match(tax_address) || Zone.default_tax
Zone.match(tax_address) || Zone.default_tax
end
deprecate tax_zone: "Please use Spree::Order#tax_address instead.",
deprecator: Spree::Deprecation

# Returns the address for taxation based on configuration
def tax_address
Expand Down Expand Up @@ -591,11 +593,6 @@ def can_approve?
!approved?
end

def reload(options = nil)
remove_instance_variable(:@tax_zone) if defined?(@tax_zone)
super
end

def quantity
line_items.sum(:quantity)
end
Expand Down
7 changes: 3 additions & 4 deletions core/app/models/spree/tax/item_adjuster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,20 @@ def initialize(item, options = {})
@item = item
@order = @item.order
# set instance variable so `TaxRate.match` is only called when necessary
@rates_for_order_zone = options[:rates_for_order_zone]
@rates_for_order = options[:rates_for_order]
@rates_for_default_zone = options[:rates_for_default_zone]
@order_tax_zone = options[:order_tax_zone]
end

# Deletes all existing tax adjustments and creates new adjustments for all
# (geographically and category-wise) applicable tax rates.
#
# @return [Array<Spree::Adjustment>] newly created adjustments
def adjust!
return unless order_tax_zone(order)
return unless order.tax_address.country_id

item.adjustments.destroy(item.adjustments.select(&:tax?))

rates_for_item(item).map { |rate| rate.adjust(order_tax_zone(order), item) }
rates_for_item(item).map { |rate| rate.adjust(nil, item) }
end
end
end
Expand Down
7 changes: 3 additions & 4 deletions core/app/models/spree/tax/order_adjuster.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def initialize(order)
# Creates tax adjustments for all taxable items (shipments and line items)
# in the given order.
def adjust!
return unless order_tax_zone(order)
return unless order.tax_address.country_id

(order.line_items + order.shipments).each do |item|
ItemAdjuster.new(item, order_wide_options).adjust!
Expand All @@ -25,9 +25,8 @@ def adjust!

def order_wide_options
{
rates_for_order_zone: rates_for_order_zone(order),
rates_for_default_zone: rates_for_default_zone,
order_tax_zone: order_tax_zone(order),
rates_for_order: rates_for_order(order),
rates_for_default_zone: rates_for_default_zone
}
end
end
Expand Down
12 changes: 4 additions & 8 deletions core/app/models/spree/tax/tax_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,22 @@ module TaxHelpers
#
# For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
def applicable_rates(order)
order_zone_tax_categories = rates_for_order_zone(order).map(&:tax_category)
order_zone_tax_categories = rates_for_order(order).map(&:tax_category)
default_rates_with_unmatched_tax_category = rates_for_default_zone.to_a.delete_if do |default_rate|
order_zone_tax_categories.include?(default_rate.tax_category)
end

(rates_for_order_zone(order) + default_rates_with_unmatched_tax_category).uniq
(rates_for_order(order) + default_rates_with_unmatched_tax_category).uniq
end

def rates_for_order_zone(order)
@rates_for_order_zone ||= Spree::TaxRate.for_zone(order_tax_zone(order))
def rates_for_order(order)
@rates_for_order ||= Spree::TaxRate.for_address(order.tax_address)
end

def rates_for_default_zone
@rates_for_default_zone ||= Spree::TaxRate.for_zone(Spree::Zone.default_tax)
end

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
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/tax_rate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class TaxRate < Spree::Base
scope :included_in_price, -> { where(included_in_price: true) }

# Creates necessary tax adjustments for the order.
def adjust(order_tax_zone, item)
def adjust(_order_tax_zone, item)
amount = compute_amount(item)
return if amount == 0

Expand Down
1 change: 0 additions & 1 deletion core/spec/lib/spree/core/unreturned_item_charger_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
context 'in tax zone' do
let!(:tax_zone) { create(:zone, countries: [ship_address.country]) }
let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_category: original_variant.tax_category) }
before { tax_zone.update_attributes!(default_tax: true) }

it "applies tax" do
exchange_order = exchange_shipment.order
Expand Down
6 changes: 4 additions & 2 deletions core/spec/lib/tasks/exchanges_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
let(:return_item_1) { build(:exchange_return_item, inventory_unit: order.inventory_units.first) }
let(:return_item_2) { build(:exchange_return_item, inventory_unit: order.inventory_units.last) }
let!(:rma) { create(:return_authorization, order: order, return_items: [return_item_1, return_item_2]) }
let!(:tax_rate) { create(:tax_rate, zone: order.tax_zone, tax_category: return_item_2.exchange_variant.tax_category) }
let(:zone) { create(:zone, countries: [order.tax_address.country])}
let!(:tax_rate) { create(:tax_rate, zone: zone, tax_category: return_item_2.exchange_variant.tax_category) }
before do
rma.save!
Spree::Shipment.last.ship!
Expand All @@ -44,7 +45,8 @@
let(:return_item_1) { build(:exchange_return_item, inventory_unit: order.inventory_units.first) }
let(:return_item_2) { build(:exchange_return_item, inventory_unit: order.inventory_units.last) }
let!(:rma) { create(:return_authorization, order: order, return_items: [return_item_1, return_item_2]) }
let!(:tax_rate) { create(:tax_rate, zone: order.tax_zone, tax_category: return_item_2.exchange_variant.tax_category) }
let(:zone) { create(:zone, countries: [order.tax_address.country])}
let!(:tax_rate) { create(:tax_rate, zone: zone, tax_category: return_item_2.exchange_variant.tax_category) }

before do
rma.save!
Expand Down
3 changes: 2 additions & 1 deletion core/spec/models/spree/order/checkout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ def assert_state_changed(order, from, to)
end

it "recalculates tax and updates totals" do
create(:tax_rate, tax_category: line_item.tax_category, amount: 0.05, zone: order.tax_zone)
zone = create(:zone, countries: [order.tax_address.country])
create(:tax_rate, tax_category: line_item.tax_category, amount: 0.05, zone: zone)
order.next!
expect(order).to have_attributes(
adjustment_total: 0.5,
Expand Down
29 changes: 22 additions & 7 deletions core/spec/models/spree/order/tax_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ module Spree

context "when no zones exist" do
it "should return nil" do
expect(order.tax_zone).to be_nil
Spree::Deprecation.silence do
expect(order.tax_zone).to be_nil
end
end
end

Expand All @@ -22,7 +24,9 @@ module Spree
it "should calculate using ship_address" do
expect(Spree::Zone).to receive(:match).at_least(:once).with(ship_address)
expect(Spree::Zone).not_to receive(:match).with(bill_address)
order.tax_zone
Spree::Deprecation.silence do
order.tax_zone
end
end
end

Expand All @@ -32,7 +36,9 @@ module Spree
it "should calculate using bill_address" do
expect(Spree::Zone).to receive(:match).at_least(:once).with(bill_address)
expect(Spree::Zone).not_to receive(:match).with(ship_address)
order.tax_zone
Spree::Deprecation.silence do
order.tax_zone
end
end
end

Expand All @@ -46,15 +52,19 @@ module Spree
before { allow(Spree::Zone).to receive_messages(match: zone) }

it "should return the matching zone" do
expect(order.tax_zone).to eq(zone)
Spree::Deprecation.silence do
expect(order.tax_zone).to eq(zone)
end
end
end

context "when there is no matching zone" do
before { allow(Spree::Zone).to receive_messages(match: nil) }

it "should return the default tax zone" do
expect(order.tax_zone).to eq(@default_zone)
Spree::Deprecation.silence do
expect(order.tax_zone).to eq(@default_zone)
end
end
end
end
Expand All @@ -66,18 +76,23 @@ module Spree
before { allow(Spree::Zone).to receive_messages(match: zone) }

it "should return the matching zone" do
expect(order.tax_zone).to eq(zone)
Spree::Deprecation.silence do
expect(order.tax_zone).to eq(zone)
end
end
end

context "when there is no matching zone" do
before { allow(Spree::Zone).to receive_messages(match: nil) }

it "should return nil" do
expect(order.tax_zone).to be_nil
Spree::Deprecation.silence do
expect(order.tax_zone).to be_nil
end
end
end
end

end
end
end
8 changes: 4 additions & 4 deletions core/spec/models/spree/order_contents_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@
create(:tax_rate, zone: zone, tax_category: variant.tax_category)
end

context 'when the order has a tax zone' do
context 'when the order has a taxable address' do
before do
expect(order.tax_zone).to be_present
expect(order.tax_address.country_id).to be_present
end

it 'creates a tax adjustment' do
Expand All @@ -115,10 +115,10 @@
end
end

context 'when the order does not have a tax zone' do
context 'when the order does not have a taxable address' do
before do
order.update_attributes!(ship_address: nil, bill_address: nil)
expect(order.tax_zone).to be_nil
expect(order.tax_address.country_id).to be_nil
end

it 'creates a tax adjustment' do
Expand Down
Loading

0 comments on commit 9e31d79

Please sign in to comment.