From c729f64fcff7152a425fa34944a6bf46758b0075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Sun, 24 May 2020 11:52:10 +0200 Subject: [PATCH 1/6] Include adjustments without positive taxes --- app/models/spree/order_decorator.rb | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 82609dccbc2..32ad471fae1 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -313,12 +313,14 @@ def total_tax end def tax_adjustments - adjustments.with_tax + - line_items.includes(:adjustments).map { |li| li.adjustments.with_tax }.flatten + adjustments + price_adjustments end def tax_adjustment_totals tax_adjustments.each_with_object({}) do |adjustment, hash| + # No need of dealing with a missing Tax Rate if no tax setup + next if adjustment.included_tax.zero? + 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) @@ -328,21 +330,6 @@ def tax_adjustment_totals end end - def price_adjustments - adjustments = [] - - line_items.each { |line_item| adjustments.concat line_item.adjustments } - - adjustments - end - - def price_adjustment_totals - Hash[tax_adjustment_totals.map do |tax_rate, tax_amount| - [tax_rate.name, - Spree::Money.new(tax_amount, currency: currency)] - end] - end - def has_taxes_included !line_items.with_tax.empty? end From 9abe41f6cb61ba1a16d1f7d014e6e66646df367f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 9 Jun 2020 11:03:11 +0200 Subject: [PATCH 2/6] Use OrderTaxAdjustmentsFetcher service --- app/helpers/checkout_helper.rb | 2 +- app/models/spree/adjustment_decorator.rb | 2 +- app/models/spree/order_decorator.rb | 18 ----- app/services/order_tax_adjustments_fetcher.rb | 28 ++++++++ lib/open_food_network/sales_tax_report.rb | 5 +- spec/models/spree/order_spec.rb | 64 ----------------- .../order_tax_adjustments_fetcher_spec.rb | 69 +++++++++++++++++++ 7 files changed, 102 insertions(+), 86 deletions(-) create mode 100644 app/services/order_tax_adjustments_fetcher.rb create mode 100644 spec/services/order_tax_adjustments_fetcher_spec.rb diff --git a/app/helpers/checkout_helper.rb b/app/helpers/checkout_helper.rb index 4e9ff57a58f..28401f1d7aa 100644 --- a/app/helpers/checkout_helper.rb +++ b/app/helpers/checkout_helper.rb @@ -44,7 +44,7 @@ def display_checkout_tax_total(order) end def display_checkout_taxes_hash(order) - order.tax_adjustment_totals.each_with_object({}) do |(tax_rate, tax_amount), hash| + OrderTaxAdjustmentsFetcher.new(order).totals.each_with_object({}) do |(tax_rate, tax_amount), hash| hash[number_to_percentage(tax_rate.amount * 100, precision: 1)] = Spree::Money.new tax_amount, currency: order.currency end end diff --git a/app/models/spree/adjustment_decorator.rb b/app/models/spree/adjustment_decorator.rb index ece704bdd18..dbbaa6ae950 100644 --- a/app/models/spree/adjustment_decorator.rb +++ b/app/models/spree/adjustment_decorator.rb @@ -18,7 +18,7 @@ module Spree where(originator_type: 'Spree::TaxRate', adjustable_type: 'Spree::LineItem') } - scope :with_tax, -> { where('spree_adjustments.included_tax > 0') } + scope :with_tax, -> { where('spree_adjustments.included_tax <> 0') } scope :without_tax, -> { where('spree_adjustments.included_tax = 0') } scope :payment_fee, -> { where(AdjustmentScopes::PAYMENT_FEE_SCOPE) } scope :shipping, -> { where(AdjustmentScopes::SHIPPING_SCOPE) } diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index 32ad471fae1..cc0126399ef 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -312,24 +312,6 @@ def total_tax (adjustments + price_adjustments).sum(&:included_tax) end - def tax_adjustments - adjustments + price_adjustments - end - - def tax_adjustment_totals - tax_adjustments.each_with_object({}) do |adjustment, hash| - # No need of dealing with a missing Tax Rate if no tax setup - next if adjustment.included_tax.zero? - - 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] - end] - hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 } - end - end - def has_taxes_included !line_items.with_tax.empty? end diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb new file mode 100644 index 00000000000..3a3420a8c76 --- /dev/null +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -0,0 +1,28 @@ +# This class will be used to get Tax Adjustments related to an order, +# and proceed basic calcultation over them. + +class OrderTaxAdjustmentsFetcher + def initialize(order) + @order = order + end + + def totals + all.each_with_object({}) do |adjustment, hash| + 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] + end] + hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 } + end + end + + private + + attr_reader :order + + def all + order.adjustments.with_tax + + order.line_items.includes(:adjustments).map { |li| li.adjustments.with_tax }.flatten + end +end \ No newline at end of file diff --git a/lib/open_food_network/sales_tax_report.rb b/lib/open_food_network/sales_tax_report.rb index 1281dd28a4e..458b4523afe 100644 --- a/lib/open_food_network/sales_tax_report.rb +++ b/lib/open_food_network/sales_tax_report.rb @@ -49,8 +49,9 @@ def table when "tax_rates" orders.map do |order| [order.number, order.total - order.total_tax] + - relevant_rates.map { |rate| order.tax_adjustment_totals.fetch(rate, 0) } + - [order.total_tax, order.total] + relevant_rates.map { |rate| + OrderTaxAdjustmentsFetcher.new(order).totals.fetch(rate, 0) + } + [order.total_tax, order.total] end else orders.map do |order| diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 9e821cabd6d..2151634bdef 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -287,70 +287,6 @@ end end - describe "getting a hash of all taxes" do - let(:zone) { create(:zone_with_member) } - let(:coordinator) { create(:distributor_enterprise, charges_sales_tax: true) } - - let(:tax_rate10) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.1, zone: zone) } - let(:tax_rate15) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.15, zone: zone) } - let(:tax_rate20) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.2, zone: zone) } - let(:tax_rate25) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.25, zone: zone) } - let(:tax_category10) { create(:tax_category, tax_rates: [tax_rate10]) } - let(:tax_category15) { create(:tax_category, tax_rates: [tax_rate15]) } - let(:tax_category20) { create(:tax_category, tax_rates: [tax_rate20]) } - let(:tax_category25) { create(:tax_category, tax_rates: [tax_rate25]) } - - let(:variant) { create(:variant, product: create(:product, tax_category: tax_category10)) } - let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, tax_category: tax_category20, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 48.0)) } - let(:additional_adjustment) { create(:adjustment, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0)) } - - let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, coordinator_fees: [enterprise_fee], distributors: [coordinator], variants: [variant]) } - let(:line_item) { create(:line_item, variant: variant, price: 44.0) } - let(:order) do - create( - :order, - line_items: [line_item], - bill_address: create(:address), - order_cycle: order_cycle, - distributor: coordinator, - adjustments: [additional_adjustment] - ) - end - - before do - allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) - allow(Spree::Config).to receive(:shipping_tax_rate).and_return(tax_rate15.amount) - end - - let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 46.0)) } - let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } - - before do - order.create_tax_charge! - order.update_distribution_charge! - end - - it "returns a hash with all 3 taxes" do - expect(order.tax_adjustment_totals.size).to eq(4) - end - - it "contains tax on line_item" do - expect(order.tax_adjustment_totals[tax_rate10]).to eq(4.0) - end - - it "contains tax on shipping_fee" do - expect(order.tax_adjustment_totals[tax_rate15]).to eq(6.0) - end - - it "contains tax on enterprise_fee" do - expect(order.tax_adjustment_totals[tax_rate20]).to eq(8.0) - end - - it "contains tax on order adjustment" do - expect(order.tax_adjustment_totals[tax_rate25]).to eq(10.0) - end - end - describe "setting the distributor" do it "sets the distributor when no order cycle is set" do d = create(:distributor_enterprise) diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb new file mode 100644 index 00000000000..e56e40d5af3 --- /dev/null +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -0,0 +1,69 @@ +require "spec_helper" + +describe OrderTaxAdjustmentsFetcher do + describe "#totals" do + let(:zone) { create(:zone_with_member) } + let(:coordinator) { create(:distributor_enterprise, charges_sales_tax: true) } + + let(:tax_rate10) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.1, zone: zone) } + let(:tax_rate15) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.15, zone: zone) } + let(:tax_rate20) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.2, zone: zone) } + let(:tax_rate25) { create(:tax_rate, included_in_price: true, calculator: Spree::Calculator::DefaultTax.new, amount: 0.25, zone: zone) } + let(:tax_category10) { create(:tax_category, tax_rates: [tax_rate10]) } + let(:tax_category15) { create(:tax_category, tax_rates: [tax_rate15]) } + let(:tax_category20) { create(:tax_category, tax_rates: [tax_rate20]) } + let(:tax_category25) { create(:tax_category, tax_rates: [tax_rate25]) } + + let(:variant) { create(:variant, product: create(:product, tax_category: tax_category10)) } + let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, tax_category: tax_category20, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 48.0)) } + let(:additional_adjustment) { create(:adjustment, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0)) } + + let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator, coordinator_fees: [enterprise_fee], distributors: [coordinator], variants: [variant]) } + let(:line_item) { create(:line_item, variant: variant, price: 44.0) } + let(:order) do + create( + :order, + line_items: [line_item], + bill_address: create(:address), + order_cycle: order_cycle, + distributor: coordinator, + adjustments: [additional_adjustment] + ) + end + + before do + allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) + allow(Spree::Config).to receive(:shipping_tax_rate).and_return(tax_rate15.amount) + end + + let(:shipping_method) { create(:shipping_method, calculator: Spree::Calculator::FlatRate.new(preferred_amount: 46.0)) } + let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } + + before do + order.create_tax_charge! + order.update_distribution_charge! + end + + subject { OrderTaxAdjustmentsFetcher.new(order).totals } + + it "returns a hash with all 3 taxes" do + expect(subject.size).to eq(4) + end + + it "contains tax on line_item" do + expect(subject[tax_rate10]).to eq(4.0) + end + + it "contains tax on shipping_fee" do + expect(subject[tax_rate15]).to eq(6.0) + end + + it "contains tax on enterprise_fee" do + expect(subject[tax_rate20]).to eq(8.0) + end + + it "contains tax on order adjustment" do + expect(subject[tax_rate25]).to eq(10.0) + end + end +end From b4952dcbfbd643832b59729d85675a250ffa3448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 9 Jun 2020 16:09:29 +0200 Subject: [PATCH 3/6] Cosmetics --- app/helpers/checkout_helper.rb | 7 +++-- app/services/order_tax_adjustments_fetcher.rb | 27 ++++++++++++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/helpers/checkout_helper.rb b/app/helpers/checkout_helper.rb index 28401f1d7aa..c87abd0afb0 100644 --- a/app/helpers/checkout_helper.rb +++ b/app/helpers/checkout_helper.rb @@ -44,8 +44,11 @@ def display_checkout_tax_total(order) end def display_checkout_taxes_hash(order) - OrderTaxAdjustmentsFetcher.new(order).totals.each_with_object({}) do |(tax_rate, tax_amount), hash| - hash[number_to_percentage(tax_rate.amount * 100, precision: 1)] = Spree::Money.new tax_amount, currency: order.currency + totals = OrderTaxAdjustmentsFetcher.new(order).totals + + totals.each_with_object({}) do |(tax_rate, tax_amount), hash| + hash[number_to_percentage(tax_rate.amount * 100, precision: 1)] = + Spree::Money.new tax_amount, currency: order.currency end end diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 3a3420a8c76..8053d9e0ab5 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This class will be used to get Tax Adjustments related to an order, # and proceed basic calcultation over them. @@ -8,11 +10,7 @@ def initialize(order) def totals all.each_with_object({}) do |adjustment, hash| - 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] - end] + tax_rates_hash = tax_rates_hash(adjustment) hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 } end end @@ -23,6 +21,21 @@ def totals def all order.adjustments.with_tax + - order.line_items.includes(:adjustments).map { |li| li.adjustments.with_tax }.flatten + order.line_items.includes(:adjustments).map { |li| + li.adjustments.with_tax + }.flatten + end + + def tax_rates_hash(adjustment) + tax_rates = TaxRateFinder.tax_rates_of(adjustment) + + Hash[tax_rates.collect do |tax_rate| + tax_amount = if tax_rates.one? + adjustment.included_tax + else + tax_rate.compute_tax(adjustment.amount) + end + [tax_rate, tax_amount] + end] end -end \ No newline at end of file +end From 00c9dd7ece951260af878367d31d3bfc070cc8da Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 12 Jun 2020 13:01:24 +0200 Subject: [PATCH 4/6] Remove N+1 fetching order and li adjustments --- app/services/order_tax_adjustments_fetcher.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 8053d9e0ab5..debce057b88 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -20,10 +20,15 @@ def totals attr_reader :order def all - order.adjustments.with_tax + - order.line_items.includes(:adjustments).map { |li| - li.adjustments.with_tax - }.flatten + Spree::Adjustment + .with_tax + .where(adjustable_id: order.id, adjustable_type: 'Spree::Order') + .order('created_at ASC') + + + Spree::Adjustment + .with_tax + .where(adjustable_id: order.line_item_ids, adjustable_type: 'Spree::LineItem') + .order('created_at ASC') end def tax_rates_hash(adjustment) From ff4d7fbc45f9ef303cedff64fc0ca710e11a48e2 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Fri, 12 Jun 2020 13:23:12 +0200 Subject: [PATCH 5/6] Refactor with Arel to perform a single query --- app/services/order_tax_adjustments_fetcher.rb | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index debce057b88..085119172ce 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -22,13 +22,22 @@ def totals def all Spree::Adjustment .with_tax - .where(adjustable_id: order.id, adjustable_type: 'Spree::Order') - .order('created_at ASC') + + .where(order_adjustments.or(line_item_adjustments)) + .order('created_at ASC') + end + + def order_adjustments + table[:adjustable_id].eq(order.id) + .and(table[:adjustable_type].eq('Spree::Order')) + end + + def line_item_adjustments + table[:adjustable_id].eq(order.line_item_ids.join(',')) + .and(table[:adjustable_type].eq('Spree::LineItem')) + end - Spree::Adjustment - .with_tax - .where(adjustable_id: order.line_item_ids, adjustable_type: 'Spree::LineItem') - .order('created_at ASC') + def table + @table ||= Spree::Adjustment.arel_table end def tax_rates_hash(adjustment) From 6672cfdaf5a1a4c68341a747e60adbd100c9a5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Turbelin?= Date: Tue, 16 Jun 2020 16:13:56 +0200 Subject: [PATCH 6/6] Readd price_adustments after bad rebase --- app/models/spree/order_decorator.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/models/spree/order_decorator.rb b/app/models/spree/order_decorator.rb index cc0126399ef..a36d8e13122 100644 --- a/app/models/spree/order_decorator.rb +++ b/app/models/spree/order_decorator.rb @@ -312,6 +312,21 @@ def total_tax (adjustments + price_adjustments).sum(&:included_tax) end + def price_adjustments + adjustments = [] + + line_items.each { |line_item| adjustments.concat line_item.adjustments } + + adjustments + end + + def price_adjustment_totals + Hash[tax_adjustment_totals.map do |tax_rate, tax_amount| + [tax_rate.name, + Spree::Money.new(tax_amount, currency: currency)] + end] + end + def has_taxes_included !line_items.with_tax.empty? end