(fix) O3-3982: Validate against starting a visit at a future time #2013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @denniskigen here is the PR. we can follow from |
denniskigen
changed the title
denniskigen
force-pushed
the
O3-3982
branch
from
d1566bc
to
7361790
Compare
denniskigen
reviewed
Sep 24, 2024
| @@ -464,70 +445,66 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
| .pipe(first()) | |||
| .subscribe({ | |||
| next: (response) => { | |||
| if (response.status === 201) { | |||
There was a problem hiding this comment.
openmrsFetch handles checking the response headers for us, so explicitly checking the status as we're doing here isn't necessary.
denniskigen
force-pushed
the
O3-3982
branch
from
a4ea943
to
aa0d778
Compare
njiddasalifu
commented
Sep 25, 2024
denniskigen
force-pushed
the
O3-3982
branch
2 times, most recently
from
6ff6226
to
000bd97
Compare
denniskigen
reviewed
Sep 27, 2024
| @@ -121,59 +121,120 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
| }); | |||
|
|
|||
| const displayVisitStopDateTimeFields = useMemo( | |||
| () => visitToEdit?.stopDatetime || showVisitEndDateTimeFields, | |||
| () => Boolean(visitToEdit?.stopDatetime) || showVisitEndDateTimeFields, | |||
There was a problem hiding this comment.
This ensures displayVisitStopDateTimeFields always returns a boolean.
| onChange={([date]) => onChange(date)} | ||
| value={value ? dayjs(value).format('DD/MM/YYYY') : null} | ||
| > | ||
| <DatePickerInput | ||
| id={`${dateFieldName}Input`} | ||
| invalid={Boolean(errors[dateFieldName])} |
There was a problem hiding this comment.
The invalid prop expects a boolean.
| @@ -78,26 +78,24 @@ const VisitDateTimeField: React.FC<VisitDateTimeFieldProps> = ({ | |||
| render={({ field: { onBlur, onChange, value } }) => ( | |||
| <TimePicker | |||
| id={timeFieldName} | |||
| invalid={Boolean(errors[timeFieldName])} | |||
| @@ -121,58 +121,73 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({ | |||
| }); | |||
|
|
|||
| const displayVisitStopDateTimeFields = useMemo( | |||
| () => visitToEdit?.stopDatetime || showVisitEndDateTimeFields, | |||
| () => Boolean(visitToEdit?.stopDatetime || showVisitEndDateTimeFields), | |||
There was a problem hiding this comment.
We want to ensure that this variable is always a boolean.
Comment on lines
+187
to
+190
| .refine((data) => validateStartTime(data), { | ||
| message: t('futureStartTime', 'Visit start time cannot be in the future'), | ||
| path: ['visitStartTime'], | ||
| }); |
There was a problem hiding this comment.
This is the key change. It runs whatever value gets filled into the Visit start time input against validateStartTime. If the start time is in the future, the validation error message above is rendered.
Comment on lines
+234
to
+235
| // TODO: Figure out why this test is failing | ||
| xit('displays an error message when the visit start time is in the future', async () => { |
There was a problem hiding this comment.
I don't know why this test is failing. It mimics the user interaction of filling in the field with a value that ought to trigger the validation (similar to the test just above).
denniskigen
requested review from
NethmiRodrigo,
jayasanka-sack and
mogoodrich
This PR adds validation logic that ensures a visit cannot be started at a time in the future. It does so by adding a refinement to the visitFormSchema zod schema that compares the value of the visitStartTime field with the current timestamp. If the visitStartTime is in the future, a validation error message will be shown under the field, and the form will not be submitted. The error message shown reads "Visit start time cannot be in the future". Additionally, this PR refactors the zod schema, centralizing some of the validation logic into reusable helper functions that are hopefully more easier to maintain. It also adds some test coverage for the new validation logic.
vasharma05
approved these changes
Oct 2, 2024
packages/esm-patient-chart-app/src/visit/visit-form/visit-date-time.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-form/visit-date-time.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx
Outdated
Show resolved
Hide resolved
|
Thanks for the review, @vasharma05! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR adds validation logic that ensures a visit cannot be started at a time in the future. It does so by adding a refinement to the
visitFormSchemazod schema that compares the value of thevisitStartTimefield with the current timestamp. If thevisitStartTimeis in the future, a validation error message will be shown under the field, and the form will not be submitted. The error message shown reads "Visit start time cannot be in the future".Screenshots
visit-start-time.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-3982
Other