[web] Storage: mount point selector shown only if use can change it

Problem : When editing a file system, the form shows a disabled selector for the mount point field. Using a disabled widget does not make sense in this case because the mount point will never be editable. The disabled widget should be replaced by plain text. Moreover, the selector currently uses a FormSelect component. It should be replaced by the Select component, similar to the file system type selector. Solution : - Updated the MountPointFormSelector to use Select component Instead of the previous FormSelect component. - The select component adds more customizability and some advanced features as well. - Updated the mountpoint field to show the mountpath when currentVolume is defined and prompts the user to choose the mountpoint from the list of volume templates in case the currentVolume is undefined. Testing : Tested manually Screenshots: Before: ![Screenshot 2024-01-09 232254](https://github.com/openSUSE/agama/assets/125825782/a68fb82c-977a-447d-a54a-f2b673eb7da1) ![Screenshot 2024-01-09 232710](https://github.com/openSUSE/agama/assets/125825782/a8960c3e-a437-4e03-946f-d8704da13a63) After: ![image](https://github.com/openSUSE/agama/assets/125825782/21680bfa-da97-4e93-a0b2-692fcf42f71b) ![image](https://github.com/openSUSE/agama/assets/125825782/17ed2255-1ec9-4267-a0bd-6ef531059b81) --------- Co-authored-by: David Díaz González <[email protected]>
agama-project · Jan 19, 2024 · ee1bae7 · ee1bae7
1 parent 6b34668
commit ee1bae7
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 38 deletions.
diff --git a/web/package/cockpit-agama.changes b/web/package/cockpit-agama.changes
@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Fri Jan 19 09:34:26 UTC 2024 -  Nagu <[email protected]>
+
+- Storage UI: show mount point selector only when the user can change it.
+  (gh#openSUSE/agama#1007)
+
 -------------------------------------------------------------------
 Thu Jan 18 08:33:52 UTC 2024 - Ancor Gonzalez Sosa <[email protected]>
 

diff --git a/web/src/components/storage/ProposalVolumes.test.jsx b/web/src/components/storage/ProposalVolumes.test.jsx
@@ -215,8 +215,6 @@ describe("if there are volumes", () => {
 
     const popup = await screen.findByRole("dialog");
     within(popup).getByText("Edit file system");
-    const mountPointSelector = within(popup).getByRole("combobox", { name: "Mount point" });
-    expect(mountPointSelector).toHaveAttribute("disabled");
   });
 
   describe("and there is transactional Btrfs volume", () => {

diff --git a/web/src/components/storage/VolumeForm.jsx b/web/src/components/storage/VolumeForm.jsx
@@ -71,15 +71,55 @@ const SizeUnitFormSelect = ({ units, ...formSelectProps }) => {
  * Based on {@link PF/FormSelect https://www.patternfly.org/components/forms/form-select}
  *
  * @param {object} props
+ * @param {string} props.value - mountPath of current selected volume
  * @param {Array<Volume>} props.volumes - a collection of storage volumes
- * @param {object} props.formSelectProps - @see {@link https://www.patternfly.org/components/forms/form-select#props}
+ * @param {onChangeFn} props.onChange - callback for notifying input changes
+ * @param {object} props.selectProps - other props sent to {@link https://www.patternfly.org/components/menus/select#props PF/Select}
  * @returns {ReactComponent}
- */
-const MountPointFormSelect = ({ volumes, ...formSelectProps }) => {
+*/
+
+const MountPointFormSelect = ({ value, volumes, onChange, ...selectProps }) => {
+  const [isOpen, setIsOpen] = useState(false);
+
+  const onSelect = (_, mountPath) => {
+    setIsOpen(false);
+    onChange(mountPath);
+  };
+
+  const onToggleClick = () => {
+    setIsOpen(!isOpen);
+  };
+
+  const toggle = toggleRef => {
+    return (
+      <MenuToggle
+       id="mountPoint"
+       ref={toggleRef}
+       onClick={onToggleClick}
+       isExpanded={isOpen}
+       className="full-width"
+      >
+        {value || _("Select a value")}
+      </MenuToggle>
+    );
+  };
   return (
-    <FormSelect { ...formSelectProps }>
-      { volumes.map(v => <FormSelectOption key={v.mountPath} value={v.mountPath} label={v.mountPath} />) }
-    </FormSelect>
+    <Select
+      onOpenChange={setIsOpen}
+      onSelect={onSelect}
+      isOpen={isOpen}
+      toggle={toggle}
+      shouldFocusToggleOnSelect
+      { ...selectProps }
+    >
+      <SelectList>
+        {volumes.map(v => (
+          <SelectOption isSelected={value === v.mountPath} key={v.mountPath} value={v.mountPath}>
+            {v.mountPath}
+          </SelectOption>
+        ))}
+      </SelectList>
+    </Select>
   );
 };
 
@@ -676,7 +716,7 @@ const reducer = (state, action) => {
 export default function VolumeForm({ id, volume: currentVolume, templates = [], onSubmit }) {
   const [state, dispatch] = useReducer(reducer, currentVolume || templates[0], createInitialState);
 
-  const changeVolume = (_, mountPath) => {
+  const changeVolume = (mountPath) => {
     const volume = templates.find(t => t.mountPath === mountPath);
     dispatch({ type: "CHANGE_VOLUME", payload: { volume } });
   };
@@ -722,17 +762,31 @@ export default function VolumeForm({ id, volume: currentVolume, templates = [],
 
   const { fsType, snapshots } = state.formData;
 
+  const ShowMountPointSelector = () => (
+    <MountPointFormSelect
+        value={state.formData.mountPoint}
+        onChange={changeVolume}
+        volumes={currentVolume ? [currentVolume] : templates}
+    />
+  );
+
+  const ShowMountPoint = () => <p>{state.formData.mountPoint}</p>;
+
   return (
     <Form id={id} onSubmit={submitForm}>
-      <FormGroup isRequired label={_("Mount point")} fieldId="mountPoint">
-        <MountPointFormSelect
-          id="mountPoint"
-          value={state.formData.mountPoint}
-          onChange={changeVolume}
-          volumes={currentVolume ? [currentVolume] : templates}
-          isDisabled={currentVolume !== undefined}
-        />
-      </FormGroup>
+      <If
+        condition={currentVolume !== undefined}
+        then={
+          <FormGroup label={_("Mount point")}>
+            <ShowMountPoint />
+          </FormGroup>
+        }
+        else={
+          <FormGroup isRequired label={_("Mount point")} fieldId="mountPoint">
+            <ShowMountPointSelector />
+          </FormGroup>
+        }
+      />
       <FsField
         value={{ fsType, snapshots }}
         volume={state.volume}

diff --git a/web/src/components/storage/VolumeForm.test.jsx b/web/src/components/storage/VolumeForm.test.jsx
@@ -94,12 +94,6 @@ beforeEach(() => {
   props = { templates: [volumes.root, volumes.home, volumes.swap] };
 });
 
-it("renders a control for displaying/selecting the mount point", () => {
-  plainRender(<VolumeForm {...props} />);
-
-  screen.getByRole("combobox", { name: "Mount point" });
-});
-
 it("renders a control for displaying/selecting the file system type", async () => {
   const { user } = plainRender(<VolumeForm {...props} />);
 
@@ -113,9 +107,10 @@ it("renders a control for displaying/selecting the file system type", async () =
 it("does not render the file system control if there is only one option", async () => {
   const { user } = plainRender(<VolumeForm {...props} />);
 
-  const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" });
-  const swap = within(mountPointSelector).getByRole("option", { name: "swap" });
-  await user.selectOptions(mountPointSelector, swap);
+  const mountPointButton = screen.getByRole("button", { name: "Mount point" });
+  await user.click(mountPointButton);
+  const swap = screen.getByRole("option", { name: "swap" });
+  await user.click(swap);
   await screen.findByText("Swap");
   await waitFor(() => (
     expect(screen.queryByRole("button", { name: "File system type" })).not.toBeInTheDocument())
@@ -151,12 +146,13 @@ it("renders the 'Auto' size option only when a volume with 'adaptive sizes' is s
   // that it's configured for allowing adaptive sizes too.
   screen.getByRole("radio", { name: "Auto" });
 
-  const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" });
-  const homeVolumeOption = screen.getByRole("option", { name: "/home" });
+  const mountPointButton = screen.getByRole("button", { name: "Mount point" });
+  await user.click(mountPointButton);
 
   // And we know that /home volume is not set to allow adaptive sizes. Thus,
   // let's select it.
-  await user.selectOptions(mountPointSelector, homeVolumeOption);
+  const homeVolumeOption = screen.getByRole("option", { name: "/home" });
+  await user.click(homeVolumeOption);
 
   const autoSizeOption = screen.queryByRole("radio", { name: "Auto" });
   expect(autoSizeOption).toBeNull();
@@ -296,19 +292,31 @@ describe("size validations", () => {
 describe("when editing a new volume", () => {
   beforeEach(() => { props.volume = volumes.root });
 
-  it("renders the mount point selector as disabled", () => {
+  it("renders the mount point", () => {
     plainRender(<VolumeForm {...props} />);
 
-    const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" });
-    expect(mountPointSelector).toBeDisabled();
+    screen.getByText("/");
   });
-});
 
-describe("when adding a new volume", () => {
-  it("renders the mount point selector as enabled", () => {
+  it("does not render a control for changing the mount point", async () => {
     plainRender(<VolumeForm {...props} />);
+    await waitFor(() => (
+      expect(screen.queryByRole("button", { name: "Mount point" })).not.toBeInTheDocument()
+    ));
+  });
+});
 
-    const mountPointSelector = screen.getByRole("combobox", { name: "Mount point" });
-    expect(mountPointSelector).toBeEnabled();
+describe("when adding a new volume", () => {
+  it("renders a control for setting the mount point", async () => {
+    const { user } = plainRender(<VolumeForm {...props} />);
+
+    let mountPointButton = screen.getByRole("button", { name: "Mount point" });
+    await user.click(mountPointButton);
+    screen.getByRole("option", { name: "/" });
+    screen.getByRole("option", { name: "swap" });
+    const homeMountPoint = screen.getByRole("option", { name: "/home" });
+    await user.click(homeMountPoint);
+    mountPointButton = screen.getByRole("button", { name: "Mount point" });
+    await within(mountPointButton).findByText("/home");
   });
 });