From a7594559d4efb45072738f6c464e8bd9b1796d60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 13:52:19 +0100 Subject: [PATCH 01/15] fix(web): better layout for transactional alert The storage alert for a transactional root file system was placed in the header to keep it sticky there all the time. However, we've concluded that should be enough to put it at the very top of the page with a better, more prominent presentation. To so so, it stop using the custom Agama/Reminder component in favor of PatternFly/Alert. --- web/src/components/storage/ProposalPage.jsx | 8 +++++--- web/src/components/storage/ProposalTransactionalInfo.jsx | 9 ++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index 7b9708ec1d..eff3bbcfa3 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -264,12 +264,14 @@ export default function ProposalPage() { <>

{_("Storage")}

- + + + {description}; + return {description}; } From 5558dd58e6675183aadd991e831e5c8c37348cc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 13:56:39 +0100 Subject: [PATCH 02/15] fix(web): drop core/Reminder component --- web/src/components/core/Reminder.jsx | 86 ----------------------- web/src/components/core/Reminder.test.jsx | 71 ------------------- web/src/components/core/index.js | 1 - 3 files changed, 158 deletions(-) delete mode 100644 web/src/components/core/Reminder.jsx delete mode 100644 web/src/components/core/Reminder.test.jsx diff --git a/web/src/components/core/Reminder.jsx b/web/src/components/core/Reminder.jsx deleted file mode 100644 index cd4e0943ad..0000000000 --- a/web/src/components/core/Reminder.jsx +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright (c) [2024] SUSE LLC - * - * All Rights Reserved. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as published - * by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for - * more details. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, contact SUSE LLC. - * - * To contact SUSE LLC about this file by physical or electronic mail, you may - * find current contact information at www.suse.com. - */ - -// @ts-check - -import React from "react"; -import { Icon } from "~/components/layout"; - -/** - * Internal component for rendering the icon - * - * @param {object} props - * @params {string} [props.name] - The icon name. - */ -const ReminderIcon = ({ name }) => { - if (!name?.length) return; - - return ( -
- -
- ); -}; - -/** - * Internal component for rendering the title - * - * @param {object} props - * @params {JSX.Element|string} [props.children] - The title content. - */ -const ReminderTitle = ({ children }) => { - if (!children) return; - if (typeof children === "string" && !children.length) return; - - return ( -

{children}

- ); -}; - -/** - * Renders a reminder with given role, status by default - * @component - * - * @param {object} props - * @param {string} [props.icon] - The name of desired icon. - * @param {JSX.Element|string} [props.title] - The content for the title. - * @param {string} [props.role="status"] - The reminder's role, "status" by default. - * @param {("subtle")} [props.variant] - The reminder's variant, none by default. - * default. See {@link https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/status_role} - * @param {JSX.Element} [props.children] - The content for the description. - */ -export default function Reminder ({ - icon, - title, - role = "status", - variant, - children -}) { - return ( -
- -
- {title} - { children } -
-
- ); -} diff --git a/web/src/components/core/Reminder.test.jsx b/web/src/components/core/Reminder.test.jsx deleted file mode 100644 index 41ebfaf300..0000000000 --- a/web/src/components/core/Reminder.test.jsx +++ /dev/null @@ -1,71 +0,0 @@ -/* - * Copyright (c) [2023] SUSE LLC - * - * All Rights Reserved. - * - * This program is free software; you can redistribute it and/or modify it - * under the terms of version 2 of the GNU General Public License as published - * by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for - * more details. - * - * You should have received a copy of the GNU General Public License along - * with this program; if not, contact SUSE LLC. - * - * To contact SUSE LLC about this file by physical or electronic mail, you may - * find current contact information at www.suse.com. - */ - -import React from "react"; -import { screen, within } from "@testing-library/react"; -import { plainRender } from "~/test-utils"; -import { Reminder } from "~/components/core"; - -describe("Reminder", () => { - it("renders a status region by default", () => { - plainRender(Example); - const reminder = screen.getByRole("status"); - within(reminder).getByText("Example"); - }); - - it("renders a region with given role", () => { - plainRender(Example); - const reminder = screen.getByRole("alert"); - within(reminder).getByText("Example"); - }); - - it("renders a region with given data-variant, if any", () => { - plainRender(Example); - const reminder = screen.getByRole("alert"); - expect(reminder).toHaveAttribute("data-variant", "subtle"); - }); - - it("renders given title", () => { - plainRender( - Kindly reminder}> - Visit the settings section - - ); - screen.getByRole("heading", { name: "Kindly reminder", level: 4 }); - }); - - it("does not render a heading if title is not given", () => { - plainRender(Without title); - expect(screen.queryByRole("heading")).toBeNull(); - }); - - it("does not render a heading if title is an empty string", () => { - plainRender(Without title); - expect(screen.queryByRole("heading")).toBeNull(); - }); - - it("renders given children", () => { - plainRender( - Visit the settings section - ); - screen.getByRole("link", { name: "Visit the settings section" }); - }); -}); diff --git a/web/src/components/core/index.js b/web/src/components/core/index.js index f1e9fc2b16..f4c43161fb 100644 --- a/web/src/components/core/index.js +++ b/web/src/components/core/index.js @@ -52,7 +52,6 @@ export { default as Selector } from "./Selector"; export { default as ServerError } from "./ServerError"; export { default as ExpandableSelector } from "./ExpandableSelector"; export { default as OptionsPicker } from "./OptionsPicker"; -export { default as Reminder } from "./Reminder"; export { default as TreeTable } from "./TreeTable"; export { default as CardField } from "./CardField"; export { default as ButtonLink } from "./ButtonLink"; From 31a9e8d45e58e84426a232b65ab1310b201aa683 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 15:51:58 +0100 Subject: [PATCH 03/15] fix(web): shows TPM hint when proceed There were two problems: - The code for getting the encryption data was commented out. - The condition for displaying the hint was negated (for testing purposes) This changes fix these problems and bring back the unit tests. --- .../components/core/InstallationFinished.jsx | 9 +++----- .../core/InstallationFinished.test.jsx | 22 +++++-------------- 2 files changed, 8 insertions(+), 23 deletions(-) diff --git a/web/src/components/core/InstallationFinished.jsx b/web/src/components/core/InstallationFinished.jsx index 0c6c7e5eea..70acd4b99b 100644 --- a/web/src/components/core/InstallationFinished.jsx +++ b/web/src/components/core/InstallationFinished.jsx @@ -74,12 +74,9 @@ function InstallationFinished() { const iguana = await client.manager.useIguana(); // FIXME: This logic should likely not be placed here, it's too coupled to storage internals. // Something to fix when this whole page is refactored in a (hopefully near) future. - // const { settings: { encryptionPassword, encryptionMethod } } = await client.storage.proposal.getResult(); - // TODO: The storage client is not adapted to the HTTP API yet. - const encryptionPassword = null; - const encryptionMethod = null; + const { settings: { encryptionPassword, encryptionMethod } } = await client.storage.proposal.getResult(); setUsingIguana(iguana); - setUsingTpm(encryptionPassword?.length && encryptionMethod === EncryptionMethods.TPM); + setUsingTpm(encryptionPassword?.length > 0 && encryptionMethod === EncryptionMethods.TPM); } // TODO: display the page in a loading mode while needed data is being fetched. @@ -107,7 +104,7 @@ function InstallationFinished() { ? _("At this point you can power off the machine.") : _("At this point you can reboot the machine to log in to the new system.")} - {!usingTpm && } + {usingTpm && } diff --git a/web/src/components/core/InstallationFinished.test.jsx b/web/src/components/core/InstallationFinished.test.jsx index 017ffdafc3..eef9d63afc 100644 --- a/web/src/components/core/InstallationFinished.test.jsx +++ b/web/src/components/core/InstallationFinished.test.jsx @@ -34,7 +34,7 @@ const finishInstallationFn = jest.fn(); let encryptionPassword; let encryptionMethod; -describe.skip("InstallationFinished", () => { +describe("InstallationFinished", () => { beforeEach(() => { encryptionPassword = "n0tS3cr3t"; encryptionMethod = EncryptionMethods.LUKS2; @@ -72,7 +72,7 @@ describe.skip("InstallationFinished", () => { expect(finishInstallationFn).toHaveBeenCalled(); }); - describe.skip("when TPM is set as encryption method", () => { + describe("when TPM is set as encryption method", () => { beforeEach(() => { encryptionMethod = EncryptionMethods.TPM; }); @@ -90,27 +90,15 @@ describe.skip("InstallationFinished", () => { }); it("does not show the TPM reminder", async () => { - const { user } = installerRender(); - // Forcing the test to slow down a bit with a fake user interaction - // because actually the reminder will be not rendered immediately - // making the queryAllByText to produce a false positive if triggered - // too early here. - const congratsText = screen.getByText("Congratulations!"); - await user.click(congratsText); - expect(screen.queryAllByText(/TPM/)).toHaveLength(0); + installerRender(); + screen.queryAllByText(/TPM/); }); }); }); describe("when TPM is not set as encryption method", () => { it("does not show the TPM reminder", async () => { - const { user } = installerRender(); - // Forcing the test to slow down a bit with a fake user interaction - // because actually the reminder will be not rendered immediately - // making the queryAllByText to produce a false positive if triggered - // too early here. - const congratsText = screen.getByText("Congratulations!"); - await user.click(congratsText); + installerRender(); expect(screen.queryAllByText(/TPM/)).toHaveLength(0); }); }); From 5c56c49f60f5fef7886ed29cfdb5a413ae872508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 15:57:22 +0100 Subject: [PATCH 04/15] fix(web): make TPMHint looks similar to transactional alert --- web/src/components/core/InstallationFinished.jsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/web/src/components/core/InstallationFinished.jsx b/web/src/components/core/InstallationFinished.jsx index 70acd4b99b..c60b2943b3 100644 --- a/web/src/components/core/InstallationFinished.jsx +++ b/web/src/components/core/InstallationFinished.jsx @@ -35,13 +35,14 @@ import { Center, Icon } from "~/components/layout"; import { EncryptionMethods } from "~/client/storage"; import { _ } from "~/i18n"; import { useInstallerClient } from "~/context/installer"; +import alignmentStyles from '@patternfly/react-styles/css/utilities/Alignment/alignment'; const TpmHint = () => { const [isExpanded, setIsExpanded] = useState(false); const title = _("TPM sealing requires the new system to be booted directly."); return ( - {title}}> + {title}}> {_("If a local media was used to run this installer, remove it before the next boot.")} Date: Mon, 17 Jun 2024 21:53:42 +0100 Subject: [PATCH 05/15] fix(web): core/ProgressReport component improvements --- web/src/components/core/ProgressReport.jsx | 56 ++++++++++-------- .../components/core/ProgressReport.test.jsx | 59 ++++++++----------- 2 files changed, 58 insertions(+), 57 deletions(-) diff --git a/web/src/components/core/ProgressReport.jsx b/web/src/components/core/ProgressReport.jsx index f3bf05973f..bdd1a8017e 100644 --- a/web/src/components/core/ProgressReport.jsx +++ b/web/src/components/core/ProgressReport.jsx @@ -20,11 +20,10 @@ */ import React, { useState, useEffect } from "react"; +import { Flex, Progress, Spinner, Text } from "@patternfly/react-core"; import { useCancellablePromise } from "~/utils"; import { useInstallerClient } from "~/context/installer"; -import { Grid, GridItem, Progress, Text } from "@patternfly/react-core"; - const ProgressReport = () => { const client = useInstallerClient(); const { cancellablePromise } = useCancellablePromise(); @@ -54,34 +53,45 @@ const ProgressReport = () => { }); }, [client.software]); - if (!progress.steps) return Waiting for progress status...; + if (!progress.steps) { + return ( + + + Waiting for progress status... + + ); + } return ( - - - + + + { + subProgress && - - + } + ); }; diff --git a/web/src/components/core/ProgressReport.test.jsx b/web/src/components/core/ProgressReport.test.jsx index c22565c604..c70589ea2e 100644 --- a/web/src/components/core/ProgressReport.test.jsx +++ b/web/src/components/core/ProgressReport.test.jsx @@ -32,15 +32,14 @@ jest.mock("~/client"); let callbacks; let onManagerProgressChange = jest.fn(); let onSoftwareProgressChange = jest.fn(); +const getProgressFn = jest.fn(); beforeEach(() => { createClient.mockImplementation(() => { return { manager: { onProgressChange: onManagerProgressChange, - getProgress: jest.fn().mockResolvedValue( - { message: "Reading repositories", current: 1, total: 10 } - ) + getProgress: getProgressFn }, software: { onProgressChange: onSoftwareProgressChange @@ -51,10 +50,13 @@ beforeEach(() => { describe("ProgressReport", () => { describe("when there is not progress information available", () => { - it.skip("renders a waiting message", () => { - installerRender(); + beforeEach(() => { + getProgressFn.mockResolvedValue({}); + }); - expect(screen.findByText(/Waiting for/i)).toBeInTheDocument(); + it("renders a waiting message", async () => { + installerRender(); + await screen.findByText(/Waiting for progress status/i); }); }); @@ -64,13 +66,15 @@ describe("ProgressReport", () => { const [onSoftwareProgress, softwareCallbacks] = createCallbackMock(); onManagerProgressChange = onManagerProgress; onSoftwareProgressChange = onSoftwareProgress; + getProgressFn.mockResolvedValue( + { message: "Reading repositories", current: 1, total: 10 } + ); callbacks = { manager: managerCallbacks, software: softwareCallbacks }; }); it("shows the main progress bar", async () => { installerRender(); - await screen.findByText(/Waiting/i); await screen.findByText(/Reading/i); // NOTE: there can be more than one subscriptions to the @@ -80,43 +84,30 @@ describe("ProgressReport", () => { cb({ message: "Partitioning", current: 1, total: 10 }); }); - await screen.findByLabelText("Partitioning"); + await screen.findByRole("progressbar", { name: "Partitioning" }); }); - it("does not show secondary progress bar", async () => { + it("shows secondary progress bar when there is information from software service ", async () => { installerRender(); - await screen.findByText(/Waiting/i); + const managerCallback = callbacks.manager[callbacks.manager.length - 1]; + const softwareCallback = callbacks.software[callbacks.software.length - 1]; - const cb = callbacks.manager[callbacks.manager.length - 1]; act(() => { - cb({ message: "Partitioning", current: 1, total: 10 }); + managerCallback({ message: "Partitioning", current: 1, total: 10 }); }); - await screen.findAllByRole("progressbar", { hidden: true }); - }); + await screen.findByRole("progressbar", { name: "Partitioning" }); + const bars = await screen.findAllByRole("progressbar"); + expect(bars.length).toBe(1); - describe("when there is progress information from the software service", () => { - it("shows the secondary progress bar", async () => { - installerRender(); - - await screen.findByText(/Waiting/i); - - // NOTE: there can be more than one subscriptions to the - // manager#onChange. We're interested in the latest one here. - const cb0 = callbacks.manager[callbacks.manager.length - 1]; - act(() => { - cb0({ message: "Installing software", current: 4, total: 10 }); - }); - - const cb1 = callbacks.software[callbacks.software.length - 1]; - act(() => { - cb1({ message: "Installing YaST2", current: 256, total: 500, finished: false }); - }); - - const bars = screen.queryAllByRole("progressbar", { hidden: false }); - expect(bars.length).toEqual(2); + act(() => { + managerCallback({ message: "Installing software", current: 4, total: 10 }); + softwareCallback({ message: "Installing YaST2", current: 256, total: 500, finished: false }); }); + + await screen.findByRole("progressbar", { name: "Installing software" }); + await screen.findByRole("progressbar", { name: "Installing YaST2" }); }); }); }); From 9d34aa8bff65f1e0e419e9018b8c9d120bbee51c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 22:09:07 +0100 Subject: [PATCH 06/15] fix(web): BootSelection manual mode The Accept button does not get disabled when switching to the manual mode. To do so, the code ensures the first device is selected when proceed, keeping consistent with the option rendered in the UI. --- web/src/components/storage/BootSelection.jsx | 9 ++---- ...Dialog.test.jsx => BootSelection.test.jsx} | 32 +++++++++---------- 2 files changed, 18 insertions(+), 23 deletions(-) rename web/src/components/storage/{BootSelectionDialog.test.jsx => BootSelection.test.jsx} (87%) diff --git a/web/src/components/storage/BootSelection.jsx b/web/src/components/storage/BootSelection.jsx index db1b84c295..00448860cd 100644 --- a/web/src/components/storage/BootSelection.jsx +++ b/web/src/components/storage/BootSelection.jsx @@ -39,6 +39,8 @@ import { useCancellablePromise } from "~/utils"; import { useInstallerClient } from "~/context/installer"; import textStyles from '@patternfly/react-styles/css/utilities/Text/text'; +// FIXME: improve and rename to BootSelectionDialog + /** * @typedef {import ("~/client/storage").StorageDevice} StorageDevice */ @@ -46,7 +48,6 @@ import textStyles from '@patternfly/react-styles/css/utilities/Text/text'; const BOOT_AUTO_ID = "boot-auto"; const BOOT_MANUAL_ID = "boot-manual"; const BOOT_DISABLED_ID = "boot-disabled"; -const OPTIONS_NAME = "boot-mode"; /** * Allows the user to select the boot configuration. @@ -76,8 +77,6 @@ export default function BootSelectionDialog() { const availableDevices = await loadAvailableDevices(); const { bootDevice, configureBoot, defaultBootDevice } = settings; - console.log(settings); - if (!configureBoot) { selectedOption = BOOT_DISABLED_ID; } else if (configureBoot && bootDevice === "") { @@ -88,7 +87,7 @@ export default function BootSelectionDialog() { setState({ load: true, - bootDevice: availableDevices.find(d => d.name === bootDevice), + bootDevice: availableDevices.find(d => d.name === bootDevice) || availableDevices[0], configureBoot, defaultBootDevice, availableDevices, @@ -113,8 +112,6 @@ export default function BootSelectionDialog() { bootDevice: state.selectedOption === BOOT_MANUAL_ID ? state.bootDevice.name : undefined, }; - console.log("newSettings", newSettings); - await client.proposal.calculate({ ...settings, ...newSettings }); navigate(".."); }; diff --git a/web/src/components/storage/BootSelectionDialog.test.jsx b/web/src/components/storage/BootSelection.test.jsx similarity index 87% rename from web/src/components/storage/BootSelectionDialog.test.jsx rename to web/src/components/storage/BootSelection.test.jsx index 472da2cb26..19d1688f61 100644 --- a/web/src/components/storage/BootSelectionDialog.test.jsx +++ b/web/src/components/storage/BootSelection.test.jsx @@ -24,10 +24,9 @@ import React from "react"; import { screen, within } from "@testing-library/react"; import { plainRender } from "~/test-utils"; -import { BootSelectionDialog } from "~/components/storage"; +import BootSelection from "./BootSelection"; /** - * @typedef {import("./BootSelectionDialog").BootSelectionDialogProps} BootSelectionDialogProps * @typedef {import ("~/client/storage").StorageDevice} StorageDevice */ @@ -49,7 +48,7 @@ const sda = { description: "", size: 1024, recoverableSize: 0, - systems : [], + systems: [], udevIds: ["ata-Micron_1100_SATA_512GB_12563", "scsi-0ATA_Micron_1100_SATA_512GB"], udevPaths: ["pci-0000:00-12", "pci-0000:00-12-ata"], }; @@ -72,7 +71,7 @@ const sdb = { description: "", size: 2048, recoverableSize: 0, - systems : [], + systems: [], udevIds: [], udevPaths: ["pci-0000:00-19"] }; @@ -95,15 +94,14 @@ const sdc = { description: "", size: 2048, recoverableSize: 0, - systems : [], + systems: [], udevIds: [], udevPaths: ["pci-0000:00-19"] }; -/** @type {BootSelectionDialogProps} */ let props; -describe.skip("BootSelectionDialog", () => { +describe.skip("BootSelection", () => { beforeEach(() => { props = { isOpen: true, @@ -122,18 +120,18 @@ describe.skip("BootSelectionDialog", () => { const diskSelector = () => screen.queryByRole("combobox", { name: /choose a disk/i }); it("offers an option to configure boot in the installation disk", () => { - plainRender(); + plainRender(); expect(automaticOption()).toBeInTheDocument(); }); it("offers an option to configure boot in a selected disk", () => { - plainRender(); + plainRender(); expect(selectDiskOption()).toBeInTheDocument(); expect(diskSelector()).toBeInTheDocument(); }); it("offers an option to not configure boot", () => { - plainRender(); + plainRender(); expect(notConfigureOption()).toBeInTheDocument(); }); @@ -144,7 +142,7 @@ describe.skip("BootSelectionDialog", () => { }); it("selects 'Automatic' option by default", () => { - plainRender(); + plainRender(); expect(automaticOption()).toBeChecked(); expect(selectDiskOption()).not.toBeChecked(); expect(diskSelector()).toBeDisabled(); @@ -159,7 +157,7 @@ describe.skip("BootSelectionDialog", () => { }); it("selects 'Select a disk' option by default", () => { - plainRender(); + plainRender(); expect(automaticOption()).not.toBeChecked(); expect(selectDiskOption()).toBeChecked(); expect(diskSelector()).toBeEnabled(); @@ -174,7 +172,7 @@ describe.skip("BootSelectionDialog", () => { }); it("selects 'Do not configure' option by default", () => { - plainRender(); + plainRender(); expect(automaticOption()).not.toBeChecked(); expect(selectDiskOption()).not.toBeChecked(); expect(diskSelector()).toBeDisabled(); @@ -183,7 +181,7 @@ describe.skip("BootSelectionDialog", () => { }); it("does not call onAccept on cancel", async () => { - const { user } = plainRender(); + const { user } = plainRender(); const cancel = screen.getByRole("button", { name: "Cancel" }); await user.click(cancel); @@ -198,7 +196,7 @@ describe.skip("BootSelectionDialog", () => { }); it("calls onAccept with the selected options on accept", async () => { - const { user } = plainRender(); + const { user } = plainRender(); await user.click(automaticOption()); @@ -219,7 +217,7 @@ describe.skip("BootSelectionDialog", () => { }); it("calls onAccept with the selected options on accept", async () => { - const { user } = plainRender(); + const { user } = plainRender(); await user.click(selectDiskOption()); const selector = diskSelector(); @@ -243,7 +241,7 @@ describe.skip("BootSelectionDialog", () => { }); it("calls onAccept with the selected options on accept", async () => { - const { user } = plainRender(); + const { user } = plainRender(); await user.click(notConfigureOption()); From 182f61251598ad36c1dc278e76af71bbbec57829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 22:28:01 +0100 Subject: [PATCH 07/15] fix(web): Do not include KiB as unit for maximum in Volume/SizeOptionsField --- web/src/components/storage/VolumeFields.jsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/web/src/components/storage/VolumeFields.jsx b/web/src/components/storage/VolumeFields.jsx index d2e7c7913c..a74b52a7ae 100644 --- a/web/src/components/storage/VolumeFields.jsx +++ b/web/src/components/storage/VolumeFields.jsx @@ -39,6 +39,8 @@ import { _, N_ } from "~/i18n"; import { sprintf } from "sprintf-js"; import { SIZE_METHODS, SIZE_UNITS } from '~/components/storage/utils'; +const { K, ...MAX_SIZE_UNITS } = SIZE_UNITS; + /** * @typedef {import ("~/client/storage").Volume} Volume */ @@ -422,7 +424,7 @@ and maximum. If no maximum is given then the file system will be as big as possi /** @ts-expect-error: for some reason using id makes TS complain */ id="maxSizeUnit" aria-label={_("Unit for the maximum size")} - units={Object.values(SIZE_UNITS)} + units={Object.values(MAX_SIZE_UNITS)} value={formData.maxSizeUnit || formData.minSizeUnit} onChange={(_, maxSizeUnit) => onChange({ maxSizeUnit })} isDisabled={isDisabled} From 4adbc58486befd5757d8f20e5889ec3b00744e9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 17 Jun 2024 22:39:50 +0100 Subject: [PATCH 08/15] fix(web): force inline-size of size unit selectors --- web/src/assets/styles/blocks.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss index a148b07f1e..e0ec9c93b6 100644 --- a/web/src/assets/styles/blocks.scss +++ b/web/src/assets/styles/blocks.scss @@ -550,6 +550,10 @@ table.proposal-result { input { text-align: end; } + + select { + min-inline-size: fit-content; + } } [data-type="agama/options-picker"] { From 01432f73947ebe56e032f0746ee978e4e58ea833 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz?= <1691872+dgdavid@users.noreply.github.com> Date: Tue, 18 Jun 2024 10:51:10 +0100 Subject: [PATCH 09/15] Update from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Iván López --- web/src/components/storage/BootSelection.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/storage/BootSelection.jsx b/web/src/components/storage/BootSelection.jsx index 00448860cd..2f83385876 100644 --- a/web/src/components/storage/BootSelection.jsx +++ b/web/src/components/storage/BootSelection.jsx @@ -87,7 +87,7 @@ export default function BootSelectionDialog() { setState({ load: true, - bootDevice: availableDevices.find(d => d.name === bootDevice) || availableDevices[0], + bootDevice: availableDevices.find(d => d.name === bootDevice) || defaultBootDevice || availableDevices[0], configureBoot, defaultBootDevice, availableDevices, From 45986f8cb3749e0dda27a321a998e79fe365f26d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz?= <1691872+dgdavid@users.noreply.github.com> Date: Tue, 18 Jun 2024 14:12:58 +0100 Subject: [PATCH 10/15] Revert changes from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Iván López --- web/src/components/storage/BootSelection.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/storage/BootSelection.jsx b/web/src/components/storage/BootSelection.jsx index 2f83385876..00448860cd 100644 --- a/web/src/components/storage/BootSelection.jsx +++ b/web/src/components/storage/BootSelection.jsx @@ -87,7 +87,7 @@ export default function BootSelectionDialog() { setState({ load: true, - bootDevice: availableDevices.find(d => d.name === bootDevice) || defaultBootDevice || availableDevices[0], + bootDevice: availableDevices.find(d => d.name === bootDevice) || availableDevices[0], configureBoot, defaultBootDevice, availableDevices, From ae0a12a8d867e76043c2c62188c9bed13295da79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Tue, 18 Jun 2024 14:57:38 +0100 Subject: [PATCH 11/15] web: fix default boot device --- web/src/components/storage/BootSelection.jsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/web/src/components/storage/BootSelection.jsx b/web/src/components/storage/BootSelection.jsx index 00448860cd..680c1767cf 100644 --- a/web/src/components/storage/BootSelection.jsx +++ b/web/src/components/storage/BootSelection.jsx @@ -85,11 +85,13 @@ export default function BootSelectionDialog() { selectedOption = BOOT_MANUAL_ID; } + const findDevice = (name) => availableDevices.find(d => d.name === name); + setState({ load: true, - bootDevice: availableDevices.find(d => d.name === bootDevice) || availableDevices[0], + bootDevice: findDevice(bootDevice) || findDevice(defaultBootDevice) || availableDevices[0], configureBoot, - defaultBootDevice, + defaultBootDevice: findDevice(defaultBootDevice), availableDevices, selectedOption }); From ef59b86105f2681a9be899e5b30cdfd60132dedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Tue, 18 Jun 2024 14:58:28 +0100 Subject: [PATCH 12/15] rust: include defaultBootDevice --- rust/agama-lib/src/storage/model.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/agama-lib/src/storage/model.rs b/rust/agama-lib/src/storage/model.rs index 67e586bc54..84bc9c7951 100644 --- a/rust/agama-lib/src/storage/model.rs +++ b/rust/agama-lib/src/storage/model.rs @@ -264,6 +264,7 @@ pub struct ProposalSettings { pub target_pv_devices: Option>, pub configure_boot: bool, pub boot_device: String, + pub default_boot_device: String, pub encryption_password: String, pub encryption_method: String, #[serde(rename = "encryptionPBKDFunction")] @@ -283,6 +284,7 @@ impl TryFrom> for ProposalSettings { target_pv_devices: get_optional_property(&hash, "TargetPVDevices")?, configure_boot: get_property(&hash, "ConfigureBoot")?, boot_device: get_property(&hash, "BootDevice")?, + default_boot_device: get_property(&hash, "DefaultBootDevice")?, encryption_password: get_property(&hash, "EncryptionPassword")?, encryption_method: get_property(&hash, "EncryptionMethod")?, encryption_pbkd_function: get_property(&hash, "EncryptionPBKDFunction")?, From b4819ac853f007931b9343fb2bed088c5979748e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Tue, 18 Jun 2024 14:58:59 +0100 Subject: [PATCH 13/15] rust: fix snapshots affect sizes --- rust/agama-lib/src/storage/model.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/agama-lib/src/storage/model.rs b/rust/agama-lib/src/storage/model.rs index 84bc9c7951..3ef15cafa3 100644 --- a/rust/agama-lib/src/storage/model.rs +++ b/rust/agama-lib/src/storage/model.rs @@ -375,7 +375,7 @@ pub struct VolumeOutline { support_auto_size: bool, adjust_by_ram: bool, snapshots_configurable: bool, - snaphosts_affect_sizes: bool, + snapshots_affect_sizes: bool, size_relevant_volumes: Vec, } @@ -390,7 +390,7 @@ impl TryFrom> for VolumeOutline { support_auto_size: get_property(&mvalue, "SupportAutoSize")?, adjust_by_ram: get_property(&mvalue, "AdjustByRam")?, snapshots_configurable: get_property(&mvalue, "SnapshotsConfigurable")?, - snaphosts_affect_sizes: get_property(&mvalue, "SnapshotsAffectSizes")?, + snapshots_affect_sizes: get_property(&mvalue, "SnapshotsAffectSizes")?, size_relevant_volumes: get_property(&mvalue, "SizeRelevantVolumes")?, }; From 4af4b9c3d766c95402acee1b61e38c567a6528c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 19 Jun 2024 09:27:19 +0100 Subject: [PATCH 14/15] service: only set calculated sizes if size is auto --- .../volume_conversion/from_y2storage.rb | 20 ++++--- .../volume_conversion/from_y2storage_test.rb | 52 ++++++++++++++----- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/service/lib/agama/storage/volume_conversion/from_y2storage.rb b/service/lib/agama/storage/volume_conversion/from_y2storage.rb index 01415a4135..0c493fe707 100644 --- a/service/lib/agama/storage/volume_conversion/from_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/from_y2storage.rb @@ -48,14 +48,22 @@ def convert # @return [Agama::Storage::ProposalSettings] attr_reader :volume + # Recovers the range of sizes used by the Y2Storage proposal, if needed. + # + # If the volume is configured to use auto sizes, then the final range of sizes used by the + # Y2Storage proposal depends on the fallback sizes (if this volume is fallback for other + # volume) and the size for snapshots (if snapshots is active). The planned device contains + # the real range of sizes used by the Y2Storage proposal. + # + # FIXME: Recovering the sizes from the planned device is done to know the range of sizes + # assigned to the volume and to present that information in the UI. But such information + # should be provided in a different way, for example as part of the proposal result + # reported on D-Bus: { success:, settings:, strategy:, computed_sizes: }. + # # @param target [Agama::Storage::Volume] def sizes_conversion(target) - # The final range of sizes used by the Y2Storage proposal depends on the fallback sizes - # (if this volume is fallback for other volume) and the size for snapshots (if snapshots - # is active). The planned device contains the real range of sizes used by the proposal. - # - # From Agama point of view, this is the way of recovering the range of sizes used by - # Y2Storage when a volume is set to have auto size. + return unless target.auto_size? + planned = planned_device_for(target.mount_path) return unless planned diff --git a/service/test/agama/storage/volume_conversion/from_y2storage_test.rb b/service/test/agama/storage/volume_conversion/from_y2storage_test.rb index 55da4f8edb..ceecba866b 100644 --- a/service/test/agama/storage/volume_conversion/from_y2storage_test.rb +++ b/service/test/agama/storage/volume_conversion/from_y2storage_test.rb @@ -89,35 +89,59 @@ instance_double(Y2Storage::MinGuidedProposal, planned_devices: planned_devices) end - context "if there is a planned device for the volume" do - let(:planned_devices) { [planned_volume] } + let(:planned_devices) { [planned_volume] } - let(:planned_volume) do - Y2Storage::Planned::LvmLv.new("/").tap do |planned| - planned.min = Y2Storage::DiskSize.GiB(10) - planned.max = Y2Storage::DiskSize.GiB(40) + context "if the volume is configured with auto size" do + before do + volume.auto_size = true + end + + context "if there is a planned device for the volume" do + let(:planned_volume) do + Y2Storage::Planned::LvmLv.new("/").tap do |planned| + planned.min = Y2Storage::DiskSize.GiB(10) + planned.max = Y2Storage::DiskSize.GiB(40) + end + end + + it "sets the min and max sizes according to the planned device" do + result = subject.convert + + expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(10)) + expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(40)) end end - it "sets the min and max sizes according to the planned device" do - result = subject.convert + context "if there is no planned device for the volume" do + let(:planned_volume) do + Y2Storage::Planned::LvmLv.new("/home").tap do |planned| + planned.min = Y2Storage::DiskSize.GiB(10) + planned.max = Y2Storage::DiskSize.GiB(40) + end + end + + it "keeps the sizes of the given volume" do + result = subject.convert - expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(10)) - expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(40)) + expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(5)) + expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(20)) + end end end - context "if there is no planned device for the volume" do - let(:planned_devices) { [planned_volume] } + context "if the volume is not configured with auto size" do + before do + volume.auto_size = false + end let(:planned_volume) do - Y2Storage::Planned::LvmLv.new("/home").tap do |planned| + Y2Storage::Planned::LvmLv.new("/").tap do |planned| planned.min = Y2Storage::DiskSize.GiB(10) planned.max = Y2Storage::DiskSize.GiB(40) end end - it "sets the sizes of the given volume" do + it "keeps the sizes of the given volume" do result = subject.convert expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(5)) From e183ea88429bb283dcb9966ffbdc80878e87d319 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Wed, 19 Jun 2024 09:37:31 +0100 Subject: [PATCH 15/15] Add vfat option for arbitrary mount points --- products.d/tumbleweed.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/products.d/tumbleweed.yaml b/products.d/tumbleweed.yaml index 767081178b..ca7fade155 100644 --- a/products.d/tumbleweed.yaml +++ b/products.d/tumbleweed.yaml @@ -213,3 +213,4 @@ storage: - ext3 - ext4 - xfs + - vfat