feat(web): edit volume group #2177

joseivanlopez · 2025-03-19T15:47:34Z

Allow editing a volume group.

- The method #buildModel is extracted to a helper because it will be used by other hooks and/or helpers.

- Helpers do not modify the given apiModel object.

- The logical volumes are kept.

dgdavid · 2025-03-21T13:36:23Z

web/src/components/storage/LvmPage.tsx

  useEffect(() => {
-    if (model && !model.volumeGroups.length) setName("system");
-  }, [model]);
+    if (volumeGroup) {
+      setName(volumeGroup.vgName);
+      const targetNames = volumeGroup.getTargetDevices().map((d) => d.name);
+      const targetDevices = allDevices.filter((d) => targetNames.includes(d.name));
+      setSelectedDevices(targetDevices);
+    } else if (model && !model.volumeGroups.length) {
+      setName("system");
+    }
+  }, [model, volumeGroup, allDevices]);


I'm having troubles with this useEffect while trying to write a basic unit test. No matter the solution for making the test work, I would like to find another way to set these default values in forms.

As the project evolves, I find myself questioning the best approach more often. In this case, I’d prefer to wait until all the necessary information is available before rendering. I thought this might be achievable with Suspense queries, or perhaps with a feature from the Router (I can’t recall the name right now—I'll look it up later).

I suspect the useEffect is triggered more than needed because of model or allDevices, which are objects. I saw an useMemo somewhere, but still having problems when using the RTL user.clear(): it does work but immediately the name is updated to "system" again because of this useEffect.

In anycase, since it works manually I'll put a todo in the test in order to keep going.

I finally found the problem in testing: the lack of mocking the useVolumeGroup hook despite of useModel being mocked. Since it is exported and mocked as default, the useVolumeGroup was using (or trying to use, actually don't know) the real implementation.

In any case, still thinking we should revisit, when possible, the approach for initializing the forms when it comes to edit, trying to avoid these useEffects when possible since they make things a bit confusing and put the form at risk to be re-rendered more than needed. Specially when dependencies are objects.

dgdavid · 2025-03-21T13:51:44Z

web/src/components/storage/LvmPage.tsx

+  model: model.Model,
+  volumeGroup?: model.VolumeGroup,
+): string | undefined {
+  if (!vgName.length) return sprintf(_("Name is empty"), vgName);


Locally I've removed the sprintf here, which is actually not needed. The change will be sent along the unit test.

That said, let me add something regarding these errors that I forgot to mention in the previous PR.

Although is true that we have agreed on going ahead and unify later how to render the form errors, these message errors can be improved right now bearing in mind that all rendered fields are considered mandatory.

So, I would go for something like

Form has errors and cannot be sent * Provide a name for the volume group. * Select at least one disk for placing the volume group.

Form has errors and cannot be sent * There is a volume group using "system" name already. Please, enter a different name

Or something like that.

"Name is empty" and "No disk is selected" are somehow useless and obvious from a helpful error message point of view.

Also, when we can provide even useful warning errors to users in the future, we could go for something like

Form has errors and cannot be sent * There is a volume group using "system" name already. Please choose a different one or going to *edit system* instead.

Where edit system would be a link for switching to the edition form

Sure, feel free to adapt it. I simply got inspired by the errors in the encryption form.

I have changed them.

I simply got inspired by the errors in the encryption form.

As I said in the commit message, I'm aware of

it remains inconsistent with the rest of the interface until the tone, style, and error handling can be unified.

The test is limited and could be improved, especially in terms of mocking. It also highlights potential issues with form initialization and the use of useEffect. However, it ensures the form works as expected at a basic level.

Introduced by mistake by undoing changes.

dgdavid · 2025-03-22T16:36:39Z

web/src/components/storage/LvmPage.test.tsx

+  __esModule: true,
+  useVolumeGroup: (id: string) => (id ? mockRootVolumeGroup : null),
+  default: () => mockUseModel,


A side effect of exporting these hooks as default 🤷‍♂️

dgdavid · 2025-03-22T16:37:47Z

web/src/components/storage/LvmPage.test.tsx

+jest.mock("~/hooks/storage/model", () => ({
+  ...jest.requireActual("~/hooks/storage/model"),
+  __esModule: true,
+  useVolumeGroup: (id: string) => (id ? mockRootVolumeGroup : null),


Thought 💭 Mocking is becoming each time more ugly

I feel we should mock the queries instead of the hooks, but I don't know if it is possible.

And going further, I would like to simply mock the result of a http call (we have already talked about it).

dgdavid · 2025-03-22T16:41:03Z

web/src/hooks/storage/api-model.ts

 import { QueryHookOptions } from "~/types/queries";

-function useApiModel(options?: QueryHookOptions): apiModel.Config | null {
+export default function useApiModel(options?: QueryHookOptions): apiModel.Config | null {


No matter where default is placed. I still feeling is weird exporting hooks in that way

I wrote export default function because somehow I am following "this rule": if there is only one thing exported and it is exported by default (i.e., that cases in which the file name reveals what it is exporting) then I use export default function directly. Otherwise I export things at the end.

I see, but it is not the rule we were following. As said previously, it feel weird having hooks exported by default. It's not wrong, but weird. It even make mocking worst.

Refactor error messages to be clearer, more helpful, and concise. While this improves the messaging, it remains inconsistent with the rest of the interface until the tone, style, and error handling can be unified.

dgdavid · 2025-03-22T19:34:35Z

I've added a simple unit test file that has A LOT OF room for improvements. I really sorry for that. But as said in the commit message

The test is limited and could be improved, especially in terms of
mocking. It also highlights potential issues with form initialization
and the use of useEffect. However, it ensures the form works as expected
at a basic level.

Hopefully, we will have time for improve at some point in the future. Ideally after having time to rework forms and unify how they behaves on all aspects: initialization, validations, tone, etc.

web/src/components/storage/LvmPage.test.tsx

joseivanlopez · 2025-03-24T05:48:39Z

web/src/components/storage/LvmPage.test.tsx

+  ...jest.requireActual("~/hooks/storage/edit-volume-group"),
+  __esModule: true,
+  default: () => mockEditVolumeGroup,
+}));


In other tests we do this for mocking a default:

jest.mock("./ConfigEditorMenu", () => () => <div>config editor menu</div>);

Would the same work for these hooks?

jest.mock("~/hooks/storage/edit-volume-group", () => mockEditVolumeGroup);

I tried and didn't work, but maybe I was doing something wrong at some point. I'll try again later, but still being a pain in the ass mocking some modules in a way and others in another.

I can live we such a pain, though. But don't know why still adding more complexity to simple things.

Yes, it works. Like you in your above example, I was missing a function in the middle in my previous attempts 🤷

diff --git a/web/src/components/storage/LvmPage.test.tsx b/web/src/components/storage/LvmPage.test.tsx index d3014b167..145166b95 100644 --- a/web/src/components/storage/LvmPage.test.tsx +++ b/web/src/components/storage/LvmPage.test.tsx @@ -139,11 +139,7 @@ jest.mock("~/hooks/storage/add-volume-group", () => ({ default: () => mockAddVolumeGroup, })); -jest.mock("~/hooks/storage/edit-volume-group", () => ({ - ...jest.requireActual("~/hooks/storage/edit-volume-group"), - __esModule: true, - default: () => mockEditVolumeGroup, -})); +jest.mock("~/hooks/storage/edit-volume-group", () => () => mockEditVolumeGroup); describe("LvmPage", () => { describe("when creating a new volume group", () => {

However, I will keep mocking as it is now because, leaving aside the fact that the whole topic about these default exports is not clear, we have in a rush with other topics.

Feel free to change it before merging if you consider it really important. I still thinking that it is

a pain in the ass mocking some modules in a way and others in another

just because changing the way hooks are exported.

web/src/components/storage/LvmPage.test.tsx

joseivanlopez · 2025-03-24T05:57:34Z

web/src/components/storage/LvmPage.tsx

        <Form id="lvmForm" onSubmit={onSubmit}>
          {errors.length > 0 && (
-            <Alert variant="warning" isInline title={_("Something went wrong")}>
+            <Alert variant="warning" isInline title={_("Check the following before continuing")}>


NP: Should it end with colons?

I don't think so. It's acting as a title.

Avoid using full stops in titles and headings unless they are complete sentences. https://freelingual.com/full-stop-punctuation

Also having a look to the SUSE documentation style guide I've noticed they do not use it in titles neither

Use `model.Drive` instead of directly importing the `Drive` type. This aims to make clear which Drive type is being used when reading, writing, or maintaining the code. However, StorageDevice is not under the `model` namespace (yet?) because it doesn’t currently conflict with a type from `apiModel`. It would be nice to find a better organization for these types, but there is neither time nor a clear vision for how to structure this complex area at the moment.

joseivanlopez

For me, everything looks good ;)

dgdavid

Approving and merging based on Iván's comment

Prepare for releasing Agama 13: - #2041 - #2074 - #2088 - #2089 - #2094 - #2097 - #2098 - #2100 - #2101 - #2104 - #2106 - #2107 - #2108 - #2113 - #2115 - #2116 - #2117 - #2118 - #2120 - #2121 - #2122 - #2123 - #2124 - #2125 - #2126 - #2128 - #2132 - #2135 - #2136 - #2137 - #2139 - #2142 - #2145 - #2146 - #2147 - #2148 - #2151 - #2153 - #2155 - #2156 - #2157 - #2158 - #2159 - #2160 - #2161 - #2163 - #2165 - #2172 - #2174 - #2177 - #2179 - #2180 - #2181 - #2182 - #2183 - #2184 - #2186 - #2187 - #2188 - #2191 - #2192 - #2193 - #2195 - #2197 - #2198 - #2199 - #2200 - #2201 - #2203 - #2204 - #2205 - #2206 - #2207 - #2209 - #2212 - #2213 - #2214 - #2215 - #2216

joseivanlopez force-pushed the edit-vg branch 5 times, most recently from 2c1b0ac to af51dc1 Compare March 21, 2025 09:15

joseivanlopez added 4 commits March 21, 2025 09:31

web: add #isUsed to model drives

e5f0ef0

- The method #buildModel is extracted to a helper because it will be used by other hooks and/or helpers.

web: add hook for editing a volume group

ca4e1d0

web: reorganize storage paths

e3c2238

web: allow editing a volume group

1fb8f6d

joseivanlopez force-pushed the edit-vg branch from af51dc1 to 1fb8f6d Compare March 21, 2025 09:40

joseivanlopez mentioned this pull request Mar 21, 2025

feat(web): delete volume group #2186

Merged

joseivanlopez added 2 commits March 21, 2025 12:29

web: improvements in the storage hooks

a47f1a6

- Helpers do not modify the given apiModel object.

web: fix helper for editing a volume group

2c3eef8

- The logical volumes are kept.

dgdavid reviewed Mar 21, 2025

View reviewed changes

joseivanlopez and others added 3 commits March 21, 2025 15:39

Merge branch 'storage-lvm' into edit-vg

0ffc48a

web: add basic unit test for LVM form

0136416

The test is limited and could be improved, especially in terms of mocking. It also highlights potential issues with form initialization and the use of useEffect. However, it ensures the form works as expected at a basic level.

fix(web): drop repeated lines

13cc747

Introduced by mistake by undoing changes.

dgdavid reviewed Mar 22, 2025

View reviewed changes

fix(web): improve error messages

77a435f

Refactor error messages to be clearer, more helpful, and concise. While this improves the messaging, it remains inconsistent with the rest of the interface until the tone, style, and error handling can be unified.

joseivanlopez commented Mar 24, 2025

View reviewed changes

web/src/components/storage/LvmPage.test.tsx Outdated Show resolved Hide resolved

joseivanlopez commented Mar 24, 2025

View reviewed changes

web/src/components/storage/LvmPage.test.tsx Outdated Show resolved Hide resolved

joseivanlopez commented Mar 24, 2025

View reviewed changes

dgdavid added 2 commits March 24, 2025 07:58

doc(web): drop repeated word

235e38f

joseivanlopez marked this pull request as ready for review March 24, 2025 11:56

joseivanlopez commented Mar 24, 2025

View reviewed changes

dgdavid approved these changes Mar 24, 2025

View reviewed changes

dgdavid merged commit 10526e8 into storage-lvm Mar 24, 2025
3 checks passed

dgdavid deleted the edit-vg branch March 24, 2025 12:30

imobachgs mentioned this pull request Mar 27, 2025

Update to release 13 #2217

Merged

feat(web): edit volume group #2177

feat(web): edit volume group #2177

Uh oh!

Conversation

joseivanlopez commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgdavid Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgdavid Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgdavid Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgdavid commented Mar 22, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgdavid Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseivanlopez left a comment

Choose a reason for hiding this comment

Uh oh!

dgdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joseivanlopez commented Mar 19, 2025 •

edited

Loading

dgdavid Mar 21, 2025 •

edited

Loading

dgdavid Mar 21, 2025 •

edited

Loading

dgdavid Mar 24, 2025 •

edited

Loading

dgdavid Mar 24, 2025 •

edited

Loading