Merge pull request #12313 from anthonyms/11482-fix-rubocop-rails-issue-has_many

Fix Rubocop Rails: Rails/HasManyOrHasOneDependent

.rubocop_todo.yml

@@ -565,17 +565,6 @@ RSpecRails/InferredSpecType:
     - 'spec/requests/voucher_adjustments_spec.rb'
     - 'spec/routing/stripe_spec.rb'
 
-# Offense count: 11
-# Configuration parameters: Include.
-# Include: app/models/**/*.rb
-Rails/HasManyOrHasOneDependent:
-  Exclude:
-    - 'app/models/enterprise.rb'
-    - 'app/models/spree/address.rb'
-    - 'app/models/spree/stock_item.rb'
-    - 'app/models/spree/tax_rate.rb'
-    - 'app/models/spree/variant.rb'
-
 # Offense count: 22
 # Configuration parameters: IgnoreScopes, Include.
 # Include: app/models/**/*.rb

app/models/enterprise.rb

@@ -43,25 +43,31 @@ class Enterprise < ApplicationRecord
                                foreign_key: 'supplier_id',
                                dependent: :destroy
   has_many :supplied_variants, through: :supplied_products, source: :variants
-  has_many :distributed_orders, class_name: 'Spree::Order', foreign_key: 'distributor_id'
+  has_many :distributed_orders, class_name: 'Spree::Order',
+                                foreign_key: 'distributor_id',
+                                dependent: :restrict_with_exception
   belongs_to :address, class_name: 'Spree::Address'
   belongs_to :business_address, optional: true, class_name: 'Spree::Address', dependent: :destroy
-  has_many :enterprise_fees
+  has_many :enterprise_fees, dependent: :restrict_with_exception
   has_many :enterprise_roles, dependent: :destroy
   has_many :users, through: :enterprise_roles
   belongs_to :owner, class_name: 'Spree::User',
                      inverse_of: :owned_enterprises
   has_many :distributor_payment_methods,
-           inverse_of: :distributor, foreign_key: :distributor_id
+           inverse_of: :distributor,
+           foreign_key: :distributor_id,
+           dependent: :restrict_with_exception
   has_many :distributor_shipping_methods,
-           inverse_of: :distributor, foreign_key: :distributor_id
+           inverse_of: :distributor,
+           foreign_key: :distributor_id,
+           dependent: :restrict_with_exception
   has_many :payment_methods, through: :distributor_payment_methods
   has_many :shipping_methods, through: :distributor_shipping_methods
   has_many :customers, dependent: :destroy
   has_many :inventory_items, dependent: :destroy
   has_many :tag_rules, dependent: :destroy
   has_one :stripe_account, dependent: :destroy
-  has_many :vouchers
+  has_many :vouchers, dependent: :restrict_with_exception
   has_many :connected_apps, dependent: :destroy
   has_one :custom_tab, dependent: :destroy
 
@@ -128,6 +134,7 @@ class Enterprise < ApplicationRecord
 
   after_create :set_default_contact
   after_create :relate_to_owners_enterprises
+
   after_rollback :restore_permalink
   after_touch :touch_distributors
   after_create_commit :send_welcome_email

app/models/spree/address.rb

@@ -12,7 +12,7 @@ module Spree
     belongs_to :state, class_name: "Spree::State", optional: true
 
     has_one :enterprise, dependent: :restrict_with_exception
-    has_many :shipments
+    has_many :shipments, dependent: :restrict_with_exception
 
     validates :address1, :city, :phone, presence: true
     validates :company, presence: true, unless: -> { first_name.blank? || last_name.blank? }

app/models/spree/stock_item.rb

@@ -8,7 +8,7 @@ module Spree
 
     belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items
     belongs_to :variant, -> { with_deleted }, class_name: 'Spree::Variant'
-    has_many :stock_movements
+    has_many :stock_movements, dependent: :destroy
 
     validates :stock_location, :variant, presence: true
     validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }

app/models/spree/stock_movement.rb

@@ -14,10 +14,6 @@ module Spree
 
     scope :recent, -> { order('created_at DESC') }
 
-    def readonly?
-      !new_record?
-    end
-
     private
 
     def update_stock_item_quantity

app/models/spree/tax_rate.rb

@@ -21,7 +21,7 @@ module Spree
 
     belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates
     belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates
-    has_many :adjustments, as: :originator
+    has_many :adjustments, as: :originator, dependent: nil
 
     validates :amount, presence: true, numericality: true
     validates :tax_category, presence: true

app/models/spree/variant.rb

@@ -32,8 +32,8 @@ module Spree
 
     delegate :name, :name=, :description, :description=, :meta_keywords, to: :product
 
-    has_many :inventory_units, inverse_of: :variant
-    has_many :line_items, inverse_of: :variant
+    has_many :inventory_units, inverse_of: :variant, dependent: nil
+    has_many :line_items, inverse_of: :variant, dependent: nil
 
     has_many :stock_items, dependent: :destroy, inverse_of: :variant
     has_many :stock_locations, through: :stock_items
@@ -53,7 +53,7 @@ module Spree
              :currency, :currency=,
              to: :find_or_build_default_price
 
-    has_many :exchange_variants
+    has_many :exchange_variants, dependent: nil
     has_many :exchanges, through: :exchange_variants
     has_many :variant_overrides, dependent: :destroy
     has_many :inventory_items, dependent: :destroy

spec/factories/payment_method_factory.rb

@@ -24,4 +24,9 @@ FactoryBot.define do
     distributors { [FactoryBot.create(:stripe_account).enterprise] }
     preferred_enterprise_id { distributors.first.id }
   end
+
+  factory :distributor_payment_method, class: DistributorPaymentMethod do
+    distributor { FactoryBot.create(:distributor_enterprise) }
+    payment_method { FactoryBot.create(:payment_method) }
+  end
 end

spec/factories/shipping_method_factory.rb

@@ -68,4 +68,9 @@ FactoryBot.define do
       distributors { [create(:distributor_enterprise_with_tax)] }
     end
   end
+
+  factory :distributor_shipping_method, class: DistributorShippingMethod do
+    shipping_method { FactoryBot.create(:shipping_method) }
+    distributor { FactoryBot.create(:distributor_enterprise) }
+  end
 end

spec/models/enterprise_spec.rb

@@ -57,6 +57,63 @@ describe Enterprise do
       expect(EnterpriseRelationship.where(id: [er1, er2])).to be_empty
     end
 
+    it "raises a DeleteRestrictionError on destroy if distributed_orders exist" do
+      enterprise = create(:distributor_enterprise)
+      create_list(:order, 2, distributor: enterprise)
+
+      expect do
+        enterprise.destroy
+      end.to raise_error(ActiveRecord::DeleteRestrictionError,
+                         /Cannot delete record because of dependent distributed_orders/)
+        .and change { Spree::Order.count }.by(0)
+    end
+
+    it "raises an DeleteRestrictionError on destroy if distributor_payment_methods exist" do
+      enterprise = create(:distributor_enterprise)
+      create_list(:distributor_payment_method, 2, distributor: enterprise)
+
+      expect do
+        enterprise.destroy
+      end.to raise_error(ActiveRecord::DeleteRestrictionError,
+                         /Cannot delete record because of dependent distributor_payment_methods/)
+        .and change { DistributorPaymentMethod.count }.by(0)
+    end
+
+    it "raises an DeleteRestrictionError on destroy if distributor_shipping_methods exist" do
+      enterprise = create(:distributor_enterprise)
+      create_list(:distributor_shipping_method, 2, distributor: enterprise)
+
+      expect do
+        enterprise.destroy
+      end.to raise_error(ActiveRecord::DeleteRestrictionError,
+                         /Cannot delete record because of dependent distributor_shipping_methods/)
+        .and change { DistributorShippingMethod.count }.by(0)
+    end
+
+    it "does not destroy enterprise_fees upon destroy" do
+      enterprise = create(:enterprise)
+      create_list(:enterprise_fee, 2, enterprise:)
+
+      expect do
+        enterprise.destroy
+      end.to raise_error(ActiveRecord::DeleteRestrictionError,
+                         /Cannot delete record because of dependent enterprise_fees/)
+        .and change { EnterpriseFee.count }.by(0)
+    end
+
+    it "does not destroy vouchers upon destroy" do
+      enterprise = create(:enterprise)
+      (1..2).map do |code|
+        create(:voucher, enterprise:, code: "new code #{code}")
+      end
+
+      expect do
+        enterprise.destroy
+      end.to raise_error(ActiveRecord::DeleteRestrictionError,
+                         /Cannot delete record because of dependent vouchers/)
+        .and change { Voucher.count }.by(0)
+    end
+
     describe "relationships to other enterprises" do
       let(:e) { create(:distributor_enterprise) }
       let(:p) { create(:supplier_enterprise) }

spec/models/spree/address_spec.rb

@@ -134,6 +134,17 @@ describe Spree::Address do
     end
   end
 
+  context "associations" do
+    it "destroys shipments upon destroy" do
+      address = create(:address)
+      create(:shipment, address:)
+
+      expect {
+        address.destroy
+      }.to raise_error(ActiveRecord::DeleteRestrictionError)
+    end
+  end
+
   context ".default" do
     it "sets up a new record the default country" do
       expect(Spree::Address.default.country).to eq DefaultCountry.country

spec/models/spree/stock_item_spec.rb

@@ -109,8 +109,8 @@ RSpec.describe Spree::StockItem do
     context "with stock movements" do
       before { Spree::StockMovement.create(stock_item: subject, quantity: 1) }
 
-      it "doesnt raise ReadOnlyRecord error" do
-        expect { subject.destroy }.not_to raise_error
+      it "does not destroy stock_movements when destroyed" do
+        expect { subject.destroy }.to change { Spree::StockMovement.count }.by(-1)
       end
     end
   end

spec/models/spree/stock_movement_spec.rb

@@ -11,13 +11,6 @@ describe Spree::StockMovement do
     expect(subject).to respond_to(:stock_item)
   end
 
-  it 'is readonly unless new' do
-    subject.save
-    expect {
-      subject.save
-    }.to raise_error(ActiveRecord::ReadOnlyRecord)
-  end
-
   context "when quantity is negative" do
     context "after save" do
       it "should decrement the stock item count on hand" do