web: Add file systems for arbitrary mount points #1154

joseivanlopez · 2024-04-16T16:26:04Z

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.

Screenshots

- Missing unit tests.

Because, according to HTML specification, labels can be used only with "labelable elements" which is not the case of Agama read-only values. > Some elements, not all of them form-associated, are categorized as > labelable elements. These are elements that can be associated with a > label element. See https://html.spec.whatwg.org/multipage/forms.html#categories

dgdavid

It LGTM. I just added a few comments for the record.

dgdavid · 2024-04-25T11:00:33Z

web/src/components/storage/PartitionsField.jsx

  const deleteVolume = (volume) => {
    const newVolumes = volumes.filter(v => v.mountPath !== volume.mountPath);
    onVolumesChange(newVolumes);
  };

+  /** @type {() => React.ReactElement[]|React.ReactElement} */


Wondering if ReactNode fits better here,

Suggested change

/** @type {() => React.ReactElement[]|React.ReactElement} */

/** @type {() => React.ReactNode} */

dgdavid · 2024-04-25T11:09:40Z

web/src/components/storage/PartitionsField.jsx

+  const showAddVolume = () => {
+    const hasOptionalVolumes = () => {
+      return templates.find(t => t.mountPath.length && !t.outline.required) !== undefined;
+    };
+
+    return !isTransactionalSystem(templates) || hasOptionalVolumes();
+  };


You already explained to me the reason why you tend to use functions for a better logic encapsulation. But still, I think that sometimes we're abusing a bit of their usage.

Here, I can see the benefit of having hasOptionalVolumes defined as an small function in showAddVolume function. It prevents to evaluate the find if it's actually not needed because is not a transactional system. But maybe hasOptionalVolumes also deserves a place in storage/utils and should recive templates as an argument instead of getting them from the scope, as isTransactionalSysmte alrady does. Thus, the showAddVolume would be not needed at all, and instead you can go for a <If condition={!isTransactionalSystem(templates) || hasOptionalVolumes(templates)} ... /> below. Much clear and less indirection IMHO. Plus, developers can reuse hasOptionalVolumes when needed by taking a look at what we have in storage/utils, avoiding to create yet another function to do the same.

Anyway, I know that this is just a matter of taste.

It sounds good. Feel free to move it.

BTW, the meaning of this #hasOptionalVolumes is a bit specific to the use case. I mean, it is ignoring the template for arbitrary volumes. A generic #hasOptionalVolumes should consider the arbirary volume. If you move the method to utils, then please either rename it as #hasOptionalPredefinedVolumes or include a parameter like #hasOptionalVolumes(volumes, { includeArbitrary: true }).

The PR is already merged. Let's move it to utils in the next opportunity. Making a note in my todo list.

dgdavid · 2024-04-25T11:14:13Z

web/src/components/storage/VolumeDialog.jsx

+      <div className="split">
+        <span>{sprintf(_("There is already a file system for %s."), path)}</span>
+        <Button variant="link" isInline onClick={() => onClick(volume)}>
+          {_("Do you want to edit it?")}
+        </Button>
+      </div>


Just a note: it LGTM for this iteration, but I plan to make "assisted errors" to look a bit different than "format errors" in a future. The former should have a less "red" (Not be part of FormValidationError) and more prominent look and feel. I would came with a proposal when time permits.

web/src/components/storage/VolumeDialog.jsx

web/src/components/storage/VolumeFields.jsx

dgdavid · 2024-04-25T11:20:55Z

web/src/components/storage/VolumeFields.jsx

 */
-const FsSelect = ({ id, value, volume, onChange }) => {
+const FsSelect = ({ id, value, volume, isDisabled, onChange }) => {


Question: below, in the line 154 (former 174), the onChange callback sent { fsType: option, snapshots: false }. Still snapshots: false being relevant here?

I good say it is even wrong. Snapshots should be configured only by means of the snapshots field. Good catch!

web/src/components/storage/VolumeFields.test.jsx

lslezak

Names?

web/src/components/storage/VolumeDialog.jsx

- Leftover. Now there is a specific field for configuring snapshots.

dgdavid · 2024-04-25T13:44:29Z

BTW, not directly related to this PR since it actually comes from #543 (00a5eba) and #590 (94cc4e1) , but we have

agama/web/src/components/storage/VolumeFields.jsx

Lines 262 to 273 in 1b4bb22

    
             conditions.push(sprintf(_("the presence of the file system for %s"), 
        
               // TRANSLATORS: conjunction for merging two list items 
        
                                     volume.outline.sizeRelevantVolumes.join(_(", ")))); 
        
           if (volume.outline.adjustByRam) 
        
             // TRANSLATORS: item which affects the final computed partition size 
        
             conditions.push(_("the amount of RAM in the system")); 
        
           // TRANSLATORS: the %s is replaced by the items which affect the computed size 
        
           const conditionsText = sprintf(_("The final size depends on %s."), 
        
             // TRANSLATORS: conjunction for merging two texts 
        
                                          conditions.join(_(" and ")));

which make me think again that we should start using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat instead.

What do you think?

lslezak · 2024-04-25T14:05:49Z

which make me think again that we should start using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat instead.

What do you think?

Yes, I think it is a good idea. I didn't know about these functions when I implemented this.

web/src/components/storage/index.js

coveralls · 2024-04-25T15:05:43Z

coverage: 75.053% (+0.3%) from 74.742%
when pulling 1bbfada on joseivanlopez:add-arbitrary-volumes
into ace7b96 on openSUSE:master.

dgdavid · 2024-04-25T17:17:55Z

web/src/components/storage/VolumeDialog.jsx

+/** @fixme Redesign *Error classes.
+ *
+ *  Having different *Error classes does not seem to be a good design. Note these classes do not
+ *  represent an error but a helper to check and render an error. It would be a better approach to
+ *  have something like a volume checker which generates errors:
+ *
+ *  For example:
+ *
+ *  const checker = new VolumeChecker(volume, volumes, templates);
+ *  const error = checker.existingMountPathError();
+ *  const message = error?.render(onClick);
+*/
+


Prepare for releasing Agama 8. It includes the following pull requests: * #884 * #886 * #914 * #918 * #956 * #957 * #958 * #959 * #960 * #961 * #962 * #963 * #964 * #965 * #966 * #969 * #970 * #976 * #977 * #978 * #979 * #980 * #981 * #983 * #984 * #985 * #986 * #988 * #991 * #992 * #995 * #996 * #997 * #999 * #1003 * #1004 * #1006 * #1007 * #1008 * #1009 * #1010 * #1011 * #1012 * #1014 * #1015 * #1016 * #1017 * #1020 * #1022 * #1023 * #1024 * #1025 * #1027 * #1028 * #1029 * #1030 * #1031 * #1032 * #1033 * #1034 * #1035 * #1036 * #1038 * #1039 * #1041 * #1042 * #1043 * #1045 * #1046 * #1047 * #1048 * #1052 * #1054 * #1056 * #1057 * #1060 * #1061 * #1062 * #1063 * #1064 * #1066 * #1067 * #1068 * #1069 * #1071 * #1072 * #1073 * #1074 * #1075 * #1079 * #1080 * #1081 * #1082 * #1085 * #1086 * #1087 * #1088 * #1089 * #1090 * #1091 * #1092 * #1093 * #1094 * #1095 * #1096 * #1097 * #1098 * #1099 * #1100 * #1102 * #1103 * #1104 * #1105 * #1106 * #1109 * #1110 * #1111 * #1112 * #1114 * #1116 * #1117 * #1118 * #1119 * #1120 * #1121 * #1122 * #1123 * #1125 * #1126 * #1127 * #1128 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1135 * #1136 * #1138 * #1139 * #1140 * #1141 * #1142 * #1143 * #1144 * #1145 * #1146 * #1147 * #1148 * #1149 * #1151 * #1152 * #1153 * #1154 * #1155 * #1156 * #1157 * #1158 * #1160 * #1161 * #1162 * #1163 * #1164 * #1165 * #1166 * #1167 * #1168 * #1169 * #1170 * #1171 * #1172 * #1173 * #1174 * #1175 * #1177 * #1178 * #1180 * #1181 * #1182 * #1183 * #1184 * #1185 * #1187 * #1188 * #1189 * #1190 * #1191 * #1192 * #1193 * #1194 * #1195 * #1196 * #1198 * #1199 * #1200 * #1201 * #1203 * #1204 * #1205 * #1206 * #1207 * #1208 * #1209 * #1210 * #1211 * #1212 * #1213 * #1214 * #1215 * #1216 * #1217 * #1219 * #1220 * #1221 * #1222 * #1223 * #1224 * #1225 * #1226 * #1227 * #1229

joseivanlopez force-pushed the add-arbitrary-volumes branch from 776f3ab to 0634933 Compare April 22, 2024 11:12

joseivanlopez added 4 commits April 22, 2024 14:59

web: Indicate if a volume is defined by product

071b7ab

web: Do not consider boot device if not needed

b3d863d

web: Do not send boot device if not needed

b506ad8

web: Fix tests

9ca19c1

joseivanlopez force-pushed the add-arbitrary-volumes branch from 6c46d3e to c12a67f Compare April 22, 2024 15:42

joseivanlopez added 3 commits April 22, 2024 17:28

web: Add VolumeDialog

22c2126

- Missing unit tests.

web: Allow adding arbitrary volumes

2291c32

- Missing unit tests.

web: Fix some types

f89af42

joseivanlopez force-pushed the add-arbitrary-volumes branch from c12a67f to f89af42 Compare April 22, 2024 16:28

This comment was marked as outdated.

Sign in to view

dgdavid added 6 commits April 25, 2024 11:45

web: Adapt VolumeDialog unit tests

f6c7e5c

web: Add storage/PartitionsField unit tests

0d9bfed

web: Fix core/FormValidationError types

306c99c

web: Add storage/VolumeFields tests

c939a9a

web: Enable core/FormReadOnlyField type checking

5ffa5fc

Merge branch 'master' into add-arbitrary-volumes

c92f553

dgdavid marked this pull request as ready for review April 25, 2024 10:54

dgdavid approved these changes Apr 25, 2024

View reviewed changes

lslezak reviewed Apr 25, 2024

View reviewed changes

web/src/components/storage/VolumeDialog.jsx Show resolved Hide resolved

web: Do not set snapshots

1b4bb22

- Leftover. Now there is a specific field for configuring snapshots.

joseivanlopez added 2 commits April 25, 2024 15:29

web: Add FIXME about *Error classes

176641b

web: Remove leftover

5c580a1

joseivanlopez commented Apr 25, 2024

View reviewed changes

web/src/components/storage/index.js Outdated Show resolved Hide resolved

joseivanlopez added 2 commits April 25, 2024 16:00

web: Fix test

18b7f6b

web: Changelog

6e3d326

Merge branch 'master' into add-arbitrary-volumes

1bbfada

lslezak approved these changes Apr 25, 2024

View reviewed changes

joseivanlopez merged commit bd3018b into openSUSE:master Apr 25, 2024
1 check passed

dgdavid reviewed Apr 25, 2024

View reviewed changes

imobachgs mentioned this pull request May 17, 2024

Update to release 8 #1232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: Add file systems for arbitrary mount points #1154

web: Add file systems for arbitrary mount points #1154

joseivanlopez commented Apr 16, 2024 •

edited

This comment was marked as outdated.

dgdavid left a comment

dgdavid Apr 25, 2024

dgdavid Apr 25, 2024

joseivanlopez Apr 25, 2024 •

edited

joseivanlopez Apr 25, 2024

dgdavid Apr 25, 2024

dgdavid Apr 25, 2024

dgdavid Apr 25, 2024

joseivanlopez Apr 25, 2024

lslezak left a comment

dgdavid commented Apr 25, 2024

lslezak commented Apr 25, 2024

coveralls commented Apr 25, 2024 •

edited

dgdavid Apr 25, 2024

	/** @type {() => React.ReactElement[]\|React.ReactElement} */
	/** @type {() => React.ReactNode} */

web: Add file systems for arbitrary mount points #1154

web: Add file systems for arbitrary mount points #1154

Conversation

joseivanlopez commented Apr 16, 2024 • edited

This comment was marked as outdated.

dgdavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseivanlopez Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak left a comment

Choose a reason for hiding this comment

dgdavid commented Apr 25, 2024

lslezak commented Apr 25, 2024

coveralls commented Apr 25, 2024 • edited

Choose a reason for hiding this comment

joseivanlopez commented Apr 16, 2024 •

edited

joseivanlopez Apr 25, 2024 •

edited

coveralls commented Apr 25, 2024 •

edited