Issue#932

[nagu165]

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:

After:

[dgdavid]

Hi @nagu165,

Thanks a lot for your interest in the project and for your first contribution to it.

I've left some comments there, please check them.

Additionally, I'd like to recommend you reading the new React documentation if you haven't done it yet. It will help you a lot to understand the concepts used in the Agama web UI. Beyond JSX, React is a powerful UI library that needs investing time to fully understand it. Also, would be nice if you check the React Testing Library in order you can update or create the related unit tests. Last but not least, you can have at hand the https://javascript.info for checking it anytime you have doubts about JavaScript, has to recap something, or you simply learn something new you are not familiar with. MDN Web Docs is also a very good learning resource. I use all of them 🤓

[dgdavid]

I didn't check it by myself yet, but reading the code I'd say this is incomplete because now nothing will be shown when currentVolume is defined. Remember that the form is used for both cases, when defined and when not.

{currentVolume !== undefined && <p>{state.formData.mountPoint</p>}

Note the use of less curly braces. You can have a look to React conditional rendering documentation at https://react.dev/learn/conditional-rendering.

You can use the Agama core/<If> component instead, which is what we're using for conditional rendering in JSX

<If condition={currentVolume !== undefined} then={<p>{state.formData.mounPoint}</p>} />

Moreover, actually for this use case you need to use both core/If component props, then and else, in order to show the selector or the mount point depending on the value of currentVolume. Something like,

const MountPointSelector = () => (
  <MountPointFormSelect
    ...
  />
);

const MountPoint = () => <p>{state.formData.mountPoint}</p>;

<If
  condition={currentVolume}
  then={<MountPointSelector />}
  else={<MountPoint />}
/> 

Please, note that code above is just an incomplete example for illustrating how I'd proceed. I'm using kind of internal components there instead of just variables to not evaluate them until really needed. Check the React documentation about components at https://react.dev/learn/your-first-component and also have a look to https://javascript.info/arrow-functions-basics if you're not familiar with arrow functions to fully understand the example.

[nagu165]

Hello Sir, Thank you so much for the detailed explanation of the errors I have made and also for the Recommendations to follow for learning more about the project. I will try my best to rectify my mistakes.
Sorry for the inconvenience caused.

[dgdavid]

No problem at all.

The only mistake there was not mounting/rendering the selector when needed. That's why unit tests are really important, for covering such use cases and checking that we do not overlook them. The rest was just a different way of thinking/doing things.

In any case, if you tell us about your knowledge level, maybe we can find easier tasks that allow you to learn and contribute at the same time. Also, I'd like to know if you finally were able to run the environment in the virtual machine or not. It's important that you can safely play with the project while contributing to help you to understand what is going on.

That said, these links I put in other comments are always helpful no matter your knowledge or the project you're contributing to (as long as the project is about JavaScript / React).

[nagu165]

I am a beginner to open source and am currently in my 2nd year of university.
My skills Include C/C++, JavaScript, Typescript(Basic), Python(Basic), MERN stack (still practicing), Next JS (Basic).
I am still having issues with running the environment. I did as sir @joseivanlopez recommended #932 (comment)
but when I point to localhost 8080 It's not working.

I selected openSUSE Tumbleweed when asked and also installed all the packages. After that, it told me to reboot to start with the project but when I did reboot it was still showing this page only.
Thank you for the response.

[nagu165]

I am thinking of running the project locally. Is it okay to do so.

[lslezak]

I think I found the problem after seeing the 10.0.2.15 IP address.

Are you using VirtualBox for running the virtual machine (VM)? That 10.0.2.15 IP address is used in VirtualBox VM as the default IP when using the NAT network type. And in that mode the machine is not reachable from outside, not even from the host (the same machine) unless you configure port forwarding.

Configuring and using port forwarding is not trivial so I'd recommend to use the bridged network mode instead. Just go to the VM configuration and change the "NAT" option to the "Bridged Adapter":

(Note: You should change that only if the VM is not running, if it is running you should shut it down first.)

You can find more details about the network modes in the VirtualBox documentation.

[dgdavid]

Thanks a lot for that hint @lslezak! 👍

Let's see if @nagu165 is able to get the development environment ready after applying it 🤞

[nagu165]

Thank you so much @dgdavid sir and @lslezak sir. After following your instructions I was successful in setting up the development environment. I will do my best to contribute well to the project.

[dgdavid]

Glad to read that ;)

We've learned a few things about problems faced by new contributors. We hope to update the development documentation ASAP.

BTW, a bit off-topic but now that we know your experience, I'm curious to know a bit more about how @balsa-asanovic set up his development environment since he already made a few contributions. If you don't mind @balsa-asanovic, tell us a bit about your environment and if you remember you had troubles when setting up Agama. Thanks in advance.

[balsa-asanovic]

Hey guys,

@nagu165 welcome to the project!

I'm curious to know a bit more about how @balsa-asanovic set up his development environment

I use two environments, initially I made a separate partition on my HDD and installed Open SUSE Tumbleweed. Then I'd boot to Tumbleweed and proceed with the development there.

As to the project itself, I'd just do a git clone and then run ./setup.sh (with sudo).
I don't remember having any major issues. I do remember running into some errors when running the ./setup.sh for the first couple of times, but this usually amounted to doing zypper install somePackage that was missing.

For example, the last issue I had was actually just a couple days ago. After syncing my fork with the main project the ./setup.sh starting reporting error for the rustc version. I ended up updating cargo (and rustc within that) and this fixed the issue.

Lately, I've been using a second environment. I've set up a Windows Subsystem for Linux on my Windows OS. I installed Tumbleweed in WSL as well. I've used this WSL for the large part of my last PR, so I can say that it is working nicely.

Previously, I did encounter that some things didn't work as expected in Agama application when ran in WSL and then I'd just revert to using the Tumbleweed on my HDD. However, lately everything seems to work as expected in WSL as well, this is possibly due to the improvements in the project itself.

In relation the issues @nagu165 is having, I didn't really use VirtualBox or VmWare at all, so not much help there. 😅

[dgdavid]

Why are these compiler options needed in the context of this change? If they are really needed, I prefer a specific PR for them documenting the reason in the PR description.

[nagu165]

VSCode was showing some errors while I was working on the code and it recommended me to use those two options.
Stackoverflow also recommended those settings for writing a overall better code.
I am sorry regarding the PR. I will be sure to keep it in mind for the future.
Thank you.

[dgdavid]

Ok, I've checked the TypeScript Config documentation and the forceCosistentCasingInFileNames definitely makes sense for avoiding import errors when working with case-insensitive filesystems as it is the case of Windows (read more at https://learn.microsoft.com/en-us/windows/wsl/case-sensitivity)

I've doubts about using strict, I need to read it with more attention and do some tests before going ahead.

Anyway, let's keep this file out of this PR.

Thanks a lot for letting us know about these VSCode suggestions you found 💯

[dgdavid]

Is that working? As far as I can see in both, documentation and code, SelectOption does not have support for label prop but children instead. And children can be rendered like they were previously, wrapped by <SelectOption></SelectOption>

[nagu165]

There are some issues there I couldn't find more details regarding the select component. I was testing the code on how these changes were affecting the overall code.

[dgdavid]

Ok, I can start helping you by making commits, do you agree?

In this case, I think you need to apply this change:

Suggested change

	{ volumes.map(v => <SelectOption key={v.mountPath} value={v.mountPath} label={v.mountPath} />) }
	{ volumes.map(v => <SelectOption key={v.mountPath} value={v.mountPath}>{v.mountPath}</SelectOption>) }

[dgdavid]

BTW, how the UI looks at this time for you when adding and editing a file system?

[nagu165]

Okay sir, Thank you for the help.
I will change it right away.

[nagu165]

BTW, how the UI looks at this time for you when adding and editing a file system?

It's showing errors

[dgdavid]

I see, that's because you're missing some important props for the PatternFly/Select component like toggle. Please, have a look to the previous work done by @joseivanlopez at https://github.com/openSUSE/agama/blob/927352ba83c0d7cc2db6efd3ab693c7a1f140947/web/src/components/storage/VolumeForm.jsx#L223-L230 but, most important, to the Select documentation for understanding how the component works.

Don't be in a hurry. Once you have it more clear we can continue.

[nagu165]

I see, that's because you're missing some important props for the PatternFly/Select component like toggle. Please, have a look to the previous work done by @joseivanlopez at

https://github.com/openSUSE/agama/blob/927352ba83c0d7cc2db6efd3ab693c7a1f140947/web/src/components/storage/VolumeForm.jsx#L223-L230

but, most important, to the Select documentation for understanding how the component works.
Don't be in a hurry. Once you have it more clear we can continue.

Thank you sir, I will surely go through the documentation and make appropriate changes.

[dgdavid]

BTW, since I know you're learning I'd like to suggest to use more descriptive commit messages. We do not have an official style guide for them in Agama, but in YaST was suggested

Each commit in the pull request should do only one thing, which is clearly described by its commit message. Especially avoid mixing formatting changes and functional changes into one commit. When writing commit messages, adhere to widely used conventions.

Think that these messages are kind of documentation too. Meaningful commits are an important piece of the version control system.

If you're curious, there are other conventions like https://www.conventionalcommits.org/en/v1.0.0/ which I'm thinking to propose for the project.

[nagu165]

BTW, since I know you're learning I'd suggest to use more descriptive commit messages. We do not have an official style guide for them in Agama, but in YaST was suggested

Each commit in the pull request should do only one thing, which is clearly described by its commit message. Especially avoid mixing formatting changes and functional changes into one commit. When writing commit messages, adhere to widely used conventions.

Think that these messages are kind of documentation too. Meaningful commits are an important piece of the version control system.

If you're curious, there are other conventions like https://www.conventionalcommits.org/en/v1.0.0/ which I'm thinking to propose for the project.

Okay sir, Thank you for the suggestion.

[dgdavid]

@nagu165,

First of all, thank you for start using better commit messages and for updating the PR description too 💯 💯 💯

Also, thanks for keeping going 👌 We know that Agama is not a simple project for getting started and so we really appreciate you still trying to shape that PR in spite of the amount of comments I'm doing 😅 😅

That said, I've left a few comments requesting changes. Please, check them.

[dgdavid]

I prefer using the MenuToggle as you can see in the Select documentation examples.

Regarding default value, I'd use something more simple like Select a value or Select a mount point, no strong opinion. In any case, Agama has support for both, i18n and l10n, so please always mark texts for translations by using the _ function. I.e., _("Select a value") instead of "Select a value". Read more at https://github.com/openSUSE/agama/blob/master/doc/i18n.md#marking-texts-for-translation

[dgdavid]

Use camelCase. I.e., setChosen instead of setchosen.

Suggested change

	const [chosen, setchosen] = useState(false);
	const [chosen, setChosen] = useState(false);

Do not forget updating other lines where using setchosen.

[dgdavid]

If you revisit the Select documentation, you can see that options should be wrapped in a <SelectList>. Please, fix it.

[nagu165]

Sorry sir, I missed that part. I'll fix it. Thank you

[dgdavid]

Ok, as already commented, it's needed to revert changes made in the .tsconfig file. If you have a look at the failed continuous integration check you can see there that npm run check-types is crashing. Running it locally reports Found 512 errors in 23 files.. That's a consequence of strict: true that I do not know how to fix now.

So, let's keep this file as it was and postpone changes on it for a separated PR. To do so, simply run a git revert 3e2fc75e403ab760ba602075788e91332b98e6c94 and then a git push.

[dgdavid]

Hi @nagu165,

As you can check, I've added a few commits to your PR in order to move it forward at a higher speed 😉 Somehow, I feel we failed labeling this as a good-first-issue since that VolumeForm component is rather complex. Nevertheless, thanks to your contribution the issue is almost fixed 👏

I highly recommend you going through my commits to understand what was wrong and what missing. In short,

• Update unit tests

• Make the mount point selector work as expected

  Please, check it manually too. Look at the different behavior before and after my commit to get an idea why it was wrong.

There is, however, a small thing I've left for you: the * mark meaning that the field is required must be there only when the mount point is a widget that allows the user to modify it. You've a hint in the FsField, around lines 288 to 303 (of master branch) https://github.com/openSUSE/agama/blob/a805fc2e9887df1e0fa663dd3abe7bdf0e3138f5/web/src/components/storage/VolumeForm.jsx#L288-L303

Another idea could be to move all the implied parts to the same component 🙄 . I.e,

Click to show/hide an idea

---

diff --git a/web/src/components/storage/VolumeForm.jsx b/web/src/components/storage/VolumeForm.jsx
index 8cee09ae..6471baf9 100644
--- a/web/src/components/storage/VolumeForm.jsx
+++ b/web/src/components/storage/VolumeForm.jsx
@@ -77,7 +77,6 @@ const SizeUnitFormSelect = ({ units, ...formSelectProps }) => {
  * @param {object} props.selectProps - other props sent to {@link https://www.patternfly.org/components/menus/select#props PF/Select}
  * @returns {ReactComponent}
 */
-
 const MountPointFormSelect = ({ value, volumes, onChange, ...selectProps }) => {
   const [isOpen, setIsOpen] = useState(false);
 
@@ -280,6 +279,36 @@ const FsSelect = ({ id, value, volume, onChange }) => {
   );
 };
 
+/**
+ * Widget for rendering the mount point field
+ *
+ * Allows selecting the mount point when creating a new volume. It renders the
+ * volume mount path otherwise.
+ * @component
+ *
+ * @param {object} props
+ * @param {string} selectedMountPoint - The mountPoint value for the selected volume.
+ * @param {Volume|undefined} currentVolume - The volume being edited. Undefined when creating a new one.
+ * @param {Array<Volume>} volume - A collection of available storage volume.
+ * @param {onChangeFn} props.onChange - Callback for notifying input changes.
+ *
+ * @returns {ReactComponent}
+ */
+const MountPointField = ({ currentVolume, selectedMountPoint, volumes, onChange }) => {
+  let labelProps = { label: _("Mount point") };
+  if (!currentVolume) labelProps = { ...labelProps, fieldId: "mountPoint", isRequired: true };
+
+  return (
+    <FormGroup {...labelProps}>
+      <If
+        condition={currentVolume}
+        then={<p>{currentVolume?.mountPath}</p>}
+        else={<MountPointFormSelect value={selectedMountPoint} onChange={onChange} volumes={volumes} /> }
+      />
+    </FormGroup>
+  );
+};
+
 /**
  * Widget for rendering the file system configuration.
  *
@@ -762,25 +791,14 @@ 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">
-        <If
-          condition={currentVolume !== undefined}
-          then={<ShowMountPoint />}
-          else={<ShowMountPointSelector />}
-        />
-      </FormGroup>
+      <MountPointField
+        currentVolume={currentVolume}
+        selectedMountPoint={state.formData.mountPoint}
+        volumes={templates}
+        onChange={changeVolume}
+      />
       <FsField
         value={{ fsType, snapshots }}
         volume={state.volume}

Hope it helps.

[coveralls]

coverage: 74.882% (+0.03%) from 74.852%
when pulling a68be13 on nagu165:issue#932
into d805b37 on openSUSE:master.

[dgdavid]

Also, I took a few notes to explain you to understand changes in tests.

Let's take the when editing a new volume test example. After the changes you introduced, it's no longer needed to chck that an element with role combobox is disabled because there is no such an element anymore. Instead, it's enough checking that the mount path is rendered.

diff --git a/web/src/components/storage/VolumeForm.test.jsx b/web/src/components/storage/VolumeForm.test.jsx
index bcad0d41..79cafcd1 100644
--- a/web/src/components/storage/VolumeForm.test.jsx
+++ b/web/src/components/storage/VolumeForm.test.jsx
@@ -296,11 +296,10 @@ 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("/");
   });
 });

Click to show/hide a somehow advanced note

---

It'd be nice to check that it's rendered in the expected place. I.e., along with the "Mount point" label. But sadly, the current HTML output is too generic to allow us checking that in a simple way. Personally, I do not want to look for the "Mount point" node and goes up to a parent with "pf-v5-c-form_group" CSS class to finally check that the expected value is rendered there. Too tied to the HTML/CSS structure, definitely not how React Testing Library encourage to test the UI.

---

Maybe we could improve a bit the above example by looking for role "paragraph".

diff --git a/web/src/components/storage/VolumeForm.test.jsx b/web/src/components/storage/VolumeForm.test.jsx
index 79cafcd1..314e1b74 100644
--- a/web/src/components/storage/VolumeForm.test.jsx
+++ b/web/src/components/storage/VolumeForm.test.jsx
@@ -299,7 +299,7 @@ describe("when editing a new volume", () => {
   it("renders the mount point", () => {
     plainRender(<VolumeForm {...props} />);
 
-    screen.getByText("/");
+    screen.getByRole("paragraph", "/");
   });
 });

However, it does not work at the time of writing (see testing-library/dom-testing-library#1234)

To finish with this example, we can check that there is not a control for changing the mount point, although I'm not so sure if we should do it but let's keep some consistency with the rest of testsuite

diff --git a/web/src/components/storage/VolumeForm.test.jsx b/web/src/components/storage/VolumeForm.test.jsx
index 79cafcd1..5c619f96 100644
--- a/web/src/components/storage/VolumeForm.test.jsx
+++ b/web/src/components/storage/VolumeForm.test.jsx
@@ -301,6 +301,13 @@ describe("when editing a new volume", () => {
 
     screen.getByText("/");
   });
+
+  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()
+    ))
+  });
 });

While it works, it's actually too coupled to the implementation details. We're looking for a button because we know that now we're using a Select component that is using a button behind the scene for displaying the options. But if someone adds a plain HTML select for changing the mount point... the test will still pass. In any case, let's keep there for the sake of consistency.

And that's it. I'm skipping the rest of changes needed for updating the test suite and making it validate the current behavior, but with a bit of luck you got it. In any case, please, have a look at the rest of updated test examples and ask doubts if you have any after reading them. Do not forget to check the React Testing Library documentation 😉 , since it's an important piece for contributing Agama Web UI code.

[nagu165]

Hi @nagu165,

As you can check, [...]

Quoted text above edited by @dgdavid to improve readability of the answer

Thank you so much for the response sir, Sorry for the trouble I've caused you the past 2 weeks. I am going through the commits and learning how the changes work. I'll inform you in case of issues.
Thank you.

[dgdavid]

Thank you so much for the response sir, Sorry for the trouble I've caused you the past 2 weeks.

No troubles at all :)

I'd be more than happy if the PR helped you to learn and, of course, let you keep contributing to the Agama project. Moreover, I encourage you to put in practice what you've learned until now, taking your time for reading React, PatternFly and friends documentation while coding, asking doubts as soon as they arise, and also giving love to the code documentation and commit messages as well.

Taking time is the key for improving your skills. After all, we're not in a hurry.

[dgdavid]

Ups, I was solving a conflict you introduced in the changes file but suddenly... you force-pushed it and closed the PR. Do you need help?

[nagu165]

Ups, I was solving a conflict you introduced in the changes file but suddenly... you force-pushed it and closed the PR. Do you need help?

Really sorry sir, I was trying to resolve the issue but accidentally messed up. I forgot to pull from the upstream before opening a pull request that might have caused the issue.