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/enterprise.rb b/app/models/enterprise.rb index 22a18473a9..dd5a63a5fc 100644 --- a/app/models/enterprise.rb +++ b/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 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/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 192a154b3d..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 + 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/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 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 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..8fd7c29f1f 100644 --- a/spec/models/enterprise_spec.rb +++ b/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) } 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 diff --git a/spec/models/spree/stock_item_spec.rb b/spec/models/spree/stock_item_spec.rb index d3b1b26bab..c3f94ef71a 100644 --- a/spec/models/spree/stock_item_spec.rb +++ b/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 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