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

Check conditions for presence validators to see if an attribute is required #693

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

donv
Copy link
Collaborator

@donv donv commented Aug 15, 2023

Fixes #691

@donv donv requested a review from lcreid August 15, 2023 07:07
@donv donv self-assigned this Aug 15, 2023
@donv donv added the Rails 7.1 label Aug 15, 2023
@donv donv merged commit 6a27593 into main Aug 15, 2023
24 checks passed
@donv donv deleted the required_attribute_with_condition branch August 15, 2023 10:16
@donv
Copy link
Collaborator Author

donv commented Aug 15, 2023

Merging this since main branch is broken.

Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Travis days, failures on edge weren't blocking failures. Maybe we have to look into that.

Thanks for jumping on this so quickly!

Comment on lines +43 to +45
if_option = validator.options[:if]
unless_opt = validator.options[:unless]
(!if_option || call_with_self(object, if_option)) && (!unless_opt || !call_with_self(object, unless_opt))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we now handle conditional validators, right?

Comment on lines +54 to +56
validator_class == ActiveModel::Validations::PresenceValidator ||
(defined?(ActiveRecord::Validations::PresenceValidator) &&
target_validators.include?(ActiveRecord::Validations::PresenceValidator))
validator_class == ActiveRecord::Validations::PresenceValidator)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never understood why the second part of the || is going to make a difference.

Comment on lines +7 to +11
if required
option[:required] = true
option[:aria] ||= {}
option[:aria][:required] = true
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rails 7.1] Association Validator Keyed by Association Name
2 participants