From 53f5d63e4ad917a6190a2e057b78930493c0736d Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Sun, 20 Aug 2023 23:16:00 +0900 Subject: [PATCH 01/16] Delete Rails/HasManyOrHasOneDependent TODOs --- .rubocop_todo.yml | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b38b39daec..799cbba4fc 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -418,48 +418,7 @@ Naming/VariableNumber: - 'spec/models/spree/tax_rate_spec.rb' - 'spec/requests/api/orders_spec.rb' -# Offense count: 11 -# This cop supports unsafe autocorrection (--autocorrect-all). -# Configuration parameters: AllowedMethods, AllowedPatterns. -# AllowedMethods: order, limit, select, lock -Rails/FindEach: - Exclude: - - 'app/controllers/admin/order_cycles_controller.rb' - - 'app/jobs/subscription_confirm_job.rb' - - 'app/services/orders_bulk_cancel_service.rb' - - 'app/services/products_renderer.rb' - - 'lib/tasks/data.rake' - - 'lib/tasks/subscriptions/debug.rake' - - 'spec/system/admin/bulk_order_management_spec.rb' - - 'spec/system/admin/enterprise_relationships_spec.rb' -# Offense count: 46 -# Configuration parameters: Include. -# Include: app/models/**/*.rb -Rails/HasManyOrHasOneDependent: - Exclude: - - 'app/models/customer.rb' - - 'app/models/enterprise.rb' - - 'app/models/order_cycle.rb' - - 'app/models/spree/address.rb' - - 'app/models/spree/adjustment.rb' - - 'app/models/spree/country.rb' - - 'app/models/spree/credit_card.rb' - - 'app/models/spree/order.rb' - - 'app/models/spree/payment.rb' - - 'app/models/spree/payment_method.rb' - - 'app/models/spree/property.rb' - - 'app/models/spree/return_authorization.rb' - - 'app/models/spree/shipment.rb' - - 'app/models/spree/shipping_category.rb' - - 'app/models/spree/shipping_method.rb' - - 'app/models/spree/stock_item.rb' - - 'app/models/spree/tax_rate.rb' - - 'app/models/spree/taxonomy.rb' - - 'app/models/spree/user.rb' - - 'app/models/spree/variant.rb' - -# Offense count: 25 # Configuration parameters: Include. # Include: app/helpers/**/*.rb Rails/HelperInstanceVariable: From b76fb10d466b7112aa6abc2a1d9721f9cf039132 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Wed, 30 Aug 2023 09:32:03 +0900 Subject: [PATCH 02/16] Fix Rails/HasManyOrHasOneDependent with Destroy --- app/models/customer.rb | 2 +- app/models/enterprise.rb | 19 ++++++++++--------- app/models/order_cycle.rb | 12 ++++++++---- app/models/spree/adjustment.rb | 2 +- app/models/spree/country.rb | 2 +- app/models/spree/order.rb | 6 +++--- app/models/spree/payment_method.rb | 2 +- app/models/spree/property.rb | 2 +- app/models/spree/shipment.rb | 2 +- app/models/spree/shipping_category.rb | 2 +- app/models/spree/shipping_method.rb | 8 ++++---- app/models/spree/stock_item.rb | 2 +- app/models/spree/tax_rate.rb | 2 +- app/models/spree/user.rb | 4 ++-- app/models/spree/variant.rb | 12 ++++++------ 15 files changed, 42 insertions(+), 37 deletions(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 732c15c1ff..10f171e2b1 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -16,7 +16,7 @@ class Customer < ApplicationRecord belongs_to :enterprise belongs_to :user, class_name: "Spree::User", optional: true - has_many :orders, class_name: "Spree::Order" + has_many :orders, class_name: "Spree::Order", dependent: :destroy before_validation :downcase_email before_validation :empty_code before_create :associate_user diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index a7c68b0e4b..a75d417ecf 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -37,31 +37,32 @@ class Enterprise < ApplicationRecord dependent: :destroy has_and_belongs_to_many :groups, join_table: 'enterprise_groups_enterprises', class_name: 'EnterpriseGroup' - has_many :producer_properties, foreign_key: 'producer_id' + has_many :producer_properties, foreign_key: 'producer_id', dependent: :destroy has_many :properties, through: :producer_properties has_many :supplied_products, class_name: 'Spree::Product', 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 + has_many :enterprise_fees, dependent: :destroy 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: :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 - has_many :inventory_items - has_many :tag_rules + 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: :destroy has_one :custom_tab, dependent: :destroy delegate :latitude, :longitude, :city, :state_name, to: :address diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index d9f8205ef3..b6e7d80913 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -11,7 +11,7 @@ class OrderCycle < ApplicationRecord belongs_to :coordinator, class_name: 'Enterprise' - has_many :coordinator_fee_refs, class_name: 'CoordinatorFee' + has_many :coordinator_fee_refs, class_name: 'CoordinatorFee', dependent: :destroy has_many :coordinator_fees, through: :coordinator_fee_refs, source: :enterprise_fee, dependent: :destroy @@ -19,12 +19,16 @@ class OrderCycle < ApplicationRecord # These scope names are prepended with "cached_" because there are existing accessor methods # :incoming_exchanges and :outgoing_exchanges. - has_many :cached_incoming_exchanges, -> { where incoming: true }, class_name: "Exchange" - has_many :cached_outgoing_exchanges, -> { where incoming: false }, class_name: "Exchange" + has_many :cached_incoming_exchanges, -> { + where incoming: true + }, class_name: "Exchange", dependent: :destroy + has_many :cached_outgoing_exchanges, -> { + where incoming: false + }, class_name: "Exchange", dependent: :destroy has_many :suppliers, -> { distinct }, source: :sender, through: :cached_incoming_exchanges has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges - has_many :order_cycle_schedules + has_many :order_cycle_schedules, dependent: :destroy has_many :schedules, through: :order_cycle_schedules has_and_belongs_to_many :selected_distributor_payment_methods, class_name: 'DistributorPaymentMethod', diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index f07201539c..a1ffcc86c7 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -36,7 +36,7 @@ module Spree # Deletion of metadata is handled in the database. # So we don't need the option `dependent: :destroy` as long as # AdjustmentMetadata has no destroy logic itself. - has_one :metadata, class_name: 'AdjustmentMetadata' + has_one :metadata, class_name: 'AdjustmentMetadata', dependent: :destroy has_many :adjustments, as: :adjustable, dependent: :destroy belongs_to :adjustable, polymorphic: true diff --git a/app/models/spree/country.rb b/app/models/spree/country.rb index 6647b8d698..cc9e3dbed9 100644 --- a/app/models/spree/country.rb +++ b/app/models/spree/country.rb @@ -2,7 +2,7 @@ module Spree class Country < ApplicationRecord - has_many :states, -> { order('name ASC') } + has_many :states, -> { order('name ASC') }, dependent: :destroy validates :name, :iso_name, presence: true diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 64c5aa60c0..807371feed 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -45,7 +45,7 @@ module Spree belongs_to :ship_address, class_name: 'Spree::Address' alias_attribute :shipping_address, :ship_address - has_many :state_changes, as: :stateful + has_many :state_changes, as: :stateful, dependent: :destroy has_many :line_items, -> { order('created_at ASC') }, class_name: "Spree::LineItem", dependent: :destroy @@ -71,12 +71,12 @@ module Spree }, class_name: 'Spree::Adjustment', dependent: :destroy - has_many :invoices + has_many :invoices, dependent: :destroy belongs_to :order_cycle belongs_to :distributor, class_name: 'Enterprise' belongs_to :customer - has_one :proxy_order + has_one :proxy_order, dependent: :destroy has_one :subscription, through: :proxy_order accepts_nested_attributes_for :line_items diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index 891f418eed..f3011b222c 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -15,7 +15,7 @@ module Spree DISPLAY = [:both, :back_end].freeze default_scope -> { where(deleted_at: nil) } - has_many :credit_cards, class_name: "Spree::CreditCard" + has_many :credit_cards, class_name: "Spree::CreditCard", dependent: :destroy validates :name, presence: true validate :distributor_validation diff --git a/app/models/spree/property.rb b/app/models/spree/property.rb index 3762c3b393..9f45270a78 100644 --- a/app/models/spree/property.rb +++ b/app/models/spree/property.rb @@ -4,7 +4,7 @@ module Spree class Property < ApplicationRecord has_many :product_properties, dependent: :destroy has_many :products, through: :product_properties - has_many :producer_properties + has_many :producer_properties, dependent: :destroy validates :name, :presentation, presence: true diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 1f528bc4a8..476658b13a 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -12,7 +12,7 @@ module Spree has_many :shipping_rates, dependent: :delete_all has_many :shipping_methods, through: :shipping_rates - has_many :state_changes, as: :stateful + has_many :state_changes, as: :stateful, dependent: :destroy has_many :inventory_units, dependent: :delete_all has_many :adjustments, as: :adjustable, dependent: :destroy diff --git a/app/models/spree/shipping_category.rb b/app/models/spree/shipping_category.rb index 6bbfb7f603..4f452ee95c 100644 --- a/app/models/spree/shipping_category.rb +++ b/app/models/spree/shipping_category.rb @@ -4,7 +4,7 @@ module Spree class ShippingCategory < ApplicationRecord validates :name, presence: true has_many :products - has_many :shipping_method_categories, inverse_of: :shipping_method + has_many :shipping_method_categories, inverse_of: :shipping_method, dependent: :destroy has_many :shipping_methods, through: :shipping_method_categories end end diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index d84cac35a8..1110892590 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -13,11 +13,11 @@ module Spree default_scope -> { where(deleted_at: nil) } - has_many :shipping_rates, inverse_of: :shipping_method + has_many :shipping_rates, inverse_of: :shipping_method, dependent: :destroy has_many :shipments, through: :shipping_rates - has_many :shipping_method_categories - has_many :shipping_categories, through: :shipping_method_categories - has_many :distributor_shipping_methods + has_many :shipping_method_categories, dependent: :destroy + has_many :shipping_categories, through: :shipping_method_categories, dependent: :destroy + has_many :distributor_shipping_methods, dependent: :destroy has_many :distributors, through: :distributor_shipping_methods, class_name: 'Enterprise', foreign_key: 'distributor_id' 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/tax_rate.rb b/app/models/spree/tax_rate.rb index 62dade25b1..852e155906 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: :destroy validates :amount, presence: true, numericality: true validates :tax_category, presence: true diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 84b2d56919..f00b3c758b 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -36,8 +36,8 @@ module Spree foreign_key: :owner_id, inverse_of: :owner has_many :owned_groups, class_name: 'EnterpriseGroup', foreign_key: :owner_id, inverse_of: :owner - has_many :customers - has_many :credit_cards + has_many :customers, dependent: :destroy + has_many :credit_cards, dependent: :destroy has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy has_many :webhook_endpoints, dependent: :destroy diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 3ca3317aa8..0e9bbd5a1d 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -33,12 +33,12 @@ 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: :destroy + has_many :line_items, inverse_of: :variant, dependent: :destroy has_many :stock_items, dependent: :destroy, inverse_of: :variant has_many :stock_locations, through: :stock_items - has_many :stock_movements + has_many :stock_movements, dependent: :destroy has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" @@ -54,10 +54,10 @@ module Spree delegate :display_price, :display_amount, :price, :price=, :currency, :currency=, to: :find_or_build_default_price - has_many :exchange_variants + has_many :exchange_variants, dependent: :destroy has_many :exchanges, through: :exchange_variants - has_many :variant_overrides - has_many :inventory_items + has_many :variant_overrides, dependent: :destroy + has_many :inventory_items, dependent: :destroy localize_number :price, :weight From 825342914dd585c7204d16d657f31a4112b76fcb Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Thu, 24 Aug 2023 00:04:28 +0900 Subject: [PATCH 03/16] Fix Rails/HasManyOrHasOneDependent with nullify --- app/models/spree/address.rb | 2 +- app/models/spree/credit_card.rb | 2 +- app/models/spree/payment.rb | 2 +- app/models/spree/return_authorization.rb | 4 ++-- app/models/spree/shipping_category.rb | 2 +- app/models/spree/taxonomy.rb | 2 +- app/models/spree/user.rb | 4 ++-- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/spree/address.rb b/app/models/spree/address.rb index 703babb4ee..6ff007e62d 100644 --- a/app/models/spree/address.rb +++ b/app/models/spree/address.rb @@ -13,7 +13,7 @@ module Spree belongs_to :state, class_name: "Spree::State" has_one :enterprise, dependent: :restrict_with_exception - has_many :shipments + has_many :shipments, dependent: :nullify validates :address1, :city, :country, :phone, presence: true validates :company, presence: true, unless: -> { first_name.blank? || last_name.blank? } diff --git a/app/models/spree/credit_card.rb b/app/models/spree/credit_card.rb index 529f657014..c4ec4f0967 100644 --- a/app/models/spree/credit_card.rb +++ b/app/models/spree/credit_card.rb @@ -7,7 +7,7 @@ module Spree belongs_to :payment_method belongs_to :user - has_many :payments, as: :source + has_many :payments, as: :source, dependent: :nullify before_save :set_last_digits diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 51d19d72ad..4bb1bd7304 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -21,7 +21,7 @@ module Spree belongs_to :payment_method, class_name: 'Spree::PaymentMethod' has_many :offsets, -> { where("source_type = 'Spree::Payment' AND amount < 0").completed }, - class_name: "Spree::Payment", foreign_key: :source_id + class_name: "Spree::Payment", foreign_key: :source_id, dependent: :nullify has_many :log_entries, as: :source, dependent: :destroy has_one :adjustment, as: :adjustable, dependent: :destroy diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index 61a9e219ae..69858ccf48 100644 --- a/app/models/spree/return_authorization.rb +++ b/app/models/spree/return_authorization.rb @@ -8,8 +8,8 @@ module Spree belongs_to :order, class_name: 'Spree::Order', inverse_of: :return_authorizations - has_many :inventory_units, inverse_of: :return_authorization - has_one :stock_location + has_many :inventory_units, inverse_of: :return_authorization, dependent: :nullify + has_one :stock_location, dependent: :nullify before_save :force_positive_amount before_create :generate_number diff --git a/app/models/spree/shipping_category.rb b/app/models/spree/shipping_category.rb index 4f452ee95c..deec192c8f 100644 --- a/app/models/spree/shipping_category.rb +++ b/app/models/spree/shipping_category.rb @@ -3,7 +3,7 @@ module Spree class ShippingCategory < ApplicationRecord validates :name, presence: true - has_many :products + has_many :products, dependent: :nullify has_many :shipping_method_categories, inverse_of: :shipping_method, dependent: :destroy has_many :shipping_methods, through: :shipping_method_categories end diff --git a/app/models/spree/taxonomy.rb b/app/models/spree/taxonomy.rb index 50778e37cf..a8600caa27 100644 --- a/app/models/spree/taxonomy.rb +++ b/app/models/spree/taxonomy.rb @@ -4,7 +4,7 @@ module Spree class Taxonomy < ApplicationRecord validates :name, presence: true - has_many :taxons + has_many :taxons, dependent: :nullify has_one :root, -> { where parent_id: nil }, class_name: "Spree::Taxon", dependent: :destroy after_save :set_name diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index f00b3c758b..38b2e94580 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -33,9 +33,9 @@ module Spree has_many :enterprise_roles, dependent: :destroy has_many :enterprises, through: :enterprise_roles has_many :owned_enterprises, class_name: 'Enterprise', - foreign_key: :owner_id, inverse_of: :owner + foreign_key: :owner_id, inverse_of: :owner, dependent: :nullify has_many :owned_groups, class_name: 'EnterpriseGroup', - foreign_key: :owner_id, inverse_of: :owner + foreign_key: :owner_id, inverse_of: :owner, dependent: :nullify has_many :customers, dependent: :destroy has_many :credit_cards, dependent: :destroy has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy From 070d2cb855dbb91dca61dcac0fb99f8c48ac32b5 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Fri, 25 Aug 2023 00:00:34 +0900 Subject: [PATCH 04/16] Fix stock_movements relation in variant --- app/models/spree/variant.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 0e9bbd5a1d..01c174766e 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -38,7 +38,7 @@ module Spree has_many :stock_items, dependent: :destroy, inverse_of: :variant has_many :stock_locations, through: :stock_items - has_many :stock_movements, dependent: :destroy + has_many :stock_movements, through: :stock_items, dependent: :destroy has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" From 019b54ea95cf91a5f3de31eb1e8d83ecb50c4295 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Fri, 25 Aug 2023 00:01:55 +0900 Subject: [PATCH 05/16] Fix Rails/HasManyOrHasOneDependent with delete_all --- app/models/spree/stock_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/stock_item.rb b/app/models/spree/stock_item.rb index 565a165e6f..f28cfe7f65 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: :destroy + has_many :stock_movements, dependent: :delete_all # delete_all required to avoid ReadOnlyError validates :stock_location, :variant, presence: true validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] } From 7f8ac949338a1c65b204c9eb316a5ceb3bd034ca Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Wed, 30 Aug 2023 09:33:26 +0900 Subject: [PATCH 06/16] Fix Rails/HasManyOrHasOneDependent with nil --- app/models/spree/adjustment.rb | 2 +- app/models/spree/tax_rate.rb | 2 +- app/models/spree/user.rb | 2 +- app/models/spree/variant.rb | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index a1ffcc86c7..7625b34962 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -36,7 +36,7 @@ module Spree # Deletion of metadata is handled in the database. # So we don't need the option `dependent: :destroy` as long as # AdjustmentMetadata has no destroy logic itself. - has_one :metadata, class_name: 'AdjustmentMetadata', dependent: :destroy + has_one :metadata, class_name: 'AdjustmentMetadata', dependent: nil has_many :adjustments, as: :adjustable, dependent: :destroy belongs_to :adjustable, polymorphic: true diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index 852e155906..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, dependent: :destroy + has_many :adjustments, as: :originator, dependent: nil validates :amount, presence: true, numericality: true validates :tax_category, presence: true diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 38b2e94580..8f19bc4e5d 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -14,7 +14,7 @@ module Spree encryptor: 'authlogic_sha512', reconfirmable: true, omniauth_providers: [:openid_connect] - has_many :orders + has_many :orders, dependent: nil belongs_to :ship_address, class_name: 'Spree::Address' belongs_to :bill_address, class_name: 'Spree::Address' diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 01c174766e..4fccf0161d 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -33,8 +33,8 @@ module Spree delegate :name, :name=, :description, :description=, :meta_keywords, to: :product - has_many :inventory_units, inverse_of: :variant, dependent: :destroy - has_many :line_items, inverse_of: :variant, dependent: :destroy + 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 From 921347f0f24caccfdf4bb60cfce67803d7711bbc Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Thu, 24 Aug 2023 08:42:09 +0900 Subject: [PATCH 07/16] Update .rubocop_todo.yml --- .rubocop_todo.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 799cbba4fc..1796de338c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,12 +6,13 @@ # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 1 +# Offense count: 2 # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: Max, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns. # URISchemes: http, https Layout/LineLength: Exclude: + - 'spec/controllers/admin/reports_controller_spec.rb' - 'spec/system/admin/tag_rules_spec.rb' # Offense count: 17 @@ -723,6 +724,12 @@ Rails/WhereExists: - 'lib/tasks/sample_data/order_cycle_factory.rb' - 'lib/tasks/sample_data/taxon_factory.rb' +# Offense count: 3 +# This cop supports safe autocorrection (--autocorrect). +Rails/WhereNot: + Exclude: + - 'lib/reporting/reports/users_and_enterprises/base.rb' + # Offense count: 1 Security/Open: Exclude: From 7c9bc58a4b2433d828117f233f7b4a2f5eba6ce1 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Fri, 1 Sep 2023 09:21:26 +0900 Subject: [PATCH 08/16] Add nullify to orders relationship, instead of manually nullifying --- app/models/customer.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 10f171e2b1..a60c0d8013 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -16,7 +16,7 @@ class Customer < ApplicationRecord belongs_to :enterprise belongs_to :user, class_name: "Spree::User", optional: true - has_many :orders, class_name: "Spree::Order", dependent: :destroy + has_many :orders, class_name: "Spree::Order", dependent: :nullify before_validation :downcase_email before_validation :empty_code before_create :associate_user @@ -70,6 +70,5 @@ class Customer < ApplicationRecord throw :abort end Subscription.where(customer_id: id).destroy_all - orders.update_all(customer_id: nil) end end From e22e08e666566c91d96cfa9dbcb56623454a90cd Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Fri, 1 Sep 2023 09:22:04 +0900 Subject: [PATCH 09/16] Change destroy to nullify --- app/models/enterprise.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index a75d417ecf..d9d41f5d91 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -44,10 +44,10 @@ class Enterprise < ApplicationRecord dependent: :destroy has_many :supplied_variants, through: :supplied_products, source: :variants has_many :distributed_orders, class_name: 'Spree::Order', foreign_key: 'distributor_id', - dependent: :destroy + dependent: :nullify belongs_to :address, class_name: 'Spree::Address' belongs_to :business_address, optional: true, class_name: 'Spree::Address', dependent: :destroy - has_many :enterprise_fees, dependent: :destroy + has_many :enterprise_fees, dependent: :nullify has_many :enterprise_roles, dependent: :destroy has_many :users, through: :enterprise_roles belongs_to :owner, class_name: 'Spree::User', From 180cd4abe6cd76c6a4ab06c5be23c8f5fe0429cf Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Thu, 7 Sep 2023 16:34:07 +0900 Subject: [PATCH 10/16] Revert Complicated Rails/HasManyOrHasOneDependent Errors --- app/models/enterprise.rb | 11 +++++------ app/models/spree/address.rb | 2 +- app/models/spree/stock_item.rb | 2 +- app/models/spree/tax_rate.rb | 2 +- app/models/spree/variant.rb | 6 +++--- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index d9d41f5d91..bd9aedbc93 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -43,26 +43,25 @@ 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', - dependent: :nullify + has_many :distributed_orders, class_name: 'Spree::Order', foreign_key: 'distributor_id' belongs_to :address, class_name: 'Spree::Address' belongs_to :business_address, optional: true, class_name: 'Spree::Address', dependent: :destroy - has_many :enterprise_fees, dependent: :nullify + has_many :enterprise_fees 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 has_many :distributor_shipping_methods, - inverse_of: :distributor, foreign_key: :distributor_id, dependent: :destroy + inverse_of: :distributor, foreign_key: :distributor_id 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: :destroy + has_many :vouchers has_one :custom_tab, dependent: :destroy delegate :latitude, :longitude, :city, :state_name, to: :address diff --git a/app/models/spree/address.rb b/app/models/spree/address.rb index 6ff007e62d..703babb4ee 100644 --- a/app/models/spree/address.rb +++ b/app/models/spree/address.rb @@ -13,7 +13,7 @@ module Spree belongs_to :state, class_name: "Spree::State" has_one :enterprise, dependent: :restrict_with_exception - has_many :shipments, dependent: :nullify + has_many :shipments validates :address1, :city, :country, :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 f28cfe7f65..192a154b3d 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: :delete_all # delete_all required to avoid ReadOnlyError + has_many :stock_movements validates :stock_location, :variant, presence: true validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] } diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index d7d91a7b73..62dade25b1 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, dependent: nil + has_many :adjustments, as: :originator 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 4fccf0161d..da462602b0 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -33,8 +33,8 @@ module Spree delegate :name, :name=, :description, :description=, :meta_keywords, to: :product - has_many :inventory_units, inverse_of: :variant, dependent: nil - has_many :line_items, inverse_of: :variant, dependent: nil + has_many :inventory_units, inverse_of: :variant + has_many :line_items, inverse_of: :variant has_many :stock_items, dependent: :destroy, inverse_of: :variant has_many :stock_locations, through: :stock_items @@ -54,7 +54,7 @@ module Spree delegate :display_price, :display_amount, :price, :price=, :currency, :currency=, to: :find_or_build_default_price - has_many :exchange_variants, dependent: :destroy + has_many :exchange_variants has_many :exchanges, through: :exchange_variants has_many :variant_overrides, dependent: :destroy has_many :inventory_items, dependent: :destroy From 2520d222bbb2bc0cf0ddb1bd4cab386dc44ecc5e Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Thu, 7 Sep 2023 11:07:41 +0900 Subject: [PATCH 11/16] Fix Rails/HasManyOrHasDependent with restrict_with_exception --- app/models/spree/user.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb index 8f19bc4e5d..fce0828b4c 100644 --- a/app/models/spree/user.rb +++ b/app/models/spree/user.rb @@ -33,9 +33,11 @@ module Spree has_many :enterprise_roles, dependent: :destroy has_many :enterprises, through: :enterprise_roles has_many :owned_enterprises, class_name: 'Enterprise', - foreign_key: :owner_id, inverse_of: :owner, dependent: :nullify + foreign_key: :owner_id, inverse_of: :owner, + dependent: :restrict_with_exception has_many :owned_groups, class_name: 'EnterpriseGroup', - foreign_key: :owner_id, inverse_of: :owner, dependent: :nullify + foreign_key: :owner_id, inverse_of: :owner, + dependent: :restrict_with_exception has_many :customers, dependent: :destroy has_many :credit_cards, dependent: :destroy has_many :report_rendering_options, class_name: "::ReportRenderingOptions", dependent: :destroy From d295a3bdaeba280485fdf03192c2f3f15d65f0f7 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Tue, 3 Oct 2023 09:49:49 +0900 Subject: [PATCH 12/16] Update .rubocop_todo.yml for Reversions --- .rubocop_todo.yml | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 1796de338c..31339ee76b 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -12,7 +12,7 @@ # URISchemes: http, https Layout/LineLength: Exclude: - - 'spec/controllers/admin/reports_controller_spec.rb' + - 'app/models/spree/payment.rb' - 'spec/system/admin/tag_rules_spec.rb' # Offense count: 17 @@ -419,7 +419,33 @@ Naming/VariableNumber: - 'spec/models/spree/tax_rate_spec.rb' - 'spec/requests/api/orders_spec.rb' +# Offense count: 11 +# This cop supports unsafe autocorrection (--autocorrect-all). +# Configuration parameters: AllowedMethods, AllowedPatterns. +# AllowedMethods: order, limit, select, lock +Rails/FindEach: + Exclude: + - 'app/controllers/admin/order_cycles_controller.rb' + - 'app/jobs/subscription_confirm_job.rb' + - 'app/services/orders_bulk_cancel_service.rb' + - 'app/services/products_renderer.rb' + - 'lib/tasks/data.rake' + - 'lib/tasks/subscriptions/debug.rake' + - 'spec/system/admin/bulk_order_management_spec.rb' + - 'spec/system/admin/enterprise_relationships_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: 25 # Configuration parameters: Include. # Include: app/helpers/**/*.rb Rails/HelperInstanceVariable: @@ -724,12 +750,6 @@ Rails/WhereExists: - 'lib/tasks/sample_data/order_cycle_factory.rb' - 'lib/tasks/sample_data/taxon_factory.rb' -# Offense count: 3 -# This cop supports safe autocorrection (--autocorrect). -Rails/WhereNot: - Exclude: - - 'lib/reporting/reports/users_and_enterprises/base.rb' - # Offense count: 1 Security/Open: Exclude: @@ -785,11 +805,6 @@ Style/ClassAndModuleChildren: - 'spec/controllers/spree/admin/base_controller_spec.rb' - 'spec/models/spree/payment_method_spec.rb' -# Offense count: 1 -Style/ClassVars: - Exclude: - - 'lib/spree/core/delegate_belongs_to.rb' - # Offense count: 2 # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: MaxUnannotatedPlaceholdersAllowed, AllowedMethods, AllowedPatterns. @@ -838,13 +853,14 @@ Style/HashLikeCase: Exclude: - 'app/models/enterprise.rb' -# Offense count: 356 +# Offense count: 184 # This cop supports safe autocorrection (--autocorrect). # Configuration parameters: EnforcedStyle, EnforcedShorthandSyntax, UseHashRocketsWithSymbolValues, PreferHashRocketsForNonAlnumEndingSymbols. # SupportedStyles: ruby19, hash_rockets, no_mixed_keys, ruby19_no_mixed_keys # SupportedShorthandSyntax: always, never, either, consistent Style/HashSyntax: Exclude: + - 'spec/services/voucher_adjustments_service_spec.rb' - 'spec/system/admin/reports_spec.rb' - 'spec/system/admin/tag_rules_spec.rb' - 'spec/system/admin/variant_overrides_spec.rb' @@ -924,7 +940,7 @@ Style/OpenStructUse: - 'spec/lib/reports/users_and_enterprises_report_spec.rb' - 'spec/serializers/api/enterprise_serializer_spec.rb' -# Offense count: 16 +# Offense count: 15 # Configuration parameters: AllowedMethods. # AllowedMethods: respond_to_missing? Style/OptionalBooleanParameter: @@ -938,7 +954,6 @@ Style/OptionalBooleanParameter: - 'app/models/spree/shipment.rb' - 'engines/order_management/app/services/order_management/stock/estimator.rb' - 'lib/spree/core/controller_helpers/order.rb' - - 'lib/spree/core/delegate_belongs_to.rb' - 'spec/support/request/web_helper.rb' # Offense count: 1 From e49489cd1c826b8e3a3fd0f6d8b9e9510ffd913e Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Tue, 19 Sep 2023 09:10:06 +0900 Subject: [PATCH 13/16] Fix invoices dependent relationship --- app/models/spree/order.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 807371feed..68ca179bdc 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -71,7 +71,7 @@ module Spree }, class_name: 'Spree::Adjustment', dependent: :destroy - has_many :invoices, dependent: :destroy + has_many :invoices, dependent: :restrict_with_exception belongs_to :order_cycle belongs_to :distributor, class_name: 'Enterprise' From cf07a055d01d053329f23f9b5dfeeaccaf799ba4 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Thu, 21 Sep 2023 23:24:48 +0900 Subject: [PATCH 14/16] Fix Rails/HasManyOrHasOneDependent with nil --- app/models/spree/shipping_method.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 1110892590..fea4601d9e 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -16,7 +16,7 @@ module Spree has_many :shipping_rates, inverse_of: :shipping_method, dependent: :destroy has_many :shipments, through: :shipping_rates has_many :shipping_method_categories, dependent: :destroy - has_many :shipping_categories, through: :shipping_method_categories, dependent: :destroy + has_many :shipping_categories, through: :shipping_method_categories, dependent: nil has_many :distributor_shipping_methods, dependent: :destroy has_many :distributors, through: :distributor_shipping_methods, class_name: 'Enterprise', From fec59e5ae2e7f4df5605d9bfa4e385e5ae4d6e81 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Tue, 3 Oct 2023 09:49:24 +0900 Subject: [PATCH 15/16] Apply Changes Suggested by Code Review --- app/models/spree/payment.rb | 2 +- app/models/spree/shipping_method.rb | 2 +- app/models/spree/variant.rb | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 4bb1bd7304..6f32de6f9b 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -21,7 +21,7 @@ module Spree belongs_to :payment_method, class_name: 'Spree::PaymentMethod' has_many :offsets, -> { where("source_type = 'Spree::Payment' AND amount < 0").completed }, - class_name: "Spree::Payment", foreign_key: :source_id, dependent: :nullify + class_name: "Spree::Payment", foreign_key: :source_id, dependent: :restrict_with_exception has_many :log_entries, as: :source, dependent: :destroy has_one :adjustment, as: :adjustable, dependent: :destroy diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index fea4601d9e..42ab253ef7 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -16,7 +16,7 @@ module Spree has_many :shipping_rates, inverse_of: :shipping_method, dependent: :destroy has_many :shipments, through: :shipping_rates has_many :shipping_method_categories, dependent: :destroy - has_many :shipping_categories, through: :shipping_method_categories, dependent: nil + has_many :shipping_categories, through: :shipping_method_categories has_many :distributor_shipping_methods, dependent: :destroy has_many :distributors, through: :distributor_shipping_methods, class_name: 'Enterprise', diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index da462602b0..db87d93d74 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -38,7 +38,6 @@ module Spree has_many :stock_items, dependent: :destroy, inverse_of: :variant has_many :stock_locations, through: :stock_items - has_many :stock_movements, through: :stock_items, dependent: :destroy has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" From cd34e160bf6588b8d554547caa4c7b4bd49efe0e Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 3 Oct 2023 14:52:08 +1100 Subject: [PATCH 16/16] Fix deleting of return_authorizations return_authorizations have a stock_location_id, not the other way round. So there's no dependent field to nullify. --- app/models/spree/return_authorization.rb | 2 +- .../admin/return_authorizations_controller_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index 69858ccf48..ad291f2bf3 100644 --- a/app/models/spree/return_authorization.rb +++ b/app/models/spree/return_authorization.rb @@ -9,7 +9,7 @@ module Spree belongs_to :order, class_name: 'Spree::Order', inverse_of: :return_authorizations has_many :inventory_units, inverse_of: :return_authorization, dependent: :nullify - has_one :stock_location, dependent: :nullify + has_one :stock_location, dependent: nil before_save :force_positive_amount before_create :generate_number diff --git a/spec/controllers/spree/admin/return_authorizations_controller_spec.rb b/spec/controllers/spree/admin/return_authorizations_controller_spec.rb index aaf7bc81d6..d8b0a1ca42 100644 --- a/spec/controllers/spree/admin/return_authorizations_controller_spec.rb +++ b/spec/controllers/spree/admin/return_authorizations_controller_spec.rb @@ -33,6 +33,18 @@ module Spree expect(return_authorization.amount.to_s).to eq "10.2" expect(return_authorization.reason.to_s).to eq "half broken" end + + context "with a return authorization" do + let!(:return_authorization) { create(:return_authorization, order:) } + + it "deletes a return authorization" do + expect{ + spree_delete :destroy, id: return_authorization.id, order_id: order.number + }.to change { order.return_authorizations.without_deleted.count }.by(-1) + + expect(response).to redirect_to spree.admin_order_return_authorizations_url(order.number) + end + end end end end