[UI] Volume Batch Creation #2981

ChengYanJin · 2020-12-14T11:22:35Z

Component: ui, volumes

Context:
We want to be able to batch create volume from UI.

Summary:
Note that:
For RawBlockDevice, the recommended path only happens with '/dev/vda', we will see later if
For SparseLoopDevice, the size of the batch volume are the same as the default one.

Acceptance criteria:

The Default Volume Creation Form
Volume type RawBlockDevice batch creation
Volume type SparseLoopDevice batch creation. (From big HD screen)

Closes: #2964

bert-e · 2020-12-14T11:22:37Z

Hello chengyanjin,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2020-12-14T11:22:42Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

JBWatenbergScality · 2020-12-16T16:18:25Z

ui/src/services/utils.js

+export const linuxDrivesNamingIncrement = (string, increment) => {
+  if (string.match(/^\/dev\/vd[a-z]/)) {
+    while (increment--) {
+      var lastChar = string.slice(-1);


I'm not yet sure about your styleguide conventions but I would always prefer usage of let or const for variable definitions, using const here will reflect clearly that this variable doesn't change elsewhere and will slightly simplify reading this code

JBWatenbergScality · 2020-12-16T16:23:24Z

ui/src/services/utils.js

+ *
+ * const nextDevicePath = linuxDrivesNamingIncrement(string)
+ */
+export const linuxDrivesNamingIncrement = (string, increment) => {


I would rename string to something more explicit such as drivePath

Additionally for utils function like this one it seems important for me to add a unit test that covers the edge cases you want to support, but not sure if it is your traditional way of working so we can discuss about it.

Indeed, it's a very bad name. I will rename it to devicePath.

JBWatenbergScality · 2020-12-16T16:24:27Z

ui/src/services/utils.js

+ * @example
+ * const string = '/dev/vda'
+ *
+ * const nextDevicePath = linuxDrivesNamingIncrement(string)


This example lack increment parameter

JBWatenbergScality · 2020-12-16T16:29:52Z

ui/src/services/utils.js

@@ -419,3 +419,45 @@ export const useDynamicChartSize = (container_id) => {

  return [graphWidth, window.innerHeight / 6 - 30];
 };
+
+/**
+ * This function increments the string by 1


This description doesn't seems to be accurate

JBWatenbergScality · 2020-12-17T09:17:28Z

ui/src/containers/CreateVolume.js

+  const RecommendNameField = (props) => {
+    const {
+      values: { name },
+      touched,
+      setFieldValue,
+    } = useFormikContext();
+    const [field, meta] = useField(props);
+    React.useEffect(() => {
+      if (name.trim() !== '' && touched.name) {
+        setFieldValue(props.name, `${name}${props.index + 1}`);
+      }
+    }, [name, touched.name, setFieldValue, props.name, props.index]);
+
+    return (
+      <>
+        <Input {...props} {...field} />
+        {!!meta.touched && !!meta.error && <div>{meta.error}</div>}
+      </>
+    );
+  };
+
+  const RecommendDevicePathField = (props) => {
+    const {
+      values: { path },
+      touched,
+      setFieldValue,
+    } = useFormikContext();
+    const [field, meta] = useField(props);
+    React.useEffect(() => {
+      if (path.trim() !== '' && touched.path) {
+        setFieldValue(
+          props.name,
+          linuxDrivesNamingIncrement(path, props.index + 1),
+        );
+      }
+    }, [path, touched.path, setFieldValue, props.name, props.index]);
+
+    return (
+      <>
+        <Input {...props} {...field} />
+        {!!meta.touched && !!meta.error && <div>{meta.error}</div>}
+      </>
+    );
  };


As those two components only differs in the way they setFieldValue, they might be factorized.

JBWatenbergScality · 2020-12-17T10:34:22Z

ui/src/ducks/app/volumes.js

-          message: intl.translate('volume_creation_failed', {
-            name: newVolume.name,
-          }),
+          title: 'Volume Form Error',


Translation seems to be missing here

JBWatenbergScality · 2020-12-17T10:41:43Z

ui/src/containers/CreateVolume.js

@@ -54,6 +55,8 @@ const ActionContainer = styled.div`
 const CreateVolumeLayout = styled.div`
  display: inline-block;
  margin-top: ${padding.base};
+  overflow-y: scroll;


Is it intended to display scrollbars by default even for non scrollable content ? You might prefer to use auto instead

JBWatenbergScality · 2020-12-17T10:48:20Z

ui/src/containers/CreateVolume.js

+                              placement="right"
+                              overlay={
+                                <div style={{ minWidth: '200px' }}>
+                                  A devicePath or a partition mount point.


I think there is a missing translation here

JBWatenbergScality · 2020-12-17T10:51:23Z

ui/src/containers/CreateVolume.js

-              createVolume(newVolume);
+              const newVolumes = { ...values };
+              newVolumes.size = `${values.sizeInput}${values.selectedUnit}`;
+              if (newVolumes.multiVolumeCreation) {


This if could be transferred in formatDataForBatchVolumeCreation as it already partially exists there and just missing the else branch to return a one element array

JBWatenbergScality · 2020-12-17T10:59:33Z

ui/src/services/NodeVolumesUtils.js

@@ -276,3 +276,28 @@ export const getVolumeListData = createSelector(
    return nodeVolumes.sort((a, b) => compareHealth(b.health, a.health));
  },
 );
+
+export const formatDataForBatchVolumeCreation = (newVolumes) => {


I would add a simple unit test for the two branches of this function, also as mentioned in the usage of this function I would handle the !multiVolumeCreation case here and return a single entry array containing newVolumes, that would allow a simpler usage of the function as its ouput will always be of the same shape (newVolumes: VolumeCreationFormState) => Volume[] instead of (newVolumes: VolumeCreationFormState) => Volume[] | void

bert-e · 2020-12-17T11:00:14Z

Conflict

There is a conflict between your branch feature/2964-volume-batch-creation and the
destination branch development/2.7.

Please resolve the conflict on the feature branch (feature/2964-volume-batch-creation).

 $ git fetch
 $ git checkout origin/feature/2964-volume-batch-creation
 $ git merge origin/development/2.7
 $ # <intense conflict resolution>
 $ git commit
 $ git push origin HEAD:feature/2964-volume-batch-creation

We KILL breadcrumb! Refs: #2964

Refs: #2964

"overflow-y: scroll" protentially would display the scrollbar by default even for non scrollable content, which casue the issue of we have the scrollbar everywhere. Refs: #2964

Handle the single volume creation case in the utility function instead of in the createVolume component so that the return of this function is always an array. For single volume, return an array with the origin volume. For batch volume, return an array with the formatted volumes. Refs: #2964

Correct the variable declaration too Refs: #2964

bert-e · 2020-12-18T10:57:58Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

Refs: #2964

thomasdanan

@ChengYanJin I like the behavior.
I would change one little thing:
When you create multiple Volumes, the Device Path for each Volume is computed from the Global Device Path field. I think the first Volume should get what you input in the Global Device Path field (i.e. not increment the Device Path of the first Volume).

Also when playing with multiple Volume check box and the number of Volumes, you will observe strange behavior. Maybe some state not properly managed

I wonder if we should disable Global Name and Device Path global fields when creating multiple Volumes because if you override Name or Device Path and then you change the Global field, you will lose all your customizations (@JBWatenbergScality what do you think?)

Also for number increments, I would do 01, 02, 03 etc ... instead of 1, 2, 3 ....

Thanks and well done

bert-e · 2020-12-18T14:16:29Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@thomasdanan

JBWatenbergScality · 2020-12-18T14:57:52Z

@thomasdanan @ChengYanJin From my point of view once the name or path is touched we shouldn't change automatically their values to maintain the user intentions but should still let the ability for the user to change the first volume name and path.

Also the ui might be a little bit misleading as the user is unsure if the default values will actually create a volume. To be precise mentioning /dev/vda by default and then creating one more volume will pre-complete the first entry with /dev/vdb in the current behavior but when submitting the form just /dev/vdb will be created.

The simplest interaction I could think of closed enough to the current implementation would be to remove the default name and path fields when checking Create multiple volumes? and compute the names of the volumes from the first one if subsequent one are not touched.

Ref: #2964

The recommend default device path should start from the global one Refs: #2964

We only recommend the name/path for the empty field, means if the global name/page get updated, the defaults won't change. Refs: #2964

JBWatenbergScality · 2020-12-21T15:59:51Z

ui/src/services/utils.js

+
+/**
+ * This function formats the name base on the index
+ * @param {string} name - The name the default volume name


Remove redundant name The name the default volume

JBWatenbergScality · 2020-12-21T16:00:40Z

ui/src/services/utils.js

+ * @example
+ * const name = 'volume-test'
+ *
+ * const nextDevicePath = linuxDrivesNamingIncrement(name, 1)


const formatedName = formatBatchName(name, 1)

JBWatenbergScality · 2020-12-21T16:32:14Z

ui/src/containers/CreateVolume.js

+                                {intl.translate('number_volume_create')}
+                              </span>
+                              {/* TODO: Generalize the number input component to core-ui. */}
+                              <InputNumberComponentStyle


I think we should handle a max number of volumes to avoid dom freeze in case of the user mistakenly ask 10000 volumes, simply add a max prop here should be enough

Indeed, good point!
I am going to set the max number to70 for the moment.

ChengYanJin · 2020-12-22T16:48:24Z

Hello @thomasdanan @JBWatenbergScality ,

According to the feedback from you and our offline discussions. I've addressed the following issues.

Thomas: When you create multiple Volumes, the Device Path for each Volume is computed from the Global Device Path field. I think the first Volume should get what you input in the Global Device Path field (i.e. not increment the Device Path of the first Volume).

Fix the bug of the Device path, the first volume should be the same as the Global field.

Thomas: Also when playing with multiple Volume check box and the number of Volumes, you will observe strange behavior. Maybe some state not properly managed

Fix the bug, will add interaction test of the form in the following PR.

Thomas: I wonder if we should disable Global Name and Device Path global fields when creating multiple Volumes because if you override Name or Device Path and then you change the Global field, you will lose all your customizations (@JBWatenbergScality what do you think?)

In order to keep the user customization, edit the global value(name/path) should not affect the defaults pre-fill of the touched field.
Unless the users uncheck the box of multi-volume creation and re-check again, we only update the untouched field with the new global value.

Thomas: Also for number increments, I would do 01, 02, 03 etc ... instead of 1, 2, 3 ....

Done.

Set the maximum number of the volume to create 70 to prevent the UI frozen.
The UI should pop up the error message when the input number is bigger than 70 (Waiting for the PR #213 in core-ui to be landed)

Please let me know if you see any improvements/ bugs to be done.
Thank you!

Refs: #2964

Set the max number of the input volume creation to 70, since it frozes the UI when rendering the form. We may even want to lower the max number depents on the use case. Refs: #2964

Refs: #2964

JBWatenbergScality · 2020-12-23T11:03:49Z

ui/src/containers/CreateVolume.js

+                  if (
+                    touched &&
+                    touched.volumes &&
+                    touched?.volumes[i]?.name === true
+                  ) {
+                    continue;
+                  } else {
+                    setFieldValue(`volumes[${i}]name`, '');
+                  }
+                  if (
+                    touched &&
+                    touched.volumes &&
+                    touched?.volumes[i]?.path === true
+                  ) {
+                    continue;
+                  } else {
+                    setFieldValue(`volumes[${i}]path`, '');
+                  }


I think this can be simplified to :

if (!touched?.volumes[i]?.name) { setFieldValue(`volumes[${i}]name`, ''); } if (!touched?.volumes[i]?.path) { setFieldValue(`volumes[${i}]path`, ''); }

Hi @JBWatenbergScality ,

Yes. Good point. But I guess we need to use optional chaining to make sure the Volumes array exists.
Please see:

if (!touched?.volumes?.[i]?.name) { setFieldValue(`volumes[${i}]name`, ''); } if (!touched?.volumes?.[i]?.path) { setFieldValue(`volumes[${i}]path`, ''); }

In order to keep the user customization, edit the global value (name/path) should not affect the defaults pre-fill of the touched field. Unless the users uncheck the box of multi-volume creation and re-check again, we only update the untouched field with the new global value. Refs: #2964

ChengYanJin · 2020-12-23T16:29:15Z

/approve

bert-e · 2020-12-23T16:29:20Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@thomasdanan

The following options are set: approve

Addressed the issues and improvements.

bert-e · 2020-12-23T16:30:16Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.7

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2020-12-27T20:13:01Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.7

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4
development/2.5
development/2.6

Please check the status of the associated issue None.

Goodbye chengyanjin.

ChengYanJin requested review from MonPote, Cuervino, thomasdanan and gdemonet December 14, 2020 11:22

ChengYanJin requested a review from a team as a code owner December 14, 2020 11:22

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from 45a0312 to ec378cf Compare December 14, 2020 15:25

ChengYanJin requested a review from JBWatenbergScality December 16, 2020 14:16

JBWatenbergScality reviewed Dec 17, 2020

View reviewed changes

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from e497e57 to 1bc8143 Compare December 18, 2020 10:46

ChengYanJin added 11 commits December 18, 2020 11:47

ui/styles: Remove the breadcrumb in Create New Volume page

15436b3

We KILL breadcrumb! Refs: #2964

ui/volumes: Set up the form of batch volume creation

79d964d

Refs: #2964

ui/utils: Add utility function to generate the linux drive name

091781a

Refs: #2964

ui/volumes: Add dependents field base on the base field

dc10550

ui/package: Upgrade the version of formik to v2

214b5d1

Refs: #2964

ui/ducks/volumes: Change the payload of createVolume to an array

03e4f76

Refs: #2964

ui/utils: Add util function to format the data for batch volume

a5cf6a1

Refs: #2964

ui/volumes: Adapt the creationVolume container for batch volume creation

f53301d

Refs: #2964

ui/styles: Fix the scroll bar issue by overflow:auto

6b4ee9e

"overflow-y: scroll" protentially would display the scrollbar by default even for non scrollable content, which casue the issue of we have the scrollbar everywhere. Refs: #2964

ui/volumes: Factorize the recommendField

4ecfc4e

Correct the variable declaration too Refs: #2964

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from 1bc8143 to ac5c3ff Compare December 18, 2020 10:57

ui/translation: Add missing translation for batch volume creation

ac5c3ff

Refs: #2964

thomasdanan previously requested changes Dec 18, 2020

View reviewed changes

ui/volumes: Fix the bug of input number doesn't sync with form

2083cee

Ref: #2964

ChengYanJin added 2 commits December 20, 2020 18:13

ui/volumes: Fix the defaults path start from the input path

5cd9a87

The recommend default device path should start from the global one Refs: #2964

ui/volumes: Change the behaviour of the default name/path

ec0309f

We only recommend the name/path for the empty field, means if the global name/page get updated, the defaults won't change. Refs: #2964

JBWatenbergScality reviewed Dec 21, 2020

View reviewed changes

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from 1c36230 to 4c3cbe3 Compare December 22, 2020 16:34

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from 4c3cbe3 to 6622ce5 Compare December 23, 2020 09:06

ChengYanJin added 3 commits December 23, 2020 10:06

ui/volumes: Improve the name of the batch volume

743ce67

Refs: #2964

ui/volumes: Fix the default path bug when the index is over 53

ed0b77c

Set the max number of the input volume creation to 70, since it frozes the UI when rendering the form. We may even want to lower the max number depents on the use case. Refs: #2964

ui/volumes: Set the max number of volume batch creation to 70

1db10a8

Refs: #2964

ChengYanJin requested a review from thomasdanan December 23, 2020 09:06

JBWatenbergScality reviewed Dec 23, 2020

View reviewed changes

ChengYanJin force-pushed the feature/2964-volume-batch-creation branch from 6622ce5 to 5d584ae Compare December 23, 2020 11:40

JBWatenbergScality approved these changes Dec 23, 2020

View reviewed changes

bert-e merged commit 7286a2f into development/2.7 Dec 27, 2020

bert-e deleted the feature/2964-volume-batch-creation branch December 27, 2020 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI] Volume Batch Creation #2981

[UI] Volume Batch Creation #2981

ChengYanJin commented Dec 14, 2020

bert-e commented Dec 14, 2020

bert-e commented Dec 14, 2020

JBWatenbergScality Dec 16, 2020

JBWatenbergScality Dec 16, 2020

JBWatenbergScality Dec 16, 2020

ChengYanJin Dec 17, 2020

JBWatenbergScality Dec 16, 2020

JBWatenbergScality Dec 16, 2020

JBWatenbergScality Dec 17, 2020

JBWatenbergScality Dec 17, 2020

JBWatenbergScality Dec 17, 2020

JBWatenbergScality Dec 17, 2020

JBWatenbergScality Dec 17, 2020

JBWatenbergScality Dec 17, 2020

bert-e commented Dec 17, 2020

bert-e commented Dec 18, 2020

thomasdanan left a comment

bert-e commented Dec 18, 2020

JBWatenbergScality commented Dec 18, 2020 •

edited

Loading

JBWatenbergScality Dec 21, 2020

JBWatenbergScality Dec 21, 2020

JBWatenbergScality Dec 21, 2020

ChengYanJin Dec 22, 2020

ChengYanJin commented Dec 22, 2020 •

edited

Loading

JBWatenbergScality Dec 23, 2020

ChengYanJin Dec 23, 2020

ChengYanJin commented Dec 23, 2020

bert-e commented Dec 23, 2020

bert-e commented Dec 23, 2020

bert-e commented Dec 27, 2020

[UI] Volume Batch Creation #2981

[UI] Volume Batch Creation #2981

Conversation

ChengYanJin commented Dec 14, 2020

bert-e commented Dec 14, 2020

Hello chengyanjin,

bert-e commented Dec 14, 2020

Waiting for approval

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

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

bert-e commented Dec 17, 2020

Conflict

bert-e commented Dec 18, 2020

Waiting for approval

thomasdanan left a comment

Choose a reason for hiding this comment

bert-e commented Dec 18, 2020

Waiting for approval

JBWatenbergScality commented Dec 18, 2020 • edited Loading

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

ChengYanJin commented Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengYanJin commented Dec 23, 2020

bert-e commented Dec 23, 2020

Waiting for approval

bert-e commented Dec 23, 2020

In the queue

bert-e commented Dec 27, 2020

JBWatenbergScality commented Dec 18, 2020 •

edited

Loading

ChengYanJin commented Dec 22, 2020 •

edited

Loading