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

refactor: migrate date validator to ts #749

Merged
merged 9 commits into from
Dec 8, 2020
Merged

refactor: migrate date validator to ts #749

merged 9 commits into from
Dec 8, 2020

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Nov 30, 2020

Problem

Part of #7

Solution

Improvements

  • Migrates Date validation to Typescript

Tests

  • Create two date fields. Hide one of them behind logic.
  • Check that validation on both visible and hidden date fields work when submitting programatically
  • Date input is rejected if it is not in the format 'DD MMM YYYY'
  • Date input is rejected if it is not within the validation range (isFutureOnly, isPastOnly, Custom Min / Max)

@tshuli tshuli mentioned this pull request Nov 30, 2020
Merged
2 tasks
karrui
karrui reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

some preliminary questions

src/app/utils/field-validation/validators/dateValidator.ts Outdated
const { selectedDateValidation, customMinDate, customMaxDate } =
dateField.dateValidation || {}

const isFutureOnly = selectedDateValidation === 'Disallow past dates'
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the enum DateSelectedValidation instead of hardcoded string? So this won't erroneously pass even if the enum has changed; but then the frontend is not using that enum...........

Can you also create an issue to track this? Move the enum to shared and make frontend also rely on that enum instead of DATE_VALIDATION_OPTIONS in src/public/modules/forms/admin/controllers/edit-fields-modal.client.controller.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edited + created issue

src/shared/util/date-validation.ts Outdated
* @param date
* @returns a moment with the date in the format 'DD MMM YYYY'
*/
export const createMomentFromDateString = (date: string): moment.Moment => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason this is in shared? Seems to be only used in dateValidator.ts and can probably be inlined there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shifted to inline

@karrui
Copy link
Contributor

karrui commented Nov 30, 2020

also can add typescript tests for all the migrated validators also? do the date one in this PR and do the others in a separate PR ba

liangyuanruo
liangyuanruo suggested changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

To address karrui's reviews.

@tshuli tshuli mentioned this pull request Dec 2, 2020
Closed
@tshuli tshuli force-pushed the refactor/date-validator branch from 0ae2692 to 2c5d369 Compare December 2, 2020 03:27
@tshuli
Copy link
Contributor Author

tshuli commented Dec 2, 2020

@karrui On the tests, I think the plan was to update them after the validator has been migrated and running in production for at least a week

@tshuli tshuli requested review from karrui and liangyuanruo December 2, 2020 03:29
@karrui
Copy link
Contributor

karrui commented Dec 2, 2020

@karrui On the tests, I think the plan was to update them after the validator has been migrated and running in production for at least a week

Don't need migrate them yet, just add new tests in typescript first

@tshuli tshuli force-pushed the refactor/date-validator branch from 2c5d369 to 49b6c93 Compare December 2, 2020 04:25
@tshuli tshuli force-pushed the refactor/date-validator branch 2 times, most recently from 37139c1 to b1d8260 Compare December 5, 2020 04:12
@tshuli tshuli marked this pull request as draft December 5, 2020 05:07
@tshuli tshuli force-pushed the refactor/date-validator branch from b1d8260 to c7befe4 Compare December 7, 2020 02:34
@tshuli tshuli marked this pull request as ready for review December 7, 2020 03:46
@tshuli tshuli requested a review from mantariksh December 7, 2020 03:46
@tshuli
Copy link
Contributor Author

tshuli commented Dec 7, 2020

@karrui actually I looked at the tests again and I couldn't think of any other test cases to add - any to suggest?
The ones for hidden fields on encrypt mode submissions will be added in a different PR

mantariksh
mantariksh approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

nice, minor refactors suggested

src/app/utils/field-validation/validators/dateValidator.ts Outdated Show resolved Hide resolved
src/app/utils/field-validation/validators/dateValidator.ts Outdated Show resolved Hide resolved
@tshuli tshuli force-pushed the refactor/date-validator branch from b7d21cd to 327744f Compare December 7, 2020 12:21
@tshuli
Copy link
Contributor Author

tshuli commented Dec 7, 2020

@liangyuanruo for approval

liangyuanruo
liangyuanruo approved these changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

LGTM - could you also elaborate further why we subtract 12h or add 14h?

@tshuli tshuli merged commit 2704df2 into develop Dec 8, 2020
@mantariksh mantariksh mentioned this pull request Dec 10, 2020
Merged
@liangyuanruo liangyuanruo deleted the refactor/date-validator branch January 26, 2021 13:53
@tshuli tshuli mentioned this pull request Feb 9, 2021
Closed
@yong-jie yong-jie mentioned this pull request Apr 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantariksh mantariksh mantariksh approved these changes

@liangyuanruo liangyuanruo liangyuanruo approved these changes

@karrui karrui Awaiting requested review from karrui

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tshuli @karrui @liangyuanruo @mantariksh