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.

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

🐛 [release-0.2] Fix state management and validation of date pickers in migration wave modal #1110

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Jul 11, 2023

Currently, the date pickers in the create/edit migration wave modal misbehave when the user attempts to enter a date string manually.

  • The required MM/DD/YYYY format is not enforced.
  • Invalid date formats are coerced to the nearest matching date, which leads to inaccurate validation running before the user is done typing.
  • Entering a start date that is out of the allowable range (before the current day) disables the field.

The problem seemed to mostly stem from the fact that these dates were stored in form state as Date objects, which made it impossible to reference the user's entered string in validation if it was formatted incorrectly. This PR changes the form state to use strings instead of Dates, updates the validation accordingly, and transforms the strings into the correct format on submit.

Note also that the interdependent validation has been changed: before, there was a validation error on the start date if it was after the end date AND a validation error on the end date if it was before the start date. Since these validations have been moved to use .when and .test (because .min with yup.ref is unreliable when the value could be in an invalid format), yup gives an error if there is a circular dependency in validation. To address this, only the end date field is validated to enforce the requirement that it is after the start date, and if the start date is changed, validation is re-triggered on the end date.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

❗ No coverage uploaded for pull request base (release-0.2@27b0f53). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@              Coverage Diff               @@
##             release-0.2    #1110   +/-   ##
==============================================
  Coverage               ?   46.95%           
==============================================
  Files                  ?      177           
  Lines                  ?     4432           
  Branches               ?      987           
==============================================
  Hits                   ?     2081           
  Misses                 ?     2337           
  Partials               ?       14           
Flag Coverage Δ
unitests 46.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

LGTM

@mturley mturley merged commit e0b0db3 into konveyor:release-0.2 Jul 11, 2023
@mturley mturley deleted the release-0.2-datepicker-fix branch July 11, 2023 19:00
Copy link
Contributor

@gildub gildub left a comment

Choose a reason for hiding this comment

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

LGTM and works well.
Thanks @mturley

mturley added a commit that referenced this pull request Jul 11, 2023
…tion wave modal (#1111)

Ports #1110 forward to main.

I implemented this fix first on the release-0.2 branch in anticipation
that it would be different for the PF4 and PF5 DatePicker components,
since it is a high priority for the release. However, this is a pretty
straightforward cherry-pick (with conflicts resolved) because the fix
required turned out to be the same for PF5.

Signed-off-by: Mike Turley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants