fix(web): storage UI fixes and improvements (#1351)

## Problem No long ago a [full UI rewrite](#1202) was started and it was recently merged into `master` branch despite not being fully finished. It was just in a good shape for adhering to the [release early, release often](https://en.wikipedia.org/wiki/Release_early,_release_often) philosophy. The idea is to add fixes and improvements little by little against `master` branch according to the (internally) assigned priorities. But the [openSUSE conference](https://events.opensuse.org/conferences/oSC24) is around the corner and there are few details we'd like to have more polished for the [Agama in action](https://events.opensuse.org/conferences/oSC24/program/proposals/4560) and [The future of Agama and openSUSE](https://events.opensuse.org/conferences/oSC24/program/proposals/4557) events. ## Solution Address some of these details as part of this PR. Go commit by commit to see them. ## Testing - Tested manually - Adapted / re-enabled some unit tests. ## Note for reviewers No changelog entry required. ## Screenshots <details> <summary>Click to show/hide</summary> --- * Transactional root file system alert | Before | After | |-|-| | ![Screenshot from 2024-06-17 13-59-15](https://github.com/openSUSE/agama/assets/1691872/0d69dc88-033e-4431-ad1e-ee66fde451e6) | ![Screenshot from 2024-06-17 13-58-36](https://github.com/openSUSE/agama/assets/1691872/e6f332a5-f2d4-4b40-af87-d05d07727f34) | |![Screenshot from 2024-06-17 13-59-19](https://github.com/openSUSE/agama/assets/1691872/fbd4dad7-d48f-44cf-adc6-c5cab5759d80) |![Screenshot from 2024-06-17 13-58-46](https://github.com/openSUSE/agama/assets/1691872/22fc0ca8-5f51-4207-8ca6-066aa4c2ee10) | * TPM reminder look&feel | Before | After | |-|-| | ![Screenshot from 2024-06-17 17-12-26](https://github.com/openSUSE/agama/assets/1691872/9f6ed1df-dbf0-4b85-ac55-e4aec5719448) | ![Screenshot from 2024-06-17 17-11-28](https://github.com/openSUSE/agama/assets/1691872/a27b8422-db1f-4e94-917d-38eb8cf398ce) | * `Accept` button when manually selecting the boot device | Before | After | |-|-| | ![Screenshot from 2024-06-17 22-14-04](https://github.com/openSUSE/agama/assets/1691872/5b08a4e9-c84b-461e-84da-6445d60e001e) | ![Screenshot from 2024-06-17 22-13-02](https://github.com/openSUSE/agama/assets/1691872/c19f42a4-3bfc-46a7-a569-5476255e299f) | * Size unit selector | Before | After | |-|-| | ![Screenshot from 2024-06-17 22-41-15](https://github.com/openSUSE/agama/assets/1691872/a9ec9a19-0ddd-449c-ace4-9734b3e202a6) | ![Screenshot from 2024-06-17 22-39-44](https://github.com/openSUSE/agama/assets/1691872/29f6e689-78f3-4dde-a550-682305fe4d59) | </details> --- Related to https://trello.com/c/H3oZyzXS (internal link)
openSUSE · Jun 19, 2024 · c516de3 · c516de3
2 parents 043f87d + e183ea8
commit c516de3
Show file tree

Hide file tree

Showing 17 changed files with 162 additions and 294 deletions.
diff --git a/products.d/tumbleweed.yaml b/products.d/tumbleweed.yaml
@@ -213,3 +213,4 @@ storage:
           - ext3
           - ext4
           - xfs
+          - vfat
diff --git a/rust/agama-lib/src/storage/model.rs b/rust/agama-lib/src/storage/model.rs
@@ -264,6 +264,7 @@ pub struct ProposalSettings {
     pub target_pv_devices: Option<Vec<String>>,
     pub configure_boot: bool,
     pub boot_device: String,
+    pub default_boot_device: String,
     pub encryption_password: String,
     pub encryption_method: String,
     #[serde(rename = "encryptionPBKDFunction")]
@@ -283,6 +284,7 @@ impl TryFrom<HashMap<String, OwnedValue>> for ProposalSettings {
             target_pv_devices: get_optional_property(&hash, "TargetPVDevices")?,
             configure_boot: get_property(&hash, "ConfigureBoot")?,
             boot_device: get_property(&hash, "BootDevice")?,
+            default_boot_device: get_property(&hash, "DefaultBootDevice")?,
             encryption_password: get_property(&hash, "EncryptionPassword")?,
             encryption_method: get_property(&hash, "EncryptionMethod")?,
             encryption_pbkd_function: get_property(&hash, "EncryptionPBKDFunction")?,
@@ -373,7 +375,7 @@ pub struct VolumeOutline {
     support_auto_size: bool,
     adjust_by_ram: bool,
     snapshots_configurable: bool,
-    snaphosts_affect_sizes: bool,
+    snapshots_affect_sizes: bool,
     size_relevant_volumes: Vec<String>,
 }
 
@@ -388,7 +390,7 @@ impl TryFrom<zbus::zvariant::Value<'_>> for VolumeOutline {
             support_auto_size: get_property(&mvalue, "SupportAutoSize")?,
             adjust_by_ram: get_property(&mvalue, "AdjustByRam")?,
             snapshots_configurable: get_property(&mvalue, "SnapshotsConfigurable")?,
-            snaphosts_affect_sizes: get_property(&mvalue, "SnapshotsAffectSizes")?,
+            snapshots_affect_sizes: get_property(&mvalue, "SnapshotsAffectSizes")?,
             size_relevant_volumes: get_property(&mvalue, "SizeRelevantVolumes")?,
         };
 

diff --git a/service/lib/agama/storage/volume_conversion/from_y2storage.rb b/service/lib/agama/storage/volume_conversion/from_y2storage.rb
@@ -48,14 +48,22 @@ def convert
         # @return [Agama::Storage::ProposalSettings]
         attr_reader :volume
 
+        # Recovers the range of sizes used by the Y2Storage proposal, if needed.
+        #
+        # If the volume is configured to use auto sizes, then the final range of sizes used by the
+        # Y2Storage proposal depends on the fallback sizes (if this volume is fallback for other
+        # volume) and the size for snapshots (if snapshots is active). The planned device contains
+        # the real range of sizes used by the Y2Storage proposal.
+        #
+        # FIXME: Recovering the sizes from the planned device is done to know the range of sizes
+        #   assigned to the volume and to present that information in the UI. But such information
+        #   should be provided in a different way, for example as part of the proposal result
+        #   reported on D-Bus: { success:, settings:, strategy:, computed_sizes: }.
+        #
         # @param target [Agama::Storage::Volume]
         def sizes_conversion(target)
-          # The final range of sizes used by the Y2Storage proposal depends on the fallback sizes
-          # (if this volume is fallback for other volume) and the size for snapshots (if snapshots
-          # is active). The planned device contains the real range of sizes used by the proposal.
-          #
-          # From Agama point of view, this is the way of recovering the range of sizes used by
-          # Y2Storage when a volume is set to have auto size.
+          return unless target.auto_size?
+
           planned = planned_device_for(target.mount_path)
           return unless planned
 

diff --git a/service/test/agama/storage/volume_conversion/from_y2storage_test.rb b/service/test/agama/storage/volume_conversion/from_y2storage_test.rb
@@ -89,35 +89,59 @@
         instance_double(Y2Storage::MinGuidedProposal, planned_devices: planned_devices)
       end
 
-      context "if there is a planned device for the volume" do
-        let(:planned_devices) { [planned_volume] }
+      let(:planned_devices) { [planned_volume] }
 
-        let(:planned_volume) do
-          Y2Storage::Planned::LvmLv.new("/").tap do |planned|
-            planned.min = Y2Storage::DiskSize.GiB(10)
-            planned.max = Y2Storage::DiskSize.GiB(40)
+      context "if the volume is configured with auto size" do
+        before do
+          volume.auto_size = true
+        end
+
+        context "if there is a planned device for the volume" do
+          let(:planned_volume) do
+            Y2Storage::Planned::LvmLv.new("/").tap do |planned|
+              planned.min = Y2Storage::DiskSize.GiB(10)
+              planned.max = Y2Storage::DiskSize.GiB(40)
+            end
+          end
+
+          it "sets the min and max sizes according to the planned device" do
+            result = subject.convert
+
+            expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(10))
+            expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(40))
           end
         end
 
-        it "sets the min and max sizes according to the planned device" do
-          result = subject.convert
+        context "if there is no planned device for the volume" do
+          let(:planned_volume) do
+            Y2Storage::Planned::LvmLv.new("/home").tap do |planned|
+              planned.min = Y2Storage::DiskSize.GiB(10)
+              planned.max = Y2Storage::DiskSize.GiB(40)
+            end
+          end
+
+          it "keeps the sizes of the given volume" do
+            result = subject.convert
 
-          expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(10))
-          expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(40))
+            expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(5))
+            expect(result.max_size).to eq(Y2Storage::DiskSize.GiB(20))
+          end
         end
       end
 
-      context "if there is no planned device for the volume" do
-        let(:planned_devices) { [planned_volume] }
+      context "if the volume is not configured with auto size" do
+        before do
+          volume.auto_size = false
+        end
 
         let(:planned_volume) do
-          Y2Storage::Planned::LvmLv.new("/home").tap do |planned|
+          Y2Storage::Planned::LvmLv.new("/").tap do |planned|
             planned.min = Y2Storage::DiskSize.GiB(10)
             planned.max = Y2Storage::DiskSize.GiB(40)
           end
         end
 
-        it "sets the sizes of the given volume" do
+        it "keeps the sizes of the given volume" do
           result = subject.convert
 
           expect(result.min_size).to eq(Y2Storage::DiskSize.GiB(5))

diff --git a/web/src/assets/styles/blocks.scss b/web/src/assets/styles/blocks.scss
@@ -550,6 +550,10 @@ table.proposal-result {
   input {
     text-align: end;
   }
+
+  select {
+    min-inline-size: fit-content;
+  }
 }
 
 [data-type="agama/options-picker"] {

diff --git a/web/src/components/core/InstallationFinished.jsx b/web/src/components/core/InstallationFinished.jsx
@@ -35,13 +35,14 @@ import { Center, Icon } from "~/components/layout";
 import { EncryptionMethods } from "~/client/storage";
 import { _ } from "~/i18n";
 import { useInstallerClient } from "~/context/installer";
+import alignmentStyles from '@patternfly/react-styles/css/utilities/Alignment/alignment';
 
 const TpmHint = () => {
   const [isExpanded, setIsExpanded] = useState(false);
   const title = _("TPM sealing requires the new system to be booted directly.");
 
   return (
-    <Alert isInline variant="info" className="tpm-hint" title={<strong>{title}</strong>}>
+    <Alert isInline className={alignmentStyles.textAlignLeft} title={<strong>{title}</strong>}>
       <Stack hasGutter>
         {_("If a local media was used to run this installer, remove it before the next boot.")}
         <ExpandableSection
@@ -74,12 +75,9 @@ function InstallationFinished() {
       const iguana = await client.manager.useIguana();
       // FIXME: This logic should likely not be placed here, it's too coupled to storage internals.
       // Something to fix when this whole page is refactored in a (hopefully near) future.
-      // const { settings: { encryptionPassword, encryptionMethod } } = await client.storage.proposal.getResult();
-      // TODO: The storage client is not adapted to the HTTP API yet.
-      const encryptionPassword = null;
-      const encryptionMethod = null;
+      const { settings: { encryptionPassword, encryptionMethod } } = await client.storage.proposal.getResult();
       setUsingIguana(iguana);
-      setUsingTpm(encryptionPassword?.length && encryptionMethod === EncryptionMethods.TPM);
+      setUsingTpm(encryptionPassword?.length > 0 && encryptionMethod === EncryptionMethods.TPM);
     }
 
     // TODO: display the page in a loading mode while needed data is being fetched.
@@ -107,7 +105,7 @@ function InstallationFinished() {
                           ? _("At this point you can power off the machine.")
                           : _("At this point you can reboot the machine to log in to the new system.")}
                       </Text>
-                      {!usingTpm && <TpmHint />}
+                      {usingTpm && <TpmHint />}
                     </EmptyStateBody>
                   </EmptyState>
                   <Flex direction={{ default: "rowReverse" }}>

diff --git a/web/src/components/core/InstallationFinished.test.jsx b/web/src/components/core/InstallationFinished.test.jsx
@@ -34,7 +34,7 @@ const finishInstallationFn = jest.fn();
 let encryptionPassword;
 let encryptionMethod;
 
-describe.skip("InstallationFinished", () => {
+describe("InstallationFinished", () => {
   beforeEach(() => {
     encryptionPassword = "n0tS3cr3t";
     encryptionMethod = EncryptionMethods.LUKS2;
@@ -72,7 +72,7 @@ describe.skip("InstallationFinished", () => {
     expect(finishInstallationFn).toHaveBeenCalled();
   });
 
-  describe.skip("when TPM is set as encryption method", () => {
+  describe("when TPM is set as encryption method", () => {
     beforeEach(() => {
       encryptionMethod = EncryptionMethods.TPM;
     });
@@ -90,27 +90,15 @@ describe.skip("InstallationFinished", () => {
       });
 
       it("does not show the TPM reminder", async () => {
-        const { user } = installerRender(<InstallationFinished />);
-        // Forcing the test to slow down a bit with a fake user interaction
-        // because actually the reminder will be not rendered immediately
-        // making the queryAllByText to produce a false positive if triggered
-        // too early here.
-        const congratsText = screen.getByText("Congratulations!");
-        await user.click(congratsText);
-        expect(screen.queryAllByText(/TPM/)).toHaveLength(0);
+        installerRender(<InstallationFinished />);
+        screen.queryAllByText(/TPM/);
       });
     });
   });
 
   describe("when TPM is not set as encryption method", () => {
     it("does not show the TPM reminder", async () => {
-      const { user } = installerRender(<InstallationFinished />);
-      // Forcing the test to slow down a bit with a fake user interaction
-      // because actually the reminder will be not rendered immediately
-      // making the queryAllByText to produce a false positive if triggered
-      // too early here.
-      const congratsText = screen.getByText("Congratulations!");
-      await user.click(congratsText);
+      installerRender(<InstallationFinished />);
       expect(screen.queryAllByText(/TPM/)).toHaveLength(0);
     });
   });

diff --git a/web/src/components/core/ProgressReport.jsx b/web/src/components/core/ProgressReport.jsx
@@ -20,11 +20,10 @@
  */
 
 import React, { useState, useEffect } from "react";
+import { Flex, Progress, Spinner, Text } from "@patternfly/react-core";
 import { useCancellablePromise } from "~/utils";
 import { useInstallerClient } from "~/context/installer";
 
-import { Grid, GridItem, Progress, Text } from "@patternfly/react-core";
-
 const ProgressReport = () => {
   const client = useInstallerClient();
   const { cancellablePromise } = useCancellablePromise();
@@ -54,34 +53,45 @@ const ProgressReport = () => {
     });
   }, [client.software]);
 
-  if (!progress.steps) return <Text>Waiting for progress status...</Text>;
+  if (!progress.steps) {
+    return (
+      <Flex
+        direction={{ default: "column" }}
+        rowGap={{ default: "rowGapXl" }}
+        alignItems={{ default: "alignItemsCenter" }}
+        justifyContent={{ default: "justifyContentCenter" }}
+      >
+        <Spinner />
+        <Text component="h1">Waiting for progress status...</Text>
+      </Flex>
+    );
+  }
 
   return (
-    <Grid hasGutter>
-      <GridItem sm={12}>
-        <Progress
-          min={0}
-          max={progress.steps}
-          value={progress.step}
-          title={progress.message}
-          label={" "}
-          aria-label={progress.message}
-        />
+    <Flex
+      direction={{ default: "column" }}
+      rowGap={{ default: "rowGapMd" }}
+    >
+      <Progress
+        min={0}
+        max={progress.steps}
+        value={progress.step}
+        title={progress.message}
+        measureLocation="none"
+      />
 
+      {
+        subProgress &&
         <Progress
-          size="sm"
           min={0}
-          max={subProgress?.steps}
-          value={subProgress?.step}
-          title={subProgress?.message}
-          label={" "}
+          max={subProgress.steps}
+          value={subProgress.step}
+          title={subProgress.message}
           measureLocation="none"
-          className={!subProgress && 'hidden'}
-          aria-label={subProgress?.message || " "}
-          aria-hidden={!subProgress}
+          size="sm"
         />
-      </GridItem>
-    </Grid>
+      }
+    </Flex>
   );
 };