From e431192b536ac90dca10bc4a0b7068d74798b3a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20D=C3=ADaz=20Gonz=C3=A1lez?= Date: Wed, 21 Feb 2024 08:16:01 +0000 Subject: [PATCH] [web] Move device selection to its own section And take the opportunity to avoid triggering the onChange callback when the user actually does not change the selected device but clicks the "Accept" button of the selection dialog. --- .../storage/ProposalDeviceSection.jsx | 208 ++++++++++++++++++ .../storage/ProposalDeviceSection.test.jsx | 182 +++++++++++++++ web/src/components/storage/ProposalPage.jsx | 7 + .../storage/ProposalSettingsSection.jsx | 148 ------------- .../storage/ProposalSettingsSection.test.jsx | 110 --------- web/src/components/storage/index.js | 1 + 6 files changed, 398 insertions(+), 258 deletions(-) create mode 100644 web/src/components/storage/ProposalDeviceSection.jsx create mode 100644 web/src/components/storage/ProposalDeviceSection.test.jsx diff --git a/web/src/components/storage/ProposalDeviceSection.jsx b/web/src/components/storage/ProposalDeviceSection.jsx new file mode 100644 index 0000000000..8c2f3dc61c --- /dev/null +++ b/web/src/components/storage/ProposalDeviceSection.jsx @@ -0,0 +1,208 @@ +/* + * Copyright (c) [2024] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of version 2 of the GNU General Public License as published + * by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, contact SUSE LLC. + * + * To contact SUSE LLC about this file by physical or electronic mail, you may + * find current contact information at www.suse.com. + */ + +import React, { useState } from "react"; +import { Button, Form, Skeleton } from "@patternfly/react-core"; + +import { _ } from "~/i18n"; +import { If, Section, Popup } from "~/components/core"; +import { DeviceSelector } from "~/components/storage"; +import { deviceLabel } from '~/components/storage/utils'; +import { noop } from "~/utils"; + +/** + * @typedef {import ("~/client/storage").ProposalManager.ProposalSettings} ProposalSettings + * @typedef {import ("~/client/storage").DevicesManager.StorageDevice} StorageDevice + */ + +/** + * Form for selecting the installation device. + * @component + * + * @param {object} props + * @param {string} props.id - Form ID. + * @param {StorageDevice|undefined} [props.current] - Currently selected device, if any. + * @param {StorageDevice[]} [props.devices=[]] - Available devices for the selection. + * @param {onSubmitFn} [props.onSubmit=noop] - On submit callback. + * + * @callback onSubmitFn + * @param {string} device - Name of the selected device. + */ +const InstallationDeviceForm = ({ + id, + current, + devices = [], + onSubmit = noop +}) => { + const [device, setDevice] = useState(current || devices[0]); + + const changeSelected = (deviceId) => { + setDevice(devices.find(d => d.sid === deviceId)); + }; + + const submitForm = (e) => { + e.preventDefault(); + if (device !== undefined) onSubmit(device); + }; + + return ( +
+ + + ); +}; + +/** + * Allows to select the installation device. + * @component + * + * @callback onChangeFn + * @param {string} device - Name of the selected device. + * + * @param {object} props + * @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. + */ +const InstallationDeviceField = ({ + current, + devices = [], + isLoading = false, + onChange = noop +}) => { + const [device, setDevice] = useState(devices.find(d => d.name === current)); + const [isFormOpen, setIsFormOpen] = useState(false); + + const openForm = () => setIsFormOpen(true); + + const closeForm = () => setIsFormOpen(false); + + const acceptForm = (selectedDevice) => { + closeForm(); + setDevice(selectedDevice); + onChange(selectedDevice); + }; + + /** + * Renders a button that allows changing selected device + * + * NOTE: if a device is already selected, its name and size will be used for + * the button text. Otherwise, a "No device selected" text will be shown. + * + * @param {object} props + * @param {StorageDevice|undefined} [props.current] - Currently selected device, if any. + */ + const DeviceContent = ({ device }) => { + return ( + + ); + }; + + if (isLoading) { + return ; + } + + const description = _("Select the device for installing the system."); + + return ( + <> +
+ {_("Installation device")} + +
+ + + } + /> + + + {_("Accept")} + + + + + + ); +}; + +/** + * Section for editing the selected device + * @component + * + * @callback onChangeFn + * @param {object} settings + * + * @param {object} props + * @param {ProposalSettings} props.settings + * @param {StorageDevice[]} [props.availableDevices=[]] + * @param {boolean} [isLoading=false] + * @param {onChangeFn} [props.onChange=noop] + */ +export default function ProposalDeviceSection({ + settings, + availableDevices = [], + isLoading = false, + onChange = noop +}) { + // FIXME: we should work with devices objects ASAP + const { bootDevice } = settings; + + const changeBootDevice = (device) => { + if (device.name !== bootDevice) { + onChange({ bootDevice: device.name }); + } + }; + + return ( +
+ +
+ ); +} diff --git a/web/src/components/storage/ProposalDeviceSection.test.jsx b/web/src/components/storage/ProposalDeviceSection.test.jsx new file mode 100644 index 0000000000..e90db6a054 --- /dev/null +++ b/web/src/components/storage/ProposalDeviceSection.test.jsx @@ -0,0 +1,182 @@ +/* + * Copyright (c) [2024] SUSE LLC + * + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of version 2 of the GNU General Public License as published + * by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, contact SUSE LLC. + * + * To contact SUSE LLC about this file by physical or electronic mail, you may + * find current contact information at www.suse.com. + */ + +import React from "react"; +import { screen, within } from "@testing-library/react"; +import { plainRender } from "~/test-utils"; +import { ProposalDeviceSection } from "~/components/storage"; + +const sda = { + 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, + recoverableSize: 0, + systems : [], + udevIds: ["ata-Micron_1100_SATA_512GB_12563", "scsi-0ATA_Micron_1100_SATA_512GB"], + udevPaths: ["pci-0000:00-12", "pci-0000:00-12-ata"], +}; + +const sdb = { + sid: "62", + isDrive: true, + type: "disk", + vendor: "Samsung", + model: "Samsung Evo 8 Pro", + driver: ["ahci"], + bus: "IDE", + busId: "", + transport: "", + dellBOSS: false, + sdCard: false, + active: true, + name: "/dev/sdb", + size: 2048, + recoverableSize: 0, + systems : [], + udevIds: [], + udevPaths: ["pci-0000:00-19"] +}; + +const props = { + settings: { + bootDevice: "/dev/sda", + }, + availableDevices: [sda, sdb], + isLoading: false, + onChange: jest.fn() +}; + +describe("ProposalDeviceSection", () => { + describe("when set as loading", () => { + beforeEach(() => { + props.isLoading = true; + }); + + describe("and selected device is not defined yet", () => { + beforeEach(() => { + props.settings = { bootDevice: undefined }; + }); + + it("renders a loading hint", () => { + plainRender(); + screen.getByText("Loading selected device"); + }); + }); + }); + + describe("when installation device is not selected yet", () => { + beforeEach(() => { + props.settings = { bootDevice: "" }; + }); + + it("uses a 'No device selected yet' text for the selection button", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: "No device selected yet" }); + + await user.click(button); + + screen.getByRole("dialog", { name: "Installation device" }); + }); + }); + + describe("when installation device is selected", () => { + beforeEach(() => { + props.settings = { bootDevice: "/dev/sda" }; + }); + + it("uses its name as part of the text for the selection button", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: /\/dev\/sda/ }); + + await user.click(button); + + screen.getByRole("dialog", { name: "Installation device" }); + }); + }); + + it("allows changing the selected device", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: "/dev/sda, 1 KiB" }); + + await user.click(button); + + const selector = await screen.findByRole("dialog", { name: "Installation device" }); + const sdbOption = within(selector).getByRole("radio", { name: /sdb/ }); + const accept = within(selector).getByRole("button", { name: "Accept" }); + + await user.click(sdbOption); + await user.click(accept); + + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(props.onChange).toHaveBeenCalledWith({ bootDevice: sdb.name }); + }); + + it("allows canceling a device selection", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: "/dev/sda, 1 KiB" }); + + await user.click(button); + + const selector = await screen.findByRole("dialog", { name: "Installation device" }); + const sdbOption = within(selector).getByRole("radio", { name: /sdb/ }); + const cancel = within(selector).getByRole("button", { name: "Cancel" }); + + await user.click(sdbOption); + await user.click(cancel); + + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(props.onChange).not.toHaveBeenCalled(); + }); + + it("does not trigger the onChange callback when selection actually did not change", async () => { + const { user } = plainRender(); + const button = screen.getByRole("button", { name: "/dev/sda, 1 KiB" }); + + await user.click(button); + + const selector = await screen.findByRole("dialog", { name: "Installation device" }); + const sdaOption = within(selector).getByRole("radio", { name: /sda/ }); + const sdbOption = within(selector).getByRole("radio", { name: /sdb/ }); + const accept = within(selector).getByRole("button", { name: "Accept" }); + + // User selects a different device + await user.click(sdbOption); + // but then goes back to the selected device + await user.click(sdaOption); + // and clicks on Accept button + await user.click(accept); + + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + // There is no reason for triggering the onChange callback + expect(props.onChange).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/components/storage/ProposalPage.jsx b/web/src/components/storage/ProposalPage.jsx index 4d730cce93..faeb34f240 100644 --- a/web/src/components/storage/ProposalPage.jsx +++ b/web/src/components/storage/ProposalPage.jsx @@ -32,6 +32,7 @@ import { ProposalPageMenu, ProposalSettingsSection, ProposalSpacePolicySection, + ProposalDeviceSection, } from "~/components/storage"; import { IDLE } from "~/client/status"; @@ -215,6 +216,12 @@ export default function ProposalPage() { customIcon={} title={_("Devices will not be modified until installation starts.")} /> + { - const [device, setDevice] = useState(current || devices[0]); - - const changeSelected = (deviceId) => { - setDevice(devices.find(d => d.sid === deviceId)); - }; - - const submitForm = (e) => { - e.preventDefault(); - if (device !== undefined) onSubmit(device); - }; - - return ( -
- - - ); -}; - -/** - * Allows to select the installation device. - * @component - * - * @param {object} props - * @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 - Name of the selected device. - */ -const InstallationDeviceField = ({ - current, - devices = [], - isLoading = false, - onChange = noop -}) => { - const [device, setDevice] = useState(devices.find(d => d.name === current)); - const [isFormOpen, setIsFormOpen] = useState(false); - - const openForm = () => setIsFormOpen(true); - - const closeForm = () => setIsFormOpen(false); - - const acceptForm = (selectedDevice) => { - closeForm(); - setDevice(selectedDevice); - onChange(selectedDevice); - }; - - /** - * Renders a button that allows changing selected device - * - * NOTE: if a device is already selected, its name and size will be used for - * the button text. Otherwise, a "No device selected" text will be shown. - * - * @param {object} props - * @param {StorageDevice|undefined} [props.current] - Currently selected device, if any. - */ - const DeviceContent = ({ device }) => { - return ( - - ); - }; - - if (isLoading) { - return ; - } - - const description = _("Select the device for installing the system."); - - return ( - <> -
- {_("Installation device")} - -
- - - } - /> - - - {_("Accept")} - - - - - - ); -}; - /** * Form for configuring the system volume group. * @component @@ -575,11 +439,6 @@ export default function ProposalSettingsSection({ isLoading = false, onChange = noop }) { - // FIXME: we should work with devices objects ASAP - const changeBootDevice = (device) => { - onChange({ bootDevice: device.name }); - }; - const changeLVM = ({ lvm, vgDevices }) => { const settings = {}; if (lvm !== undefined) settings.lvm = lvm; @@ -596,18 +455,11 @@ export default function ProposalSettingsSection({ onChange({ volumes }); }; - const { bootDevice } = settings; const encryption = settings.encryptionPassword !== undefined && settings.encryptionPassword.length > 0; return ( <>
- { props = {}; }); -describe("Installation device field", () => { - describe("if it is loading", () => { - beforeEach(() => { - props.isLoading = true; - }); - - describe("and there is no selected device yet", () => { - beforeEach(() => { - props.settings = { bootDevice: "" }; - }); - - it("renders a message indicating that the device is not selected", () => { - plainRender(); - - screen.getByText(/Installation device/); - screen.getByText(/No device selected/); - }); - }); - - // FIXME: skipping this test example by now. - // It will be addressed when reworking the Device section - // as part of https://github.com/openSUSE/agama/pull/1031 - describe.skip("and there is a selected device", () => { - beforeEach(() => { - props.settings = { bootDevice: "/dev/vda" }; - }); - - it("renders the selected device", () => { - plainRender(); - - screen.getByText(/Installation device/); - screen.getByText("/dev/vda"); - }); - }); - }); - - describe("if there is no selected device yet", () => { - beforeEach(() => { - props.settings = { bootDevice: "" }; - }); - - it("renders a message indicating that the device is not selected", () => { - plainRender(); - - screen.getByText(/Installation device/); - screen.getByText(/No device selected/); - }); - }); - - // FIXME: skipping this test example by now. - // It will be addressed when reworking the Device section - // as part of https://github.com/openSUSE/agama/pull/1031 - describe.skip("if there is a selected device", () => { - beforeEach(() => { - props.settings = { bootDevice: "/dev/vda" }; - }); - - it("renders the selected device", () => { - plainRender(); - - screen.getByText(/Installation device/); - screen.getByText("/dev/vda"); - }); - }); - - it("allows selecting a device when clicking on the device name", async () => { - props = { - availableDevices: [vda], - settings: { bootDevice: "/dev/vda" }, - onChange: jest.fn() - }; - - const { user } = plainRender(); - - const button = screen.getByRole("button", { name: "/dev/vda, 1 KiB" }); - await user.click(button); - - const popup = await screen.findByRole("dialog"); - within(popup).getByText("Installation device"); - - const accept = within(popup).getByRole("button", { name: "Accept" }); - await user.click(accept); - - expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); - expect(props.onChange).toHaveBeenCalled(); - }); - - it("allows canceling the selection of the device", async () => { - props = { - availableDevices: [vda], - settings: { bootDevice: "/dev/vda" }, - onChange: jest.fn() - }; - - const { user } = plainRender(); - - const button = screen.getByRole("button", { name: "/dev/vda, 1 KiB" }); - await user.click(button); - - const popup = await screen.findByRole("dialog"); - within(popup).getByText("Installation device"); - - const cancel = within(popup).getByRole("button", { name: "Cancel" }); - await user.click(cancel); - - expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); - expect(props.onChange).not.toHaveBeenCalled(); - }); -}); - describe("LVM field", () => { describe("if LVM setting is not set yet", () => { beforeEach(() => { diff --git a/web/src/components/storage/index.js b/web/src/components/storage/index.js index 14717598a8..5cccae8494 100644 --- a/web/src/components/storage/index.js +++ b/web/src/components/storage/index.js @@ -23,6 +23,7 @@ export { default as ProposalPage } from "./ProposalPage"; export { default as ProposalPageMenu } from "./ProposalPageMenu"; export { default as ProposalSettingsSection } from "./ProposalSettingsSection"; export { default as ProposalSpacePolicySection } from "./ProposalSpacePolicySection"; +export { default as ProposalDeviceSection } from "./ProposalDeviceSection"; export { default as ProposalActionsSection } from "./ProposalActionsSection"; export { default as ProposalVolumes } from "./ProposalVolumes"; export { default as DASDPage } from "./DASDPage";