Fix linting errors #6240

mamhoff · 2025-05-28T16:25:35Z

Summary

This fixes the linting errors introduced by yet another release of Rubocop.

I've chosen to actually do the inverse_of/dependent dance because it does improve the software.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I agree that my PR will be published under the same license as Solidus.
I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

tvdeyen

WOW 😲

Many great things in here, BUT "Fix linting errors" is an understatement 🤣

Most of these changes warrant a dedicated PR.

tvdeyen · 2025-05-29T07:34:04Z

core/app/models/concerns/spree/user_methods.rb

+        -> { includes(:credit_type) },
+        foreign_key: "user_id",
+        class_name: "Spree::StoreCredit",
+        dependent: :nullify,


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?

tvdeyen · 2025-05-29T07:36:30Z

api/app/controllers/spree/api/base_controller.rb

@@ -26,7 +26,8 @@ class BaseController < ActionController::Base
      before_action :load_user
      before_action :authorize_for_order, if: proc { order_token.present? }
      before_action :authenticate_user
-      before_action :load_user_roles
+      # This is deprecated and will be removed in Spree 5.0
+      before_action :load_deprecated_user_roles


This is a really great change. I would like that to appear as it's own PR in the changelog, though.

tvdeyen · 2025-05-29T07:36:50Z

api/app/controllers/spree/api/base_controller.rb

-      attr_accessor :current_api_user
-
-      before_action :load_user
+      before_action :deprecated_load_user


This as well is a really great change. I would like that to appear as it's own PR in the changelog, though.

tvdeyen · 2025-05-29T07:46:03Z

core/app/models/concerns/spree/user_methods.rb

+        foreign_key: "user_id",
+        class_name: "Spree::Order",
+        inverse_of: :user,
+        dependent: :nullify


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

tvdeyen · 2025-05-29T07:50:23Z

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


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.

tvdeyen · 2025-05-29T08:24:01Z

core/app/models/spree/refund.rb

    belongs_to :reimbursement, inverse_of: :refunds, optional: true

-    has_many :log_entries, as: :source
+    has_many :log_entries, as: :source, dependent: :destroy


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.

tvdeyen · 2025-05-29T08:40:30Z

core/app/models/spree/taxon.rb

@@ -13,9 +13,6 @@ class Taxon < Spree::Base
    has_many :classifications, -> { order(:position) }, dependent: :delete_all, inverse_of: :taxon
    has_many :products, through: :classifications

-    has_many :promotion_rule_taxons


This should be a separate PR, so we can back port it.

tvdeyen · 2025-05-29T08:43:29Z

.rubocop.yml

@@ -347,6 +347,10 @@ Rails/FindEach:
  Exclude:
    - "db/migrate/**/*"

+Rails/LexicallyScopedActionFilter:
+  Exclude:
+    - "backend/app/controllers/**/*"


Why not remove the empty useless methods instead?

tvdeyen · 2025-05-29T08:45:18Z

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


abort is probably a better way of exiting in a generator?

legacy_promotions/app/patches/models/solidus_legacy_promotions/spree_taxon_patch.rb

jarednorman · 2025-05-29T20:22:16Z

I looked through my notifications starting from the most recent. I was wondering where all these excellent little changes from @mamhoff came from... and now I know.

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.

We should not delete adjustment reasons if there are adjustments created with them.

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`.

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.

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.

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.

When a permission set is destroyed, the join table entries to spree_roles should also be destroyed.

Stock locations with cartons or shipments should not be destroyed, but rather set to "inactive".

This adds dependent: x options to models related to store credits.

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.

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.

This cop wants us to write ``` def edit; super; end ``` This, in turn, wakes another cop "Useless method definition".

We do this all the time, and it's really time-consuming to fix. Also, no clear benefit.

This is fine.

mamhoff requested a review from a team as a code owner May 28, 2025 16:25

github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_legacy_promotions Changes to the solidus_legacy_promotions gem changelog:solidus_promotions Changes to the solidus_promotions gem labels May 28, 2025

mamhoff force-pushed the fix-linting-errors branch from cb337c8 to 998621e Compare May 28, 2025 16:28

tvdeyen mentioned this pull request May 28, 2025

Regenerate .rubocop-todo.yml #6239

Merged

4 tasks

chaimann mentioned this pull request May 28, 2025

[Admin][Settings] Introduce zone creation and modification capability #6224

Merged

4 tasks

tvdeyen requested changes May 29, 2025

View reviewed changes

This was referenced May 30, 2025

Addressbook: Add foreign key, dependent/inverse_of options #6265

Merged

Add dependent/inverse_of options to product-related models #6271

Merged

Legacy Promotions: Add inverse_of/dependent options #6273

Merged

Shipping category foreign keys #6278

Merged

mamhoff marked this pull request as draft June 6, 2025 10:46

mamhoff added 11 commits July 2, 2025 10:52

UserMethods: Add dependent/inverse_of to relations

1cb3c8d

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.

AdjustmentReason: Add :dependent option

7a7bcc2

We should not delete adjustment reasons if there are adjustments created with them.

Country: Add dependent/inverse_of options

c1af035

CustomerReturn: Add dependent option to has_many's

853af00

Inventory Unit: Add inverse_of/dependent options

6c3325d

Credit Card: Specify inverse_of for :user association

568b2ce

Payment Method: Add :dependent options

8e71e40

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.

Payment: Add dependent: :destroy

5d8a9c3

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.

Payment Source: Add dependent: :restrict_with_error

12472e5

mamhoff added 14 commits July 2, 2025 10:52

PermissionSet: Add dependent: :destroy

934f33b

When a permission set is destroyed, the join table entries to spree_roles should also be destroyed.

StockLocation: Add dependent: :restrict_with_error

2dfd7cf

Stock locations with cartons or shipments should not be destroyed, but rather set to "inactive".

Store Credits: Add dependent: options

2cc0638

This adds dependent: x options to models related to store credits.

Variant: Add inverse_of and dependent options

3cc7865

Price: Add inverse_of options

e150f91

Reimbursements and returns: Specify dependent/inverse_ofs

f5b5c75

Shipment: Add dependent: :destroy

31b2a03

Shipping Method: Add dependent: / inverse_of: options

a51cb30

Stock Item: Add dependent: :destroy

18f1f85

Store: Add dependent: options

75e8cad

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.

Tax Rate: Add dependent: :restrict_with_error

369b2a5

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.

Disable Rails/LexicallyScopedActionFilter

0104561

This cop wants us to write ``` def edit; super; end ``` This, in turn, wakes another cop "Useless method definition".

Backend: Disable Rails/HelperInstanceVariable cop

ea1dee3

We do this all the time, and it's really time-consuming to fix. Also, no clear benefit.

Ignore Rails/Exit warning in rescue clause

52605d6

This is fine.

mamhoff force-pushed the fix-linting-errors branch from e1dc7b6 to 52605d6 Compare July 2, 2025 08:52

github-actions bot removed changelog:solidus_api Changes to the solidus_api gem changelog:solidus_legacy_promotions Changes to the solidus_legacy_promotions gem changelog:solidus_promotions Changes to the solidus_promotions gem labels Jul 2, 2025

This was referenced Jul 8, 2025

Restrict deletion of adjustment reasons if adjustments present #6303

Merged

Country and state: Restrict deletion if addresses present #6305

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix linting errors #6240

Fix linting errors #6240

Uh oh!

mamhoff commented May 28, 2025

Uh oh!

tvdeyen left a comment

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

tvdeyen May 29, 2025

Uh oh!

Uh oh!

jarednorman commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

Fix linting errors #6240

Are you sure you want to change the base?

Fix linting errors #6240

Uh oh!

Conversation

mamhoff commented May 28, 2025

Summary

Checklist

Uh oh!

tvdeyen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jarednorman commented May 29, 2025

Uh oh!

Uh oh!