-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Support passing record to uniqueness validator's :conditions option #39602
Support passing record to uniqueness validator's :conditions option #39602
Conversation
:conditions
option
Hi @eliotsykes.
|
Thanks @p8, these techniques are in use but not entirely desirable for this particular project. If this enhancement is a welcome addition, please say and I'll refine this and add test coverage. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This seems like a useful feature, analogous to how association scopes can access the record. The example in the description could be properly backed by an expression index. |
a29efcd
to
443ec45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a changelog entry and squash your commits?
conditions = options[:conditions] | ||
|
||
relation = if conditions.arity.zero? | ||
relation.merge(conditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since options[:conditions]
can only be a callable, I think we should skip the indirection via Relation#merge
and call instance_exec
here directly, so it's more obvious how this branch differs from the one below:
relation.merge(conditions) | |
relation.instance_exec(&conditions) |
# validates_uniqueness_of :slug, | ||
# conditions: ->(record) { | ||
# where(published_at: record.published_at.beginning_of_day..record.published_at.end_of_day) | ||
# } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example should be incorporated into the API documentation instead, probably here.
|
||
today_midday = Time.current.midday | ||
|
||
todays_topic = Topic.create!(title: "Highlights of the Day", written_on: today_midday) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuboCop is complaining because this variable is assigned but unused.
If you want to keep it as documentation, you'll need to add a leading underscore:
todays_topic = Topic.create!(title: "Highlights of the Day", written_on: today_midday) | |
_todays_topic = Topic.create!(title: "Highlights of the Day", written_on: today_midday) |
31f1a7f
to
ab20be8
Compare
Thanks for the feedback @eugeneius, I think this is ready now. |
Thank you @eliotsykes! 🙌 |
Nice work @eliotsykes ! 👏 👏 |
Support passing record to uniqueness validator's
:conditions
option.This allows building conditions based on the record's attributes.
E.g. To validate slug must be unique for the day of publication:
If this is a welcome addition, please say and I'll refine this and add test coverage.