From e8d610f9df1d7f44d856fbef4e09c04aa2960a4c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:12:21 +0100 Subject: [PATCH 01/31] Remove dead code AdjustmentsController#enable_updates --- .../spree/admin/adjustments_controller.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index d86edaaac1..9569387c31 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -10,7 +10,6 @@ module Spree before_action :skip_changing_canceled_orders, only: [:create, :update] after_action :update_order, only: [:create, :update, :destroy] before_action :set_default_tax_rate, only: :edit - before_action :enable_updates, only: :update private @@ -81,17 +80,6 @@ module Spree params[:adjustment][:included_tax] = included_tax end - # Spree 2.0 keeps shipping fee adjustments open unless they are manually - # closed. But open adjustments cannot be edited. - # To preserve updates, like changing the amount of the shipping fee, - # we close the adjustment first. - # - # The Spree admin interface allows to open and close adjustments manually - # but we removed that functionality as it had no purpose for us. - def enable_updates - @adjustment.close - end - def permitted_resource_params params.require(:adjustment).permit( :label, :amount, :included_tax From 65eb33ad9eb7ec0f3df2fc2155048c8e8ee15c5f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:13:07 +0100 Subject: [PATCH 02/31] Only update totals and states in AdjustmentsController Avoids unnecessary updating of all other adjustments --- app/controllers/spree/admin/adjustments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index 9569387c31..3cfb5787e9 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -15,7 +15,7 @@ module Spree def update_order @order.reload - @order.update_order! + @order.updater.update_totals_and_states end def collection From 0c369b618deaef91bac1fb24d2d06b4af0221868 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:50:01 +0100 Subject: [PATCH 03/31] Remove code that guesses what the tax rate might be :tada: --- .../spree/admin/adjustments_controller.rb | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index 3cfb5787e9..10909f805f 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -9,7 +9,6 @@ module Spree before_action :set_order_id, only: [:create, :update] before_action :skip_changing_canceled_orders, only: [:create, :update] after_action :update_order, only: [:create, :update, :destroy] - before_action :set_default_tax_rate, only: :edit private @@ -42,34 +41,6 @@ module Spree redirect_to admin_order_adjustments_path(@order) if @order.canceled? end - # Choose a default tax rate to show on the edit form. The adjustment stores its included - # tax in dollars, but doesn't store the source of the tax (ie. TaxRate that generated it). - # We guess which tax rate here, choosing: - # 1. A tax rate that will compute to the same amount as the existing tax - # 2. If that's not present, the first tax rate that's valid for the current order - # When we have to go with 2, we show an error message to ask the admin to check that the - # correct tax is being applied. - def set_default_tax_rate - return if @adjustment.included_tax <= 0 - - tax_rates = TaxRate.match(@order) - tax_rate_with_matching_tax = find_tax_rate_with_matching_tax(tax_rates) - tax_rate_valid_for_order = tax_rates.first.andand.id - - @tax_rate_id = tax_rate_with_matching_tax || tax_rate_valid_for_order - - return unless tax_rate_with_matching_tax.nil? - - @adjustment.errors.add :tax_rate_id, I18n.t(:adjustments_tax_rate_error) - end - - def find_tax_rate_with_matching_tax(tax_rates) - tax_rates_yielding_matching_tax = tax_rates.select do |tr| - tr.compute_tax(@adjustment.amount) == @adjustment.included_tax - end - tax_rates_yielding_matching_tax.first.andand.id - end - def set_included_tax included_tax = 0 if params[:tax_rate_id].present? From f037bda1dec22573c1caeccf5d46aaad30eaa6f5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 15:58:59 +0100 Subject: [PATCH 04/31] Replace callback for applying tax to an admin adjustment :tada: --- .../spree/admin/adjustments_controller.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/app/controllers/spree/admin/adjustments_controller.rb b/app/controllers/spree/admin/adjustments_controller.rb index 10909f805f..55ab5efefe 100644 --- a/app/controllers/spree/admin/adjustments_controller.rb +++ b/app/controllers/spree/admin/adjustments_controller.rb @@ -5,10 +5,10 @@ module Spree class AdjustmentsController < ::Admin::ResourceController belongs_to 'spree/order', find_by: :number - prepend_before_action :set_included_tax, only: [:create, :update] before_action :set_order_id, only: [:create, :update] before_action :skip_changing_canceled_orders, only: [:create, :update] after_action :update_order, only: [:create, :update, :destroy] + after_action :apply_tax, only: [:create, :update] private @@ -41,19 +41,13 @@ module Spree redirect_to admin_order_adjustments_path(@order) if @order.canceled? end - def set_included_tax - included_tax = 0 - if params[:tax_rate_id].present? - tax_rate = TaxRate.find params[:tax_rate_id] - amount = params[:adjustment][:amount].to_f - included_tax = tax_rate.compute_tax amount - end - params[:adjustment][:included_tax] = included_tax + def apply_tax + Spree::TaxRate.adjust(@order, [@adjustment]) end def permitted_resource_params params.require(:adjustment).permit( - :label, :amount, :included_tax + :label, :amount, :tax_category_id ) end end From 18e5fd19fadc0f24f0053291c5d1a68b7e74a2c9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 16:29:10 +0100 Subject: [PATCH 05/31] Update admin adjustment edit forms --- .../admin/adjustments/_edit_form.html.haml | 18 ++++------ .../admin/adjustments/_new_form.html.haml | 15 ++++---- spec/features/admin/adjustments_spec.rb | 35 +++++++++---------- 3 files changed, 32 insertions(+), 36 deletions(-) diff --git a/app/views/spree/admin/adjustments/_edit_form.html.haml b/app/views/spree/admin/adjustments/_edit_form.html.haml index 58d2db990d..acfceb6df6 100644 --- a/app/views/spree/admin/adjustments/_edit_form.html.haml +++ b/app/views/spree/admin/adjustments/_edit_form.html.haml @@ -6,18 +6,14 @@ = f.error_message_on :amount - if @adjustment.admin? - .four.columns - = f.field_container :included_tax do - = f.label :included_tax, t(:included_tax) - = f.text_field :included_tax, disabled: true, class: 'fullwidth', - value: number_with_precision(f.object.included_tax, precision: 2) - = f.error_message_on :included_tax - .omega.four.columns - = f.field_container :tax_rate_id do - = f.label :tax_rate_id, t(:tax_rate) - = select_tag :tax_rate_id, options_from_collection_for_select(Spree::TaxRate.all, :id, :name, @tax_rate_id), prompt: t(:remove_tax), class: 'select2 fullwidth' - = f.error_message_on :tax_rate_id + = f.field_container :tax_category do + = f.label :tax_category, t(:tax_category) + = select_tag "adjustment[tax_category_id]", + options_from_collection_for_select(Spree::TaxCategory.all, :id, :name, @adjustment.tax_category_id), + prompt: t(:none), + class: 'select2 fullwidth' + = f.error_message_on :tax_category .row .alpha.omega.twelve.columns diff --git a/app/views/spree/admin/adjustments/_new_form.html.haml b/app/views/spree/admin/adjustments/_new_form.html.haml index a3e831aa8d..e049e0884a 100644 --- a/app/views/spree/admin/adjustments/_new_form.html.haml +++ b/app/views/spree/admin/adjustments/_new_form.html.haml @@ -1,15 +1,18 @@ .row - .alpha.three.columns + .alpha.four.columns = f.field_container :amount do = f.label :amount, raw(t(:amount) + content_tag(:span, " *", :class => "required")) = text_field :adjustment, :amount, :class => 'fullwidth' = f.error_message_on :amount - .omega.three.columns - = f.field_container :tax_rate_id do - = f.label :tax_rate_id, t(:tax_rate) - = select_tag :tax_rate_id, options_from_collection_for_select(Spree::TaxRate.all, :id, :name), prompt: t(:none), class: 'select2 fullwidth' - = f.error_message_on :tax_rate_id + .omega.four.columns + = f.field_container :tax_category do + = f.label :tax_category, t(:tax_category) + = select_tag "adjustment[tax_category_id]", + options_from_collection_for_select(Spree::TaxCategory.all, :id, :name), + prompt: t(:none), + class: 'select2 fullwidth' + = f.error_message_on :tax_category .row .alpha.omega.twelve.columns diff --git a/spec/features/admin/adjustments_spec.rb b/spec/features/admin/adjustments_spec.rb index 887a4bd59e..2d5ca5ded7 100644 --- a/spec/features/admin/adjustments_spec.rb +++ b/spec/features/admin/adjustments_spec.rb @@ -17,9 +17,10 @@ feature ' create(:order_with_totals_and_distribution, user: user, distributor: distributor, order_cycle: order_cycle, state: 'complete', payment_state: 'balance_due') } + let!(:tax_category) { create(:tax_category, name: 'GST') } let!(:tax_rate) { create(:tax_rate, name: 'GST', calculator: build(:calculator, preferred_amount: 10), - zone: create(:zone_with_member)) + zone: create(:zone_with_member), tax_category: tax_category) } before do @@ -37,19 +38,19 @@ feature ' click_link 'New Adjustment' fill_in 'adjustment_amount', with: 110 fill_in 'adjustment_label', with: 'Late fee' - select2_select 'GST', from: 'tax_rate_id' + select2_select 'GST', from: 'adjustment_tax_category_id' click_button 'Continue' # Then I should see the adjustment, with the correct tax expect(page).to have_selector 'td.label', text: 'Late fee' - expect(page).to have_selector 'td.amount', text: '110' - expect(page).to have_selector 'td.included-tax', text: '10' + expect(page).to have_selector 'td.amount', text: '110.00' + expect(page).to have_selector 'td.tax', text: '10.00' end scenario "modifying taxed adjustments on an order" do # Given a taxed adjustment adjustment = create(:adjustment, label: "Extra Adjustment", adjustable: order, - amount: 110, included_tax: 10, order: order) + amount: 110, tax_category: tax_category, order: order) # When I go to the adjustments page for the order login_as_admin_and_visit spree.admin_orders_path @@ -57,23 +58,21 @@ feature ' click_link 'Adjustments' page.find('tr', text: 'Extra Adjustment').find('a.icon-edit').click - # Then I should see the uneditable included tax and our tax rate as the default - expect(page).to have_field :adjustment_included_tax, with: '10.00', disabled: true - expect(page).to have_select2 :tax_rate_id, selected: 'GST' + expect(page).to have_select2 :adjustment_tax_category_id, selected: 'GST' # When I edit the adjustment, removing the tax - select2_select 'Remove tax', from: :tax_rate_id + select2_select 'None', from: :adjustment_tax_category_id click_button 'Continue' # Then the adjustment tax should be cleared - expect(page).to have_selector 'td.amount', text: '110' - expect(page).to have_selector 'td.included-tax', text: '0' + expect(page).to have_selector 'td.amount', text: '110.00' + expect(page).to have_selector 'td.tax', text: '0.00' end scenario "modifying an untaxed adjustment on an order" do # Given an untaxed adjustment adjustment = create(:adjustment, label: "Extra Adjustment", adjustable: order, - amount: 110, included_tax: 0, order: order) + amount: 110, tax_category: nil, order: order) # When I go to the adjustments page for the order login_as_admin_and_visit spree.admin_orders_path @@ -81,23 +80,21 @@ feature ' click_link 'Adjustments' page.find('tr', text: 'Extra Adjustment').find('a.icon-edit').click - # Then I should see the uneditable included tax and 'Remove tax' as the default tax rate - expect(page).to have_field :adjustment_included_tax, with: '0.00', disabled: true - expect(page).to have_select2 :tax_rate_id, selected: [] + expect(page).to have_select2 :adjustment_tax_category_id, selected: [] # When I edit the adjustment, setting a tax rate - select2_select 'GST', from: :tax_rate_id + select2_select 'GST', from: :adjustment_tax_category_id click_button 'Continue' # Then the adjustment tax should be recalculated - expect(page).to have_selector 'td.amount', text: '110' - expect(page).to have_selector 'td.included-tax', text: '10' + expect(page).to have_selector 'td.amount', text: '110.00' + expect(page).to have_selector 'td.tax', text: '10.00' end scenario "viewing adjustments on a canceled order" do # Given a taxed adjustment adjustment = create(:adjustment, label: "Extra Adjustment", adjustable: order, - amount: 110, included_tax: 10, order: order) + amount: 110, tax_category: tax_category, order: order) order.cancel! login_as_admin_and_visit spree.edit_admin_order_path(order) From 02fc3089d6719f5bd081011bf5d087f27807dbc4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 16:49:42 +0100 Subject: [PATCH 06/31] Remove dead code OrderTaxAdjustmentsFetcher#all --- app/services/order_tax_adjustments_fetcher.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index e39522c2ab..18b2406be4 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -9,7 +9,7 @@ class OrderTaxAdjustmentsFetcher end def totals - all.each_with_object({}) do |adjustment, hash| + order.all_adjustments.tax.each_with_object({}) do |adjustment, hash| tax_rates_hash = tax_rates_hash(adjustment) hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 } end @@ -19,13 +19,6 @@ class OrderTaxAdjustmentsFetcher attr_reader :order - def all - tax_adjustments = order.all_adjustments.tax - admin_adjustments_with_tax = order.all_adjustments.admin.with_tax - - tax_adjustments.or(admin_adjustments_with_tax) - end - def tax_rates_hash(adjustment) tax_rates = TaxRateFinder.tax_rates_of(adjustment) From ea9724a671e354cd4438e7dac1ef33e781a4e45e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 16:51:27 +0100 Subject: [PATCH 07/31] Remove dead code #adjustment_tax_amount and #no_tax_adjustments? --- app/services/order_tax_adjustments_fetcher.rb | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 18b2406be4..a4488c967d 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -24,25 +24,11 @@ class OrderTaxAdjustmentsFetcher Hash[tax_rates.collect do |tax_rate| tax_amount = if tax_rates.one? - adjustment_tax_amount(adjustment) + adjustment.amount else tax_rate.compute_tax(adjustment.amount) end [tax_rate, tax_amount] end] end - - def adjustment_tax_amount(adjustment) - if no_tax_adjustments?(adjustment) - adjustment.included_tax - else - adjustment.amount - end - end - - def no_tax_adjustments?(adjustment) - # Admin Adjustments currently do not have tax adjustments. - # The tax amount is stored in the included_tax attribute. - adjustment.originator_type.nil? - end end From 9215d3292b2bae410d7122e430ef37927a9b57e6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 17:31:58 +0100 Subject: [PATCH 08/31] Remove edit/delete buttons for shipping and payment adjustments --- .../admin/adjustments/_adjustments_table.html.haml | 5 +++-- spec/features/admin/order_spec.rb | 13 ++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/views/spree/admin/adjustments/_adjustments_table.html.haml b/app/views/spree/admin/adjustments/_adjustments_table.html.haml index cbc7df005b..f139608f61 100644 --- a/app/views/spree/admin/adjustments/_adjustments_table.html.haml +++ b/app/views/spree/admin/adjustments/_adjustments_table.html.haml @@ -19,5 +19,6 @@ %td.align-center.included-tax= adjustment.display_included_tax.to_html - unless @order.canceled? %td.actions - = link_to_edit adjustment, no_text: true - = link_to_delete adjustment, no_text: true + - if adjustment.originator_type.nil? + = link_to_edit adjustment, no_text: true + = link_to_delete adjustment, no_text: true diff --git a/spec/features/admin/order_spec.rb b/spec/features/admin/order_spec.rb index 8a9404f870..6a97135dea 100644 --- a/spec/features/admin/order_spec.rb +++ b/spec/features/admin/order_spec.rb @@ -409,15 +409,14 @@ feature ' expect(page).to have_content test_tracking_number end - scenario "editing shipping fees" do + scenario "viewing shipping fees" do + shipping_fee = order.shipment_adjustments.first + click_link "Adjustments" - shipping_adjustment_tr_selector = "tr#spree_adjustment_#{order.shipment_adjustments.first.id}" - page.find("#{shipping_adjustment_tr_selector} td.actions a.icon-edit").click - fill_in "Amount", with: "5" - click_button "Continue" - - expect(page.find("#{shipping_adjustment_tr_selector} td.amount")).to have_content "5.00" + expect(page).to have_selector "tr#spree_adjustment_#{shipping_fee.id}" + expect(page).to have_selector 'td.amount', text: shipping_fee.amount.to_s + expect(page).to have_selector 'td.tax', text: shipping_fee.included_tax_total.to_s end context "when an included variant has been deleted" do From c16e008ba1b7335a7575551cf1d43a938fb833e5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 17:34:35 +0100 Subject: [PATCH 09/31] Delete dead code Adjustment#set_absolute_included_tax! --- app/models/spree/adjustment.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 20449e1213..2494d372c9 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -129,10 +129,6 @@ module Spree state != "open" end - def set_absolute_included_tax!(tax) - update! included_tax: tax.round(2) - end - def display_included_tax Spree::Money.new(included_tax, currency: currency) end From ad1b9f3f2f8cdeeab4bc5e9c317f3eaa4cad3b36 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 17:38:27 +0100 Subject: [PATCH 10/31] Update Adjustment#has_tax? --- app/models/spree/adjustment.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 2494d372c9..4f3ab3da96 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -134,11 +134,15 @@ module Spree end def has_tax? - included_tax.positive? + tax_total.positive? end private + def tax_total + adjustments.tax.sum(:amount) + end + def update_adjustable_adjustment_total Spree::ItemAdjustments.new(adjustable).update if adjustable end From c60554a14a0a0b218e1ed4f37ccd2e6edc2f5401 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 18:13:56 +0100 Subject: [PATCH 11/31] Update display of associated tax amounts --- app/controllers/application_controller.rb | 1 + app/helpers/adjustments_helper.rb | 14 ++++++++++++++ app/models/spree/adjustment.rb | 8 ++++++++ .../admin/adjustments/_adjustments_table.html.haml | 14 +++++++++----- .../spree/admin/orders/_invoice_table.html.haml | 2 +- config/locales/en.yml | 2 ++ 6 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 app/helpers/adjustments_helper.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 077a264695..4570ed53cf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -16,6 +16,7 @@ class ApplicationController < ActionController::Base helper 'spree/orders' helper 'spree/payment_methods' helper 'shared' + helper 'adjustments' helper 'enterprises' helper 'order_cycles' helper 'order' diff --git a/app/helpers/adjustments_helper.rb b/app/helpers/adjustments_helper.rb new file mode 100644 index 0000000000..775f5b6d42 --- /dev/null +++ b/app/helpers/adjustments_helper.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module AdjustmentsHelper + def display_adjustment_taxes(adjustment) + if adjustment.included_tax_total > 0 + amount = Spree::Money.new(adjustment.included_tax_total, currency: adjustment.currency) + I18n.t(:tax_amount_included, amount: amount) + elsif adjustment.additional_tax_total > 0 + Spree::Money.new(adjustment.additional_tax_total, currency: adjustment.currency) + else + Spree::Money.new(0.00, currency: adjustment.currency) + end + end +end diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 4f3ab3da96..38b6d845c5 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -137,6 +137,14 @@ module Spree tax_total.positive? end + def included_tax_total + adjustments.tax.inclusive.sum(:amount) + end + + def additional_tax_total + adjustments.tax.additional.sum(:amount) + end + private def tax_total diff --git a/app/views/spree/admin/adjustments/_adjustments_table.html.haml b/app/views/spree/admin/adjustments/_adjustments_table.html.haml index f139608f61..2d67f6e4e1 100644 --- a/app/views/spree/admin/adjustments/_adjustments_table.html.haml +++ b/app/views/spree/admin/adjustments/_adjustments_table.html.haml @@ -4,7 +4,7 @@ %th= "#{t('spree.date')}/#{t('spree.time')}" %th= t(:description) %th= t(:amount) - %th= t(:included_tax) + %th= t(:tax) %th.actions %tbody - @collection.each do |adjustment| @@ -13,10 +13,14 @@ - tr_class = cycle('odd', 'even') - tr_id = spree_dom_id(adjustment) %tr{:class => tr_class, "data-hook" => "adjustment_row", :id => tr_id} - %td.align-center.created_at= pretty_time(adjustment.created_at) - %td.align-center.label= adjustment.label - %td.align-center.amount= adjustment.display_amount.to_html - %td.align-center.included-tax= adjustment.display_included_tax.to_html + %td.align-center.created_at + = pretty_time(adjustment.created_at) + %td.align-center.label + = adjustment.label + %td.align-center.amount + = adjustment.display_amount.to_html + %td.align-center.tax + = display_adjustment_taxes(adjustment) - unless @order.canceled? %td.actions - if adjustment.originator_type.nil? diff --git a/app/views/spree/admin/orders/_invoice_table.html.haml b/app/views/spree/admin/orders/_invoice_table.html.haml index c96099edb6..878724659a 100644 --- a/app/views/spree/admin/orders/_invoice_table.html.haml +++ b/app/views/spree/admin/orders/_invoice_table.html.haml @@ -30,7 +30,7 @@ %td{:align => "right"} 1 %td{:align => "right"} - = adjustment.included_tax > 0 ? adjustment.display_included_tax : "" + = display_adjustment_taxes(adjustment) %td{:align => "right"} = adjustment.display_amount %tfoot diff --git a/config/locales/en.yml b/config/locales/en.yml index 84140e72bc..549806905e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2162,6 +2162,8 @@ See the %{link} to find out more about %{sitename}'s features and to start using fundraising_fee: "Fundraising fee" price_graph: "Price graph" included_tax: "Included tax" + tax: "Tax" + tax_amount_included: "%{amount} (included)" remove_tax: "Remove tax" balance: "Balance" transaction: "Transaction" From b3f5780862d9cc320c194a2573ace10953932228 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:28:01 +0100 Subject: [PATCH 12/31] Refactor OrderTaxAdjustmentsFetcher Note: we're only dealing with tax adjustments here, and each adjustment has a single associated tax rate, not multiple. --- app/services/order_tax_adjustments_fetcher.rb | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index a4488c967d..424bdb100a 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true -# This class will be used to get Tax Adjustments related to an order, -# and proceed basic calcultation over them. +# Collects Tax Adjustments related to an order, and returns a hash with a total for each rate. class OrderTaxAdjustmentsFetcher def initialize(order) @@ -10,25 +9,13 @@ class OrderTaxAdjustmentsFetcher def totals order.all_adjustments.tax.each_with_object({}) do |adjustment, hash| - tax_rates_hash = tax_rates_hash(adjustment) - hash.update(tax_rates_hash) { |_tax_rate, amount1, amount2| amount1 + amount2 } + tax_rate = adjustment.originator + tax_amounts = { tax_rate => adjustment.amount } + hash.update(tax_amounts) { |_tax_rate, amount1, amount2| amount1 + amount2 } end end private attr_reader :order - - def tax_rates_hash(adjustment) - tax_rates = TaxRateFinder.tax_rates_of(adjustment) - - Hash[tax_rates.collect do |tax_rate| - tax_amount = if tax_rates.one? - adjustment.amount - else - tax_rate.compute_tax(adjustment.amount) - end - [tax_rate, tax_amount] - end] - end end From 33fc5bbaa252b1d0329bae8ab478092c31d879c1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:29:42 +0100 Subject: [PATCH 13/31] Delete dead code TaxRate#compute_tax :tada: --- app/models/spree/tax_rate.rb | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index f0a7be6e11..6a60d9e988 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -117,25 +117,6 @@ module Spree Zone.default_tax&.contains?(order.tax_zone) || order.tax_zone == zone end - # Manually apply a TaxRate to a particular amount. TaxRates normally compute against - # LineItems or Orders, so we mock out a line item here to fit the interface - # that our calculator (usually DefaultTax) expects. - def compute_tax(amount) - line_item = LineItem.new quantity: 1 - line_item.tax_category = tax_category - line_item.define_singleton_method(:price) { amount } - - # Tax on adjustments (represented by the included_tax field) is always inclusive of - # tax. However, there's nothing to stop an admin from setting one up with a tax rate - # that's marked as not inclusive of tax, and that would result in the DefaultTax - # calculator generating a slightly incorrect value. Therefore, we treat the tax - # rate as inclusive of tax for the calculations below, regardless of its original - # setting. - with_tax_included_in_price do - calculator.compute line_item - end - end - private def create_label(adjustment_amount) From f2e63fff2ea1b3e4f9ac78ceaa180f8f332df3ce Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:30:45 +0100 Subject: [PATCH 14/31] Delete dead code TaxRate: #compute_tax and #with_tax_included_in_price :tada: --- app/models/spree/tax_rate.rb | 14 ----------- spec/models/spree/tax_rate_spec.rb | 39 ------------------------------ 2 files changed, 53 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index 6a60d9e988..092ce33aa6 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -127,19 +127,5 @@ module Spree label << " (#{I18n.t('models.tax_rate.included_in_price')})" if included_in_price? label end - - def with_tax_included_in_price - old_included_in_price = included_in_price - - self.included_in_price = true - calculator.calculable.included_in_price = true - - result = yield - ensure - self.included_in_price = old_included_in_price - calculator.calculable.included_in_price = old_included_in_price - - result - end end end diff --git a/spec/models/spree/tax_rate_spec.rb b/spec/models/spree/tax_rate_spec.rb index bf50676f29..34877fade4 100644 --- a/spec/models/spree/tax_rate_spec.rb +++ b/spec/models/spree/tax_rate_spec.rb @@ -37,45 +37,6 @@ module Spree end end - describe "ensuring that tax rate is marked as tax included_in_price" do - let(:tax_rate) { - create(:tax_rate, included_in_price: false, calculator: ::Calculator::DefaultTax.new) - } - - it "sets included_in_price to true" do - tax_rate.send(:with_tax_included_in_price) do - expect(tax_rate.included_in_price).to be true - end - end - - it "sets the included_in_price value accessible to the calculator to true" do - tax_rate.send(:with_tax_included_in_price) do - expect(tax_rate.calculator.calculable.included_in_price).to be true - end - end - - it "passes through the return value of the block" do - expect(tax_rate.send(:with_tax_included_in_price) do - 'asdf' - end).to eq('asdf') - end - - it "restores both values to their original afterwards" do - tax_rate.send(:with_tax_included_in_price) {} - expect(tax_rate.included_in_price).to be false - expect(tax_rate.calculator.calculable.included_in_price).to be false - end - - it "restores both values when an exception is raised" do - expect do - tax_rate.send(:with_tax_included_in_price) { raise StandardError, 'oops' } - end.to raise_error 'oops' - - expect(tax_rate.included_in_price).to be false - 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) } From 0876f2ae0d307d94893a0bc72f6c58202ba61870 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 19:37:33 +0100 Subject: [PATCH 15/31] Remove tax rate guessing from TaxRateFinder :tada: --- app/services/tax_rate_finder.rb | 42 ++---------------------- spec/services/tax_rate_finder_spec.rb | 46 ++++++--------------------- 2 files changed, 12 insertions(+), 76 deletions(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index 59d24dd052..a1b61bb406 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -8,16 +8,13 @@ class TaxRateFinder def self.tax_rates_of(adjustment) new.tax_rates( adjustment.originator, - adjustment.adjustable, - adjustment.amount, - adjustment.included_tax + adjustment.adjustable ) end # @return [Array] - def tax_rates(originator, adjustable, amount, included_tax) - find_associated_tax_rate(originator, adjustable) || - find_closest_tax_rates_from_included_tax(amount, included_tax) + def tax_rates(originator, adjustable) + find_associated_tax_rate(originator, adjustable) end private @@ -48,37 +45,4 @@ class TaxRateFinder enterprise_fee.tax_category end end - - # There are two cases in which a line item is not associated to a tax rate. - # - # 1. Shipping fees and adjustments created from the admin panel have taxes set - # at creation in the included_tax field without relation to the - # corresponding TaxRate. - # 2. Removing line items from an order doesn't always remove the associated - # enterprise fees. These orphaned fees don't have a line item any more to - # find the item's tax rate. - # - # In these cases we try to find the used tax rate based on the included tax. - # For example, if the included tax is 10% of the adjustment, we look for a tax - # rate of 10%. Due to rounding errors, the included tax may be 9.9% of the - # adjustment. That's why we call it an approximation of the tax rate and look - # for the closest and hopefully find the 10% tax rate. - # - # This attempt can fail. - # - # - If an admin created an adjustment with a miscalculated included tax then - # we don't know which tax rate the admin intended to use. - # - An admin may also enter included tax that doesn't correspond to any tax - # rate in the system. They may enter a fee of $1.2 with tax of $0.2, but - # that doesn't mean that there is a 20% tax rate in the database. - # - The used tax rate may also have been deleted. Maybe the tax law changed. - # - # In either of these cases, we will find a tax rate that doesn't correspond - # to the included tax. - def find_closest_tax_rates_from_included_tax(amount, included_tax) - approximation = (included_tax / (amount - included_tax)) - return [] if approximation.infinite? || approximation.zero? || approximation.nan? - - [Spree::TaxRate.order(Arel.sql("ABS(amount - #{approximation})")).first] - end end diff --git a/spec/services/tax_rate_finder_spec.rb b/spec/services/tax_rate_finder_spec.rb index b7bb07db73..1c1abe425a 100644 --- a/spec/services/tax_rate_finder_spec.rb +++ b/spec/services/tax_rate_finder_spec.rb @@ -5,61 +5,33 @@ require 'spec_helper' describe TaxRateFinder do describe "getting the corresponding tax rate" do let(:amount) { BigDecimal(120) } - let(:included_tax) { BigDecimal(20) } let(:tax_rate) { create_rate(0.2) } let(:tax_category) { create(:tax_category, tax_rates: [tax_rate]) } let(:zone) { create(:zone_with_member) } let(:shipment) { create(:shipment) } + let(:line_item) { create(:line_item) } let(:enterprise_fee) { create(:enterprise_fee, tax_category: tax_category) } let(:order) { create(:order_with_taxes, zone: zone) } it "finds the tax rate of a shipping fee" do - rates = TaxRateFinder.new.tax_rates( - tax_rate, - shipment, - amount, - included_tax - ) + rates = TaxRateFinder.new.tax_rates(tax_rate, shipment) expect(rates).to eq [tax_rate] end - it "finds a close match" do + it "deals with soft-deleted tax rates" do tax_rate.destroy - close_tax_rate = create_rate(tax_rate.amount + 0.05) - # other tax rates, not as close to the real one - create_rate(tax_rate.amount + 0.06) - create_rate(tax_rate.amount - 0.06) - - rates = TaxRateFinder.new.tax_rates( - nil, - shipment, - amount, - included_tax - ) - - expect(rates).to eq [close_tax_rate] + rates = TaxRateFinder.new.tax_rates(tax_rate, shipment) + expect(rates).to eq [tax_rate] end it "finds the tax rate of an enterprise fee" do - rates = TaxRateFinder.new.tax_rates( - enterprise_fee, - order, - amount, - included_tax - ) + rates = TaxRateFinder.new.tax_rates(enterprise_fee, order) expect(rates).to eq [tax_rate] end - # There is a bug that leaves orphan adjustments on an order after - # associated line items have been removed. - # https://github.com/openfoodfoundation/openfoodnetwork/issues/3127 - it "deals with a missing line item" do - rates = TaxRateFinder.new.tax_rates( - enterprise_fee, - nil, - amount, - included_tax - ) + it "deals with a soft-deleted line item" do + line_item.destroy + rates = TaxRateFinder.new.tax_rates(enterprise_fee, line_item) expect(rates).to eq [tax_rate] end From 12c9914d1b2af4156b35c6036ff9209c5f862a88 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 22:12:42 +0100 Subject: [PATCH 16/31] Delete old Adjustment scopes #with_tax and #without_tax --- app/models/spree/adjustment.rb | 2 -- lib/open_food_network/xero_invoices_report.rb | 4 +-- spec/features/admin/reports_spec.rb | 10 +++---- spec/models/spree/adjustment_spec.rb | 27 ------------------- 4 files changed, 7 insertions(+), 36 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 38b6d845c5..d19f7bb30e 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -77,8 +77,6 @@ module Spree scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } scope :admin, -> { where(originator_type: nil) } - scope :with_tax, -> { where('spree_adjustments.included_tax <> 0') } - scope :without_tax, -> { where('spree_adjustments.included_tax = 0') } scope :payment_fee, -> { where(AdjustmentScopes::PAYMENT_FEE_SCOPE) } scope :shipping, -> { where(AdjustmentScopes::SHIPPING_SCOPE) } scope :eligible, -> { where(AdjustmentScopes::ELIGIBLE_SCOPE) } diff --git a/lib/open_food_network/xero_invoices_report.rb b/lib/open_food_network/xero_invoices_report.rb index 2207ee2dc3..1a6e2b187c 100644 --- a/lib/open_food_network/xero_invoices_report.rb +++ b/lib/open_food_network/xero_invoices_report.rb @@ -219,11 +219,11 @@ module OpenFoodNetwork end def total_untaxable_admin_adjustments(order) - order.adjustments.admin.without_tax.sum(:amount) + order.adjustments.admin.where(tax_category: nil).sum(:amount) end def total_taxable_admin_adjustments(order) - order.adjustments.admin.with_tax.sum(:amount) + order.adjustments.admin.where.not(tax_category: nil).sum(:amount) end def detail? diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index a11c88e451..723754cc56 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -504,13 +504,13 @@ feature ' create(:adjustment, order: order1, adjustable: adj_fee2, originator: tax_rate, amount: 3, state: "closed") } - let!(:adj_manual1) { + let!(:adj_admin1) { create(:adjustment, order: order1, adjustable: order1, originator: nil, label: "Manual adjustment", amount: 30) } - let!(:adj_manual2) { + let!(:adj_admin2) { create(:adjustment, order: order1, adjustable: order1, originator: nil, - label: "Manual adjustment", amount: 40, included_tax: 3) + label: "Manual adjustment", amount: 40, tax_category: tax_category) } before do @@ -590,8 +590,8 @@ feature ' xero_invoice_header, xero_invoice_li_row(line_item1), xero_invoice_li_row(line_item2), - xero_invoice_adjustment_row(adj_manual1), - xero_invoice_adjustment_row(adj_manual2), + xero_invoice_adjustment_row(adj_admin1), + xero_invoice_adjustment_row(adj_admin2), xero_invoice_summary_row('Total untaxable fees (no tax)', 10.0, 'GST Free Income', opts), xero_invoice_summary_row('Total taxable fees (tax inclusive)', 20.0, diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 4ee57d8b77..7ae26c83c6 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -148,33 +148,6 @@ module Spree expect(adjustment.metadata).to be end - describe "querying included tax" do - let!(:adjustment_with_tax) { create(:adjustment, included_tax: 123) } - let!(:adjustment_without_tax) { create(:adjustment, included_tax: 0) } - - describe "finding adjustments with and without tax included" do - it "finds adjustments with tax" do - expect(Adjustment.with_tax).to include adjustment_with_tax - expect(Adjustment.with_tax).not_to include adjustment_without_tax - end - - it "finds adjustments without tax" do - expect(Adjustment.without_tax).to include adjustment_without_tax - expect(Adjustment.without_tax).not_to include adjustment_with_tax - end - end - - describe "checking if an adjustment includes tax" do - it "returns true when it has > 0 tax" do - expect(adjustment_with_tax).to have_tax - end - - it "returns false when it has 0 tax" do - expect(adjustment_without_tax).not_to have_tax - end - end - end - describe "recording included tax" do describe "TaxRate adjustments" do let!(:zone) { create(:zone_with_member) } From 3ae31ec1ce7e73c811d54aa2aac730e05357fab5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 22:24:36 +0100 Subject: [PATCH 17/31] Simplify tax adjustment sums in Order::Updater --- .../app/services/order_management/order/updater.rb | 3 +-- .../spec/services/order_management/order/updater_spec.rb | 3 +-- 2 files changed, 2 insertions(+), 4 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 684ca7b9ac..e17a9e3ad6 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -67,8 +67,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 = all_adjustments.tax.inclusive.sum(:amount) + - adjustments.admin.sum(:included_tax) + order.included_tax_total = all_adjustments.tax.inclusive.sum(:amount) end def update_order_total 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 b18ce59912..76ab5ec521 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 @@ -34,12 +34,11 @@ module OrderManagement :sum).and_return(20) 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 expect(order.adjustment_total).to eq(-5) expect(order.additional_tax_total).to eq(20) - expect(order.included_tax_total).to eq(17) + expect(order.included_tax_total).to eq(15) end end From cb039fd880e9f5402823b1d11befcb179aeda7c3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 16 Jun 2021 22:28:51 +0100 Subject: [PATCH 18/31] Delete dead code Adjustment#display_included_tax --- app/models/spree/adjustment.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index d19f7bb30e..8312229736 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -127,10 +127,6 @@ module Spree state != "open" end - def display_included_tax - Spree::Money.new(included_tax, currency: currency) - end - def has_tax? tax_total.positive? end From fe92c31f4a4a7c96eeea284f0e710c26c0e849ad Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 17 Jun 2021 17:45:23 +0100 Subject: [PATCH 19/31] Update AdjustmentsController specs --- .../admin/adjustments_controller_spec.rb | 131 ++++++++++++------ 1 file changed, 85 insertions(+), 46 deletions(-) diff --git a/spec/controllers/spree/admin/adjustments_controller_spec.rb b/spec/controllers/spree/admin/adjustments_controller_spec.rb index b8e1a2bb13..1e6fe5f8a5 100644 --- a/spec/controllers/spree/admin/adjustments_controller_spec.rb +++ b/spec/controllers/spree/admin/adjustments_controller_spec.rb @@ -43,71 +43,110 @@ module Spree end end - describe "setting included tax" do + describe "setting the adjustment's tax" do let(:order) { create(:order) } - let(:tax_rate) { create(:tax_rate, amount: 0.1, calculator: ::Calculator::DefaultTax.new) } + let(:zone) { create(:zone_with_member) } + let(:tax_rate) { create(:tax_rate, amount: 0.1, zone: zone, included_in_price: true ) } describe "creating an adjustment" do - it "sets included tax to zero when no tax rate is specified" do - spree_post :create, order_id: order.number, - adjustment: { label: 'Testing included tax', amount: '110' }, tax_rate_id: '' - expect(response).to redirect_to spree.admin_order_adjustments_path(order) + let(:tax_category_param) { '' } + let(:params) { + { + order_id: order.number, + adjustment: { + label: 'Testing included tax', amount: '110', tax_category_id: tax_category_param + } + } + } - a = Adjustment.last - expect(a.label).to eq('Testing included tax') - expect(a.amount).to eq(110) - expect(a.included_tax).to eq(0) - expect(a.order_id).to eq(order.id) + context "when no tax category is specified" do + it "doesn't apply tax" do + spree_post :create, params + expect(response).to redirect_to spree.admin_order_adjustments_path(order) - expect(order.reload.total).to eq 110 + new_adjustment = Adjustment.admin.last + + expect(new_adjustment.label).to eq('Testing included tax') + expect(new_adjustment.amount).to eq(110) + expect(new_adjustment.tax_category).to be_nil + expect(new_adjustment.order_id).to eq(order.id) + + expect(order.reload.total).to eq 110 + expect(order.included_tax_total).to eq 0 + end end - it "calculates included tax when a tax rate is provided" do - spree_post :create, order_id: order.number, - adjustment: { label: 'Testing included tax', amount: '110' }, tax_rate_id: tax_rate.id.to_s - expect(response).to redirect_to spree.admin_order_adjustments_path(order) + context "when a tax category is provided" do + let(:tax_category_param) { tax_rate.tax_category.id.to_s } - a = Adjustment.last - expect(a.label).to eq('Testing included tax') - expect(a.amount).to eq(110) - expect(a.included_tax).to eq(10) - expect(a.order_id).to eq(order.id) + it "applies tax" do + spree_post :create, params + expect(response).to redirect_to spree.admin_order_adjustments_path(order) - expect(order.reload.total).to eq 110 + new_adjustment = Adjustment.admin.last + + expect(new_adjustment.label).to eq('Testing included tax') + expect(new_adjustment.amount).to eq(110) + expect(new_adjustment.tax_category).to eq tax_rate.tax_category + expect(new_adjustment.order_id).to eq(order.id) + + expect(order.reload.total).to eq 110 + expect(order.included_tax_total).to eq 10 + end end end describe "updating an adjustment" do + let(:old_tax_category) { create(:tax_category) } + let(:tax_category_param) { '' } + let(:params) { + { + id: adjustment.id, + order_id: order.number, + adjustment: { + label: 'Testing included tax', amount: '110', tax_category_id: tax_category_param + } + } + } let(:adjustment) { - create(:adjustment, adjustable: order, order: order, amount: 1100, included_tax: 100) + create(:adjustment, adjustable: order, order: order, + amount: 1100, tax_category: old_tax_category) } - it "sets included tax to zero when no tax rate is specified" do - spree_put :update, order_id: order.number, id: adjustment.id, - adjustment: { label: 'Testing included tax', amount: '110' }, tax_rate_id: '' - expect(response).to redirect_to spree.admin_order_adjustments_path(order) + context "when no tax category is specified" do + it "doesn't apply tax" do + spree_put :update, params + expect(response).to redirect_to spree.admin_order_adjustments_path(order) - a = Adjustment.last - expect(a.label).to eq('Testing included tax') - expect(a.amount).to eq(110) - expect(a.included_tax).to eq(0) - expect(a.order_id).to eq(order.id) + adjustment = Adjustment.admin.last - expect(order.reload.total).to eq 110 + expect(adjustment.label).to eq('Testing included tax') + expect(adjustment.amount).to eq(110) + expect(adjustment.tax_category).to be_nil + expect(adjustment.order_id).to eq(order.id) + + expect(order.reload.total).to eq 110 + expect(order.included_tax_total).to eq 0 + end end - it "calculates included tax when a tax rate is provided" do - spree_put :update, order_id: order.number, id: adjustment.id, - adjustment: { label: 'Testing included tax', amount: '110' }, tax_rate_id: tax_rate.id.to_s - expect(response).to redirect_to spree.admin_order_adjustments_path(order) + context "when a tax category is provided" do + let(:tax_category_param) { tax_rate.tax_category.id.to_s } - a = Adjustment.last - expect(a.label).to eq('Testing included tax') - expect(a.amount).to eq(110) - expect(a.included_tax).to eq(10) - expect(a.order_id).to eq(order.id) + it "applies tax" do + spree_put :update, params + expect(response).to redirect_to spree.admin_order_adjustments_path(order) - expect(order.reload.total).to eq 110 + adjustment = Adjustment.admin.last + + expect(adjustment.label).to eq('Testing included tax') + expect(adjustment.amount).to eq(110) + expect(adjustment.tax_category).to eq tax_rate.tax_category + expect(adjustment.order_id).to eq(order.id) + + expect(order.reload.total).to eq 110 + expect(order.included_tax_total).to eq 10 + end end end end @@ -151,7 +190,7 @@ module Spree let(:order) { create(:completed_order_with_totals) } let(:tax_rate) { create(:tax_rate, amount: 0.1, calculator: ::Calculator::DefaultTax.new) } let(:adjustment) { - create(:adjustment, adjustable: order, order: order, amount: 1100, included_tax: 100) + create(:adjustment, adjustable: order, order: order, amount: 1100) } before do @@ -161,7 +200,7 @@ module Spree it "doesn't create adjustments" do expect { spree_post :create, order_id: order.number, - adjustment: { label: "Testing", amount: "110" }, tax_rate_id: "" + adjustment: { label: "Testing", amount: "110" } }.to_not change { [Adjustment.count, order.reload.total] } expect(response).to redirect_to spree.admin_order_adjustments_path(order) @@ -170,7 +209,7 @@ module Spree it "doesn't change adjustments" do expect { spree_put :update, order_id: order.number, id: adjustment.id, - adjustment: { label: "Testing", amount: "110" }, tax_rate_id: "" + adjustment: { label: "Testing", amount: "110" } }.to_not change { [adjustment.reload.amount, order.reload.total] } expect(response).to redirect_to spree.admin_order_adjustments_path(order) From 200dde18e85437f81937845c9f29f6e296e77de5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 17 Jun 2021 19:00:22 +0100 Subject: [PATCH 20/31] Update admin adjustment taxes in OrderAdjustmentsFetcher spec --- spec/services/order_tax_adjustments_fetcher_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb index 0f0d1055b5..b8e9bb7470 100644 --- a/spec/services/order_tax_adjustments_fetcher_spec.rb +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -52,8 +52,10 @@ describe OrderTaxAdjustmentsFetcher do calculator: Calculator::FlatRate.new(preferred_amount: 48.0)) end let(:admin_adjustment) do - create(:adjustment, order: order, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0), - label: "Admin Adjustment") + create(:adjustment, order: order, amount: 50.0, tax_category: tax_category25, + label: "Admin Adjustment").tap do |adjustment| + Spree::TaxRate.adjust(order, [adjustment]) + end end let(:order_cycle) do From 77e4a6c46530b0cf8797ba6eea1c688535866988 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 18 Jun 2021 11:13:11 +0100 Subject: [PATCH 21/31] Migrate admin adjustment tax amounts --- ...0210617203927_migrate_admin_tax_amounts.rb | 52 ++++++++++ db/schema.rb | 2 +- .../migrate_admin_tax_amounts_spec.rb | 94 +++++++++++++++++++ 3 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20210617203927_migrate_admin_tax_amounts.rb create mode 100644 spec/migrations/migrate_admin_tax_amounts_spec.rb diff --git a/db/migrate/20210617203927_migrate_admin_tax_amounts.rb b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb new file mode 100644 index 0000000000..a91765f4d4 --- /dev/null +++ b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb @@ -0,0 +1,52 @@ +class MigrateAdminTaxAmounts < ActiveRecord::Migration[6.0] + class Spree::Adjustment < ApplicationRecord + belongs_to :originator, 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 :admin, -> { where(originator_type: nil) } + end + + def up + migrate_admin_taxes! + end + + def migrate_admin_taxes! + Spree::Adjustment.admin.where('included_tax <> 0').find_each do |adjustment| + + tax_rate = find_tax_rate(adjustment.amount, adjustment.included_tax) + tax_category = tax_rate&.tax_category + label = tax_adjustment_label(tax_rate) + + adjustment.update_columns(tax_category_id: tax_category.id) if tax_category.present? + + Spree::Adjustment.create!( + label: label, + amount: adjustment.included_tax, + order_id: adjustment.order_id, + adjustable: adjustment, + originator_type: "Spree::TaxRate", + originator_id: tax_rate&.id, + state: "closed", + included: true + ) + end + end + + def find_tax_rate(amount, included_tax) + approximation = (included_tax / (amount - included_tax)) + return if approximation.infinite? || approximation.zero? || approximation.nan? + + Spree::TaxRate.order(Arel.sql("ABS(amount - #{approximation})")).first + 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 +end diff --git a/db/schema.rb b/db/schema.rb index 0197f4e382..ba2b22c1e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_05_27_201938) do +ActiveRecord::Schema.define(version: 2021_06_17_203927) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/migrate_admin_tax_amounts_spec.rb b/spec/migrations/migrate_admin_tax_amounts_spec.rb new file mode 100644 index 0000000000..d069e6a087 --- /dev/null +++ b/spec/migrations/migrate_admin_tax_amounts_spec.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative '../../db/migrate/20210617203927_migrate_admin_tax_amounts' + +describe MigrateAdminTaxAmounts do + subject { MigrateAdminTaxAmounts.new } + + let(:tax_category10) { create(:tax_category) } + let(:tax_category50) { create(:tax_category) } + let!(:tax_rate10) { create(:tax_rate, amount: 0.1, tax_category: tax_category10) } + let!(:tax_rate50) { create(:tax_rate, amount: 0.5, tax_category: tax_category50) } + let(:adjustment10) { create(:adjustment, amount: 100, included_tax: 10) } + let(:adjustment50) { create(:adjustment, amount: 100, included_tax: 50) } + + describe '#migrate_admin_taxes!' do + context "when the adjustment has no tax" do + let!(:adjustment_without_tax) { create(:adjustment, included_tax: 0) } + + it "doesn't move the tax to an adjustment" do + expect(Spree::Adjustment).to_not receive(:create!) + + subject.migrate_admin_taxes! + end + end + + context "when the adjustments have tax" do + before { adjustment10; adjustment50 } + + it "moves the tax to an adjustment" do + expect(Spree::Adjustment).to receive(:create!).at_least(:once).and_call_original + + subject.migrate_admin_taxes! + expect(adjustment10.reload.tax_category).to eq tax_category10 + expect(adjustment50.reload.tax_category).to eq tax_category50 + + tax_adjustment10 = Spree::Adjustment.tax.where(adjustable_id: adjustment10).first + + expect(tax_adjustment10.amount).to eq adjustment10.included_tax + expect(tax_adjustment10.adjustable).to eq adjustment10 + expect(tax_adjustment10.originator).to eq tax_rate10 + expect(tax_adjustment10.state).to eq "closed" + expect(tax_adjustment10.included).to eq true + + tax_adjustment50 = Spree::Adjustment.tax.where(adjustable_id: adjustment50).first + + expect(tax_adjustment50.amount).to eq adjustment50.included_tax + expect(tax_adjustment50.adjustable).to eq adjustment50 + expect(tax_adjustment50.originator).to eq tax_rate50 + expect(tax_adjustment50.state).to eq "closed" + expect(tax_adjustment50.included).to eq true + end + end + end + + describe "#find_tax_rate" do + it "matches rates correctly" do + expect( + subject.find_tax_rate(adjustment10.amount, adjustment10.included_tax) + ).to eq(tax_rate10) + + expect( + subject.find_tax_rate(adjustment50.amount, adjustment50.included_tax) + ).to eq(tax_rate50) + end + + context "without a perfect match" do + let(:adjustment45) { create(:adjustment, amount: 100, included_tax: 45) } + + it "finds the closest match" do + expect( + subject.find_tax_rate(adjustment45.amount, adjustment45.included_tax) + ).to eq(tax_rate50) + 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 5da545c751710166678aa521b7240a94dc845b53 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 18 Jun 2021 12:24:36 +0100 Subject: [PATCH 22/31] Improve adjustment listing in UI --- app/helpers/adjustments_helper.rb | 5 +++++ .../spree/admin/adjustments/_adjustments_table.html.haml | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/helpers/adjustments_helper.rb b/app/helpers/adjustments_helper.rb index 775f5b6d42..7b7206ca2c 100644 --- a/app/helpers/adjustments_helper.rb +++ b/app/helpers/adjustments_helper.rb @@ -11,4 +11,9 @@ module AdjustmentsHelper Spree::Money.new(0.00, currency: adjustment.currency) end end + + def display_adjustment_total_with_tax(adjustment) + total = adjustment.amount + adjustment.additional_tax_total + Spree::Money.new(total, currency: adjustment.currency) + end end diff --git a/app/views/spree/admin/adjustments/_adjustments_table.html.haml b/app/views/spree/admin/adjustments/_adjustments_table.html.haml index 2d67f6e4e1..b24f650de7 100644 --- a/app/views/spree/admin/adjustments/_adjustments_table.html.haml +++ b/app/views/spree/admin/adjustments/_adjustments_table.html.haml @@ -4,12 +4,15 @@ %th= "#{t('spree.date')}/#{t('spree.time')}" %th= t(:description) %th= t(:amount) + %th= t(:tax_category) %th= t(:tax) + %th= t(:total_incl_tax) %th.actions %tbody - @collection.each do |adjustment| - @edit_url = edit_admin_order_adjustment_path(@order, adjustment) - @delete_url = admin_order_adjustment_path(@order, adjustment) + - taxable = adjustment.adjustable_type == "Spree::Shipment" ? adjustment.adjustable : adjustment - tr_class = cycle('odd', 'even') - tr_id = spree_dom_id(adjustment) %tr{:class => tr_class, "data-hook" => "adjustment_row", :id => tr_id} @@ -19,8 +22,12 @@ = adjustment.label %td.align-center.amount = adjustment.display_amount.to_html + %td.align-center.tax-category + = taxable.tax_category&.name || "-" %td.align-center.tax - = display_adjustment_taxes(adjustment) + = display_adjustment_taxes(taxable) + %td.align-center.total + = display_adjustment_total_with_tax(taxable) - unless @order.canceled? %td.actions - if adjustment.originator_type.nil? From 8f62ccb9b7d784f30d81819c1ae1e94a34c877b7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 21 Jun 2021 11:00:14 +0100 Subject: [PATCH 23/31] Match tax rates using rates applicable to the order --- .../20210617203927_migrate_admin_tax_amounts.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/db/migrate/20210617203927_migrate_admin_tax_amounts.rb b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb index a91765f4d4..1cf8b8bce3 100644 --- a/db/migrate/20210617203927_migrate_admin_tax_amounts.rb +++ b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb @@ -14,9 +14,9 @@ class MigrateAdminTaxAmounts < ActiveRecord::Migration[6.0] end def migrate_admin_taxes! - Spree::Adjustment.admin.where('included_tax <> 0').find_each do |adjustment| + Spree::Adjustment.admin.where('included_tax <> 0').includes(:order).find_each do |adjustment| - tax_rate = find_tax_rate(adjustment.amount, adjustment.included_tax) + tax_rate = find_tax_rate(adjustment) tax_category = tax_rate&.tax_category label = tax_adjustment_label(tax_rate) @@ -35,11 +35,15 @@ class MigrateAdminTaxAmounts < ActiveRecord::Migration[6.0] end end - def find_tax_rate(amount, included_tax) + def find_tax_rate(adjustment) + amount = adjustment.amount + included_tax = adjustment.included_tax approximation = (included_tax / (amount - included_tax)) + return if approximation.infinite? || approximation.zero? || approximation.nan? - Spree::TaxRate.order(Arel.sql("ABS(amount - #{approximation})")).first + applicable_rates = Spree::TaxRate.match(adjustment.order) + applicable_rates.min_by{ |rate| (rate.amount - approximation).abs } end def tax_adjustment_label(tax_rate) From ca6979e3945caade0301e3ae3a8f011e0baf6aae Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 28 Jun 2021 16:40:55 +0100 Subject: [PATCH 24/31] Extract call to `Spree::TaxRate.match` to a separate method and update tests --- ...0210617203927_migrate_admin_tax_amounts.rb | 9 ++- .../migrate_admin_tax_amounts_spec.rb | 57 +++++++++++++++---- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/db/migrate/20210617203927_migrate_admin_tax_amounts.rb b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb index 1cf8b8bce3..b6fb3da24f 100644 --- a/db/migrate/20210617203927_migrate_admin_tax_amounts.rb +++ b/db/migrate/20210617203927_migrate_admin_tax_amounts.rb @@ -42,8 +42,13 @@ class MigrateAdminTaxAmounts < ActiveRecord::Migration[6.0] return if approximation.infinite? || approximation.zero? || approximation.nan? - applicable_rates = Spree::TaxRate.match(adjustment.order) - applicable_rates.min_by{ |rate| (rate.amount - approximation).abs } + applicable_rates(adjustment).min_by{ |rate| (rate.amount - approximation).abs } + end + + def applicable_rates(adjustment) + return [] unless adjustment.order&.distributor_id.present? + + Spree::TaxRate.match(adjustment.order) end def tax_adjustment_label(tax_rate) diff --git a/spec/migrations/migrate_admin_tax_amounts_spec.rb b/spec/migrations/migrate_admin_tax_amounts_spec.rb index d069e6a087..b0e1296075 100644 --- a/spec/migrations/migrate_admin_tax_amounts_spec.rb +++ b/spec/migrations/migrate_admin_tax_amounts_spec.rb @@ -25,7 +25,10 @@ describe MigrateAdminTaxAmounts do end context "when the adjustments have tax" do - before { adjustment10; adjustment50 } + before do + adjustment10; adjustment50 + allow(subject).to receive(:applicable_rates) { [tax_rate10, tax_rate50] } + end it "moves the tax to an adjustment" do expect(Spree::Adjustment).to receive(:create!).at_least(:once).and_call_original @@ -54,23 +57,55 @@ describe MigrateAdminTaxAmounts do end describe "#find_tax_rate" do - it "matches rates correctly" do - expect( - subject.find_tax_rate(adjustment10.amount, adjustment10.included_tax) - ).to eq(tax_rate10) + before do + allow(subject).to receive(:applicable_rates) { [tax_rate10, tax_rate50] } + end - expect( - subject.find_tax_rate(adjustment50.amount, adjustment50.included_tax) - ).to eq(tax_rate50) + it "matches rates correctly" do + expect(subject.find_tax_rate(adjustment10)).to eq(tax_rate10) + + expect(subject.find_tax_rate(adjustment50)).to eq(tax_rate50) end context "without a perfect match" do let(:adjustment45) { create(:adjustment, amount: 100, included_tax: 45) } it "finds the closest match" do - expect( - subject.find_tax_rate(adjustment45.amount, adjustment45.included_tax) - ).to eq(tax_rate50) + expect(subject.find_tax_rate(adjustment45)).to eq(tax_rate50) + end + end + end + + describe "#applicabe_rates" do + let(:distributor) { create(:enterprise) } + let(:order) { create(:order, distributor: distributor) } + let!(:adjustment) { create(:adjustment, order: order) } + + context "when the order is nil" do + let(:order) { nil } + + it "returns an empty array" do + expect(Spree::TaxRate).to_not receive(:match) + + expect(subject.applicable_rates(adjustment)).to eq [] + end + end + + context "when the order has no distributor" do + let(:distributor) { nil } + + it "returns an empty array" do + expect(Spree::TaxRate).to_not receive(:match) + + expect(subject.applicable_rates(adjustment)).to eq [] + end + end + + context "when the order has a distributor" do + it "calls TaxRate#match for an array of applicable taxes for the order" do + expect(Spree::TaxRate).to receive(:match) { [tax_rate10] } + + expect(subject.applicable_rates(adjustment)).to eq [tax_rate10] end end end From d55079f4747c25bdf0789dad3e9bf86e914b8f2e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 28 Jun 2021 15:30:06 +0100 Subject: [PATCH 25/31] Extract comment-method --- .../spree/admin/orders/customer_details_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb index 3644fbaf65..3b25a16671 100644 --- a/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -25,9 +25,9 @@ module Spree @order.associate_user!(Spree.user_class.find_by(email: @order.email)) end + refresh_shipment_rates OrderWorkflow.new(@order).advance_to_payment - @order.shipments.map(&:refresh_rates) flash[:success] = Spree.t('customer_details_updated') redirect_to spree.admin_order_customer_path(@order) else @@ -43,6 +43,10 @@ module Spree private + def refresh_shipment_rates + @order.shipments.map(&:refresh_rates) + end + def order_params params.require(:order).permit( :email, From 41757254d62c4b39fd773cebb6101d7e3102442e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 28 Jun 2021 15:35:01 +0100 Subject: [PATCH 26/31] Recalculate taxes when an order's customer details are changed --- .../spree/admin/orders/customer_details_controller.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb index 3b25a16671..5b4806c815 100644 --- a/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -26,6 +26,7 @@ module Spree end refresh_shipment_rates + recalculate_taxes OrderWorkflow.new(@order).advance_to_payment flash[:success] = Spree.t('customer_details_updated') @@ -47,6 +48,15 @@ module Spree @order.shipments.map(&:refresh_rates) end + def recalculate_taxes + # If the order's address has been changed, the tax zone could be different, + # which means a different set of tax rates might be applicable. + @order.create_tax_charge! + Spree::TaxRate.adjust(@order, @order.adjustments.admin) + + @order.updater.update_totals_and_states + end + def order_params params.require(:order).permit( :email, From 69e4670a175f87eee1fb95e54fa1c459456b36f1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 28 Jun 2021 17:12:13 +0100 Subject: [PATCH 27/31] Tidy up building default address objects --- .../spree/admin/orders/customer_details_controller.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb index 5b4806c815..271b6b1b16 100644 --- a/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -14,9 +14,7 @@ module Spree end def edit - country_id = Address.default.country.id - @order.build_bill_address(country_id: country_id) if @order.bill_address.nil? - @order.build_ship_address(country_id: country_id) if @order.ship_address.nil? + build_addresses end def update @@ -44,6 +42,12 @@ module Spree private + def build_addresses + country_id = Address.default.country.id + @order.build_bill_address(country_id: country_id) if @order.bill_address.nil? + @order.build_ship_address(country_id: country_id) if @order.ship_address.nil? + end + def refresh_shipment_rates @order.shipments.map(&:refresh_rates) end From 091bfc710f6d6041c176534e028e3817e1d76de5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 12 Jul 2021 09:22:43 +0100 Subject: [PATCH 28/31] Add test for applying multiple tax rates per tax zone --- .../admin/adjustments_controller_spec.rb | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/spec/controllers/spree/admin/adjustments_controller_spec.rb b/spec/controllers/spree/admin/adjustments_controller_spec.rb index 1e6fe5f8a5..7c832fd835 100644 --- a/spec/controllers/spree/admin/adjustments_controller_spec.rb +++ b/spec/controllers/spree/admin/adjustments_controller_spec.rb @@ -94,6 +94,42 @@ module Spree expect(order.included_tax_total).to eq 10 end end + + context "when the tax category has multiple rates for the same tax zone" do + let(:tax_category) { create(:tax_category) } + let!(:tax_rate1) { + create(:tax_rate, amount: 0.1, zone: zone, included_in_price: false, + tax_category: tax_category ) + } + let!(:tax_rate2) { + create(:tax_rate, amount: 0.2, zone: zone, included_in_price: false, + tax_category: tax_category ) + } + let(:tax_category_param) { tax_category.id.to_s } + let(:params) { + { + order_id: order.number, + adjustment: { + label: 'Testing multiple rates', amount: '100', tax_category_id: tax_category_param + } + } + } + + it "applies both rates" do + spree_post :create, params + expect(response).to redirect_to spree.admin_order_adjustments_path(order) + + new_adjustment = Adjustment.admin.last + + expect(new_adjustment.amount).to eq(100) + expect(new_adjustment.tax_category).to eq tax_category + expect(new_adjustment.order_id).to eq(order.id) + expect(new_adjustment.adjustments.tax.count).to eq 2 + + expect(order.reload.total).to eq 130 + expect(order.additional_tax_total).to eq 30 + end + end end describe "updating an adjustment" do From cab36f375bab24bd67416d06b34462160f9dad54 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 13 Jul 2021 13:09:11 +0200 Subject: [PATCH 29/31] Update app/services/order_tax_adjustments_fetcher.rb Co-authored-by: Maikel --- app/services/order_tax_adjustments_fetcher.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 424bdb100a..83ea65dfc1 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -10,8 +10,7 @@ class OrderTaxAdjustmentsFetcher def totals order.all_adjustments.tax.each_with_object({}) do |adjustment, hash| tax_rate = adjustment.originator - tax_amounts = { tax_rate => adjustment.amount } - hash.update(tax_amounts) { |_tax_rate, amount1, amount2| amount1 + amount2 } + hash[tax_rate] = hash[tax_rate].to_f + adjustment.amount end end From cb114283eaf9e71ca4838c09659ff082551b4391 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 13 Jul 2021 13:13:07 +0200 Subject: [PATCH 30/31] Update spec/migrations/migrate_admin_tax_amounts_spec.rb Co-authored-by: Maikel --- spec/migrations/migrate_admin_tax_amounts_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/migrations/migrate_admin_tax_amounts_spec.rb b/spec/migrations/migrate_admin_tax_amounts_spec.rb index b0e1296075..af5df9c110 100644 --- a/spec/migrations/migrate_admin_tax_amounts_spec.rb +++ b/spec/migrations/migrate_admin_tax_amounts_spec.rb @@ -18,9 +18,9 @@ describe MigrateAdminTaxAmounts do let!(:adjustment_without_tax) { create(:adjustment, included_tax: 0) } it "doesn't move the tax to an adjustment" do - expect(Spree::Adjustment).to_not receive(:create!) - - subject.migrate_admin_taxes! + expect { subject.migrate_admin_taxes! }.to_not change { + Spree::Adjustment.count + } end end From 6daa37a5c0c0318f8b53eb8420ad87d3e8914856 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 20 Jul 2021 12:14:41 +0100 Subject: [PATCH 31/31] Ensure TaxRateFinder returns an array if the given adjustment has no tax --- app/services/tax_rate_finder.rb | 7 ++----- spec/services/tax_rate_finder_spec.rb | 27 +++++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index a1b61bb406..1f90304efe 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -6,15 +6,12 @@ class TaxRateFinder # @return [Array] def self.tax_rates_of(adjustment) - new.tax_rates( - adjustment.originator, - adjustment.adjustable - ) + new.tax_rates(adjustment.originator, adjustment.adjustable) end # @return [Array] def tax_rates(originator, adjustable) - find_associated_tax_rate(originator, adjustable) + find_associated_tax_rate(originator, adjustable) || [] end private diff --git a/spec/services/tax_rate_finder_spec.rb b/spec/services/tax_rate_finder_spec.rb index 1c1abe425a..8fcfdb738c 100644 --- a/spec/services/tax_rate_finder_spec.rb +++ b/spec/services/tax_rate_finder_spec.rb @@ -5,7 +5,9 @@ require 'spec_helper' describe TaxRateFinder do describe "getting the corresponding tax rate" do let(:amount) { BigDecimal(120) } - let(:tax_rate) { create_rate(0.2) } + let(:tax_rate) { + create(:tax_rate, amount: 0.2, calculator: Calculator::DefaultTax.new, zone: zone) + } let(:tax_category) { create(:tax_category, tax_rates: [tax_rate]) } let(:zone) { create(:zone_with_member) } let(:shipment) { create(:shipment) } @@ -13,35 +15,36 @@ describe TaxRateFinder do let(:enterprise_fee) { create(:enterprise_fee, tax_category: tax_category) } let(:order) { create(:order_with_taxes, zone: zone) } + subject { TaxRateFinder.new } + it "finds the tax rate of a shipping fee" do - rates = TaxRateFinder.new.tax_rates(tax_rate, shipment) + rates = subject.tax_rates(tax_rate, shipment) expect(rates).to eq [tax_rate] end it "deals with soft-deleted tax rates" do tax_rate.destroy - rates = TaxRateFinder.new.tax_rates(tax_rate, shipment) + rates = subject.tax_rates(tax_rate, shipment) expect(rates).to eq [tax_rate] end it "finds the tax rate of an enterprise fee" do - rates = TaxRateFinder.new.tax_rates(enterprise_fee, order) + rates = subject.tax_rates(enterprise_fee, order) expect(rates).to eq [tax_rate] end it "deals with a soft-deleted line item" do line_item.destroy - rates = TaxRateFinder.new.tax_rates(enterprise_fee, line_item) + rates = subject.tax_rates(enterprise_fee, line_item) expect(rates).to eq [tax_rate] end - def create_rate(amount) - create( - :tax_rate, - amount: amount, - calculator: Calculator::DefaultTax.new, - zone: zone - ) + context "when the given adjustment has no associated tax" do + let(:adjustment) { create(:adjustment) } + + it "returns an empty array" do + expect(subject.tax_rates(adjustment.originator, adjustment.adjustable)).to eq [] + end end end end