From 63fa26a221c1b1c388d5a31e45e0fa02e4d6536a Mon Sep 17 00:00:00 2001 From: Katerina Koukiou Date: Wed, 2 Aug 2023 19:43:56 +0200 Subject: [PATCH] webui: fix logic for when to re-create the partitioning Now partitioning is marked for re-creation when device selection changes and when the user re-scans the device tree. The latter invalidates the partitioning indirectly, by re-setting the selected devices. This commit also simplifies the partitioning object update in state, as a clean state update was needed to make the above feature work as expected. --- ui/webui/src/actions/storage-actions.js | 4 +-- ui/webui/src/apis/storage.js | 2 +- ui/webui/src/components/AnacondaWizard.jsx | 18 ++++++++++-- .../components/storage/EncryptedDevices.jsx | 4 --- .../components/storage/MountPointMapping.jsx | 22 ++++++-------- ui/webui/src/reducer.js | 2 +- ui/webui/test/check-storage | 29 +++++++++++++++---- ui/webui/test/helpers/installer.py | 8 +++-- 8 files changed, 57 insertions(+), 32 deletions(-) diff --git a/ui/webui/src/actions/storage-actions.js b/ui/webui/src/actions/storage-actions.js index fb3b89caaf4..c77a7a4d2e1 100644 --- a/ui/webui/src/actions/storage-actions.js +++ b/ui/webui/src/actions/storage-actions.js @@ -85,12 +85,12 @@ export const getDiskSelectionAction = () => { }; }; -export const getPartitioningDataAction = ({ requests, partitioning, updateOnly }) => { +export const getPartitioningDataAction = ({ requests, partitioning }) => { return async (dispatch) => { const props = { path: partitioning }; const convertRequests = reqs => reqs.map(request => Object.entries(request).reduce((acc, [key, value]) => ({ ...acc, [key]: value.v }), {})); - if (!updateOnly) { + if (!requests) { props.method = await getPartitioningMethod({ partitioning }); if (props.method === "MANUAL") { const reqs = await gatherRequests({ partitioning }); diff --git a/ui/webui/src/apis/storage.js b/ui/webui/src/apis/storage.js index d7c851b8fdd..3394d4aca97 100644 --- a/ui/webui/src/apis/storage.js +++ b/ui/webui/src/apis/storage.js @@ -488,7 +488,7 @@ export const startEventMonitorStorage = ({ dispatch }) => { if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage.DiskSelection") { dispatch(getDiskSelectionAction()); } else if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage.Partitioning.Manual" && Object.hasOwn(args[1], "Requests")) { - dispatch(getPartitioningDataAction({ requests: args[1].Requests.v, partitioning: path, updateOnly: true })); + dispatch(getPartitioningDataAction({ requests: args[1].Requests.v, partitioning: path })); } else if (args[0] === "org.fedoraproject.Anaconda.Modules.Storage" && Object.hasOwn(args[1], "CreatedPartitioning")) { const last = args[1].CreatedPartitioning.v.length - 1; dispatch(getPartitioningDataAction({ partitioning: args[1].CreatedPartitioning.v[last] })); diff --git a/ui/webui/src/components/AnacondaWizard.jsx b/ui/webui/src/components/AnacondaWizard.jsx index 54273147051..1637d91dbbb 100644 --- a/ui/webui/src/components/AnacondaWizard.jsx +++ b/ui/webui/src/components/AnacondaWizard.jsx @@ -15,7 +15,7 @@ * along with This program; If not, see . */ import cockpit from "cockpit"; -import React, { useState, useMemo } from "react"; +import React, { useEffect, useState, useMemo } from "react"; import { ActionList, @@ -56,6 +56,20 @@ export const AnacondaWizard = ({ dispatch, isBootIso, osRelease, storageData, lo const [isInProgress, setIsInProgress] = useState(false); const [storageEncryption, setStorageEncryption] = useState(getStorageEncryptionState()); const [storageScenarioId, setStorageScenarioId] = useState(window.sessionStorage.getItem("storage-scenario-id") || getDefaultScenario().id); + const [reusePartitioning, setReusePartitioning] = useState(false); + + const availableDevices = useMemo(() => { + return Object.keys(storageData.devices); + }, [storageData.devices]); + + useEffect(() => { + /* + * When disk selection changes or the user re-scans the devices we need to re-create the partitioning. + * For Automatic partitioning we do it each time we go to review page, + * but for custom mount assignment we try to reuse the partitioning when possible. + */ + setReusePartitioning(false); + }, [availableDevices, storageData.diskSelection.selectedDisks]); const language = useMemo(() => { for (const l of Object.keys(localizationData.languages)) { @@ -86,7 +100,7 @@ export const AnacondaWizard = ({ dispatch, isBootIso, osRelease, storageData, lo label: _("Disk configuration"), steps: [{ component: MountPointMapping, - data: { deviceData: storageData.devices, diskSelection: storageData.diskSelection, partitioningData: storageData.partitioning, dispatch }, + data: { deviceData: storageData.devices, diskSelection: storageData.diskSelection, partitioningData: storageData.partitioning, dispatch, reusePartitioning, setReusePartitioning }, id: "mount-point-mapping", label: _("Manual disk configuration"), isHidden: storageScenarioId !== "mount-point-mapping" diff --git a/ui/webui/src/components/storage/EncryptedDevices.jsx b/ui/webui/src/components/storage/EncryptedDevices.jsx index 351d443f670..cbacce6514c 100644 --- a/ui/webui/src/components/storage/EncryptedDevices.jsx +++ b/ui/webui/src/components/storage/EncryptedDevices.jsx @@ -41,7 +41,6 @@ import { ModalError } from "cockpit-components-inline-notification.jsx"; import { getDevicesAction } from "../../actions/storage-actions.js"; import { - createPartitioning, unlockDevice, } from "../../apis/storage.js"; @@ -111,9 +110,6 @@ const UnlockDialog = ({ lockedLUKSDevices, onClose, dispatch }) => { dispatch(getDevicesAction()); if (res.every(r => r.value[0])) { - // Also refresh the partitioning data which will now show the children - // of the unlocked device. - createPartitioning({ method: "MANUAL" }); onClose(); } else { dialogErrorSet(_("Incorrect passphrase")); diff --git a/ui/webui/src/components/storage/MountPointMapping.jsx b/ui/webui/src/components/storage/MountPointMapping.jsx index 9510a7d527f..b007694ef9b 100644 --- a/ui/webui/src/components/storage/MountPointMapping.jsx +++ b/ui/webui/src/components/storage/MountPointMapping.jsx @@ -453,18 +453,11 @@ const MountPointMappingContent = ({ deviceData, partitioningData, dispatch, idPr } }; -export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, dispatch, idPrefix, setIsFormValid, onAddErrorNotification, stepNotification }) => { - const [creatingPartitioning, setCreatingPartitioning] = useState(true); - - // If device selection changed since the last partitioning request redo the partitioning - const selectedDevices = diskSelection.selectedDisks; - const partitioningDevices = partitioningData?.requests?.map(r => r["device-spec"]) || []; - const canReusePartitioning = selectedDevices.length === partitioningDevices.length && selectedDevices.every(d => partitioningDevices.includes(d)); +export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, dispatch, idPrefix, setIsFormValid, onAddErrorNotification, reusePartitioning, setReusePartitioning, stepNotification }) => { + const [usedPartitioning, setUsedPartitioning] = useState(partitioningData?.path); useEffect(() => { - if (canReusePartitioning) { - setCreatingPartitioning(false); - } else { + if (!reusePartitioning || partitioningData?.method !== "MANUAL") { /* Reset the bootloader drive before we schedule partitions * The bootloader drive is automatically set during the partitioning, so * make sure we always reset the previous value before we run another one, @@ -473,11 +466,14 @@ export const MountPointMapping = ({ deviceData, diskSelection, partitioningData, */ setBootloaderDrive({ drive: "" }) .then(() => createPartitioning({ method: "MANUAL" })) - .then(() => setCreatingPartitioning(false)); + .then(path => { + setUsedPartitioning(path[0]); + setReusePartitioning(true); + }); } - }, [canReusePartitioning]); + }, [reusePartitioning, setReusePartitioning, partitioningData?.method, partitioningData?.path]); - if (creatingPartitioning || !partitioningData?.path || (partitioningData?.requests?.length || 0) < 1) { + if (!reusePartitioning || usedPartitioning !== partitioningData.path) { return ; } diff --git a/ui/webui/src/reducer.js b/ui/webui/src/reducer.js index 5ed4bda0754..9e8aaba23a2 100644 --- a/ui/webui/src/reducer.js +++ b/ui/webui/src/reducer.js @@ -73,7 +73,7 @@ export const storageReducer = (state = storageInitialState, action) => { } else if (action.type === "GET_DISK_SELECTION") { return { ...state, diskSelection: action.payload.diskSelection }; } else if (action.type === "GET_PARTITIONING_DATA") { - return { ...state, partitioning: action.payload.partitioningData }; + return { ...state, partitioning: { ...state.partitioning, ...action.payload.partitioningData } }; } else { return state; } diff --git a/ui/webui/test/check-storage b/ui/webui/test/check-storage index 3d0ab7a5ef7..9068c1828fd 100755 --- a/ui/webui/test/check-storage +++ b/ui/webui/test/check-storage @@ -485,13 +485,30 @@ class TestStorageMountPoints(anacondalib.VirtInstallMachineCase): i.next() # verify review screen - disk = "vda" - r.check_disk(disk, "16.1 GB vda (0x1af4)") + r.check_disk(dev, "16.1 GB vda (0x1af4)") - r.check_disk_row(disk, 1, "vda1, 1.05 MB: biosboot") - r.check_disk_row(disk, 2, "vda2, 1.07 GB: format as ext4, /boot") - r.check_disk_row(disk, 3, "btrfstest, 10.7 GB: format as btrfs, /") - r.check_disk_row(disk, 4, "vda4, 4.29 GB: mount, /home") + r.check_disk_row(dev, 1, "vda1, 1.05 MB: biosboot") + r.check_disk_row(dev, 2, "vda2, 1.07 GB: format as ext4, /boot") + r.check_disk_row(dev, 3, "btrfstest, 10.7 GB: format as btrfs, /") + r.check_disk_row(dev, 4, "vda4, 4.29 GB: mount, /home") + + applied_partitioning = s.dbus_get_applied_partitioning() + + # When adding a new partition a new partitioning should be created + i.back(previous_page=i.steps.CUSTOM_MOUNT_POINT) + i.back() + + m.execute(f"sgdisk --new=0:0:0 {disk}") + s.rescan_disks() + self.select_mountpoint(i, s, [(dev, True)]) + self.check_row_device(1, "Select a device") + self.check_row_device(2, "Select a device") + self.select_row_device(1, f"{dev}2") + self.select_row_device(2, f"btrfs") + + i.next() + new_applied_partitioning = s.dbus_get_applied_partitioning() + self.assertNotEqual(new_applied_partitioning, applied_partitioning) @nondestructive def testNoRootMountPoint(self): diff --git a/ui/webui/test/helpers/installer.py b/ui/webui/test/helpers/installer.py index d0ab67d78cd..9041d4cbe67 100644 --- a/ui/webui/test/helpers/installer.py +++ b/ui/webui/test/helpers/installer.py @@ -102,7 +102,7 @@ def check_next_disabled(self, disabled=True): self.browser.wait_visible(f"#installation-next-btn:not([aria-disabled={value}]") @log_step(snapshot_before=True) - def back(self, should_fail=False): + def back(self, should_fail=False, previous_page=""): current_page = self.get_current_page() self.browser.click("button:contains(Back)") @@ -110,8 +110,10 @@ def back(self, should_fail=False): if should_fail: self.wait_current_page(current_page) else: - prev = [k for k, v in self.steps._steps_jump.items() if current_page in v][0] - self.wait_current_page(prev) + if not previous_page: + previous_page = [k for k, v in self.steps._steps_jump.items() if current_page in v][0] + + self.wait_current_page(previous_page) @log_step() def open(self, step="installation-language"):