Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails/RedundantPresenceValidationOnBelongsTo should be marked as unsafe for auto-correct #615

Closed
TonyArra opened this issue Jan 10, 2022 · 2 comments · Fixed by #616
Closed

Comments

@TonyArra
Copy link
Contributor

TonyArra commented Jan 10, 2022

Is your feature request related to a problem? Please describe.

RedundantPresenceValidationOnBelongsTo is currently marked as safe for auto-correction. This is incorrect, as removing the validation changes the error message that is generated when the association is missing.

The default error message when using validates :foo, presence: true is "foo can't be blank".

The default error message for a required belongs_to :foo without the presence validation is "foo must exist".

This can break tests/behavior that rely on the error message.

Describe the solution you'd like

RedundantPresenceValidationOnBelongsTo should be marked as unsafe for auto-correct.

Describe alternatives you've considered

Additional context

Here's an example spec failure, using ThoughtBot's shoulda_matchers, after the auto-correction was applied:

     Failure/Error: it { is_expected.to validate_presence_of(:location) }

       Expected Category to validate that :location cannot be
       empty/falsy, but this could not be proved.
         After setting :location to ‹nil›, the matcher expected the
         Category to be invalid and to produce the validation error
         "can't be blank" on :location. The record was indeed invalid, but it
         produced these validation errors instead:

         * taxonomy: ["must exist"]

         You're getting this error because ActiveRecord is configured to add a
         presence validation to all `belongs_to` associations, and this
         includes yours. *This* presence validation doesn't use "can't be
         blank", the usual validation message, but "must exist" instead.

         With that said, did you know that the `belong_to` matcher can test
         this validation for you? Instead of using `validate_presence_of`, try
         the following instead:

             it { should belong_to(:location) }
@pirj
Copy link
Member

pirj commented Jan 10, 2022

I'm not convinced that validate_presence_of failure itself is indicative.
Just like validates ..., presence: true, it is redundant, as belong_to implies the required option.

As for the error message itself, "can't be blank" vs "must exist", I agree. It affects the error message shown to the end users, and changing it blindly may be problematic in some cases. E.g. when errors.messages.blank is properly translated to languages the project supports, while errors.messages.required only has the default English translation.

Would you like to send a PR to change the cop's SafeAutoCorrect setting to false?

@TonyArra
Copy link
Contributor Author

@pirj will do so shortly

TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Jan 10, 2022
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Jan 10, 2022
TonyArra added a commit to TonyArra/rubocop-rails that referenced this issue Jan 11, 2022
@koic koic closed this as completed in #616 Jan 11, 2022
koic added a commit that referenced this issue Jan 11, 2022
[Fix #615] Change Rails/RedundantPresenceValidationOnBelongsTo to SafeAutoCorrect: false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants