Merge pull request #10512 from abdellani/fix-tax-not-charged-when-tax-category-set-to-inherit-from-product

Prevent saving the enterprise fee when a per order calculator is selected along with 'Inherit from product'

.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'

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

app/models/enterprise_fee.rb

@@ -58,6 +58,12 @@ 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(:base, :inherit_tax_requires_per_item_calculator)
+      throw :abort
+    end
+
     true
   end
 end

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,7 +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"
-
   # Used by active_storage_validations
   errors:
     messages:

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_many :calculator, class_name: "Spree::Calculator", foreign_key: :calculable_id
+  end
+
+  module 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, calculable_type: 'EnterpriseFee')
+  end
+
+  def per_order_calculators
+    ['Calculator::FlatRate',
+     'Calculator::FlexiRate',
+     'Calculator::PriceSack']
+  end
+end

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,

spec/models/enterprise_fee_spec.rb

@@ -9,6 +9,48 @@ describe EnterpriseFee do
 
   describe "validations" do
     it { is_expected.to validate_presence_of(:name) }
+
+    describe "requires a per-item calculator to inherit tax" do
+      let(:per_order_calculators){
+        [
+          Calculator::FlatRate,
+          Calculator::FlexiRate,
+          Calculator::PriceSack,
+        ]
+      }
+
+      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
 
   describe "callbacks" do

spec/models/spree/adjustment_spec.rb

@@ -436,7 +436,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))
             }