Correct "required" asterisk when using validation option :on
#1872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I want to add some new cases here, and I find that the current
form of these specs makes things difficult:
* They depend on artificial validations ("unless false") in the
example app.
* These artificial validations are mystery guests.
* Adding new variants is not scalable, as the example app only has so
many models.
So I'm moving specs to a new location where they can be expressed in a
more low-level setting, making them easier to setup and work with.
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| v.class == ActiveRecord::Validations::PresenceValidator && | ||
| !v.options.include?(:if) && | ||
| !v.options.include?(:unless) | ||
| next false unless v.class == ActiveRecord::Validations::PresenceValidator |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [83/80]
pablobm
changed the title
:on
|
I like this change! I think we should — and so I am going to — merge it in as is. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1860
We show a "required" asterisk when fields are required. However there are some edge cases, like when the requirement has an if/unless. We don't consider those actually required and we don't show the asterisk there.
But there are other edge cases. Issue #1860 shows how the validation option
:oncan also create cases where an apparent requirement is not actually a requirement. Normally this option has the values:createor:update, but there are other possibilities.This PR tries to solve the issue. There are three commits, each an incremental improvement:
:onto be the same as:ifand:unless, and hide the asterisk.:if,:unless, and:on. This also allows for more scalable specs, should we decide to add more fine-grained handling of these options.on: :createandon: :update, and hide the asterisk for any other:onvalue.So we could go just with the first commit, or the first two, or the whole thing. Or more than that! Opinions?