(fix) O3-3013 Appointment Form should validate invalid time values #1069
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
|
Size Change: +120 B (0%) Total Size: 3.1 MB ℹ️ View Unchanged
|
mseaton
commented
Mar 29, 2024
| } from '@openmrs/esm-framework'; | ||
| import { convertTime12to24 } from '@openmrs/esm-patient-common-lib'; |
There was a problem hiding this comment.
The use of this function from patient-common-lib was a big part of the problem, as it is what is responsible for (silently) returning a value of "midnight" if the converted time fails to match the expected format. This gets rid of the usage of this and adds the appropriate handling directly.
There was a problem hiding this comment.
@ibacher @denniskigen assumedly we should ticket and fix the convertTime12to24 as well, unless for some reason that behavior is expected?
| const time12HourFormatRegexPattern = '(1[0-2]|0?[1-9]):[0-5][0-9]'; | ||
| function isValidTime(timeStr) { | ||
| return timeStr.match(new RegExp(time12HourFormatRegexPattern)); | ||
| } |
There was a problem hiding this comment.
Simple validation function to check the time format - used within the carbon field component to control whether the "invalid time" message appears, and within the form schema to control whether the form submission is allowed.
| @@ -66,7 +68,7 @@ const appointmentsFormSchema = z.object({ | |||
| recurringPatternPeriod: z.number(), | |||
| recurringPatternDaysOfWeek: z.array(z.string()), | |||
| selectedDaysOfWeekText: z.string().optional(), | |||
| startTime: z.string(), | |||
| startTime: z.string().refine((value) => isValidTime(value)), | |||
There was a problem hiding this comment.
This seems to be needed to prevent the form submission if the value fails validation
| pattern="([\d]+:[\d]{2})" | ||
| pattern={time12HourFormatRegexPattern} | ||
| invalid={!isValidTime(value)} | ||
| invalidText={t('invalidTime', 'Invalid time')} |
There was a problem hiding this comment.
I don't quite know what the pattern property here accomplishes. It doesn't seem to have much effect. The invalid and invalidText seem to be what is needed to display a validation error below the field if the input value is invalid.
There was a problem hiding this comment.
fwiw, the pattern attribute is explained here: https://react.carbondesignsystem.com/?path=/docs/components-timepicker--overview
On quick glance, it seems like it is supposed to do what the invalid function is doing? I have found that React Carbon elements don't have the greatest documentation (in this case, perhaps the pattern works in conjunction with warning, just a guess?) so if we can't get this to work and are relying on invalid, probably just remove the pattern attribute entirely to avoid future confusion.
There was a problem hiding this comment.
pattern seems to be what the browser uses to check pattern on form submission. https://www.w3schools.com/TAGS/att_input_pattern.asp
I'm guessing invalid can be set whenever, and doesn't need to be tied to form submission action.
| const [hours, minutes] = convertTime12to24(startTime, timeFormat); | ||
| const hoursAndMinutes = startTime.split(':').map((item) => parseInt(item, 10)); | ||
| const hours = (hoursAndMinutes[0] % 12) + (timeFormat === 'PM' ? 12 : 0); | ||
| const minutes = hoursAndMinutes[1]; |
There was a problem hiding this comment.
This logic is essentially what was in the convertTime12to24 method without the erroneous handling of invalid values.
ojwanganto
approved these changes
Mar 29, 2024
mogoodrich
approved these changes
Mar 29, 2024
| } from '@openmrs/esm-framework'; | ||
| import { convertTime12to24 } from '@openmrs/esm-patient-common-lib'; |
There was a problem hiding this comment.
@ibacher @denniskigen assumedly we should ticket and fix the convertTime12to24 as well, unless for some reason that behavior is expected?
| pattern="([\d]+:[\d]{2})" | ||
| pattern={time12HourFormatRegexPattern} | ||
| invalid={!isValidTime(value)} | ||
| invalidText={t('invalidTime', 'Invalid time')} |
There was a problem hiding this comment.
fwiw, the pattern attribute is explained here: https://react.carbondesignsystem.com/?path=/docs/components-timepicker--overview
On quick glance, it seems like it is supposed to do what the invalid function is doing? I have found that React Carbon elements don't have the greatest documentation (in this case, perhaps the pattern works in conjunction with warning, just a guess?) so if we can't get this to work and are relying on invalid, probably just remove the pattern attribute entirely to avoid future confusion.
chibongho
approved these changes
Mar 29, 2024
| pattern="([\d]+:[\d]{2})" | ||
| pattern={time12HourFormatRegexPattern} | ||
| invalid={!isValidTime(value)} | ||
| invalidText={t('invalidTime', 'Invalid time')} |
There was a problem hiding this comment.
pattern seems to be what the browser uses to check pattern on form submission. https://www.w3schools.com/TAGS/att_input_pattern.asp
I'm guessing invalid can be set whenever, and doesn't need to be tied to form submission action.
senthil-athiban
pushed a commit
to senthil-athiban/openmrs-esm-patient-management
that referenced
this pull request
Apr 15, 2024
denniskigen
added a commit
that referenced
this pull request
Aug 8, 2024
…e left empty (#1182) * (fix) fix typo on recurring appointment label (#1047) * (fix) service queues - make edit queue entry modal body scrollable if content is too long (#1042) * (fix) O3-2471 recommended visit type tab should only show if configured to do so (#1045) * (fix) service queues - unescape queue name when passed into t() (#1043) Co-authored-by: Jayasanka Weerasinghe <33048395+jayasanka-sack@users.noreply.github.com> * (refactor) O3-2891: Refactor Patient Appointments ESM into Appointments ESM (#1048) * (feat) re-use upcomming appointment work to mark appointments as `Checked In` (#1050) * (chore) Update translations from Transifex (#1011) Co-authored-by: OpenMRS Bot <infrastructure@openmrs.org> Co-authored-by: Mark Goodrich <mgoodrich@pih.org> * (feat) use configured identifier type on appointment table (#1054) * (fix) add `@openmrs/esm-patient-common-lib` as peer dependency and register appointment workspace correctly (#1053) * (fix): Enhanced Tablet View: Service Queues Metrics Header Overhaul (#1046) * (fix): Tablet-UI. Making Tablet Service-queues metrics header responsive on tablet mode Signed-off-by: Joshua Murithi <brodiamyzalius@gmail.com> * Fixup --------- Signed-off-by: Joshua Murithi <brodiamyzalius@gmail.com> Co-authored-by: Dennis Kigen <kigen.work@gmail.com> * (chore) Update translations from Transifex (#1055) Co-authored-by: OpenMRS Bot <infrastructure@openmrs.org> Co-authored-by: Mark Goodrich <mgoodrich@pih.org> * Enhanced Monthly Calendar UI for Appointment Scheduler (#1049) * Enhanced Monthly Calendar UI for Appointment Scheduler * clicking on a day from within the month to change the appointment date * Localized days in weeks * Removed weekly calendar view on appointment scheduler --------- Co-authored-by: Donald Kibet <chelashawdonald@yahoo.com> * (BREAKING) O3-2748 service queues - add ability to undo transitions and void queue entries (#1052) * (BREAKGING) O3-2748 service queues - add ability to undo transitions and void queue entries * add tests * minor wording fixup * remove unused imports * change undo transition route per update in backend PR * address PR comments * (chore) O3-2989 service queues - refactor <PatientSearch> to not have things unrelated to patient serach (#1058) * (feat) O3-2892: Add Patient-Centric View within the Appointments App (#1057) * O3-2788 (fix) appointments table should correctly search by patient name or identifier (#1067) * (fix) O3-3009 Appointment form should limit location to those tagged as 'Appointment Location' (#1066) * (fix) O3-3013 Appointment Form should validate invalid time values (#1069) * (fix) O3-3013 appointment time validation should match entire string (#1073) * (feature) O3-2930: Add date of birth to patient details in Appointments Module Co-authored-by: Mark Goodrich <mgoodrich@pih.org> * (feat) O3-3025: Add validation in change status modal for service queues (#1076) * (feat) O3-3025: Add validation in change status modal for service queues * (test) updated test to reflect changes * (fix) O3-2748 service queue - fix NPE in undo transition dialog (#1077) * (fix) Fix queue filtering by location (#1078) * (chore) Separate linting and formatting concerns (#1080) * (feat) O3-3030 - make new queue table support creating new queue entry (#1079) * (feat) O3-3030 - make new queue table support creating new queue entry * fix metrics * address PR comment * (fix):O3-3039-Fix Incorrect Client Selection Issue When Navigating Pages in Queue Table (#1082) * (refactor) O3-3012: Switch registering appointments form workspace ba… (#1068) * (fix) Fix error for undifined attribute type (#1083) * (test) O3-3005: Add E2E test for registering an unknown patient (#1070) Co-authored-by: Dennis Kigen <kigen.work@gmail.com> * (fix) Handle null start date on patient list details (#1081) * Handle null start date on list details * Removed unnecessary optional chaining --------- Co-authored-by: Dennis Kigen <kigen.work@gmail.com> * (chore) Update translations from Transifex (#1059) Co-authored-by: OpenMRS Bot <infrastructure@openmrs.org> Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> * (refactor) O3-2685: Inconsistent / Duplicated Appointments List UI between Home App and Appointments App (#1084) * (chore) Fix Prettier glob patterns * O3-3030 service queues: fix NPE in mapVisitQueueEntryProperties (#1085) * O3-3030 service queues: fix NPE in mapVisitQueueEntryProperties * change default rep string for queue entries to include visit encounters --------- Co-authored-by: Makombe Kennedy <kennedymakombe@gmail.com> * (test) Remove mock implementations of usePagination (#1088) * (test) Remove mock implementations of usePagination * Fixup * (chore) Bump playwright (#1090) * O3-3043 - (feat) service queues - add new view of queue entries showin… (#1086) * O3-3043 - (feat) service queues - add new view of queue entries showing one table per status * move queue-table-for-all-statuses into view/ * (chore) Flag rules-of-hooks lint violations (#1091) * (chore) Flag rules-of-hooks lint violations * Fix e2e tests * (chore) Update translations from Transifex (#1087) Co-authored-by: OpenMRS Bot <infrastructure@openmrs.org> Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> * (feat) O3-3063: UI tweaks for the Today's appointments widget (#1092) * (feat) UI tweaks for the Today's appointments widget * Appointments dashboard fixes * (fix) O3-3063: Fix appointments table empty state text (#1094) * (feat) O3:3063: Appointments dashboard UI tweaks (#1096) * (fix) Fix appointment metrics error state * Enhanced relationship display logic to differentiate 'Patient' roles (#1089) * (fix): improve client side form validation * (fix): error validation * Fixup --------- Signed-off-by: Joshua Murithi <brodiamyzalius@gmail.com> Co-authored-by: Donald Kibet <chelashawdonald@yahoo.com> Co-authored-by: chibongho <cbho@pih.org> Co-authored-by: Michael Seaton <mseaton@pih.org> Co-authored-by: Jayasanka Weerasinghe <33048395+jayasanka-sack@users.noreply.github.com> Co-authored-by: Mark Goodrich <mgoodrich@pih.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: OpenMRS Bot <infrastructure@openmrs.org> Co-authored-by: Joshua Murithi <brodiamyzalius@gmail.com> Co-authored-by: Dennis Kigen <kigen.work@gmail.com> Co-authored-by: Gabriel Mbatha Ngao <gabriel.mbatha@gmail.com> Co-authored-by: sparsh989 <151712330+sparsh989@users.noreply.github.com> Co-authored-by: Makombe Kennedy <kennedymakombe@gmail.com> Co-authored-by: Daud Kakumirizi <dkakumirizii@gmail.com> Co-authored-by: Pedro Sousa - ICRC <68599335+icrc-psousa@users.noreply.github.com> Co-authored-by: Ian <52504170+ibacher@users.noreply.github.com> Co-authored-by: Piyush Mishra <91911367+piyushmishra1416@users.noreply.github.com>
# for free
to join this conversation on GitHub.
Already have an account?
# 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
If you enter an invalid time in the time field input when scheduling an appointment, nothing prevents the form submission, and the invalid time is discarded and the time is set to midnight on the appointment. This is problematic, particularly since times like "11" might be entered, which are invalid, and are just silently saved as midnight.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-3013