Merge pull request #440 from KKoukiou/disk-selection-redesign

storage: re-design the disk selection component

src/components/storage/InstallationDestination.scss

@@ -8,7 +8,23 @@
  }
 }
 
-.installation-method-target-disk-size {
+.installation-method-target-disk-size,
+.installation-method-target-disk-existing-os {
  color: var(--pf-v5-global--Color--200);
  font-size: var(--pf-v5-global--FontSize--sm);
 }
+
+// Add some border to the disk selection menu
+.installation-method-disk-selection-stack {
+ border: var(--pf-v5-global--BorderWidth--sm) solid var(--pf-v5-global--BorderColor--100);
+
+ .installation-method-disk-selection-rescan {
+ margin-top: var(--pf-v5-global--spacer--md);
+ margin-bottom: var(--pf-v5-global--spacer--md);
+ align-self: center;
+ }
+
+ .pf-v5-c-empty-state__body {
+ margin-block-start: 0;
+ }
+}

src/components/storage/InstallationScenario.jsx

@@ -21,6 +21,7 @@ import React, { useContext, useEffect, useState } from "react";
 import {
  Checkbox,
  FormGroup,
+ FormSection,
  Radio,
  Title,
 } from "@patternfly/react-core";
@@ -403,8 +404,9 @@ export const InstallationScenario = ({
  const { storageScenarioId } = useContext(StorageContext);
 
  return (
- <>
- <Title headingLevel={headingLevel}>{_("How would you like to install?")}</Title>
+ <FormSection
+ title={<Title headingLevel={headingLevel}>{_("How would you like to install?")}</Title>}
+ >
  <FormGroup className={idPrefix + "-scenario-group"} isStack hasNoPaddingTop data-scenario={storageScenarioId}>
  <InstallationScenarioSelector
  dispatch={dispatch}
@@ -414,6 +416,6 @@ export const InstallationScenario = ({
  showStorage={showStorage}
  />
  </FormGroup>
- </>
+ </FormSection>
  );
 };

test/check-basic

@@ -226,7 +226,7 @@ class TestBasic(VirtInstallMachineCase):
  # to avoid crashing the test browser
  m.execute("journalctl --rotate; journalctl --vacuum-time=1s")
 
- s.rescan_disks()
+ s.rescan_disks(expect_failure=True)
 
  b.wait_visible("#critical-error-bz-report-modal.pf-v5-c-modal-box")
  b.wait_in_text("#critical-error-bz-report-modal header", "Rescanning of the disks failed")

test/check-language

@@ -74,7 +74,7 @@ class TestLanguage(VirtInstallMachineCase):
  # to avoid crashing the test browser
  m.execute("journalctl --rotate; journalctl --vacuum-time=1s")
 
- s.rescan_disks()
+ s.rescan_disks(expect_failure=True)
 
  b.wait_in_text(
  "#critical-error-bz-report-modal.pf-v5-c-modal-box",

test/check-storage-basic

@@ -40,8 +40,7 @@ class TestStorage(VirtInstallMachineCase, StorageHelpers):
  i.reach(i.steps.INSTALLATION_METHOD)
 
  # Check the auto-selected disk's details
- s.wait_no_disks_detected_not_present()
- s.check_single_disk_destination("vda", "16.1 GB")
+ s.check_disk_selected("vda", selected=True, size="16.1 GB")
 
  # Pixel test the storage step
  b.assert_pixels(
@@ -56,13 +55,7 @@ class TestStorage(VirtInstallMachineCase, StorageHelpers):
  # until the test clicks on the re-scan button
  dev = self.add_ram_disk(2)
  dev = dev.split("/")[-1]
- s.rescan_disks()
-
- s.check_disk_visible("vda")
- s.check_disk_visible(dev)
-
- # Check the newly added disk generated a notification
- s.wait_disk_added(dev)
+ s.rescan_disks(["vda", dev])
 
  # Check that the disk selection persists when moving next and back
  s.check_disk_selected("vda", True)
@@ -73,22 +66,18 @@ class TestStorage(VirtInstallMachineCase, StorageHelpers):
  s.check_disk_selected(dev, False)
 
  # Try unselecting the single disk and expect and error
- s.select_disk("vda", False)
+ s.select_disks([("vda", False), (dev, False)])
  s.wait_no_disks()
  # Check the next button is disabled if no disks are selected
  i.check_next_disabled()
 
  # Check that disk selection is kept on Next and Back
- s.select_disk(dev, True)
- s.select_disk("vda", True)
+ s.select_disks([(dev, True), ("vda", True)])
  i.next()
  i.back()
  for disk in ["vda", dev]:
  s.check_disk_selected(disk)
 
- # Check clear selection of disks
- s.select_none_disks_and_check([dev, "vda"])
-
  def testScenarioSelection(self):
  """
  Test that the use-free-space scenario is conditionally available

test/check-storage-cockpit

@@ -37,7 +37,7 @@ class TestStorageCockpitIntegration(VirtInstallMachineCase, StorageCase):
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
  s.wait_scenario_visible("use-configured-storage", False)
- s.check_single_disk_destination("vda")
+ s.check_disk_selected("vda")
  s.modify_storage()
  s.confirm_entering_cockpit_storage()
  b.wait_visible(".cockpit-storage-integration-sidebar")
@@ -97,7 +97,7 @@ class TestStorageCockpitIntegration(VirtInstallMachineCase, StorageCase):
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
  s.wait_scenario_visible("use-configured-storage", False)
- s.check_single_disk_destination("vda")
+ s.check_disk_selected("vda")
  s.modify_storage()
  s.confirm_entering_cockpit_storage()
  b.wait_visible(".cockpit-storage-integration-sidebar")
@@ -185,7 +185,7 @@ class TestStorageCockpitIntegration(VirtInstallMachineCase, StorageCase):
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
  s.wait_scenario_visible("use-configured-storage", False)
- s.check_single_disk_destination("vda")
+ s.check_disk_selected("vda")
  s.modify_storage()
  s.confirm_entering_cockpit_storage()
  b.wait_visible(".cockpit-storage-integration-sidebar")
@@ -259,7 +259,7 @@ class TestStorageCockpitIntegration(VirtInstallMachineCase, StorageCase):
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
  s.wait_scenario_visible("use-configured-storage", False)
- s.check_single_disk_destination("vda")
+ s.check_disk_selected("vda")
  s.modify_storage()
  s.confirm_entering_cockpit_storage()
  b.wait_visible(".cockpit-storage-integration-sidebar")

test/check-storage-erase-all

@@ -141,10 +141,10 @@ class TestFedoraPlansEraseAll(VirtInstallMachineCase, StorageHelpers):
 
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
- s.rescan_disks()
+ s.rescan_disks(["vda", "vdb"])
 
  s.check_disk_selected("vda", True)
- s.select_disk("vdb", True)
+ s.select_disks([("vdb", True), ("vda", True)])
 
  i.reach(i.steps.REVIEW)
  r.check_checkbox_not_present()

test/check-storage-mountpoints

@@ -178,12 +178,10 @@ class TestStorageMountPoints(VirtInstallMachineCase, StorageCase):
 
  # Select only vdb disk and verify that mount point assignment is
  # not available (missing biosboot)
- s.select_none_disks_and_check([dev1, dev2])
  s.select_disks([(dev1, False), (dev2, True)])
  s.wait_scenario_available("mount-point-mapping", False)
 
  # Select only vda disk and verify that the partitioning request is correct
- s.select_none_disks_and_check([dev1, dev2])
  s.select_mountpoint([(dev1, True), (dev2, False)])
 
  s.check_mountpoint_row_device_available(1, "vdb1", False)
@@ -192,7 +190,6 @@ class TestStorageMountPoints(VirtInstallMachineCase, StorageCase):
  # Go back and change the disk selection. The partitioning should be re-created
  i.back()
 
- s.select_none_disks_and_check([dev1, dev2])
  s.select_mountpoint([(dev1, True), (dev2, True)])
 
  s.check_mountpoint_row_device_available(1, "vda2", True)
@@ -315,7 +312,7 @@ class TestStorageMountPoints(VirtInstallMachineCase, StorageCase):
 
  i.open()
  i.reach(i.steps.INSTALLATION_METHOD)
- s.check_single_disk_destination("vda")
+ s.check_disk_selected("vda")
 
  s.modify_storage()
  s.confirm_entering_cockpit_storage()

test/helpers/storage.py

@@ -39,67 +39,37 @@ def __init__(self, browser, machine):
  self.browser = browser
  self.machine = machine
 
- @log_step()
- def select_disk(self, disk, selected=True, is_single_disk=False):
- if not self.browser.is_present(f".pf-v5-c-menu[aria-labelledby='{id_prefix}-disk-selector-title']"):
- self.browser.click(f"#{id_prefix}-disk-selector-toggle > button")
-
- if selected:
- self.browser.click(f"#{id_prefix}-disk-selector-option-{disk}:not(.pf-m-selected)")
- else:
- self.browser.click(f"#{id_prefix}-disk-selector-option-{disk}.pf-m-selected")
-
- if is_single_disk:
- self.check_single_disk_destination(disk)
- else:
- self.check_disk_selected(disk, selected)
-
- @log_step()
- def select_none_disks_and_check(self, disks):
- self.browser.click(f"#{id_prefix}-disk-selector-clear")
- for disk in disks:
- self.check_disk_selected(disk, False)
-
- def check_single_disk_destination(self, disk, capacity=None):
- self.browser.wait_in_text(f"#{id_prefix}-target-disk", disk)
- if capacity:
- self.browser.wait_in_text(f"#{id_prefix}-target-disk", capacity)
-
  @log_step(snapshot_before=True)
- def check_disk_selected(self, disk, selected=True):
+ def check_disk_selected(self, disk, selected=True, size=None):
  if selected:
- self.browser.wait_visible(f"#{id_prefix}-selector-form li.pf-v5-c-chip-group__list-item:contains('{disk}')")
+ self.browser.wait_visible(f"#{id_prefix}-target-disk-{disk}")
  else:
- self.browser.wait_not_present(f"#{id_prefix}-selector-form li.pf-v5-c-chip-group__list-item:contains({disk})")
+ self.browser.wait_not_present(f"#{id_prefix}-target-disk-{disk}")
 
- def get_disk_selected(self, disk):
- return (
- self.browser.is_present(f"#{id_prefix}-selector-form li.pf-v5-c-chip-group__list-item:contains({disk})") or
- (self.browser.is_present(f"#{id_prefix}-target-disk") and
- disk in self.browser.text(f"#{id_prefix}-target-disk"))
- )
+ if size is not None:
+ self.browser.wait_in_text(f"#{id_prefix}-target-disk-{disk}", size)
 
  @log_step()
  def wait_no_disks(self):
  self.browser.wait_in_text("#next-helper-text",
  "To continue, select the devices to install to.")
 
- @log_step()
- def wait_no_disks_detected_not_present(self):
- self.browser.wait_not_present("#no-disks-detected-alert")
-
- def wait_disk_added(self, disk):
- self.browser.wait_in_text("#disks-changed-alert", f"The following disk was detected: {disk}")
-
  @log_step(snapshots=True)
- def rescan_disks(self):
+ def rescan_disks(self, expected_disks=None, expect_failure=False):
  b = self.browser
+ b.click(f"#{self._step}-change-destination-button")
  b.click(f"#{self._step}-rescan-disks")
  b.wait_visible(f"#{self._step}-rescan-disks.pf-m-disabled")
  # Default 15 seconds is not always enough for re-scanning disks
  with b.wait_timeout(30):
  b.wait_not_present(f"#{self._step}-rescan-disks.pf-m-disabled")
 
+ for disk in expected_disks or []:
+ b.wait_visible(f"#{self._step}-disk-selection-menu-item-{disk}")
+
+ if not expect_failure:
+ b.click(f"#{self._step}-change-destination-modal button:contains('Cancel')")
+
  def check_constraint(self, constraint, required=True):
  if required:
  self.browser.wait_visible(f"ul.cockpit-storage-integration-requirements-hint-list:first-of-type li:contains('{constraint}')")
@@ -119,19 +89,6 @@ def return_to_installation_confirm(self):
  def modify_storage(self):
  self.browser.click(f"#{self._step}-modify-storage")
 
- @log_step(snapshot_before=True)
- def check_disk_visible(self, disk, visible=True):
- if not self.browser.is_present(f".pf-v5-c-menu[aria-labelledby='{id_prefix}-disk-selector-title']"):
- self.browser.click(f"#{id_prefix}-disk-selector-toggle > button")
-
- if visible:
- self.browser.wait_visible(f"#{id_prefix}-disk-selector-option-{disk}")
- else:
- self.browser.wait_not_present(f"#{id_prefix}-disk-selector-option-{disk}")
-
- self.browser.click(f"#{id_prefix}-disk-selector-toggle > button")
- self.browser.wait_not_present(f".pf-v5-c-menu[aria-labelledby='{id_prefix}-disk-selector-title']")
-
 
 class StorageEncryption():
  encryption_id_prefix = "disk-encryption"
@@ -492,10 +449,14 @@ def check_mountpoint_row(self, row, mountpoint=None, device=None, reformat=None,
  def select_disks(self, disks):
  self.browser.wait(lambda: self.disks_loaded(disks))
 
- for disk in disks:
- current_selection = self.get_disk_selected(disk[0])
- if current_selection != disk[1]:
- self.select_disk(disk[0], disk[1], len(disks) == 1)
+ self.browser.click(f"#{id_prefix}-change-destination-button")
+
+ for (disk, selected) in disks:
+ self.browser.set_checked(f"#{id_prefix}-disk-selection-menu-item-{disk} input[type=checkbox]", selected)
+
+ self.browser.click(f"#{id_prefix}-change-destination-modal button:contains('Select')")
+ for (disk, selected) in disks:
+ self.check_disk_selected(disk, selected)
 
  def select_mountpoint(self, disks, encrypted=False):
  self.select_disks(disks)