Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1cb3c8d
UserMethods: Add dependent/inverse_of to relations
mamhoff May 28, 2025
7a7bcc2
AdjustmentReason: Add :dependent option
mamhoff May 28, 2025
c1af035
Country: Add dependent/inverse_of options
mamhoff May 28, 2025
853af00
CustomerReturn: Add dependent option to has_many's
mamhoff May 28, 2025
6c3325d
Inventory Unit: Add inverse_of/dependent options
mamhoff May 28, 2025
568b2ce
Credit Card: Specify inverse_of for :user association
mamhoff May 28, 2025
45145af
Line Item: Specify dependent option for inventory units
mamhoff May 28, 2025
8d188fa
Order: Add inverse_of/dependent options
mamhoff May 28, 2025
8e71e40
Payment Method: Add :dependent options
mamhoff May 28, 2025
5d8a9c3
Payment: Add dependent: :destroy
mamhoff May 28, 2025
12472e5
Payment Source: Add dependent: :restrict_with_error
mamhoff May 28, 2025
934f33b
PermissionSet: Add dependent: :destroy
mamhoff May 28, 2025
2dfd7cf
StockLocation: Add dependent: :restrict_with_error
mamhoff May 28, 2025
2cc0638
Store Credits: Add dependent: options
mamhoff May 28, 2025
3cc7865
Variant: Add inverse_of and dependent options
mamhoff May 28, 2025
e150f91
Price: Add inverse_of options
mamhoff May 28, 2025
f5b5c75
Reimbursements and returns: Specify dependent/inverse_ofs
mamhoff May 28, 2025
31b2a03
Shipment: Add dependent: :destroy
mamhoff May 28, 2025
a51cb30
Shipping Method: Add dependent: / inverse_of: options
mamhoff May 28, 2025
18f1f85
Stock Item: Add dependent: :destroy
mamhoff May 28, 2025
75e8cad
Store: Add dependent: options
mamhoff May 28, 2025
369b2a5
Tax Rate: Add dependent: :restrict_with_error
mamhoff May 28, 2025
0104561
Disable Rails/LexicallyScopedActionFilter
mamhoff May 28, 2025
ea1dee3
Backend: Disable Rails/HelperInstanceVariable cop
mamhoff May 28, 2025
52605d6
Ignore Rails/Exit warning in rescue clause
mamhoff May 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,15 @@ Rails/FindEach:
Exclude:
- "db/migrate/**/*"

Rails/LexicallyScopedActionFilter:
Exclude:
- "backend/app/controllers/**/*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not remove the empty useless methods instead?


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:
Expand Down
66 changes: 52 additions & 14 deletions core/app/models/concerns/spree/user_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is also very welcome, but warrants its own PR.

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be totally fine for deactivated users to actually lose their store credit as well, but since store credit might be used for past payments as source we cannot do that, is that right?

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
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/adjustment_reason.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using restrict_with_error instead? So that we can display the error message on the record we try do delete instead of rendering a 500 in the admin. Otherwise we would need to rescue_from ActiveRecord::DeleteRestrictionError in the backend.


validates :name, presence: true, uniqueness: { case_sensitive: false, allow_blank: true }
validates :code, presence: true, uniqueness: { case_sensitive: false, allow_blank: true }
Expand Down
16 changes: 13 additions & 3 deletions core/app/models/spree/country.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 5 additions & 1 deletion core/app/models/spree/credit_card.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for historical reasons? Credit cards should never be associated directly to a user, but through their wallet payment source, right?

Should we deprecate this association? cc @solidusio/core-team

belongs_to :address, optional: true

before_save :set_last_digits
Expand Down
11 changes: 8 additions & 3 deletions core/app/models/spree/customer_return.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions core/app/models/spree/inventory_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/line_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Maybe worth adding a comment here and point to the before_destroy :destroy_inventory_units method?


before_validation :normalize_quantity
before_validation :set_required_attributes
Expand Down
23 changes: 18 additions & 5 deletions core/app/models/spree/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did not added a dependent: false here, although mentioned in the comment.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. You did not added a dependent: false here, although mentioned in the comment.

alias_method :shipping_address, :ship_address
alias_method :shipping_address=, :ship_address=

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do that, can we then also remove all store credit records when we delete a user (see previous comment on UserMethods)?


# 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
Expand Down
8 changes: 4 additions & 4 deletions core/app/models/spree/payment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so sure about this one. Refunds are associated with reimbursements and we won't delete those, right?
We should raise an exception I think.


before_validation :validate_source, unless: :invalid?
before_create :set_unique_identifier
Expand Down
6 changes: 3 additions & 3 deletions core/app/models/spree/payment_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we could also use restrict_with_error instead and display a nice error to the user, right?

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) }
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/payment_source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. Why you opted for restrict_with_error here, although there is no UI associated with payment sources (that I am aware of)? I think here we actually should raise an exception

has_many :wallet_payment_sources,
class_name: 'Spree::WalletPaymentSource',
as: :payment_source,
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/permission_set.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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") }
Expand Down
14 changes: 12 additions & 2 deletions core/app/models/spree/price.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions core/app/models/spree/refund.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this 🤔 . Actually I do not think refunds should ever be allowed to be destroyed. They are kind of legal records. I know that this just makes sure that if we delete a refund the log entry is removed as well, but I am not so sure about this as well.


validates :payment, presence: true
validates :reason, presence: true
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/refund_reason.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
15 changes: 11 additions & 4 deletions core/app/models/spree/reimbursement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/reimbursement_type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
6 changes: 5 additions & 1 deletion core/app/models/spree/return_authorization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 5 additions & 1 deletion core/app/models/spree/return_item.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/return_reason.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
Loading
Loading