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.
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
Add supplemental regex setting to validate username #592
Add supplemental regex setting to validate username #592
Changes from 1 commit
a1bd0e1
98507b2
8ef11a4
6d20577
733d359
15c9328
9c0a518
ae147c1
436c7f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
3.37
insteadThere 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.
Is this
msg_on_fail
error message displayed also to the web UI or only in the logs?Can I have a sample content of this
msg_on_fail
error message? Is is understandable by someone not well versed with Magpie, like a new node admin.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.
The value is defined here: https://github.com/mishaschwartz/Magpie/blob/a1bd0e15c88d353df51f5ca7988e7ffbc34d202f/magpie/api/schemas.py#L1961C1-L1961C57
and says:
"Invalid 'user_name' value specified."
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.
Right, it does not say "why" and there is no way to add supplemental info that the reason is the new regex. I hope the node admin will be able to guess it.
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 can make or respond with a different message. How about: "Invalid 'user_name' specified. Does not match the supplemental user name regex."
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.
Sure that would be better if it does not take to much of your time. Creating a new sub-class with a new hardcoded message?
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.
msg_on_fail
is only the generic message value in the API body.For the "why", that would be reported in the API response body. It provides the
param_name
,param_compare
, the input value and so on with more details about the specific check that failed.On the UI, it would simply be a red message next to the input text field because the contents are limited in the code. That could be improved.
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.
The message in the UI I mentioned is here:
Magpie/magpie/ui/management/templates/add_user.mako
Lines 39 to 41 in 92ff2d2
It uses the property obtained from this:
Magpie/magpie/ui/management/views.py
Lines 87 to 88 in 92ff2d2
Which default to this:
Magpie/magpie/ui/utils.py
Lines 304 to 308 in 92ff2d2
That value could be overridden with more explicit details according to the contents parsed from the API response obtained here before returning the UI response:
Magpie/magpie/ui/management/views.py
Lines 106 to 108 in 92ff2d2
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.
@fmigneault @tlvu with the current update to
msg_on_fail
the UI looks like this:Is this sufficient or are there other changes you would suggest?
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.
Still need rename of the variable/setting to
MAGPIE_USER_NAME_EXTRA_REGEX
.This is to align with other variable names, such as
MAGPIE_USER_NAME_MAX_LENGTH
, for similar checks.