From 50d0952dd505658838404f9bf354af6acc2f6277 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 21:46:18 +0000 Subject: [PATCH 01/46] Bring in TaxRate#potentially_applicable method and add eager-loading --- app/models/spree/tax_rate.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index f3f28495e6..745672815c 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -30,8 +30,8 @@ module Spree return [] if order.distributor && !order.distributor.charges_sales_tax return [] unless order.tax_zone - all.select do |rate| - rate.zone == order.tax_zone || rate.zone.contains?(order.tax_zone) || rate.zone.default_tax + all.includes(zone: { zone_members: :zoneable }).load.select do |rate| + rate.potentially_applicable?(order.tax_zone) end end @@ -56,6 +56,15 @@ module Spree rate || 0 end + def potentially_applicable?(order_tax_zone) + # If the rate's zone matches the order's tax zone, then it's applicable. + zone == order_tax_zone || + # If the rate's zone *contains* the order's tax zone, then it's applicable. + zone.contains?(order_tax_zone) || + # The rate's zone is the default zone, then it's always applicable. + (included_in_price? && zone.default_tax) + end + # Creates necessary tax adjustments for the order. def adjust(order) label = create_label From d69f714032a124d594350493765c387a4cb67adf Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 22:11:42 +0000 Subject: [PATCH 02/46] Bring in changes to TaxRate#adjust --- app/models/spree/tax_rate.rb | 79 ++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 27 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index 745672815c..dc9c75e010 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -16,8 +16,10 @@ module Spree class TaxRate < ApplicationRecord acts_as_paranoid include Spree::Core::CalculatedAdjustments + belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates + has_many :adjustments, as: :originator validates :amount, presence: true, numericality: true validates :tax_category, presence: true @@ -35,11 +37,27 @@ module Spree end end - def self.adjust(order) - order.all_adjustments.tax.destroy_all + def self.adjust(order, items) + applicable_rates = match(order) + applicable_tax_categories = applicable_rates.map(&:tax_category) - match(order).each do |rate| - rate.adjust(order) + relevant_items, non_relevant_items = items.partition do |item| + applicable_tax_categories.include?(item.tax_category) + end + + relevant_items.each do |item| + item.adjustments.tax.delete_all + relevant_rates = applicable_rates.select { |rate| rate.tax_category == item.tax_category } + relevant_rates.each do |rate| + rate.adjust(order, item) + end + end + + non_relevant_items.each do |item| + if item.adjustments.tax.present? + item.adjustments.tax.delete_all + Spree::ItemAdjustments.new(item).update + end end end @@ -65,31 +83,38 @@ module Spree (included_in_price? && zone.default_tax) end - # Creates necessary tax adjustments for the order. - def adjust(order) - label = create_label - if included_in_price - if default_zone_or_zone_match? order - order.line_items.each { |line_item| create_adjustment(label, line_item, false, "open") } - order.shipments.each { |shipment| create_adjustment(label, shipment, false, "open") } - else - amount = -1 * calculator.compute(order) - label = Spree.t(:refund) + label + # Creates necessary tax adjustments for the item. + def adjust(order, item) + amount = compute_amount(item) + return if amount.zero? - order.adjustments.create( - amount: amount, - originator: self, - order: order, - state: "closed", - label: label - ) - end - else - create_adjustment(label, order, false, "open") + included = included_in_price && default_zone_or_zone_match?(order) + + if amount.negative? + label = "#{Spree.t(:refund)} #{create_label}" end - order.adjustments.reload - order.line_items.reload + self.adjustments.create!( + adjustable: item, + amount: amount, + order: order, + label: label || create_label, + included: included + ) + end + + # This method is used by Adjustment#update to recalculate the cost. + def compute_amount(item) + if included_in_price + if default_zone_or_zone_match?(item.order) + calculator.compute(item) + else + # In this case, it's a refund. + calculator.compute(item) * - 1 + end + else + calculator.compute(item) + end end def default_zone_or_zone_match?(order) @@ -119,7 +144,7 @@ module Spree def create_label label = "" - label << (name.presence || tax_category.name) + " " + label << "#{(name.presence || tax_category.name)} " label << (show_rate_in_label? ? "#{amount * 100}%" : "") label << " (#{I18n.t('models.tax_rate.included_in_price')})" if included_in_price? label From f92c082df8d5631eda2b773a679be7f9ea021c64 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 22:18:31 +0000 Subject: [PATCH 03/46] Refactor tax adjustment create_label method --- app/models/spree/tax_rate.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index dc9c75e010..bd00dbd9cc 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -90,15 +90,11 @@ module Spree included = included_in_price && default_zone_or_zone_match?(order) - if amount.negative? - label = "#{Spree.t(:refund)} #{create_label}" - end - self.adjustments.create!( adjustable: item, amount: amount, order: order, - label: label || create_label, + label: create_label(amount), included: included ) end @@ -142,8 +138,9 @@ module Spree private - def create_label + def create_label(amount) label = "" + label << "#{Spree.t(:refund)} " if amount.negative? label << "#{(name.presence || tax_category.name)} " label << (show_rate_in_label? ? "#{amount * 100}%" : "") label << " (#{I18n.t('models.tax_rate.included_in_price')})" if included_in_price? From 1c28b9783f946b11773ddb49c5141ec5ea6d8437 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 14 Apr 2021 14:18:29 +0100 Subject: [PATCH 04/46] Bring in Spree::TaxRate test coverage --- spec/models/spree/tax_rate_spec.rb | 380 ++++++++++++++++++++++++++++- 1 file changed, 379 insertions(+), 1 deletion(-) diff --git a/spec/models/spree/tax_rate_spec.rb b/spec/models/spree/tax_rate_spec.rb index fa7c5c9f9a..4db5494ab1 100644 --- a/spec/models/spree/tax_rate_spec.rb +++ b/spec/models/spree/tax_rate_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' module Spree describe TaxRate do - describe "selecting tax rates to apply to an order" do + describe "#match" do let!(:zone) { create(:zone_with_member) } let!(:order) { create(:order, distributor: hub, bill_address: create(:address)) } let!(:tax_rate) { create(:tax_rate, included_in_price: true, calculator: ::Calculator::FlatRate.new(preferred_amount: 0.1), zone: zone) } @@ -70,5 +70,383 @@ module Spree expect(tax_rate.calculator.calculable.included_in_price).to be false end end + + context "original Spree::TaxRate specs" do + context "match" do + let(:order) { create(:order) } + let(:country) { create(:country) } + let(:tax_category) { create(:tax_category) } + let(:calculator) { ::Calculator::FlatRate.new } + + it "should return an empty array when tax_zone is nil" do + allow(order).to receive(:tax_zone) { nil } + expect(Spree::TaxRate.match(order)).to eq [] + end + + context "when no rate zones match the tax zone" do + before do + Spree::TaxRate.create(amount: 1, zone: create(:zone)) + end + + context "when there is no default tax zone" do + before do + @zone = create(:zone, name: "Country Zone", default_tax: false, zone_members: []) + @zone.zone_members.create(zoneable: country) + end + + it "should return an empty array" do + order.stub tax_zone: @zone + expect(Spree::TaxRate.match(order)).to eq [] + end + + it "should return the rate that matches the rate zone" do + rate = Spree::TaxRate.create( + amount: 1, + zone: @zone, + tax_category: tax_category, + calculator: calculator + ) + + order.stub tax_zone: @zone + expect(Spree::TaxRate.match(order)).to eq [rate] + end + + it "should return all rates that match the rate zone" do + rate1 = Spree::TaxRate.create( + amount: 1, + zone: @zone, + tax_category: tax_category, + calculator: calculator + ) + + rate2 = Spree::TaxRate.create( + amount: 2, + zone: @zone, + tax_category: tax_category, + calculator: ::Calculator::FlatRate.new + ) + + order.stub tax_zone: @zone + expect(Spree::TaxRate.match(order)).to eq [rate1, rate2] + end + + context "when the tax_zone is contained within a rate zone" do + before do + sub_zone = create(:zone, name: "State Zone", zone_members: []) + sub_zone.zone_members.create(zoneable: create(:state, country: country)) + order.stub tax_zone: sub_zone + @rate = Spree::TaxRate.create( + amount: 1, + zone: @zone, + tax_category: tax_category, + calculator: calculator + ) + end + + it "should return the rate zone" do + expect(Spree::TaxRate.match(order)).to eq [@rate] + end + end + end + + context "when there is a default tax zone" do + before do + @zone = create(:zone, name: "Country Zone", default_tax: true, zone_members: []) + @zone.zone_members.create(zoneable: country) + end + + let(:included_in_price) { false } + let!(:rate) do + Spree::TaxRate.create(amount: 1, + zone: @zone, + tax_category: tax_category, + calculator: calculator, + included_in_price: included_in_price) + end + + subject { Spree::TaxRate.match(order) } + + context "when the order has the same tax zone" do + before do + order.stub tax_zone: @zone + order.stub billing_address: tax_address + end + + let(:tax_address) { build_stubbed(:address) } + + context "when the tax is not a VAT" do + it { is_expected.to eq [rate] } + end + + context "when the tax is a VAT" do + let(:included_in_price) { true } + it { is_expected.to eq [rate] } + end + end + + context "when the order has a different tax zone" do + let(:other_zone) { create(:zone, name: "Other Zone") } + + before do + allow(order).to receive(:tax_zone) { other_zone } + allow(order).to receive(:billing_address) { tax_address } + end + + context "when the order has a tax_address" do + let(:tax_address) { build_stubbed(:address) } + + context "when the tax is a VAT" do + let(:included_in_price) { true } + # The rate should match in this instance because: + # 1) It's the default rate (and as such, a negative adjustment should apply) + it { is_expected.to eq [rate] } + end + + context "when the tax is not VAT" do + it "returns no tax rate" do + expect(subject).to be_empty + end + end + end + + context "when the order does not have a tax_address" do + let(:tax_address) { nil } + + context "when the tax is a VAT" do + let(:included_in_price) { true } + # The rate should match in this instance because: + # 1) The order has no tax address by this stage + # 2) With no tax address, it has no tax zone + # 3) Therefore, we assume the default tax zone + # 4) This default zone has a default tax rate. + it { is_expected.to eq [rate] } + end + + context "when the tax is not a VAT" do + it { is_expected.to be_empty } + end + end + end + end + end + end + + context "adjust" do + let(:order) { create(:order) } + let(:tax_category_1) { build_stubbed(:tax_category) } + let(:tax_category_2) { build_stubbed(:tax_category) } + let(:rate_1) { build_stubbed(:tax_rate, tax_category: tax_category_1) } + let(:rate_2) { build_stubbed(:tax_rate, tax_category: tax_category_2) } + let(:line_items) { [build_stubbed(:line_item)] } + + context "with line items" do + let(:line_item) { build_stubbed(:line_item, tax_category: tax_category_1) } + let(:line_items) { [line_item] } + + before do + allow(Spree::TaxRate).to receive(:match) { [rate_1, rate_2] } + end + + it "should apply adjustments for two tax rates to the order" do + expect(rate_1).to receive(:adjust) + expect(rate_2).to_not receive(:adjust) + Spree::TaxRate.adjust(order, line_items) + end + end + + context "with shipments" do + let(:shipment) { build_stubbed(:shipment, order: order) } + let(:shipments) { [shipment] } + + before do + allow(shipment).to receive(:tax_category) { tax_category_1 } + allow(Spree::TaxRate).to receive(:match) { [rate_1, rate_2] } + end + + it "should apply adjustments for two tax rates to the order" do + expect(rate_1).to receive(:adjust) + expect(rate_2).to_not receive(:adjust) + Spree::TaxRate.adjust(order, shipments) + end + end + end + + context "default" do + let(:tax_category) { create(:tax_category) } + let(:country) { create(:country) } + let(:calculator) { ::Calculator::FlatRate.new } + + context "when there is no default tax_category" do + before { tax_category.is_default = false } + + it "should return 0" do + expect(Spree::TaxRate.default).to eq 0 + end + end + + context "when there is a default tax_category" do + before { tax_category.update_column :is_default, true } + + context "when the default category has tax rates in the default tax zone" do + before(:each) do + Spree::Config[:default_country_id] = country.id + @zone = create(:zone, name: "Country Zone", default_tax: true) + @zone.zone_members.create(zoneable: country) + rate = Spree::TaxRate.create( + amount: 1, + zone: @zone, + tax_category: tax_category, + calculator: calculator + ) + end + + it "should return the correct tax_rate" do + expect(Spree::TaxRate.default.to_f).to eq 1.0 + end + end + + context "when the default category has no tax rates in the default tax zone" do + it "should return 0" do + expect(Spree::TaxRate.default).to eq 0 + end + end + end + end + + context "#adjust" do + before do + @country = create(:country) + @zone = create(:zone, name: "Country Zone", default_tax: true, zone_members: []) + @zone.zone_members.create(zoneable: @country) + @category = Spree::TaxCategory.create(name: "Taxable Foo") + @category2 = Spree::TaxCategory.create(name: "Non Taxable") + @rate1 = Spree::TaxRate.create( + amount: 0.10, + calculator: ::Calculator::DefaultTax.new, + tax_category: @category, + zone: @zone + ) + @rate2 = Spree::TaxRate.create( + amount: 0.05, + calculator: ::Calculator::DefaultTax.new, + tax_category: @category, + zone: @zone + ) + @order = Spree::Order.create! + @taxable = create(:product, tax_category: @category) + @nontaxable = create(:product, tax_category: @category2) + end + + context "not taxable line item " do + let!(:line_item) { @order.contents.add(@nontaxable.variants.first, 1) } + + it "should not create a tax adjustment" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.tax.charge.count).to eq 0 + end + + it "should not create a refund" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.credit.count).to eq 0 + end + end + + context "taxable line item" do + let!(:line_item) { @order.contents.add(@taxable.variants.first, 1) } + + before do + @rate1.update_column(:included_in_price, true) + @rate2.update_column(:included_in_price, true) + end + + context "when price includes tax" do + context "when zone is contained by default tax zone" do + it "should create two adjustments, one for each tax rate" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.count).to eq 2 + end + + it "should not create a tax refund" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.credit.count).to eq 0 + end + end + + context "when order's zone is neither the default zone, or included in the default zone, but matches the rate's zone" do + before do + # With no zone members, this zone will not contain anything + @zone.zone_members.delete_all + end + + it "should create an adjustment" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.charge.count).to eq 2 + end + + it "should not create a tax refund for each tax rate" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.credit.count).to eq 0 + end + end + end + + context "when order's zone does not match default zone, is not included in the default zone, AND does not match the rate's zone" do + before do + @new_zone = create(:zone, name: "New Zone", default_tax: false) + @new_country = create(:country, name: "New Country") + @new_zone.zone_members.create(zoneable: @new_country) + @new_state = create(:state, country: @new_country) + @order.ship_address = create(:address, country: @new_country, state: @new_state) + @order.save + end + + it "should not create positive adjustments" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.charge.count).to eq 0 + end + + it "should create a tax refund for each tax rate" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.credit.count).to eq 2 + end + end + + context "when price does not include tax" do + before do + allow(@order).to receive(:tax_zone) { @zone } + + [@rate1, @rate2].each do |rate| + rate.included_in_price = false + rate.zone = @zone + rate.save + end + end + + it "should not delete adjustments for complete order when taxrate is deleted" do + @order.update_column :completed_at, Time.now + @rate1.destroy! + @rate2.destroy! + expect(line_item.adjustments.count).to eq 2 + end + + it "should create adjustments" do + expect(line_item.adjustments.count).to eq 2 + end + + it "should not create a tax refund" do + expect(line_item.adjustments.credit.count).to eq 0 + end + + it "should remove adjustments when tax_zone is removed" do + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.count).to eq 2 + allow(@order).to receive(:tax_zone) { nil } + Spree::TaxRate.adjust(@order, @order.line_items) + expect(line_item.adjustments.count).to eq 0 + end + end + end + end + end end end From d450aff6073d51cc74632b41f7a0bf89a99902c7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 23:30:04 +0000 Subject: [PATCH 05/46] Update Order#create_tax_charge! TaxRate#adjust now handles individual items instead of the whole order :tada: We can use this elsewhere too, for example to re-apply taxes on a single line item, we can do: Spree::TaxRate.adjust(order, [line_item]) --- app/models/spree/order.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index c9bbec9989..1ed3c80da0 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -293,7 +293,8 @@ module Spree # Creates new tax charges if there are any applicable rates. If prices already # include taxes then price adjustments are created instead. def create_tax_charge! - Spree::TaxRate.adjust(self) + Spree::TaxRate.adjust(self, line_items) + Spree::TaxRate.adjust(self, shipments) if shipments.any? end def name From 8479d4ff7cb3509a60a8e0facd4a483df9fe51d3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 23:30:18 +0000 Subject: [PATCH 06/46] Update outdated calls to TaxRate#adjust in specs --- spec/features/admin/order_print_ticket_spec.rb | 6 +++--- spec/features/admin/order_spec.rb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/features/admin/order_print_ticket_spec.rb b/spec/features/admin/order_print_ticket_spec.rb index 9bfddf205d..f5d9d30ab0 100644 --- a/spec/features/admin/order_print_ticket_spec.rb +++ b/spec/features/admin/order_print_ticket_spec.rb @@ -17,9 +17,9 @@ feature ' let!(:order) do create(:order_with_taxes, distributor: distributor, ship_address: create(:address), product_price: 110, tax_rate_amount: 0.1, - tax_rate_name: "Tax 1").tap do |record| - Spree::TaxRate.adjust(record) - record.update_shipping_fees! + tax_rate_name: "Tax 1").tap do |order| + order.create_tax_charge! + order.update_shipping_fees! end end diff --git a/spec/features/admin/order_spec.rb b/spec/features/admin/order_spec.rb index 34921c68d1..9ed8df28d0 100644 --- a/spec/features/admin/order_spec.rb +++ b/spec/features/admin/order_spec.rb @@ -308,9 +308,9 @@ feature ' let!(:order) do create(:order_with_taxes, distributor: distributor1, ship_address: create(:address), product_price: 110, tax_rate_amount: 0.1, - tax_rate_name: "Tax 1").tap do |record| - Spree::TaxRate.adjust(record) - record.update_shipping_fees! + tax_rate_name: "Tax 1").tap do |order| + order.create_tax_charge! + order.update_shipping_fees! end end From ff9ad96b748748f2f929aa829eb42a6d1dd84cd6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 15:27:30 +0000 Subject: [PATCH 07/46] Update TaxRate specs --- spec/models/spree/adjustment_spec.rb | 57 ++++++++++++---------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 5d66493d64..e02df21ea2 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -177,22 +177,38 @@ module Spree let!(:zone) { create(:zone_with_member) } let!(:order) { create(:order, bill_address: create(:address)) } let!(:line_item) { create(:line_item, order: order) } - let(:tax_rate) { create(:tax_rate, included_in_price: true, calculator: ::Calculator::FlatRate.new(preferred_amount: 0.1)) } + let(:tax_category) { create(:tax_category, tax_rates: [tax_rate]) } + let(:tax_rate) { create(:tax_rate, included_in_price: true, amount: 0.10) } let(:adjustment) { line_item.adjustments.reload.first } before do order.reload - tax_rate.adjust(order) + tax_rate.adjust(order, line_item) end - it "has tax included" do - expect(adjustment.amount).to be_positive - expect(adjustment.included).to be true + context "when the tax rate is inclusive" do + it "has 10% inclusive tax correctly recorded" do + amount = line_item.amount - (line_item.amount / (1 + tax_rate.amount)) + rounded_amount = tax_rate.calculator.__send__(:round_to_two_places, amount) + expect(adjustment.amount).to eq rounded_amount + expect(adjustment.amount).to eq 0.91 + expect(adjustment.included).to be true + end + + it "does not crash when order data has been updated previously" do + order.line_item_adjustments.first.destroy + tax_rate.adjust(order, line_item) + end end - it "does not crash when order data has been updated previously" do - order.line_item_adjustments.first.destroy - tax_rate.adjust(order) + context "when the tax rate is additional" do + let(:tax_rate) { create(:tax_rate, included_in_price: false, amount: 0.10) } + + it "has 10% added tax correctly recorded" do + expect(adjustment.amount).to eq line_item.amount * tax_rate.amount + expect(adjustment.amount).to eq 1.0 + expect(adjustment.included).to be false + end end end @@ -248,30 +264,7 @@ module Spree context "when the shipment has an added tax rate" do let(:inclusive_tax) { false } - # Current behaviour. Will be replaced by the pending test below - it "records the tax on the order's adjustments" do - order.shipments = [shipment] - order.create_tax_charge! - order.update_totals - - expect(order.shipment_adjustments.tax.count).to be_zero - - # Finding the added tax for an amount: - # total * rate - # 50 * 0.25 - # = 12.5 - expect(order.adjustments.tax.first.amount).to eq(12.50) - expect(order.adjustments.tax.first.included).to eq false - - expect(shipment.reload.cost).to eq(50) - expect(shipment.included_tax_total).to eq(0) - expect(shipment.additional_tax_total).to eq(0) - - expect(order.included_tax_total).to eq(0) - expect(order.additional_tax_total).to eq(12.50) - end - - xit "records the tax on the shipment's adjustments" do + it "records the tax on the shipment's adjustments" do order.shipments = [shipment] order.create_tax_charge! order.update_totals From 8c9733d8da2c035736e5e79959c23de4fbf232cf Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 16:01:05 +0000 Subject: [PATCH 08/46] Remove included tax check from CalculatedAdjustments This is handled in TaxRate now. --- lib/spree/core/calculated_adjustments.rb | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/lib/spree/core/calculated_adjustments.rb b/lib/spree/core/calculated_adjustments.rb index 690dcb19a7..5b45063c3d 100644 --- a/lib/spree/core/calculated_adjustments.rb +++ b/lib/spree/core/calculated_adjustments.rb @@ -36,8 +36,7 @@ module Spree order: order_object_for(adjustable), label: label, mandatory: mandatory, - state: state, - included: tax_included?(self, adjustable) + state: state } if adjustable.respond_to?(:adjustments) @@ -65,15 +64,6 @@ module Spree private - # Used for setting the #included boolean on tax adjustments. This will be removed in a - # later step, as the responsibility for creating all adjustments related to tax will be - # moved into the Spree::TaxRate class. - def tax_included?(originator, target) - originator.is_a?(Spree::TaxRate) && - originator.included_in_price && - originator.default_zone_or_zone_match?(order_object_for(target)) - end - def order_object_for(target) # Temporary method for adjustments transition. if target.is_a? Spree::Order From a22cc96ea56d92045e89f84aeea5ca6e5734158a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 16:17:46 +0000 Subject: [PATCH 09/46] Simplify DefaultTax calculator This logic is handled in TaxRate and doesn't need to be duplicated here. --- app/models/calculator/default_tax.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/models/calculator/default_tax.rb b/app/models/calculator/default_tax.rb index fb764340fa..be1447930d 100644 --- a/app/models/calculator/default_tax.rb +++ b/app/models/calculator/default_tax.rb @@ -79,14 +79,10 @@ module Calculator end def compute_shipment_or_line_item(item) - if item.tax_category == rate.tax_category - if rate.included_in_price - deduced_total_by_rate(item.amount, rate) - else - round_to_two_places(item.amount * rate.amount) - end + if rate.included_in_price + deduced_total_by_rate(item.amount, rate) else - 0 + round_to_two_places(item.amount * rate.amount) end end alias_method :compute_shipment, :compute_shipment_or_line_item From 6c340f8ed402c707059fde596b3556ea6cb0ae9f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 19:04:01 +0000 Subject: [PATCH 10/46] Update SalesTax test setup --- spec/features/admin/reports_spec.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 7073da51be..d96c63e0b3 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -186,7 +186,7 @@ feature ' let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } let!(:shipping_method) { create(:shipping_method_with, :expensive_name, distributors: [distributor1], tax_category: shipping_tax_category) } let(:enterprise_fee) { create(:enterprise_fee, enterprise: user1.enterprises.first, tax_category: product2.tax_category, calculator: Calculator::FlatRate.new(preferred_amount: 120.0)) } - let(:order_cycle) { create(:simple_order_cycle, coordinator: distributor1, coordinator_fees: [enterprise_fee], distributors: [distributor1], variants: [product1.master]) } + let(:order_cycle) { create(:simple_order_cycle, coordinator: distributor1, coordinator_fees: [enterprise_fee], distributors: [distributor1], variants: [product1.variants.first, product2.variants.first]) } let!(:zone) { create(:zone_with_member) } let(:address) { create(:address) } @@ -194,20 +194,20 @@ feature ' let(:product1) { create(:taxed_product, zone: zone, price: 12.54, tax_rate_amount: 0) } let(:product2) { create(:taxed_product, zone: zone, price: 500.15, tax_rate_amount: 0.2) } - let!(:line_item1) { create(:line_item, variant: product1.master, price: 12.54, quantity: 1, order: order1) } - let!(:line_item2) { create(:line_item, variant: product2.master, price: 500.15, quantity: 3, order: order1) } + let!(:line_item1) { create(:line_item, variant: product1.variants.first, price: 12.54, quantity: 1, order: order1) } + let!(:line_item2) { create(:line_item, variant: product2.variants.first, price: 500.15, quantity: 3, order: order1) } before do order1.reload - 2.times { order1.next } - order1.select_shipping_method shipping_method.id - order1.reload.recreate_all_fees! - order1.create_tax_charge! - order1.update_order! - order1.finalize! + break unless order1.next! until order1.delivery? + order1.select_shipping_method(shipping_method.id) + order1.recreate_all_fees! + break unless order1.next! until order1.payment? + create(:payment, state: "checkout", order: order1, amount: order1.reload.total, + payment_method: create(:payment_method, distributors: [distributor1])) + break unless order1.next! until order1.complete? login_as_admin_and_visit spree.admin_reports_path - click_link "Sales Tax" select("Tax types", from: "report_type") end From c2211c501d0f43760c030250284b86b65b18766c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 12:43:49 +0000 Subject: [PATCH 11/46] Improve test setup in Xero Invoices spec --- spec/features/admin/reports_spec.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index d96c63e0b3..01f4cb9c29 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -397,15 +397,17 @@ feature ' let(:order_cycle) { create(:simple_order_cycle, coordinator: distributor1, coordinator_fees: [enterprise_fee1, enterprise_fee2], distributors: [distributor1], variants: [product1.master]) } let!(:zone) { create(:zone_with_member) } - let(:country) { Spree::Country.find Spree::Config.default_country_id } - let(:bill_address) { create(:address, firstname: 'Customer', lastname: 'Name', address1: 'customer l1', address2: '', city: 'customer city', zipcode: 1234, country: country) } + let(:bill_address) { + create(:address, firstname: 'Customer', lastname: 'Name', address1: 'customer l1', + address2: '', city: 'customer city', zipcode: 1234) + } let(:order1) { create(:order, order_cycle: order_cycle, distributor: user1.enterprises.first, shipments: [shipment], bill_address: bill_address) } let(:product1) { create(:taxed_product, zone: zone, price: 12.54, tax_rate_amount: 0, sku: 'sku1') } let(:product2) { create(:taxed_product, zone: zone, price: 500.15, tax_rate_amount: 0.2, sku: 'sku2') } describe "with adjustments" do - let!(:line_item1) { create(:line_item, variant: product1.master, price: 12.54, quantity: 1, order: order1) } - let!(:line_item2) { create(:line_item, variant: product2.master, price: 500.15, quantity: 3, order: order1) } + let!(:line_item1) { create(:line_item, variant: product1.variants.first, price: 12.54, quantity: 1, order: order1) } + let!(:line_item2) { create(:line_item, variant: product2.variants.first, price: 500.15, quantity: 3, order: order1) } let!(:adj_shipping) { create(:adjustment, order: order1, adjustable: order1, label: "Shipping", originator: shipping_method, amount: 100.55) } let!(:adj_fee1) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee1, label: "Enterprise fee untaxed", amount: 10, included_tax: 0) } @@ -418,8 +420,10 @@ feature ' order1.update_attribute :email, 'customer@email.com' order1.shipment.update_columns(included_tax_total: 10.06) Timecop.travel(Time.zone.local(2015, 4, 25, 14, 0, 0)) { order1.finalize! } - login_as_admin_and_visit spree.admin_reports_path + order1.reload + order1.create_tax_charge! + login_as_admin_and_visit spree.admin_reports_path click_link 'Xero Invoices' end @@ -508,7 +512,7 @@ feature ' end def xero_invoice_row(sku, description, amount, quantity, tax_type, opts = {}) - opts.reverse_merge!(customer_name: 'Customer Name', address1: 'customer l1', city: 'customer city', state: 'Victoria', zipcode: '1234', country: country.name, invoice_number: order1.number, order_number: order1.number, invoice_date: '2015-04-26', due_date: '2015-05-26', account_code: 'food sales') + opts.reverse_merge!(customer_name: 'Customer Name', address1: 'customer l1', city: 'customer city', state: 'Victoria', zipcode: '1234', country: 'Australia', invoice_number: order1.number, order_number: order1.number, invoice_date: '2015-04-26', due_date: '2015-05-26', account_code: 'food sales') [opts[:customer_name], 'customer@email.com', opts[:address1], '', '', '', opts[:city], opts[:state], opts[:zipcode], opts[:country], opts[:invoice_number], opts[:order_number], opts[:invoice_date], opts[:due_date], From 2de442f44d4df2a479e27df9887474c4667752f8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 14:10:38 +0000 Subject: [PATCH 12/46] Move taxing of enterprise fees to TaxRate --- app/models/spree/adjustment.rb | 2 ++ app/models/spree/order.rb | 1 + .../enterprise_fee_applicator.rb | 16 ++++++++-------- lib/spree/core/calculated_adjustments.rb | 5 +++-- spec/models/spree/order/state_machine_spec.rb | 2 +- 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 973f7facee..366ade4935 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -35,10 +35,12 @@ module Spree # So we don't need the option `dependent: :destroy` as long as # AdjustmentMetadata has no destroy logic itself. has_one :metadata, class_name: 'AdjustmentMetadata' + has_many :adjustments, as: :adjustable, dependent: :destroy belongs_to :adjustable, polymorphic: true belongs_to :originator, -> { with_deleted }, polymorphic: true belongs_to :order, class_name: "Spree::Order" + belongs_to :tax_category, class_name: 'Spree::TaxCategory' belongs_to :tax_rate, -> { where spree_adjustments: { originator_type: 'Spree::TaxRate' } }, foreign_key: 'originator_id' diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 1ed3c80da0..ba0cdc2b80 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -295,6 +295,7 @@ module Spree def create_tax_charge! Spree::TaxRate.adjust(self, line_items) Spree::TaxRate.adjust(self, shipments) if shipments.any? + Spree::TaxRate.adjust(self, all_adjustments.enterprise_fee) end def name diff --git a/lib/open_food_network/enterprise_fee_applicator.rb b/lib/open_food_network/enterprise_fee_applicator.rb index fe4544c604..8b12e17e76 100644 --- a/lib/open_food_network/enterprise_fee_applicator.rb +++ b/lib/open_food_network/enterprise_fee_applicator.rb @@ -11,11 +11,11 @@ module OpenFoodNetwork private def create_adjustment(label, adjustable) - adjustment = enterprise_fee.create_adjustment(label, adjustable, true) + adjustment = enterprise_fee.create_adjustment( + label, adjustable, true, "closed", tax_category(adjustable) + ) AdjustmentMetadata.create! adjustment: adjustment, enterprise: enterprise_fee.enterprise, fee_name: enterprise_fee.name, fee_type: enterprise_fee.fee_type, enterprise_role: role - - adjustment.set_absolute_included_tax! adjustment_tax(adjustment) end def line_item_adjustment_label @@ -30,11 +30,11 @@ module OpenFoodNetwork I18n.t(:enterprise_fee_by, type: enterprise_fee.fee_type, role: role, enterprise_name: enterprise_fee.enterprise.name) end - def adjustment_tax(adjustment) - tax_rates = TaxRateFinder.tax_rates_of(adjustment) - - tax_rates.select(&:included_in_price).sum do |rate| - rate.compute_tax adjustment.amount + def tax_category(target) + if target.is_a?(Spree::LineItem) && enterprise_fee.inherits_tax_category? + target.product.tax_category + else + enterprise_fee.tax_category end end end diff --git a/lib/spree/core/calculated_adjustments.rb b/lib/spree/core/calculated_adjustments.rb index 5b45063c3d..3e06a95179 100644 --- a/lib/spree/core/calculated_adjustments.rb +++ b/lib/spree/core/calculated_adjustments.rb @@ -26,7 +26,7 @@ module Spree # (which is any class that has_many :adjustments) and sets amount based on the # calculator as applied to the given calculable (Order, LineItems[], Shipment, etc.) # By default the adjustment will not be considered mandatory - def create_adjustment(label, adjustable, mandatory = false, state = "closed") + def create_adjustment(label, adjustable, mandatory = false, state = "closed", tax_category = nil) amount = compute_amount(adjustable) return if amount.zero? && !mandatory @@ -36,7 +36,8 @@ module Spree order: order_object_for(adjustable), label: label, mandatory: mandatory, - state: state + state: state, + tax_category: tax_category } if adjustable.respond_to?(:adjustments) diff --git a/spec/models/spree/order/state_machine_spec.rb b/spec/models/spree/order/state_machine_spec.rb index 0b37ee41fa..e61b84a9f4 100644 --- a/spec/models/spree/order/state_machine_spec.rb +++ b/spec/models/spree/order/state_machine_spec.rb @@ -55,7 +55,7 @@ describe Spree::Order do end it "adjusts tax rates when transitioning to payment" do - expect(Spree::TaxRate).to receive(:adjust) + expect(Spree::TaxRate).to receive(:adjust).at_least(:once) order.next! end end From b3240f859ac4b4895675718d26e12f91960e6c80 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 23 Mar 2021 18:01:23 +0000 Subject: [PATCH 13/46] Add tax category id to adjustments --- .../20210323175627_add_tax_category_to_adjustments.rb | 6 ++++++ db/schema.rb | 2 ++ 2 files changed, 8 insertions(+) create mode 100644 db/migrate/20210323175627_add_tax_category_to_adjustments.rb diff --git a/db/migrate/20210323175627_add_tax_category_to_adjustments.rb b/db/migrate/20210323175627_add_tax_category_to_adjustments.rb new file mode 100644 index 0000000000..37cdb3f5a8 --- /dev/null +++ b/db/migrate/20210323175627_add_tax_category_to_adjustments.rb @@ -0,0 +1,6 @@ +class AddTaxCategoryToAdjustments < ActiveRecord::Migration[5.0] + def change + add_column :spree_adjustments, :tax_category_id, :integer + add_index :spree_adjustments, :tax_category_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 2a16f4a7d7..430ee9018a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -388,9 +388,11 @@ ActiveRecord::Schema.define(version: 2021_04_14_171109) do t.string "state", limit: 255 t.integer "order_id" t.boolean "included", default: false + t.integer "tax_category_id" t.index ["adjustable_type", "adjustable_id"], name: "index_spree_adjustments_on_adjustable_type_and_adjustable_id" t.index ["order_id"], name: "index_spree_adjustments_on_order_id" t.index ["originator_type", "originator_id"], name: "index_spree_adjustments_on_originator_type_and_originator_id" + t.index ["tax_category_id"], name: "index_spree_adjustments_on_tax_category_id" end create_table "spree_assets", force: :cascade do |t| From 52452f79398620b454fcc532336b06892d1ee480 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 20:37:21 +0000 Subject: [PATCH 14/46] Update default tax calculator Line items, shipments and fees can now all be calculated in the same way when applying tax. --- app/models/calculator/default_tax.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/models/calculator/default_tax.rb b/app/models/calculator/default_tax.rb index be1447930d..b4133f60ec 100644 --- a/app/models/calculator/default_tax.rb +++ b/app/models/calculator/default_tax.rb @@ -12,10 +12,8 @@ module Calculator case computable when Spree::Order compute_order(computable) - when Spree::Shipment - compute_shipment(computable) - when Spree::LineItem - compute_line_item(computable) + when Spree::Shipment, Spree::LineItem, Spree::Adjustment + compute_item(computable) end end @@ -78,15 +76,13 @@ module Calculator .sum { |applicator| applicator.enterprise_fee.compute_amount(order) } end - def compute_shipment_or_line_item(item) + def compute_item(item) if rate.included_in_price deduced_total_by_rate(item.amount, rate) else round_to_two_places(item.amount * rate.amount) end end - alias_method :compute_shipment, :compute_shipment_or_line_item - alias_method :compute_line_item, :compute_shipment_or_line_item def round_to_two_places(amount) BigDecimal(amount.to_s).round(2, BigDecimal::ROUND_HALF_UP) From 510f74f654c254d9b1985427e15fba7dac767d68 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 21:13:18 +0000 Subject: [PATCH 15/46] Update OrderTaxAdjustmentsFetcher Taxes on Enterprise Fees are now recorded in proper tax adjustments, so they don't need special treatment. --- app/services/order_tax_adjustments_fetcher.rb | 8 +++----- spec/services/order_tax_adjustments_fetcher_spec.rb | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 7c01a2b175..e39522c2ab 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -21,10 +21,9 @@ class OrderTaxAdjustmentsFetcher def all tax_adjustments = order.all_adjustments.tax - enterprise_fees_with_tax = order.all_adjustments.enterprise_fee.with_tax admin_adjustments_with_tax = order.all_adjustments.admin.with_tax - tax_adjustments.or(enterprise_fees_with_tax).or(admin_adjustments_with_tax) + tax_adjustments.or(admin_adjustments_with_tax) end def tax_rates_hash(adjustment) @@ -49,9 +48,8 @@ class OrderTaxAdjustmentsFetcher end def no_tax_adjustments?(adjustment) - # Enterprise Fees and Admin Adjustments currently do not have tax adjustments. + # Admin Adjustments currently do not have tax adjustments. # The tax amount is stored in the included_tax attribute. - adjustment.originator_type == "EnterpriseFee" || - adjustment.originator_type.nil? + adjustment.originator_type.nil? end end diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb index 866927d39f..6651b772d1 100644 --- a/spec/services/order_tax_adjustments_fetcher_spec.rb +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -77,8 +77,8 @@ describe OrderTaxAdjustmentsFetcher do before do order.reload order.adjustments << admin_adjustment - order.create_tax_charge! order.recreate_all_fees! + order.create_tax_charge! end subject { OrderTaxAdjustmentsFetcher.new(order).totals } From a1438bdb3d1790b051b955ac0f7c61587f0a17b6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Mar 2021 22:07:41 +0000 Subject: [PATCH 16/46] Update enterprise fee tax adjustment specs --- spec/models/spree/adjustment_spec.rb | 46 ++++++++++++---------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index e02df21ea2..76c63879dc 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -333,35 +333,34 @@ module Spree 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) } let(:order) { create(:order, line_items: [line_item], order_cycle: order_cycle, distributor: coordinator) } - let(:adjustment) { order.all_adjustments.reload.enterprise_fee.first } + let(:fee) { order.all_adjustments.reload.enterprise_fee.first } + let(:fee_tax) { fee.adjustments.tax.first } context "when enterprise fees have a fixed tax_category" do before do - order.reload.recreate_all_fees! + order.recreate_all_fees! end context "when enterprise fees are taxed per-order" do let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, tax_category: fee_tax_category, calculator: ::Calculator::FlatRate.new(preferred_amount: 50.0)) } describe "when the tax rate includes the tax in the price" do - it "records the tax on the enterprise fee adjustments" do + it "records the correct amount in a tax adjustment" do # The fee is $50, tax is 10%, and the fee is inclusive of tax # Therefore, the included tax should be 0.1/1.1 * 50 = $4.55 - expect(adjustment.included_tax).to eq(4.55) + expect(fee_tax.amount).to eq(4.55) end end describe "when the tax rate does not include the tax in the price" do before do fee_tax_rate.update_attribute :included_in_price, false - order.reload.create_tax_charge! # Updating line_item or order has the same effect order.recreate_all_fees! end - it "records the tax on TaxRate adjustment on the order" do - expect(adjustment.included_tax).to eq(0) - expect(order.adjustments.tax.first.amount).to eq(5.0) + it "records the correct amount in a tax adjustment" do + expect(fee_tax.amount).to eq(5.0) end end @@ -373,7 +372,7 @@ module Spree end it "records no tax as charged" do - expect(adjustment.included_tax).to eq(0) + expect(fee_tax).to be_nil end end end @@ -382,21 +381,19 @@ module Spree let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, tax_category: fee_tax_category, calculator: ::Calculator::PerItem.new(preferred_amount: 50.0)) } describe "when the tax rate includes the tax in the price" do - it "records the tax on the enterprise fee adjustments" do - expect(adjustment.included_tax).to eq(4.55) + it "records the correct amount in a tax adjustment" do + expect(fee_tax.amount).to eq(4.55) end end describe "when the tax rate does not include the tax in the price" do before do fee_tax_rate.update_attribute :included_in_price, false - order.reload.create_tax_charge! # Updating line_item or order has the same effect order.recreate_all_fees! end - it "records the tax on TaxRate adjustment on the order" do - expect(adjustment.included_tax).to eq(0) - expect(order.adjustments.tax.first.amount).to eq(5.0) + it "records the correct amount in a tax adjustment" do + expect(fee_tax.amount).to eq(5.0) end end end @@ -410,8 +407,6 @@ module Spree before do variant.product.update_attribute(:tax_category_id, product_tax_category.id) - - order.create_tax_charge! # Updating line_item or order has the same effect order.recreate_all_fees! end @@ -423,7 +418,7 @@ module Spree # EnterpriseFee tax category is nil and inheritance only applies to per item fees # so tax on the enterprise_fee adjustment will be 0 # Tax on line item is: 0.2/1.2 x $10 = $1.67 - expect(adjustment.included_tax).to eq(0.0) + expect(fee_tax).to be_nil expect(line_item.adjustments.tax.first.amount).to eq(1.67) end end @@ -431,7 +426,6 @@ module Spree describe "when the tax rate does not include the tax in the price" do before do product_tax_rate.update_attribute :included_in_price, false - order.reload.create_tax_charge! # Updating line_item or order has the same effect order.reload.recreate_all_fees! end @@ -439,8 +433,7 @@ module Spree # EnterpriseFee tax category is nil and inheritance only applies to per item fees # so total tax on the order is only that which applies to the line_item itself # ie. $10 x 0.2 = $2.0 - expect(adjustment.included_tax).to eq(0) - expect(order.adjustments.tax.first.amount).to eq(2.0) + expect(fee_tax).to be_nil end end end @@ -449,11 +442,11 @@ module Spree let(:enterprise_fee) { create(:enterprise_fee, enterprise: coordinator, inherits_tax_category: true, calculator: ::Calculator::PerItem.new(preferred_amount: 50.0)) } describe "when the tax rate includes the tax in the price" do - it "records the tax on the enterprise fee adjustments" do + it "records the correct amount in a tax adjustment" do # Applying product tax rate of 0.2 to enterprise fee of $50 # gives tax on fee of 0.2/1.2 x $50 = $8.33 # Tax on line item is: 0.2/1.2 x $10 = $1.67 - expect(adjustment.included_tax).to eq(8.33) + expect(fee_tax.amount).to eq(8.33) expect(line_item.adjustments.tax.first.amount).to eq(1.67) end end @@ -461,16 +454,15 @@ module Spree describe "when the tax rate does not include the tax in the price" do before do product_tax_rate.update_attribute :included_in_price, false - order.reload.create_tax_charge! # Updating line_item or order has the same effect order.recreate_all_fees! end - it "records the tax on TaxRate adjustment on the order" do + it "records the correct amount in a tax adjustment" do # EnterpriseFee inherits tax_category from product so total tax on # the order is that which applies to the line item itself, plus the # same rate applied to the fee of $50. ie. ($10 + $50) x 0.2 = $12.0 - expect(adjustment.included_tax).to eq(0) - expect(order.adjustments.tax.first.amount).to eq(12.0) + expect(fee_tax.amount).to eq(10.0) + expect(order.all_adjustments.tax.sum(:amount)).to eq(12.0) end end end From 6e9ae0b0db113923b4b8dcae0efe88c188a2f4f9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 9 Mar 2021 17:01:44 +0000 Subject: [PATCH 17/46] Update tax totals calculation in Order::Updater Line items, shipments, and fees now all have taxes recorded in a uniform way, so we can drop more complexity here (and the number of queries). --- .../app/services/order_management/order/updater.rb | 4 +--- .../spec/services/order_management/order/updater_spec.rb | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index fb1f7cda01..0e29c7c012 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -65,9 +65,7 @@ module OrderManagement def update_adjustment_total order.adjustment_total = all_adjustments.additional.eligible.sum(:amount) order.additional_tax_total = all_adjustments.tax.additional.sum(:amount) - order.included_tax_total = order.line_item_adjustments.tax.inclusive.sum(:amount) + - all_adjustments.enterprise_fee.sum(:included_tax) + - order.shipment_adjustments.tax.inclusive.sum(:amount) + + order.included_tax_total = all_adjustments.tax.inclusive.sum(:amount) + adjustments.admin.sum(:included_tax) end diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index c2ddf1f9d7..10260c56db 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -30,9 +30,7 @@ module OrderManagement it "updates adjustment totals" do allow(order).to receive_message_chain(:all_adjustments, :additional, :eligible, :sum).and_return(-5) allow(order).to receive_message_chain(:all_adjustments, :tax, :additional, :sum).and_return(20) - allow(order).to receive_message_chain(:all_adjustments, :enterprise_fee, :sum).and_return(10) - allow(order).to receive_message_chain(:all_adjustments, :shipping, :sum).and_return(5) - allow(order).to receive_message_chain(:shipment_adjustments, :tax, :inclusive, :sum).and_return(5) + allow(order).to receive_message_chain(:all_adjustments, :tax, :inclusive, :sum).and_return(15) allow(order).to receive_message_chain(:adjustments, :admin, :sum).and_return(2) updater.update_adjustment_total From 93e422ec598bd27f146997449e86a9d6f29b9385 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 9 Mar 2021 20:18:58 +0000 Subject: [PATCH 18/46] Update order fee tax test setup --- spec/models/spree/order_spec.rb | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 67c6306680..f5d25a2b5a 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -647,18 +647,27 @@ describe Spree::Order do describe "getting the total tax" do let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25) } + let(:fee_tax_rate) { create(:tax_rate, amount: 0.10) } let(:order) { create(:order) } let(:shipping_method) { create(:shipping_method_with, :flat_rate) } let!(:shipment) do create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) end let(:enterprise_fee) { create(:enterprise_fee) } - - before do - create(:adjustment, adjustable: order, originator: enterprise_fee, label: "EF", amount: 123, - included_tax: 2, order: order) + let!(:fee) { + create(:adjustment, adjustable: order, originator: enterprise_fee, label: "EF", amount: 20, + order: order) + } + let!(:fee_tax) { + create(:adjustment, adjustable: fee, originator: fee_tax_rate, + amount: 2, order: order, state: "closed") + } + let!(:shipping_tax) { create(:adjustment, adjustable: shipment, originator: shipping_tax_rate, amount: 10, order: order, state: "closed") + } + + before do order.update_order! end From 19f32b7825ec77ba82bc15c956fe9655e2c3688a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 9 Mar 2021 21:07:25 +0000 Subject: [PATCH 19/46] Update checkout tax display test setup --- spec/features/consumer/shopping/checkout_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index d0f4c03241..e4cd164c05 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -13,7 +13,9 @@ feature "As a consumer I want to check out my cart", js: true do let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true) } let(:supplier) { create(:supplier_enterprise) } let!(:order_cycle) { create(:simple_order_cycle, suppliers: [supplier], distributors: [distributor], coordinator: create(:distributor_enterprise), variants: [variant]) } - let(:enterprise_fee) { create(:enterprise_fee, amount: 1.23, tax_category: product.tax_category) } + let(:enterprise_fee) { create(:enterprise_fee, amount: 1.23, tax_category: fee_tax_category) } + let(:fee_tax_rate) { create(:tax_rate, amount: 0.10, zone: zone, included_in_price: true) } + let(:fee_tax_category) { create(:tax_category, tax_rates: [fee_tax_rate]) } let(:product) { create(:taxed_product, supplier: supplier, price: 10, zone: zone, tax_rate_amount: 0.1) } let(:variant) { product.variants.first } let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor, bill_address_id: nil, ship_address_id: nil) } From 84a40e6ae052ecc29430a0ec39d4c0d27c100016 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Mar 2021 13:06:38 +0000 Subject: [PATCH 20/46] Improve Order#enterpise_fee_tax --- app/models/spree/order.rb | 2 +- spec/models/spree/order_spec.rb | 29 ++++++++++++++++++----------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index ba0cdc2b80..817a0d716b 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -565,7 +565,7 @@ module Spree end def enterprise_fee_tax - all_adjustments.reload.enterprise_fee.sum(:included_tax) + all_adjustments.tax.where(adjustable: all_adjustments.enterprise_fee).sum(:amount) end def total_tax diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index f5d25a2b5a..5286c02fa2 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -627,21 +627,28 @@ describe Spree::Order do end end - describe "getting the enterprise fee tax" do + describe "#enterprise_fee_tax" do let!(:order) { create(:order) } - let(:enterprise_fee1) { create(:enterprise_fee) } - let(:enterprise_fee2) { create(:enterprise_fee) } - let!(:adjustment1) { - create(:adjustment, adjustable: order, originator: enterprise_fee1, label: "EF 1", - amount: 123, included_tax: 10.00, order: order) + let(:enterprise_fee) { create(:enterprise_fee) } + let!(:fee_adjustment) { + create(:adjustment, adjustable: order, originator: enterprise_fee, + amount: 100, order: order, state: "closed") } - let!(:adjustment2) { - create(:adjustment, adjustable: order, originator: enterprise_fee2, label: "EF 2", - amount: 123, included_tax: 2.00, order: order) + let!(:fee_tax1) { + create(:adjustment, adjustable: fee_adjustment, originator_type: "Spree::TaxRate", + amount: 12.3, order: order, state: "closed") + } + let!(:fee_tax2) { + create(:adjustment, adjustable: fee_adjustment, originator_type: "Spree::TaxRate", + amount: 4.5, order: order, state: "closed") + } + let!(:admin_adjustment) { + create(:adjustment, adjustable: order, originator: nil, + amount: 6.7, order: order, state: "closed") } - it "returns a sum of the tax included in all enterprise fees" do - expect(order.reload.enterprise_fee_tax).to eq(12) + it "returns a sum of all taxes on enterprise fees" do + expect(order.reload.enterprise_fee_tax).to eq(16.8) end end From a31487a86d92d22aaad56845cf5344273eff6a74 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 23 Mar 2021 19:56:29 +0000 Subject: [PATCH 21/46] Clear any legacy taxes when applying tax to an order #create_tax_charge! adds tax (adjustments) to all taxable items on an order. If an order order has legacy taxes (lump-sum tax adjustments on the order object), we remove them here before re-applying taxes (using the new setup). --- app/models/spree/order.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 817a0d716b..556b8b0c13 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -293,6 +293,8 @@ module Spree # Creates new tax charges if there are any applicable rates. If prices already # include taxes then price adjustments are created instead. def create_tax_charge! + clear_legacy_taxes! + Spree::TaxRate.adjust(self, line_items) Spree::TaxRate.adjust(self, shipments) if shipments.any? Spree::TaxRate.adjust(self, all_adjustments.enterprise_fee) @@ -592,6 +594,13 @@ module Spree @fee_handler ||= OrderFeesHandler.new(self) end + def clear_legacy_taxes! + # For instances that use additional taxes, old orders can have taxes recorded in + # lump-sum amounts per-order. We clear them here before re-applying the order's taxes, + # which will now be applied per-item. + adjustments.tax.additional.delete_all + end + def process_each_payment raise Core::GatewayError, Spree.t(:no_pending_payments) if pending_payments.empty? From b427fd6876cf7c2e1f95d27799fbcb80d09cda7a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 6 Apr 2021 18:34:59 +0100 Subject: [PATCH 22/46] Add data migration for enterprise fee tax amounts --- ...1242_migrate_enterprise_fee_tax_amounts.rb | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb new file mode 100644 index 0000000000..c634a8d81c --- /dev/null +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -0,0 +1,80 @@ +class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] + class Spree::Adjustment < ActiveRecord::Base + belongs_to :originator, -> { with_deleted }, polymorphic: true + belongs_to :adjustable, polymorphic: true + belongs_to :order, class_name: "Spree::Order" + belongs_to :tax_category, class_name: 'Spree::TaxCategory' + has_many :adjustments, as: :adjustable, dependent: :destroy + + scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } + end + class EnterpriseFee < ActiveRecord::Base + belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' + end + class Spree::LineItem < ActiveRecord::Base + belongs_to :variant, class_name: "Spree::Variant" + has_one :product, through: :variant + end + class Spree::Variant < ActiveRecord::Base + belongs_to :product, class_name: 'Spree::Product' + has_many :line_items, inverse_of: :variant + end + class Spree::Product < ActiveRecord::Base + belongs_to :tax_category, class_name: 'Spree::TaxCategory' + has_many :variants, class_name: 'Spree::Variant' + end + + def up + migrate_enterprise_fee_taxes! + end + + def migrate_enterprise_fee_taxes! + Spree::Adjustment.enterprise_fee.where('included_tax <> 0').includes(:originator).find_each do |fee| + tax_category = tax_category_for(fee) + tax_rate = tax_rate_for(tax_category) + + fee.update_columns(tax_category_id: tax_category.id) if tax_category.present? + + Spree::Adjustment.create!( + label: tax_adjustment_label(tax_rate), + amount: fee.included_tax, + order_id: fee.order_id, + adjustable: fee, + originator_type: "Spree::TaxRate", + originator_id: tax_rate&.id, + state: "closed", + included: true + ) + end + end + + def tax_adjustment_label(tax_rate) + if tax_rate.nil? + I18n.t('included_tax') + else + "#{tax_rate.name} #{tax_rate.amount * 100}% (#{I18n.t('models.tax_rate.included_in_price')})" + end + end + + def tax_category_for(fee) + enterprise_fee = fee.originator + + return if enterprise_fee.nil? + + if line_item_fee?(fee) && enterprise_fee.inherits_tax_category? + fee.adjustable.product.tax_category + else + enterprise_fee.tax_category + end + end + + def line_item_fee?(fee) + fee.adjustable_type == "Spree::LineItem" + end + + def tax_rate_for(tax_category) + return if tax_category.nil? + + tax_category.tax_rates.first + end +end From 43877f4e34131d975d5735fad8722a13bd84d8f7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 7 Apr 2021 18:21:41 +0100 Subject: [PATCH 23/46] Add unit tests for data migration --- ...migrate_enterprise_fee_tax_amounts_spec.rb | 177 ++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb diff --git a/spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb b/spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb new file mode 100644 index 0000000000..62a6ddf16e --- /dev/null +++ b/spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb @@ -0,0 +1,177 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative '../../db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts' + +describe MigrateEnterpriseFeeTaxAmounts do + subject { MigrateEnterpriseFeeTaxAmounts.new } + + let(:tax_category_regular) { create(:tax_category) } + let(:tax_rate_regular) { create(:tax_rate, tax_category: tax_category_regular) } + let(:tax_category_inherited) { create(:tax_category) } + let(:tax_rate_inherited) { create(:tax_rate, tax_category: tax_category_inherited) } + let(:enterprise_fee_regular) { create(:enterprise_fee, inherits_tax_category: false, + tax_category: tax_category_regular) } + let(:enterprise_fee_inheriting) { create(:enterprise_fee, inherits_tax_category: true) } + let(:fee_without_tax) { create(:adjustment, originator: enterprise_fee_regular, included_tax: 0) } + let(:fee_regular) { create(:adjustment, originator: enterprise_fee_regular, included_tax: 1.23) } + let(:fee_inheriting) { create(:adjustment, originator: enterprise_fee_inheriting, + adjustable: line_item, included_tax: 4.56) } + let(:product) { create(:product, tax_category: tax_category_inherited) } + let!(:line_item) { create(:line_item, variant: product.variants.first) } + + describe '#migrate_enterprise_fee_taxes!' do + context "when the fee has no tax" do + before { fee_without_tax } + + it "doesn't move the tax to an adjustment" do + expect(Spree::Adjustment).to_not receive(:create!) + + subject.migrate_enterprise_fee_taxes! + end + end + + context "when the fee has (non-inheriting) tax" do + before { fee_regular; tax_rate_regular } + + it "moves the tax to an adjustment" do + expect(Spree::Adjustment).to receive(:create!).and_call_original + + subject.migrate_enterprise_fee_taxes! + + expect(fee_regular.reload.tax_category).to eq tax_category_regular + + tax_adjustment = Spree::Adjustment.tax.last + + expect(tax_adjustment.amount).to eq fee_regular.included_tax + expect(tax_adjustment.adjustable).to eq fee_regular + expect(tax_adjustment.originator).to eq tax_rate_regular + expect(tax_adjustment.state).to eq "closed" + expect(tax_adjustment.included).to eq true + end + end + + context "when the fee has tax and inherits tax category from product" do + before { fee_inheriting; tax_rate_inherited } + + it "moves the tax to an adjustment" do + expect(Spree::Adjustment).to receive(:create!).and_call_original + + subject.migrate_enterprise_fee_taxes! + + expect(fee_inheriting.reload.tax_category).to eq tax_category_inherited + + tax_adjustment = Spree::Adjustment.tax.last + + expect(tax_adjustment.amount).to eq fee_inheriting.included_tax + expect(tax_adjustment.adjustable).to eq fee_inheriting + expect(tax_adjustment.originator).to eq tax_rate_inherited + expect(tax_adjustment.state).to eq "closed" + expect(tax_adjustment.included).to eq true + end + end + + context "when the fee has a soft-deleted EnterpriseFee" do + before do + enterprise_fee_regular.update_columns(deleted_at: Time.zone.now) + fee_regular + tax_rate_regular + end + + it "moves the tax to an adjustment" do + expect(Spree::Adjustment).to receive(:create!).and_call_original + + subject.migrate_enterprise_fee_taxes! + + expect(fee_regular.reload.tax_category).to eq tax_category_regular + + tax_adjustment = Spree::Adjustment.tax.last + + expect(tax_adjustment.amount).to eq fee_regular.included_tax + expect(tax_adjustment.adjustable).to eq fee_regular + expect(tax_adjustment.originator).to eq tax_rate_regular + expect(tax_adjustment.state).to eq "closed" + expect(tax_adjustment.included).to eq true + end + end + + context "when the fee has a hard-deleted EnterpriseFee" do + before do + fee_regular + tax_rate_regular + EnterpriseFee.delete_all + expect(fee_regular.reload.originator).to eq nil + end + + it "moves the tax to an adjustment" do + expect(Spree::Adjustment).to receive(:create!).and_call_original + + subject.migrate_enterprise_fee_taxes! + + expect(fee_regular.reload.tax_category).to eq nil + + tax_adjustment = Spree::Adjustment.tax.last + + expect(tax_adjustment.amount).to eq fee_regular.included_tax + expect(tax_adjustment.adjustable).to eq fee_regular + expect(tax_adjustment.originator_id).to eq nil + expect(tax_adjustment.originator_type).to eq "Spree::TaxRate" + expect(tax_adjustment.state).to eq "closed" + expect(tax_adjustment.included).to eq true + end + end + end + + describe '#tax_category_for' do + it "returns the correct tax category when not inherited from line item" do + expect(subject.tax_category_for(fee_regular)).to eq tax_category_regular + end + + it "returns the correct tax category when inherited from line item" do + expect(subject.tax_category_for(fee_inheriting)).to eq tax_category_inherited + end + + it "returns nil if the associated EnterpriseFee was hard-deleted and can't be found" do + fee_regular + EnterpriseFee.delete_all + fee_regular.reload + + expect(subject.tax_category_for(fee_regular)).to eq nil + end + end + + describe '#tax_rate_for' do + let!(:tax_category) { create(:tax_category) } + let!(:tax_rate) { create(:tax_rate, tax_category: tax_category) } + + context "when a tax rate exists" do + it "returns a valid tax rate" do + expect(subject.tax_rate_for(tax_category)).to eq tax_rate + end + end + + context "when the tax category is nil" do + it "returns nil" do + expect(subject.tax_rate_for(nil)).to eq nil + end + end + end + + describe '#tax_adjustment_label' do + let(:tax_rate) { create(:tax_rate, name: "Test Rate", amount: 0.20) } + + context "when a tax rate is given" do + it "makes a detailed label" do + expect(subject.tax_adjustment_label(tax_rate)). + to eq("Test Rate 20.0% (Included in price)") + end + end + + context "when the tax rate is nil" do + it "makes a basic label" do + expect(subject.tax_adjustment_label(nil)). + to eq("Included tax") + end + end + end +end From 0f5c39317af89202c5a55463d24d7b8ed63b3fca Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 7 Apr 2021 15:01:24 +0100 Subject: [PATCH 24/46] Re-apply taxes in Admin::OrdersController#update Ensures taxes are updated properly when hitting the "Update and Recalculate Fees" button on the order edit page. --- app/controllers/spree/admin/orders_controller.rb | 5 +++++ .../spree/admin/orders_controller_spec.rb | 15 +++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb index b1ee4f3dc6..51f4bbe84d 100644 --- a/app/controllers/spree/admin/orders_controller.rb +++ b/app/controllers/spree/admin/orders_controller.rb @@ -37,6 +37,11 @@ module Spree def update @order.recreate_all_fees! + unless @order.cart? + @order.create_tax_charge! + @order.update_order! + end + unless order_params.present? && @order.update(order_params) && @order.line_items.present? if @order.line_items.empty? && !params[:suppress_error_msg] @order.errors.add(:line_items, Spree.t('errors.messages.blank')) diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index 0d881bc82f..bae8400fc2 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -58,12 +58,19 @@ describe Spree::Admin::OrdersController, type: :controller do expect(response.status).to eq 302 end - it "updates distribution charges and redirects to order details page" do - expect_any_instance_of(Spree::Order).to receive(:recreate_all_fees!) + context "recalculating fees and taxes" do + before do + allow(Spree::Order).to receive_message_chain(:includes, :find_by!) { order } + end - spree_put :update, params + it "updates fees and taxes and redirects to order details page" do + expect(order).to receive(:recreate_all_fees!) + expect(order).to receive(:create_tax_charge!) - expect(response).to redirect_to spree.edit_admin_order_path(order) + spree_put :update, params + + expect(response).to redirect_to spree.edit_admin_order_path(order) + end end context "recalculating enterprise fees" do From 355837547ed0f16401f8d996ada1cb9c07ad974f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 7 Apr 2021 15:26:02 +0100 Subject: [PATCH 25/46] Add test coverage for legacy additional taxes in OrderTaxAdjustmentsFetcher OrderTaxAdjustmentsFetcher is used in various places to display the taxes on an order. This test validates that legacy taxes are still returned and displayed as normal. --- .../order_tax_adjustments_fetcher_spec.rb | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb index 6651b772d1..0f0d1055b5 100644 --- a/spec/services/order_tax_adjustments_fetcher_spec.rb +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -31,10 +31,17 @@ describe OrderTaxAdjustmentsFetcher do amount: 0.25, zone: zone) end + let(:tax_rate30) do + create(:tax_rate, included_in_price: false, + calculator: Calculator::DefaultTax.new, + amount: 0.30, + zone: zone) + end 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(:tax_category30) { create(:tax_category, tax_rates: [tax_rate30]) } let(:variant) do create(:variant, product: create(:product, tax_category: tax_category10)) @@ -73,18 +80,23 @@ describe OrderTaxAdjustmentsFetcher do let!(:shipment) do create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) end + let(:legacy_tax_adjustment) do + create(:adjustment, order: order, adjustable: order, amount: 1.23, originator: tax_rate30, + label: "Additional Tax Adjustment", state: "closed") + end before do order.reload order.adjustments << admin_adjustment order.recreate_all_fees! order.create_tax_charge! + legacy_tax_adjustment end subject { OrderTaxAdjustmentsFetcher.new(order).totals } - it "returns a hash with all 4 taxes" do - expect(subject.size).to eq(4) + it "returns a hash with all 5 taxes" do + expect(subject.size).to eq(5) end it "contains tax on all line_items" do @@ -102,5 +114,9 @@ describe OrderTaxAdjustmentsFetcher do it "contains tax on admin adjustment" do expect(subject[tax_rate25]).to eq(10.0) end + + it "contains (legacy) additional taxes recorded on the order" do + expect(subject[tax_rate30]).to eq(1.23) + end end end From c84f9e56fbb4af42e3d4f5369a1d62b68a4cc03d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 13 Apr 2021 12:11:41 +0100 Subject: [PATCH 26/46] Tax enterprise fee adjustments when calling #recreate_all_fees! --- app/models/spree/order.rb | 4 ++-- app/services/order_fees_handler.rb | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 556b8b0c13..f5000a763c 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -67,7 +67,7 @@ module Spree delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher delegate :update_totals, to: :updater delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, - :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler + :update_line_item_fees!, :recreate_all_fees!, :tax_enterprise_fees!, to: :fee_handler # Needs to happen before save_permalink is called before_validation :set_currency @@ -297,7 +297,7 @@ module Spree Spree::TaxRate.adjust(self, line_items) Spree::TaxRate.adjust(self, shipments) if shipments.any? - Spree::TaxRate.adjust(self, all_adjustments.enterprise_fee) + tax_enterprise_fees! end def name diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb index 6aa71dd9b9..89b05b1c60 100644 --- a/app/services/order_fees_handler.rb +++ b/app/services/order_fees_handler.rb @@ -19,6 +19,7 @@ class OrderFeesHandler create_line_item_fees! create_order_fees! + tax_enterprise_fees! end order.update_order! @@ -38,6 +39,10 @@ class OrderFeesHandler calculator.create_order_adjustments_for order end + def tax_enterprise_fees! + Spree::TaxRate.adjust(order, order.all_adjustments.enterprise_fee) + end + def update_line_item_fees!(line_item) line_item.adjustments.enterprise_fee.each do |fee| fee.update_adjustment!(line_item, force: true) From f791f6fa203135d482ec18626c351bcea131f7da Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 13 Apr 2021 13:22:17 +0100 Subject: [PATCH 27/46] Add test coverage to recalculating fees and taxes on completed orders --- .../spree/admin/orders_controller_spec.rb | 72 ++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index bae8400fc2..b4fdd52b9a 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -77,7 +77,9 @@ describe Spree::Admin::OrdersController, type: :controller do let(:user) { create(:admin_user) } let(:variant1) { create(:variant) } let(:variant2) { create(:variant) } - let(:distributor) { create(:distributor_enterprise, allow_order_changes: true) } + let(:distributor) { + create(:distributor_enterprise, allow_order_changes: true, charges_sales_tax: true) + } let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor]) } let(:enterprise_fee) { create(:enterprise_fee, calculator: build(:calculator_per_item) ) } let!(:exchange) { create(:exchange, incoming: true, sender: variant1.product.supplier, receiver: order_cycle.coordinator, variants: [variant1, variant2], enterprise_fees: [enterprise_fee]) } @@ -140,6 +142,74 @@ describe Spree::Admin::OrdersController, type: :controller do expect(order.adjustment_total).to eq 0 end end + + context "with taxes on enterprise fees" do + let(:zone) { create(:zone_with_member) } + let(:tax_included) { true } + let(:tax_rate) { + create(:tax_rate, amount: 0.25, included_in_price: tax_included, zone: zone) + } + let!(:enterprise_fee) { + create(:enterprise_fee, tax_category: tax_rate.tax_category, amount: 1) + } + + before do + allow(order).to receive(:tax_zone) { zone } + end + + context "with included taxes" do + it "taxes fees correctly" do + spree_put :update, { id: order.number } + order.reload + + expect(order.all_adjustments.tax.count).to eq 2 + expect(order.enterprise_fee_tax).to eq 0.4 + + expect(order.included_tax_total).to eq 0.4 + expect(order.additional_tax_total).to eq 0 + end + end + + context "with added taxes" do + let(:tax_included) { false } + + it "taxes fees correctly" do + spree_put :update, { id: order.number } + order.reload + + expect(order.all_adjustments.tax.count).to eq 2 + expect(order.enterprise_fee_tax).to eq 0.5 + + expect(order.included_tax_total).to eq 0 + expect(order.additional_tax_total).to eq 0.5 + end + + context "when the order has legacy taxes" do + let(:legacy_tax_adjustment) { + create(:adjustment, amount: 0.5, included: false, originator: tax_rate, + order: order, adjustable: order, state: "closed") + } + + before do + order.all_adjustments.tax.delete_all + order.adjustments << legacy_tax_adjustment + end + + it "removes legacy tax adjustments before recalculating tax" do + expect(order.all_adjustments.tax.count).to eq 1 + expect(order.all_adjustments.tax).to include legacy_tax_adjustment + expect(order.additional_tax_total).to eq 0.5 + + spree_put :update, { id: order.number } + order.reload + + expect(order.all_adjustments.tax.count).to eq 2 + expect(order.all_adjustments.tax).to_not include legacy_tax_adjustment + expect(order.additional_tax_total).to eq 0.5 + end + end + end + end end end From 38811b5a28b47156abbe19af1f9433554754ccf8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 13 Apr 2021 18:43:41 +0100 Subject: [PATCH 28/46] Add Bugsnag message if legacy taxes are used --- app/models/calculator/default_tax.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/calculator/default_tax.rb b/app/models/calculator/default_tax.rb index b4133f60ec..f9c118905a 100644 --- a/app/models/calculator/default_tax.rb +++ b/app/models/calculator/default_tax.rb @@ -23,8 +23,13 @@ module Calculator calculable end - # Enable calculation of tax for enterprise fees with tax rates where included_in_price = false def compute_order(order) + # This legacy tax calculation applies to additional taxes only, and is no longer used. + # In theory it should never be called any more after this has been deployed. + # If the message below doesn't show up in Bugsnag, we can safely delete this method and all + # the related methods below it. + Bugsnag.notify("Calculator::DefaultTax was called with legacy tax calculations") + calculator = OpenFoodNetwork::EnterpriseFeeCalculator.new(order.distributor, order.order_cycle) From 3f9a3b41da8d30e28d796f26e77cc14d335f692b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 14 Apr 2021 14:00:53 +0100 Subject: [PATCH 29/46] Refactor and improve EnterpriseFeeApplicator spec --- .../enterprise_fee_applicator_spec.rb | 112 +++++++++++------- 1 file changed, 66 insertions(+), 46 deletions(-) diff --git a/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb index 7ddab9d50d..be95930f95 100644 --- a/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb +++ b/spec/lib/open_food_network/enterprise_fee_applicator_spec.rb @@ -5,65 +5,85 @@ require 'open_food_network/enterprise_fee_applicator' module OpenFoodNetwork describe EnterpriseFeeApplicator do - it "creates an adjustment for a line item" do - line_item = create(:line_item) - enterprise_fee = create(:enterprise_fee) - product = create(:simple_product) + let(:line_item) { create(:line_item) } + let(:inherits_tax) { true } + let(:enterprise_fee) { + create(:enterprise_fee, inherits_tax_category: inherits_tax, tax_category: fee_tax_category) + } + let(:fee_tax_category) { nil } + let(:tax_category) { create(:tax_category) } + let(:product) { create(:simple_product, tax_category: tax_category) } + let(:target_variant) { product.variants.first } + let(:applicator) { EnterpriseFeeApplicator.new(enterprise_fee, target_variant, 'role') } - efa = EnterpriseFeeApplicator.new enterprise_fee, product.master, 'role' - allow(efa).to receive(:line_item_adjustment_label) { 'label' } - efa.create_line_item_adjustment line_item + describe "#create_line_item_adjustment" do + it "creates an adjustment for a line item" do + allow(applicator).to receive(:line_item_adjustment_label) { 'label' } + applicator.create_line_item_adjustment line_item - adjustment = Spree::Adjustment.last - expect(adjustment.label).to eq('label') - expect(adjustment.adjustable).to eq(line_item) - expect(adjustment.originator).to eq(enterprise_fee) - expect(adjustment).to be_mandatory + adjustment = Spree::Adjustment.last + expect(adjustment.label).to eq('label') + expect(adjustment.adjustable).to eq(line_item) + expect(adjustment.originator).to eq(enterprise_fee) + expect(adjustment.tax_category).to eq(tax_category) + expect(adjustment).to be_mandatory - md = adjustment.metadata - expect(md.enterprise).to eq(enterprise_fee.enterprise) - expect(md.fee_name).to eq(enterprise_fee.name) - expect(md.fee_type).to eq(enterprise_fee.fee_type) - expect(md.enterprise_role).to eq('role') + metadata = adjustment.metadata + expect(metadata.enterprise).to eq(enterprise_fee.enterprise) + expect(metadata.fee_name).to eq(enterprise_fee.name) + expect(metadata.fee_type).to eq(enterprise_fee.fee_type) + expect(metadata.enterprise_role).to eq('role') + end end - it "creates an adjustment for an order" do - order = create(:order) - enterprise_fee = create(:enterprise_fee) - product = create(:simple_product) + describe "#create_order_adjustment" do + let(:target_variant) { nil } + let(:inherits_tax) { false } + let(:fee_tax_category) { tax_category } + let(:order) { line_item.order } - efa = EnterpriseFeeApplicator.new enterprise_fee, nil, 'role' - allow(efa).to receive(:order_adjustment_label) { 'label' } - efa.create_order_adjustment order + it "creates an adjustment for an order" do + allow(applicator).to receive(:order_adjustment_label) { 'label' } + applicator.create_order_adjustment order - adjustment = Spree::Adjustment.last - expect(adjustment.label).to eq('label') - expect(adjustment.adjustable).to eq(order) - expect(adjustment.originator).to eq(enterprise_fee) - expect(adjustment).to be_mandatory + adjustment = Spree::Adjustment.last + expect(adjustment.label).to eq('label') + expect(adjustment.adjustable).to eq(order) + expect(adjustment.originator).to eq(enterprise_fee) + expect(adjustment.tax_category).to eq(tax_category) + expect(adjustment).to be_mandatory - md = adjustment.metadata - expect(md.enterprise).to eq(enterprise_fee.enterprise) - expect(md.fee_name).to eq(enterprise_fee.name) - expect(md.fee_type).to eq(enterprise_fee.fee_type) - expect(md.enterprise_role).to eq('role') + metadata = adjustment.metadata + expect(metadata.enterprise).to eq(enterprise_fee.enterprise) + expect(metadata.fee_name).to eq(enterprise_fee.name) + expect(metadata.fee_type).to eq(enterprise_fee.fee_type) + expect(metadata.enterprise_role).to eq('role') + end end - it "makes an adjustment label for a line item" do - variant = double(:variant, product: double(:product, name: 'Bananas')) - enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) + describe "making labels" do + let(:variant) { double(:variant, product: double(:product, name: 'Bananas')) } + let(:enterprise_fee) { + double(:enterprise_fee, fee_type: 'packing', + enterprise: double(:enterprise, name: 'Ballantyne')) + } + let(:applicator) { EnterpriseFeeApplicator.new enterprise_fee, variant, 'distributor' } - efa = EnterpriseFeeApplicator.new enterprise_fee, variant, 'distributor' + describe "#line_item_adjustment_label" do + it "makes an adjustment label for a line item" do + expect(applicator.send(:line_item_adjustment_label)). + to eq("Bananas - packing fee by distributor Ballantyne") + end + end - expect(efa.send(:line_item_adjustment_label)).to eq("Bananas - packing fee by distributor Ballantyne") - end + describe "#order_adjustment_label" do + let(:applicator) { EnterpriseFeeApplicator.new enterprise_fee, nil, 'distributor' } - it "makes an adjustment label for an order" do - enterprise_fee = double(:enterprise_fee, fee_type: 'packing', enterprise: double(:enterprise, name: 'Ballantyne')) - - efa = EnterpriseFeeApplicator.new enterprise_fee, nil, 'distributor' - - expect(efa.send(:order_adjustment_label)).to eq("Whole order - packing fee by distributor Ballantyne") + it "makes an adjustment label for an order" do + expect(applicator.send(:order_adjustment_label)). + to eq("Whole order - packing fee by distributor Ballantyne") + end + end end end end From f20cf509d3e7a3b3e4fe6b704db7c286eb2c2f17 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 14 Apr 2021 14:17:11 +0100 Subject: [PATCH 30/46] Add missing Spree translation --- config/locales/en.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index 18085ef2d0..49d8938da6 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3139,6 +3139,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using payment_method_not_supported: "Payment method not supported" resend_authorization_email: "Resend authorization email" rma_credit: "RMA credit" + refund: "Refund" server_error: "Server error" shipping_method_names: UPS Ground: "UPS Ground" From 28ebb303af462e839e1a7c55130fc8500b20b0d6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 22 Apr 2021 21:00:29 +0100 Subject: [PATCH 31/46] Simplify order interface --- app/models/spree/order.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index f5000a763c..7c79cd7da9 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -67,7 +67,7 @@ module Spree delegate :admin_and_handling_total, :payment_fee, :ship_total, to: :adjustments_fetcher delegate :update_totals, to: :updater delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, - :update_line_item_fees!, :recreate_all_fees!, :tax_enterprise_fees!, to: :fee_handler + :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler # Needs to happen before save_permalink is called before_validation :set_currency @@ -297,7 +297,7 @@ module Spree Spree::TaxRate.adjust(self, line_items) Spree::TaxRate.adjust(self, shipments) if shipments.any? - tax_enterprise_fees! + fee_handler.tax_enterprise_fees! end def name From bfea47802e3b6836247cc0201de8ffb370facf46 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 22 Apr 2021 22:41:54 +0100 Subject: [PATCH 32/46] Move fee tax call outside of lock and simplify order updating --- app/services/order_fees_handler.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb index 89b05b1c60..5f7862beb1 100644 --- a/app/services/order_fees_handler.rb +++ b/app/services/order_fees_handler.rb @@ -19,9 +19,9 @@ class OrderFeesHandler create_line_item_fees! create_order_fees! - tax_enterprise_fees! end + tax_enterprise_fees! order.update_order! end From dc7c674a0e950e11dbd5b279d430569d6efea510 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 23 Apr 2021 15:12:19 +0100 Subject: [PATCH 33/46] Switch to ApplicationRecord in migation spec Fixes: An error occurred while loading ./spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb. Failure/Error: require_relative '../../db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts' TypeError: superclass mismatch for class Adjustment # ./db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb:2:in `' # ./db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb:1:in `' # ./spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb:4:in `require_relative' # ./spec/migrations/migrate_enterprise_fee_tax_amounts_spec.rb:4:in `' # -e:1:in `
' --- ...0210406161242_migrate_enterprise_fee_tax_amounts.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb index c634a8d81c..3e7f8fa4c3 100644 --- a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -1,5 +1,5 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] - class Spree::Adjustment < ActiveRecord::Base + class Spree::Adjustment < ApplicationRecord belongs_to :originator, -> { with_deleted }, polymorphic: true belongs_to :adjustable, polymorphic: true belongs_to :order, class_name: "Spree::Order" @@ -8,18 +8,18 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } end - class EnterpriseFee < ActiveRecord::Base + class EnterpriseFee < ApplicationRecord belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' end - class Spree::LineItem < ActiveRecord::Base + class Spree::LineItem < ApplicationRecord belongs_to :variant, class_name: "Spree::Variant" has_one :product, through: :variant end - class Spree::Variant < ActiveRecord::Base + class Spree::Variant < ApplicationRecord belongs_to :product, class_name: 'Spree::Product' has_many :line_items, inverse_of: :variant end - class Spree::Product < ActiveRecord::Base + class Spree::Product < ApplicationRecord belongs_to :tax_category, class_name: 'Spree::TaxCategory' has_many :variants, class_name: 'Spree::Variant' end From 832fbb7d5d72cafa94d321fc6c1d5396d2e61cfb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 5 May 2021 11:47:47 +0100 Subject: [PATCH 34/46] Update data migration model definitions The good_migrations gem was not working with polymorphic associations. --- ...6161242_migrate_enterprise_fee_tax_amounts.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb index 3e7f8fa4c3..b97283d85e 100644 --- a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -1,3 +1,8 @@ +# It turns out the good_migrations gem doesn't play nicely with loading classes on polymorphic +# associations. The only workaround seems to be to load the class explicitly, which essentially +# skips the whole point of good_migrations... :/ +require 'enterprise_fee' + class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] class Spree::Adjustment < ApplicationRecord belongs_to :originator, -> { with_deleted }, polymorphic: true @@ -8,9 +13,6 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } end - class EnterpriseFee < ApplicationRecord - belongs_to :tax_category, class_name: 'Spree::TaxCategory', foreign_key: 'tax_category_id' - end class Spree::LineItem < ApplicationRecord belongs_to :variant, class_name: "Spree::Variant" has_one :product, through: :variant @@ -23,6 +25,14 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] belongs_to :tax_category, class_name: 'Spree::TaxCategory' has_many :variants, class_name: 'Spree::Variant' end + class Spree::TaxCategory < ApplicationRecord + has_many :tax_rates, dependent: :destroy, inverse_of: :tax_category + end + class Spree::TaxRate < ApplicationRecord + belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates + belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates + has_many :adjustments, as: :originator + end def up migrate_enterprise_fee_taxes! From 7007de752d078b28c5d70cef26c02ac368272c6d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 21 May 2021 12:12:11 +0100 Subject: [PATCH 35/46] Update BulkLineItemsController spec now that taxes on enterprise fees are updated correctly Tax on fees previously stayed the same unless the fees were deleted recreated from scratch, instead of updating when the fee amounts changed --- spec/controllers/admin/bulk_line_items_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/admin/bulk_line_items_controller_spec.rb b/spec/controllers/admin/bulk_line_items_controller_spec.rb index ea6cf9da30..6d5fda5ce7 100644 --- a/spec/controllers/admin/bulk_line_items_controller_spec.rb +++ b/spec/controllers/admin/bulk_line_items_controller_spec.rb @@ -366,7 +366,7 @@ describe Admin::BulkLineItemsController, type: :controller do expect(order.shipment_adjustments.sum(:amount)).to eq 12.57 expect(order.item_total).to eq 40.0 expect(order.adjustment_total).to eq 27.0 - expect(order.included_tax_total).to eq 2.93 # Pending: taxes on enterprise fees unchanged + expect(order.included_tax_total).to eq 3.38 expect(order.payment_state).to eq "balance_due" end end From 00988dc1e799bc85e591bd30275ac58005121a94 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 21 May 2021 13:01:24 +0100 Subject: [PATCH 36/46] Use :order_with_totals factory in payments controller spec This factory makes an order that actually has a line item :+1: --- .../spree/admin/orders/payments/payments_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb index 8d64b3a7f2..3edf6a5fb6 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -311,7 +311,7 @@ describe Spree::Admin::PaymentsController, type: :controller do end context "the order contains an item that is out of stock" do - let!(:order) { create(:order, distributor: shop, state: 'payment') } + let!(:order) { create(:order_with_totals, distributor: shop, state: 'payment') } before do order.line_items.first.variant.update_attribute(:on_hand, 0) From 9e1d8ab369b8facc13db9577d05a36ed876035ee Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 24 May 2021 18:57:54 +0100 Subject: [PATCH 37/46] Introduce legacy tax handing in Order::Updater whenever order totals change This will remove legacy tax adjustments, recalculate any relevant taxes for items in the order and re-apply them in non-legacy tax adjustments --- app/models/spree/adjustment.rb | 1 + app/models/spree/order.rb | 2 +- .../order_management/order/updater.rb | 9 ++++ .../order_management/order/updater_spec.rb | 46 +++++++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 366ade4935..20449e1213 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -72,6 +72,7 @@ module Spree scope :return_authorization, -> { where(originator_type: "Spree::ReturnAuthorization") } scope :inclusive, -> { where(included: true) } scope :additional, -> { where(included: false) } + scope :legacy_tax, -> { additional.tax.where(adjustable_type: "Spree::Order") } scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } scope :admin, -> { where(originator_type: nil) } diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 7c79cd7da9..bf8d9a057d 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -598,7 +598,7 @@ module Spree # For instances that use additional taxes, old orders can have taxes recorded in # lump-sum amounts per-order. We clear them here before re-applying the order's taxes, # which will now be applied per-item. - adjustments.tax.additional.delete_all + adjustments.legacy_tax.delete_all end def process_each_payment diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index 0e29c7c012..e700cf111f 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -24,6 +24,8 @@ module OrderManagement end def update_totals_and_states + handle_legacy_taxes + update_totals if order.completed? @@ -211,6 +213,13 @@ module OrderManagement def failed_payments? payments.present? && payments.valid.empty? end + + # Re-applies tax if any legacy taxes are present + def handle_legacy_taxes + return unless order.completed? && order.adjustments.legacy_tax.any? + + order.create_tax_charge! + end end end end diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index 10260c56db..7531ff7ec5 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -289,6 +289,52 @@ module OrderManagement end end end + + describe "updating order totals" do + describe "#update_totals_and_states" do + it "deals with legacy taxes" do + expect(updater).to receive(:handle_legacy_taxes) + + updater.update_totals_and_states + end + end + + describe "#handle_legacy_taxes" do + context "when the order is incomplete" do + it "doesn't touch taxes" do + allow(order).to receive(:completed?) { false } + + expect(order).to_not receive(:create_tax_charge!) + updater.__send__(:handle_legacy_taxes) + end + end + + context "when the order is complete" do + before { allow(order).to receive(:completed?) { true } } + + context "and the order has legacy taxes" do + let!(:legacy_tax_adjustment) { + create(:adjustment, order: order, adjustable: order, included: false, + originator_type: "Spree::TaxRate") + } + + it "re-applies order taxes" do + expect(order).to receive(:create_tax_charge!) + + updater.__send__(:handle_legacy_taxes) + end + end + + context "and the order has no legacy taxes" do + it "leaves taxes untouched" do + expect(order).to_not receive(:create_tax_charge!) + + updater.__send__(:handle_legacy_taxes) + end + end + end + end + end end end end From e21ef3f182a02121bbd5de493c198e2deb3d84d3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 24 May 2021 22:15:44 +0100 Subject: [PATCH 38/46] Add test coverage to #create_tax_charge! with legacy taxes --- spec/models/spree/order/tax_spec.rb | 55 +++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/spec/models/spree/order/tax_spec.rb b/spec/models/spree/order/tax_spec.rb index dae075c9fa..072e8999cd 100644 --- a/spec/models/spree/order/tax_spec.rb +++ b/spec/models/spree/order/tax_spec.rb @@ -113,5 +113,60 @@ module Spree end end end + + describe "#create_tax_charge!" do + context "handling legacy taxes" do + let(:order) { create(:order) } + let(:zone) { create(:zone_with_member) } + let(:tax_rate20) { + create(:tax_rate, amount: 0.20, included_in_price: false, zone: zone) + } + let(:tax_rate30) { + create(:tax_rate, amount: 0.30, included_in_price: false, zone: zone) + } + let!(:variant) { + create(:variant, tax_category: tax_rate20.tax_category, price: 10) + } + let!(:line_item) { + create(:line_item, variant: variant, order: order, quantity: 2) + } + let!(:shipping_method) { + create(:shipping_method, tax_category: tax_rate30.tax_category) + } + let!(:shipment) { + create(:shipment_with, :shipping_method, order: order, cost: 50, + shipping_method: shipping_method) + } + + before do + shipment.update_columns(cost: 20.0) + order.reload + + allow(order).to receive(:completed_at) { Time.zone.now } + allow(order).to receive(:tax_zone) { zone } + end + + context "when the order has legacy taxes" do + let!(:legacy_tax_adjustment) { + create(:adjustment, order: order, adjustable: order, included: false, + label: "legacy", originator_type: "Spree::TaxRate") + } + + it "removes any legacy tax adjustments on order" do + order.create_tax_charge! + + expect(order.reload.adjustments).to_not include legacy_tax_adjustment + end + + it "re-applies taxes on individual items" do + order.create_tax_charge! + + expect(order.all_adjustments.tax.count).to eq 2 + expect(line_item.adjustments.tax.first.amount).to eq 4 + expect(shipment.adjustments.tax.first.amount).to eq 6 + end + end + end + end end end From 401dd99225701aaf9868f5ca1b9c6c3f14f951d2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 1 Jun 2021 17:45:28 +0100 Subject: [PATCH 39/46] Update stubbing of default country id in TaxRate tests --- spec/models/spree/tax_rate_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/tax_rate_spec.rb b/spec/models/spree/tax_rate_spec.rb index 4db5494ab1..e057373256 100644 --- a/spec/models/spree/tax_rate_spec.rb +++ b/spec/models/spree/tax_rate_spec.rb @@ -289,7 +289,7 @@ module Spree context "when the default category has tax rates in the default tax zone" do before(:each) do - Spree::Config[:default_country_id] = country.id + allow(DefaultCountry).to receive(:id) { country.id } @zone = create(:zone, name: "Country Zone", default_tax: true) @zone.zone_members.create(zoneable: country) rate = Spree::TaxRate.create( From 76756923ba39552887f42cd9575196b00509f462 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 1 Jun 2021 17:48:57 +0100 Subject: [PATCH 40/46] Use non-legacy-tax example in paypal items spec --- spec/services/paypal_items_builder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/paypal_items_builder_spec.rb b/spec/services/paypal_items_builder_spec.rb index db0b0cef8b..767c1f460f 100644 --- a/spec/services/paypal_items_builder_spec.rb +++ b/spec/services/paypal_items_builder_spec.rb @@ -43,7 +43,7 @@ describe PaypalItemsBuilder do originator: included_tax_rate, included: true, state: "closed") } let!(:additional_tax_adjustment) { - create(:adjustment, label: "Additional Tax Adjustment", order: order, adjustable: order, + create(:adjustment, label: "Additional Tax Adjustment", order: order, adjustable: order.shipment, amount: 78, originator: additional_tax_rate, state: "closed") } let!(:enterprise_fee) { create(:enterprise_fee) } From d9f459d94a45e16f0190f79addb9beb47f02719f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Jun 2021 12:06:27 +0200 Subject: [PATCH 41/46] Update db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb Co-authored-by: Maikel --- .../20210406161242_migrate_enterprise_fee_tax_amounts.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb index b97283d85e..570a2789bd 100644 --- a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -83,8 +83,6 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] end def tax_rate_for(tax_category) - return if tax_category.nil? - - tax_category.tax_rates.first + tax_category&.tax_rates&.first end end From 7ecd67a3fee1579274ed726e89e00be24fec5320 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Jun 2021 13:15:24 +0100 Subject: [PATCH 42/46] Skip orphaned adjustments There are a handful of enterprise fee adjustments on line items in production data where the line item has actually been deleted and no longer exists, but the fee adjustment is still in the database. --- db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb index 570a2789bd..93cf62d095 100644 --- a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -72,7 +72,7 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] return if enterprise_fee.nil? if line_item_fee?(fee) && enterprise_fee.inherits_tax_category? - fee.adjustable.product.tax_category + fee.adjustable&.product&.tax_category else enterprise_fee.tax_category end From 7bcb0bcaa83b13d4094d4bd4c1646c1c3894f453 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Jun 2021 14:17:49 +0100 Subject: [PATCH 43/46] Eager-load the adjustment's adjustable Reduces the total migration time by ~15%. Requires including some more models, as order objects are now being eager-loaded as part of the polymorphic associations. --- .../20210406161242_migrate_enterprise_fee_tax_amounts.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb index 93cf62d095..f0f99e2bc3 100644 --- a/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb +++ b/db/migrate/20210406161242_migrate_enterprise_fee_tax_amounts.rb @@ -2,6 +2,8 @@ # associations. The only workaround seems to be to load the class explicitly, which essentially # skips the whole point of good_migrations... :/ require 'enterprise_fee' +require 'concerns/balance' +require 'spree/order' class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] class Spree::Adjustment < ApplicationRecord @@ -39,7 +41,9 @@ class MigrateEnterpriseFeeTaxAmounts < ActiveRecord::Migration[5.0] end def migrate_enterprise_fee_taxes! - Spree::Adjustment.enterprise_fee.where('included_tax <> 0').includes(:originator).find_each do |fee| + Spree::Adjustment.enterprise_fee.where('included_tax <> 0'). + includes(:originator, :adjustable).find_each do |fee| + tax_category = tax_category_for(fee) tax_rate = tax_rate_for(tax_category) From 7b641ace23ec3811023dcad6a238690a9da07cf9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 4 Jun 2021 20:35:31 +0100 Subject: [PATCH 44/46] Update Xero Invoices scopes for taxable fees Taxable enterprise fee adjustments now have a tax_category --- lib/open_food_network/xero_invoices_report.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/open_food_network/xero_invoices_report.rb b/lib/open_food_network/xero_invoices_report.rb index 93b7dbb4e2..a92c133607 100644 --- a/lib/open_food_network/xero_invoices_report.rb +++ b/lib/open_food_network/xero_invoices_report.rb @@ -188,11 +188,11 @@ module OpenFoodNetwork end def total_untaxable_fees(order) - order.all_adjustments.enterprise_fee.without_tax.sum(:amount) + order.all_adjustments.enterprise_fee.where(tax_category: nil).sum(:amount) end def total_taxable_fees(order) - order.all_adjustments.enterprise_fee.with_tax.sum(:amount) + order.all_adjustments.enterprise_fee.where.not(tax_category: nil).sum(:amount) end def total_shipping(order) From 5bfe079262104e53183e1bceb94c1cac0d3fa883 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 5 Jun 2021 07:58:52 +0100 Subject: [PATCH 45/46] Update Xero Invoices test setup Uses the new format of enterprise fee taxes --- spec/features/admin/reports_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 01f4cb9c29..460562e98f 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -409,10 +409,13 @@ feature ' let!(:line_item1) { create(:line_item, variant: product1.variants.first, price: 12.54, quantity: 1, order: order1) } let!(:line_item2) { create(:line_item, variant: product2.variants.first, price: 500.15, quantity: 3, order: order1) } + let!(:tax_category) { create(:tax_category) } + let!(:tax_rate) { create(:tax_rate, tax_category: tax_category) } let!(:adj_shipping) { create(:adjustment, order: order1, adjustable: order1, label: "Shipping", originator: shipping_method, amount: 100.55) } - let!(:adj_fee1) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee1, label: "Enterprise fee untaxed", amount: 10, included_tax: 0) } - let!(:adj_fee2) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee2, label: "Enterprise fee taxed", amount: 20, included_tax: 2) } - let!(:adj_manual1) { create(:adjustment, order: order1, adjustable: order1, originator: nil, label: "Manual adjustment", amount: 30, included_tax: 0) } + let!(:adj_fee1) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee1, label: "Enterprise fee untaxed", amount: 10) } + let!(:adj_fee2) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee2, label: "Enterprise fee taxed", amount: 20, tax_category: tax_category) } + let!(:adj_fee2_tax) { create(:adjustment, order: order1, adjustable: adj_fee2, originator: tax_rate, amount: 3, state: "closed") } + let!(:adj_manual1) { create(:adjustment, order: order1, adjustable: order1, originator: nil, label: "Manual adjustment", amount: 30) } let!(:adj_manual2) { create(:adjustment, order: order1, adjustable: order1, originator: nil, label: "Manual adjustment", amount: 40, included_tax: 3) } before do From 937cede9b8c2f84afbf353d85fec2be5b52d2cab Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 10 Jun 2021 17:16:45 +0100 Subject: [PATCH 46/46] Fix tax adjustment label :amount is actually a property on the TaxRate object itself that refers to the rate (eg 10% expressed as 0.10), and is not the same as the adjustment amount being passed in to this method (eg: $4.28) --- app/models/spree/tax_rate.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index bd00dbd9cc..369aabb3bf 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -138,9 +138,9 @@ module Spree private - def create_label(amount) + def create_label(adjustment_amount) label = "" - label << "#{Spree.t(:refund)} " if amount.negative? + label << "#{Spree.t(:refund)} " if adjustment_amount.negative? label << "#{(name.presence || tax_category.name)} " label << (show_rate_in_label? ? "#{amount * 100}%" : "") label << " (#{I18n.t('models.tax_rate.included_in_price')})" if included_in_price?