Bug 1879875: Fix that the Start Pipeline form is not always validated correctly

[jerolimov]

Fixes:
https://issues.redhat.com/browse/ODC-4364
https://bugzilla.redhat.com/show_bug.cgi?id=1879875

Analysis / Root cause:
When switching between different volume types in start pipeline modal and switching back to a valid volume type the "start" button is still disabled. This happen because Formik does not remove the errors when the user switches from "Empty Directory" to "Secret" and "Empty Directory" again.

The error was set to an invalidate state because the form was validated multiple times with the old and the new form fields.

To check how often and in which order the schema validation was called, you can add a console.warn (for example) output to volumeTypeSchema in frontend/packages/dev-console/src/components/pipelines/modals/common/validation-utils.ts. Like this one:

const volumeTypeSchema = yup
  .object()
  .when('type', {
-    is: (type) => VolumeTypes[type] === VolumeTypes.Secret,
+    is: (type) => {
+      console.warn('Validate pipeline form', type);
+      return VolumeTypes[type] === VolumeTypes.Secret,
+    }
    then: yup.object().shape({

Solution Description:
The problem is based on the order of changes to the form. The Workspace type dropdown uses the DropdownField onChange prop to clear the workspace.data. But the DropdownField itself sets the type field after this change. Both method calls automatically triggers a form validation.

See DropdownField onChange:

console/frontend/packages/console-shared/src/components/formik-fields/DropdownField.tsx

Lines 34 to 38 in 910f7ce

	onChange={(value: string) => {
	props.onChange && props.onChange(value);
	setFieldValue(props.name, value);
	setFieldTouched(props.name, true);
	}}

The form was also additional changed each time when the value of a DropdownField changes:

console/frontend/packages/console-shared/src/components/formik-fields/DropdownField.tsx

Line 17 in 910f7ce

useFormikValidationFix(field.value);

All these calls triggers an asynchronous form validation with old and new data which confuses the form error state.

As solution I added an 3rd parameter to the setFieldValue and setFieldTouched calls to disable that the fields are marked as "needs revalidation". The revalidation was now only triggered by the already used useFormikValidationFix hook.

⚠️ Component compatibility and alternatives

Alternative 1
Component tree:

  StartPipelineTree
    ..
    PipelineWorkspacesSection
      console-shared/formik-fields/components/DropdownField (to select the workspace type)
        console/internal/components/Dropdown
      MultipleResourceKeySelector (if workspace type = ConfigMap or Secret)

Also if this is a really small code change this affects the widely used DropdownField component.

As alternative its also possible to change just the PiplelineWorkspacesSection component instead. Instead of using the DropdownField this component could directly use the Dropdown to keep the same UI, but handle the formik state (calling setFieldValue in onChange) itself.

@divyanshiGupta WDYT?

Alternative 2
When starting with the analyse of this issue, I expected that the problem cause is that the data was removed and the errors for removed data wasn't re-validated. My initial idea was then to not save the sub-data (config / secretMap) in one data field and use different sub-keys instead.

This also fixes that the entered data was removed when the user switches between the types. I expected that this is also a (small) issue because we have different issues like ODC-2443 which says that form fields shouldn't be cleared when switching such a type selection.

But after implementing this I noticed that the original issue (could not submit form with Empty Directory after selecting ConfigMap) still exists. This also requires this code change I proposed with this PR here, or the Alternative 1.

The much bigger change looks like this: master...jerolimov:odc-4364-fix-datadrop

@divyanshiGupta WDYT? Should we change this here as well or should we change / fix this after we start 4.7?

Screen shots / Gifs for design review:

Unit test coverage report:
Unchanged

Test setup:
Unchanged

Browser conformance:

• Chrome
• Firefox
• Safari
• Edge

[openshift-ci-robot]

@jerolimov: This pull request references Bugzilla bug 1879875, 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.6.0) matches configured target release for branch (4.6.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 1879875: Fix that the Start Pipeline form is not always validated correctly

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.

[jerolimov]

/assign @divyanshiGupta

[jerolimov]

/kind bug

[divyanshiGupta]

@jerolimov I think explicitly setting the revalidation prop to false in DropdownField setFieldValue and setFieldTouched should be fine since we are already doing the validation using useFormikValidationFix hook or either we keep it as true and remove the hook, anyways the validation should happen only once when the field value changes.

I have tested these changes against a few usecases including this one and it seemed to work fine without breaking anything. I will also recommend you to test it against other usecases just to be sure.

@rohitkrai03 wdyt?

[andrewballantyne]

I also investigated this, and I agree with what Christoph as come up with.

I'll wait until EOD Monday before giving this a LGTM. @rohitkrai03 please carve out a moment on Monday to think about this; we need to start closing PRs for the coming Code Freeze so this cannot stay open much longer.

[rohitkrai03]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jerolimov, rohitkrai03

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/packages/console-shared/OWNERS [andrewballantyne,rohitkrai03]
• frontend/packages/dev-console/OWNERS [andrewballantyne,rohitkrai03]

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]

@jerolimov: All pull requests linked via external trackers have merged:

• openshift/console#6673

Bugzilla bug 1879875 has been moved to the MODIFIED state.

In response to this:

Bug 1879875: Fix that the Start Pipeline form is not always validated correctly

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.

[jerolimov]

/cherry-pick release-4.5

[openshift-cherrypick-robot]

@jerolimov: new pull request created: #6853

In response to this:

/cherry-pick release-4.5

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.