From a341561446c0bc67ccfba599ded6bd51855b2c13 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 18 Dec 2018 17:30:36 +1100 Subject: [PATCH 1/6] Move tax rate finding into service and spec it The `#tax_rates` method is actually not present in Spree. We encapsulate the logic in this commit and enable better unit testing. The new tests cover a current bug: https://github.com/openfoodfoundation/openfoodnetwork/issues/3127 --- app/models/spree/adjustment_decorator.rb | 24 +------- app/services/tax_rate_finder.rb | 33 +++++++++++ spec/models/spree/adjustment_spec.rb | 15 ----- spec/services/tax_rate_finder_spec.rb | 73 ++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 38 deletions(-) create mode 100644 app/services/tax_rate_finder.rb create mode 100644 spec/services/tax_rate_finder_spec.rb diff --git a/app/models/spree/adjustment_decorator.rb b/app/models/spree/adjustment_decorator.rb index bca08752da..61e968d844 100644 --- a/app/models/spree/adjustment_decorator.rb +++ b/app/models/spree/adjustment_decorator.rb @@ -42,29 +42,7 @@ module Spree # @return [Array] def tax_rates - case originator - when Spree::TaxRate - [originator] - when EnterpriseFee - case source - when Spree::LineItem - tax_category = originator.inherits_tax_category? ? source.product.tax_category : originator.tax_category - return tax_category ? tax_category.tax_rates.match(source.order) : [] - when Spree::Order - return originator.tax_category ? originator.tax_category.tax_rates.match(source) : [] - end - else - find_closest_tax_rates_from_included_tax - end - end - - # shipping fees and adjustments created from the admin panel have - # taxes set at creation in the included_tax field without relation - # to the corresponding TaxRate, so we look for the closest one - def find_closest_tax_rates_from_included_tax - approximation = (included_tax / (amount - included_tax)) - return [] if approximation.infinite? or approximation.zero? - [Spree::TaxRate.order("ABS(amount - #{approximation})").first] + TaxRateFinder.new.tax_rates(originator, source, amount, included_tax) end def self.without_callbacks diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb new file mode 100644 index 0000000000..b0a5c65446 --- /dev/null +++ b/app/services/tax_rate_finder.rb @@ -0,0 +1,33 @@ +# Finds tax rates on which an adjustment is based on. +# For example a packaging fee may contain VAT. This service finds the VAT rate +# for the tax included in the packaging fee. +class TaxRateFinder + # @return [Array] + def tax_rates(originator, source, amount, included_tax) + case originator + when Spree::TaxRate + [originator] + when EnterpriseFee + case source + when Spree::LineItem + tax_category = originator.inherits_tax_category? ? source.product.tax_category : originator.tax_category + tax_category ? tax_category.tax_rates.match(source.order) : [] + when Spree::Order + originator.tax_category ? originator.tax_category.tax_rates.match(source) : [] + end + else + find_closest_tax_rates_from_included_tax(amount, included_tax) + end + end + + private + + # shipping fees and adjustments created from the admin panel have + # taxes set at creation in the included_tax field without relation + # to the corresponding TaxRate, so we look for the closest one + def find_closest_tax_rates_from_included_tax(amount, included_tax) + approximation = (included_tax / (amount - included_tax)) + return [] if approximation.infinite? || approximation.zero? + [Spree::TaxRate.order("ABS(amount - #{approximation})").first] + end +end diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index fa24de3122..f6466ec0d1 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -281,21 +281,6 @@ module Spree adjustment.included_tax.should == 10.00 end end - - describe "getting the corresponding tax rate" do - let!(:adjustment_with_tax) { create(:adjustment, amount: 50, included_tax: 10) } - let!(:adjustment_without_tax) { create(:adjustment, amount: 50, included_tax: 0) } - let!(:tax_rate) { create(:tax_rate, calculator: Spree::Calculator::DefaultTax.new, amount: 0.25) } - let!(:other_tax_rate) { create(:tax_rate, calculator: Spree::Calculator::DefaultTax.new, amount: 0.3) } - - it "returns [] if there is no included tax" do - adjustment_without_tax.find_closest_tax_rates_from_included_tax.should == [] - end - - it "returns the most accurate tax rate" do - adjustment_with_tax.find_closest_tax_rates_from_included_tax.should == [tax_rate] - end - end end context "extends LocalizedNumber" do diff --git a/spec/services/tax_rate_finder_spec.rb b/spec/services/tax_rate_finder_spec.rb new file mode 100644 index 0000000000..7debe8d154 --- /dev/null +++ b/spec/services/tax_rate_finder_spec.rb @@ -0,0 +1,73 @@ +require 'spec_helper' + +describe TaxRateFinder do + describe "getting the corresponding tax rate" do + let(:amount) { BigDecimal(100) } + let(:included_tax) { BigDecimal(20) } + let(:tax_rate) { create_rate(0.2) } + let(:close_tax_rate) { create_rate(0.25) } + let(:other_tax_rate) { create_rate(0.3) } + let(:tax_category) { create(:tax_category, tax_rates: [tax_rate]) } + # This zone is used by :order_with_taxes and needs to match it + let(:zone) { create(:zone, name: "GlobalZone") } + let(:shipment) { create(:shipment) } + let(:enterprise_fee) { create(:enterprise_fee, tax_category: tax_category) } + let(:order) { create(:order_with_taxes) } + + it "finds the tax rate of a shipping fee" do + rates = TaxRateFinder.new.tax_rates( + tax_rate, + shipment, + amount, + included_tax + ) + expect(rates).to eq [tax_rate] + end + + it "finds a close match" do + close_tax_rate + other_tax_rate + + rates = TaxRateFinder.new.tax_rates( + nil, + shipment, + amount, + included_tax + ) + + expect(rates).to eq [close_tax_rate] + end + + it "finds the tax rate of an enterprise fee" do + rates = TaxRateFinder.new.tax_rates( + enterprise_fee, + order, + amount, + included_tax + ) + expect(rates).to eq [tax_rate] + end + + # There is a bug that leaves orphan adjustments on an order after + # associated line items have been removed. + # https://github.com/openfoodfoundation/openfoodnetwork/issues/3127 + pending "deals with a missing line item" do + rates = TaxRateFinder.new.tax_rates( + enterprise_fee, + nil, + amount, + included_tax + ) + expect(rates).to eq [tax_rate] + end + + def create_rate(amount) + create( + :tax_rate, + amount: amount, + calculator: Spree::Calculator::DefaultTax.new, + zone: zone + ) + end + end +end From 3169a384ba94b1cf860021efbd7995698ed91d37 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 18 Dec 2018 17:45:12 +1100 Subject: [PATCH 2/6] Find approximate tax rate if no other is found There are several cases in which a tax rate is not associated with an adjustment. We find the closest one for reporting here. It is not a good solution, but a workaround introduced for reporting: https://github.com/openfoodfoundation/openfoodnetwork/pull/1496 --- app/services/tax_rate_finder.rb | 11 +++++++---- spec/services/tax_rate_finder_spec.rb | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index b0a5c65446..ede2e1c629 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -4,6 +4,13 @@ class TaxRateFinder # @return [Array] def tax_rates(originator, source, amount, included_tax) + find_associated_tax_rate(originator, source) || + find_closest_tax_rates_from_included_tax(amount, included_tax) + end + + private + + def find_associated_tax_rate(originator, source) case originator when Spree::TaxRate [originator] @@ -15,13 +22,9 @@ class TaxRateFinder when Spree::Order originator.tax_category ? originator.tax_category.tax_rates.match(source) : [] end - else - find_closest_tax_rates_from_included_tax(amount, included_tax) end end - private - # shipping fees and adjustments created from the admin panel have # taxes set at creation in the included_tax field without relation # to the corresponding TaxRate, so we look for the closest one diff --git a/spec/services/tax_rate_finder_spec.rb b/spec/services/tax_rate_finder_spec.rb index 7debe8d154..4b3790d4ae 100644 --- a/spec/services/tax_rate_finder_spec.rb +++ b/spec/services/tax_rate_finder_spec.rb @@ -51,7 +51,7 @@ describe TaxRateFinder do # There is a bug that leaves orphan adjustments on an order after # associated line items have been removed. # https://github.com/openfoodfoundation/openfoodnetwork/issues/3127 - pending "deals with a missing line item" do + it "deals with a missing line item" do rates = TaxRateFinder.new.tax_rates( enterprise_fee, nil, From fc1b18227574130ac0c82c626b630ddd1c9c39ed Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 19 Dec 2018 14:09:48 +1100 Subject: [PATCH 3/6] Simplify method complexity and comply with rubocop --- app/services/tax_rate_finder.rb | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index ede2e1c629..2ce475fc5a 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -15,13 +15,25 @@ class TaxRateFinder when Spree::TaxRate [originator] when EnterpriseFee - case source - when Spree::LineItem - tax_category = originator.inherits_tax_category? ? source.product.tax_category : originator.tax_category - tax_category ? tax_category.tax_rates.match(source.order) : [] - when Spree::Order - originator.tax_category ? originator.tax_category.tax_rates.match(source) : [] - end + enterprise_fee_tax_rates(originator, source) + end + end + + def enterprise_fee_tax_rates(enterprise_fee, source) + case source + when Spree::LineItem + tax_category = line_item_tax_category(enterprise_fee, source) + tax_category ? tax_category.tax_rates.match(source.order) : [] + when Spree::Order + enterprise_fee.tax_category ? enterprise_fee.tax_category.tax_rates.match(source) : [] + end + end + + def line_item_tax_category(enterprise_fee, line_item) + if enterprise_fee.inherits_tax_category? + line_item.product.tax_category + else + enterprise_fee.tax_category end end From a978e992bfa7dbd2f2c70f04b78a41c2ab96a158 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 19 Dec 2018 14:42:38 +1100 Subject: [PATCH 4/6] Remove `tax_rates` shortcut from adjustment Becoming less dependent on Spree, using our own namespace, keeping decorators small. --- app/helpers/checkout_helper.rb | 2 +- app/models/spree/adjustment_decorator.rb | 5 ----- app/models/spree/order_decorator.rb | 2 +- app/services/tax_rate_finder.rb | 10 ++++++++++ lib/open_food_network/enterprise_fee_applicator.rb | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/helpers/checkout_helper.rb b/app/helpers/checkout_helper.rb index bc885bd5dc..4d64735cfc 100644 --- a/app/helpers/checkout_helper.rb +++ b/app/helpers/checkout_helper.rb @@ -54,7 +54,7 @@ module CheckoutHelper end def display_adjustment_tax_rates(adjustment) - tax_rates = adjustment.tax_rates + tax_rates = TaxRateFinder.tax_rates_of(adjustment) tax_rates.map { |tr| number_to_percentage(tr.amount * 100, :precision => 1) }.join(", ") end diff --git a/app/models/spree/adjustment_decorator.rb b/app/models/spree/adjustment_decorator.rb index 61e968d844..e7ffcf41f0 100644 --- a/app/models/spree/adjustment_decorator.rb +++ b/app/models/spree/adjustment_decorator.rb @@ -40,11 +40,6 @@ module Spree included_tax > 0 end - # @return [Array] - def tax_rates - TaxRateFinder.new.tax_rates(originator, source, amount, included_tax) - end - def self.without_callbacks skip_callback :save, :after, :update_adjustable skip_callback :destroy, :after, :update_adjustable diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 5baf95fa1c..be6fcd8fa1 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -293,7 +293,7 @@ Spree::Order.class_eval do def tax_adjustment_totals tax_adjustments.each_with_object(Hash.new) do |adjustment, hash| - tax_rates = adjustment.tax_rates + tax_rates = TaxRateFinder.tax_rates_of(adjustment) tax_rates_hash = Hash[tax_rates.collect do |tax_rate| tax_amount = tax_rates.one? ? adjustment.included_tax : tax_rate.compute_tax(adjustment.amount) [tax_rate, tax_amount] diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index 2ce475fc5a..dc6bdd83a2 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -2,6 +2,16 @@ # For example a packaging fee may contain VAT. This service finds the VAT rate # for the tax included in the packaging fee. class TaxRateFinder + # @return [Array] + def self.tax_rates_of(adjustment) + new.tax_rates( + adjustment.originator, + adjustment.source, + adjustment.amount, + adjustment.included_tax + ) + end + # @return [Array] def tax_rates(originator, source, amount, included_tax) find_associated_tax_rate(originator, source) || diff --git a/lib/open_food_network/enterprise_fee_applicator.rb b/lib/open_food_network/enterprise_fee_applicator.rb index e213204047..cafe67ea7b 100644 --- a/lib/open_food_network/enterprise_fee_applicator.rb +++ b/lib/open_food_network/enterprise_fee_applicator.rb @@ -32,7 +32,7 @@ module OpenFoodNetwork end def adjustment_tax(adjustable, adjustment) - tax_rates = adjustment.tax_rates + tax_rates = TaxRateFinder.tax_rates_of(adjustment) tax_rates.select(&:included_in_price).sum do |rate| rate.compute_tax adjustment.amount From a8ccb9b818433dde551600c961530d5366222719 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 4 Jan 2019 17:24:45 +1100 Subject: [PATCH 5/6] Explain why we look for the closest tax rate of a fee --- app/services/tax_rate_finder.rb | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index dc6bdd83a2..3f7397af94 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -47,9 +47,32 @@ class TaxRateFinder end end - # shipping fees and adjustments created from the admin panel have - # taxes set at creation in the included_tax field without relation - # to the corresponding TaxRate, so we look for the closest one + # There are two cases in which a line item is not associated to a tax rate. + # + # 1. Shipping fees and adjustments created from the admin panel have taxes set + # at creation in the included_tax field without relation to the + # corresponding TaxRate. + # 2. Removing line items from an order doesn't always remove the associated + # enterprise fees. These orphaned fees don't have a line item any more to + # find the item's tax rate. + # + # In these cases we try to find the used tax rate based on the included tax. + # For example, if the included tax is 10% of the adjustment, we look for a tax + # rate of 10%. Due to rounding errors, the included tax may be 9.9% of the + # adjustment. That's why we call it an approximation of the tax rate and look + # for the closest and hopefully find the 10% tax rate. + # + # This attempt can fail. + # + # - If an admin created an adjustment with a miscalculated included tax then + # we don't know which tax rate the admin intended to use. + # - An admin may also enter included tax that doesn't correspond to any tax + # rate in the system. They may enter a fee of $1.2 with tax of $0.2, but + # that doesn't mean that there is a 20% tax rate in the database. + # - The used tax rate may also have been deleted. Maybe the tax law changed. + # + # In either of these cases, we will find a tax rate that doesn't correspond + # to the included tax. def find_closest_tax_rates_from_included_tax(amount, included_tax) approximation = (included_tax / (amount - included_tax)) return [] if approximation.infinite? || approximation.zero? From d2ba305ce6663c7b01815702f7b86104d6b36677 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 4 Jan 2019 18:28:29 +1100 Subject: [PATCH 6/6] Fix context data for tax rate finder spec The spec was accidentally finding an exact tax rate match. The new test data provides only close matches to choose from. The naming of variables has been simplified as well. --- spec/services/tax_rate_finder_spec.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/spec/services/tax_rate_finder_spec.rb b/spec/services/tax_rate_finder_spec.rb index 4b3790d4ae..3f5daadfcf 100644 --- a/spec/services/tax_rate_finder_spec.rb +++ b/spec/services/tax_rate_finder_spec.rb @@ -2,11 +2,9 @@ require 'spec_helper' describe TaxRateFinder do describe "getting the corresponding tax rate" do - let(:amount) { BigDecimal(100) } + let(:amount) { BigDecimal(120) } let(:included_tax) { BigDecimal(20) } let(:tax_rate) { create_rate(0.2) } - let(:close_tax_rate) { create_rate(0.25) } - let(:other_tax_rate) { create_rate(0.3) } let(:tax_category) { create(:tax_category, tax_rates: [tax_rate]) } # This zone is used by :order_with_taxes and needs to match it let(:zone) { create(:zone, name: "GlobalZone") } @@ -25,8 +23,11 @@ describe TaxRateFinder do end it "finds a close match" do - close_tax_rate - other_tax_rate + tax_rate.destroy + close_tax_rate = create_rate(tax_rate.amount + 0.05) + # other tax rates, not as close to the real one + create_rate(tax_rate.amount + 0.06) + create_rate(tax_rate.amount - 0.06) rates = TaxRateFinder.new.tax_rates( nil,