diff --git a/doc/dbus/bus/org.opensuse.Agama.Storage1.Proposal.bus.xml b/doc/dbus/bus/org.opensuse.Agama.Storage1.Proposal.bus.xml index 96ad98238b..3a5ee0600f 100644 --- a/doc/dbus/bus/org.opensuse.Agama.Storage1.Proposal.bus.xml +++ b/doc/dbus/bus/org.opensuse.Agama.Storage1.Proposal.bus.xml @@ -29,7 +29,7 @@ - + diff --git a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml index 35e40e2833..899dd6d3ce 100644 --- a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml +++ b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml @@ -6,7 +6,7 @@ - + diff --git a/service/lib/agama/dbus/storage/proposal.rb b/service/lib/agama/dbus/storage/proposal.rb index 73e4fd24b1..fe1df2f179 100644 --- a/service/lib/agama/dbus/storage/proposal.rb +++ b/service/lib/agama/dbus/storage/proposal.rb @@ -48,7 +48,7 @@ def initialize(backend, logger) dbus_reader :lvm, "b", dbus_name: "LVM" - dbus_reader :system_vg_devices, "aas", dbus_name: "SystemVGDevices" + dbus_reader :system_vg_devices, "as", dbus_name: "SystemVGDevices" dbus_reader :encryption_password, "s" diff --git a/service/lib/agama/storage/proposal.rb b/service/lib/agama/storage/proposal.rb index 58a353781e..7232bc71a6 100644 --- a/service/lib/agama/storage/proposal.rb +++ b/service/lib/agama/storage/proposal.rb @@ -85,10 +85,13 @@ def settings proposal.settings, config: config ).tap do |settings| - # FIXME: Currently, the conversion from Y2Storage cannot infer the space policy. Copying - # space settings from the original settings. + # FIXME: The conversion from Y2Storage cannot infer the space policy. Copying space + # settings from the original settings. settings.space.policy = original_settings.space.policy settings.space.actions = original_settings.space.actions + # FIXME: The conversion from Y2Storage cannot reliably infer the system VG devices in all + # cases. Copying system VG devices from the original settings. + settings.lvm.system_vg_devices = original_settings.lvm.system_vg_devices end end diff --git a/service/lib/agama/storage/proposal_settings_conversion/from_y2storage.rb b/service/lib/agama/storage/proposal_settings_conversion/from_y2storage.rb index 03b1495c8c..db13a73bc8 100644 --- a/service/lib/agama/storage/proposal_settings_conversion/from_y2storage.rb +++ b/service/lib/agama/storage/proposal_settings_conversion/from_y2storage.rb @@ -64,8 +64,11 @@ def boot_device_conversion(target) def lvm_conversion(target) target.lvm.enabled = settings.lvm - # Only assign system VG devices if candidate devices contains any device different to the - # root device. + # FIXME: The candidate devices list represents the system VG devices if it contains any + # device different to the root device. If the candidate devices only contains the root + # device, then there is no way to know whether the root device was explicitly assigned + # as system VG device. Note that candidate devices will also contain the root device + # when the system VG devices list was empty. candidate_devices = settings.candidate_devices || [] return unless candidate_devices.reject { |d| d == settings.root_device }.any? diff --git a/service/package/rubygem-agama.changes b/service/package/rubygem-agama.changes index 3e587f016b..6d4adcfcfa 100644 --- a/service/package/rubygem-agama.changes +++ b/service/package/rubygem-agama.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Sep 27 12:12:59 UTC 2023 - José Iván López González + +- Fix D-Bus type for SystemVGDevices and restore system VG devices + from previous settings (gh#openSUSE/agama#763). + ------------------------------------------------------------------- Tue Sep 26 15:57:08 UTC 2023 - Imobach Gonzalez Sosa diff --git a/service/test/agama/storage/proposal_test.rb b/service/test/agama/storage/proposal_test.rb index 915172c24f..d276a2f8c8 100644 --- a/service/test/agama/storage/proposal_test.rb +++ b/service/test/agama/storage/proposal_test.rb @@ -164,6 +164,55 @@ ) ) end + + # Checking system VG devices explicitly here because the settings converter cannot infer the + # system VG devices from the Y2Storage settings in all cases. The system VG devices are + # directly recovered from the original settings passed to #calculate. + context "system VG devices" do + let(:settings) do + achievable_settings.tap do |settings| + settings.lvm.system_vg_devices = system_vg_devices + end + end + + context "if no devices were assigned as system VG devices" do + let(:system_vg_devices) { [] } + + it "returns settings containing no system VG devices " do + expect(subject.settings).to have_attributes( + lvm: an_object_having_attributes( + system_vg_devices: be_empty + ) + ) + end + end + + context "if only boot device was assigned as system VG device" do + let(:system_vg_devices) { ["/dev/sdb"] } + + # This case cannot be inferred by conversion from Y2Storage, so the test does not pass if + # system VG devices are not copied from the settings passed to #calculate. + it "returns settings containing only boot device as system VG device" do + expect(subject.settings).to have_attributes( + lvm: an_object_having_attributes( + system_vg_devices: contain_exactly("/dev/sdb") + ) + ) + end + end + + context "if several devices were assigned as system VG devices" do + let(:system_vg_devices) { ["/dev/sdb", "/dev/sdc"] } + + it "returns settings containing the system VG devices" do + expect(subject.settings).to have_attributes( + lvm: an_object_having_attributes( + system_vg_devices: contain_exactly("/dev/sdb", "/dev/sdc") + ) + ) + end + end + end end end diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes index 029a09838d..7ae18309f4 100644 --- a/web/package/cockpit-agama.changes +++ b/web/package/cockpit-agama.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Wed Sep 27 12:15:13 UTC 2023 - José Iván López González + +- Allow to select the devices for the system volume group + (gh#openSUSE/agama#763). + ------------------------------------------------------------------- Tue Sep 26 15:57:21 UTC 2023 - Imobach Gonzalez Sosa diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss index 8592e76f7c..0777d103cb 100644 --- a/web/src/assets/styles/blocks.scss +++ b/web/src/assets/styles/blocks.scss @@ -259,9 +259,8 @@ span.notification-mark[data-variant="sidebar"] { } } -.device-selector { +.device-list { [role="option"] { - cursor: pointer; padding: var(--spacer-normal); border: 2px solid var(--color-gray-dark); background: var(--color-gray-light); diff --git a/web/src/assets/styles/patternfly-overrides.scss b/web/src/assets/styles/patternfly-overrides.scss index 758bb3c6b3..6b8c3c904a 100644 --- a/web/src/assets/styles/patternfly-overrides.scss +++ b/web/src/assets/styles/patternfly-overrides.scss @@ -170,3 +170,8 @@ table td > .pf-c-empty-state { .password-toggler span.pf-c-button__icon { display: flex; } + +.pf-c-toggle-group__button.pf-m-selected { + --pf-c-toggle-group__button--m-selected--BackgroundColor: var(--color-primary); + --pf-c-toggle-group__button--Color: var(--color-gray-light); +} diff --git a/web/src/assets/styles/utilities.scss b/web/src/assets/styles/utilities.scss index 81a8ec0db6..5af45860fb 100644 --- a/web/src/assets/styles/utilities.scss +++ b/web/src/assets/styles/utilities.scss @@ -151,3 +151,7 @@ inline-size: 95dvw; max-inline-size: calc(var(--ui-max-inline-size) + var(--spacer-large)) } + +.cursor-pointer { + cursor: pointer; +} diff --git a/web/src/client/storage.js b/web/src/client/storage.js index 11c55f9936..bf4d2bd7ef 100644 --- a/web/src/client/storage.js +++ b/web/src/client/storage.js @@ -226,8 +226,9 @@ class ProposalManager { /** * @typedef {object} ProposalSettings * @property {string} bootDevice - * @property {boolean} lvm * @property {string} encryptionPassword + * @property {boolean} lvm + * @property {string[]} systemVGDevices * @property {Volume[]} volumes * * @typedef {object} Volume @@ -322,6 +323,7 @@ class ProposalManager { settings: { bootDevice: proxy.BootDevice, lvm: proxy.LVM, + systemVGDevices: proxy.SystemVGDevices, encryptionPassword: proxy.EncryptionPassword, volumes: proxy.Volumes.map(this.buildVolume), }, @@ -338,7 +340,7 @@ class ProposalManager { * @param {ProposalSettings} settings * @returns {Promise} 0 on success, 1 on failure */ - async calculate({ bootDevice, encryptionPassword, lvm, volumes }) { + async calculate({ bootDevice, encryptionPassword, lvm, systemVGDevices, volumes }) { const dbusVolume = (volume) => { return removeUndefinedCockpitProperties({ MountPath: { t: "s", v: volume.mountPath }, @@ -354,6 +356,7 @@ class ProposalManager { BootDevice: { t: "s", v: bootDevice }, EncryptionPassword: { t: "s", v: encryptionPassword }, LVM: { t: "b", v: lvm }, + SystemVGDevices: { t: "as", v: systemVGDevices }, Volumes: { t: "aa{sv}", v: volumes?.map(dbusVolume) } }); diff --git a/web/src/client/storage.test.js b/web/src/client/storage.test.js index 5b3fbd254f..78547bc08c 100644 --- a/web/src/client/storage.test.js +++ b/web/src/client/storage.test.js @@ -153,6 +153,7 @@ const contexts = { cockpitProxies.proposal = { BootDevice: "/dev/sda", LVM: true, + SystemVGDevices: ["/dev/sda", "/dev/sdb"], EncryptionPassword: "00000", Volumes: [ { @@ -694,7 +695,7 @@ describe("#proposal", () => { client = new StorageClient(); }); - it.only("returns the list of product mount points", async () => { + it("returns the list of product mount points", async () => { const mount_points = await client.proposal.getProductMountPoints(); expect(mount_points).toEqual(["/", "swap", "/home"]); }); @@ -814,6 +815,7 @@ describe("#proposal", () => { expect(settings).toStrictEqual({ bootDevice: "/dev/sda", lvm: true, + systemVGDevices: ["/dev/sda", "/dev/sdb"], encryptionPassword: "00000", volumes: [ { @@ -877,6 +879,7 @@ describe("#proposal", () => { bootDevice: "/dev/vdb", encryptionPassword: "12345", lvm: true, + systemVGDevices: ["/dev/sdc"], volumes: [ { mountPath: "/test1", @@ -897,6 +900,7 @@ describe("#proposal", () => { BootDevice: { t: "s", v: "/dev/vdb" }, EncryptionPassword: { t: "s", v: "12345" }, LVM: { t: "b", v: true }, + SystemVGDevices: { t: "as", v: ["/dev/sdc"] }, Volumes: { t: "aa{sv}", v: [ diff --git a/web/src/components/storage/DeviceSelector.test.jsx b/web/src/components/storage/DeviceSelector.test.jsx deleted file mode 100644 index 9d4254df45..0000000000 --- a/web/src/components/storage/DeviceSelector.test.jsx +++ /dev/null @@ -1,279 +0,0 @@ -/* - * Copyright (c) [2022-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. - */ - -// cspell:ignore dasda ddgdcbibhd - -import React from "react"; -import { screen, within } from "@testing-library/react"; -import { plainRender } from "~/test-utils"; -import { DeviceSelector } from "~/components/storage"; - -let props; -const onSelectFn = jest.fn(); - -const vda = { - sid: "59", - type: "disk", - vendor: "Micron", - model: "Micron 1100 SATA", - driver: ["ahci", "mmcblk"], - bus: "IDE", - transport: "usb", - dellBOSS: false, - sdCard: true, - active: true, - name: "/dev/vda", - size: 1024, - systems : ["Windows 11", "openSUSE Leap 15.2"], - udevIds: ["ata-Micron_1100_SATA_512GB_12563", "scsi-0ATA_Micron_1100_SATA_512GB"], - udevPaths: ["pci-0000:00-12", "pci-0000:00-12-ata"], - partitionTable: { type: "gpt", partitions: ["/dev/vda1", "/dev/vda2"] } -}; - -const md0 = { - sid: "62", - type: "md", - level: "raid0", - uuid: "12345:abcde", - members: ["/dev/vdb"], - active: true, - name: "/dev/md0", - size: 2048, - systems : [], - udevIds: [], - udevPaths: [] -}; - -const raid = { - sid: "63", - type: "raid", - devices: ["/dev/sda", "/dev/sdb"], - vendor: "Dell", - model: "Dell BOSS-N1 Modular", - driver: [], - bus: "", - busId: "", - transport: "", - dellBOSS: true, - sdCard: false, - active: true, - name: "/dev/mapper/isw_ddgdcbibhd_244", - size: 2048, - systems : [], - udevIds: [], - udevPaths: [] -}; - -const multipath = { - sid: "64", - type: "multipath", - wires: ["/dev/sdc", "/dev/sdd"], - vendor: "", - model: "", - driver: [], - bus: "", - busId: "", - transport: "", - dellBOSS: false, - sdCard: false, - active: true, - name: "/dev/mapper/36005076305ffc73a00000000000013b4", - size: 2048, - systems : [], - udevIds: [], - udevPaths: [] -}; - -const dasd = { - sid: "65", - type: "dasd", - vendor: "IBM", - model: "IBM", - driver: [], - bus: "", - busId: "0.0.0150", - transport: "", - dellBOSS: false, - sdCard: false, - active: true, - name: "/dev/dasda", - size: 2048, - systems : [], - udevIds: [], - udevPaths: [] -}; - -describe("DeviceSelector", () => { - describe("when no devices are given", () => { - it("renders an empty listbox", () => { - plainRender(); - - const listbox = screen.queryByRole("listbox"); - - expect(listbox).toBeEmptyDOMElement(); - }); - }); - - describe("when devices are given", () => { - beforeEach(() => { - props = { devices: [vda, md0], selected: vda, onSelect: onSelectFn }; - }); - - it("renders a listbox with an option per device", () => { - plainRender(); - - const listbox = screen.queryByRole("listbox"); - - within(listbox).getByRole("option", { name: /vda/ }); - within(listbox).getByRole("option", { name: /md0/ }); - }); - - it("renders as selected the option matching with the selected device", () => { - plainRender(); - - const selectedOptions = screen.queryByRole("option", { selected: true }); - const vdaOption = screen.getByRole("option", { name: /vda/ }); - expect(selectedOptions).toEqual(vdaOption); - }); - - it("updates the selection when the user clicks an option", async () => { - const { user } = plainRender(); - - const vdaOption = screen.getByRole("option", { name: /vda/ }); - const md0Option = screen.getByRole("option", { name: /md0/ }); - - expect(vdaOption).toHaveAttribute("aria-selected"); - expect(md0Option).not.toHaveAttribute("aria-selected"); - - await user.click(md0Option); - - expect(vdaOption).not.toHaveAttribute("aria-selected"); - expect(md0Option).toHaveAttribute("aria-selected"); - }); - - it("triggers the onSelect callback when the selection change", async () => { - const { user } = plainRender(); - - const vdaOption = screen.getByRole("option", { name: /vda/ }); - const md0Option = screen.getByRole("option", { name: /md0/ }); - - // Clicking on an already selected option must do nothing - await user.click(vdaOption); - expect(onSelectFn).not.toHaveBeenCalled(); - - // Clicking multiple times in a not selected option must trigger change - // only once - await user.click(md0Option); - await user.click(md0Option); - - expect(onSelectFn).toHaveBeenCalledTimes(1); - expect(onSelectFn).toHaveBeenCalledWith(md0); - }); - }); - - describe("DeviceSelector options content", () => { - it("renders the device size", () => { - plainRender(); - screen.getByText("1 KiB"); - }); - - it("renders the device name", () => { - plainRender(); - screen.getByText("/dev/vda"); - }); - - it("renders the device model", () => { - plainRender(); - screen.getByText("Micron 1100 SATA"); - }); - - describe("when device is a SDCard", () => { - it("renders 'SD Card'", () => { - const sdCard = { ...vda, sdCard: true }; - plainRender(); - screen.getByText("SD Card"); - }); - }); - - describe("when content is given", () => { - it("renders the partition table info", () => { - plainRender(); - screen.getByText("GPT with 2 partitions"); - }); - - it("renders systems info", () => { - plainRender(); - screen.getByText("Windows 11"); - screen.getByText("openSUSE Leap 15.2"); - }); - }); - - describe("when content is not given", () => { - it("renders 'No content found'", () => { - plainRender(); - screen.getByText("No content found"); - }); - }); - - describe("when device is software RAID", () => { - it("renders its level", () => { - plainRender(); - screen.getByText("Software RAID0"); - }); - - it("renders its members", () => { - plainRender(); - screen.getByText(/Members/); - screen.getByText(/vdb/); - }); - }); - - describe("when device is RAID", () => { - it("renders its devices", () => { - plainRender(); - screen.getByText(/Devices/); - screen.getByText(/sda/); - screen.getByText(/sdb/); - }); - }); - - describe("when device is a multipath", () => { - it("renders 'Multipath'", () => { - plainRender(); - screen.getByText("Multipath"); - }); - - it("renders its wires", () => { - plainRender(); - screen.getByText(/Wires/); - screen.getByText(/sdc/); - screen.getByText(/sdd/); - }); - }); - - describe("when device is DASD", () => { - it("renders its bus id", () => { - plainRender(); - screen.getByText("DASD 0.0.0150"); - }); - }); - }); -}); diff --git a/web/src/components/storage/ProposalSettingsSection.jsx b/web/src/components/storage/ProposalSettingsSection.jsx index a810e20bc7..2277ae2ebb 100644 --- a/web/src/components/storage/ProposalSettingsSection.jsx +++ b/web/src/components/storage/ProposalSettingsSection.jsx @@ -23,12 +23,13 @@ import React, { useEffect, useState } from "react"; import { Button, Form, Skeleton, Switch, + ToggleGroup, ToggleGroupItem, Tooltip } from "@patternfly/react-core"; import { _ } from "~/i18n"; import { If, PasswordAndConfirmationInput, Section, Popup } from "~/components/core"; -import { DeviceSelector, ProposalVolumes } from "~/components/storage"; +import { DeviceList, DeviceSelector, ProposalVolumes } from "~/components/storage"; import { deviceLabel } from '~/components/storage/utils'; import { Icon } from "~/components/layout"; import { noop } from "~/utils"; @@ -40,19 +41,24 @@ import { noop } from "~/utils"; */ /** - * Form for selecting the installation device + * Form for selecting the installation device. * @component * * @param {object} props - * @param {string} props.id - Form ID - * @param {string|undefined} props.current - Device name, if any - * @param {StorageDevice[]} props.devices - Available devices for the selection - * @param {onSubmitFn} props.onSubmit - On submit callback + * @param {string} props.id - Form ID. + * @param {string|undefined} [props.current] - Device name, if any. + * @param {StorageDevice[]} [props.devices=[]] - Available devices for the selection. + * @param {onSubmitFn} [props.onSubmit=noop] - On submit callback. * * @callback onSubmitFn - * @param {string} device + * @param {string} device - Name of the selected device. */ -const InstallationDeviceForm = ({ id, current, devices, onSubmit }) => { +const InstallationDeviceForm = ({ + id, + current, + devices = [], + onSubmit = noop +}) => { const [device, setDevice] = useState(current); useEffect(() => { @@ -68,35 +74,36 @@ const InstallationDeviceForm = ({ id, current, devices, onSubmit }) => { if (device !== undefined) onSubmit(device); }; - const selectDevice = (d) => setDevice(d.name); - - const selected = devices.find(d => d.name === device); - return (
); }; /** - * Allows to select the installation device + * Allows to select the installation device. * @component * * @param {object} props - * @param {string|undefined} props.current - Device name, if any - * @param {StorageDevice[]} props.devices - Available devices for the selection - * @param {boolean} props.isLoading - Whether to show the selector as loading - * @param {onChangeFn} props.onChange - On change callback + * @param {string} [props.current] - Device name, if any. + * @param {StorageDevice[]} [props.devices=[]] - Available devices for the selection. + * @param {boolean} [props.isLoading=false] - Whether to show the selector as loading. + * @param {onChangeFn} [props.onChange=noop] - On change callback. * * @callback onChangeFn - * @param {string} device + * @param {string} device - Name of the selected device. */ -const InstallationDeviceField = ({ current, devices, isLoading, onChange }) => { +const InstallationDeviceField = ({ + current, + devices = [], + isLoading = false, + onChange = noop +}) => { const [device, setDevice] = useState(current); const [isFormOpen, setIsFormOpen] = useState(false); @@ -125,8 +132,7 @@ const InstallationDeviceField = ({ current, devices, isLoading, onChange }) => { return ; } - const description = _("Select the device for installing the system. All the \ -file systems will be created on the selected device."); + const description = _("Select the device for installing the system."); return ( <> @@ -167,47 +173,207 @@ file systems will be created on the selected device."); }; /** - * Allows to select LVM + * Form for configuring the system volume group. + * @component + * + * @param {object} props + * @param {string} props.id - Form ID. + * @param {ProposalSettings} props.settings - Settings used for calculating a proposal. + * @param {StorageDevice[]} [props.devices=[]] - Available storage devices. + * @param {onSubmitFn} [props.onSubmit=noop] - On submit callback. + * @param {onValidateFn} [props.onValidate=noop] - On validate callback. + * + * @callback onSubmitFn + * @param {string[]} devices - Name of the selected devices. + * + * @callback onValidateFn + * @param {boolean} valid + */ +const LVMSettingsForm = ({ + id, + settings, + devices = [], + onSubmit: onSubmitProp = noop, + onValidate = noop +}) => { + const [vgDevices, setVgDevices] = useState(settings.systemVGDevices); + const [isBootDeviceSelected, setIsBootDeviceSelected] = useState(settings.systemVGDevices.length === 0); + const [editedDevices, setEditedDevices] = useState(false); + + const selectBootDevice = () => { + setIsBootDeviceSelected(true); + onValidate(true); + }; + + const selectCustomDevices = () => { + setIsBootDeviceSelected(false); + const { bootDevice } = settings; + const customDevices = (vgDevices.length === 0 && !editedDevices) ? [bootDevice] : vgDevices; + setVgDevices(customDevices); + onValidate(customDevices.length > 0); + }; + + const onChangeDevices = (devices) => { + setVgDevices(devices); + setEditedDevices(true); + onValidate(devices.length > 0); + }; + + const onSubmit = (e) => { + e.preventDefault(); + const customDevices = isBootDeviceSelected ? [] : vgDevices; + onSubmitProp(customDevices); + }; + + const BootDevice = () => { + const bootDevice = devices.find(d => d.name === settings.bootDevice); + + return ; + }; + + return ( +
+
+ {_("Devices for creating the volume group")} + + + + +
+ } + else={ + + } + /> + + ); +}; + +/** + * Allows to select LVM and configure the system volume group. * @component * * @param {object} props - * @param {boolean} props.selected - Whether LVM is selected - * @param {boolean} props.isLoading - Whether to show the selector as loading - * @param {onChangeFn} props.onChange - On change callback + * @param {ProposalSettings} props.settings - Settings used for calculating a proposal. + * @param {StorageDevice[]} [props.devices=[]] - Available storage devices. + * @param {boolean} [props.isChecked=false] - Whether LVM is selected. + * @param {boolean} [props.isLoading=false] - Whether to show the selector as loading. + * @param {onChangeFn} [props.onChange=noop] - On change callback. * * @callback onChangeFn * @param {boolean} lvm */ -const LVMField = ({ selected: selectedProp, isLoading, onChange }) => { - const [selected, setSelected] = useState(selectedProp); +const LVMField = ({ + settings, + devices = [], + isChecked: isCheckedProp = false, + isLoading = false, + onChange: onChangeProp = noop +}) => { + const [isChecked, setIsChecked] = useState(isCheckedProp); + const [isFormOpen, setIsFormOpen] = useState(false); + const [isFormValid, setIsFormValid] = useState(true); - const changeSelected = (value) => { - setSelected(value); - onChange(value); + const onChange = (value) => { + setIsChecked(value); + onChangeProp({ lvm: value, vgDevices: [] }); + }; + + const openForm = () => setIsFormOpen(true); + + const closeForm = () => setIsFormOpen(false); + + const onValidateForm = (valid) => setIsFormValid(valid); + + const onSubmitForm = (vgDevices) => { + closeForm(); + onChangeProp({ vgDevices }); + }; + + const description = _("Configuration of the system volume group. All the file systems will be \ +created in a logical volume of the system volume group."); + + const LVMSettingsButton = () => { + return ( + + + + ); }; if (isLoading) return ; return ( - +
+ + } /> + + + + + {_("Accept")} + + + + +
); }; /** - * Form for configuring the encryption password + * Form for configuring the encryption password. * @component * * @param {object} props - * @param {string} props.id - Form ID - * @param {string} props.password - Password for encryption - * @param {onSubmitFn} props.onSubmit - On submit callback - * @param {onValidateFn} props.onValidate - On validate callback + * @param {string} props.id - Form ID. + * @param {string} props.password - Password for encryption. + * @param {onSubmitFn} [props.onSubmit=noop] - On submit callback. + * @param {onValidateFn} [props.onValidate=noop] - On validate callback. * * @callback onSubmitFn * @param {string} password @@ -215,7 +381,12 @@ const LVMField = ({ selected: selectedProp, isLoading, onChange }) => { * @callback onValidateFn * @param {boolean} valid */ -const EncryptionPasswordForm = ({ id, password: passwordProp, onSubmit, onValidate }) => { +const EncryptionPasswordForm = ({ + id, + password: passwordProp, + onSubmit = noop, + onValidate = noop +}) => { const [password, setPassword] = useState(passwordProp || ""); useEffect(() => { @@ -246,16 +417,21 @@ const EncryptionPasswordForm = ({ id, password: passwordProp, onSubmit, onValida * @component * * @param {object} props - * @param {boolean} props.selected - Whether encryption is selected - * @param {string} props.password - Password for encryption - * @param {boolean} props.isLoading - Whether to show the selector as loading - * @param {onChangeFn} props.onChange - On change callback + * @param {string} [props.password=""] - Password for encryption + * @param {boolean} [props.isChecked=false] - Whether encryption is selected + * @param {boolean} [props.isLoading=false] - Whether to show the selector as loading + * @param {onChangeFn} [props.onChange=noop] - On change callback * * @callback onChangeFn * @param {object} settings */ -const EncryptionPasswordField = ({ selected: selectedProp, password: passwordProp, isLoading, onChange }) => { - const [selected, setSelected] = useState(selectedProp); +const EncryptionPasswordField = ({ + password: passwordProp = "", + isChecked: isCheckedProp = false, + isLoading = false, + onChange = noop +}) => { + const [isChecked, setIsChecked] = useState(isCheckedProp); const [password, setPassword] = useState(passwordProp); const [isFormOpen, setIsFormOpen] = useState(false); const [isFormValid, setIsFormValid] = useState(true); @@ -267,24 +443,24 @@ const EncryptionPasswordField = ({ selected: selectedProp, password: passwordPro const acceptForm = (newPassword) => { closeForm(); setPassword(newPassword); - onChange({ selected, password: newPassword }); + onChange({ isChecked, password: newPassword }); }; const cancelForm = () => { closeForm(); - if (password.length === 0) setSelected(false); + if (password.length === 0) setIsChecked(false); }; const validateForm = (valid) => setIsFormValid(valid); const changeSelected = (value) => { - setSelected(value); + setIsChecked(value); if (value && password.length === 0) openForm(); if (!value) { setPassword(""); - onChange({ selected: false, password: "" }); + onChange({ isChecked: false, password: "" }); } }; @@ -312,10 +488,10 @@ const EncryptionPasswordField = ({ selected: selectedProp, password: passwordPro id="encryption" label={_("Use encryption")} isReversed - isChecked={selected} + isChecked={isChecked} onChange={changeSelected} /> - { selected && } + { isChecked && } { - if (onChange === noop) return; onChange({ bootDevice: device }); }; - const changeLVM = (lvm) => { - if (onChange === noop) return; - onChange({ lvm }); + const changeLVM = ({ lvm, vgDevices }) => { + const settings = {}; + if (lvm !== undefined) settings.lvm = lvm; + if (vgDevices !== undefined) settings.systemVGDevices = vgDevices; + + onChange(settings); }; const changeEncryption = ({ password }) => { - if (onChange === noop) return; onChange({ encryptionPassword: password }); }; const changeVolumes = (volumes) => { - if (onChange === noop) return; onChange({ volumes }); }; @@ -386,15 +562,17 @@ export default function ProposalSettingsSection({ onChange={changeBootDevice} /> { props = {}; }); @@ -189,7 +217,8 @@ describe("LVM field", () => { describe("if LVM is set to true", () => { beforeEach(() => { - props.settings = { lvm: true }; + props.availableDevices = [vda, md0, md1]; + props.settings = { bootDevice: "/dev/vda", lvm: true, systemVGDevices: [] }; props.onChange = jest.fn(); }); @@ -200,6 +229,12 @@ describe("LVM field", () => { expect(checkbox).toBeChecked(); }); + it("renders a button for changing the LVM settings", () => { + plainRender(); + + screen.getByRole("button", { name: /LVM settings/ }); + }); + it("changes the selection on click", async () => { const { user } = plainRender(); @@ -209,6 +244,84 @@ describe("LVM field", () => { expect(checkbox).not.toBeChecked(); expect(props.onChange).toHaveBeenCalled(); }); + + describe("and user clicks on LVM settings", () => { + it("opens the LVM settings dialog", async () => { + const { user } = plainRender(); + const settingsButton = screen.getByRole("button", { name: /LVM settings/ }); + + await user.click(settingsButton); + + const popup = await screen.findByRole("dialog"); + within(popup).getByText("System Volume Group"); + }); + + it("allows selecting either installation device or custom devices", async() => { + const { user } = plainRender(); + const settingsButton = screen.getByRole("button", { name: /LVM settings/ }); + + await user.click(settingsButton); + + const popup = await screen.findByRole("dialog"); + screen.getByText("System Volume Group"); + + within(popup).getByRole("button", { name: "Installation device" }); + within(popup).getByRole("button", { name: "Custom devices" }); + }); + + it("allows to set the installation device as system volume group", async () => { + const { user } = plainRender(); + const settingsButton = screen.getByRole("button", { name: /LVM settings/ }); + + await user.click(settingsButton); + + const popup = await screen.findByRole("dialog"); + screen.getByText("System Volume Group"); + + const bootDeviceButton = within(popup).getByRole("button", { name: "Installation device" }); + const customDevicesButton = within(popup).getByRole("button", { name: "Custom devices" }); + const acceptButton = within(popup).getByRole("button", { name: "Accept" }); + + await user.click(customDevicesButton); + await user.click(bootDeviceButton); + await user.click(acceptButton); + + expect(props.onChange).toHaveBeenCalledWith( + expect.objectContaining({ systemVGDevices: [] }) + ); + }); + + it("allows customize the system volume group", async () => { + const { user } = plainRender(); + const settingsButton = screen.getByRole("button", { name: /LVM settings/ }); + + await user.click(settingsButton); + + const popup = await screen.findByRole("dialog"); + screen.getByText("System Volume Group"); + + const customDevicesButton = within(popup).getByRole("button", { name: "Custom devices" }); + const acceptButton = within(popup).getByRole("button", { name: "Accept" }); + + await user.click(customDevicesButton); + + const vdaOption = within(popup).getByRole("option", { name: /vda/ }); + const md0Option = within(popup).getByRole("option", { name: /md0/ }); + const md1Option = within(popup).getByRole("option", { name: /md1/ }); + + // unselect the boot devices + await user.click(vdaOption); + + await user.click(md0Option); + await user.click(md1Option); + + await user.click(acceptButton); + + expect(props.onChange).toHaveBeenCalledWith( + expect.objectContaining({ systemVGDevices: ["/dev/md0", "/dev/md1"] }) + ); + }); + }); }); describe("if LVM is set to false", () => { @@ -224,6 +337,13 @@ describe("LVM field", () => { expect(checkbox).not.toBeChecked(); }); + it("does not render a button for changing the LVM settings", () => { + plainRender(); + + const button = screen.queryByRole("button", { name: /LVM settings/ }); + expect(button).toBeNull(); + }); + it("changes the selection on click", async () => { const { user } = plainRender(); diff --git a/web/src/components/storage/DeviceSelector.jsx b/web/src/components/storage/device-utils.jsx similarity index 76% rename from web/src/components/storage/DeviceSelector.jsx rename to web/src/components/storage/device-utils.jsx index b237e3fb2c..30900d6017 100644 --- a/web/src/components/storage/DeviceSelector.jsx +++ b/web/src/components/storage/device-utils.jsx @@ -19,7 +19,7 @@ * find current contact information at www.suse.com. */ -import React, { useState } from "react"; +import React from "react"; import { sprintf } from "sprintf-js"; import { _ } from "~/i18n"; @@ -34,8 +34,7 @@ import { deviceSize } from "~/components/storage/utils"; const ListBox = ({ children, ...props }) =>
    {children}
; -const ListBoxItem = ({ isSelected, children, onClick }) => { - const props = {}; +const ListBoxItem = ({ isSelected, children, onClick, ...props }) => { if (isSelected) props['aria-selected'] = true; return ( @@ -56,7 +55,7 @@ const ListBoxItem = ({ isSelected, children, onClick }) => { * @param {Object} props * @param {StorageDevice} props.device */ -const ItemContent = ({ device }) => { +const DeviceItem = ({ device }) => { const BasicInfo = () => { const DeviceIcon = () => { const names = { @@ -224,38 +223,64 @@ const ItemContent = ({ device }) => { }; /** - * Component for selecting a storage device + * Component for listing storage devices. * @component * * @param {Object} props - * @param {StorageDevice} [props.selected] - Default selected device, in any - * @param {StorageDevice[]=[]} [props.devices] - Devices to show in the selector - * @param {onSelectFn} [props.onSelect] - Function to be called when a device is selected + * @param {StorageDevice[]} props.devices - Devices to show. + */ +const DeviceList = ({ devices }) => { + return ( + + { devices.map(device => ( + + + + ))} + + ); +}; + +/** + * Component for selecting storage devices. + * @component + * + * @param {Object} props + * @param {StorageDevice[]} props.devices - Devices to show in the selector. + * @param {StorageDevice|StorageDevice[]} [props.selected] - Currently selected device. In case of + * multi selection, an array of devices can be used. + * @param {boolean} [props.isMultiple=false] - Activate multi selection. + * @param {onChangeFn} [props.onChange] - Callback to be called when the selected devices changes. * - * @callback onSelectFn - * @param {StorageDevice} device - Selected device + * @callback onChangeFn + * @param {StorageDevice|StorageDevice[]} selected - Selected device, or array of selected devices + * in case of multi selection. */ -export default function DeviceSelector ({ selected, devices = [], onSelect = noop }) { - const [selectedDevice, setSelectedDevice] = useState(selected); +const DeviceSelector = ({ devices, selected, isMultiple = false, onChange = noop }) => { + const selectedDevices = () => [selected].flat().filter(Boolean); - const onOptionClick = (device) => { - if (device === selectedDevice) return; + const isSelected = (device) => selectedDevices().includes(device); - setSelectedDevice(device); - onSelect(device); + const onOptionClick = (device) => { + if (!isMultiple && !isSelected(device)) onChange(device); + if (isMultiple && !isSelected(device)) onChange([...selectedDevices(), device]); + if (isMultiple && isSelected(device)) onChange(selectedDevices().filter(d => d !== device)); }; return ( - + { devices.map(device => ( onOptionClick(device)} - isSelected={device === selectedDevice} + onClick={() => onOptionClick(device.name)} + isSelected={isSelected(device.name)} + className="cursor-pointer" > - + ))} ); -} +}; + +export { DeviceList, DeviceSelector }; diff --git a/web/src/components/storage/device-utils.test.jsx b/web/src/components/storage/device-utils.test.jsx new file mode 100644 index 0000000000..b23f6af0d8 --- /dev/null +++ b/web/src/components/storage/device-utils.test.jsx @@ -0,0 +1,331 @@ +/* + * Copyright (c) [2022-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. + */ + +// cspell:ignore dasda ddgdcbibhd + +import React, { useState } from "react"; +import { screen, within } from "@testing-library/react"; +import { plainRender } from "~/test-utils"; +import { DeviceList, DeviceSelector } from "~/components/storage/device-utils"; + +const vda = { + sid: "59", + type: "disk", + vendor: "Micron", + model: "Micron 1100 SATA", + driver: ["ahci", "mmcblk"], + bus: "IDE", + transport: "usb", + dellBOSS: false, + sdCard: true, + active: true, + name: "/dev/vda", + size: 1024, + systems : ["Windows 11", "openSUSE Leap 15.2"], + udevIds: ["ata-Micron_1100_SATA_512GB_12563", "scsi-0ATA_Micron_1100_SATA_512GB"], + udevPaths: ["pci-0000:00-12", "pci-0000:00-12-ata"], + partitionTable: { type: "gpt", partitions: ["/dev/vda1", "/dev/vda2"] } +}; + +const md0 = { + sid: "62", + type: "md", + level: "raid0", + uuid: "12345:abcde", + members: ["/dev/vdb"], + active: true, + name: "/dev/md0", + size: 2048, + systems : [], + udevIds: [], + udevPaths: [] +}; + +const raid = { + sid: "63", + type: "raid", + devices: ["/dev/sda", "/dev/sdb"], + vendor: "Dell", + model: "Dell BOSS-N1 Modular", + driver: [], + bus: "", + busId: "", + transport: "", + dellBOSS: true, + sdCard: false, + active: true, + name: "/dev/mapper/isw_ddgdcbibhd_244", + size: 2048, + systems : [], + udevIds: [], + udevPaths: [] +}; + +const multipath = { + sid: "64", + type: "multipath", + wires: ["/dev/sdc", "/dev/sdd"], + vendor: "", + model: "", + driver: [], + bus: "", + busId: "", + transport: "", + dellBOSS: false, + sdCard: false, + active: true, + name: "/dev/mapper/36005076305ffc73a00000000000013b4", + size: 2048, + systems : [], + udevIds: [], + udevPaths: [] +}; + +const dasd = { + sid: "65", + type: "dasd", + vendor: "IBM", + model: "IBM", + driver: [], + bus: "", + busId: "0.0.0150", + transport: "", + dellBOSS: false, + sdCard: false, + active: true, + name: "/dev/dasda", + size: 2048, + systems : [], + udevIds: [], + udevPaths: [] +}; + +const renderOptions = (Component) => { + return () => describe("content", () => { + describe("when no devices are given", () => { + it("renders an empty listbox", () => { + plainRender(); + + const listbox = screen.queryByRole("listbox"); + + expect(listbox).toBeEmptyDOMElement(); + }); + }); + + describe("when devices are given", () => { + it("renders a listbox with an option per device", () => { + plainRender(); + + const listbox = screen.queryByRole("listbox"); + + within(listbox).getByRole("option", { name: /vda/ }); + within(listbox).getByRole("option", { name: /md0/ }); + }); + + it("renders the device size", () => { + plainRender(); + screen.getByText("1 KiB"); + }); + + it("renders the device name", () => { + plainRender(); + screen.getByText("/dev/vda"); + }); + + it("renders the device model", () => { + plainRender(); + screen.getByText("Micron 1100 SATA"); + }); + + describe("when device is a SDCard", () => { + it("renders 'SD Card'", () => { + const sdCard = { ...vda, sdCard: true }; + plainRender(); + screen.getByText("SD Card"); + }); + }); + + describe("when content is given", () => { + it("renders the partition table info", () => { + plainRender(); + screen.getByText("GPT with 2 partitions"); + }); + + it("renders systems info", () => { + plainRender(); + screen.getByText("Windows 11"); + screen.getByText("openSUSE Leap 15.2"); + }); + }); + + describe("when content is not given", () => { + it("renders 'No content found'", () => { + plainRender(); + screen.getByText("No content found"); + }); + }); + + describe("when device is software RAID", () => { + it("renders its level", () => { + plainRender(); + screen.getByText("Software RAID0"); + }); + + it("renders its members", () => { + plainRender(); + screen.getByText(/Members/); + screen.getByText(/vdb/); + }); + }); + + describe("when device is RAID", () => { + it("renders its devices", () => { + plainRender(); + screen.getByText(/Devices/); + screen.getByText(/sda/); + screen.getByText(/sdb/); + }); + }); + + describe("when device is a multipath", () => { + it("renders 'Multipath'", () => { + plainRender(); + screen.getByText("Multipath"); + }); + + it("renders its wires", () => { + plainRender(); + screen.getByText(/Wires/); + screen.getByText(/sdc/); + screen.getByText(/sdd/); + }); + }); + + describe("when device is DASD", () => { + it("renders its bus id", () => { + plainRender(); + screen.getByText("DASD 0.0.0150"); + }); + }); + }); + }); +}; + +describe("DeviceList", renderOptions(DeviceList)); +describe("DeviceList", () => { + it("renders all options as selected", () => { + plainRender(); + + const selectedOptions = screen.queryAllByRole("option", { selected: true }); + expect(selectedOptions.length).toEqual(3); + }); +}); + +describe("DeviceSelector", renderOptions(DeviceSelector)); +describe("DeviceSelector", () => { + it("renders as selected options matching selected device(s)", () => { + plainRender( + + ); + + const selectedOptions = screen.queryAllByRole("option", { selected: true }); + const vdaOption = screen.getByRole("option", { name: /vda/ }); + const dasdOption = screen.getByRole("option", { name: /dasda/ }); + expect(selectedOptions).toEqual([vdaOption, dasdOption]); + }); + + describe("when user clicks an option", () => { + describe("and it only allows single selection", () => { + const onChangeFn = jest.fn(); + + const TestSingleDeviceSelection = () => { + const [selected, setSelected] = useState("/dev/vda"); + + onChangeFn.mockImplementation(device => setSelected(device)); + + return ( + + ); + }; + + it("notifies selected device if it has changed", async () => { + const { user } = plainRender(); + + const vdaOption = screen.getByRole("option", { name: /vda/ }); + const md0Option = screen.getByRole("option", { name: /md0/ }); + + // click on selected device to check nothing is notified + await user.click(vdaOption); + expect(onChangeFn).not.toHaveBeenCalled(); + + await user.click(md0Option); + expect(onChangeFn).toHaveBeenCalledWith("/dev/md0"); + + await user.click(vdaOption); + expect(onChangeFn).toHaveBeenCalledWith("/dev/vda"); + }); + }); + + describe("and it allows multiple selection", () => { + const onChangeFn = jest.fn(); + + const TestMultipleDeviceSelection = () => { + const [selected, setSelected] = useState("/dev/vda"); + + onChangeFn.mockImplementation(devices => setSelected(devices)); + + return ( + + ); + }; + + it("notifies selected devices", async () => { + const { user } = plainRender(); + + const vdaOption = screen.getByRole("option", { name: /vda/ }); + const md0Option = screen.getByRole("option", { name: /md0/ }); + const dasdOption = screen.getByRole("option", { name: /dasda/ }); + + await user.click(md0Option); + expect(onChangeFn).toHaveBeenCalledWith(["/dev/vda", "/dev/md0"]); + + await user.click(dasdOption); + expect(onChangeFn).toHaveBeenCalledWith(["/dev/vda", "/dev/md0", "/dev/dasda"]); + + // click on selected device to check it is notified as not selected + await user.click(vdaOption); + expect(onChangeFn).toHaveBeenCalledWith(["/dev/md0", "/dev/dasda"]); + }); + }); + }); +}); diff --git a/web/src/components/storage/index.js b/web/src/components/storage/index.js index 4021ec4667..bb9b0d03dd 100644 --- a/web/src/components/storage/index.js +++ b/web/src/components/storage/index.js @@ -30,5 +30,5 @@ export { default as DASDFormatProgress } from "./DASDFormatProgress"; export { default as ZFCPPage } from "./ZFCPPage"; export { default as ZFCPDiskForm } from "./ZFCPDiskForm"; export { default as ISCSIPage } from "./ISCSIPage"; -export { default as DeviceSelector } from "./DeviceSelector"; +export { DeviceList, DeviceSelector } from "./device-utils"; export { default as VolumeForm } from "./VolumeForm";