From 7fdf1a110d5eaa2eb0a1a4a26e4d59e9963ebc47 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Tue, 27 Feb 2024 10:45:57 +0100 Subject: [PATCH 1/3] Fix incorrect results for multiple tax rates in report - total_excl_tax would sum amounts and taxes without knowledge if taxes rates were really applied or not - spec shows use case described in issue 12066 --- .../sales_tax/sales_tax_totals_by_producer.rb | 10 ++- .../sales_tax_totals_by_producer_spec.rb | 63 ++++++++++++++++++- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb b/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb index 0271b38d59..864ba54956 100644 --- a/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb +++ b/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb @@ -125,8 +125,14 @@ module Reporting end def total_excl_tax(query_result_row) - line_items(query_result_row).sum(&:amount) - - line_items(query_result_row).sum(&:included_tax) + line_items(query_result_row)&.map do |line_item| + if line_item.adjustments.eligible.tax + .where(originator_id: tax_rate_id(query_result_row)).empty? + 0 + else + (line_item.amount - line_item.included_tax) + end + end&.sum end def tax(query_result_row) diff --git a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb index 964537a037..c602bb8981 100644 --- a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb +++ b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb @@ -3,7 +3,7 @@ require 'system_helper' describe "Sales Tax Totals By Producer" do - # Scenarion 1: added tax + # Scenario 1: added tax # 1 producer # 1 distributor # 1 product that costs 100$ @@ -30,6 +30,7 @@ describe "Sales Tax Totals By Producer" do let!(:state_tax_rate){ create(:tax_rate, zone: state_zone, tax_category:) } let!(:country_tax_rate){ create(:tax_rate, zone: country_zone, tax_category:) } let!(:ship_address){ create(:ship_address) } + let(:another_state){ create(:state, name: 'Another state', country: ship_address.country) } let!(:variant){ create(:variant) } let!(:product){ variant.product } @@ -119,6 +120,66 @@ describe "Sales Tax Totals By Producer" do end end + context 'Order not to be shipped in a state affected by state tax rate' do + # Therefore, do not apply both tax rates here, only country one + before do + ship_address.update!({ state_id: another_state.id }) + order.line_items.create({ variant:, quantity: 1, price: 100 }) + order.update!({ + order_cycle_id: order_cycle.id, + ship_address_id: ship_address.id + }) + + while !order.completed? + break unless order.next! + end + end + + it 'generates the report' do + login_as admin + visit admin_reports_path + click_on 'Sales Tax Totals By Producer' + + run_report + expect(page.find("table.report__table thead tr").text).to have_content(table_header) + + expect(page.find("table.report__table tbody").text).to have_content([ + "Distributor", + "Yes", + "Supplier", + "Yes", + "oc1", + "tax_category", + "State", + "1.5 %", + "0", + "0", + "0" + ].join(" ")) + + expect(page.find("table.report__table tbody").text).to have_content([ + "Distributor", + "Yes", + "Supplier", + "Yes", + "oc1", + "tax_category", + "Country", + "2.5 %", + "100.0", + "2.5", + "102.5" + ].join(" ")) + + expect(page.find("table.report__table tbody").text).to have_content([ + "TOTAL", + "100.0", + "2.5", + "102.5" + ].join(" ")) + end + end + context 'included tax' do before do state_tax_rate.update!({ included_in_price: true }) From 62739c04584d7bb2bdb5bd0eb0aed3847fc6fdec Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 28 Feb 2024 10:00:31 +0100 Subject: [PATCH 2/3] Requested changes in spec - use of new service instead of manually advancing an order --- .../sales_tax_totals_by_producer_spec.rb | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb index c602bb8981..f36ccdde08 100644 --- a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb +++ b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb @@ -70,9 +70,7 @@ describe "Sales Tax Totals By Producer" do ship_address_id: ship_address.id }) - while !order.completed? - break unless order.next! - end + OrderWorkflow.new(order).complete! end it "generates the report" do @@ -130,9 +128,7 @@ describe "Sales Tax Totals By Producer" do ship_address_id: ship_address.id }) - while !order.completed? - break unless order.next! - end + OrderWorkflow.new(order).complete! end it 'generates the report' do @@ -191,9 +187,7 @@ describe "Sales Tax Totals By Producer" do ship_address_id: ship_address.id }) - while !order.completed? - break unless order.next! - end + OrderWorkflow.new(order).complete! end it "generates the report" do login_as admin @@ -369,9 +363,7 @@ describe "Sales Tax Totals By Producer" do ship_address_id: customer1.bill_address_id, customer_id: customer1.id }) - while !order.completed? - break unless order.next! - end + OrderWorkflow.new(order).complete! order2.line_items.create({ variant:, quantity: 1, price: 200 }) order2.update!({ @@ -379,9 +371,7 @@ describe "Sales Tax Totals By Producer" do ship_address_id: customer2.bill_address_id, customer_id: customer2.id }) - while !order2.completed? - break unless order2.next! - end + OrderWorkflow.new(order2).complete! login_as admin visit admin_reports_path click_on 'Sales Tax Totals By Producer' From 7d99197ddefa4736c1293d0fb19512d034a1df32 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Thu, 29 Feb 2024 15:01:50 +0100 Subject: [PATCH 3/3] Requested changes: original fix moved up in code - instead of selecting out unapplied tax rates in the total tax method, did it in the query_result instead. Reverted the total_excl_tax method to its initial state. - spec and logic of testing affected also. --- .../sales_tax/sales_tax_totals_by_producer.rb | 14 ++++---------- .../sales_tax_totals_by_producer_spec.rb | 18 ++++-------------- 2 files changed, 8 insertions(+), 24 deletions(-) diff --git a/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb b/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb index 864ba54956..a2c00ea082 100644 --- a/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb +++ b/lib/reporting/reports/sales_tax/sales_tax_totals_by_producer.rb @@ -24,9 +24,9 @@ module Reporting # [tax_rate, supplier_id, distributor_id and order_cycle_id] report_line_items.list .flat_map do |line_item| - line_item.tax_rates.map do |tax_rate| + line_item.adjustments.eligible.tax.map do |tax_rate| { - tax_rate_id: tax_rate.id, + tax_rate_id: tax_rate.originator_id, line_item: } end @@ -125,14 +125,8 @@ module Reporting end def total_excl_tax(query_result_row) - line_items(query_result_row)&.map do |line_item| - if line_item.adjustments.eligible.tax - .where(originator_id: tax_rate_id(query_result_row)).empty? - 0 - else - (line_item.amount - line_item.included_tax) - end - end&.sum + line_items(query_result_row).sum(&:amount) - + line_items(query_result_row).sum(&:included_tax) end def tax(query_result_row) diff --git a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb index f36ccdde08..b1d319b887 100644 --- a/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb +++ b/spec/system/admin/reports/sales_tax/sales_tax_totals_by_producer_spec.rb @@ -139,20 +139,6 @@ describe "Sales Tax Totals By Producer" do run_report expect(page.find("table.report__table thead tr").text).to have_content(table_header) - expect(page.find("table.report__table tbody").text).to have_content([ - "Distributor", - "Yes", - "Supplier", - "Yes", - "oc1", - "tax_category", - "State", - "1.5 %", - "0", - "0", - "0" - ].join(" ")) - expect(page.find("table.report__table tbody").text).to have_content([ "Distributor", "Yes", @@ -173,6 +159,10 @@ describe "Sales Tax Totals By Producer" do "2.5", "102.5" ].join(" ")) + + # Even though 2 tax rates exist (but only one has been applied), we should get only 2 lines: + # one line item + total + expect(page.all("table.report__table tbody tr").count).to eq(2) end end