-
Notifications
You must be signed in to change notification settings - Fork 76
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) Retrieve scheduled appointments in clinical forms workspace #471
base: main
Are you sure you want to change the base?
Conversation
@jnsereko please update the description and add some screenshots or video to provide more context for the reviewers |
The CI seems run endlessly so i've cancelled it |
appointmentKind: string; | ||
appointmentNumber: string; | ||
comments: string; | ||
endDateTime: Date | number; |
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 looks like this property is a Date on the backend. Why are we typing it as Date | number
?
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.
Actually, it returns the number
, neither the ISO string, nor a Date object
src/api/index.ts
Outdated
const filteredAppointments = appointments.filter((appointment) => { | ||
return !appointment.fulfillingEncounters.includes(encounterUuid); | ||
}); | ||
return filteredAppointments.map((appointment) => { | ||
return updateAppointment(url, appointment, encounterUuid, abortController); | ||
}); |
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.
nit: This code would be more concise and easier to read without the explicit return statements
src/api/index.ts
Outdated
appointmentKind: appointment.appointmentKind, | ||
status: appointment.status, | ||
startDateTime: appointment.startDateTime, | ||
endDateTime: appointment.endDateTime.toString(), |
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.
Why do we need toString()
here? If it's a timestamp, isn't it preferable to convert it to a date string using something like toIsoString
or a dayjs
utility function instead?
src/api/index.ts
Outdated
uuid: appointment.uuid | ||
}; | ||
|
||
return openmrsFetch(`${url}`, { |
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 openmrsFetch(`${url}`, { | |
return openmrsFetch(url, { |
src/api/index.ts
Outdated
export const getPatientAppointment = (appointmentUuid: string) => { | ||
return openmrsFetch( | ||
`${restBaseUrl}/appointments/${appointmentUuid}`, | ||
).then(({ data }) => { | ||
if (data) { | ||
return data; | ||
} | ||
return null; | ||
}); | ||
}; |
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.
Would it be useful to have some kind of error handling here? Something along the lines of:
export const getPatientAppointment = (appointmentUuid: string) => { | |
return openmrsFetch( | |
`${restBaseUrl}/appointments/${appointmentUuid}`, | |
).then(({ data }) => { | |
if (data) { | |
return data; | |
} | |
return null; | |
}); | |
}; | |
export const getPatientAppointment = async (appointmentUuid: string) => { | |
try { | |
const response = await openmrsFetch(`${restBaseUrl}/appointments/${appointmentUuid}`); | |
return response.data || null; | |
} catch (error) { | |
console.error('Error fetching appointment:', error); | |
throw error; // Re-throw to allow caller to handle the error | |
} | |
}; |
import { openmrsFetch, restBaseUrl, useOpenmrsSWR } from '@openmrs/esm-framework'; | ||
import dayjs from 'dayjs'; | ||
import useSWR, { mutate, SWRResponse } from 'swr'; | ||
import { type AppointmentsResponse } from '../types'; | ||
import { useCallback, useMemo, useState } from 'react'; |
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 looks like we're not using openmrsFetch
and the SWR imports here. Can we remove them?
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.
Yes @denniskigen, working on it!
Thanks for the catch!
@@ -202,6 +203,26 @@ export class EncounterFormProcessor extends FormProcessor { | |||
critical: true, | |||
}); | |||
} | |||
// handle appointments | |||
try { | |||
const {appointments: myAppointments} = context |
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.
Looks like we're not using this code
const appointmentsResponse = await Promise.all(addFulfillingEncounters(abortController, appointments, savedEncounter.uuid)); | ||
if (appointmentsResponse?.length) { | ||
showSnackbar({ | ||
title: translateFn('appointmentsSaved', 'Appointment(s) saved successfully'), | ||
kind: 'success', | ||
isLowContrast: true, | ||
}); | ||
} |
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.
Should we modify this so that we only proceed if there are appointments?
if (appointments && appointments.length > 0) {
// ...
}
const appointmentsResponse = await Promise.all(addFulfillingEncounters(abortController, appointments, savedEncounter.uuid)); | ||
if (appointmentsResponse?.length) { | ||
showSnackbar({ | ||
title: translateFn('appointmentsSaved', 'Appointment(s) saved successfully'), |
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.
Does translateFrom
handle pluralization? If not, I think we could just default it to plural:
title: translateFn('appointmentsSaved', 'Appointment(s) saved successfully'), | |
title: translateFn('appointmentsSaved', 'Appointments saved successfully'), |
} catch (error) { | ||
const errorMessages = Array.isArray(error) ? error.map((err) => err.message) : [error.message]; | ||
return Promise.reject({ | ||
title: translateFn('errorSavingAppointments', 'Error saving appointment(s)'), |
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.
title: translateFn('errorSavingAppointments', 'Error saving appointment(s)'), | |
title: translateFn('errorSavingAppointments', 'Error saving appointments'), |
} | ||
}; | ||
|
||
const AppointmentsTable = ({ appointments }) => { |
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.
@jnsereko , never create a component inside another component.
const updatedAppointment: AppointmentsPayload = { | ||
fulfillingEncounters: updatedFulfillingEncounters, | ||
serviceUuid: appointment.service.uuid, | ||
locationUuid: appointment.location.uuid, | ||
patientUuid: appointment.patient.uuid, | ||
dateAppointmentScheduled: appointment.startDateTime, | ||
appointmentKind: appointment.appointmentKind, | ||
status: appointment.status, | ||
startDateTime: appointment.startDateTime, | ||
endDateTime: toOmrsIsoString(appointment.endDateTime), | ||
providers: [{ uuid: appointment.providers[0]?.uuid }], | ||
comments: appointment.comments, | ||
uuid: appointment.uuid, |
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.
Isn't this weird that for updating 1 value of the appointment, we need to pass the whole object?
Can this be worked on?
CC: @denniskigen @jnsereko @ibacher @mogoodrich
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.
Can this be worked on?
Agree with you @vasharma05. A nice improvement to do in the backend
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.
@jnsereko , can you create a Backend Ticket for this?
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.
Agree... we should likely have a REST endpoint just for adding a fulfilling encounter to an appointment.
@@ -20,7 +30,11 @@ const WorkspaceLauncher: React.FC<FormFieldInputProps> = ({ field }) => { | |||
isLowContrast: true, | |||
}); | |||
} | |||
launchWorkspace(); | |||
if (field.meta?.handleAppointmentCreation) { |
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 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.
@pirupius @jnsereko, instead of having the check by the workspace name, since workspace names can be changed, I added a check by adding new prop
handleAppointmentCreation
in the fieldmeta
Should it really be optional though @vasharma05
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.
@pirupius , did you have a look here?
appointmentKind: string; | ||
appointmentNumber: string; | ||
comments: string; | ||
endDateTime: Date | number; |
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.
Actually, it returns the number
, neither the ISO string, nor a Date object
Whilst we’re still editing, maybe “(feat) Retrieve scheduled appointments in clinical forms workspace” reads better? |
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.
So I added one comment but didn't review fully yet. This functionality looks really good! But, unfortunately, if I'm following this correctly I think there's a fundamental problem with it. It looks like this sets the "fulfillingEncounter" is set to the encounter when the appointment was scheduled? Is this what is happening? The "fulfillingEncounters" are meant to be set to any encounters that fulfill the encounter... ie, in this case, it should be the encounter that gets created on March 8th 2025 if/when the patient returns that day. It's not used to store the encounter when the appointment was created. See: https://bahmni.atlassian.net/browse/BAH-3239
Am I understanding this correctly?
const updatedAppointment: AppointmentsPayload = { | ||
fulfillingEncounters: updatedFulfillingEncounters, | ||
serviceUuid: appointment.service.uuid, | ||
locationUuid: appointment.location.uuid, | ||
patientUuid: appointment.patient.uuid, | ||
dateAppointmentScheduled: appointment.startDateTime, | ||
appointmentKind: appointment.appointmentKind, | ||
status: appointment.status, | ||
startDateTime: appointment.startDateTime, | ||
endDateTime: toOmrsIsoString(appointment.endDateTime), | ||
providers: [{ uuid: appointment.providers[0]?.uuid }], | ||
comments: appointment.comments, | ||
uuid: appointment.uuid, |
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.
Agree... we should likely have a REST endpoint just for adding a fulfilling encounter to an appointment.
Requirements
Summary
This PR adds ability to
Requirements
cc-ing @vasharma05 @pirupius @ibacher @denniskigen
Screenshots
Screen.Recording.2025-02-27.at.16.10.27.mov
Related Issue
[should be created]
Other