Validate user account value #47

rvykydal · 2023-11-20T10:42:22Z

The PR was originally open on anaconda repository and has some history and discussion there: rhinstaller/anaconda#5305

rvykydal · 2023-11-20T10:48:23Z

@garrett please see rhinstaller/anaconda#5305 (review)

garrett

I've made a bunch of suggestions.

garrett · 2023-11-20T16:20:04Z

src/components/users/Accounts.jsx

+            valid = null;
+        } else if (userAccount.length > 32) {
+            valid = false;
+            setUserAccountInvalidHint(_("The user name is too long"));


Order of preference for the account name is:

Account name (doesn't use "user"; this is good, as "user" is bad to show in interfaces)

Username (the actual word, according to English dictionaries)

User name (an uncommon variant, according to English dictionaries)

As both GNOME and KDE talk about "user" accounts and have a "name" field, I guess keeping with "User name" is probably OK enough for consistency, even if it's not ideal.

Therefore, this is fine.

On the other hand, it would be good to mention what the cutoff is.

Suggested change

setUserAccountInvalidHint(_("The user name is too long"));

setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));

garrett · 2023-11-20T16:25:40Z

src/components/users/Accounts.jsx

+            setUserAccountInvalidHint(_("The user name is too long"));
+        } else if (reservedNames.includes(userAccount)) {
+            valid = false;
+            setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));


Since this is blocked exclusively because it is reserved word, it might be nice to say that specifically here.

Suggested change

setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));

setUserAccountInvalidHint(_("This is a reserved word; please try another user name"));

garrett · 2023-11-20T16:29:40Z

src/components/users/Accounts.jsx

+            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"), "-_"));


A few improvements:

Rewording

"lowercase" is one word

Oxford comma for an inline list

Suggested change

setUserAccountInvalidHint(cockpit.format(_("The user name should usually only consist of lower case letters from a-z, digits and the following characters: $0"), "-_"));

setUserAccountInvalidHint(cockpit.format(_("User names may only contain lowercase letters from a-z, digits, and the following characters: $0"), "-_"));

garrett · 2023-11-20T16:32:00Z

⚠️ Also, in the future, when you're tagging me on GitHub, please use my name without typos. When you make typos, it makes other people upset... and I also won't see the ping.

(My name has two Rs and two Ts.)

rvykydal · 2023-11-21T13:20:44Z

⚠️ Also, in the future, when you're tagging me on GitHub, please use my name without typos. When you make typos, it makes other people upset... and I also won't see the ping.

(My name has two Rs and two Ts.)

Sorry for that, relying on auto-completion makes me ignorant, now I'll remember :)

rvykydal · 2023-11-21T14:27:48Z

garrett

Thanks for the screenshots!

I'm looking at this all again, in context now, with a clearer head (less of a post-COVID headache today), and I have another round of suggestions. Apologies!

This time around, I'll show high fidelity mockups and explain it:

Since we're talking about "user names", we should be consistent with the label.
This is using PatternFly recommendations for errors in the forms, with the red helper text .
I shortened the text to "User name must be shorter than 33 characters"
Like the original mockups, the user name field is shorter, to show the limit visually. (It should roughly fit 33 wide characters, like 33 "w"s side by side, plus a little padding for the error icon.)

We don't need to say "please change" as that's implied
I changed the text to be a little clearer
I've added a (?) icon for a popover to explain it
The popover shows the reserved words
This icon and popover is a "nice to have" and should be a follow-up... do not block this PR for it 😉

I changed the text to be shorter for the description and made it more consistent with the details (with the word first, then symbols after)

Again: Apologies for another review with different changes after you already addressed what I said yesterday. Thanks in advance!

I've also attached the changes as suggestions here, so I hope that makes it easier to apply. I did not see where the "User account" label is in this PR (so I could change it to "User name", to make it more consistent with the rest of the writing as well as with GNOME, KDE, and probably other desktops.)

garrett · 2023-11-22T13:38:05Z

src/components/users/Accounts.jsx

+            valid = null;
+        } else if (userAccount.length > 32) {
+            valid = false;
+            setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));


Suggested change

setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));

setUserAccountInvalidHint(_("User name must be shorter than 33 characters"));

garrett · 2023-11-22T13:38:37Z

src/components/users/Accounts.jsx

+            setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));
+        } else if (reservedNames.includes(userAccount)) {
+            valid = false;
+            setUserAccountInvalidHint(_("This is a reserved word; please try another user name"));


Suggested change

setUserAccountInvalidHint(_("This is a reserved word; please try another user name"));

setUserAccountInvalidHint(_("User name must not be a reserved word"));

garrett · 2023-11-22T13:39:36Z

src/components/users/Accounts.jsx

+            setUserAccountInvalidHint(_("This is a reserved word; please try another user name"));
+        } else if (isUserAccountWithInvalidCharacters(userAccount)) {
+            valid = false;
+            setUserAccountInvalidHint(cockpit.format(_("User names may only contain lowercase letters from a-z, digits, and the following characters: $0"), "-._"));


Suggested change

setUserAccountInvalidHint(cockpit.format(_("User names may only contain lowercase letters from a-z, digits, and the following characters: $0"), "-._"));

setUserAccountInvalidHint(cockpit.format(_("User name may only contain: letters from a-z, digits 0-9, dash -, period ., and underscore _")));

rvykydal · 2023-11-23T11:56:39Z

* Since we're talking about "user names", we should be consistent with the label.

* This is using PatternFly recommendations for errors in the forms, with the red helper text .

* I shortened the text to "User name must be shorter than 33 characters"

* Like the original mockups, the user name field is shorter, to show the limit visually. (It should roughly fit 33 wide characters, like 33 "w"s side by side, plus a little padding for the error icon.)

Modified. 33 "w"s seems maybe too wide to me (too close to the width of the other entries) but I am ok with it:

* We don't need to say "please change" as that's implied

* I changed the text to be a little clearer

Updated

* I've added a (?) icon for a popover to explain it

* The popover shows the reserved words

* This icon and popover is a "nice to have" and should be a follow-up... do not block this PR for it 😉

I'd leave it as eventual follow-up.
(Personally I don't find the list very useful, IMO the change you hit the reserved name is quite low. Also there is a plan to move the validation to the backend so the backend would need to provide the list)

* I changed the text to be shorter for the description and made it more consistent with the details (with the word first, then symbols after)

Updated.
Just a note - some rejected names falling under this message are not really consistent with the message (same as in gnome users where the original message comes from) but I think it is acceptable given they are pretty rare (maybe except for numbers-only). (https://github.com/rvykydal/anaconda-webui/blob/da5daf8533c5b030f8a78d36a0c1c8ec099aead7/src/components/users/Accounts.jsx#L81, https://github.com/rvykydal/anaconda-webui/blob/da5daf8533c5b030f8a78d36a0c1c8ec099aead7/test/check-users#L125)

garrett · 2023-11-23T12:36:59Z

Thanks for the changes!

Modified. 33 "w"s seems maybe too wide to me (too close to the width of the other entries) but I am ok with it:

Yeah, that does seem wide. I guess on a larger resolution screen, there might be more of a difference?

I'd leave it as eventual follow-up.
(Personally I don't find the list very useful, IMO the change you hit the reserved name is quite low. Also there is a plan to move the validation to the backend so the backend would need to provide the list)

Yeah, agreed on both points. It's pretty unlikely someone will hit multiples of the reserved words. Having it be a follow-up is totally fine. Forgetting to follow-up is probably OK too, as it's pretty unlikely.

rejected names falling under this message are not really consistent with the message (same as in gnome users where the original message comes from) but I think it is acceptable given they are pretty rare (maybe except for numbers-only)

Should we have an error for numbers only then?

"User names must contain at least one letter from a-z."

Although I guess you could have a username of $, right? 🤔

KKoukiou · 2023-11-23T13:08:49Z

src/components/users/Accounts.jsx

@@ -55,6 +58,33 @@ export const accountsToDbusUsers = (accounts) => {
    }];
 };

+const reservedNames = [


This code now is duplicated pyanaconda/core/users.py

I will create an issue to expose it via an API and remove the duplication.

https://issues.redhat.com/browse/INSTALLER-3839

rvykydal · 2023-11-23T13:39:16Z

Thanks for the changes!

Modified. 33 "w"s seems maybe too wide to me (too close to the width of the other entries) but I am ok with it:

Yeah, that does seem wide. I guess on a larger resolution screen, there might be more of a difference?

I don't think so, actually the width of other entries is already limited by limiting the form width: https://github.com/rvykydal/anaconda-webui/blob/da5daf8533c5b030f8a78d36a0c1c8ec099aead7/src/components/users/Accounts.scss#L3.

rejected names falling under this message are not really consistent with the message (same as in gnome users where the original message comes from) but I think it is acceptable given they are pretty rare (maybe except for numbers-only)

Should we have an error for numbers only then?

"User names must contain at least one letter from a-z."

Although I guess you could have a username of $, right? 🤔

If anything, I'd say something like "numbers-only user name is not allowed" because for example these are valid: 3.4, 4$. (But I am fine with the current PR state)

So that it indicates that the length of the name is limited to the user.

For better consistency with system tools.

rvykydal marked this pull request as ready for review November 20, 2023 10:42

rvykydal mentioned this pull request Nov 20, 2023

Validate user account value rhinstaller/anaconda#5305

Closed

rvykydal requested a review from KKoukiou November 20, 2023 10:44

rvykydal force-pushed the validate-user-account-value branch 2 times, most recently from 984388b to b002be4 Compare November 20, 2023 14:08

garrett requested changes Nov 20, 2023

View reviewed changes

rvykydal force-pushed the validate-user-account-value branch from b002be4 to 577a227 Compare November 21, 2023 14:11

garrett requested changes Nov 22, 2023

View reviewed changes

rvykydal force-pushed the validate-user-account-value branch 2 times, most recently from 1fb49ea to da5daf8 Compare November 23, 2023 11:07

KKoukiou approved these changes Nov 23, 2023

View reviewed changes

rvykydal added 5 commits November 23, 2023 15:16

webui: add test for user account creating

b680596

webui: add user account validation to accounts screen

2d4b974

webui: Make user name input entry shorter

ac096d1

So that it indicates that the length of the name is limited to the user.

webui: rename "User account" to "User name".

c35ba67

For better consistency with system tools.

webui: update pixel test references for user account validation

8bde528

rvykydal force-pushed the validate-user-account-value branch from da5daf8 to 8bde528 Compare November 23, 2023 14:19

KKoukiou approved these changes Nov 23, 2023

View reviewed changes

rvykydal merged commit eaac83c into rhinstaller:main Nov 24, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate user account value #47

Validate user account value #47

rvykydal commented Nov 20, 2023

rvykydal commented Nov 20, 2023 •

edited by KKoukiou

Loading

garrett left a comment

garrett Nov 20, 2023

garrett Nov 20, 2023

rvykydal Nov 21, 2023

garrett Nov 20, 2023

rvykydal Nov 21, 2023

garrett Nov 20, 2023

rvykydal Nov 21, 2023

garrett commented Nov 20, 2023

rvykydal commented Nov 21, 2023

rvykydal commented Nov 21, 2023

garrett left a comment

garrett Nov 22, 2023

garrett Nov 22, 2023

garrett Nov 22, 2023

rvykydal commented Nov 23, 2023

garrett commented Nov 23, 2023

KKoukiou Nov 23, 2023

KKoukiou Nov 23, 2023

rvykydal commented Nov 23, 2023 •

edited

Loading

	setUserAccountInvalidHint(_("The user name is too long"));
	setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));

	setUserAccountInvalidHint(_("Sorry, that user name is not available. Please try another."));
	setUserAccountInvalidHint(_("This is a reserved word; please try another user name"));

	setUserAccountInvalidHint(cockpit.format(_("The user name should usually only consist of lower case letters from a-z, digits and the following characters: $0"), "-_"));
	setUserAccountInvalidHint(cockpit.format(_("User names may only contain lowercase letters from a-z, digits, and the following characters: $0"), "-_"));

	setUserAccountInvalidHint(_("User names must contain fewer than 33 characters"));
	setUserAccountInvalidHint(_("User name must be shorter than 33 characters"));

Validate user account value #47

Validate user account value #47

Conversation

rvykydal commented Nov 20, 2023

rvykydal commented Nov 20, 2023 • edited by KKoukiou Loading

garrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett commented Nov 20, 2023

rvykydal commented Nov 21, 2023

rvykydal commented Nov 21, 2023

garrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Nov 23, 2023

garrett commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Nov 23, 2023 • edited Loading

rvykydal commented Nov 20, 2023 •

edited by KKoukiou

Loading

rvykydal commented Nov 23, 2023 •

edited

Loading