Users step #5280

rvykydal · 2023-10-25T14:11:02Z

This is a draft pull request for https://issues.redhat.com/browse/INSTALLER-3205.
I have some questions (some related to specific commits, so it might make sense to follow the PR with the commits) and items for follow up steps.

The strenght indicator does not scale for isHorizontal Form

Compare to disk encryption:

KKoukiou

Will review this tomorrow morning.

KKoukiou · 2023-10-25T15:23:13Z

ui/webui/src/components/Password.jsx

+
+// eslint-disable-next-line camelcase
+import { password_quality } from "cockpit-components-password.jsx";
+import ExclamationCircleIcon from "@patternfly/react-icons/dist/esm/icons/exclamation-circle-icon";


Please use named imports from @patternfly/react-icons

import { IconONe, IconTwo } from "@patternfly/react-icons"

KKoukiou · 2023-10-26T07:13:46Z

ui/webui/src/components/storage/DiskEncryption.jsx

+        return {
+            id: rule.id,
+            text: rule.text(policy, password),
+            satisfied: password.length > 0 ? rule.check(policy, password) : null,


satisfied is a boolean like the isWarning variable. For consistency purposes, let's pick one and use either the pattern 'isMyBoolVariable' or 'myBoolVariable'.

KKoukiou · 2023-10-26T07:18:36Z

ui/webui/src/components/storage/DiskEncryption.jsx

+        id: "length",
+        text: (policy) => cockpit.format(_("Must be at least $0 characters"), policy["min-length"].v),
+        check: (policy, password) => password.length >= policy["min-length"].v,
+        isWarning: false,


I think it's more explicit to use isError: false instead of isWarning: true. and vice versa. I had to read the code through to understand that isWarning: true is not causing the password to be invalid.

KKoukiou · 2023-10-26T07:25:36Z

ui/webui/src/components/storage/DiskEncryption.jsx

@@ -303,14 +322,10 @@ export const DiskEncryption = ({
 }) => {
    const [password, setPassword] = useState(storageEncryption.password);
    const [confirmPassword, setConfirmPassword] = useState(storageEncryption.confirmPassword);
-    const [passwordStrength, setPasswordStrength] = useState("");
+    const [isValid, setIsValid] = useState(false);


I don't see the purpose of maintaining the isValid as a state variable here. I suggest to directly use the setIsFormValid to invalidate or validate the form from the children component and remove this.

DiskEncryption should always setIsFormValid(true) if the isEncrypted is false.

The Password form fields should setIsFormValid(satisfied) where satisfied is the password rules result.

KKoukiou · 2023-10-26T07:41:27Z

ui/webui/src/components/users/Accounts.jsx

+
+    return (
+        <div id={idPrefix + "-create-account"}>
+            <Stack hasGutter>


Instead of using Stack/StackItems you can put everything in a <Form isHorizontal>, spacings will be correct automatically.

I am using the Stack to get the spacing between rows (like in the mockup), without it it looks like this:

The proper way to space form elements it to wrap everything with a '
' and let Patternfly do the trick :)

Ah, this works if I move Form isHorizontal from Accounts to Create Account (having it in Accounts does not work). I wanted the Form to contain also (future) root component. I guess I'll use two forms then - Create Account and the other one for the root component? Well, it can be addressed when I add the UI for root. For now I'm moving Form to Create Accounts.

KKoukiou · 2023-10-26T07:45:17Z

ui/webui/src/components/AnacondaWizard.jsx

@@ -344,6 +349,8 @@ const Footer = ({
                    setStepNotification();
                },
            });
+        } else if (activeStep.id === "accounts") {
+            onNext();


Hm does this need special condition? The else { does the same.

There is setUsers ? Strange it is not visible here. IIRC I did one rebase of the PR but it was on Wed afternoon, nevermind...

rvykydal · 2023-10-31T11:32:55Z

I think I addressed the remarks above and I am going to work on the items from TODO list in the description.

rvykydal · 2023-10-31T14:56:59Z

ui/webui/test/check-storage

@@ -422,6 +423,7 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase, StorageHelpers)
        applied_partitioning = s.dbus_get_applied_partitioning()

        # When adding a new partition a new partitioning should be created
+        i.back()


This will not work for cases where the accounts screen is disabled (eg Workstation). We may need something like reach() for going back.
And run the tests on live.

ui/webui/src/components/AnacondaWizard.jsx

M4rtinK · 2023-10-31T15:12:36Z

@rvykydal @KKoukiou So thinking about it. I think the user screen needs to be conventionalized not to show up on Fedora Workstation. They have Gnome Initial Setup for user creation, so the extra screen would change the expected and agreed upon flow (and might break Fedora QA testing if an unexpected screens just shows up).

I think the logic should be kinda like this:

Live images in general: YES
Fedora Workstation : NO
boot.iso: YES

I think this logic should either be part of this PR before its merged or alternatively we should not do a new release before the logic id in place via a followup. :)

rvykydal · 2023-11-01T09:14:21Z

@rvykydal @KKoukiou So thinking about it. I think the user screen needs to be conventionalized not to show up on Fedora Workstation. They have Gnome Initial Setup for user creation, so the extra screen would change the expected and agreed upon flow (and might break Fedora QA testing if an unexpected screens just shows up).

I think the logic should be kinda like this:
* Live images in general: YES

* Fedora Workstation : NO

* boot.iso: YES
I think this logic should either be part of this PR before its merged or alternatively we should not do a new release before the logic id in place via a followup. :)

I think in scope of this PR just using isBootIso logic similar as with Language selection should do (would disable the users screen at live images in general, including Fedora WS), but yes we may soon need to use the per-product/variant logic rather then simple isBootIso.

pep8speaks · 2023-11-02T13:54:48Z

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

In the file ui/webui/test/helpers/end2end.py:

Line 33:1: E402 module level import not at top of file
Line 34:1: E402 module level import not at top of file

In the file ui/webui/test/helpers/password.py:

Line 24:1: E402 module level import not at top of file
Line 26:1: E302 expected 2 blank lines, found 1

In the file ui/webui/test/helpers/users.py:

Line 24:1: E402 module level import not at top of file
Line 25:1: E402 module level import not at top of file

Comment last updated at 2023-11-06 11:07:15 UTC

rvykydal · 2023-11-02T14:20:32Z

@KKoukiou I think you didn't review the last 8 commits - mostly adding the tests.
I believe the PR is mergeable (see the missing items in the description).

ui/webui/test/check-basic

rvykydal · 2023-11-03T05:33:33Z

Need to do a tests update.

rvykydal · 2023-11-03T10:34:55Z

/webui-test

rvykydal · 2023-11-03T10:38:14Z

@KKoukiou I think you didn't review the last 7 commits - mostly adding the tests.

Regarding the commits with test updates - in case you look at the commits sequentially just a warning that the
webui: create required user when reaching a test step by default is a change of approach to a previous commit.

I believe the PR is mergeable (see the missing items in the description).

rvykydal · 2023-11-03T10:39:20Z

/kickstart-test --waive webui-only

rvykydal · 2023-11-03T11:45:48Z

/kickstart-test --waive webui-only

rvykydal · 2023-11-03T11:46:17Z

/webui-test

KKoukiou · 2023-11-03T14:10:27Z

ui/webui/src/components/users/Accounts.jsx

+
+    return (
+        <>
+            <CreateAccount


Uneeded wrapper on empty compoent.

Yes, it is waiting for the root part. Either I'll keep it or remove when I add root.

KKoukiou · 2023-11-03T14:10:57Z

ui/webui/src/components/users/Accounts.scss

@@ -0,0 +1,4 @@
+// Span disk encryption password fields to take slightly more width than the default


Wrong comment.

fixed in webui: fix password strength indicator layout in horizontal form

KKoukiou · 2023-11-03T14:17:12Z

ui/webui/test/check-basic

@@ -44,7 +45,8 @@ class TestBasic(anacondalib.VirtInstallMachineCase):
        # Do not start the installation in non-destructive tests as this performs non revertible changes
        # with the pages basically empty of common elements (as those are provided by the top-level installer widget)
        # we at least iterate over them to check this works as expected
-        i.reach(i.steps.REVIEW)
+        # Create user on the first pass through users step
+        i.reach(i.steps.REVIEW, step_callbacks={i.steps.ACCOUNTS: lambda: create_user(self)})


I would like that the callback would be by default set if not specified to avoid repeating code.

Yes, it is fixed in the next commit.

KKoukiou · 2023-11-03T14:18:13Z

ui/webui/test/check-storage

@@ -1045,8 +1045,8 @@ class TestStorageMountPointsEFI(anacondalib.VirtInstallMachineCase):
        s.select_mountpoint_row_device(3, f"{dev}3")
        s.check_mountpoint_row_format_type(3, "xfs")

-        # Create user on the first pass through users step
-        i.reach(i.steps.REVIEW, step_callbacks={i.steps.ACCOUNTS: lambda: create_user(self)})
+        self.addCleanup(lambda: dbus_reset_users(self.machine))


This should be added in anacondalib and be run by default for all tests.

Okay, sounds good, only on destructive tests if failed for me.
I think we can address it in a follow up PR.

KKoukiou

All in all I have some comments, but they are all not blockers. This is a huge chunk of work and in very good state. I am ok to merge this as is, and do the improvements in followups.

KKoukiou

Agreed with @rvykydal to block this on fixing the strength indicator.

KKoukiou

@rvykydal I think I have a workaround:

[kkoukiou@sequioa webui]$ git diff
diff --git ui/webui/src/components/users/Accounts.scss ui/webui/src/components/users/Accounts.scss
index f53ec05624..bdce550c8f 100644
--- ui/webui/src/components/users/Accounts.scss
+++ ui/webui/src/components/users/Accounts.scss
@@ -2,3 +2,8 @@
 #accounts-create-account {
   width: min(60ch, 100%);
 }
+
+// FIXME: Undo when fixed upstream: https://github.com/patternfly/patternfly/issues/6032
+#accounts-create-account .pf-v5-c-form__group-label.pf-m-info {
+    align-items: baseline
+}

And since it affects also the disk selection, actually the workaround needs to be on the CSS file of the Password component. Add some helper class there and replace my used ID.

rvykydal · 2023-11-06T08:45:13Z

@rvykydal I think I have a workaround:

[kkoukiou@sequioa webui]$ git diff
diff --git ui/webui/src/components/users/Accounts.scss ui/webui/src/components/users/Accounts.scss
index f53ec05624..bdce550c8f 100644
--- ui/webui/src/components/users/Accounts.scss
+++ ui/webui/src/components/users/Accounts.scss
@@ -2,3 +2,8 @@
 #accounts-create-account {
   width: min(60ch, 100%);
 }
+
+// FIXME: Undo when fixed upstream: https://github.com/patternfly/patternfly/issues/6032
+#accounts-create-account .pf-v5-c-form__group-label.pf-m-info {
+    align-items: baseline
+}

This didn't work for me:

But adding also flex-direction: column

+
+// FIXME: Undo when fixed upstream: https://github.com/patternfly/patternfly/issues/6032
+#accounts-create-account .pf-v5-c-form__group-label.pf-m-info {
+  flex-direction: column;
+  align-items: baseline;
+}

did work:

And since it affects also the disk selection, actually the workaround needs to be on the CSS file of the Password component. Add some helper class there and replace my used ID.

Not sure what you mean by the disk selection. I know about ths issue (using the indicator in horizontal form) only at this place, so the ID would be OK.

rvykydal · 2023-11-06T10:43:18Z

But adding also flex-direction: column

+
+// FIXME: Undo when fixed upstream: https://github.com/patternfly/patternfly/issues/6032
+#accounts-create-account .pf-v5-c-form__group-label.pf-m-info {
+  flex-direction: column;
+  align-items: baseline;
+}

Actually on Chrome the baseline does not look good, using flex-start instead.

KKoukiou · 2023-11-06T10:46:15Z

But adding also flex-direction: column
+
+// FIXME: Undo when fixed upstream: https://github.com/patternfly/patternfly/issues/6032
+#accounts-create-account .pf-v5-c-form__group-label.pf-m-info {
+  flex-direction: column;
+  align-items: baseline;
+}
Actually on Chrome the baseline does not look good, using flex-start instead.

I don't like the change that the 'direction: column' does. The design seems of when the strength goes bellow imo. Fine with the flex-start.

rvykydal · 2023-11-06T11:48:27Z

/kickstart-test --waive webui-only

KKoukiou

Thanks. As said I dont like the column direction for the strength indicator but I am happy to send a followup fix. Also I think we need to show the new user name in the review step as well? Also fine for followup.

rvykydal · 2023-11-06T12:41:45Z

Thanks. As said I dont like the column direction for the strength indicator but I am happy to send a followup fix. Also I think we need to show the new user name in the review step as well? Also fine for followup.

Agreed, I have the review step in the TODO list at the beginning. I would merge this. (I've got the feeling we may end up with completely different indicator, similar to the bar in cockpit or Anaconda.)

rvykydal · 2023-11-08T08:28:58Z

Follow up issues tracked here: https://issues.redhat.com/browse/INSTALLER-3781

github-actions bot added webui f40 labels Oct 25, 2023

rvykydal force-pushed the users-step branch 2 times, most recently from 8c6d769 to c69b0e6 Compare October 25, 2023 14:31

rvykydal requested a review from KKoukiou October 25, 2023 14:32

KKoukiou reviewed Oct 25, 2023

View reviewed changes

KKoukiou requested changes Oct 26, 2023

View reviewed changes

rvykydal force-pushed the users-step branch 4 times, most recently from 9d061e8 to 0b9d17f Compare October 31, 2023 11:06

rvykydal commented Oct 31, 2023

View reviewed changes

ui/webui/src/components/AnacondaWizard.jsx Outdated Show resolved Hide resolved

rvykydal force-pushed the users-step branch 2 times, most recently from 6d0afe5 to 6db2a4d Compare November 2, 2023 13:54

rvykydal force-pushed the users-step branch 2 times, most recently from 5718a8b to 3eb4f91 Compare November 2, 2023 14:09

rvykydal marked this pull request as ready for review November 2, 2023 14:18

rvykydal commented Nov 3, 2023

View reviewed changes

ui/webui/test/check-basic Outdated Show resolved Hide resolved

rvykydal marked this pull request as draft November 3, 2023 05:33

rvykydal force-pushed the users-step branch from 3eb4f91 to 04caf91 Compare November 3, 2023 10:07

rvykydal added 3 commits November 3, 2023 11:08

webui: use dynamic rules in password form component

95107cf

webui: move also password strength logic into pw form component

5f40775

webui: move pasword form component into a separate file

17901f1

rvykydal added 4 commits November 3, 2023 11:08

webui: add simple test for users screen

1593bcd

webui: add users screen to tests for sidebar navigation

7865158

webui: allow to create user more easily for reaching a step in test

3d23299

webui: create required user when reaching a test step by default

3bf083a

rvykydal force-pushed the users-step branch from 04caf91 to 730f060 Compare November 3, 2023 10:08

rvykydal marked this pull request as ready for review November 3, 2023 10:38

rvykydal requested a review from KKoukiou November 3, 2023 10:41

KKoukiou reviewed Nov 3, 2023

View reviewed changes

KKoukiou approved these changes Nov 3, 2023

View reviewed changes

KKoukiou reviewed Nov 3, 2023

View reviewed changes

rvykydal added 3 commits November 6, 2023 11:57

webui: update end2end tests for the new users screen

415843e

webui: fix password strength indicator layout in horizontal form

1588dad

webui: update pixel test images

fb8257c

rvykydal force-pushed the users-step branch from c68f39e to fb8257c Compare November 6, 2023 11:07

KKoukiou approved these changes Nov 6, 2023

View reviewed changes

rvykydal merged commit e81d86b into rhinstaller:master Nov 6, 2023
14 checks passed

		@@ -0,0 +1,4 @@
		// Span disk encryption password fields to take slightly more width than the default

Users step #5280

Users step #5280

Conversation

rvykydal commented Oct 25, 2023 • edited Loading

KKoukiou 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

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 Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

rvykydal commented Oct 31, 2023

rvykydal Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

M4rtinK commented Oct 31, 2023

rvykydal commented Nov 1, 2023

pep8speaks commented Nov 2, 2023 • edited Loading

Comment last updated at 2023-11-06 11:07:15 UTC

rvykydal commented Nov 2, 2023 • edited Loading

rvykydal commented Nov 3, 2023

rvykydal commented Nov 3, 2023

rvykydal commented Nov 3, 2023

rvykydal commented Nov 3, 2023

rvykydal commented Nov 3, 2023

rvykydal commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal Nov 6, 2023 • edited Loading

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

KKoukiou left a comment

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

KKoukiou left a comment • edited Loading

Choose a reason for hiding this comment

rvykydal commented Nov 6, 2023 • edited Loading

rvykydal commented Nov 6, 2023

KKoukiou commented Nov 6, 2023

rvykydal commented Nov 6, 2023

KKoukiou left a comment

Choose a reason for hiding this comment

rvykydal commented Nov 6, 2023

rvykydal commented Nov 8, 2023

rvykydal commented Oct 25, 2023 •

edited

Loading

rvykydal Oct 31, 2023 •

edited

Loading

rvykydal Oct 30, 2023 •

edited

Loading

rvykydal Oct 31, 2023 •

edited

Loading

pep8speaks commented Nov 2, 2023 •

edited

Loading

rvykydal commented Nov 2, 2023 •

edited

Loading

rvykydal Nov 6, 2023 •

edited

Loading

KKoukiou left a comment •

edited

Loading

rvykydal commented Nov 6, 2023 •

edited

Loading