Introduce field validation to Create container dialog #1354

skobyda · 2023-07-13T11:15:21Z

Fixes #609

This introduces both dynamic validation run with "onBlur" and static validation when form is submitted:

Dynamic validation

Screencast.from.2023-07-13.13-16-41.webm

On Form submit validation:

Screencast.from.2023-07-13.13-17-47.webm

marusak · 2023-07-13T11:59:46Z

Not a review, just very quickly noticed on screenshot that you require everything but for example for ports that is not the case. From man pages: --publish, -p=[[ip:][hostPort]:]containerPort[/protocol]

Similarly for volumes you don't have to specify source. (Not sure if we want to support that 🤷‍♂️ )

Also when 'required' appears it breaks the [-] button alignment. And there is isRequired prop which adds * to fields, we should use that one as well.

skobyda · 2023-07-25T12:23:29Z

Not a review, just very quickly noticed on screenshot that you require everything but for example for ports that is not the case. From man pages: --publish, -p=[[ip:][hostPort]:]containerPort[/protocol]

Similarly for volumes you don't have to specify source. (Not sure if we want to support that man_shrugging )

Also when 'required' appears it breaks the [-] button alignment. And there is isRequired prop which adds * to fields, we should use that one as well.

Fixed

garrett · 2023-08-07T15:34:06Z

Please refer to the form design documentation:

https://www.patternfly.org/components/forms/form/design-guidelines/#errors-and-validation

Helper Text for error messages is not supposed to have icons, as there are icons in the forms
Be more descriptive than "required", when it's possible.
Are all of these actually required? There should be a few with some defaults, like the port. If you specify one, it should default to the other, I thought, as long as there is one port? IP address is optional too; I thought it'll default to 0.0.0.0 (localhost) when the entry is blank. If a value isn't needed, then we shouldn't show that it's required.
Anything that's required should be marked with a required asterisk. (I thought we did this. A few things have it already.)

An aside:

The - button is misaligned... and shouldn't be a secondary button; it should be a trash icon

skobyda · 2023-09-12T12:32:48Z

Updated the PR to use debounce, now just waiting for cockpit-project/cockpit#19251 to land

garrett

Looks good overall! Thanks!

I did find a couple of things that could be handled in follow-ups.

Trash can icon is misaligned when there are elements pushing it vertically.

But this can be handled in a follow-up.

I get an error of file not found in the autocomplete widget, but not as a message. Is this intended?

This can also be a follow-up.

martinpitt

Thanks! I now see where you want to go with the field validation.

n_m needs a refresh for reposchutz, but we can ignore that for now, as it will bitrot exceedingly fast. Let's resolve the code issues first, and then we can refresh it as the last thing (we can even do this via the workflow now).

src/util.js

src/PublishPort.jsx

src/ImageRunModal.jsx

src/PublishPort.jsx

src/util.js

src/ImageRunModal.jsx

test/check-application

src/ImageRunModal.jsx

martinpitt

Thanks! A few remaining small issues, and I also had a look at the coverage report.

src/util.js

package.json

src/PublishPort.jsx

src/Volume.jsx

src/Env.jsx

Clears a single field in validationFailed object

Fixes cockpit-project#609

martinpitt

🎉 Cheers!

cockpituous · 2023-10-18T12:09:53Z

src/Env.jsx

+        break;
+    case "envValue":
+        break;
+    default:


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/Env.jsx

+            if (!envKey || !envVar) {
+                continue;


These 2 added lines are not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/Env.jsx

+            >
+                <TextInput id={id + "-value"}
+                       value={item.envValue || ''}
+                       validated={validationFailed?.envValue ? "error" : "default"}


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/Env.jsx

+                       validated={validationFailed?.envValue ? "error" : "default"}
+                       onChange={(_event, value) => {
+                           utils.validationClear(validationFailed, "envValue", onValidationChange);
+                           utils.validationDebounce(() => onValidationChange({ ...validationFailed, envValue: validateEnvVar(value, "envValue") }));


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/ImageRunModal.jsx

+        const containerNameValidation = await this.validateContainerName(containerName);
+
+        if (containerNameValidation)
+            validationFailed.containerName = containerNameValidation;


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/ImageRunModal.jsx

                           {_("Create and run")}
                       </Button>
-                       <Button variant='secondary' id="create-image-create-btn" onClick={() => this.onCreateClicked(false)} isDisabled={!image && selectedImage === ""}>
+                       <Button variant='secondary' id="create-image-create-btn" onClick={() => this.onCreateClicked(false)} isDisabled={(!image && selectedImage === "") || this.isFormInvalid(dialogValues.validationFailed)}>


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:53Z

src/PublishPort.jsx

+
+        break;
+    }
+    default:


This added line is not executed by any test. Details

cockpituous · 2023-10-18T12:09:54Z

src/Volume.jsx

+            return _("Container path must not be empty");
+
+        break;
+    default:


This added line is not executed by any test. Details

skobyda marked this pull request as draft July 13, 2023 11:15

skobyda requested a review from garrett July 13, 2023 11:19

skobyda force-pushed the validateImageRun branch 4 times, most recently from 108e1b8 to 68cb803 Compare July 25, 2023 12:22

skobyda mentioned this pull request Aug 28, 2023

lib: Add validation to DynamicListForm cockpit-project/cockpit#19251

Merged

skobyda force-pushed the validateImageRun branch 2 times, most recently from fc6254f to 2489fce Compare August 30, 2023 11:20

skobyda force-pushed the validateImageRun branch 2 times, most recently from a9f37bd to 9f74a06 Compare September 12, 2023 12:04

skobyda added the blocked label Sep 12, 2023

skobyda force-pushed the validateImageRun branch 4 times, most recently from 607719f to b4c4a7e Compare October 10, 2023 11:04

skobyda marked this pull request as ready for review October 10, 2023 11:04

skobyda force-pushed the validateImageRun branch from b4c4a7e to 5fedc1c Compare October 10, 2023 11:08

skobyda removed the blocked label Oct 10, 2023

skobyda force-pushed the validateImageRun branch 7 times, most recently from 48971d2 to ce1ba8a Compare October 16, 2023 13:07

skobyda force-pushed the validateImageRun branch from f28eebb to b927b71 Compare October 17, 2023 15:00

garrett previously approved these changes Oct 18, 2023

View reviewed changes

martinpitt requested changes Oct 18, 2023

View reviewed changes

skobyda dismissed garrett’s stale review via 8016309 October 18, 2023 09:24

skobyda force-pushed the validateImageRun branch 3 times, most recently from f50e3f7 to beb14b8 Compare October 18, 2023 09:57

github-advanced-security bot found potential problems Oct 18, 2023

View reviewed changes

src/ImageRunModal.jsx Fixed Show fixed Hide fixed

src/ImageRunModal.jsx Fixed Show fixed Hide fixed

skobyda force-pushed the validateImageRun branch from beb14b8 to f200e65 Compare October 18, 2023 10:01

skobyda requested a review from martinpitt October 18, 2023 10:01

skobyda force-pushed the validateImageRun branch 3 times, most recently from 001012e to 52a46fb Compare October 18, 2023 10:35

martinpitt added the node_modules label Oct 18, 2023

martinpitt temporarily deployed to npm-update October 18, 2023 10:38 — with GitHub Actions Inactive

github-actions bot removed the node_modules label Oct 18, 2023

skobyda force-pushed the validateImageRun branch from 52a46fb to 6efd18e Compare October 18, 2023 10:44

martinpitt requested changes Oct 18, 2023

View reviewed changes

src/util.js Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/PublishPort.jsx Outdated Show resolved Hide resolved

src/Volume.jsx Outdated Show resolved Hide resolved

src/Env.jsx Outdated Show resolved Hide resolved

src/Env.jsx Show resolved Hide resolved

skobyda added 7 commits October 18, 2023 13:49

utils: Add validationClear helper function

64f578d

Clears a single field in validationFailed object

Validate Port mapping fields in Create container dialog

47b1aef

Validate volumes fields in Create container dialog

5d09e89

Move EnvVar component to a separate file

9a030ca

Validate Environment variables fields in Create container dialog

a028096

Validate of container name is already in use

4fc1086

Fixes cockpit-project#609

Update pixel test reference

6cd6364

skobyda force-pushed the validateImageRun branch from 6efd18e to 6cd6364 Compare October 18, 2023 11:54

martinpitt approved these changes Oct 18, 2023

View reviewed changes

cockpituous reviewed Oct 18, 2023

View reviewed changes

martinpitt merged commit 332f835 into cockpit-project:main Oct 18, 2023
18 checks passed

skobyda mentioned this pull request Oct 19, 2023

Few issues with Container create validation #1459

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce field validation to Create container dialog #1354

Introduce field validation to Create container dialog #1354

skobyda commented Jul 13, 2023 •

edited

Loading

marusak commented Jul 13, 2023

skobyda commented Jul 25, 2023

garrett commented Aug 7, 2023

skobyda commented Sep 12, 2023

garrett left a comment

martinpitt left a comment

martinpitt left a comment

martinpitt left a comment

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

cockpituous Oct 18, 2023

Introduce field validation to Create container dialog #1354

Introduce field validation to Create container dialog #1354

Conversation

skobyda commented Jul 13, 2023 • edited Loading

Dynamic validation

On Form submit validation:

marusak commented Jul 13, 2023

skobyda commented Jul 25, 2023

garrett commented Aug 7, 2023

skobyda commented Sep 12, 2023

garrett left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a 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

skobyda commented Jul 13, 2023 •

edited

Loading