Bug 1829445: Fixes: Container image does not validate dynamically #5231

sahil143 · 2020-04-29T14:31:04Z

Fixes: https://issues.redhat.com/browse/ODC-3178

Analysis / Root cause:

container image validation only happens on blur, which caused some issue

Solution Description:

enable image validation while user types

Screen shots / Gifs for design review:

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-04-29T14:37:45Z

@sahil143: This pull request references Bugzilla bug 1829445, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1829445: Fixes: Container image does not validate dynamically

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

invincibleJai · 2020-04-29T17:12:17Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx

-              kind: 'DockerImage',
-              name: _.trim(searchTermImage),
+  const handleSearch = React.useCallback(
+    _.debounce((newValues: any) => {


nit: type here

rohitkrai03 · 2020-04-29T21:35:17Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx

@@ -151,8 +136,8 @@ const ImageSearch: React.FC = () => {
        helpText={getHelpText()}
        helpTextInvalid={helpTextInvalid}
        validated={validated}
-        onBlur={() => {
-          handleSearch();
+        onKeyUp={() => {


Why not use onChange here?

rohitkrai03 · 2020-04-30T19:36:48Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx

@@ -125,10 +120,6 @@ const ImageSearch: React.FC = () => {
    <span>{values.searchTerm === '' ? 'Required' : values.isi.status?.message}</span>
  );

-  React.useEffect(() => {


This would break edit application scenario. We need this logic here.

rohitkrai03 · 2020-04-30T19:37:31Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx

-        onBlur={() => {
-          handleSearch();
+        onChange={(e: KeyboardEvent) => {
+          handleSearch((e.target as HTMLInputElement).value);
          setFieldTouched('searchTerm', true);


Pretty sure you don't need to do that now since default onBlur on the field will handle this.

rohitkrai03 · 2020-04-30T19:40:00Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx


  const handleSave = (name: string) => {
    setNewImageSecret(name);
-    values.searchTerm && handleSearch();


I think this one is being used to trigger the validation again when a pull secret is added. So we need this one as well.

rohitkrai03 · 2020-05-05T15:22:38Z

/assign

christianvogt · 2020-05-06T04:09:39Z

The gif looks odd to me.
Firstly you are typing fairly quick and yet it still started validating.

Secondly as soon as the user inputs a value which no longer matches that which is currently validating or that which has already errored I think that the helper text should be cleared immediately.

Please involve UX for approval.

sahil143 · 2020-05-06T07:21:25Z

/cc @serenamarie125 @beaumorley @openshift/team-devconsole-ux

sahil143 · 2020-05-06T10:08:16Z

updated Gif

rohitkrai03

/lgtm

serenamarie125

This looks great thanks @sahil143 !

christianvogt · 2020-05-06T17:30:32Z

/lgtm cancel

I can submit the form after immediately changing the image name to something that is invalid. The form submits with the previous valid value.

The field state should be immediately invalidated on change of the image name value.

sahil143 · 2020-05-07T11:09:11Z

/lgtm cancel

I can submit the form after immediately changing the image name to something that is invalid. The form submits with the previous valid value.

The field state should be immediately invalidated on change of the image name value.

Fixed. @christianvogt Please review again

christianvogt · 2020-05-07T16:16:07Z

@sahil143 while the create button is properly disabled, the helper text remains while typing. This helper text should also be cleared.

sahil143 · 2020-05-07T17:41:34Z

@sahil143 while the create button is properly disabled, the helper text remains while typing. This helper text should also be cleared.

fixed!

christianvogt

I also noticed that after entering a valid image, then changing to a new valid image, the old app name and name values remain which no longer coincide with the new image. They should be modified again if the user never touched those fields. I'm ok if we tackle this in a new issue though

christianvogt · 2020-05-07T18:17:03Z

frontend/packages/dev-console/src/components/import/image-search/ImageSearch.tsx

-          handleSearch();
-          setFieldTouched('searchTerm', true);
+        onChange={(e: KeyboardEvent) => {
+          setFieldValue('disableSubmit', true);


I really don't think this field is necessary. We were able to have a disabled submit button without it. You should be resetting the values previously set by the error or success conditions. See what is done in the catch of the search.

disabled submit button is based on only two props i.e. dirty and errors. Not sure how we can disable the button based on these and in catch block we are only setting the image stream data to empty values and error.

The submit button disabled state is controlled by the validation of the form data. By deleting the required image values the form will be in an invalid state, thereby disabling the submit button.

By not having a valid image, the submit button will be disabled. So setting the values back to empty (as they were on initializing the form) and any other state necessary, we get the desired outcome.

We can remove the disableSubmit prop from everywhere and here we can set setFieldValue('isi', {}); so the form will be disabled when user starts to type and if there is no valid image in the isi

sahil143 · 2020-05-08T16:39:24Z

I also noticed that after entering a valid image, then changing to a new valid image, the old app name and name values remain which no longer coincide with the new image. They should be modified again if the user never touched those fields. I'm ok if we tackle this in a new issue though

fixed! added a method to reset the fields based on form type

abhi-kn · 2020-05-08T17:47:35Z

/bugzilla refresh

openshift-ci-robot · 2020-05-08T17:47:39Z

@abhi-kn: This pull request references Bugzilla bug 1829445, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…lidates fix changes

christianvogt · 2020-05-08T19:37:07Z

/lgtm
/approve

openshift-ci-robot · 2020-05-08T19:37:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: beaumorley, christianvogt, rohitkrai03, sahil143, serenamarie125

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-05-09T00:49:45Z

@sahil143: All pull requests linked via external trackers have merged: openshift/console#5231. Bugzilla bug 1829445 has been moved to the MODIFIED state.

In response to this:

Bug 1829445: Fixes: Container image does not validate dynamically

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from debsmita1 and jcaianirh April 29, 2020 14:31

openshift-ci-robot added component/dev-console Related to dev-console component/shared Related to console-shared labels Apr 29, 2020

sahil143 changed the title ~~Fixes: Container image does not validate dynamically~~ Bug 1829445: Fixes: Container image does not validate dynamically Apr 29, 2020

openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 29, 2020

invincibleJai reviewed Apr 29, 2020

View reviewed changes

sahil143 force-pushed the import-flow-validation branch from 921bc0a to b0543ed Compare April 29, 2020 17:26

rohitkrai03 suggested changes Apr 29, 2020

View reviewed changes

sahil143 force-pushed the import-flow-validation branch from b0543ed to 290f902 Compare April 30, 2020 14:27

rohitkrai03 suggested changes Apr 30, 2020

View reviewed changes

sahil143 force-pushed the import-flow-validation branch from 290f902 to 3ca0209 Compare May 4, 2020 06:17

openshift-ci-robot assigned rohitkrai03 May 5, 2020

openshift-ci-robot requested review from beaumorley and serenamarie125 May 6, 2020 07:21

sahil143 added 2 commits May 6, 2020 14:49

create a reusable hook for debounce useDebounceCallback

3f2c848

enable image search validation on keypress

355ac27

sahil143 force-pushed the import-flow-validation branch from 5033c43 to 355ac27 Compare May 6, 2020 09:19

rohitkrai03 approved these changes May 6, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

serenamarie125 approved these changes May 6, 2020

View reviewed changes

beaumorley approved these changes May 6, 2020

View reviewed changes

openshift-ci-robot assigned christianvogt May 6, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

sahil143 force-pushed the import-flow-validation branch from 18f2bcb to fe8095b Compare May 7, 2020 15:06

sahil143 force-pushed the import-flow-validation branch from fe8095b to 705578a Compare May 7, 2020 17:39

christianvogt reviewed May 7, 2020

View reviewed changes

sahil143 force-pushed the import-flow-validation branch from 705578a to bf17a1c Compare May 8, 2020 16:32

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels May 8, 2020

add reset fields , change the name and application name once image va…

468ff5c

…lidates fix changes

sahil143 force-pushed the import-flow-validation branch from bf17a1c to 468ff5c Compare May 8, 2020 19:10

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2020

openshift-merge-robot merged commit b1ee685 into openshift:master May 9, 2020

spadgett added this to the v4.5 milestone May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1829445: Fixes: Container image does not validate dynamically #5231

Bug 1829445: Fixes: Container image does not validate dynamically #5231

sahil143 commented Apr 29, 2020

openshift-ci-robot commented Apr 29, 2020

invincibleJai Apr 29, 2020

rohitkrai03 Apr 29, 2020

sahil143 Apr 30, 2020

rohitkrai03 Apr 30, 2020

rohitkrai03 Apr 30, 2020

rohitkrai03 Apr 30, 2020

rohitkrai03 commented May 5, 2020

christianvogt commented May 6, 2020

sahil143 commented May 6, 2020

sahil143 commented May 6, 2020

rohitkrai03 left a comment

serenamarie125 left a comment

christianvogt commented May 6, 2020

sahil143 commented May 7, 2020

christianvogt commented May 7, 2020

sahil143 commented May 7, 2020

christianvogt left a comment

christianvogt May 7, 2020

sahil143 May 7, 2020

christianvogt May 7, 2020 •

edited

karthikjeeyar May 8, 2020 •

edited

sahil143 May 8, 2020

sahil143 commented May 8, 2020

abhi-kn commented May 8, 2020

openshift-ci-robot commented May 8, 2020

christianvogt commented May 8, 2020

openshift-ci-robot commented May 8, 2020

openshift-ci-robot commented May 9, 2020

Bug 1829445: Fixes: Container image does not validate dynamically #5231

Bug 1829445: Fixes: Container image does not validate dynamically #5231

Conversation

sahil143 commented Apr 29, 2020

openshift-ci-robot commented Apr 29, 2020

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

rohitkrai03 commented May 5, 2020

christianvogt commented May 6, 2020

sahil143 commented May 6, 2020

sahil143 commented May 6, 2020

rohitkrai03 left a comment

Choose a reason for hiding this comment

serenamarie125 left a comment

Choose a reason for hiding this comment

christianvogt commented May 6, 2020

sahil143 commented May 7, 2020

christianvogt commented May 7, 2020

sahil143 commented May 7, 2020

christianvogt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt May 7, 2020 • edited

Choose a reason for hiding this comment

karthikjeeyar May 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 commented May 8, 2020

abhi-kn commented May 8, 2020

openshift-ci-robot commented May 8, 2020

christianvogt commented May 8, 2020

openshift-ci-robot commented May 8, 2020

openshift-ci-robot commented May 9, 2020

christianvogt May 7, 2020 •

edited

karthikjeeyar May 8, 2020 •

edited