Issue#932

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]>
 

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", () => {

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}

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");
   });
 });