From e5f0ef0fbaafc8ca0381780720876cd018135e43 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 Mar 2025 15:42:50 +0000 Subject: [PATCH 01/11] web: add #isUsed to model drives - The method #buildModel is extracted to a helper because it will be used by other hooks and/or helpers. --- web/src/hooks/storage/helpers/build-model.ts | 110 +++++++++++++++++++ web/src/hooks/storage/model.ts | 62 +---------- web/src/types/storage/model.ts | 1 + 3 files changed, 112 insertions(+), 61 deletions(-) create mode 100644 web/src/hooks/storage/helpers/build-model.ts diff --git a/web/src/hooks/storage/helpers/build-model.ts b/web/src/hooks/storage/helpers/build-model.ts new file mode 100644 index 0000000000..ef43c1c99e --- /dev/null +++ b/web/src/hooks/storage/helpers/build-model.ts @@ -0,0 +1,110 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 { apiModel } from "~/api/storage/types"; +import { model } from "~/types/storage"; + +const findDrive = (model: model.Model, name: string): model.Drive | undefined => { + return model.drives.find((d) => d.name === name); +}; + +function buildDrive( + apiDrive: apiModel.Drive, + apiModel: apiModel.Config, + model: model.Model, +): model.Drive { + const getVolumeGroups = (): model.VolumeGroup[] => { + return model.volumeGroups.filter((v) => + v.getTargetDevices().some((d) => d.name === apiDrive.name), + ); + }; + + const isExplicitBoot = (): boolean => { + return ( + apiModel.boot?.configure && + !apiModel.boot.device?.default && + apiModel.boot.device?.name === apiDrive.name + ); + }; + + const isTargetDevice = (): boolean => { + const targetDevices = (apiModel.volumeGroups || []).flatMap((v) => v.targetDevices || []); + return targetDevices.includes(apiDrive.name); + }; + + const isUsed = (): boolean => { + return ( + isExplicitBoot() || + isTargetDevice() || + apiDrive.mountPath !== undefined || + apiDrive.partitions?.some((p) => p.mountPath) + ); + }; + + return { + ...apiDrive, + isUsed: isUsed(), + getVolumeGroups, + }; +} + +function buildLogicalVolume(logicalVolumeData: apiModel.LogicalVolume): model.LogicalVolume { + return { ...logicalVolumeData }; +} + +function buildVolumeGroup( + apiVolumeGroup: apiModel.VolumeGroup, + model: model.Model, +): model.VolumeGroup { + const buildLogicalVolumes = (): model.LogicalVolume[] => { + return (apiVolumeGroup.logicalVolumes || []).map(buildLogicalVolume); + }; + + const findTargetDevices = (): model.Drive[] => { + return (apiVolumeGroup.targetDevices || []).map((d) => findDrive(model, d)).filter((d) => d); + }; + + return { + ...apiVolumeGroup, + logicalVolumes: buildLogicalVolumes(), + getTargetDevices: findTargetDevices, + }; +} + +export default function buildModel(apiModel: apiModel.Config): model.Model { + const model: model.Model = { + drives: [], + volumeGroups: [], + }; + + const buildDrives = (): model.Drive[] => { + return (apiModel.drives || []).map((d) => buildDrive(d, apiModel, model)); + }; + + const buildVolumeGroups = (): model.VolumeGroup[] => { + return (apiModel.volumeGroups || []).map((v) => buildVolumeGroup(v, model)); + }; + + // Important! Modify the model object instead of assigning a new one. + model.drives = buildDrives(); + model.volumeGroups = buildVolumeGroups(); + return model; +} diff --git a/web/src/hooks/storage/model.ts b/web/src/hooks/storage/model.ts index 630f82500d..ad0b83b930 100644 --- a/web/src/hooks/storage/model.ts +++ b/web/src/hooks/storage/model.ts @@ -22,70 +22,10 @@ import { useMemo } from "react"; import useApiModel from "~/hooks/storage/api-model"; +import buildModel from "~/hooks/storage/helpers/build-model"; import { QueryHookOptions } from "~/types/queries"; -import { apiModel } from "~/api/storage/types"; import { model } from "~/types/storage"; -const findDrive = (model: model.Model, name: string): model.Drive | undefined => { - return model.drives.find((d) => d.name === name); -}; - -function buildDrive(apiDrive: apiModel.Drive, model: model.Model): model.Drive { - const findVolumeGroups = (targetName: string): model.VolumeGroup[] => { - return model.volumeGroups.filter((v) => - v.getTargetDevices().some((d) => d.name === targetName), - ); - }; - - return { - ...apiDrive, - getVolumeGroups: () => findVolumeGroups(apiDrive.name), - }; -} - -function buildLogicalVolume(logicalVolumeData: apiModel.LogicalVolume): model.LogicalVolume { - return { ...logicalVolumeData }; -} - -function buildVolumeGroup( - apiVolumeGroup: apiModel.VolumeGroup, - model: model.Model, -): model.VolumeGroup { - const buildLogicalVolumes = (): model.LogicalVolume[] => { - return (apiVolumeGroup.logicalVolumes || []).map(buildLogicalVolume); - }; - - const findTargetDevices = (): model.Drive[] => { - return (apiVolumeGroup.targetDevices || []).map((d) => findDrive(model, d)).filter((d) => d); - }; - - return { - ...apiVolumeGroup, - logicalVolumes: buildLogicalVolumes(), - getTargetDevices: findTargetDevices, - }; -} - -function buildModel(apiModel: apiModel.Config): model.Model { - const model: model.Model = { - drives: [], - volumeGroups: [], - }; - - const buildDrives = (): model.Drive[] => { - return (apiModel.drives || []).map((d) => buildDrive(d, model)); - }; - - const buildVolumeGroups = (): model.VolumeGroup[] => { - return (apiModel.volumeGroups || []).map((v) => buildVolumeGroup(v, model)); - }; - - // Important! Modify the model object instead of assigning a new one. - model.drives = buildDrives(); - model.volumeGroups = buildVolumeGroups(); - return model; -} - function useModel(options?: QueryHookOptions): model.Model | null { const apiModel = useApiModel(options); diff --git a/web/src/types/storage/model.ts b/web/src/types/storage/model.ts index 587744c128..955d09e85b 100644 --- a/web/src/types/storage/model.ts +++ b/web/src/types/storage/model.ts @@ -28,6 +28,7 @@ type Model = { }; interface Drive extends apiModel.Drive { + isUsed: boolean; getVolumeGroups: () => VolumeGroup[]; } From ca4e1d0c64c1d034e7d14241173239859c81e0f9 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 Mar 2025 15:45:49 +0000 Subject: [PATCH 02/11] web: add hook for editing a volume group --- web/src/hooks/storage/add-volume-group.ts | 38 ++--------- web/src/hooks/storage/edit-volume-group.ts | 59 +++++++++++++++++ web/src/hooks/storage/helpers/drive.ts | 43 ++++++++++++ web/src/hooks/storage/helpers/volume-group.ts | 65 +++++++++++++++++++ 4 files changed, 171 insertions(+), 34 deletions(-) create mode 100644 web/src/hooks/storage/edit-volume-group.ts create mode 100644 web/src/hooks/storage/helpers/drive.ts create mode 100644 web/src/hooks/storage/helpers/volume-group.ts diff --git a/web/src/hooks/storage/add-volume-group.ts b/web/src/hooks/storage/add-volume-group.ts index cdd15902b4..fa970ee515 100644 --- a/web/src/hooks/storage/add-volume-group.ts +++ b/web/src/hooks/storage/add-volume-group.ts @@ -22,49 +22,19 @@ import useApiModel from "~/hooks/storage/api-model"; import useUpdateApiModel from "~/hooks/storage/update-api-model"; +import { addVolumeGroup } from "~/hooks/storage/helpers/volume-group"; import { QueryHookOptions } from "~/types/queries"; -import { apiModel } from "~/api/storage/types"; -function toLogicalVolume(partition: apiModel.Partition) { - return { ...partition }; -} - -function movePartitions(drive: apiModel.Drive, volumeGroup: apiModel.VolumeGroup) { - if (!drive.partitions) return; - - const newPartitions = drive.partitions.filter((p) => !p.name); - const reusedPartitions = drive.partitions.filter((p) => p.name); - drive.partitions = [...reusedPartitions]; - const logicalVolumes = volumeGroup.logicalVolumes || []; - volumeGroup.logicalVolumes = [...logicalVolumes, ...newPartitions.map(toLogicalVolume)]; -} - -function addVolumeGroup( - apiModel: apiModel.Config, +export type AddVolumeGroupFn = ( vgName: string, targetDevices: string[], moveContent: boolean, -): apiModel.Config { - const volumeGroup = { vgName, targetDevices }; - if (moveContent) { - (apiModel.drives || []) - .filter((d) => targetDevices.includes(d.name)) - .forEach((d) => movePartitions(d, volumeGroup)); - } - apiModel.volumeGroups ||= []; - apiModel.volumeGroups.push(volumeGroup); - return apiModel; -} +) => void; -type AddVolumeGroupFn = (vgName: string, targetDevices: string[], moveContent: boolean) => void; - -function useAddVolumeGroup(options?: QueryHookOptions): AddVolumeGroupFn { +export default function useAddVolumeGroup(options?: QueryHookOptions): AddVolumeGroupFn { const apiModel = useApiModel(options); const updateApiModel = useUpdateApiModel(); return (vgName: string, targetDevices: string[], moveContent: boolean) => { updateApiModel(addVolumeGroup(apiModel, vgName, targetDevices, moveContent)); }; } - -export { useAddVolumeGroup as default }; -export type { AddVolumeGroupFn }; diff --git a/web/src/hooks/storage/edit-volume-group.ts b/web/src/hooks/storage/edit-volume-group.ts new file mode 100644 index 0000000000..d0fa723e43 --- /dev/null +++ b/web/src/hooks/storage/edit-volume-group.ts @@ -0,0 +1,59 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 useApiModel from "~/hooks/storage/api-model"; +import useUpdateApiModel from "~/hooks/storage/update-api-model"; +import { addVolumeGroup } from "~/hooks/storage/helpers/volume-group"; +import { deleteIfUnused } from "~/hooks/storage/helpers/drive"; +import { QueryHookOptions } from "~/types/queries"; +import { apiModel } from "~/api/storage/types"; + +function editVolumeGroup( + apiModel: apiModel.Config, + oldVgName: string, + vgName: string, + targetDevices: string[], +): apiModel.Config { + const index = (apiModel.volumeGroups || []).findIndex((v) => v.vgName === oldVgName); + if (index === -1) return; + + const oldTargetDevices = apiModel.volumeGroups[index].targetDevices || []; + + addVolumeGroup(apiModel, vgName, targetDevices, false, index); + oldTargetDevices.forEach((d) => deleteIfUnused(apiModel, d)); + + return apiModel; +} + +export type EditVolumeGroupFn = ( + odlVgName: string, + VgName: string, + targetDevices: string[], +) => void; + +export default function useEditVolumeGroup(options?: QueryHookOptions): EditVolumeGroupFn { + const apiModel = useApiModel(options); + const updateApiModel = useUpdateApiModel(); + return (oldVgName: string, vgName: string, targetDevices: string[]) => { + updateApiModel(editVolumeGroup(apiModel, oldVgName, vgName, targetDevices)); + }; +} diff --git a/web/src/hooks/storage/helpers/drive.ts b/web/src/hooks/storage/helpers/drive.ts new file mode 100644 index 0000000000..13e17636a4 --- /dev/null +++ b/web/src/hooks/storage/helpers/drive.ts @@ -0,0 +1,43 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 { apiModel } from "~/api/storage/types"; +import { model } from "~/types/storage"; +import buildModel from "~/hooks/storage/helpers/build-model"; + +function buildDrive(apiModel: apiModel.Config, name: string): model.Drive | undefined { + const model = buildModel(apiModel); + return model.drives.find((d) => d.name === name); +} + +function deleteIfUnused(apiModel: apiModel.Config, name: string) { + const index = (apiModel.drives || []).findIndex((d) => d.name === name); + if (index === -1) return; + + const drive = buildDrive(apiModel, name); + if (!drive || drive.isUsed) return; + + apiModel.drives.splice(index, 1); + return apiModel; +} + +export { deleteIfUnused }; diff --git a/web/src/hooks/storage/helpers/volume-group.ts b/web/src/hooks/storage/helpers/volume-group.ts new file mode 100644 index 0000000000..6be4365d62 --- /dev/null +++ b/web/src/hooks/storage/helpers/volume-group.ts @@ -0,0 +1,65 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 { apiModel } from "~/api/storage/types"; + +function toLogicalVolume(partition: apiModel.Partition) { + return { ...partition }; +} + +function movePartitions(drive: apiModel.Drive, volumeGroup: apiModel.VolumeGroup) { + if (!drive.partitions) return; + + const newPartitions = drive.partitions.filter((p) => !p.name); + const reusedPartitions = drive.partitions.filter((p) => p.name); + drive.partitions = [...reusedPartitions]; + const logicalVolumes = volumeGroup.logicalVolumes || []; + volumeGroup.logicalVolumes = [...logicalVolumes, ...newPartitions.map(toLogicalVolume)]; +} + +function addVolumeGroup( + apiModel: apiModel.Config, + vgName: string, + targetDevices: string[], + moveContent: boolean, + index?: number, +): apiModel.Config { + const volumeGroup = { vgName, targetDevices }; + + if (moveContent) { + (apiModel.drives || []) + .filter((d) => targetDevices.includes(d.name)) + .forEach((d) => movePartitions(d, volumeGroup)); + } + + apiModel.volumeGroups ||= []; + + if (index === undefined || index >= apiModel.volumeGroups.length) { + apiModel.volumeGroups.push(volumeGroup); + } else { + apiModel.volumeGroups.splice(index, 1, volumeGroup); + } + + return apiModel; +} + +export { addVolumeGroup }; From e3c2238d4c5075d46a621ef99b99e7e8de7ef95f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Thu, 20 Mar 2025 11:02:37 +0000 Subject: [PATCH 03/11] web: reorganize storage paths --- .../storage/ConfigEditorMenu.test.tsx | 2 +- .../components/storage/ConfigEditorMenu.tsx | 4 ++-- .../components/storage/DriveEditor.test.tsx | 2 +- web/src/components/storage/DriveEditor.tsx | 12 +++++++---- .../storage/EncryptionSection.test.tsx | 2 +- .../components/storage/EncryptionSection.tsx | 2 +- web/src/routes/paths.ts | 20 +++++++++++-------- web/src/routes/storage.tsx | 18 ++++++++--------- 8 files changed, 35 insertions(+), 27 deletions(-) diff --git a/web/src/components/storage/ConfigEditorMenu.test.tsx b/web/src/components/storage/ConfigEditorMenu.test.tsx index 8fd8124006..d28568b246 100644 --- a/web/src/components/storage/ConfigEditorMenu.test.tsx +++ b/web/src/components/storage/ConfigEditorMenu.test.tsx @@ -68,7 +68,7 @@ it("allows users to change the boot options", async () => { const { user, menu } = await openMenu(); const bootItem = within(menu).getByRole("menuitem", { name: /boot options/ }); await user.click(bootItem); - expect(mockNavigateFn).toHaveBeenCalledWith(PATHS.bootDevice); + expect(mockNavigateFn).toHaveBeenCalledWith(PATHS.editBootDevice); }); it("allows users to reset the config", async () => { diff --git a/web/src/components/storage/ConfigEditorMenu.tsx b/web/src/components/storage/ConfigEditorMenu.tsx index 2f3238243c..095a01737c 100644 --- a/web/src/components/storage/ConfigEditorMenu.tsx +++ b/web/src/components/storage/ConfigEditorMenu.tsx @@ -64,7 +64,7 @@ export default function ConfigEditorMenu() { navigate(PATHS.lvm.create)} + onClick={() => navigate(PATHS.volumeGroup.add)} description={_("Extend the installation using LVM")} > {_("Add LVM volume group")} @@ -72,7 +72,7 @@ export default function ConfigEditorMenu() { navigate(PATHS.bootDevice)} + onClick={() => navigate(PATHS.editBootDevice)} description={_("Select the disk to configure partitions for booting")} > {_("Change boot options")} diff --git a/web/src/components/storage/DriveEditor.test.tsx b/web/src/components/storage/DriveEditor.test.tsx index ea5b55dae0..48ba96aa98 100644 --- a/web/src/components/storage/DriveEditor.test.tsx +++ b/web/src/components/storage/DriveEditor.test.tsx @@ -215,7 +215,7 @@ describe("PartitionMenuItem", () => { name: "Edit swap", }); await user.click(editSwapButton); - expect(mockNavigateFn).toHaveBeenCalledWith("/storage/devices/sda/partitions/swap/edit"); + expect(mockNavigateFn).toHaveBeenCalledWith("/storage/drives/sda/partitions/swap/edit"); }); }); diff --git a/web/src/components/storage/DriveEditor.tsx b/web/src/components/storage/DriveEditor.tsx index 605b2ef27c..e212a69967 100644 --- a/web/src/components/storage/DriveEditor.tsx +++ b/web/src/components/storage/DriveEditor.tsx @@ -84,7 +84,7 @@ const SpacePolicySelector = ({ drive, driveDevice }: DriveEditorProps) => { const { setSpacePolicy } = useDrive(drive.name); const onSpacePolicyChange = (spacePolicy: apiModel.SpacePolicy) => { if (spacePolicy === "custom") { - return navigate(generatePath(PATHS.findSpace, { id: baseName(drive.name) })); + return navigate(generatePath(PATHS.drive.editSpacePolicy, { id: baseName(drive.name) })); } else { setSpacePolicy(spacePolicy); } @@ -470,7 +470,9 @@ const PartitionsNoContentSelector = ({ drive, toggleAriaLabel }) => { itemId="add-partition" description={_("Add another partition or mount an existing one")} role="menuitem" - onClick={() => navigate(generatePath(PATHS.addPartition, { id: baseName(drive.name) }))} + onClick={() => + navigate(generatePath(PATHS.drive.partition.add, { id: baseName(drive.name) })) + } > {_("Add or use partition")} @@ -484,7 +486,7 @@ const PartitionsNoContentSelector = ({ drive, toggleAriaLabel }) => { const PartitionMenuItem = ({ driveName, mountPath }) => { const drive = useDrive(driveName); const partition = drive.getPartition(mountPath); - const editPath = generatePath(PATHS.editPartition, { + const editPath = generatePath(PATHS.drive.partition.edit, { id: baseName(driveName), partitionId: encodeURIComponent(mountPath), }); @@ -519,7 +521,9 @@ const PartitionsWithContentSelector = ({ drive, toggleAriaLabel }) => { key="add-partition" itemId="add-partition" description={_("Add another partition or mount an existing one")} - onClick={() => navigate(generatePath(PATHS.addPartition, { id: baseName(drive.name) }))} + onClick={() => + navigate(generatePath(PATHS.drive.partition.add, { id: baseName(drive.name) })) + } > {_("Add or use partition")} diff --git a/web/src/components/storage/EncryptionSection.test.tsx b/web/src/components/storage/EncryptionSection.test.tsx index a56a6c6c03..f2f70243db 100644 --- a/web/src/components/storage/EncryptionSection.test.tsx +++ b/web/src/components/storage/EncryptionSection.test.tsx @@ -79,6 +79,6 @@ describe("EncryptionSection", () => { it("renders a link for navigating to encryption settings", () => { plainRender(); const editLink = screen.getByRole("link", { name: "Edit" }); - expect(editLink).toHaveAttribute("href", STORAGE.encryption); + expect(editLink).toHaveAttribute("href", STORAGE.editEncryption); }); }); diff --git a/web/src/components/storage/EncryptionSection.tsx b/web/src/components/storage/EncryptionSection.tsx index 22974b4bc1..7f4c9dddd7 100644 --- a/web/src/components/storage/EncryptionSection.tsx +++ b/web/src/components/storage/EncryptionSection.tsx @@ -47,7 +47,7 @@ export default function EncryptionSection() { the new file systems, including data, programs, and system files.", )} pfCardBodyProps={{ isFilled: true }} - actions={{_("Edit")}} + actions={{_("Edit")}} > diff --git a/web/src/routes/paths.ts b/web/src/routes/paths.ts index 249b6c1b33..b5f86e7a18 100644 --- a/web/src/routes/paths.ts +++ b/web/src/routes/paths.ts @@ -71,15 +71,19 @@ const SOFTWARE = { const STORAGE = { root: "/storage", - bootDevice: "/storage/select-boot-device", - encryption: "/storage/encryption", - addPartition: "/storage/devices/:id/partitions/new", - editPartition: "/storage/devices/:id/partitions/:partitionId/edit", - findSpace: "/storage/devices/:id/space/edit", - iscsi: "/storage/iscsi", - lvm: { - create: "/storage/lvm/new", + editBootDevice: "/storage/boot-device/edit", + editEncryption: "/storage/encryption/edit", + drive: { + editSpacePolicy: "/storage/drives/:id/space-policy/edit", + partition: { + add: "/storage/drives/:id/partitions/add", + edit: "/storage/drives/:id/partitions/:partitionId/edit", + }, + }, + volumeGroup: { + add: "/storage/volume-groups/add", }, + iscsi: "/storage/iscsi", dasd: "/storage/dasd", zfcp: { root: "/storage/zfcp", diff --git a/web/src/routes/storage.tsx b/web/src/routes/storage.tsx index 06e6de1e7d..61bf5b5226 100644 --- a/web/src/routes/storage.tsx +++ b/web/src/routes/storage.tsx @@ -47,29 +47,29 @@ const routes = (): Route => ({ element: , }, { - path: PATHS.bootDevice, + path: PATHS.editBootDevice, element: , }, { - path: PATHS.lvm.create, - element: , - }, - { - path: PATHS.encryption, + path: PATHS.editEncryption, element: , }, { - path: PATHS.findSpace, + path: PATHS.drive.editSpacePolicy, element: , }, { - path: PATHS.addPartition, + path: PATHS.drive.partition.add, element: , }, { - path: PATHS.editPartition, + path: PATHS.drive.partition.edit, element: , }, + { + path: PATHS.volumeGroup.add, + element: , + }, { path: PATHS.iscsi, element: , From 1fb8f6df9bb4605358770d9a6f4929f23d95da53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Thu, 20 Mar 2025 11:42:31 +0000 Subject: [PATCH 04/11] web: allow editing a volume group --- web/src/components/storage/LvmPage.tsx | 89 ++++++++++++------- .../components/storage/VolumeGroupEditor.tsx | 18 +++- web/src/routes/paths.ts | 1 + web/src/routes/storage.tsx | 4 + 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/web/src/components/storage/LvmPage.tsx b/web/src/components/storage/LvmPage.tsx index 18bb9c2ff5..b04f48700d 100644 --- a/web/src/components/storage/LvmPage.tsx +++ b/web/src/components/storage/LvmPage.tsx @@ -21,7 +21,7 @@ */ import React, { useState, useEffect } from "react"; -import { useNavigate } from "react-router-dom"; +import { useParams, useNavigate } from "react-router-dom"; import { ActionGroup, Alert, @@ -37,47 +37,59 @@ import { import { Page, SubtleContent } from "~/components/core"; import { useAvailableDevices } from "~/queries/storage"; import { StorageDevice, model } from "~/types/storage"; -import useModel from "~/hooks/storage/model"; +import useModel, { useVolumeGroup } from "~/hooks/storage/model"; import useAddVolumeGroup from "~/hooks/storage/add-volume-group"; +import useEditVolumeGroup from "~/hooks/storage/edit-volume-group"; import { deviceLabel } from "./utils"; import { contentDescription, filesystemLabels, typeDescription } from "./utils/device"; import { STORAGE as PATHS } from "~/routes/paths"; import { sprintf } from "sprintf-js"; import { _ } from "~/i18n"; -function checkErrors(model: model.Model, vgName: string, targetDevices: StorageDevice[]): string[] { - const vgNameError = (): string | undefined => { - if (!vgName.length) return sprintf(_("Name is empty"), vgName); +function vgNameError( + vgName: string, + model: model.Model, + volumeGroup?: model.VolumeGroup, +): string | undefined { + if (!vgName.length) return sprintf(_("Name is empty"), vgName); - const exist = model.volumeGroups.some((v) => v.vgName === vgName); - if (exist) return sprintf(_("'%s' already exists"), vgName); - }; - - const targetDevicesError = (): string | undefined => { - if (!targetDevices.length) return _("No disk is selected"); - }; + const exist = model.volumeGroups.some((v) => v.vgName === vgName); + if (exist && vgName !== volumeGroup?.vgName) return sprintf(_("'%s' already exists"), vgName); +} - return [vgNameError(), targetDevicesError()].filter((d) => d); +function targetDevicesError(targetDevices: StorageDevice[]): string | undefined { + if (!targetDevices.length) return _("No disk is selected"); } /** - * Form for creating a LVM volume group + * Form for configuring a LVM volume group. */ export default function LvmPage() { const navigate = useNavigate(); const model = useModel(); const addVolumeGroup = useAddVolumeGroup(); + const editVolumeGroup = useEditVolumeGroup(); const allDevices = useAvailableDevices(); const [name, setName] = useState(""); const [selectedDevices, setSelectedDevices] = useState([]); const [moveMountPoints, setMoveMountPoints] = useState(true); const [errors, setErrors] = useState([]); + const { id } = useParams(); + const volumeGroup = useVolumeGroup(id); useEffect(() => { - if (model && !model.volumeGroups.length) setName("system"); - }, [model]); + if (volumeGroup) { + setName(volumeGroup.vgName); + const targetNames = volumeGroup.getTargetDevices().map((d) => d.name); + const targetDevices = allDevices.filter((d) => targetNames.includes(d.name)); + setSelectedDevices(targetDevices); + } else if (model && !model.volumeGroups.length) { + setName("system"); + } + }, [model, volumeGroup, allDevices]); const updateName = (_, value) => setName(value); + const updateSelectedDevices = (value) => { setSelectedDevices( selectedDevices.includes(value) @@ -86,19 +98,28 @@ export default function LvmPage() { ); }; + const checkErrors = (): string[] => { + return [vgNameError(name, model, volumeGroup), targetDevicesError(selectedDevices)].filter( + (e) => e, + ); + }; + const onSubmit = (e) => { e.preventDefault(); - const errors = checkErrors(model, name, selectedDevices); + const errors = checkErrors(); setErrors(errors); if (errors.length) return; - addVolumeGroup( - name, - selectedDevices.map((d) => d.name), - moveMountPoints, - ); + const selectedDeviceNames = selectedDevices.map((d) => d.name); + + if (!volumeGroup) { + addVolumeGroup(name, selectedDeviceNames, moveMountPoints); + } else { + editVolumeGroup(volumeGroup.vgName, name, selectedDeviceNames); + } + navigate(PATHS.root); }; @@ -157,17 +178,19 @@ export default function LvmPage() { ))} - - setMoveMountPoints(!moveMountPoints)} - /> - + {!volumeGroup && ( + + setMoveMountPoints(v)} + /> + + )} diff --git a/web/src/components/storage/VolumeGroupEditor.tsx b/web/src/components/storage/VolumeGroupEditor.tsx index 5b1402c26e..03a52689ad 100644 --- a/web/src/components/storage/VolumeGroupEditor.tsx +++ b/web/src/components/storage/VolumeGroupEditor.tsx @@ -21,8 +21,10 @@ */ import React from "react"; +import { useNavigate, generatePath } from "react-router-dom"; import { _ } from "~/i18n"; import { sprintf } from "sprintf-js"; +import { STORAGE as PATHS } from "~/routes/paths"; import { apiModel } from "~/api/storage/types"; import { model } from "~/types/storage"; import { contentDescription } from "~/components/storage/utils/volume-group"; @@ -53,10 +55,18 @@ const RemoveVgOption = ({ vg }: { vg: model.VolumeGroup }) => { ); }; -const EditVgOption = () => { +const EditVgOption = ({ vg }: { vg: model.VolumeGroup }) => { + const navigate = useNavigate(); + return ( - - {_("Edit volume group")} + navigate(generatePath(PATHS.volumeGroup.edit, { id: vg.vgName }))} + > + {_("Edit volume group")} ); }; @@ -65,7 +75,7 @@ const VgMenu = ({ vg }: { vg: model.VolumeGroup }) => { return ( {vg.vgName}}> - + diff --git a/web/src/routes/paths.ts b/web/src/routes/paths.ts index b5f86e7a18..298ed442ea 100644 --- a/web/src/routes/paths.ts +++ b/web/src/routes/paths.ts @@ -82,6 +82,7 @@ const STORAGE = { }, volumeGroup: { add: "/storage/volume-groups/add", + edit: "/storage/volume-groups/:id/edit", }, iscsi: "/storage/iscsi", dasd: "/storage/dasd", diff --git a/web/src/routes/storage.tsx b/web/src/routes/storage.tsx index 61bf5b5226..cbaf786eb8 100644 --- a/web/src/routes/storage.tsx +++ b/web/src/routes/storage.tsx @@ -70,6 +70,10 @@ const routes = (): Route => ({ path: PATHS.volumeGroup.add, element: , }, + { + path: PATHS.volumeGroup.edit, + element: , + }, { path: PATHS.iscsi, element: , From a47f1a66759c031336d8661fedeacf86d1a5a831 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 21 Mar 2025 11:28:34 +0000 Subject: [PATCH 05/11] web: improvements in the storage hooks - Helpers do not modify the given apiModel object. --- web/src/hooks/storage/api-model.ts | 13 ++------- web/src/hooks/storage/edit-volume-group.ts | 21 +-------------- .../hooks/storage/helpers/copy-api-model.ts | 27 +++++++++++++++++++ web/src/hooks/storage/helpers/drive.ts | 9 ++++--- web/src/hooks/storage/helpers/volume-group.ts | 27 ++++++++++++++++++- 5 files changed, 62 insertions(+), 35 deletions(-) create mode 100644 web/src/hooks/storage/helpers/copy-api-model.ts diff --git a/web/src/hooks/storage/api-model.ts b/web/src/hooks/storage/api-model.ts index 5f387be88b..afc4260dc6 100644 --- a/web/src/hooks/storage/api-model.ts +++ b/web/src/hooks/storage/api-model.ts @@ -20,23 +20,14 @@ * find current contact information at www.suse.com. */ -import { useMemo } from "react"; import { useQuery, useSuspenseQuery } from "@tanstack/react-query"; import { configModelQuery } from "~/queries/storage/config-model"; import { apiModel } from "~/api/storage/types"; import { QueryHookOptions } from "~/types/queries"; -function useApiModel(options?: QueryHookOptions): apiModel.Config | null { +export default function useApiModel(options?: QueryHookOptions): apiModel.Config | null { const query = configModelQuery; const func = options?.suspense ? useSuspenseQuery : useQuery; const { data } = func(query); - - const apiModel = useMemo((): apiModel.Config | null => { - // Returns a copy. - return data ? JSON.parse(JSON.stringify(data)) : null; - }, [data]); - - return apiModel; + return data || null; } - -export { useApiModel as default }; diff --git a/web/src/hooks/storage/edit-volume-group.ts b/web/src/hooks/storage/edit-volume-group.ts index d0fa723e43..bd8402b325 100644 --- a/web/src/hooks/storage/edit-volume-group.ts +++ b/web/src/hooks/storage/edit-volume-group.ts @@ -22,27 +22,8 @@ import useApiModel from "~/hooks/storage/api-model"; import useUpdateApiModel from "~/hooks/storage/update-api-model"; -import { addVolumeGroup } from "~/hooks/storage/helpers/volume-group"; -import { deleteIfUnused } from "~/hooks/storage/helpers/drive"; +import { editVolumeGroup } from "~/hooks/storage/helpers/volume-group"; import { QueryHookOptions } from "~/types/queries"; -import { apiModel } from "~/api/storage/types"; - -function editVolumeGroup( - apiModel: apiModel.Config, - oldVgName: string, - vgName: string, - targetDevices: string[], -): apiModel.Config { - const index = (apiModel.volumeGroups || []).findIndex((v) => v.vgName === oldVgName); - if (index === -1) return; - - const oldTargetDevices = apiModel.volumeGroups[index].targetDevices || []; - - addVolumeGroup(apiModel, vgName, targetDevices, false, index); - oldTargetDevices.forEach((d) => deleteIfUnused(apiModel, d)); - - return apiModel; -} export type EditVolumeGroupFn = ( odlVgName: string, diff --git a/web/src/hooks/storage/helpers/copy-api-model.ts b/web/src/hooks/storage/helpers/copy-api-model.ts new file mode 100644 index 0000000000..049a48de2f --- /dev/null +++ b/web/src/hooks/storage/helpers/copy-api-model.ts @@ -0,0 +1,27 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 { apiModel } from "~/api/storage/types"; + +export default function copyApiModel(apiModel: apiModel.Config): apiModel.Config { + return JSON.parse(JSON.stringify(apiModel)); +} diff --git a/web/src/hooks/storage/helpers/drive.ts b/web/src/hooks/storage/helpers/drive.ts index 13e17636a4..ac6e0bda88 100644 --- a/web/src/hooks/storage/helpers/drive.ts +++ b/web/src/hooks/storage/helpers/drive.ts @@ -22,6 +22,7 @@ import { apiModel } from "~/api/storage/types"; import { model } from "~/types/storage"; +import copyApiModel from "~/hooks/storage/helpers/copy-api-model"; import buildModel from "~/hooks/storage/helpers/build-model"; function buildDrive(apiModel: apiModel.Config, name: string): model.Drive | undefined { @@ -29,12 +30,14 @@ function buildDrive(apiModel: apiModel.Config, name: string): model.Drive | unde return model.drives.find((d) => d.name === name); } -function deleteIfUnused(apiModel: apiModel.Config, name: string) { +function deleteIfUnused(apiModel: apiModel.Config, name: string): apiModel.Config { + apiModel = copyApiModel(apiModel); + const index = (apiModel.drives || []).findIndex((d) => d.name === name); - if (index === -1) return; + if (index === -1) return apiModel; const drive = buildDrive(apiModel, name); - if (!drive || drive.isUsed) return; + if (!drive || drive.isUsed) return apiModel; apiModel.drives.splice(index, 1); return apiModel; diff --git a/web/src/hooks/storage/helpers/volume-group.ts b/web/src/hooks/storage/helpers/volume-group.ts index 6be4365d62..95f7436129 100644 --- a/web/src/hooks/storage/helpers/volume-group.ts +++ b/web/src/hooks/storage/helpers/volume-group.ts @@ -21,6 +21,8 @@ */ import { apiModel } from "~/api/storage/types"; +import copyApiModel from "~/hooks/storage/helpers/copy-api-model"; +import { deleteIfUnused } from "~/hooks/storage/helpers/drive"; function toLogicalVolume(partition: apiModel.Partition) { return { ...partition }; @@ -43,6 +45,8 @@ function addVolumeGroup( moveContent: boolean, index?: number, ): apiModel.Config { + apiModel = copyApiModel(apiModel); + const volumeGroup = { vgName, targetDevices }; if (moveContent) { @@ -62,4 +66,25 @@ function addVolumeGroup( return apiModel; } -export { addVolumeGroup }; +function editVolumeGroup( + apiModel: apiModel.Config, + oldVgName: string, + vgName: string, + targetDevices: string[], +): apiModel.Config { + apiModel = copyApiModel(apiModel); + + const index = (apiModel.volumeGroups || []).findIndex((v) => v.vgName === oldVgName); + if (index === -1) return apiModel; + + const oldTargetDevices = apiModel.volumeGroups[index].targetDevices || []; + + apiModel = addVolumeGroup(apiModel, vgName, targetDevices, false, index); + oldTargetDevices.forEach((d) => { + apiModel = deleteIfUnused(apiModel, d); + }); + + return apiModel; +} + +export { addVolumeGroup, editVolumeGroup }; From 2c3eef8561b0ba6f11093ccb3c737a3398d39f1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 21 Mar 2025 12:41:56 +0000 Subject: [PATCH 06/11] web: fix helper for editing a volume group - The logical volumes are kept. --- web/src/hooks/storage/helpers/volume-group.ts | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/web/src/hooks/storage/helpers/volume-group.ts b/web/src/hooks/storage/helpers/volume-group.ts index 95f7436129..d14713d458 100644 --- a/web/src/hooks/storage/helpers/volume-group.ts +++ b/web/src/hooks/storage/helpers/volume-group.ts @@ -43,7 +43,6 @@ function addVolumeGroup( vgName: string, targetDevices: string[], moveContent: boolean, - index?: number, ): apiModel.Config { apiModel = copyApiModel(apiModel); @@ -56,12 +55,7 @@ function addVolumeGroup( } apiModel.volumeGroups ||= []; - - if (index === undefined || index >= apiModel.volumeGroups.length) { - apiModel.volumeGroups.push(volumeGroup); - } else { - apiModel.volumeGroups.splice(index, 1, volumeGroup); - } + apiModel.volumeGroups.push(volumeGroup); return apiModel; } @@ -77,10 +71,11 @@ function editVolumeGroup( const index = (apiModel.volumeGroups || []).findIndex((v) => v.vgName === oldVgName); if (index === -1) return apiModel; - const oldTargetDevices = apiModel.volumeGroups[index].targetDevices || []; + const oldVolumeGroup = apiModel.volumeGroups[index]; + const newVolumeGroup = { ...oldVolumeGroup, vgName, targetDevices }; - apiModel = addVolumeGroup(apiModel, vgName, targetDevices, false, index); - oldTargetDevices.forEach((d) => { + apiModel.volumeGroups.splice(index, 1, newVolumeGroup); + (oldVolumeGroup.targetDevices || []).forEach((d) => { apiModel = deleteIfUnused(apiModel, d); }); From 01364168349841186de4fa0066546e44349d4307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Fri, 21 Mar 2025 17:40:07 +0000 Subject: [PATCH 07/11] web: add basic unit test for LVM form The test is limited and could be improved, especially in terms of mocking. It also highlights potential issues with form initialization and the use of useEffect. However, it ensures the form works as expected at a basic level. --- web/src/components/storage/LvmPage.test.tsx | 300 ++++++++++++++++++++ web/src/components/storage/LvmPage.tsx | 4 +- 2 files changed, 303 insertions(+), 1 deletion(-) create mode 100644 web/src/components/storage/LvmPage.test.tsx diff --git a/web/src/components/storage/LvmPage.test.tsx b/web/src/components/storage/LvmPage.test.tsx new file mode 100644 index 0000000000..af13d8b4a7 --- /dev/null +++ b/web/src/components/storage/LvmPage.test.tsx @@ -0,0 +1,300 @@ +/* + * Copyright (c) [2025] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * + * 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 { installerRender, mockParams } from "~/test-utils"; +import { model, StorageDevice } from "~/types/storage"; +import { apiModel } from "~/api/storage/types"; +import { gib } from "./utils"; +import { Drive } from "~/types/storage/model"; +import LvmPage from "./LvmPage"; + +const sda1: StorageDevice = { + sid: 69, + name: "/dev/sda1", + description: "Swap partition", + isDrive: false, + type: "partition", + size: gib(2), + shrinking: { unsupported: ["Resizing is not supported"] }, + start: 1, +}; + +const sda: StorageDevice = { + sid: 59, + isDrive: true, + type: "disk", + vendor: "Micron", + model: "Micron 1100 SATA", + driver: ["ahci", "mmcblk"], + bus: "IDE", + busId: "", + transport: "usb", + dellBOSS: false, + sdCard: true, + active: true, + name: "/dev/sda", + size: 1024, + shrinking: { unsupported: ["Resizing is not supported"] }, + systems: [], + partitionTable: { + type: "gpt", + partitions: [sda1], + unpartitionedSize: 0, + unusedSlots: [{ start: 3, size: gib(2) }], + }, + udevIds: ["ata-Micron_1100_SATA_512GB_12563", "scsi-0ATA_Micron_1100_SATA_512GB"], + udevPaths: ["pci-0000:00-12", "pci-0000:00-12-ata"], + description: "", +}; + +const mockSdaDrive: Drive = { + name: "/dev/sda", + spacePolicy: "delete", + partitions: [ + { + mountPath: "swap", + size: { + min: gib(2), + default: false, // false: user provided, true: calculated + }, + filesystem: { default: false, type: "swap" }, + }, + { + mountPath: "/home", + size: { + min: gib(16), + default: true, + }, + filesystem: { default: false, type: "xfs" }, + }, + ], + isUsed: true, + getVolumeGroups: () => [], +}; + +const mockRootVolumeGroup: model.VolumeGroup = { + vgName: "fakeRootVg", + getTargetDevices: () => [mockSdaDrive], + logicalVolumes: [], +}; + +const mockAddVolumeGroup = jest.fn(); +const mockEditVolumeGroup = jest.fn(); +let mockVolumeGroups: apiModel.VolumeGroup[] = []; + +let mockUseModel = { + drives: [mockSdaDrive], + volumeGroups: mockVolumeGroups, +}; + +const mockUseAllDevices = [sda]; + +jest.mock("~/queries/issues", () => ({ + ...jest.requireActual("~/queries/issues"), + useIssuesChanges: jest.fn(), + useIssues: () => [], +})); + +jest.mock("~/queries/storage", () => ({ + ...jest.requireActual("~/queries/storage"), + useAvailableDevices: () => mockUseAllDevices, + useDevices: () => [sda], +})); + +jest.mock("~/hooks/storage/model", () => ({ + ...jest.requireActual("~/hooks/storage/model"), + __esModule: true, + useVolumeGroup: (id: string) => (id ? mockRootVolumeGroup : null), + default: () => mockUseModel, +})); + +jest.mock("~/hooks/storage/add-volume-group", () => ({ + ...jest.requireActual("~/hooks/storage/add-volume-group"), + __esModule: true, + default: () => mockAddVolumeGroup, +})); + +jest.mock("~/hooks/storage/edit-volume-group", () => ({ + ...jest.requireActual("~/hooks/storage/edit-volume-group"), + __esModule: true, + default: () => mockEditVolumeGroup, +})); + +describe("LvmPage", () => { + describe("when creating a new volume group", () => { + beforeEach(() => { + mockVolumeGroups = []; + }); + + it("allows configuring a new LVM volume group (without moving mount points)", async () => { + const { user } = installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + const disks = screen.getByRole("group", { name: "Disks" }); + const sdaCheckbox = within(disks).getByRole("checkbox", { name: "/dev/sda, 1 KiB" }); + const moveMountPointsCheckbox = screen.getByRole("checkbox", { + name: /Move the mount points currently configured at the selected disks to logical volumes/, + }); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + + // Clear default value for name + await user.clear(name); + await user.type(name, "root-vg"); + await user.click(sdaCheckbox); + // By default move move mount points should be checked + expect(moveMountPointsCheckbox).toBeChecked(); + await user.click(moveMountPointsCheckbox); + expect(moveMountPointsCheckbox).not.toBeChecked(); + await user.click(acceptButton); + expect(mockAddVolumeGroup).toHaveBeenCalledWith("root-vg", ["/dev/sda"], false); + }); + + it("allows configuring a new LVM volume group (moving mount points)", async () => { + const { user } = installerRender(); + const disks = screen.getByRole("group", { name: "Disks" }); + const sdaCheckbox = within(disks).getByRole("checkbox", { name: "/dev/sda, 1 KiB" }); + const moveMountPointsCheckbox = screen.getByRole("checkbox", { + name: /Move the mount points currently configured at the selected disks to logical volumes/, + }); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + + await user.click(sdaCheckbox); + expect(moveMountPointsCheckbox).toBeChecked(); + await user.click(acceptButton); + expect(mockAddVolumeGroup).toHaveBeenCalledWith("system", ["/dev/sda"], true); + }); + + it("performs basic validations", async () => { + const { user } = installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + const disks = screen.getByRole("group", { name: "Disks" }); + const sdaCheckbox = within(disks).getByRole("checkbox", { name: "/dev/sda, 1 KiB" }); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + + // Let's clean the default given name + await user.clear(name); + await user.click(acceptButton); + screen.getByText("Warning alert:"); + screen.getByText("Name is empty"); + screen.getByText("No disk is selected"); + + // Type a name + await user.type(name, "root-vg"); + await user.click(acceptButton); + expect(screen.queryByText("Name is empty")).toBeNull(); + screen.getByText("Warning alert:"); + screen.getByText("No disk is selected"); + + // Select a disk + expect(sdaCheckbox).not.toBeChecked(); + await user.click(sdaCheckbox); + expect(sdaCheckbox).toBeChecked(); + await user.click(acceptButton); + expect(screen.queryByText("Name is empty")).toBeNull(); + expect(screen.queryByText("Warning alert:")).toBeNull(); + expect(screen.queryByText("No disk is selected")).toBeNull(); + }); + + describe("when there are LVM volume groups", () => { + beforeEach(() => { + mockUseModel = { + drives: [mockSdaDrive], + volumeGroups: [mockRootVolumeGroup], + }; + }); + + it("does not pre-fill the name input", () => { + installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + expect(name).toHaveValue(""); + }); + }); + + describe("when there are no LVM volume groups yet", () => { + beforeEach(() => { + mockUseModel = { + drives: [mockSdaDrive], + volumeGroups: [], + }; + }); + + it("pre-fills the name input with 'system'", () => { + installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + expect(name).toHaveValue("system"); + }); + }); + }); + + describe("when editing", () => { + beforeEach(() => { + mockParams({ id: "fakeRootVg" }); + mockVolumeGroups = [mockRootVolumeGroup]; + }); + + it("performs basic validations", async () => { + const { user } = installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + const disks = screen.getByRole("group", { name: "Disks" }); + const sdaCheckbox = within(disks).getByRole("checkbox", { name: "/dev/sda, 1 KiB" }); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + + // Let's clean the default given name + await user.clear(name); + await user.click(sdaCheckbox); + expect(name).toHaveValue(""); + expect(sdaCheckbox).not.toBeChecked(); + await user.click(acceptButton); + screen.getByText("Warning alert:"); + screen.getByText("Name is empty"); + screen.getByText("No disk is selected"); + }); + + it("pre-fills form with the current volume group configuration", async () => { + installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + const sdaCheckbox = screen.getByRole("checkbox", { name: "/dev/sda, 1 KiB" }); + expect(name).toHaveValue("fakeRootVg"); + expect(sdaCheckbox).toBeChecked(); + }); + + it("does not offer option for moving mount points", () => { + installerRender(); + expect( + screen.queryByRole("checkbox", { + name: /Move the mount points currently configured at the selected disks to logical volumes/, + }), + ).toBeNull(); + }); + + it("triggers the hook for updating the volume group when user accepts changes", async () => { + const { user } = installerRender(); + const name = screen.getByRole("textbox", { name: "Name" }); + const acceptButton = screen.getByRole("button", { name: "Accept" }); + await user.clear(name); + await user.type(name, "updatedRootVg"); + await user.click(acceptButton); + expect(mockEditVolumeGroup).toHaveBeenCalledWith("fakeRootVg", "updatedRootVg", ["/dev/sda"]); + }); + }); +}); diff --git a/web/src/components/storage/LvmPage.tsx b/web/src/components/storage/LvmPage.tsx index b04f48700d..1b8e85beed 100644 --- a/web/src/components/storage/LvmPage.tsx +++ b/web/src/components/storage/LvmPage.tsx @@ -51,7 +51,7 @@ function vgNameError( model: model.Model, volumeGroup?: model.VolumeGroup, ): string | undefined { - if (!vgName.length) return sprintf(_("Name is empty"), vgName); + if (!vgName.length) return _("Name is empty"); const exist = model.volumeGroups.some((v) => v.vgName === vgName); if (exist && vgName !== volumeGroup?.vgName) return sprintf(_("'%s' already exists"), vgName); @@ -65,8 +65,10 @@ function targetDevicesError(targetDevices: StorageDevice[]): string | undefined * Form for configuring a LVM volume group. */ export default function LvmPage() { + const { id } = useParams(); const navigate = useNavigate(); const model = useModel(); + const volumeGroup = useVolumeGroup(id); const addVolumeGroup = useAddVolumeGroup(); const editVolumeGroup = useEditVolumeGroup(); const allDevices = useAvailableDevices(); From 13cc74786a7366396a313e1efe13de757c40997e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Fri, 21 Mar 2025 18:06:21 +0000 Subject: [PATCH 08/11] fix(web): drop repeated lines Introduced by mistake by undoing changes. --- web/src/components/storage/LvmPage.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/web/src/components/storage/LvmPage.tsx b/web/src/components/storage/LvmPage.tsx index 1b8e85beed..30f8821408 100644 --- a/web/src/components/storage/LvmPage.tsx +++ b/web/src/components/storage/LvmPage.tsx @@ -76,8 +76,6 @@ export default function LvmPage() { const [selectedDevices, setSelectedDevices] = useState([]); const [moveMountPoints, setMoveMountPoints] = useState(true); const [errors, setErrors] = useState([]); - const { id } = useParams(); - const volumeGroup = useVolumeGroup(id); useEffect(() => { if (volumeGroup) { From 77a435fbfd66675512ef677fb627a36584a8e39a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Sat, 22 Mar 2025 19:21:00 +0000 Subject: [PATCH 09/11] fix(web): improve error messages Refactor error messages to be clearer, more helpful, and concise. While this improves the messaging, it remains inconsistent with the rest of the interface until the tone, style, and error handling can be unified. --- web/src/components/storage/LvmPage.test.tsx | 40 ++++++++++++--------- web/src/components/storage/LvmPage.tsx | 9 ++--- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/web/src/components/storage/LvmPage.test.tsx b/web/src/components/storage/LvmPage.test.tsx index af13d8b4a7..d3014b167f 100644 --- a/web/src/components/storage/LvmPage.test.tsx +++ b/web/src/components/storage/LvmPage.test.tsx @@ -24,7 +24,6 @@ import React from "react"; import { screen, within } from "@testing-library/react"; import { installerRender, mockParams } from "~/test-utils"; import { model, StorageDevice } from "~/types/storage"; -import { apiModel } from "~/api/storage/types"; import { gib } from "./utils"; import { Drive } from "~/types/storage/model"; import LvmPage from "./LvmPage"; @@ -99,13 +98,18 @@ const mockRootVolumeGroup: model.VolumeGroup = { logicalVolumes: [], }; +const mockHomeVolumeGroup: model.VolumeGroup = { + vgName: "fakeHomeVg", + getTargetDevices: () => [mockSdaDrive], + logicalVolumes: [], +}; + const mockAddVolumeGroup = jest.fn(); const mockEditVolumeGroup = jest.fn(); -let mockVolumeGroups: apiModel.VolumeGroup[] = []; let mockUseModel = { drives: [mockSdaDrive], - volumeGroups: mockVolumeGroups, + volumeGroups: [], }; const mockUseAllDevices = [sda]; @@ -143,10 +147,6 @@ jest.mock("~/hooks/storage/edit-volume-group", () => ({ describe("LvmPage", () => { describe("when creating a new volume group", () => { - beforeEach(() => { - mockVolumeGroups = []; - }); - it("allows configuring a new LVM volume group (without moving mount points)", async () => { const { user } = installerRender(); const name = screen.getByRole("textbox", { name: "Name" }); @@ -195,24 +195,24 @@ describe("LvmPage", () => { await user.clear(name); await user.click(acceptButton); screen.getByText("Warning alert:"); - screen.getByText("Name is empty"); - screen.getByText("No disk is selected"); + screen.getByText(/Enter a name/); + screen.getByText(/Select at least one disk/); // Type a name await user.type(name, "root-vg"); await user.click(acceptButton); - expect(screen.queryByText("Name is empty")).toBeNull(); screen.getByText("Warning alert:"); - screen.getByText("No disk is selected"); + expect(screen.queryByText(/Enter a name/)).toBeNull(); + screen.getByText(/Select at least one disk/); // Select a disk expect(sdaCheckbox).not.toBeChecked(); await user.click(sdaCheckbox); expect(sdaCheckbox).toBeChecked(); await user.click(acceptButton); - expect(screen.queryByText("Name is empty")).toBeNull(); expect(screen.queryByText("Warning alert:")).toBeNull(); - expect(screen.queryByText("No disk is selected")).toBeNull(); + expect(screen.queryByText(/Enter a name/)).toBeNull(); + expect(screen.queryByText(/Select at least one disk/)).toBeNull(); }); describe("when there are LVM volume groups", () => { @@ -249,7 +249,10 @@ describe("LvmPage", () => { describe("when editing", () => { beforeEach(() => { mockParams({ id: "fakeRootVg" }); - mockVolumeGroups = [mockRootVolumeGroup]; + mockUseModel = { + drives: [mockSdaDrive], + volumeGroups: [mockRootVolumeGroup, mockHomeVolumeGroup], + }; }); it("performs basic validations", async () => { @@ -266,8 +269,13 @@ describe("LvmPage", () => { expect(sdaCheckbox).not.toBeChecked(); await user.click(acceptButton); screen.getByText("Warning alert:"); - screen.getByText("Name is empty"); - screen.getByText("No disk is selected"); + screen.getByText(/Enter a name/); + screen.getByText(/Select at least one disk/); + // Enter a name already in use + await user.type(name, "fakeHomeVg"); + await user.click(acceptButton); + expect(screen.queryByText(/Enter a name/)).toBeNull(); + screen.getByText(/Enter a different name/); }); it("pre-fills form with the current volume group configuration", async () => { diff --git a/web/src/components/storage/LvmPage.tsx b/web/src/components/storage/LvmPage.tsx index 30f8821408..dd1420f263 100644 --- a/web/src/components/storage/LvmPage.tsx +++ b/web/src/components/storage/LvmPage.tsx @@ -51,14 +51,15 @@ function vgNameError( model: model.Model, volumeGroup?: model.VolumeGroup, ): string | undefined { - if (!vgName.length) return _("Name is empty"); + if (!vgName.length) return _("Enter a name for the volume group."); const exist = model.volumeGroups.some((v) => v.vgName === vgName); - if (exist && vgName !== volumeGroup?.vgName) return sprintf(_("'%s' already exists"), vgName); + if (exist && vgName !== volumeGroup?.vgName) + return sprintf(_("Volume group '%s' already exists. Enter a different name."), vgName); } function targetDevicesError(targetDevices: StorageDevice[]): string | undefined { - if (!targetDevices.length) return _("No disk is selected"); + if (!targetDevices.length) return _("Select at least one disk."); } /** @@ -132,7 +133,7 @@ export default function LvmPage() {
{errors.length > 0 && ( - + {errors.map((e, i) => (

{e}

))} From ff0b31c293ed8806978f70859809b4f47b69812d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 24 Mar 2025 07:52:32 +0000 Subject: [PATCH 10/11] fix(web): suggestion from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use `model.Drive` instead of directly importing the `Drive` type. This aims to make clear which Drive type is being used when reading, writing, or maintaining the code. However, StorageDevice is not under the `model` namespace (yet?) because it doesn’t currently conflict with a type from `apiModel`. It would be nice to find a better organization for these types, but there is neither time nor a clear vision for how to structure this complex area at the moment. --- web/src/components/storage/LvmPage.test.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/web/src/components/storage/LvmPage.test.tsx b/web/src/components/storage/LvmPage.test.tsx index d3014b167f..e1e65aaf29 100644 --- a/web/src/components/storage/LvmPage.test.tsx +++ b/web/src/components/storage/LvmPage.test.tsx @@ -25,7 +25,6 @@ import { screen, within } from "@testing-library/react"; import { installerRender, mockParams } from "~/test-utils"; import { model, StorageDevice } from "~/types/storage"; import { gib } from "./utils"; -import { Drive } from "~/types/storage/model"; import LvmPage from "./LvmPage"; const sda1: StorageDevice = { @@ -67,7 +66,7 @@ const sda: StorageDevice = { description: "", }; -const mockSdaDrive: Drive = { +const mockSdaDrive: model.Drive = { name: "/dev/sda", spacePolicy: "delete", partitions: [ From 235e38f13e617e631f3a342cd8da9cde4b464c4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Mon, 24 Mar 2025 08:01:21 +0000 Subject: [PATCH 11/11] doc(web): drop repeated word --- web/src/components/storage/LvmPage.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/storage/LvmPage.test.tsx b/web/src/components/storage/LvmPage.test.tsx index e1e65aaf29..4c437be9a1 100644 --- a/web/src/components/storage/LvmPage.test.tsx +++ b/web/src/components/storage/LvmPage.test.tsx @@ -160,7 +160,7 @@ describe("LvmPage", () => { await user.clear(name); await user.type(name, "root-vg"); await user.click(sdaCheckbox); - // By default move move mount points should be checked + // By default move mount points should be checked expect(moveMountPointsCheckbox).toBeChecked(); await user.click(moveMountPointsCheckbox); expect(moveMountPointsCheckbox).not.toBeChecked();