From 0df64cd57eba80d36e2cecf016f5b2f5a9591d33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 18 Mar 2025 08:15:37 +0000 Subject: [PATCH 1/5] refactor(web): use a checkbox for enabling encryption Instead of using a switch. The previous implementation immediately triggered the encryption settings when toggled, but this is no longer the case. Switches should typically be used for immediate actions, whereas encryption settings are now part of a form that must be submitted before changes are applied. Additionally, the way the form is rendered has been modified to show fields only when necessary or requested, instead of just toggling their enabled/disabled states. This ensures a more consistent interface by displaying only required fields and aligns with the user experience found on the authentication page. --- .../storage/EncryptionSettingsPage.test.tsx | 36 +++++-------- .../storage/EncryptionSettingsPage.tsx | 54 ++++++++++--------- 2 files changed, 41 insertions(+), 49 deletions(-) diff --git a/web/src/components/storage/EncryptionSettingsPage.test.tsx b/web/src/components/storage/EncryptionSettingsPage.test.tsx index 38fa6dc462..aa9ac5e2df 100644 --- a/web/src/components/storage/EncryptionSettingsPage.test.tsx +++ b/web/src/components/storage/EncryptionSettingsPage.test.tsx @@ -78,9 +78,9 @@ describe("EncryptionSettingsPage", () => { it("allows enabling the encryption", async () => { const { user } = installerRender(); - const toggle = screen.getByRole("switch", { name: "Encrypt the system" }); - expect(toggle).not.toBeChecked(); - await user.click(toggle); + const encryptionCheckbox = screen.getByRole("checkbox", { name: "Encrypt the system" }); + expect(encryptionCheckbox).not.toBeChecked(); + await user.click(encryptionCheckbox); const passwordInput = screen.getByLabelText("Password"); const passwordConfirmationInput = screen.getByLabelText("Password confirmation"); fireEvent.change(passwordInput, { target: { value: "12345" } }); @@ -96,25 +96,15 @@ describe("EncryptionSettingsPage", () => { mockUseEncryption.mockReturnValue(mockLuks2Encryption); }); - describe("and user chooses to not use encryption", () => { - it("allows disabling the encryption", async () => { - const { user } = installerRender(); - const toggle = screen.getByRole("switch", { name: "Encrypt the system" }); - expect(toggle).toBeChecked(); - await user.click(toggle); - const passwordInput = screen.getByLabelText("Password"); - const passwordConfirmationInput = screen.getByLabelText("Password confirmation"); - const tpmCheckbox = screen.getByRole("checkbox", { name: /Use.*TPM/ }); - - expect(passwordInput).toBeDisabled(); - expect(passwordConfirmationInput).toBeDisabled(); - expect(tpmCheckbox).toBeDisabled(); - - const acceptButton = screen.getByRole("button", { name: "Accept" }); - await user.click(acceptButton); - - expect(mockLuks2Encryption.disable).toHaveBeenCalled(); - }); + it("allows disabling the encryption", async () => { + const { user } = installerRender(); + const encryptionCheckbox = screen.getByRole("checkbox", { name: "Encrypt the system" }); + expect(encryptionCheckbox).toBeChecked(); + await user.click(encryptionCheckbox); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + await user.click(acceptButton); + + expect(mockLuks2Encryption.disable).toHaveBeenCalled(); }); }); @@ -142,7 +132,7 @@ describe("EncryptionSettingsPage", () => { it("does not offer TPM", () => { installerRender(); - expect(screen.queryByRole("checkbox", { name: /Use.*TPM/ })).not.toBeInTheDocument(); + expect(screen.queryByRole("checkbox", { name: /Use.*TPM/ })).toBeNull(); }); }); }); diff --git a/web/src/components/storage/EncryptionSettingsPage.tsx b/web/src/components/storage/EncryptionSettingsPage.tsx index bb4e938f34..cc7324262d 100644 --- a/web/src/components/storage/EncryptionSettingsPage.tsx +++ b/web/src/components/storage/EncryptionSettingsPage.tsx @@ -22,8 +22,8 @@ import React, { useState, useRef } from "react"; import { useNavigate } from "react-router-dom"; -import { ActionGroup, Alert, Checkbox, Content, Form, Switch } from "@patternfly/react-core"; -import { Page, PasswordAndConfirmationInput } from "~/components/core"; +import { ActionGroup, Alert, Checkbox, Content, Form } from "@patternfly/react-core"; +import { NestedContent, Page, PasswordAndConfirmationInput } from "~/components/core"; import { useEncryptionMethods } from "~/queries/storage"; import { useEncryption } from "~/queries/storage/config-model"; import { EncryptionMethod } from "~/api/storage/types/config-model"; @@ -103,12 +103,6 @@ directly on its first run.", {_("Encryption settings")} - - {_( - "Full Disk Encryption (FDE) allows to protect the information stored \ -at the new file systems, including data, programs, and system files.", - )} - @@ -120,28 +114,36 @@ at the new file systems, including data, programs, and system files.", ))} )} - setIsEnabled(!isEnabled)} /> - - {isTpmAvailable && ( - + {isEnabled && ( + + + {isTpmAvailable && ( + + )} + )} From 25d2fb3cc82b847dbc99166693edd5dcc86eec88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 18 Mar 2025 08:32:53 +0000 Subject: [PATCH 2/5] web: adjust nested form sections with CSS To make them "respect" the gap of their parent grid. --- web/src/assets/styles/index.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/src/assets/styles/index.scss b/web/src/assets/styles/index.scss index 7529be7e40..ac9a8d8292 100644 --- a/web/src/assets/styles/index.scss +++ b/web/src/assets/styles/index.scss @@ -365,6 +365,12 @@ label.pf-m-disabled + .pf-v6-c-check__description { justify-self: flex-start; } +// Nested content inside forms should respect parent grid gap +.pf-v6-c-form [class*="pf-v6-u-mx"] { + display: grid; + gap: inherit; +} + // Some utilities not found at PF .w-14ch { inline-size: 14ch; From eddae6b4426ffaf7060024f6e7ed262a5e593ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 18 Mar 2025 08:35:23 +0000 Subject: [PATCH 3/5] fix(web): follow conventions Special mention for the fireEvent change > Most projects have a few use cases for fireEvent, but the majority of > the time you should probably use @testing-library/user-event. https://testing-library.com/docs/dom-testing-library/api-events/ --- .../storage/EncryptionSettingsPage.test.tsx | 6 +++--- .../storage/EncryptionSettingsPage.tsx | 16 +++++++--------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/web/src/components/storage/EncryptionSettingsPage.test.tsx b/web/src/components/storage/EncryptionSettingsPage.test.tsx index aa9ac5e2df..9a32663447 100644 --- a/web/src/components/storage/EncryptionSettingsPage.test.tsx +++ b/web/src/components/storage/EncryptionSettingsPage.test.tsx @@ -21,7 +21,7 @@ */ import React from "react"; -import { screen, fireEvent } from "@testing-library/react"; +import { screen } from "@testing-library/react"; import { installerRender } from "~/test-utils"; import EncryptionSettingsPage from "./EncryptionSettingsPage"; import { EncryptionHook } from "~/queries/storage/config-model"; @@ -83,8 +83,8 @@ describe("EncryptionSettingsPage", () => { await user.click(encryptionCheckbox); const passwordInput = screen.getByLabelText("Password"); const passwordConfirmationInput = screen.getByLabelText("Password confirmation"); - fireEvent.change(passwordInput, { target: { value: "12345" } }); - fireEvent.change(passwordConfirmationInput, { target: { value: "12345" } }); + await user.type(passwordInput, "12345"); + await user.type(passwordConfirmationInput, "12345"); const acceptButton = screen.getByRole("button", { name: "Accept" }); await user.click(acceptButton); expect(mockNoEncryption.enable).toHaveBeenCalledWith("luks2", "12345"); diff --git a/web/src/components/storage/EncryptionSettingsPage.tsx b/web/src/components/storage/EncryptionSettingsPage.tsx index cc7324262d..31bbe551eb 100644 --- a/web/src/components/storage/EncryptionSettingsPage.tsx +++ b/web/src/components/storage/EncryptionSettingsPage.tsx @@ -20,7 +20,7 @@ * find current contact information at www.suse.com. */ -import React, { useState, useRef } from "react"; +import React, { useEffect, useState, useRef } from "react"; import { useNavigate } from "react-router-dom"; import { ActionGroup, Alert, Checkbox, Content, Form } from "@patternfly/react-core"; import { NestedContent, Page, PasswordAndConfirmationInput } from "~/components/core"; @@ -46,7 +46,7 @@ export default function EncryptionSettingsPage() { const passwordRef = useRef(); const formId = "encryptionSettingsForm"; - React.useEffect(() => { + useEffect(() => { if (encryptionConfig) { setIsEnabled(true); setMethod(encryptionConfig.method); @@ -86,12 +86,10 @@ export default function EncryptionSettingsPage() { }; // TRANSLATORS: "Trusted Platform Module" is the name of the technology and TPM its abbreviation - const tpm_label = _( - "Use the Trusted Platform Module (TPM) to decrypt automatically on each boot", - ); + const tpmLabel = _("Use the Trusted Platform Module (TPM) to decrypt automatically on each boot"); // TRANSLATORS: The word 'directly' is key here. For example, booting to the installer media and then choosing // 'Boot from Hard Disk' from there will not work. Keep it sort (this is a hint in a form) but keep it clear. - const tpm_explanation = _( + const tpmExplanation = _( "The password will not be needed to boot and access the data if the \ TPM can verify the integrity of the system. TPM sealing requires the new system to be booted \ directly on its first run.", @@ -136,9 +134,9 @@ at the new file systems, including data, programs, and system files.", /> {isTpmAvailable && ( From 9785c67354c51be38bff181ea554782f143b8c0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Tue, 18 Mar 2025 14:19:04 +0000 Subject: [PATCH 4/5] refactor(web): use checkboxes in root auth form Replaces switches with checkboxes in the root authentication form. Switches should be used for actions that immediately apply changes, but superuser authentication settings are part of a form that must be submitted before any changes take effect. --- .../components/users/RootUserForm.test.tsx | 12 +-- web/src/components/users/RootUserForm.tsx | 88 +++++++++---------- 2 files changed, 46 insertions(+), 54 deletions(-) diff --git a/web/src/components/users/RootUserForm.test.tsx b/web/src/components/users/RootUserForm.test.tsx index 184fb36229..28f163d8db 100644 --- a/web/src/components/users/RootUserForm.test.tsx +++ b/web/src/components/users/RootUserForm.test.tsx @@ -98,7 +98,7 @@ describe("RootUserForm", () => { it("allows clearing the password", async () => { const { user } = installerRender(); - const passwordToggle = screen.getByRole("switch", { name: "Use password" }); + const passwordToggle = screen.getByRole("checkbox", { name: "Use password" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); expect(passwordToggle).toBeChecked(); await user.click(passwordToggle); @@ -111,7 +111,7 @@ describe("RootUserForm", () => { it("allows setting a public SSH Key ", async () => { const { user } = installerRender(); - const sshPublicKeyToggle = screen.getByRole("switch", { name: "Use public SSH Key" }); + const sshPublicKeyToggle = screen.getByRole("checkbox", { name: "Use public SSH Key" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); await user.click(sshPublicKeyToggle); const sshPublicKeyInput = screen.getByRole("textbox", { name: "File upload" }); @@ -126,7 +126,7 @@ describe("RootUserForm", () => { it("does not allow setting an empty public SSH Key", async () => { const { user } = installerRender(); - const sshPublicKeyToggle = screen.getByRole("switch", { name: "Use public SSH Key" }); + const sshPublicKeyToggle = screen.getByRole("checkbox", { name: "Use public SSH Key" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); await user.click(sshPublicKeyToggle); expect(sshPublicKeyToggle).toBeChecked(); @@ -139,7 +139,7 @@ describe("RootUserForm", () => { it("allows clearing the public SSH Key", async () => { mockPublicKey = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQDM+ test@example"; const { user } = installerRender(); - const sshPublicKeyToggle = screen.getByRole("switch", { name: "Use public SSH Key" }); + const sshPublicKeyToggle = screen.getByRole("checkbox", { name: "Use public SSH Key" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); expect(sshPublicKeyToggle).toBeChecked(); await user.click(sshPublicKeyToggle); @@ -158,7 +158,7 @@ describe("RootUserForm", () => { it("allows preserving it", async () => { const { user } = installerRender(); - const passwordToggle = screen.getByRole("switch", { name: "Use password" }); + const passwordToggle = screen.getByRole("checkbox", { name: "Use password" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); expect(passwordToggle).toBeChecked(); screen.getByText("Using a hashed password."); @@ -170,7 +170,7 @@ describe("RootUserForm", () => { it("allows discarding it", async () => { const { user } = installerRender(); - const passwordToggle = screen.getByRole("switch", { name: "Use password" }); + const passwordToggle = screen.getByRole("checkbox", { name: "Use password" }); const acceptButton = screen.getByRole("button", { name: "Accept" }); expect(passwordToggle).toBeChecked(); await user.click(passwordToggle); diff --git a/web/src/components/users/RootUserForm.tsx b/web/src/components/users/RootUserForm.tsx index 9165ecffa9..7a45eb6e7b 100644 --- a/web/src/components/users/RootUserForm.tsx +++ b/web/src/components/users/RootUserForm.tsx @@ -25,16 +25,14 @@ import { ActionGroup, Alert, Button, - Card, - CardBody, + Checkbox, Content, FileUpload, Form, FormGroup, - Switch, } from "@patternfly/react-core"; import { useNavigate } from "react-router-dom"; -import { Page, PasswordAndConfirmationInput } from "~/components/core"; +import { NestedContent, Page, PasswordAndConfirmationInput } from "~/components/core"; import { useRootUser, useRootUserMutation } from "~/queries/users"; import { RootUser } from "~/types/users"; import { isEmpty } from "~/utils"; @@ -148,53 +146,47 @@ const RootUserForm = () => { ))} )} - toggleMethod("password")} - /> - } - > - {activeMethods.password && ( - - - {usingHashedPassword ? ( - - {_("Using a hashed password.")}{" "} - - - ) : ( - - )} - - - )} + + toggleMethod("password")} + /> + {activeMethods.password && ( + + {usingHashedPassword ? ( + + {_("Using a hashed password.")}{" "} + + + ) : ( + + )} + + )} - toggleMethod("sshPublicKey")} - /> - } - > + + toggleMethod("sshPublicKey")} + /> + + {activeMethods.sshPublicKey && ( - - - - - + + + )} From 39a266322ad5892035a092e2ea323b228fabb554 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 19 Mar 2025 12:39:40 +0000 Subject: [PATCH 5/5] doc(web): update changes file --- web/package/agama-web-ui.changes | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index 6b23968f83..e3cd032e56 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Mar 26 08:25:34 UTC 2025 - David Diaz + +- Change switches by checkboxes (gh#agama-project/agama#2168). +- Restructure the encryption form (gh#agama-project/agama#2168). + ------------------------------------------------------------------- Mon Mar 24 10:17:30 UTC 2025 - Imobach Gonzalez Sosa