Skip to content

Commit

Permalink
web: Add file systems for arbitrary mount points (#1154)
Browse files Browse the repository at this point in the history
The storage UI only allows adding file systems for the mount points
predefined by the product (typicaly */home*, *swap* or */var*). This PR
extends the storage UI making possible to add file systems for any mount
point.

Main changes:

* The *Add file system* button is now a menu button, which allows to
select either a predefined mount point (if the mount point was not added
yet) or an arbitrary one (the *Other* option).
* The *Add file system* button turns into a regular button if only the
*Other* option is available.
* The *Add file system* button is disabled if the product does not admit
arbitrary mount points (i.e., transactional system) and there is no
pending predefined mount point to be added (i.e., all the predefined
mount points were already added).
* The *Add file system* is hidden if the product does not admit optional
file systems (i.e., transactinal system without predefined mount
points).
* The file system form validates the mount point:
  * Validates mount point presence on accept.
  * Validates mount point format on accept.
* Validates whether the entered mount point is already added and offers
to edit such a file system.
* Validates whether the entered mount point maches any of the predefined
mount points and offers to add the predefined mount point.
 
Note:

This PR introduces a new validation pattern which was previoulsy
discussed and agreed. The form validates its fields on accept and the
field error is gone when the field value changes (independently on the
field is now valid or not). The rationale of this behavior is:

* The form should not complain on the fly (while typing) because the
user has not finished until clicking on accept.
* If the field shows an error, then remove the error with the first
change on its value. We can consider the user has started fixing the
field, but the form will not bother the user untill pressing on accept
again.

In the future we could improve these validations. For example, a wrong
field could show a "green check" if the user edit its value and the new
value is correct.

<details>
<summary>Screenshots</summary>

![localhost_8080_
(40)](https://github.com/openSUSE/agama/assets/1112304/b2dc3f2f-a732-479a-95b4-51850f95a95a)

![localhost_8080_
(43)](https://github.com/openSUSE/agama/assets/1112304/79fdfa61-12e0-44b7-b2c6-f1c43977f5e5)

![localhost_8080_
(42)](https://github.com/openSUSE/agama/assets/1112304/43b25082-4881-4a59-a856-e3b5a1e7fcc6)

</details>
  • Loading branch information
joseivanlopez authored Apr 25, 2024
2 parents ace7b96 + 1bbfada commit bd3018b
Show file tree
Hide file tree
Showing 25 changed files with 1,927 additions and 829 deletions.
5 changes: 5 additions & 0 deletions web/package/cockpit-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
-------------------------------------------------------------------
Thu Apr 25 15:04:05 UTC 2024 - José Iván López González <[email protected]>

- Allow adding arbitrary volumes (gh#openSUSE/agama#1154).

-------------------------------------------------------------------
Thu Apr 25 13:40:06 UTC 2024 - Ancor Gonzalez Sosa <[email protected]>

Expand Down
28 changes: 21 additions & 7 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ const ZFCP_DISK_IFACE = "org.opensuse.Agama.Storage1.ZFCP.Disk";
*
* @typedef {object} VolumeOutline
* @property {boolean} required
* @property {boolean} productDefined
* @property {string[]} fsTypes
* @property {boolean} adjustByRam
* @property {boolean} supportAutoSize
Expand Down Expand Up @@ -479,7 +480,8 @@ class ProposalManager {
async defaultVolume(mountPath) {
const proxy = await this.proxies.proposalCalculator;
const systemDevices = await this.system.getDevices();
return this.buildVolume(await proxy.DefaultVolume(mountPath), systemDevices);
const productMountPoints = await this.getProductMountPoints();
return this.buildVolume(await proxy.DefaultVolume(mountPath), systemDevices, productMountPoints);
}

/**
Expand All @@ -493,6 +495,7 @@ class ProposalManager {
if (!proxy) return undefined;

const systemDevices = await this.system.getDevices();
const productMountPoints = await this.getProductMountPoints();

const buildResult = (proxy) => {
const buildSpaceAction = dbusSpaceAction => {
Expand Down Expand Up @@ -547,10 +550,11 @@ class ProposalManager {
const values = [
dbusSettings.TargetDevice?.v,
buildTargetPVDevices(dbusSettings.TargetPVDevices),
dbusSettings.BootDevice.v,
dbusSettings.Volumes.v.map(vol => vol.v.TargetDevice.v)
].flat();

if (dbusSettings.ConfigureBoot.v) values.push(dbusSettings.BootDevice.v);

const names = uniq(compact(values)).filter(d => d.length > 0);

// #findDevice returns undefined if no device is found with the given name.
Expand All @@ -571,7 +575,9 @@ class ProposalManager {
spaceActions: dbusSettings.SpaceActions.v.map(a => buildSpaceAction(a.v)),
encryptionPassword: dbusSettings.EncryptionPassword.v,
encryptionMethod: dbusSettings.EncryptionMethod.v,
volumes: dbusSettings.Volumes.v.map(vol => this.buildVolume(vol.v, systemDevices)),
volumes: dbusSettings.Volumes.v.map(vol => (
this.buildVolume(vol.v, systemDevices, productMountPoints))
),
// NOTE: strictly speaking, installation devices does not belong to the settings. It
// should be a separate method instead of an attribute in the settings object.
// Nevertheless, it was added here for simplicity and to avoid passing more props in some
Expand Down Expand Up @@ -654,6 +660,8 @@ class ProposalManager {
* Builds a volume from the D-Bus data
*
* @param {DBusVolume} dbusVolume
* @param {StorageDevice[]} devices
* @param {string[]} productMountPoints
*
* @typedef {Object} DBusVolume
* @property {CockpitString} Target
Expand Down Expand Up @@ -699,7 +707,7 @@ class ProposalManager {
*
* @returns {Volume}
*/
buildVolume(dbusVolume, devices) {
buildVolume(dbusVolume, devices, productMountPoints) {
/**
* Builds a volume target from a D-Bus value.
*
Expand All @@ -719,11 +727,11 @@ class ProposalManager {
}
};

/** @returns {VolumeOutline} */
const buildOutline = (dbusOutline) => {
if (dbusOutline === undefined) return null;

return {
required: dbusOutline.Required.v,
productDefined: false,
fsTypes: dbusOutline.FsTypes.v.map(val => val.v),
supportAutoSize: dbusOutline.SupportAutoSize.v,
adjustByRam: dbusOutline.AdjustByRam.v,
Expand All @@ -733,7 +741,7 @@ class ProposalManager {
};
};

return {
const volume = {
target: buildTarget(dbusVolume.Target.v),
targetDevice: devices.find(d => d.name === dbusVolume.TargetDevice?.v),
mountPath: dbusVolume.MountPath.v,
Expand All @@ -745,6 +753,12 @@ class ProposalManager {
transactional: dbusVolume.Transactional.v,
outline: buildOutline(dbusVolume.Outline.v)
};

// Indicate whether a volume is defined by the product.
if (productMountPoints.includes(volume.mountPath))
volume.outline.productDefined = true;

return volume;
}

/**
Expand Down
30 changes: 25 additions & 5 deletions web/src/client/storage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1430,6 +1430,7 @@ describe("#proposal", () => {

describe("#defaultVolume", () => {
beforeEach(() => {
cockpitProxies.proposalCalculator.ProductMountPoints = ["/", "swap", "/home"];
cockpitProxies.proposalCalculator.DefaultVolume = jest.fn(mountPath => {
switch (mountPath) {
case "/home": return {
Expand Down Expand Up @@ -1504,7 +1505,8 @@ describe("#proposal", () => {
snapshotsConfigurable: false,
snapshotsAffectSizes: false,
adjustByRam: false,
sizeRelevantVolumes: []
sizeRelevantVolumes: [],
productDefined: true
}
});

Expand All @@ -1527,7 +1529,8 @@ describe("#proposal", () => {
snapshotsConfigurable: false,
snapshotsAffectSizes: false,
adjustByRam: false,
sizeRelevantVolumes: []
sizeRelevantVolumes: [],
productDefined: false
}
});
});
Expand All @@ -1550,10 +1553,12 @@ describe("#proposal", () => {
beforeEach(() => {
contexts.withSystemDevices();
contexts.withProposal();
client = new StorageClient();
cockpitProxies.proposalCalculator.ProductMountPoints = ["/", "swap"];
});

it("returns the proposal settings and actions", async () => {
client = new StorageClient();

const { settings, actions } = await client.proposal.getResult();

expect(settings).toMatchObject({
Expand Down Expand Up @@ -1585,7 +1590,8 @@ describe("#proposal", () => {
supportAutoSize: true,
snapshotsConfigurable: true,
snapshotsAffectSizes: true,
sizeRelevantVolumes: ["/home"]
sizeRelevantVolumes: ["/home"],
productDefined: true
}
},
{
Expand All @@ -1604,7 +1610,8 @@ describe("#proposal", () => {
supportAutoSize: false,
snapshotsConfigurable: false,
snapshotsAffectSizes: false,
sizeRelevantVolumes: []
sizeRelevantVolumes: [],
productDefined: false
}
}
]
Expand All @@ -1618,6 +1625,19 @@ describe("#proposal", () => {
{ device: 2, text: "Mount /dev/sdb1 as root", subvol: false, delete: false }
]);
});

describe("if boot is not configured", () => {
beforeEach(() => {
cockpitProxies.proposal.Settings.ConfigureBoot = { t: "b", v: false };
cockpitProxies.proposal.Settings.BootDevice = { t: "s", v: "/dev/sdc" };
});

it("does not include the boot device as installation device", async () => {
client = new StorageClient();
const { settings } = await client.proposal.getResult();
expect(settings.installationDevices).toEqual([sda, sdb]);
});
});
});
});

Expand Down
60 changes: 60 additions & 0 deletions web/src/components/core/FormReadOnlyField.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright (c) [2024] SUSE LLC
*
* All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License as published
* by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
* more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, contact SUSE LLC.
*
* To contact SUSE LLC about this file by physical or electronic mail, you may
* find current contact information at www.suse.com.
*/

// @ts-check
// cspell:ignore labelable

import React from "react";
import styles from '@patternfly/react-styles/css/components/Form/form';

/**
* Renders a read-only form value with a label that visually looks identically
* that a a label of an editable form value, without using the `label` HTML tag.
*
* Basically, this "mimicking component" is needed for two reasons:
*
* - The HTML specification limits the use of labels to "labelable elements".
*
* > Some elements, not all of them form-associated, are categorized as labelable
* > elements. These are elements that can be associated with a label element.
* > => button, input (if the type attribute is not in the Hidden state), meter,
* > output, progress, select, textarea, form-associated custom elements
* >
* > https://html.spec.whatwg.org/multipage/forms.html#categories
*
* - Agama does not use disabled form controls for rendering a value that users
* have no chance to change by any means, but a raw text instead.
*
* Based on PatternFly Form styles to maintain consistency.
*
* @typedef {import("react").ComponentProps<"div">} HTMLDivProps
* @param {HTMLDivProps & { label: string }} props
*/
export default function FormReadOnlyField({ label, children, className = "", ...props }) {
return (
<div className={`${className} ${styles.formGroup}`.trim()} {...props}>
<div className={styles.formGroupLabel} {...props}>
<span className={styles.formLabelText}>{label}</span>
</div>
{children}
</div>
);
}
33 changes: 33 additions & 0 deletions web/src/components/core/FormReadOnlyField.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright (c) [2024] SUSE LLC
*
* All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of version 2 of the GNU General Public License as published
* by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
* more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, contact SUSE LLC.
*
* To contact SUSE LLC about this file by physical or electronic mail, you may
* find current contact information at www.suse.com.
*/

import React from "react";
import { screen } from "@testing-library/react";
import { plainRender } from "~/test-utils";
import { FormReadOnlyField } from "~/components/core";

it("renders label and content wrapped in div nodes using expected PatternFly styles", () => {
plainRender(<FormReadOnlyField label="Product">Agama</FormReadOnlyField>);
const field = screen.getByText("Agama");
const label = screen.getByText("Product");
expect(field.classList.contains("pf-v5-c-form__group")).toBe(true);
expect(label.classList.contains("pf-v5-c-form__label-text")).toBe(true);
});
8 changes: 6 additions & 2 deletions web/src/components/core/FormValidationError.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@
* find current contact information at www.suse.com.
*/

// @ts-check

import React from "react";
import { FormHelperText, HelperText, HelperTextItem } from "@patternfly/react-core";

/**
* Helper component for displaying error messages in a PF/FormGroup
*
* @todo: allow it to accept children instead of message prop
*
* @param {object} props - component props
* @param {string} [props.message] - text to be shown as error
* @param {React.ReactNode} [props.message] - text to be shown as error
*/
export default function FormValidationError({ message }) {
if (!message) return;
Expand All @@ -35,7 +39,7 @@ export default function FormValidationError({ message }) {
<FormHelperText>
<HelperText>
<HelperTextItem variant="error">
{ message }
{message}
</HelperTextItem>
</HelperText>
</FormHelperText>
Expand Down
1 change: 1 addition & 0 deletions web/src/components/core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export { default as Disclosure } from "./Disclosure";
export { default as Sidebar } from "./Sidebar";
export { default as Section } from "./Section";
export { default as FormLabel } from "./FormLabel";
export { default as FormReadOnlyField } from "./FormReadOnlyField";
export { default as FormValidationError } from "./FormValidationError";
export { default as Fieldset } from "./Fieldset";
export { default as Em } from "./Em";
Expand Down
2 changes: 1 addition & 1 deletion web/src/components/storage/BootSelectionDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export default function BootSelectionDialog({

const onSubmit = (e) => {
e.preventDefault();
const device = isBootAuto ? undefined : bootDevice;
const device = ((configureBoot && !isBootAuto) ? bootDevice : undefined);
onAccept({ configureBoot, bootDevice: device });
};

Expand Down
2 changes: 1 addition & 1 deletion web/src/components/storage/BootSelectionDialog.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ describe("BootSelectionDialog", () => {
describe("if the 'Do not configure' option is selected", () => {
beforeEach(() => {
props.configureBoot = true;
props.bootDevice = undefined;
props.bootDevice = sda;
});

it("calls onAccept with the selected options on accept", async () => {
Expand Down
Loading

0 comments on commit bd3018b

Please sign in to comment.