From 7eca32e84fb3334ca73d5e7a3401fae35eaa08ed Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Fri, 3 Mar 2023 10:57:30 +0100 Subject: [PATCH 1/6] prevent saving the enterprise fee when a per order calculator is selected along with 'Inherit from product' --- app/models/enterprise_fee.rb | 8 ++++++++ config/locales/en.yml | 3 ++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index bac889327b..9acfc31134 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -58,6 +58,14 @@ class EnterpriseFee < ApplicationRecord elsif inherits_tax_category_changed? self.tax_category_id = nil if inherits_tax_category? end + + if inherits_tax_category? && PER_ORDER_CALCULATORS.include?(calculator_type) + errors.add(:inherits_tax_category, + I18n.t("activerecord.errors.models." \ + "enterpise_fee.cannot_inherit_from_product_when_per_order_calculator_selected")) + throw :abort + end + true end end diff --git a/config/locales/en.yml b/config/locales/en.yml index c8a9ab96fc..e991f46884 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -113,7 +113,8 @@ en: using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" on_demand_but_count_on_hand_set: "must be blank if on demand" limited_stock_but_no_count_on_hand: "must be specified because forcing limited stock" - + enterpise_fee: + cannot_inherit_from_product_when_per_order_calculator_selected: "You cannot select 'Inherit From Product' when a per order calculator is selected" # Used by active_storage_validations errors: messages: From 89eb3a1967f71eb75f887cbb1a8d4d49d79400f2 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Fri, 3 Mar 2023 11:17:03 +0100 Subject: [PATCH 2/6] fix existing tests --- .../enterprise_fee_summary_report_spec.rb | 6 +++--- spec/models/spree/adjustment_spec.rb | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb b/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb index f36faea77e..58711f74ba 100644 --- a/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb +++ b/spec/lib/reports/enterprise_fee_summary/enterprise_fee_summary_report_spec.rb @@ -319,9 +319,9 @@ describe Reporting::Reports::EnterpriseFeeSummary::Base do amount: 15) end let!(:coordinator_fee_inheriting_product_tax_category) do - create(:enterprise_fee, :flat_rate, name: "Coordinator Fee B", enterprise: coordinator, - fee_type: "admin", inherits_tax_category: true, - amount: 20) + create(:enterprise_fee, :per_item, name: "Coordinator Fee B", enterprise: coordinator, + fee_type: "admin", inherits_tax_category: true, + amount: 20) end let!(:coordinator_fee_without_tax) do create(:enterprise_fee, :flat_rate, name: "Coordinator Fee C", enterprise: coordinator, diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 2dde510c6e..87576411db 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -426,7 +426,7 @@ module Spree context "when enterprise fees are taxed per-order" do let(:enterprise_fee) { - create(:enterprise_fee, enterprise: coordinator, inherits_tax_category: true, + create(:enterprise_fee, enterprise: coordinator, inherits_tax_category: false, calculator: ::Calculator::FlatRate.new(preferred_amount: 50.0)) } From 25b7f1749c567cc2948fefc1c3effa8a1d9fd332 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Mon, 6 Mar 2023 14:37:25 +1100 Subject: [PATCH 3/6] Simplify error when tax rate can't be inherited I changed the text to focus on the resolution: the user needs to choose a tax category or a different calculator. I associated the error to the model to prevent the attribute name from being included in the error message. Alternatively, we could have changed the name of the attribute to match the UI. But this error affects the combination of two attributes, none of them is invalid on its own. I'm using Rails' default lazy lookup for error messages which results in shorter code and a standard structure. I also added a simple spec. --- app/models/enterprise_fee.rb | 4 +--- config/locales/en.yml | 4 ++-- spec/models/enterprise_fee_spec.rb | 13 +++++++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 9acfc31134..598035abed 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -60,9 +60,7 @@ class EnterpriseFee < ApplicationRecord end if inherits_tax_category? && PER_ORDER_CALCULATORS.include?(calculator_type) - errors.add(:inherits_tax_category, - I18n.t("activerecord.errors.models." \ - "enterpise_fee.cannot_inherit_from_product_when_per_order_calculator_selected")) + errors.add(:base, :inherit_tax_requires_per_item_calculator) throw :abort end diff --git a/config/locales/en.yml b/config/locales/en.yml index e991f46884..aaea8d1f1b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -92,6 +92,8 @@ en: preferred_per_unit: "Calculator Per Unit:" errors: models: + enterprise_fee: + inherit_tax_requires_per_item_calculator: "Inheriting the tax categeory requires a per-item calculator." spree/user: attributes: email: @@ -113,8 +115,6 @@ en: using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" on_demand_but_count_on_hand_set: "must be blank if on demand" limited_stock_but_no_count_on_hand: "must be specified because forcing limited stock" - enterpise_fee: - cannot_inherit_from_product_when_per_order_calculator_selected: "You cannot select 'Inherit From Product' when a per order calculator is selected" # Used by active_storage_validations errors: messages: diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 2eeb2ae027..6d579a42dd 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -9,6 +9,19 @@ describe EnterpriseFee do describe "validations" do it { is_expected.to validate_presence_of(:name) } + + it "requires a per-item calculator to inherit tax" do + subject = build( + :enterprise_fee, + inherits_tax_category: true, + calculator: Calculator::FlatRate.new + ) + + expect(subject.save).to eq false + expect(subject.errors.full_messages.first).to eq( + "Inheriting the tax categeory requires a per-item calculator." + ) + end end describe "callbacks" do From a22fe9f948c01785d8248313cee5bda7bf499d0a Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Thu, 16 Mar 2023 05:05:25 +0100 Subject: [PATCH 4/6] fix existing invalid enterprise fees --- ...category_from_per_order_enterprise_fees.rb | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb diff --git a/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb b/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb new file mode 100644 index 0000000000..4771fe826b --- /dev/null +++ b/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class DisableInheritsTaxCategoryFromPerOrderEnterpriseFees < ActiveRecord::Migration[6.1] + class EnterpriseFee < ApplicationRecord + has_one :calculator, as: :calculable, class_name: "Spree::Calculator", dependent: :destroy + end + + class Spree + class Calculator < ApplicationRecord + self.table_name = 'spree_calculators' + end + end + + def change + EnterpriseFee.joins(:calculator).merge(calculators) + .where(inherits_tax_category: true) + .update_all(inherits_tax_category: false) + end + + def calculators + Spree::Calculator.where(type: per_order_calculators) + end + + def per_order_calculators + ['Calculator::FlatRate', + 'Calculator::FlexiRate', + 'Calculator::PriceSack'] + end +end From 6771d2e7bef58cf6b1f7b39d46790fd9abfd2d10 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Sat, 8 Apr 2023 07:19:51 +0100 Subject: [PATCH 5/6] test enterprise fees creation with tax category inheritance flag --- ...category_from_per_order_enterprise_fees.rb | 6 +-- spec/models/enterprise_fee_spec.rb | 49 +++++++++++++++---- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb b/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb index 4771fe826b..07b1e8bf21 100644 --- a/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb +++ b/db/migrate/20230316034707_disable_inherits_tax_category_from_per_order_enterprise_fees.rb @@ -2,10 +2,10 @@ class DisableInheritsTaxCategoryFromPerOrderEnterpriseFees < ActiveRecord::Migration[6.1] class EnterpriseFee < ApplicationRecord - has_one :calculator, as: :calculable, class_name: "Spree::Calculator", dependent: :destroy + has_many :calculator, class_name: "Spree::Calculator", foreign_key: :calculable_id end - class Spree + module Spree class Calculator < ApplicationRecord self.table_name = 'spree_calculators' end @@ -18,7 +18,7 @@ class DisableInheritsTaxCategoryFromPerOrderEnterpriseFees < ActiveRecord::Migra end def calculators - Spree::Calculator.where(type: per_order_calculators) + Spree::Calculator.where(type: per_order_calculators, calculable_type: 'EnterpriseFee') end def per_order_calculators diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 6d579a42dd..ab6f5eda53 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -10,17 +10,46 @@ describe EnterpriseFee do describe "validations" do it { is_expected.to validate_presence_of(:name) } - it "requires a per-item calculator to inherit tax" do - subject = build( - :enterprise_fee, - inherits_tax_category: true, - calculator: Calculator::FlatRate.new - ) + describe "requires a per-item calculator to inherit tax" do + let(:per_order_calculators){ + [ + Calculator::FlatRate, + Calculator::FlexiRate, + Calculator::PriceSack, + ] + } - expect(subject.save).to eq false - expect(subject.errors.full_messages.first).to eq( - "Inheriting the tax categeory requires a per-item calculator." - ) + let(:per_item_calculators){ + [ + Calculator::PerItem, + Calculator::FlatPercentPerItem + ] + } + + it "is valid when inheriting tax and using a per-item calculator" do + per_item_calculators.each do |calculator| + subject = build( + :enterprise_fee, + inherits_tax_category: true, + calculator: calculator.new + ) + expect(subject.save).to eq true + end + end + + it "is invalid when inheriting tax and using a per-order calculator" do + per_order_calculators.each do |calculator| + subject = build( + :enterprise_fee, + inherits_tax_category: true, + calculator: calculator.new + ) + expect(subject.save).to eq false + expect(subject.errors.full_messages.first).to eq( + "Inheriting the tax categeory requires a per-item calculator." + ) + end + end end end From 7f526844888a9f2ad62abc752cd1345aae9505cf Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Tue, 9 May 2023 10:27:06 +0100 Subject: [PATCH 6/6] add check_calculators_compatibility_with_taxes before running enterprise bulk update --- .rubocop_todo.yml | 1 + app/controllers/admin/enterprise_fees_controller.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index dc22595479..1d843fdf10 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -656,6 +656,7 @@ Metrics/BlockNesting: Metrics/ClassLength: Exclude: - 'app/components/products_table_component.rb' + - 'app/controllers/admin/enterprise_fees_controller.rb' - 'app/controllers/admin/enterprises_controller.rb' - 'app/controllers/admin/order_cycles_controller.rb' - 'app/controllers/admin/resource_controller.rb' diff --git a/app/controllers/admin/enterprise_fees_controller.rb b/app/controllers/admin/enterprise_fees_controller.rb index 8bd4793c5b..3281b67317 100644 --- a/app/controllers/admin/enterprise_fees_controller.rb +++ b/app/controllers/admin/enterprise_fees_controller.rb @@ -7,6 +7,7 @@ module Admin before_action :load_enterprise_fee_set, only: :index before_action :load_data before_action :check_enterprise_fee_input, only: [:bulk_update] + before_action :check_calculators_compatibility_with_taxes, only: [:bulk_update] def index @include_calculators = params[:include_calculators].present? @@ -119,5 +120,17 @@ module Admin end end end + + def check_calculators_compatibility_with_taxes + enterprise_fee_bulk_params['collection_attributes'].each do |_, enterprise_fee| + next unless enterprise_fee['inherits_tax_category'] == "true" + next unless EnterpriseFee::PER_ORDER_CALCULATORS.include?(enterprise_fee['calculator_type']) + + flash[:error] = I18n.t( + 'activerecord.errors.models.enterprise_fee.inherit_tax_requires_per_item_calculator' + ) + return redirect_to redirect_path + end + end end end