Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1804852: Pipeline params & resources forms submission formik issue #4377

Merged
merged 2 commits into from Feb 25, 2020
Merged

Bug 1804852: Pipeline params & resources forms submission formik issue #4377

merged 2 commits into from Feb 25, 2020

Conversation

karthikjeeyar
Copy link
Contributor

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

Problem:
After the formik upgrade, error objects has null values in it, which causes the submit button to disable when the fields are edited.
new-pipeline-2 · Details · OKD

Solution:
Skip the null values in the error object while validating for enabling/disabling the submit button

AwesomeScreenshot-2020-2-19-1582135721876

cc: @debsmita1 @rohitkrai03

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 19, 2020
@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Feb 19, 2020
@karthikjeeyar karthikjeeyar changed the title Pipeline params & resources forms submission formik issue Bug 1804852: Pipeline params & resources forms submission formik issue Feb 19, 2020
@openshift-ci-robot
Copy link
Contributor

@karthikjeeyar: This pull request references Bugzilla bug 1804852, 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.

In response to this:

Bug 1804852: Pipeline params & resources forms submission formik issue

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 openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 19, 2020
@andrewballantyne
Copy link
Contributor

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 19, 2020
@christianvogt
Copy link
Contributor

/assign @rohitkrai03

@rohitkrai03
Copy link
Contributor

@karthikjeeyar I thought the problem with null values in errors was happening only when we used useFormikValidationFix() hook in the component to fix the earlier issue?

@rohitkrai03
Copy link
Contributor

@karthikjeeyar I cannot reproduce this in current master.

andrewballantyne
andrewballantyne suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karthikjeeyar why not use useFormikValidationFix(values); in each Form?

As far as I can tell it works out fine.

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Feb 20, 2020 •
edited

Also, can we fix the fact that default is required? That's just not the case. Default is optional by the spec.

@karthikjeeyar
Copy link
Contributor Author

@karthikjeeyar I cannot reproduce this in current master.

@rohitkrai03 Here are the steps to reproduces

  1. Create a pipeline using the yaml editor's default template
  2. Follow the steps shown in this screenshot

multicolumnfield

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/shared Related to console-shared and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 24, 2020
@karthikjeeyar
Copy link
Contributor Author

Updated the PR with useFormikValidationFix(values) in multicolumnfield and also made default as optional field

rohitkrai03
rohitkrai03 approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2020
andrewballantyne
andrewballantyne suggested changes Feb 24, 2020
View reviewed changes
...packages/console-shared/src/components/formik-fields/multi-column-field/MultiColumnField.tsx
}) => {
const { values } = useFormikContext<FormikValues>();
const fieldValue = _.get(values, name, []);
useFormikValidationFix(fieldValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this put a requirement on name being the one that is changed? What if you have a MultiColumnField with multiple required fields?

Test this by re-enabling the requirement on default. I suspect if you have a name, but edit default to be empty you can still submit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewballantyne multicoulmField internally uses FieldArray from formik which accepts an key and value as array(array of objects).
eg:
{ parameters: [{name: 'test', description: '', default: 'value'}] }
here the name key will be parameters/resources, the values will be the array of objects which can support multiple required fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the name of the whole formik field itself that manages the specific part of the form state. It was there before as well. Nothing to do with specific input fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see - it's actually the values... I saw "name" and I assumed it was the field name, not the fieldArray name... We are good 😄 my bad.

I had a name vs 'name' problem.

andrewballantyne
andrewballantyne approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/assign @christianvogt

Christian is needed for the approval label, but this looks good to me. This should solve our validation issues.

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, karthikjeeyar, 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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 485427d into openshift:master Feb 25, 2020
@openshift-ci-robot
Copy link
Contributor

@karthikjeeyar: All pull requests linked via external trackers have merged. Bugzilla bug 1804852 has been moved to the MODIFIED state.

In response to this:

Bug 1804852: Pipeline params & resources forms submission formik issue

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.

@karthikjeeyar
Copy link
Contributor Author

/cherrypick release-4.4

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Feb 25, 2020
Merged
@openshift-cherrypick-robot
Copy link

@karthikjeeyar: new pull request created: #4466

In response to this:

/cherrypick release-4.4

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.

@spadgett spadgett added this to the v4.5 milestone Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@andrewballantyne andrewballantyne andrewballantyne approved these changes

@christianvogt christianvogt Awaiting requested review from christianvogt

@sahil143 sahil143 Awaiting requested review from sahil143

Assignees

@rohitkrai03 rohitkrai03

@andrewballantyne andrewballantyne

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console component/shared Related to console-shared kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@karthikjeeyar @openshift-ci-robot @andrewballantyne @christianvogt @rohitkrai03 @openshift-bot @openshift-cherrypick-robot @spadgett @openshift-merge-robot