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

4366 - Date validation is inconsistent between inline and admin check(backend) #4641

Merged
merged 35 commits into from Apr 5, 2023

Conversation

Sun-Mountain
Copy link
Contributor

@Sun-Mountain Sun-Mountain commented Mar 27, 2023

Resolves #4366

Description

Date validation is happening between the in-line and admin check is inconsistent. When the admin check is enabled the validation in the page appears to work as expected. The admin check panel:

  • Doesn't catch when Start Dates are later than End Dates
  • Doesn't validate when an invalid year is entered

Chromatic Link

https://www.chromatic.com/build?appId=61d5b948cf6f17003a12bf77&number=1335

Significant changes or possible side effects

Per discussion, valid years are 1960 thru 2151.

Automated test cases written

Given When Then Type (jest, tap, cypress)
Date field with start and end date admin check is on should catch if end date is before start date with appropriate validation error cypress
Date field admin check is one should catch an invalid year with appropriate validation error cypress

Steps to manually verify this change

  1. Login and view an APD. Turn on Admin Check
  2. Navigate to Activity Schedule
  3. Enter dates

This pull request is ready to code review when

  • Automated tests are updated (and all tests are passing)
  • New automated test cases are documented above
  • Chromatic link added above
  • Pull request has been labeled, if applicable with feature, content, bug,
    tests, refactor
  • Associated OpenAPI documentation has been updated
  • The experience passes a basic manual accessibility audit (keyboard nav,
    screenreader, text scaling) OR an exemption is documented

This pull request is ready to test when

  • Code has been reviewed by someone other than the original author

This pull request is ready to review when QA has

  • Verified the functionality related to the change
  • Verified that the change works with Narrator on Windows
  • Verified that the change works with VoiceOver on Mac
  • Verified all updated pages with the WAVE tool
  • Verified tab and keyboard navigation functionality

This pull request can be merged when

  • Design has approved the experience
  • Product has approved the experience

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #4641 (f906e61) into main (e65af77) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4641      +/-   ##
==========================================
+ Coverage   94.37%   94.39%   +0.01%     
==========================================
  Files         281      281              
  Lines        8895     8898       +3     
  Branches     1798     1797       -1     
==========================================
+ Hits         8395     8399       +4     
+ Misses        476      475       -1     
  Partials       24       24              
Flag Coverage Δ
api ∅ <ø> (?)
common 99.33% <100.00%> (+<0.01%) ⬆️
web 94.12% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
common/utils/constants.js 100.00% <100.00%> (ø)
web/src/components/DateField.js 100.00% <100.00%> (+3.22%) ⬆️
...apd/activities/schedule-and-milestones/Schedule.js 96.87% <100.00%> (+3.32%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e65af77...f906e61. Read the comment docs.

@cms-eapd-bot
Copy link

cms-eapd-bot commented Mar 27, 2023

This deploy was cleaned up.

Copy link
Contributor

@thetif thetif left a comment

Choose a reason for hiding this comment

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

Looks good, you probably need to add those +s back to the strings. I'm not sure what crazy way javascript compares a string to a number

web/src/components/DateField.js Outdated Show resolved Hide resolved
web/src/components/DateField.js Outdated Show resolved Hide resolved
Copy link
Contributor

@tbolt tbolt left a comment

Choose a reason for hiding this comment

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

Looks great

web/src/components/DateField.js Outdated Show resolved Hide resolved
@Sun-Mountain Sun-Mountain changed the title 4366 - [Bug] Other State Expense validation triggering on Cancel 4366 - Date validation is inconsistent between inline and admin check(backend) Mar 28, 2023
Copy link
Contributor

@mirano-darren mirano-darren left a comment

Choose a reason for hiding this comment

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

Looks good to me! Further work will be handled in #4661

  • Date correctly wipes if any field is left blank.
  • Admin check shows end year before start year validation message

Copy link
Contributor

@jeromeleecms jeromeleecms left a comment

Choose a reason for hiding this comment

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

This works as described. We will be re-enabling inline validating in 4661 for the state date.

@Sun-Mountain Sun-Mountain merged commit bfbcd7b into main Apr 5, 2023
12 checks passed
@Sun-Mountain Sun-Mountain deleted the nz/4366-date-bug branch April 5, 2023 15:59
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.

[Bug] Date validation is inconsistent between inline and admin check(backend)
6 participants