Add ID to validation_issue #10521
Add ID to validation_issue #10521
Conversation
| @@ -46,17 +46,17 @@ def self.dob_validations(dob, competition_id = nil, name = nil) | |||
|
|
|||
| # Check if DOB is January 1 | |||
| if dob.month == 1 && dob.day == 1 | |||
| validation_issues << ValidationWarning.new(:persons, competition_id, DOB_0101_WARNING, name: name) | |||
| validation_issues << ValidationWarning.new(:dob_jan_one, :persons, competition_id, DOB_0101_WARNING, name: name) | |||
There was a problem hiding this comment.
@gregorbg This is my initial plan for adding IDs. I think it will be better if the IDs are fetched from an enum-like object instead of directly adding the ID here. What do you think?
There was a problem hiding this comment.
Well, what exactly are you suggesting? I mean, the idea sounds nice, so what kind of "enum-like" structure are you thinking about?
There was a problem hiding this comment.
I've implemented that - I feel that is more easier than typing. Can you please check the implementation now? Can I go ahead with the same for all definitions or do you have any feedbacks before making change in other definitions?
danieljames-dj
force-pushed
the
validation_id
branch
from
876cab1 to
af0c4cd
Compare
| VALIDATION_TYPES = VALIDATION_TYPES_KEYS.each_with_object({}) do |key, hash| | ||
| hash[key] = key.to_s | ||
| end.freeze |
There was a problem hiding this comment.
I am definitely sure that I told you this trick before, but I'll tell you again:
my_list.index_with { |list_elem| list_elem.to_s } or even shorter my_list.index_with(&:to_s)
There was a problem hiding this comment.
Yes, I remember you said the trick, but I didn't remembered it from top of my head so just went ahead with copilot suggestion for time being. Truly I had plan to optimize it later.
Thanks for giving the shorthand again, I've added them now.
There was a problem hiding this comment.
Hmm... I get what you're going for, and I like the "type safety" kinda idea behind your approach.
However the actual implementation is extremely verbose. Basically you have to spell out a meaningless ValidationIssue::VALIDATION_TYPES every time you build a validation.
(And the "turn the array of keys into a hash that holds its own keys as values" also feels somewhat awkward. Again, I totally get your idea, but the implentation is a bit cumbersome to use)
There was a problem hiding this comment.
Did you try consulting ChatGPT (or Copilot or Gemini or whatever) for some ideas?
From the top of my head (haven't thought this through in detail, just spontaneous) you could try passing only the :dob_jan_one constant as a parameter, and have the initialize method in the validation warning check whether that's actually a valid "type". Our test suite would then catch any use-cases with an invalid argument
There was a problem hiding this comment.
I checked with gemini, but didn't get anything good. Your idea actually looks better. I've implemented that now.
danieljames-dj
force-pushed
the
validation_id
branch
from
af0c4cd to
fc73a30
Compare
|
@gregorbg Can you please let me know if this aligns with what we discussed? If yes, can I go ahead with making these changes in the other initializations? |
danieljames-dj
force-pushed
the
validation_id
branch
from
fc73a30 to
75451a1
Compare
| @@ -18,7 +19,7 @@ function EditPersonValidations({ ticketDetails }) { | |||
| if (error) return <Errored />; | |||
|
|
|||
| return validators.dob.map((validator) => ( | |||
| <Message warning>{validator.message}</Message> | |||
| <Message warning>{I18n.t(`validators.person.${validator.id}`)}</Message> | |||
There was a problem hiding this comment.
This will not fill the i18n keys correctly, I'm afraid
There was a problem hiding this comment.
I.e. the message will look like "The date of birth of [missing 'name' param] is on January 1st"
There was a problem hiding this comment.
Yes, actually the same error is there in production as well. Since this is internal to WRT, I thought of adjusting with this error because knowing 'something is wrong' is better than knowing nothing.
TBH I didn't spend time to check if name can be populated here. I guess it should be possible through ticketDetails. I'll explore if that is possible.
There was a problem hiding this comment.
I just checked and WCA ID is available. For now I have added WCA ID. But I feel this fix should be separate PR, I'll do that later. For now I've added it in this PR, will change later.
| @kind = kind | ||
| @args = message_args | ||
| @competition_id = competition_id | ||
| end | ||
|
|
||
| def to_s | ||
| format(@message, @args) | ||
| I18n.t("validators.person.#{@id}", **@args) |
There was a problem hiding this comment.
I think you need to include @kind in this message instead of hard-coding person
danieljames-dj
force-pushed
the
validation_id
branch
2 times, most recently
from
6590b3e to
0a39b98
Compare
danieljames-dj
force-pushed
the
validation_id
branch
from
0a39b98 to
188fb17
Compare
Currently there is no identifier for validation issue, this PR aims in adding ID so that this will help in many future features.