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

Validate user account value #5305

Closed

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Nov 9, 2023

The PR is addressing part of https://issues.redhat.com/browse/INSTALLER-3781

There are 3 types of hints/messages for invalid values in the PR - the behavior should be the same as in Gnome Users settings in rawhide (using the same messages including the interpunction). Valid values are the same as in the current Gtk UI (but the Gtk UI has different hints/messages, a bit more fine grained).

Please note that I replaced "username" with "user name" in the validation messages after creating the screenshots (the tests were failing on it).

Screenshot from 2023-11-09 11-57-42
Screenshot from 2023-11-09 11-57-50
Screenshot from 2023-11-09 12-47-41
Screenshot from 2023-11-09 11-58-24
Screenshot from 2023-11-09 11-59-17

@pep8speaks
Copy link

pep8speaks commented Nov 9, 2023

Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-10 13:54:04 UTC

setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));
} else if (isUserAccountWithInvalidCharacters(userAccount)) {
valid = false;
setUserAccountInvalidHint(_("The user name should usually only consist of lower case letters from a-z, digits and the following characters: -_"));
Copy link
Contributor Author

@rvykydal rvykydal Nov 10, 2023

Choose a reason for hiding this comment

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

The actually allowed chars are . - and _. I am not sure if we want to diverge from gnome desktop message and mention also . but we could say and the following charcters: ._-

userAccount === "root" ||
userAccount === "daemon" ||
userAccount === "home" ||
userAccount === "system"
Copy link
Contributor

Choose a reason for hiding this comment

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

I could image this list in our config files, but it is probably not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, or it will rather go to the backend with the validation.
Actually I was using wrong branch for the list so I had to extend it a bit and rebase the PR (sorry).

setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));
} else if (isUserAccountWithInvalidCharacters(userAccount)) {
valid = false;
setUserAccountInvalidHint(cockpit.format(_("The user name should usually only consist of lower case letters from a-z, digits and the following characters: $0"), "-_"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a hint for the future, it makes sense to add new validation methods to the DBus API, because ideally, we want all our user interfaces to use a similar logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea. I will create a Jira task for this improvement suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I was thinking about it as well, as a follow-up.
Hopefully with debouncing it won't have performance issues.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@garrett can you please review the helper messages.

@rvykydal
Copy link
Contributor Author

Migrating to rhinstaller/anaconda-webui#47

@rvykydal rvykydal closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants