-
Notifications
You must be signed in to change notification settings - Fork 214
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
(feat) O3-2180 & O3-2300: Allow creating past visits and set visit stop date and time in the form #1704
Conversation
…en multiple conditions
Size Change: -40.2 kB (0%) Total Size: 11 MB
ℹ️ View Unchanged
|
Requesting PR review here @denniskigen @ibacher |
const openVisitForm = useCallback(() => { | ||
launchPatientWorkspace('start-visit-workspace-form', { | ||
isCreatingVisit: true, | ||
workspaceTitle: t('addPastVisit', 'Add past visit'), | ||
}); | ||
}, [patientUuid, launchPatientChart]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency array no longer seems accurate.
return { originalModule, showModal: jest.fn() }; | ||
jest.mock('@openmrs/esm-patient-common-lib', () => { | ||
const originalModule = jest.requireActual('@openmrs/esm-patient-common-lib'); | ||
return { originalModule, launchPatientWorkspace: jest.fn() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { originalModule, launchPatientWorkspace: jest.fn() }; | |
return { ...originalModule, launchPatientWorkspace: jest.fn() }; |
|
||
const dateTimeSchema = z | ||
.object({ | ||
date: z.date().max(new Date()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems slightly risky to have multiple new Date()
instances, i.e., both here and in the refine()
call. It would probably be good to have them be consistent.
"editPastVisit": "Edit Past Visit", | ||
"editPastVisit": "Edit past visit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a whole bunch of translation changes like this (i.e., shifts in capitalization). It would be nice to have an explanation for why these are part of this PR at all, especially because it's unclear why in some cases we are removing capitalizations ("Edit Past Visit" -> "Edit past visit") and in others we are adding them in ("Mark as alive" -> "Mark As Alive")
onClose={() => setError(datetimeFieldName, null)} | ||
statusIconDescription="notification" | ||
subtitle={errors?.[datetimeFieldName]?.root?.message} | ||
title={t('datetimeOutOfRange', 'Selected time is out of range')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would seem useful here to tell the user what the time range is?
@@ -78,15 +83,15 @@ | |||
"hide": "Hide", | |||
"indication": "Indication", | |||
"invalidTimeFormat": "Invalid time format", | |||
"invalidVisitStartDate": "Start date needs to be on or before {{firstEncounterDatetime}}", | |||
"invalidVisitStopDate": "Visit stop date time cannot be on or before visit start date time", | |||
"invalidVisitStartDate": "Start date needs to be before {{firstEncounterDatetime}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"on or before" is more accurate.
@@ -17,3 +22,12 @@ export type VisitFormData = { | |||
[x: string]: string; | |||
}; | |||
}; | |||
|
|||
export function getDateTimeFormat(datetime: Date | string): [Date, string, 'AM' | 'PM'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should just be part of the component that uses it.
export function getDateTimeFormat(datetime: Date | string): [Date, string, 'AM' | 'PM'] { | ||
const dayJsDatetime = dayjs(datetime); | ||
return [ | ||
dayJsDatetime.hour(0).minute(0).second(0).millisecond(0).toDate(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dayJsDatetime.hour(0).minute(0).second(0).millisecond(0).toDate(), | |
dayJsDatetime.startOf('day')).toDate(), |
} | ||
|
||
const StartVisitForm: React.FC<StartVisitFormProps> = ({ | ||
patientUuid, | ||
closeWorkspace, | ||
promptBeforeClosing, | ||
visitToEdit, | ||
showVisitEndDateTimeFields, | ||
isCreatingVisit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this variable name obscure. When is isCreatingVisit
meant to be true
? Since we separate out editing already, i.e., what's the difference between creating a visit and starting a visit?
Closing this PR, since this will be a part of RDE flow and needs more input on the design side. |
Requirements
Summary
This PR allows the user to create a new past visit for a patient.
Screenshots
Screen.Recording.2024-03-01.at.12.28.29.mov
Related Issue
https://openmrs.atlassian.net/browse/O3-2180
Also solves https://openmrs.atlassian.net/browse/O3-2300
Other