From 5559816e12169cbd4e401fc151ec0f1270608cb5 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Thu, 21 Mar 2024 15:02:38 +0300
Subject: [PATCH 1/8] Fix Rubocop Rails issue: Rails/HasManyOrHasOneDependent

---
 app/models/enterprise.rb                  |  8 ++++---
 spec/factories/payment_method_factory.rb  |  5 ++++
 spec/factories/shipping_method_factory.rb |  5 ++++
 spec/models/enterprise_spec.rb            | 29 +++++++++++++++++++++++
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 22a18473a9..5340aecddd 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -43,7 +43,9 @@ 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: :destroy
   belongs_to :address, class_name: 'Spree::Address'
   belongs_to :business_address, optional: true, class_name: 'Spree::Address', dependent: :destroy
   has_many :enterprise_fees
@@ -52,9 +54,9 @@ class Enterprise < ApplicationRecord
   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: :destroy
   has_many :distributor_shipping_methods,
-           inverse_of: :distributor, foreign_key: :distributor_id
+           inverse_of: :distributor, foreign_key: :distributor_id, dependent: :destroy
   has_many :payment_methods, through: :distributor_payment_methods
   has_many :shipping_methods, through: :distributor_shipping_methods
   has_many :customers, dependent: :destroy
diff --git a/spec/factories/payment_method_factory.rb b/spec/factories/payment_method_factory.rb
index 98e8d36a35..c93657ef21 100644
--- a/spec/factories/payment_method_factory.rb
+++ b/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
diff --git a/spec/factories/shipping_method_factory.rb b/spec/factories/shipping_method_factory.rb
index d6957e78bd..68b41106c4 100644
--- a/spec/factories/shipping_method_factory.rb
+++ b/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
diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb
index e764c9da73..06eb7cfbb0 100644
--- a/spec/models/enterprise_spec.rb
+++ b/spec/models/enterprise_spec.rb
@@ -57,6 +57,35 @@ describe Enterprise do
       expect(EnterpriseRelationship.where(id: [er1, er2])).to be_empty
     end
 
+    it "destroys all distributed_orders upon destroy" do
+      enterprise = create(:distributor_enterprise)
+      order_ids = create_list(:order, 2, distributor: enterprise).map(&:id)
+
+      expect(Spree::Order.where(id: order_ids)).to exist
+      enterprise.destroy
+      expect(Spree::Order.where(id: order_ids)).not_to exist
+    end
+
+    it "destroys all distributor_payment_methods upon destroy" do
+      enterprise = create(:distributor_enterprise)
+      payment_method_ids = create_list(:distributor_payment_method, 2,
+                                       distributor: enterprise).map(&:id)
+
+      expect(DistributorPaymentMethod.where(id: payment_method_ids)).to exist
+      enterprise.destroy
+      expect(DistributorPaymentMethod.where(id: payment_method_ids)).not_to exist
+    end
+
+    it "destroys all distributor_shipping_methods upon destroy" do
+      enterprise = create(:enterprise)
+      shipping_method_ids = create_list(:distributor_shipping_method, 2,
+                                        distributor: enterprise).map(&:id)
+
+      expect(DistributorShippingMethod.where(id: shipping_method_ids)).to exist
+      enterprise.destroy
+      expect(DistributorShippingMethod.where(id: shipping_method_ids)).not_to exist
+    end
+
     describe "relationships to other enterprises" do
       let(:e) { create(:distributor_enterprise) }
       let(:p) { create(:supplier_enterprise) }

From 1ec453df4df494a4c1c5f675b57616d3095e2a85 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Thu, 21 Mar 2024 15:54:29 +0300
Subject: [PATCH 2/8] Fix Rubocop issue: Do not delete addresses having
 shipments

The reasoning is that we should not delete an address that has
ever received a shipment
---
 app/models/spree/address.rb       |  2 +-
 spec/models/spree/address_spec.rb | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/app/models/spree/address.rb b/app/models/spree/address.rb
index 9c305be78d..9d7cac881f 100644
--- a/app/models/spree/address.rb
+++ b/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? }
diff --git a/spec/models/spree/address_spec.rb b/spec/models/spree/address_spec.rb
index 41d0a28b15..c0b4f17234 100644
--- a/spec/models/spree/address_spec.rb
+++ b/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

From 4f851bbe1f5e3a518c25ad95691ae9a26ecf63c3 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Mon, 25 Mar 2024 11:47:58 +0300
Subject: [PATCH 3/8] Fix Rubocop: Do not delete dependent stock_movements

---
 app/models/spree/stock_item.rb       | 2 +-
 spec/models/spree/stock_item_spec.rb | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb
index 192a154b3d..6ac038beb6 100644
--- a/app/models/spree/stock_item.rb
+++ b/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: nil
 
     validates :stock_location, :variant, presence: true
     validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }
diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb
index d3b1b26bab..2cf29f9918 100644
--- a/spec/models/spree/stock_item_spec.rb
+++ b/spec/models/spree/stock_item_spec.rb
@@ -112,6 +112,10 @@ RSpec.describe Spree::StockItem do
       it "doesnt raise ReadOnlyRecord error" do
         expect { subject.destroy }.not_to raise_error
       end
+
+      it "does not destroy stock_movements when destroyed" do
+        expect { subject.destroy }.not_to change { Spree::StockMovement.count }
+      end
     end
   end
 end

From 4140257fa1e3ffa1b63efa53ae694276ecf9c81a Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Tue, 26 Mar 2024 09:54:51 +0300
Subject: [PATCH 4/8] Fix Rubocop: Do not delete dependent adjustments

TaxRate acts_as_paranoid iand is thus not hard_deleted
---
 app/models/spree/tax_rate.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb
index 62dade25b1..d7d91a7b73 100644
--- a/app/models/spree/tax_rate.rb
+++ b/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

From 645cb10864d1736113ed5d51290ec37797eb8ecb Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Tue, 26 Mar 2024 10:01:02 +0300
Subject: [PATCH 5/8] Fix Rubocop: Do not delete Spree::Variant associations

Spree::Variant acts_as_paranoid and is thus not hard deleted
---
 app/models/spree/variant.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb
index c5f93e0de9..294c1edfe0 100644
--- a/app/models/spree/variant.rb
+++ b/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

From c2cbe4f0bfaae10b5136f7d3ec16bbd77ca571a9 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Tue, 26 Mar 2024 10:57:40 +0300
Subject: [PATCH 6/8] Fix Rubocop: Hard delete paranoid associations

As much as the associated models act_as_paranoid, it
doesnt make sense to keep them around after deleting the enterprise
---
 app/models/enterprise.rb       | 15 +++++++++++++--
 spec/models/enterprise_spec.rb | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 5340aecddd..3c080e7fca 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -48,7 +48,7 @@ class Enterprise < ApplicationRecord
                                 dependent: :destroy
   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: nil # paranoid association is deleted in a before_destroy
   has_many :enterprise_roles, dependent: :destroy
   has_many :users, through: :enterprise_roles
   belongs_to :owner, class_name: 'Spree::User',
@@ -63,7 +63,7 @@ class Enterprise < ApplicationRecord
   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: nil # paranoid association is deleted in a before_destroy
   has_many :connected_apps, dependent: :destroy
   has_one :custom_tab, dependent: :destroy
 
@@ -130,6 +130,9 @@ class Enterprise < ApplicationRecord
 
   after_create :set_default_contact
   after_create :relate_to_owners_enterprises
+  before_destroy :delete_all_enterprise_fees
+  before_destroy :delete_all_vouchers
+
   after_rollback :restore_permalink
   after_touch :touch_distributors
   after_create_commit :send_welcome_email
@@ -587,4 +590,12 @@ class Enterprise < ApplicationRecord
       where.not(enterprises: { id: }).
       update_all(updated_at: Time.zone.now)
   end
+
+  def delete_all_enterprise_fees
+    enterprise_fees.each(&:really_destroy!)
+  end
+
+  def delete_all_vouchers
+    vouchers.each(&:really_destroy!)
+  end
 end
diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb
index 06eb7cfbb0..d21514ff8e 100644
--- a/spec/models/enterprise_spec.rb
+++ b/spec/models/enterprise_spec.rb
@@ -86,6 +86,26 @@ describe Enterprise do
       expect(DistributorShippingMethod.where(id: shipping_method_ids)).not_to exist
     end
 
+    it "destroys all enterprise_fees upon destroy" do
+      enterprise = create(:enterprise)
+      fee_ids = create_list(:enterprise_fee, 2, enterprise:).map(&:id)
+
+      expect(EnterpriseFee.where(id: fee_ids)).to exist
+      enterprise.destroy
+      expect(EnterpriseFee.where(id: fee_ids)).not_to exist
+    end
+
+    it "destroys all vouchers upon destroy" do
+      enterprise = create(:enterprise)
+      voucher_ids = (1..2).map do |code|
+        create(:voucher, enterprise:, code: "new code #{code}")
+      end.map(&:id)
+
+      expect(Voucher.where(id: voucher_ids)).to exist
+      enterprise.destroy
+      expect(Voucher.where(id: voucher_ids)).not_to exist
+    end
+
     describe "relationships to other enterprises" do
       let(:e) { create(:distributor_enterprise) }
       let(:p) { create(:supplier_enterprise) }

From 434afb73cd28777fff7c98636814cf852df4f0b2 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Wed, 3 Apr 2024 12:34:50 +0300
Subject: [PATCH 7/8] Fix Rubocop: Update handling of enterprise associations

---
 app/models/enterprise.rb       | 24 +++++--------
 spec/models/enterprise_spec.rb | 66 +++++++++++++++++++---------------
 2 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb
index 3c080e7fca..dd5a63a5fc 100644
--- a/app/models/enterprise.rb
+++ b/app/models/enterprise.rb
@@ -45,25 +45,29 @@ class Enterprise < ApplicationRecord
   has_many :supplied_variants, through: :supplied_products, source: :variants
   has_many :distributed_orders, class_name: 'Spree::Order',
                                 foreign_key: 'distributor_id',
-                                dependent: :destroy
+                                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, dependent: nil # paranoid association is deleted in a before_destroy
+  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, dependent: :destroy
+           inverse_of: :distributor,
+           foreign_key: :distributor_id,
+           dependent: :restrict_with_exception
   has_many :distributor_shipping_methods,
-           inverse_of: :distributor, foreign_key: :distributor_id, dependent: :destroy
+           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, dependent: nil # paranoid association is deleted in a before_destroy
+  has_many :vouchers, dependent: :restrict_with_exception
   has_many :connected_apps, dependent: :destroy
   has_one :custom_tab, dependent: :destroy
 
@@ -130,8 +134,6 @@ class Enterprise < ApplicationRecord
 
   after_create :set_default_contact
   after_create :relate_to_owners_enterprises
-  before_destroy :delete_all_enterprise_fees
-  before_destroy :delete_all_vouchers
 
   after_rollback :restore_permalink
   after_touch :touch_distributors
@@ -590,12 +592,4 @@ class Enterprise < ApplicationRecord
       where.not(enterprises: { id: }).
       update_all(updated_at: Time.zone.now)
   end
-
-  def delete_all_enterprise_fees
-    enterprise_fees.each(&:really_destroy!)
-  end
-
-  def delete_all_vouchers
-    vouchers.each(&:really_destroy!)
-  end
 end
diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb
index d21514ff8e..8fd7c29f1f 100644
--- a/spec/models/enterprise_spec.rb
+++ b/spec/models/enterprise_spec.rb
@@ -57,53 +57,61 @@ describe Enterprise do
       expect(EnterpriseRelationship.where(id: [er1, er2])).to be_empty
     end
 
-    it "destroys all distributed_orders upon destroy" do
+    it "raises a DeleteRestrictionError on destroy if distributed_orders exist" do
       enterprise = create(:distributor_enterprise)
-      order_ids = create_list(:order, 2, distributor: enterprise).map(&:id)
+      create_list(:order, 2, distributor: enterprise)
 
-      expect(Spree::Order.where(id: order_ids)).to exist
-      enterprise.destroy
-      expect(Spree::Order.where(id: order_ids)).not_to exist
+      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 "destroys all distributor_payment_methods upon destroy" do
+    it "raises an DeleteRestrictionError on destroy if distributor_payment_methods exist" do
       enterprise = create(:distributor_enterprise)
-      payment_method_ids = create_list(:distributor_payment_method, 2,
-                                       distributor: enterprise).map(&:id)
+      create_list(:distributor_payment_method, 2, distributor: enterprise)
 
-      expect(DistributorPaymentMethod.where(id: payment_method_ids)).to exist
-      enterprise.destroy
-      expect(DistributorPaymentMethod.where(id: payment_method_ids)).not_to exist
+      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 "destroys all distributor_shipping_methods upon destroy" do
-      enterprise = create(:enterprise)
-      shipping_method_ids = create_list(:distributor_shipping_method, 2,
-                                        distributor: enterprise).map(&:id)
+    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(DistributorShippingMethod.where(id: shipping_method_ids)).to exist
-      enterprise.destroy
-      expect(DistributorShippingMethod.where(id: shipping_method_ids)).not_to exist
+      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 "destroys all enterprise_fees upon destroy" do
+    it "does not destroy enterprise_fees upon destroy" do
       enterprise = create(:enterprise)
-      fee_ids = create_list(:enterprise_fee, 2, enterprise:).map(&:id)
+      create_list(:enterprise_fee, 2, enterprise:)
 
-      expect(EnterpriseFee.where(id: fee_ids)).to exist
-      enterprise.destroy
-      expect(EnterpriseFee.where(id: fee_ids)).not_to exist
+      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 "destroys all vouchers upon destroy" do
+    it "does not destroy vouchers upon destroy" do
       enterprise = create(:enterprise)
-      voucher_ids = (1..2).map do |code|
+      (1..2).map do |code|
         create(:voucher, enterprise:, code: "new code #{code}")
-      end.map(&:id)
+      end
 
-      expect(Voucher.where(id: voucher_ids)).to exist
-      enterprise.destroy
-      expect(Voucher.where(id: voucher_ids)).not_to exist
+      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

From 0d03cdf8159ed0e68c122a3a4b08769fccfacdb8 Mon Sep 17 00:00:00 2001
From: Anthony Musyoki <445103+anthonyms@users.noreply.github.com>
Date: Tue, 23 Apr 2024 12:47:38 +0300
Subject: [PATCH 8/8] Fix Rubocop: Delete dependent stock_movements

---
 .rubocop_todo.yml                        | 11 -----------
 app/models/spree/stock_item.rb           |  2 +-
 app/models/spree/stock_movement.rb       |  4 ----
 spec/models/spree/stock_item_spec.rb     |  6 +-----
 spec/models/spree/stock_movement_spec.rb |  7 -------
 5 files changed, 2 insertions(+), 28 deletions(-)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 3092909883..4f407763c9 100644
--- a/.rubocop_todo.yml
+++ b/.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
diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb
index 6ac038beb6..565a165e6f 100644
--- a/app/models/spree/stock_item.rb
+++ b/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, dependent: nil
+    has_many :stock_movements, dependent: :destroy
 
     validates :stock_location, :variant, presence: true
     validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }
diff --git a/app/models/spree/stock_movement.rb b/app/models/spree/stock_movement.rb
index 1cf432df5c..02352588e2 100644
--- a/app/models/spree/stock_movement.rb
+++ b/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
diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb
index 2cf29f9918..c3f94ef71a 100644
--- a/spec/models/spree/stock_item_spec.rb
+++ b/spec/models/spree/stock_item_spec.rb
@@ -109,12 +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
-      end
-
       it "does not destroy stock_movements when destroyed" do
-        expect { subject.destroy }.not_to change { Spree::StockMovement.count }
+        expect { subject.destroy }.to change { Spree::StockMovement.count }.by(-1)
       end
     end
   end
diff --git a/spec/models/spree/stock_movement_spec.rb b/spec/models/spree/stock_movement_spec.rb
index 95f0ea5564..251ed56e5c 100644
--- a/spec/models/spree/stock_movement_spec.rb
+++ b/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