Merge pull request #321 from joseivanlopez/storage-proposal-ui

Storage proposal UI

service/lib/dinstaller/dbus/storage/proposal.rb

@@ -112,9 +112,7 @@ def lvm
         #
         # @return [String]
         def encryption_password
-          return "" unless backend.calculated_settings
-
-          backend.calculated_settings.encryption_password
+          backend.calculated_settings&.encryption_password || ""
         end
 
         # Volumes used as template for creating a new volume
@@ -172,7 +170,7 @@ def register_callbacks
         SETTINGS_CONVERSIONS = {
           "CandidateDevices"   => ["candidate_devices=", proc { |v| v }],
           "LVM"                => ["lvm=", proc { |v| v }],
-          "EncryptionPassword" => ["encryption_password=", proc { |v| v }],
+          "EncryptionPassword" => ["encryption_password=", proc { |v| v.empty? ? nil : v }],
           "Volumes"            => ["volumes=", proc { |v, o| o.send(:to_proposal_volumes, v) }]
         }.freeze
         private_constant :SETTINGS_CONVERSIONS

service/lib/dinstaller/storage/proposal.rb

@@ -255,17 +255,13 @@ def storage_manager
       def validate_proposal
         return if candidate_devices.empty? || !proposal.failed?
 
-        message = format(
-          "Could not create a storage proposal using %{devices}",
-          devices: candidate_devices.join(", ")
-        )
-        ValidationError.new(message)
+        ValidationError.new("Cannot accommodate the required file systems for installation")
       end
 
       def validate_available_devices
         return if available_devices.any?
 
-        ValidationError.new("Could not find a suitable device for installation")
+        ValidationError.new("There is no suitable device for installation")
       end
 
       def validate_candidate_devices

service/package/rubygem-d-installer.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Fri Dec  2 14:52:36 UTC 2022 - José Iván López González <[email protected]>
+
+- Improve messages of storage validation errors.
+- Do not encrypt devices when receiving an empty password from
+  D-Bus (gh#yast/d-installer#321).
+
 -------------------------------------------------------------------
 Thu Dec  1 16:22:58 UTC 2022 - Josef Reidinger <[email protected]>
 

service/test/dinstaller/storage/proposal_test.rb

@@ -371,7 +371,7 @@
       it "returns an error" do
         errors = subject.validate
         expect(errors.size).to eq(1)
-        expect(errors.first.message).to include("not find a suitable device")
+        expect(errors.first.message).to include("There is no suitable device")
       end
     end
 
@@ -381,7 +381,7 @@
       it "returns an error" do
         errors = subject.validate
         expect(errors.size).to eq(1)
-        expect(errors.first.message).to include("not create a storage proposal using /dev/sda")
+        expect(errors.first.message).to include("Cannot accommodate")
       end
     end
 

web/package/cockpit-d-installer.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Fri Dec  2 13:41:41 UTC 2022 - José Iván López González <[email protected]>
+
+- Add new UI for storage proposal offering LVM and encyption
+  options (gh#yast/d-installer#321).
+
 -------------------------------------------------------------------
 Fri Nov 18 16:27:26 UTC 2022 - Imobach Gonzalez Sosa <[email protected]>
 

web/src/app.scss

@@ -306,3 +306,9 @@ button.hidden-popover-button {
     }
   }
 }
+
+.volumes-list {
+  .pf-c-label {
+    margin-inline-end: 5px;
+  }
+}

web/src/client/storage.js

@@ -44,69 +44,117 @@ class StorageBaseClient {
   }
 
   /**
-   * Returns the actions for the current proposal
+   * Returns storage proposal values
    *
-   * @return {Promise<object[]>}
+   * @return {Promise<object>}
    */
-  async getStorageActions() {
+  async getProposal() {
     const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
-    return proxy.Actions.map(action => {
-      const { Text: { v: textVar }, Subvol: { v: subvolVar }, Delete: { v: deleteVar } } = action;
+
+    const volume = dbusVolume => {
+      const valueFrom = dbusValue => dbusValue?.v;
+
+      const valuesFrom = (dbusValues) => {
+        if (dbusValues === undefined) return [];
+        return dbusValues.v.map(valueFrom);
+      };
+
+      return {
+        mountPoint: valueFrom(dbusVolume.MountPoint),
+        optional: valueFrom(dbusVolume.Optional),
+        deviceType: valueFrom(dbusVolume.DeviceType),
+        encrypted: valueFrom(dbusVolume.Encrypted),
+        fsTypes: valuesFrom(dbusVolume.FsTypes),
+        fsType: valueFrom(dbusVolume.FsType),
+        minSize: valueFrom(dbusVolume.MinSize),
+        maxSize: valueFrom(dbusVolume.MaxSize),
+        fixedSizeLimits: valueFrom(dbusVolume.FixedSizeLimits),
+        adaptiveSizes: valueFrom(dbusVolume.AdaptiveSizes),
+        snapshots: valueFrom(dbusVolume.Snapshots),
+        snapshotsConfigurable: valueFrom(dbusVolume.SnapshotsConfigurable),
+        snapshotsAffectSizes: valueFrom(dbusVolume.SnapshotsAffectSizes),
+        sizeRelevantVolumes: valueFrom(dbusVolume.SizeRelevantVolumes)
+      };
+    };
+
+    const action = dbusAction => {
+      const { Text: { v: textVar }, Subvol: { v: subvolVar }, Delete: { v: deleteVar } } = dbusAction;
       return { text: textVar, subvol: subvolVar, delete: deleteVar };
-    });
+    };
+
+    return {
+      availableDevices: proxy.AvailableDevices.map(([id, label]) => ({ id, label })),
+      candidateDevices: proxy.CandidateDevices,
+      lvm: proxy.LVM,
+      encryptionPassword: proxy.EncryptionPassword,
+      volumes: proxy.Volumes.map(volume),
+      actions: proxy.Actions.map(action)
+    };
   }
 
   /**
-   * Returns storage proposal settings
+   * Calculates a new proposal
    *
-   * @return {Promise<object>}
+   * @todo Do not send undefined values
+   *
+   * @param {object} settings - proposal settings
+   * @param {?string[]} [settings.candidateDevices] - Devices to use for the proposal
+   * @param {?string} [settings.encryptionPassword] - Password for encrypting devices
+   * @param {?boolean} [settings.lvm] - Whether to calculate the proposal with LVM volumes
+   * @param {?object[]} [settings.volumes] - Volumes to create
+   * @return {Promise<number>} - 0 success, other for failure
    */
-  async getStorageProposal() {
+  async calculateProposal({ candidateDevices, encryptionPassword, lvm, volumes }) {
     const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
-    return {
-      availableDevices: proxy.AvailableDevices.map(([id, label]) => {
-        return { id, label };
-      }),
-      candidateDevices: proxy.CandidateDevices,
-      lvm: proxy.LVM
+
+    const dbusVolume = volume => {
+      return {
+        MountPoint: cockpit.variant("s", volume.mountPoint),
+        Encrypted: cockpit.variant("b", volume.encrypted),
+        FsType: cockpit.variant("s", volume.fsType),
+        MinSize: cockpit.variant("x", volume.minSize),
+        MaxSize: cockpit.variant("x", volume.maxSize),
+        FixedSizeLimits: cockpit.variant("b", volume.fixedSizeLimits),
+        Snapshots: cockpit.variant("b", volume.snapshots)
+      };
     };
-  }
 
-  async calculateStorageProposal({ candidateDevices }) {
-    const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
     return proxy.Calculate({
-      CandidateDevices: cockpit.variant("as", candidateDevices)
+      CandidateDevices: cockpit.variant("as", candidateDevices),
+      EncryptionPassword: cockpit.variant("s", encryptionPassword),
+      LVM: cockpit.variant("b", lvm),
+      Volumes: cockpit.variant("aa{sv}", volumes.map(dbusVolume))
     });
   }
 
   /**
-   * Registers a callback to run when properties in the Actions object change
+   * Registers a callback to run when properties in the Storage Proposal object change
    *
    * @param {function} handler - callback function
    */
-  onActionsChange(handler) {
+  onProposalChange(handler) {
     return this.client.onObjectChanged(STORAGE_PROPOSAL_PATH, STORAGE_PROPOSAL_IFACE, changes => {
-      const { Actions: actions } = changes;
-      if (actions !== undefined && Array.isArray(actions.v)) {
-        const newActions = actions.v.map(action => {
-          const { Text: textVar, Subvol: subvolVar, Delete: deleteVar } = action;
-          return { text: textVar.v, subvol: subvolVar.v, delete: deleteVar.v };
-        });
-        handler(newActions);
+      if (Array.isArray(changes.CandidateDevices.v)) {
+        // FIXME return the proposal object (see getProposal)
+        handler({ candidateDevices: changes.CandidateDevices.v });
       }
     });
   }
 
   /**
-   * Registers a callback to run when properties in the Storage Proposal object change
+   * Registers a callback to run when properties in the Actions object change
    *
    * @param {function} handler - callback function
    */
-  onStorageProposalChange(handler) {
+  onActionsChange(handler) {
     return this.client.onObjectChanged(STORAGE_PROPOSAL_PATH, STORAGE_PROPOSAL_IFACE, changes => {
-      if (Array.isArray(changes.CandidateDevices.v)) {
-        const [selected] = changes.CandidateDevices.v;
-        handler(selected);
+      const { Actions: actions } = changes;
+      if (actions !== undefined && Array.isArray(actions.v)) {
+        const newActions = actions.v.map(action => {
+          const { Text: textVar, Subvol: subvolVar, Delete: deleteVar } = action;
+          return { text: textVar.v, subvol: subvolVar.v, delete: deleteVar.v };
+        });
+        handler(newActions);
       }
     });
   }
@@ -116,7 +164,7 @@ class StorageBaseClient {
  * Allows interacting with the storage settings
  */
 class StorageClient extends WithValidation(
-  WithStatus(StorageBaseClient, STORAGE_PROPOSAL_PATH), STORAGE_PATH
+  WithStatus(StorageBaseClient, STORAGE_PATH), STORAGE_PATH
 ) {}
 
 export { StorageClient };

web/src/client/storage.test.js

@@ -34,6 +34,10 @@ const storageProposalProxy = {
   ],
   CandidateDevices: ["/dev/sda"],
   LVM: true,
+  Volumes: [
+    { MountPoint: { t: "s", v: "/test1" } },
+    { MountPoint: { t: "s", v: "/test2" } }
+  ],
   Actions: [
     {
       Text: { t: "s", v: "Mount /dev/sdb1 as root" },
@@ -49,25 +53,21 @@ beforeEach(() => {
   });
 });
 
-describe("#getStorageProposal", () => {
-  it("returns the storage proposal settings", async () => {
+describe("#getProposal", () => {
+  it("returns the storage proposal settings and actions", async () => {
     const client = new StorageClient(dbusClient);
-    const proposal = await client.getStorageProposal();
-    expect(proposal).toEqual({
-      availableDevices: [
-        { id: "/dev/sda", label: "/dev/sda, 950 GiB, Windows" },
-        { id: "/dev/sdb", label: "/dev/sdb, 500 GiB" }
-      ],
-      candidateDevices: ["/dev/sda"],
-      lvm: true
-    });
-  });
-});
+    const proposal = await client.getProposal();
+    expect(proposal.availableDevices).toEqual([
+      { id: "/dev/sda", label: "/dev/sda, 950 GiB, Windows" },
+      { id: "/dev/sdb", label: "/dev/sdb, 500 GiB" }
+    ]);
+    expect(proposal.candidateDevices).toEqual(["/dev/sda"]);
+    expect(proposal.lvm).toBeTruthy();
+    expect(proposal.actions).toEqual([
+      { text: "Mount /dev/sdb1 as root", subvol: false, delete: false }
+    ]);
 
-describe("#getStorageActions", () => {
-  it("returns the storage actions", async () => {
-    const client = new StorageClient(dbusClient);
-    const actions = await client.getStorageActions();
-    expect(actions).toEqual([{ text: "Mount /dev/sdb1 as root", subvol: false, delete: false }]);
+    expect(proposal.volumes[0].mountPoint).toEqual("/test1");
+    expect(proposal.volumes[1].mountPoint).toEqual("/test2");
   });
 });

web/src/components/core/PasswordAndConfirmationInput.jsx

@@ -26,7 +26,7 @@ import {
 } from "@patternfly/react-core";
 
 const PasswordAndConfirmationInput = ({ value, onChange, onValidation }) => {
-  const [confirmation, setConfirmation] = useState("");
+  const [confirmation, setConfirmation] = useState(value || "");
   const [error, setError] = useState("");
 
   const validate = (password, passwordConfirmation) => {

web/src/components/core/PasswordAndConfirmationInput.test.jsx

@@ -36,3 +36,15 @@ describe("when the passwords do not match", () => {
     await screen.findByText("Passwords do not match");
   });
 });
+
+it("uses the given password value for confirmation too", async () => {
+  const { user } = plainRender(
+    <PasswordAndConfirmationInput value={"12345"} />
+  );
+
+  const passwordInput = screen.getByLabelText("Password");
+  const confirmationInput = screen.getByLabelText("Password confirmation");
+
+  expect(passwordInput.value).toEqual("12345");
+  expect(passwordInput.value).toEqual(confirmationInput.value);
+});

web/src/components/core/Section.jsx

@@ -167,7 +167,7 @@ export default function Section({
               </Text>
             </TextContent>
           </StackItem> }
-          { errors &&
+          { errors?.length > 0 &&
           <StackItem>
             <ValidationErrors errors={errors} title={`${title} errors`} />
           </StackItem> }

web/src/components/core/Section.test.jsx

@@ -20,7 +20,7 @@
  */
 
 import React, { useState } from "react";
-import { act, screen } from "@testing-library/react";
+import { screen } from "@testing-library/react";
 import { installerRender } from "@/test-utils";
 import { Section } from "@components/core";
 
@@ -84,7 +84,8 @@ describe("Section", () => {
       expect(inputText).not.toBeInTheDocument();
 
       const actionIcon = screen.getByRole("figure", { name: "Awesome settings section action icon" });
-      await act(async () => user.click(actionIcon));
+
+      await user.click(actionIcon);
 
       inputText = screen.queryByRole("textbox", { name: "Awesome input" });
       expect(inputText).toBeInTheDocument();

web/src/components/core/index.js

@@ -28,7 +28,6 @@ export { default as InstallationProgress } from "./InstallationProgress";
 export { default as InstallButton } from "./InstallButton";
 export { default as InstallerSkeleton } from "./InstallerSkeleton";
 export { default as KebabMenu } from "./KebabMenu";
-export { default as Overview } from "./Overview";
 export { default as PasswordAndConfirmationInput } from "./PasswordAndConfirmationInput";
 export { default as Popup } from "./Popup";
 export { default as ProgressReport } from "./ProgressReport";