Merge pull request #392 from KKoukiou/show-part-labels

storage: reclaim dialog: show format type not device type

src/actions/storage-actions.js

@@ -69,6 +69,15 @@ export const getDevicesAction = () => {
                     const formatData = await getFormatData({ diskName: device });
                     devData.formatData = formatData;
 
+                    // FIXME: https://github.com/storaged-project/blivet/issues/1258
+                    // This is a workaround for the issue with missing partition label human readable strings in blivet
+                    const partName = await cockpit.script(`udevadm info --query=property --name=${devData.path.v} | grep PARTNAME= | cut -d= -f2`);
+                    const fsLabel = await cockpit.script(`udevadm info --query=property --name=${devData.path.v} | grep ID_FS_LABEL= | cut -d= -f2`);
+                    devData.misc = {
+                        fsLabel: fsLabel.trim(),
+                        partName: partName.trim(),
+                    };
+
                     deviceData[device] = devData;
                 } catch (error) {
                     if (error.name === "org.fedoraproject.Anaconda.Modules.Storage.UnknownDeviceError") {

src/components/storage/ReclaimSpaceModal.jsx

@@ -147,8 +147,8 @@ export const ReclaimSpaceModal = ({ isFormDisabled, onClose, onNext }) => {
                     <ListingTable
                       aria-label={_("Reclaim space")}
                       columns={[
-                          { props: { width: 20 }, title: _("Name") },
                           { props: { width: 20 }, title: _("Location") },
+                          { props: { width: 20 }, title: _("Name") },
                           { props: { width: 20 }, title: _("Type") },
                           { props: { width: 20 }, title: _("Space") },
                           { props: { width: 20 }, title: _("Actions") }
@@ -236,19 +236,15 @@ const isDeviceLocked = ({ device }) => {
 
 const getDeviceRow = (disk, devices, level = 0, unappliedActions, setUnappliedActions) => {
     const device = devices[disk];
-    const description = device.description.v ? cockpit.format("$0 ($1)", disk, device.description.v) : device.name.v;
     const isDisk = device["is-disk"].v;
-    const descriptionWithIcon = (
-        isDisk
-            ? (
-                <Flex spaceItems={{ default: "spaceItemsSm" }} alignItems={{ default: "alignItemsCenter" }} flexWrap={{ default: "nowrap" }}>
-                    <FlexItem><HddIcon /></FlexItem>
-                    <FlexItem>{description}</FlexItem>
-                </Flex>
-            )
-            : description
+    const isPartition = device.type.v === "partition";
+    const typeLabel = isPartition ? (device.misc.fsLabel || device.misc.partName) : "";
+    const diskDescription = (
+        <Flex spaceItems={{ default: "spaceItemsSm" }} alignItems={{ default: "alignItemsCenter" }} flexWrap={{ default: "nowrap" }}>
+            <FlexItem><HddIcon /></FlexItem>
+            <FlexItem>{cockpit.format("$0 ($1)", device.name.v, device.description.v)}</FlexItem>
+        </Flex>
     );
-    const location = device["is-disk"].v ? device.path.v : "";
     const classNames = [
         idPrefix + "-table-row",
         idPrefix + "-device-level-" + level,
@@ -264,10 +260,15 @@ const getDeviceRow = (disk, devices, level = 0, unappliedActions, setUnappliedAc
         }
     }
 
-    let deviceType = device.type.v;
+    let deviceType = isDisk ? device.type.v : device.formatData.type.v;
     if (isDeviceLocked({ device })) {
         deviceType = (
-            <Flex className={idPrefix + "-device-locked"} spaceItems={{ default: "spaceItemsSm" }} alignItems={{ default: "alignItemsCenter" }}>
+            <Flex
+              className={idPrefix + "-device-locked"}
+              spaceItems={{ default: "spaceItemsSm" }}
+              alignItems={{ default: "alignItemsCenter" }}
+              flexWrap={{ default: "nowrap" }}
+            >
                 <FlexItem>{deviceType}</FlexItem>
                 <FlexItem><LockIcon /></FlexItem>
             </Flex>
@@ -287,8 +288,8 @@ const getDeviceRow = (disk, devices, level = 0, unappliedActions, setUnappliedAc
     return [
         {
             columns: [
-                { title: descriptionWithIcon },
-                { title: location },
+                { props: { colSpan: isDisk ? 2 : 1 }, title: isDisk ? diskDescription : (isPartition ? device.name.v : "") },
+                ...!isDisk ? [{ title: isPartition ? typeLabel : device.name.v }] : [],
                 { title: deviceType },
                 { title: size },
                 { title: deviceActions }

src/components/storage/ReclaimSpaceModal.scss

@@ -8,21 +8,11 @@
     }
   }
 
-  tr.reclaim-space-modal-table-row.reclaim-space-modal-device-level-1 {
-    td:first-child {
-      padding-inline-start: var(--pf-v5-global--spacer--xl);
-    }
-  }
-
-  tr.reclaim-space-modal-table-row.reclaim-space-modal-device-level-2 {
-    td:first-child {
-      padding-inline-start: var(--pf-v5-global--spacer--2xl);
-    }
-  }
-
-  tr.reclaim-space-modal-table-row.reclaim-space-modal-device-level-3 {
-    td:first-child {
-      padding-inline-start: var(--pf-v5-global--spacer--3xl);
+  @for $i from 1 through 10 {
+    tr.reclaim-space-modal-table-row.reclaim-space-modal-device-level-#{$i} {
+      td:nth-child(2) {
+        padding-inline-start: calc(var(--pf-v5-c-table--cell--PaddingLeft) + #{$i - 1} * var(--pf-v5-global--spacer--md));
+      }
     }
   }
 

test/check-storage-reclaim

@@ -17,6 +17,7 @@
 
 from anacondalib import VirtInstallMachineCase
 from installer import Installer
+from operating_systems import WindowsOS
 from review import Review
 from storage import Storage
 from storagelib import StorageCase
@@ -27,7 +28,7 @@ from testlib import (
 
 
 @nondestructive
-class TestStorageUseFreeSpaceScenario(VirtInstallMachineCase, StorageCase):
+class TestReclaim(VirtInstallMachineCase, StorageCase):
     def setup_partitions(self, s, i):
         disk = "/dev/vda"
         btrfsname = "btrfstest"
@@ -113,11 +114,11 @@ class TestStorageUseFreeSpaceScenario(VirtInstallMachineCase, StorageCase):
         s.reclaim_check_available_space("1.03 MB")
 
         # Check that all partitions are present
-        s.reclaim_check_device_row("vda (0x", "/dev/vda", "disk", "16.1 GB")
-        s.reclaim_check_device_row("vda1", "", "partition", "1.05 MB")
-        s.reclaim_check_device_row("vda2", "", "partition", "4.29 GB")
-        s.reclaim_check_device_row("vda3", "", "partition", "5.37 GB")
-        s.reclaim_check_device_row("vda4", "", "partition", "6.44 GB")
+        s.reclaim_check_device_row("vda (0x", "", "disk", "16.1 GB")
+        s.reclaim_check_device_row("vda1", "", "biosboot", "1.05 MB")
+        s.reclaim_check_device_row("vda2", "", "ext4", "4.29 GB")
+        s.reclaim_check_device_row("vda3", "", "btrfs", "5.37 GB")
+        s.reclaim_check_device_row("vda4", "", "btrfs", "6.44 GB")
 
         # Check that deleting a disk will delete all contained partitions
         s.reclaim_remove_device("vda (0x")
@@ -236,13 +237,19 @@ class TestReclaimLUKS(VirtInstallMachineCase, StorageCase):
         s.set_partitioning("use-free-space")
         s.reclaim_set_checkbox(True)
         i.next(True)
-        s.reclaim_check_device_row("vda1", "", "partition", "4.29 GB", locked=True)
+
+        b.assert_pixels(
+            "#reclaim-space-modal",
+            "reclaim-space-modal-encrypted",
+        )
+
+        s.reclaim_check_device_row("vda1", "", "luks", "4.29 GB", locked=True)
         s.reclaim_check_action_button_present("vda1", "shrink", True, True)
         s.reclaim_check_action_button_present("vda1", "delete", True)
 
 
 @nondestructive
-class TestStorageUseFreeSpaceScenarioExistingSystemFedora(VirtInstallMachineCase, StorageCase):
+class TestReclaimExistingSystemFedora(VirtInstallMachineCase, StorageCase):
     disk_image = "fedora-rawhide"
 
     def testDeletePartition(self):
@@ -257,6 +264,13 @@ class TestStorageUseFreeSpaceScenarioExistingSystemFedora(VirtInstallMachineCase
         s.set_partitioning("use-free-space")
         s.reclaim_set_checkbox(True)
         i.next(True)
+
+        b.assert_pixels(
+            "#reclaim-space-modal",
+            "reclaim-space-modal-fedora",
+            ignore=["td[data-label=Space]"],
+        )
+
         s.reclaim_remove_device("vda4")
         s.reclaim_modal_submit()
 
@@ -265,5 +279,32 @@ class TestStorageUseFreeSpaceScenarioExistingSystemFedora(VirtInstallMachineCase
         r.check_some_erased_checkbox_label()
 
 
+@nondestructive
+class TestReclaimExistingSystemWindows(VirtInstallMachineCase):
+    disk_size = 20
+
+    def setUp(self):
+        super().setUp()
+        WindowsOS(machine=self.machine, browser=self.browser).partition_disk()
+
+    def testBasic(self):
+        b = self.browser
+        m = self.machine
+        i = Installer(b, m)
+        s = Storage(b, m)
+
+        i.open()
+        i.reach(i.steps.INSTALLATION_METHOD)
+        s.set_partitioning("use-free-space")
+        s.reclaim_set_checkbox(True)
+        i.next(True)
+
+        b.assert_pixels(
+            "#reclaim-space-modal",
+            "reclaim-space-modal-windows",
+            ignore=["td[data-label=Space]"],
+        )
+
+
 if __name__ == '__main__':
     test_main()

test/helpers/storage.py

@@ -363,10 +363,11 @@ class StorageReclaimDialog():
     def __init__(self, browser):
         self.browser = browser
 
-    def reclaim_check_device_row(self, name, location="", deviceType=None, space=None, locked=False):
+    def reclaim_check_device_row(self, location, name=None, deviceType=None, space=None, locked=False):
         self.browser.wait_visible(
-            f"#reclaim-space-modal-table td[data-label=Name]:contains({name}) + "
-            f"td[data-label=Location]:contains({location}) + "
+            "#reclaim-space-modal-table "
+            f"td[data-label=Location]:contains({location}) + " +
+            (f"td[data-label=Name]:contains({name}) + " if deviceType != "disk" else "") +
             f"td[data-label=Type]:contains({deviceType}) + "
             f"td[data-label=Space]:contains({space})"
         )