-
Notifications
You must be signed in to change notification settings - Fork 184
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
Added name argument to validators for ticket validations #10572
base: main
Are you sure you want to change the base?
Conversation
@@ -41,22 +41,22 @@ def include_persons? | |||
true | |||
end | |||
|
|||
def self.dob_validations(dob, competition_id = nil, name = nil) | |||
def self.dob_validations(dob, message_args = {}, competition_id = nil) |
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.
I think you can make it more Ruby-esque by something like...
def self.dob_validations(dob, message_args = {}, competition_id = nil) | |
def self.dob_validations(dob, competition_id = nil, **message_args) |
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.
If you take this suggestion, then below you can "spread" the parameters into the ValidationIssue
class as well: ValidationWarning.new(:persons, competition_id, DOB_0101_WARNING, **message_args)
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.
Okay done.
@@ -101,7 +101,7 @@ def run_validation(validator_data) | |||
name: p.name) | |||
end | |||
|
|||
PersonsValidator.dob_validations(p.dob, competition.id, p.name).each do |validation| | |||
PersonsValidator.dob_validations(p.dob, { name: p.name }, competition.id).each do |validation| |
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.
For my suggestion above, this could be changed to PersonsValidator.dob_validations(p.dob, competition.id, name: p.name)
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.
Done
@@ -49,7 +49,7 @@ def edit_person_validators | |||
ticket.metadata.tickets_edit_person_fields.each do |edit_person_field| | |||
case edit_person_field[:field_name] | |||
when TicketsEditPersonField.field_names[:dob] | |||
dob_validation_issues = ResultsValidators::PersonsValidator.dob_validations(Date.parse(edit_person_field[:new_value])) | |||
dob_validation_issues = ResultsValidators::PersonsValidator.dob_validations(Date.parse(edit_person_field[:new_value]), { name: ticket.metadata.wca_id }) |
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.
If you like my other suggestion about the method signature, then this can become...
dob_validation_issues = ResultsValidators::PersonsValidator.dob_validations(Date.parse(edit_person_field[:new_value]), { name: ticket.metadata.wca_id }) | |
dob_validation_issues = ResultsValidators::PersonsValidator.dob_validations(Date.parse(edit_person_field[:new_value]), name: ticket.metadata.wca_id) |
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.
Sidenote: You should probably extract the Date.parse
into its own line, helps with debugging :)
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.
Done
One drawback of this is still js cannot replace "%{name}" with name. So, this doesn't fix the UI issue. This should be solved when moved to i18n. This PR aims at sending
name
parameter fordob_validations
method.