From a5eda625ab8ae2727d028bf6173c07bba6957af4 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Thu, 19 Sep 2024 08:58:10 +0100 Subject: [PATCH 01/10] Added confirmation dialog --- web/src/components/storage/dasd/DASDTable.tsx | 102 +++++++++++------- 1 file changed, 65 insertions(+), 37 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index 37ea85464..16b601401 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -27,6 +27,7 @@ import { DropdownItem, DropdownList, MenuToggle, + Text, TextInputGroup, TextInputGroupMain, TextInputGroupUtilities, @@ -35,6 +36,7 @@ import { ToolbarGroup, ToolbarItem, } from "@patternfly/react-core"; +import { Popup } from "~/components/core"; import { Table, Thead, Tr, Th, Tbody, Td } from "@patternfly/react-table"; import { Page } from "~/components/core"; import { Icon } from "~/components/layout"; @@ -55,7 +57,7 @@ const columnData = (device: DASDDevice, column: { id: string; sortId?: string; l if (!device.enabled) data = ""; break; case "partitionInfo": - data = data.split(",").map((d) =>
{d}
); + data = data.split(",").map((d: string) =>
{d}
); break; } @@ -79,12 +81,29 @@ const columns = [ { id: "partitionInfo", label: _("Partition Info") }, ]; +const ConfirmFormat = ({ devices, isOpen, toggle, action }) => { + return ( + + + {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} + + {devices.join(", ")} + + action()} /> + toggle()} /> + + + ); +}; + const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: boolean }) => { const { mutate: updateDASD } = useDASDMutation(); const { mutate: formatDASD } = useFormatDASDMutation(); const [isOpen, setIsOpen] = useState(false); + const [confirmFormat, setConfirmFormat] = useState(false); const onToggle = () => setIsOpen(!isOpen); + const onToggleConfirm = () => setConfirmFormat(!confirmFormat); const onSelect = () => setIsOpen(false); const deviceIds = devices.map((d) => d.id); @@ -99,7 +118,8 @@ const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: b return false; } - return formatDASD(devices.map((d) => d.id)); + formatDASD(devices.map((d) => d.id)); + onToggleConfirm(); }; const Action = ({ children, ...props }) => ( @@ -109,41 +129,49 @@ const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: b ); return ( - ( - - {/* TRANSLATORS: drop down menu label */} - {_("Perform an action")} - - )} - > - - {/** TRANSLATORS: drop down menu action, activate the device */} - - {_("Activate")} - - {/** TRANSLATORS: drop down menu action, deactivate the device */} - - {_("Deactivate")} - - - {/** TRANSLATORS: drop down menu action, enable DIAG access method */} - - {_("Set DIAG On")} - - {/** TRANSLATORS: drop down menu action, disable DIAG access method */} - - {_("Set DIAG Off")} - - - {/** TRANSLATORS: drop down menu action, format the disk */} - - {_("Format")} - - - + <> + + ( + + {/* TRANSLATORS: drop down menu label */} + {_("Perform an action")} + + )} + > + + {/** TRANSLATORS: drop down menu action, activate the device */} + + {_("Activate")} + + {/** TRANSLATORS: drop down menu action, deactivate the device */} + + {_("Deactivate")} + + + {/** TRANSLATORS: drop down menu action, enable DIAG access method */} + + {_("Set DIAG On")} + + {/** TRANSLATORS: drop down menu action, disable DIAG access method */} + + {_("Set DIAG Off")} + + + {/** TRANSLATORS: drop down menu action, format the disk */} + setConfirmFormat(true)}> + {_("Format")} + + + + ); }; From f47fa8a820bb2856a2f15c7e751502ccb481da49 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Thu, 19 Sep 2024 09:59:13 +0100 Subject: [PATCH 02/10] Warn the user about offline DASD devices --- web/src/components/storage/dasd/DASDTable.tsx | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index 16b601401..3bb1087ed 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -82,12 +82,30 @@ const columns = [ ]; const ConfirmFormat = ({ devices, isOpen, toggle, action }) => { + const offline = devices.filter((d: DASDDevice) => !d.enabled); + + if (offline.length > 0) { + return ( + + + {_( + "The DASD devices listed below are offline and cannot be formatted, please unselect or activate them in order to continue", + )} + + {offline.map((d: DASDDevice) => d.id).join(", ")} + + toggle()}>{_("Accept")} + + + ); + } + return ( {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} - {devices.join(", ")} + {devices.map((d: DASDDevice) => d.id).join(", ")} action()} /> toggle()} /> @@ -131,7 +149,7 @@ const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: b return ( <> Date: Thu, 19 Sep 2024 11:48:19 +0100 Subject: [PATCH 03/10] Added unit test for the format dialogs --- .../storage/dasd/DASDTable.test.tsx | 71 ++++++++++++++++++- web/src/components/storage/dasd/DASDTable.tsx | 6 +- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.test.tsx b/web/src/components/storage/dasd/DASDTable.test.tsx index ce7a1e1b9..a74b5a513 100644 --- a/web/src/components/storage/dasd/DASDTable.test.tsx +++ b/web/src/components/storage/dasd/DASDTable.test.tsx @@ -27,6 +27,7 @@ import DASDTable from "./DASDTable"; let mockDASDDevices: DASDDevice[] = []; +let consoleErrorSpy; jest.mock("~/queries/storage/dasd", () => ({ useDASDDevices: () => mockDASDDevices, useDASDMutation: () => jest.fn(), @@ -34,12 +35,32 @@ jest.mock("~/queries/storage/dasd", () => ({ })); describe("DASDTable", () => { + beforeAll(() => { + consoleErrorSpy = jest.spyOn(console, "error"); + consoleErrorSpy.mockImplementation(); + }); + + afterAll(() => { + consoleErrorSpy.mockRestore(); + }); describe("when there is some DASD devices available", () => { beforeEach(() => { mockDASDDevices = [ { - id: "0.0.0200", + id: "0.0.0160", enabled: false, + deviceName: "", + deviceType: "", + formatted: false, + diag: false, + status: "offline", + accessType: "", + partitionInfo: "", + hexId: 0x160, + }, + { + id: "0.0.0200", + enabled: true, deviceName: "dasda", deviceType: "eckd", formatted: false, @@ -53,8 +74,54 @@ describe("DASDTable", () => { }); it("renders those devices", () => { - installerRender(); + installerRender(); screen.getByText("active"); }); + + it("does not allow to perform any action if not selected any device", () => { + installerRender(); + const button = screen.getByRole("button", { name: "Perform an action" }); + expect(button).toHaveAttribute("disabled"); + }); + + describe("when there are some DASD selected", () => { + it("allows to perform a set of actions over them", async () => { + const { user } = installerRender(); + const selection = screen.getByRole("checkbox", { name: "Select row 0" }); + await user.click(selection); + const button = screen.getByRole("button", { name: "Perform an action" }); + expect(button).not.toHaveAttribute("disabled"); + await user.click(button); + screen.getByRole("menuitem", { name: "Format" }); + }); + + describe("and the user click on format", () => { + it("shows a confirmation dialog if all the devices are online", async () => { + const { user } = installerRender(); + const selection = screen.getByRole("checkbox", { name: "Select row 1" }); + await user.click(selection); + const button = screen.getByRole("button", { name: "Perform an action" }); + expect(button).not.toHaveAttribute("disabled"); + await user.click(button); + const format = screen.getByRole("menuitem", { name: "Format" }); + await user.click(format); + screen.getByRole("dialog", { name: "DASD format confirmation dialog" }); + }); + + it("shows a warning dialog if some device is offline", async () => { + const { user } = installerRender(); + let selection = screen.getByRole("checkbox", { name: "Select row 0" }); + await user.click(selection); + selection = screen.getByRole("checkbox", { name: "Select row 1" }); + await user.click(selection); + const button = screen.getByRole("button", { name: "Perform an action" }); + expect(button).not.toHaveAttribute("disabled"); + await user.click(button); + const format = screen.getByRole("menuitem", { name: "Format" }); + await user.click(format); + screen.getByRole("dialog", { name: "DASD format offline warning dialog" }); + }); + }); + }); }); }); diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index 3bb1087ed..b48e33eab 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -86,7 +86,7 @@ const ConfirmFormat = ({ devices, isOpen, toggle, action }) => { if (offline.length > 0) { return ( - + {_( "The DASD devices listed below are offline and cannot be formatted, please unselect or activate them in order to continue", @@ -101,7 +101,7 @@ const ConfirmFormat = ({ devices, isOpen, toggle, action }) => { } return ( - + {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} @@ -371,7 +371,7 @@ export default function DASDTable() { - + From 105ca44724e071e75c6a024fab6a37ec47b140f6 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Fri, 20 Sep 2024 10:04:31 +0100 Subject: [PATCH 04/10] Polish DASD format confirmation dialog based on CR --- .../storage/dasd/DASDTable.test.tsx | 13 +-- web/src/components/storage/dasd/DASDTable.tsx | 107 +++++++++--------- 2 files changed, 56 insertions(+), 64 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.test.tsx b/web/src/components/storage/dasd/DASDTable.test.tsx index a74b5a513..1a34dd360 100644 --- a/web/src/components/storage/dasd/DASDTable.test.tsx +++ b/web/src/components/storage/dasd/DASDTable.test.tsx @@ -27,7 +27,6 @@ import DASDTable from "./DASDTable"; let mockDASDDevices: DASDDevice[] = []; -let consoleErrorSpy; jest.mock("~/queries/storage/dasd", () => ({ useDASDDevices: () => mockDASDDevices, useDASDMutation: () => jest.fn(), @@ -35,14 +34,6 @@ jest.mock("~/queries/storage/dasd", () => ({ })); describe("DASDTable", () => { - beforeAll(() => { - consoleErrorSpy = jest.spyOn(console, "error"); - consoleErrorSpy.mockImplementation(); - }); - - afterAll(() => { - consoleErrorSpy.mockRestore(); - }); describe("when there is some DASD devices available", () => { beforeEach(() => { mockDASDDevices = [ @@ -105,7 +96,7 @@ describe("DASDTable", () => { await user.click(button); const format = screen.getByRole("menuitem", { name: "Format" }); await user.click(format); - screen.getByRole("dialog", { name: "DASD format confirmation dialog" }); + screen.getByRole("dialog", { name: "Format confirmation" }); }); it("shows a warning dialog if some device is offline", async () => { @@ -119,7 +110,7 @@ describe("DASDTable", () => { await user.click(button); const format = screen.getByRole("menuitem", { name: "Format" }); await user.click(format); - screen.getByRole("dialog", { name: "DASD format offline warning dialog" }); + screen.getByRole("dialog", { name: "Offline DASD devices" }); }); }); }); diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index b48e33eab..9c529510f 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -80,65 +80,58 @@ const columns = [ { id: "formatted", label: _("Formatted") }, { id: "partitionInfo", label: _("Partition Info") }, ]; - -const ConfirmFormat = ({ devices, isOpen, toggle, action }) => { - const offline = devices.filter((d: DASDDevice) => !d.enabled); - - if (offline.length > 0) { - return ( - - - {_( - "The DASD devices listed below are offline and cannot be formatted, please unselect or activate them in order to continue", - )} - - {offline.map((d: DASDDevice) => d.id).join(", ")} - - toggle()}>{_("Accept")} - - - ); - } - - return ( - - - {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} - - {devices.map((d: DASDDevice) => d.id).join(", ")} - - action()} /> - toggle()} /> - - - ); -}; +const DevicesList = ({ devices }) => ( +
    + {devices.map((d: DASDDevice) => ( +
  • {d.id}
    • + ))} +
+); +const FormatNotPossible = ({ devices, onAccept }) => ( + + + {_( + "The DASD devices listed below are offline and cannot be formatted, please unselect or activate them in order to continue", + )} + + + + {_("Accept")} + + +); + +const FormatConfirmation = ({ devices, onCancel, onConfirm }) => ( + + + {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} + + + + + + + +); const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: boolean }) => { const { mutate: updateDASD } = useDASDMutation(); const { mutate: formatDASD } = useFormatDASDMutation(); const [isOpen, setIsOpen] = useState(false); - const [confirmFormat, setConfirmFormat] = useState(false); + const [requestFormat, setRequestFormat] = useState(false); const onToggle = () => setIsOpen(!isOpen); - const onToggleConfirm = () => setConfirmFormat(!confirmFormat); const onSelect = () => setIsOpen(false); + const cancelFormatRequest = () => setRequestFormat(false); const deviceIds = devices.map((d) => d.id); + const offlineDevices = devices.filter((d) => !d.enabled); + const offlineDevicesSelected = offlineDevices.length > 0; const activate = () => updateDASD({ action: "enable", devices: deviceIds }); const deactivate = () => updateDASD({ action: "disable", devices: deviceIds }); const setDiagOn = () => updateDASD({ action: "diagOn", devices: deviceIds }); const setDiagOff = () => updateDASD({ action: "diagOff", devices: deviceIds }); - const format = () => { - const offline = devices.filter((d) => !d.enabled); - - if (offline.length > 0) { - return false; - } - - formatDASD(devices.map((d) => d.id)); - onToggleConfirm(); - }; + const format = () => formatDASD(devices.map((d) => d.id)); const Action = ({ children, ...props }) => ( @@ -148,12 +141,20 @@ const Actions = ({ devices, isDisabled }: { devices: DASDDevice[]; isDisabled: b return ( <> - + {requestFormat && offlineDevicesSelected && ( + + )} + + {requestFormat && !offlineDevicesSelected && ( + { + cancelFormatRequest(); + format(); + }} + /> + )} {/** TRANSLATORS: drop down menu action, format the disk */} - setConfirmFormat(true)}> + setRequestFormat(true)}> {_("Format")} @@ -371,7 +372,7 @@ export default function DASDTable() { - + From daaf25957f0c630596588fa0dc560716927cdb8e Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Fri, 20 Sep 2024 10:08:41 +0100 Subject: [PATCH 05/10] Added changelog --- web/package/agama-web-ui.changes | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index dd9c438e2..66d33c63b 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Fri Sep 20 09:04:56 UTC 2024 - Knut Anderssen + +- Added confirmation dialog when formatting DASD devices as it is + considered a dangerous action (gh#openSUSE/agama#1618). + ------------------------------------------------------------------- Tue Sep 17 21:19:45 UTC 2024 - Knut Anderssen From 129b8534791baffe13d6c65d58abedbe92a7c1f9 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Fri, 20 Sep 2024 10:11:50 +0100 Subject: [PATCH 06/10] Removed unnecesary aria-labels --- web/src/components/storage/dasd/DASDTable.test.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.test.tsx b/web/src/components/storage/dasd/DASDTable.test.tsx index 1a34dd360..b77285073 100644 --- a/web/src/components/storage/dasd/DASDTable.test.tsx +++ b/web/src/components/storage/dasd/DASDTable.test.tsx @@ -65,19 +65,19 @@ describe("DASDTable", () => { }); it("renders those devices", () => { - installerRender(); + installerRender(); screen.getByText("active"); }); it("does not allow to perform any action if not selected any device", () => { - installerRender(); + installerRender(); const button = screen.getByRole("button", { name: "Perform an action" }); expect(button).toHaveAttribute("disabled"); }); describe("when there are some DASD selected", () => { it("allows to perform a set of actions over them", async () => { - const { user } = installerRender(); + const { user } = installerRender(); const selection = screen.getByRole("checkbox", { name: "Select row 0" }); await user.click(selection); const button = screen.getByRole("button", { name: "Perform an action" }); @@ -88,7 +88,7 @@ describe("DASDTable", () => { describe("and the user click on format", () => { it("shows a confirmation dialog if all the devices are online", async () => { - const { user } = installerRender(); + const { user } = installerRender(); const selection = screen.getByRole("checkbox", { name: "Select row 1" }); await user.click(selection); const button = screen.getByRole("button", { name: "Perform an action" }); @@ -100,7 +100,7 @@ describe("DASDTable", () => { }); it("shows a warning dialog if some device is offline", async () => { - const { user } = installerRender(); + const { user } = installerRender(); let selection = screen.getByRole("checkbox", { name: "Select row 0" }); await user.click(selection); selection = screen.getByRole("checkbox", { name: "Select row 1" }); From 214019a47adaed349b7b7df30f5e599e44c16548 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Sun, 22 Sep 2024 22:33:45 +0100 Subject: [PATCH 07/10] Modify confirmation dialog texts --- web/src/components/storage/dasd/DASDTable.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index 9c529510f..ebdc42c7f 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -88,10 +88,10 @@ const DevicesList = ({ devices }) => ( ); const FormatNotPossible = ({ devices, onAccept }) => ( - + {_( - "The DASD devices listed below are offline and cannot be formatted, please unselect or activate them in order to continue", + "Offline devices must be activated before formatting them. Please, unselect or activate the devices listed below and try it again", )} @@ -102,9 +102,11 @@ const FormatNotPossible = ({ devices, onAccept }) => ( ); const FormatConfirmation = ({ devices, onCancel, onConfirm }) => ( - + - {_("The DASD devices listed below are going to be formatted, do you want to proceed?")} + {_( + "This action could destroy any data stored on the devices listed below. Please, confirm that you really want to continue.", + )} From 4a24a8b16826148ad55018ae962d8b13722591e0 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Mon, 23 Sep 2024 10:58:10 +0100 Subject: [PATCH 08/10] Fix test --- web/src/components/storage/dasd/DASDTable.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.test.tsx b/web/src/components/storage/dasd/DASDTable.test.tsx index 6f0b69767..78d765527 100644 --- a/web/src/components/storage/dasd/DASDTable.test.tsx +++ b/web/src/components/storage/dasd/DASDTable.test.tsx @@ -97,7 +97,7 @@ describe("DASDTable", () => { await user.click(button); const format = screen.getByRole("menuitem", { name: "Format" }); await user.click(format); - screen.getByRole("dialog", { name: "Format confirmation" }); + screen.getByRole("dialog", { name: "Format selected devices?" }); }); it("shows a warning dialog if some device is offline", async () => { @@ -111,7 +111,7 @@ describe("DASDTable", () => { await user.click(button); const format = screen.getByRole("menuitem", { name: "Format" }); await user.click(format); - screen.getByRole("dialog", { name: "Offline DASD devices" }); + screen.getByRole("dialog", { name: "Cannot format all selected devices" }); }); }); }); From 59569b08817e2c7f8476d023b68354ccd5c84421 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Mon, 23 Sep 2024 15:29:09 +0100 Subject: [PATCH 09/10] AutoFocus to the cancel action to avoid confirm by error --- web/src/components/storage/dasd/DASDTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index e2734ac02..7b455f03f 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -112,7 +112,7 @@ const FormatConfirmation = ({ devices, onCancel, onConfirm }) => ( - + ); From cfc80e9bb179f4c860a767ad6b571eea6b8bce03 Mon Sep 17 00:00:00 2001 From: Knut Anderssen Date: Mon, 23 Sep 2024 15:34:46 +0100 Subject: [PATCH 10/10] Some small UI improvements --- web/src/components/storage/dasd/DASDTable.tsx | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/web/src/components/storage/dasd/DASDTable.tsx b/web/src/components/storage/dasd/DASDTable.tsx index 7b455f03f..90983a273 100644 --- a/web/src/components/storage/dasd/DASDTable.tsx +++ b/web/src/components/storage/dasd/DASDTable.tsx @@ -27,7 +27,10 @@ import { Dropdown, DropdownItem, DropdownList, + List, + ListItem, MenuToggle, + Stack, Text, TextInputGroup, TextInputGroupMain, @@ -82,20 +85,22 @@ const columns = [ { id: "partitionInfo", label: _("Partition Info") }, ]; const DevicesList = ({ devices }) => ( -
    + {devices.map((d: DASDDevice) => ( -
  • {d.id}
    • + {d.id} ))} -
+ ); const FormatNotPossible = ({ devices, onAccept }) => ( - - {_( - "Offline devices must be activated before formatting them. Please, unselect or activate the devices listed below and try it again", - )} - - + + + {_( + "Offline devices must be activated before formatting them. Please, unselect or activate the devices listed below and try it again", + )} + + + {_("Accept")} @@ -104,12 +109,14 @@ const FormatNotPossible = ({ devices, onAccept }) => ( const FormatConfirmation = ({ devices, onCancel, onConfirm }) => ( - - {_( - "This action could destroy any data stored on the devices listed below. Please, confirm that you really want to continue.", - )} - - + + + {_( + "This action could destroy any data stored on the devices listed below. Please, confirm that you really want to continue.", + )} + + +