From 1cb3c8dfea2c8fcc14bedabde5b0d62489834eae Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 14:59:28 +0200 Subject: [PATCH 01/25] UserMethods: Add dependent/inverse_of to relations I've opted for dependent: :destroy for records that do not need to be saved for posterity in the scenario of completed orders, and for :nullify if those records do need to be saved. --- .../app/models/concerns/spree/user_methods.rb | 66 +++++++++++++++---- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/core/app/models/concerns/spree/user_methods.rb b/core/app/models/concerns/spree/user_methods.rb index 9218e26cd7a..e9cf4aaf2ff 100644 --- a/core/app/models/concerns/spree/user_methods.rb +++ b/core/app/models/concerns/spree/user_methods.rb @@ -11,20 +11,58 @@ module UserMethods included do extend Spree::DisplayMoney - has_many :role_users, foreign_key: "user_id", class_name: "Spree::RoleUser", dependent: :destroy - has_many :spree_roles, through: :role_users, source: :role, class_name: "Spree::Role" - - has_many :user_stock_locations, foreign_key: "user_id", class_name: "Spree::UserStockLocation" - has_many :stock_locations, through: :user_stock_locations - - has_many :spree_orders, foreign_key: "user_id", class_name: "Spree::Order" - has_many :orders, foreign_key: "user_id", class_name: "Spree::Order" - - has_many :store_credits, -> { includes(:credit_type) }, foreign_key: "user_id", class_name: "Spree::StoreCredit" - has_many :store_credit_events, through: :store_credits - - has_many :credit_cards, class_name: "Spree::CreditCard", foreign_key: :user_id - has_many :wallet_payment_sources, foreign_key: 'user_id', class_name: 'Spree::WalletPaymentSource', inverse_of: :user + has_many :role_users, + foreign_key: "user_id", + class_name: "Spree::RoleUser", + dependent: :destroy, + inverse_of: :user + has_many :spree_roles, + through: :role_users, + source: :role, + class_name: "Spree::Role", + inverse_of: :users + + has_many :user_stock_locations, + foreign_key: "user_id", + class_name: "Spree::UserStockLocation", + inverse_of: :user, + dependent: :destroy + has_many :stock_locations, + through: :user_stock_locations, + inverse_of: :users + + has_many :spree_orders, + foreign_key: "user_id", + class_name: "Spree::Order", + inverse_of: :user, + dependent: :nullify + has_many :orders, + foreign_key: "user_id", + class_name: "Spree::Order", + inverse_of: :user, + dependent: :nullify + + has_many :store_credits, + -> { includes(:credit_type) }, + foreign_key: "user_id", + class_name: "Spree::StoreCredit", + dependent: :nullify, + inverse_of: :user + has_many :store_credit_events, + through: :store_credits, + inverse_of: false + + has_many :credit_cards, + class_name: "Spree::CreditCard", + foreign_key: :user_id, + dependent: :nullify, + inverse_of: :user + + has_many :wallet_payment_sources, + foreign_key: 'user_id', + class_name: 'Spree::WalletPaymentSource', + inverse_of: :user, + dependent: :destroy after_create :auto_generate_spree_api_key before_destroy :check_for_deletion From 7a7bcc22a427157b6dd43d368f3183b367672476 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:02:52 +0200 Subject: [PATCH 02/25] AdjustmentReason: Add :dependent option We should not delete adjustment reasons if there are adjustments created with them. --- core/app/models/spree/adjustment_reason.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/adjustment_reason.rb b/core/app/models/spree/adjustment_reason.rb index 81d74c1044f..4198ccba768 100644 --- a/core/app/models/spree/adjustment_reason.rb +++ b/core/app/models/spree/adjustment_reason.rb @@ -2,7 +2,7 @@ module Spree class AdjustmentReason < Spree::Base - has_many :adjustments, inverse_of: :adjustment_reason + has_many :adjustments, inverse_of: :adjustment_reason, dependent: :restrict_with_exception validates :name, presence: true, uniqueness: { case_sensitive: false, allow_blank: true } validates :code, presence: true, uniqueness: { case_sensitive: false, allow_blank: true } From c1af03544eee3db21e268917cae27b114ef933df Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:07:36 +0200 Subject: [PATCH 03/25] Country: Add dependent/inverse_of options --- core/app/models/spree/country.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/country.rb b/core/app/models/spree/country.rb index d06fc3cd694..5a2370d60a7 100644 --- a/core/app/models/spree/country.rb +++ b/core/app/models/spree/country.rb @@ -2,9 +2,19 @@ module Spree class Country < Spree::Base - has_many :states, -> { order(:name) }, dependent: :destroy - has_many :addresses, dependent: :nullify - has_many :prices, class_name: "Spree::Price", foreign_key: "country_iso", primary_key: "iso" + has_many :states, + -> { order(:name) }, + dependent: :destroy, + inverse_of: :country + has_many :addresses, + dependent: :nullify, + inverse_of: false + has_many :prices, + class_name: "Spree::Price", + foreign_key: "country_iso", + primary_key: "iso", + dependent: :destroy, + inverse_of: :country validates :name, :iso_name, presence: true From 853af00ab7545195b86e2e874907bf3048894acc Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:09:24 +0200 Subject: [PATCH 04/25] CustomerReturn: Add dependent option to has_many's --- core/app/models/spree/customer_return.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/customer_return.rb b/core/app/models/spree/customer_return.rb index e9622f1db3a..3fbc3505f21 100644 --- a/core/app/models/spree/customer_return.rb +++ b/core/app/models/spree/customer_return.rb @@ -6,9 +6,14 @@ class CustomerReturn < Spree::Base belongs_to :stock_location, optional: true - has_many :return_items, inverse_of: :customer_return - has_many :return_authorizations, through: :return_items - has_many :reimbursements, inverse_of: :customer_return + has_many :return_items, + inverse_of: :customer_return, + dependent: :destroy + has_many :return_authorizations, + through: :return_items + has_many :reimbursements, + inverse_of: :customer_return, + dependent: :nullify after_create :process_return! before_create :generate_number From 6c3325d06a71afe2808a8168a676d7003cffc8e4 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:11:59 +0200 Subject: [PATCH 05/25] Inventory Unit: Add inverse_of/dependent options --- core/app/models/spree/inventory_unit.rb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/inventory_unit.rb b/core/app/models/spree/inventory_unit.rb index 80af85b3b89..f164b7025cd 100644 --- a/core/app/models/spree/inventory_unit.rb +++ b/core/app/models/spree/inventory_unit.rb @@ -13,10 +13,20 @@ class InventoryUnit < Spree::Base belongs_to :carton, class_name: "Spree::Carton", inverse_of: :inventory_units, optional: true belongs_to :line_item, class_name: "Spree::LineItem", inverse_of: :inventory_units, optional: true - has_many :return_items, inverse_of: :inventory_unit, dependent: :destroy - has_one :original_return_item, class_name: "Spree::ReturnItem", foreign_key: :exchange_inventory_unit_id, dependent: :destroy - has_one :unit_cancel, class_name: "Spree::UnitCancel" - has_one :order, through: :shipment + has_many :return_items, + inverse_of: :inventory_unit, + dependent: :destroy + has_one :original_return_item, + class_name: "Spree::ReturnItem", + foreign_key: :exchange_inventory_unit_id, + dependent: :destroy, + inverse_of: :exchange_inventory_unit + has_one :unit_cancel, + class_name: "Spree::UnitCancel", + inverse_of: :inventory_unit, + dependent: :restrict_with_exception + has_one :order, + through: :shipment delegate :order_id, to: :shipment From 568b2ce1f4742af0350200edef69c9b42d98e73e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:13:20 +0200 Subject: [PATCH 06/25] Credit Card: Specify inverse_of for :user association --- core/app/models/spree/credit_card.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/credit_card.rb b/core/app/models/spree/credit_card.rb index 479a31c687c..6450a92591d 100644 --- a/core/app/models/spree/credit_card.rb +++ b/core/app/models/spree/credit_card.rb @@ -4,7 +4,11 @@ module Spree # The default `source` of a `Spree::Payment`. # class CreditCard < Spree::PaymentSource - belongs_to :user, class_name: Spree::UserClassHandle.new, foreign_key: 'user_id', optional: true + belongs_to :user, + class_name: Spree::UserClassHandle.new, + foreign_key: 'user_id', + optional: true, + inverse_of: :credit_cards belongs_to :address, optional: true before_save :set_last_digits From 45145afd46da8da1193a92c9969bc3600d06838e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:15:04 +0200 Subject: [PATCH 07/25] Line Item: Specify dependent option for inventory units We already have a `before_save` callback that runs `inventory_units.destroy_all`. In order to save us all testing whether adding `dependent: :destroy` would be a subtle change in behavior, I just set the dependent: behavior to `false`. --- core/app/models/spree/line_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/line_item.rb b/core/app/models/spree/line_item.rb index a1b82f6d531..ab51717dc7b 100644 --- a/core/app/models/spree/line_item.rb +++ b/core/app/models/spree/line_item.rb @@ -19,7 +19,7 @@ class LineItem < Spree::Base has_one :product, through: :variant has_many :adjustments, as: :adjustable, inverse_of: :adjustable, dependent: :destroy - has_many :inventory_units, inverse_of: :line_item + has_many :inventory_units, inverse_of: :line_item, dependent: false before_validation :normalize_quantity before_validation :set_required_attributes From 8d188fadb0ec97a5da8bfa65486b7e6bad279e6f Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:20:00 +0200 Subject: [PATCH 08/25] Order: Add inverse_of/dependent options I opted for `dependent: false` for addresses, because Spree::Address records are immutable and therefore one user's address might give another user's orders, which would be strange (also Spree::Address, for this reason, does not have an `orders` association). `valid_store_credit_payments` can have a dependent: :destroy just like normal payments in the line above. Orders that have reimbursement should not be able to be destroyed, because semantically that can only happen once they have shipped. --- core/app/models/spree/order.rb | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 6f5e10b2a02..edf32437d19 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -75,11 +75,19 @@ class CannotRebuildShipments < StandardError; end # Customer info belongs_to :user, class_name: Spree::UserClassHandle.new, optional: true - belongs_to :bill_address, foreign_key: :bill_address_id, class_name: 'Spree::Address', optional: true + belongs_to :bill_address, + foreign_key: :bill_address_id, + class_name: 'Spree::Address', + optional: true, + inverse_of: false alias_method :billing_address, :bill_address alias_method :billing_address=, :bill_address= - belongs_to :ship_address, foreign_key: :ship_address_id, class_name: 'Spree::Address', optional: true + belongs_to :ship_address, + foreign_key: :ship_address_id, + class_name: 'Spree::Address', + optional: true, + inverse_of: false alias_method :shipping_address, :ship_address alias_method :shipping_address=, :ship_address= @@ -113,17 +121,22 @@ def states # Payments has_many :payments, dependent: :destroy, inverse_of: :order - has_many :valid_store_credit_payments, -> { store_credits.valid }, inverse_of: :order, class_name: 'Spree::Payment', foreign_key: :order_id + has_many :valid_store_credit_payments, + -> { store_credits.valid }, + inverse_of: :order, + class_name: 'Spree::Payment', + foreign_key: :order_id, + dependent: :destroy # Returns has_many :return_authorizations, dependent: :destroy, inverse_of: :order has_many :return_items, through: :inventory_units has_many :customer_returns, -> { distinct }, through: :return_items - has_many :reimbursements, inverse_of: :order + has_many :reimbursements, inverse_of: :order, dependent: :restrict_with_exception has_many :refunds, through: :payments # Logging - has_many :state_changes, as: :stateful + has_many :state_changes, as: :stateful, dependent: :destroy belongs_to :created_by, class_name: Spree::UserClassHandle.new, optional: true belongs_to :approver, class_name: Spree::UserClassHandle.new, optional: true belongs_to :canceler, class_name: Spree::UserClassHandle.new, optional: true From 8e71e4050d32d934c72367394d426e7005705a0b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 15:30:28 +0200 Subject: [PATCH 09/25] Payment Method: Add :dependent options If there are payments associated with the payment method, we can't just destroy them. We could :nullify, but I prefer that an error is raised and eventually handled. --- core/app/models/spree/payment_method.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/payment_method.rb b/core/app/models/spree/payment_method.rb index 73eca476d10..ddc79befbc1 100644 --- a/core/app/models/spree/payment_method.rb +++ b/core/app/models/spree/payment_method.rb @@ -23,9 +23,9 @@ class UnsupportedPaymentMethod < StandardError; end validates :name, :type, presence: true - has_many :payments, class_name: "Spree::Payment", inverse_of: :payment_method - has_many :credit_cards, class_name: "Spree::CreditCard" - has_many :store_payment_methods, inverse_of: :payment_method + has_many :payments, class_name: "Spree::Payment", inverse_of: :payment_method, dependent: :restrict_with_exception + has_many :credit_cards, class_name: "Spree::CreditCard", dependent: :restrict_with_exception + has_many :store_payment_methods, inverse_of: :payment_method, dependent: :destroy has_many :stores, through: :store_payment_methods scope :ordered_by_position, -> { order(:position) } From 5d8a9c36c060c704c8faceeb6224889d780c7176 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:04:17 +0200 Subject: [PATCH 10/25] Payment: Add dependent: :destroy When a payment is actually deleted, we should destroy the records that belong to it as well. This is very rarely the case, payments mostly get invalidated instead. --- core/app/models/spree/payment.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/payment.rb b/core/app/models/spree/payment.rb index 9f034f60ac7..953203a267f 100644 --- a/core/app/models/spree/payment.rb +++ b/core/app/models/spree/payment.rb @@ -17,10 +17,10 @@ class Payment < Spree::Base belongs_to :source, polymorphic: true, optional: true belongs_to :payment_method, -> { with_discarded }, class_name: 'Spree::PaymentMethod', inverse_of: :payments, optional: true - has_many :log_entries, as: :source - has_many :state_changes, as: :stateful - has_many :capture_events, class_name: 'Spree::PaymentCaptureEvent' - has_many :refunds, inverse_of: :payment + has_many :log_entries, as: :source, dependent: :destroy + has_many :state_changes, as: :stateful, dependent: :destroy + has_many :capture_events, class_name: 'Spree::PaymentCaptureEvent', dependent: :destroy + has_many :refunds, inverse_of: :payment, dependent: :destroy before_validation :validate_source, unless: :invalid? before_create :set_unique_identifier From 12472e57e3255c36858c51c5e4c4c01d67625e28 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:05:41 +0200 Subject: [PATCH 11/25] Payment Source: Add dependent: :restrict_with_error --- core/app/models/spree/payment_source.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/payment_source.rb b/core/app/models/spree/payment_source.rb index 0832fc5de76..2ec0bb86d7f 100644 --- a/core/app/models/spree/payment_source.rb +++ b/core/app/models/spree/payment_source.rb @@ -6,7 +6,7 @@ class PaymentSource < Spree::Base belongs_to :payment_method, optional: true - has_many :payments, as: :source + has_many :payments, as: :source, dependent: :restrict_with_error has_many :wallet_payment_sources, class_name: 'Spree::WalletPaymentSource', as: :payment_source, From 934f33b4521508807c995b7f3cbbd045ab1f277c Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:06:38 +0200 Subject: [PATCH 12/25] PermissionSet: Add dependent: :destroy When a permission set is destroyed, the join table entries to spree_roles should also be destroyed. --- core/app/models/spree/permission_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/permission_set.rb b/core/app/models/spree/permission_set.rb index 10b091c1d47..3d97423c14a 100644 --- a/core/app/models/spree/permission_set.rb +++ b/core/app/models/spree/permission_set.rb @@ -2,7 +2,7 @@ module Spree class PermissionSet < Spree::Base - has_many :role_permissions + has_many :role_permissions, dependent: :destroy has_many :roles, through: :role_permissions validates :name, :set, :privilege, :category, presence: true scope :display_permissions, -> { where(privilege: "display") } From 2dfd7cfc0a1427f802f850db11aa00c430ad521e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:07:56 +0200 Subject: [PATCH 13/25] StockLocation: Add dependent: :restrict_with_error Stock locations with cartons or shipments should not be destroyed, but rather set to "inactive". --- core/app/models/spree/stock_location.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/stock_location.rb b/core/app/models/spree/stock_location.rb index b575f99d3db..76369675720 100644 --- a/core/app/models/spree/stock_location.rb +++ b/core/app/models/spree/stock_location.rb @@ -9,9 +9,9 @@ class InvalidMovementError < StandardError; end acts_as_list - has_many :shipments + has_many :shipments, dependent: :restrict_with_error has_many :stock_items, dependent: :delete_all, inverse_of: :stock_location - has_many :cartons, inverse_of: :stock_location + has_many :cartons, inverse_of: :stock_location, dependent: :restrict_with_error has_many :stock_movements, through: :stock_items has_many :user_stock_locations, dependent: :delete_all has_many :users, through: :user_stock_locations From 2cc06387b6787086d4997e82c07805c1d2427a98 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:12:48 +0200 Subject: [PATCH 14/25] Store Credits: Add dependent: options This adds dependent: x options to models related to store credits. --- core/app/models/spree/store_credit.rb | 9 +++++++-- core/app/models/spree/store_credit_reason.rb | 2 +- core/app/models/spree/store_credit_type.rb | 6 +++++- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/store_credit.rb b/core/app/models/spree/store_credit.rb index 96de906af07..448fd694540 100644 --- a/core/app/models/spree/store_credit.rb +++ b/core/app/models/spree/store_credit.rb @@ -15,8 +15,13 @@ class Spree::StoreCredit < Spree::PaymentSource belongs_to :user, class_name: Spree::UserClassHandle.new, optional: true belongs_to :created_by, class_name: Spree::UserClassHandle.new, optional: true belongs_to :category, class_name: "Spree::StoreCreditCategory", optional: true - belongs_to :credit_type, class_name: 'Spree::StoreCreditType', foreign_key: 'type_id', optional: true - has_many :store_credit_events + belongs_to :credit_type, + class_name: 'Spree::StoreCreditType', + foreign_key: 'type_id', + optional: true, + inverse_of: :store_credits + has_many :store_credit_events, + dependent: :restrict_with_error validates :user_id, :category_id, :type_id, :created_by_id, :currency, presence: true validates :amount, numericality: { greater_than: 0 } diff --git a/core/app/models/spree/store_credit_reason.rb b/core/app/models/spree/store_credit_reason.rb index 66c3e1008df..147536daabf 100644 --- a/core/app/models/spree/store_credit_reason.rb +++ b/core/app/models/spree/store_credit_reason.rb @@ -6,7 +6,7 @@ class Spree::StoreCreditReason < Spree::Base validates :name, presence: true, uniqueness: { case_sensitive: false, allow_blank: true } - has_many :store_credit_events, inverse_of: :store_credit_reason + has_many :store_credit_events, inverse_of: :store_credit_reason, dependent: :restrict_with_error self.allowed_ransackable_attributes = %w[name] end diff --git a/core/app/models/spree/store_credit_type.rb b/core/app/models/spree/store_credit_type.rb index 4996c6e776f..a31b71405c0 100644 --- a/core/app/models/spree/store_credit_type.rb +++ b/core/app/models/spree/store_credit_type.rb @@ -5,6 +5,10 @@ class StoreCreditType < Spree::Base EXPIRING = 'Expiring' NON_EXPIRING = 'Non-expiring' DEFAULT_TYPE_NAME = EXPIRING - has_many :store_credits, class_name: 'Spree::StoreCredit', foreign_key: 'type_id' + has_many :store_credits, + class_name: 'Spree::StoreCredit', + foreign_key: 'type_id', + inverse_of: :credit_type, + dependent: :restrict_with_error end end From 3cc7865922410588f8ca9c1684a636a3da8f7f11 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:16:07 +0200 Subject: [PATCH 15/25] Variant: Add inverse_of and dependent options --- core/app/models/spree/variant.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/variant.rb b/core/app/models/spree/variant.rb index 958b50f3260..0d899392c8a 100644 --- a/core/app/models/spree/variant.rb +++ b/core/app/models/spree/variant.rb @@ -45,18 +45,23 @@ class Variant < Spree::Base delegate :tax_rates, to: :tax_category delegate :price_for_options, to: :price_selector - has_many :inventory_units, inverse_of: :variant - has_many :line_items, inverse_of: :variant + has_many :inventory_units, inverse_of: :variant, dependent: :restrict_with_error + has_many :line_items, inverse_of: :variant, dependent: :restrict_with_error has_many :orders, through: :line_items has_many :stock_items, dependent: :destroy, inverse_of: :variant has_many :stock_locations, through: :stock_items has_many :stock_movements, through: :stock_items - has_many :option_values_variants + has_many :option_values_variants, dependent: :destroy has_many :option_values, through: :option_values_variants - has_many :images, -> { order(:position) }, as: :viewable, dependent: :destroy, class_name: "Spree::Image" + has_many :images, + -> { order(:position) }, + as: :viewable, + dependent: :destroy, + class_name: "Spree::Image", + inverse_of: :viewable has_many :prices, -> { with_discarded }, From e150f91506379055195eaeca3cc88a288885f475 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:17:45 +0200 Subject: [PATCH 16/25] Price: Add inverse_of options --- core/app/models/spree/price.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/price.rb b/core/app/models/spree/price.rb index 36a957468db..5395c2961e0 100644 --- a/core/app/models/spree/price.rb +++ b/core/app/models/spree/price.rb @@ -6,8 +6,18 @@ class Price < Spree::Base MAXIMUM_AMOUNT = BigDecimal('99_999_999.99') - belongs_to :variant, -> { with_discarded }, class_name: 'Spree::Variant', touch: true, optional: true - belongs_to :country, class_name: "Spree::Country", foreign_key: "country_iso", primary_key: "iso", optional: true + belongs_to :variant, + -> { with_discarded }, + class_name: 'Spree::Variant', + touch: true, + optional: true, + inverse_of: :prices + belongs_to :country, + class_name: "Spree::Country", + foreign_key: "country_iso", + primary_key: "iso", + optional: true, + inverse_of: :prices delegate :product, to: :variant delegate :tax_rates, to: :variant From f5b5c75233900e019ff12109534653be557e571b Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:25:35 +0200 Subject: [PATCH 17/25] Reimbursements and returns: Specify dependent/inverse_ofs --- core/app/models/spree/refund.rb | 8 ++++++-- core/app/models/spree/refund_reason.rb | 2 +- core/app/models/spree/reimbursement.rb | 15 +++++++++++---- core/app/models/spree/reimbursement_type.rb | 2 +- core/app/models/spree/return_authorization.rb | 6 +++++- core/app/models/spree/return_item.rb | 6 +++++- core/app/models/spree/return_reason.rb | 2 +- 7 files changed, 30 insertions(+), 11 deletions(-) diff --git a/core/app/models/spree/refund.rb b/core/app/models/spree/refund.rb index 2727177c6a7..1f4dcad91d4 100644 --- a/core/app/models/spree/refund.rb +++ b/core/app/models/spree/refund.rb @@ -5,10 +5,14 @@ class Refund < Spree::Base include Metadata belongs_to :payment, inverse_of: :refunds, optional: true - belongs_to :reason, class_name: 'Spree::RefundReason', foreign_key: :refund_reason_id, optional: true + belongs_to :reason, + class_name: 'Spree::RefundReason', + foreign_key: :refund_reason_id, + optional: true, + inverse_of: :refunds belongs_to :reimbursement, inverse_of: :refunds, optional: true - has_many :log_entries, as: :source + has_many :log_entries, as: :source, dependent: :destroy validates :payment, presence: true validates :reason, presence: true diff --git a/core/app/models/spree/refund_reason.rb b/core/app/models/spree/refund_reason.rb index 285f1bee494..912539987dc 100644 --- a/core/app/models/spree/refund_reason.rb +++ b/core/app/models/spree/refund_reason.rb @@ -9,7 +9,7 @@ class RefundReason < Spree::Base RETURN_PROCESSING_REASON = 'Return processing' - has_many :refunds + has_many :refunds, dependent: :restrict_with_error self.allowed_ransackable_attributes = %w[name code] diff --git a/core/app/models/spree/reimbursement.rb b/core/app/models/spree/reimbursement.rb index 824f1edccf7..c25c1af5e2b 100644 --- a/core/app/models/spree/reimbursement.rb +++ b/core/app/models/spree/reimbursement.rb @@ -7,10 +7,17 @@ class IncompleteReimbursementError < StandardError; end belongs_to :order, inverse_of: :reimbursements, optional: true belongs_to :customer_return, inverse_of: :reimbursements, touch: true, optional: true - has_many :refunds, inverse_of: :reimbursement - has_many :credits, inverse_of: :reimbursement, class_name: 'Spree::Reimbursement::Credit' - - has_many :return_items, inverse_of: :reimbursement + has_many :refunds, + inverse_of: :reimbursement, + dependent: :restrict_with_error + has_many :credits, + inverse_of: :reimbursement, + class_name: 'Spree::Reimbursement::Credit', + dependent: :restrict_with_error + + has_many :return_items, + inverse_of: :reimbursement, + dependent: :restrict_with_error validates :order, presence: true validate :validate_return_items_belong_to_same_order diff --git a/core/app/models/spree/reimbursement_type.rb b/core/app/models/spree/reimbursement_type.rb index d0032f93bec..3e38d056276 100644 --- a/core/app/models/spree/reimbursement_type.rb +++ b/core/app/models/spree/reimbursement_type.rb @@ -9,7 +9,7 @@ class ReimbursementType < Spree::Base ORIGINAL = 'original' - has_many :return_items + has_many :return_items, dependent: :restrict_with_error self.allowed_ransackable_attributes = %w[name] diff --git a/core/app/models/spree/return_authorization.rb b/core/app/models/spree/return_authorization.rb index c37ec82c43c..a55bbb342af 100644 --- a/core/app/models/spree/return_authorization.rb +++ b/core/app/models/spree/return_authorization.rb @@ -13,7 +13,11 @@ class ReturnAuthorization < Spree::Base has_many :customer_returns, through: :return_items belongs_to :stock_location, optional: true - belongs_to :reason, class_name: 'Spree::ReturnReason', foreign_key: :return_reason_id, optional: true + belongs_to :reason, + class_name: 'Spree::ReturnReason', + foreign_key: :return_reason_id, + optional: true, + inverse_of: :return_authorizations before_create :generate_number diff --git a/core/app/models/spree/return_item.rb b/core/app/models/spree/return_item.rb index 2a2ec45c90a..b982d72282d 100644 --- a/core/app/models/spree/return_item.rb +++ b/core/app/models/spree/return_item.rb @@ -37,7 +37,11 @@ class ReturnItem < Spree::Base belongs_to :reimbursement, inverse_of: :return_items, optional: true belongs_to :preferred_reimbursement_type, class_name: 'Spree::ReimbursementType', optional: true belongs_to :override_reimbursement_type, class_name: 'Spree::ReimbursementType', optional: true - belongs_to :return_reason, class_name: 'Spree::ReturnReason', foreign_key: :return_reason_id, optional: true + belongs_to :return_reason, + class_name: 'Spree::ReturnReason', + foreign_key: :return_reason_id, + optional: true, + inverse_of: false validate :eligible_exchange_variant validate :belongs_to_same_customer_order diff --git a/core/app/models/spree/return_reason.rb b/core/app/models/spree/return_reason.rb index 1d485868586..b45813319a6 100644 --- a/core/app/models/spree/return_reason.rb +++ b/core/app/models/spree/return_reason.rb @@ -7,7 +7,7 @@ class ReturnReason < Spree::Base validates :name, presence: true, uniqueness: { case_sensitive: false, allow_blank: true } - has_many :return_authorizations + has_many :return_authorizations, dependent: :restrict_with_error self.allowed_ransackable_attributes = %w[name] From 31b2a036dc6a535fc5dbcd7c51db539d6060002e Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:26:20 +0200 Subject: [PATCH 18/25] Shipment: Add dependent: :destroy --- core/app/models/spree/shipment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index ce80b85ef51..349efb129f6 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -13,7 +13,7 @@ class Shipment < Spree::Base has_many :inventory_units, dependent: :destroy, inverse_of: :shipment has_many :shipping_rates, -> { order(:cost) }, dependent: :destroy, inverse_of: :shipment has_many :shipping_methods, through: :shipping_rates - has_many :state_changes, as: :stateful + has_many :state_changes, as: :stateful, dependent: :destroy has_many :cartons, -> { distinct }, through: :inventory_units has_many :line_items, -> { distinct }, through: :inventory_units From a51cb30bcf00bf5c69b1b791e599eb9016d8447c Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:31:09 +0200 Subject: [PATCH 19/25] Shipping Method: Add dependent: / inverse_of: options --- core/app/models/spree/shipping_method.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/shipping_method.rb b/core/app/models/spree/shipping_method.rb index 35ef3255e9e..77118f92c68 100644 --- a/core/app/models/spree/shipping_method.rb +++ b/core/app/models/spree/shipping_method.rb @@ -9,18 +9,23 @@ class ShippingMethod < Spree::Base has_many :shipping_method_categories, dependent: :destroy has_many :shipping_categories, through: :shipping_method_categories - has_many :shipping_rates, inverse_of: :shipping_method + has_many :shipping_rates, inverse_of: :shipping_method, dependent: :restrict_with_error has_many :shipments, through: :shipping_rates - has_many :cartons, inverse_of: :shipping_method + has_many :cartons, inverse_of: :shipping_method, dependent: :restrict_with_error has_many :shipping_method_zones, dependent: :destroy has_many :zones, through: :shipping_method_zones - belongs_to :tax_category, -> { with_discarded }, class_name: 'Spree::TaxCategory', optional: true + belongs_to :tax_category, + -> { with_discarded }, + class_name: 'Spree::TaxCategory', + optional: true, + inverse_of: false + has_many :shipping_method_stock_locations, dependent: :destroy, class_name: "Spree::ShippingMethodStockLocation" has_many :stock_locations, through: :shipping_method_stock_locations - has_many :store_shipping_methods, inverse_of: :shipping_method + has_many :store_shipping_methods, inverse_of: :shipping_method, dependent: :destroy has_many :stores, through: :store_shipping_methods validates :name, presence: true From 18f1f855694297fcb30faac211705e7d4afdced4 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:32:34 +0200 Subject: [PATCH 20/25] Stock Item: Add dependent: :destroy --- core/app/models/spree/stock_item.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/app/models/spree/stock_item.rb b/core/app/models/spree/stock_item.rb index 506cf5aa1fb..16cb89a6bb4 100644 --- a/core/app/models/spree/stock_item.rb +++ b/core/app/models/spree/stock_item.rb @@ -6,7 +6,7 @@ class StockItem < Spree::Base belongs_to :stock_location, class_name: 'Spree::StockLocation', inverse_of: :stock_items, optional: true belongs_to :variant, -> { with_discarded }, class_name: 'Spree::Variant', inverse_of: :stock_items, optional: true - has_many :stock_movements, inverse_of: :stock_item + has_many :stock_movements, inverse_of: :stock_item, dependent: :destroy validates :stock_location, :variant, presence: true validates :variant_id, uniqueness: { scope: [:stock_location_id, :deleted_at] }, allow_blank: true, unless: :deleted_at From 75e8cad326c49172b1e3e6b7401f6cdc2dc2f577 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:33:53 +0200 Subject: [PATCH 21/25] Store: Add dependent: options For join tables, dependent: :destroy is right. For orders, we restrict with an error - otherwise they orders would silently reassociate to the default store on the next save, which is probably not what stores want. --- core/app/models/spree/store.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/store.rb b/core/app/models/spree/store.rb index fd5aef8289c..77d5b9596e8 100644 --- a/core/app/models/spree/store.rb +++ b/core/app/models/spree/store.rb @@ -9,13 +9,13 @@ module Spree # hosted by a single Solidus implementation can be built. # class Store < Spree::Base - has_many :store_payment_methods, inverse_of: :store + has_many :store_payment_methods, inverse_of: :store, dependent: :destroy has_many :payment_methods, through: :store_payment_methods - has_many :store_shipping_methods, inverse_of: :store + has_many :store_shipping_methods, inverse_of: :store, dependent: :destroy has_many :shipping_methods, through: :store_shipping_methods - has_many :orders, class_name: "Spree::Order" + has_many :orders, class_name: "Spree::Order", dependent: :restrict_with_error validates :code, presence: true, uniqueness: { allow_blank: true, case_sensitive: true } validates :name, presence: true From 369b2a53b9519efccf5972e4f2fac9490a0cff05 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 17:37:04 +0200 Subject: [PATCH 22/25] Tax Rate: Add dependent: :restrict_with_error We should not delete tax rates that have adjustments. Otherwise we end up with invalid data on adjustments, or even adjustments changing their type, which would be BAD. --- core/app/models/spree/tax_rate.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/tax_rate.rb b/core/app/models/spree/tax_rate.rb index 19abc4c2953..14a55d36e03 100644 --- a/core/app/models/spree/tax_rate.rb +++ b/core/app/models/spree/tax_rate.rb @@ -27,8 +27,8 @@ class TaxRate < Spree::Base class_name: 'Spree::TaxCategory', inverse_of: :tax_rates - has_many :adjustments, as: :source - has_many :shipping_rate_taxes, class_name: "Spree::ShippingRateTax" + has_many :adjustments, as: :source, dependent: :restrict_with_error + has_many :shipping_rate_taxes, class_name: "Spree::ShippingRateTax", dependent: :restrict_with_error validates :amount, presence: true, numericality: true From 0104561519dc9fdfc22ac65633c9ecbf2e2c3339 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 18:00:23 +0200 Subject: [PATCH 23/25] Disable Rails/LexicallyScopedActionFilter This cop wants us to write ``` def edit; super; end ``` This, in turn, wakes another cop "Useless method definition". --- .rubocop.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index f18e53d3ea6..c1e5096915c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -348,6 +348,10 @@ Rails/FindEach: Exclude: - "db/migrate/**/*" +Rails/LexicallyScopedActionFilter: + Exclude: + - "backend/app/controllers/**/*" + # Since we're writing library code we can't assume that # tasks should load the rails environment loaded. Rails/RakeEnvironment: From ea1dee3f9b71fc73d1c8bf9eba6f96a04e3a74e7 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 18:05:02 +0200 Subject: [PATCH 24/25] Backend: Disable Rails/HelperInstanceVariable cop We do this all the time, and it's really time-consuming to fix. Also, no clear benefit. --- .rubocop.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index c1e5096915c..b53b3de3335 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -352,6 +352,11 @@ Rails/LexicallyScopedActionFilter: Exclude: - "backend/app/controllers/**/*" +Rails/HelperInstanceVariable: + Exclude: + - "backend/app/helpers/**/*" + - "core/app/helpers/**/*" + # Since we're writing library code we can't assume that # tasks should load the rails environment loaded. Rails/RakeEnvironment: From 52605d68ff4e6c237cc9838b1c10eddcbddf1404 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Wed, 28 May 2025 18:11:00 +0200 Subject: [PATCH 25/25] Ignore Rails/Exit warning in rescue clause This is fine. --- core/lib/generators/spree/custom_user/custom_user_generator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/lib/generators/spree/custom_user/custom_user_generator.rb b/core/lib/generators/spree/custom_user/custom_user_generator.rb index 5251a053c2d..d90724f19a5 100644 --- a/core/lib/generators/spree/custom_user/custom_user_generator.rb +++ b/core/lib/generators/spree/custom_user/custom_user_generator.rb @@ -15,7 +15,7 @@ def check_for_constant klass rescue NameError @shell.say "Couldn't find #{class_name}. Are you sure that this class exists within your application and is loaded?", :red - exit(1) + exit(1) # rubocop: disable Rails/Exit end def generate