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.

We can now use plugins, and require only local modules.

This commit changes `validate_presence_of :x` and friends to `validate :x, presence: true`. No functional changes here.

We should not use `puts` here, but instead the configured Rails logger. A log level of `info` is appropriate for all of these instances.

Prior to this commit, we had a before action populating the `current_user_roles` instance variable, and then we used that instance variable in helpers. This refactors it into an actual helper that we can then use as we need.

In the same spirit as the previous commit, which refactored `@current_user_roles` into a helper method, we do the same for the `current_api_user`.

This is to satisfy Rubocop, but it's also just good behavior. If a user record is destroyed, we should also destroy their address book. For the default addresses, we don't add cascading deleted, because these are also present in the `has_many :user_addresses`.

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.

Variant property rules belong to the product, if the product goes, the property rules can go as well. For the master variant and the non-master variants, we specify to do nothing when deleting the product, because they will be deleted from the `variants_including_master` association.

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 was forgotten when extracting the legacy_promotions gem. Also adds a dependent: :destroy.

When deleting a taxonomy, its root taxon will delete all taxons, so the taxons association does not have to do it. The has_many has the inverse for taxon already defined, so we won't define another inverse.

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.

to be renamed later

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.

mamhoff added 30 commits May 28, 2025 18:24

Remove Rubocop.yml deprecation warnings

4796d16

We can now use plugins, and require only local modules.

Update validation syntax

3d0df68

This commit changes `validate_presence_of :x` and friends to `validate :x, presence: true`. No functional changes here.

Replace puts in tasks and generators with Rails.logger

abfa512

We should not use `puts` here, but instead the configured Rails logger. A log level of `info` is appropriate for all of these instances.

Refactor load_user_roles into current_user_roles helper

b133f7f

Prior to this commit, we had a before action populating the `current_user_roles` instance variable, and then we used that instance variable in helpers. This refactors it into an actual helper that we can then use as we need.

Refactor "current_api_user" into instacached helper

3c629ad

In the same spirit as the previous commit, which refactored `@current_user_roles` into a helper method, we do the same for the `current_api_user`.

UserMethods: Add dependent/inverse_of to relations

e29db18

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

da5c067

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

Country: Add dependent/inverse_of options

eb8eb2a

CustomerReturn: Add dependent option to has_many's

b0fd9a1

Inventory Unit: Add inverse_of/dependent options

f3e9a4f

Credit Card: Specify inverse_of for :user association

907309e

Payment Method: Add :dependent options

8db61db

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

8588b90

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

5123e21

PermissionSet: Add dependent: :destroy

378e325

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

StockLocation: Add dependent: :restrict_with_error

5ada7c8

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

Store Credits: Add dependent: options

0715fa8

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

UserAddress: Add :inverse_of option

639c808

Variant: Add inverse_of and dependent options

35d104d

Price: Add inverse_of options

59e3cbd

Reimbursements and returns: Specify dependent/inverse_ofs

238d4aa

Shipment: Add dependent: :destroy

b40408d

Shipping Method: Add dependent: / inverse_of: options

c2a03e0

Stock Item: Add dependent: :destroy

8541702

Store: Add dependent: options

ddea5f5

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

889c751

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.

mamhoff added 7 commits May 28, 2025 18:24

Shipping Category: Add dependent: options

e96af0c

Move Taxon -> Promotion Rule association to legacy promotions

c853f3a

This was forgotten when extracting the legacy_promotions gem. Also adds a dependent: :destroy.

Taxonomy: Add dependent: false options

0079516

When deleting a taxonomy, its root taxon will delete all taxons, so the taxons association does not have to do it. The has_many has the inverse for taxon already defined, so we won't define another inverse.

Legacy Promotions: Add inverse_of/dependent options

61d17c6

Disable Rails/LexicallyScopedActionFilter

a51058c

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

Backend: Disable Rails/HelperInstanceVariable cop

d9c3f77

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

41c66d1

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

Promotions: Specify inverse_of/dependent options

998621e

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

mamhoff added 2 commits May 28, 2025 18:54

Fix taxon patch

6ca7f71

to be renamed later

Aktualisieren von spree_taxon_patch.rb

e1dc7b6

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

Open

Shipping category foreign keys #6278

Merged

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

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!