-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(edit appointment): implement Edit Appointment #1845
feat(edit appointment): implement Edit Appointment #1845
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/hospitalrun/hospitalrun-frontend/fafryjun5 |
<Label htmlFor="patientTypeahead" text={t('scheduling.appointment.patient')} /> | ||
<Typeahead | ||
id="patientTypeahead" | ||
disabled={!isEditable || patient !== undefined} |
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 line appears to be why the Travis build is failing, because it doesn't recognize disabled
as a prop. I recently added disabled
in HospitalRun/components#290. Is Travis running it with an older version of the components library?
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.
Seems like it, since GitHub actions passed. @tehkapa can you check this out?
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.
Awesome feature 🎉
One other bug:
The tool bar button on the /appointments
route is showing the internationalized key rather than the value.
export const createAppointment = (appointment: Appointment, history: any): AppThunk => async ( | ||
dispatch, | ||
) => { | ||
dispatch(createAppointmentStart()) | ||
await AppointmentRepository.save(appointment) | ||
dispatch(createAppointmentSuccess()) | ||
history.push('/appointments') | ||
} | ||
|
||
export const updateAppointment = (appointment: Appointment, history: any): AppThunk => async ( | ||
dispatch, | ||
) => { | ||
dispatch(updateAppointmentStart()) | ||
const updatedAppointment = await AppointmentRepository.saveOrUpdate(appointment) | ||
dispatch(updateAppointmentSuccess(updatedAppointment)) | ||
history.push(`/appointments/${updatedAppointment.id}`) | ||
} |
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.
We should probably have success alerts for successful create/edit. I think we can open new issues to accomplish this, though.
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.
Yeah, I wasn't sure if we wanted Toasts for everything or which ones we wanted them for.
let newErrorMessage = '' | ||
if (isBefore(new Date(appointment.endDateTime), new Date(appointment.startDateTime))) { | ||
newErrorMessage += ` ${t('scheduling.appointment.errors.startDateMustBeBeforeEndDate')}` | ||
} | ||
|
||
if (newErrorMessage) { | ||
setErrorMessage(newErrorMessage.trim()) | ||
return | ||
} |
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 code is duplicated with the New Appointment save logic.
It seems like we should be able to use the same logic in both places.
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, but I'm not sure where it should go. It's the same for New Patient / Edit Patient.
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'm not sure either, right now. Let's think on this and we can figure out the right abstraction later.
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.
Once we have two complete modules with new/view/edit/delete, maybe we should make a point to look over the code in general with an eye for code that's duplicated as I'm sure there are a few more instances. Just now I can think of the Save/Cancel buttons, Appointment and Patient objects used in tests, getFriendlyId, form field change handlers...
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 agree! We're getting close to that point, if not at that point when this PR gets merged.
<PrivateRoute | ||
isAuthenticated={ | ||
permissions.includes(Permissions.WriteAppointments) && | ||
permissions.includes(Permissions.ReadAppointments) | ||
} | ||
exact | ||
path="/appointments/edit/:id" | ||
component={EditAppointment} |
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.
Could we add some tests around this in HospitalRun.test.tsx
. Just looking for tests around the security and then making sure the component renders when the route is accessed.
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.
Related to this, do you think it should require any Patient permissions for this route?
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.
Probably since there is patient data that can be seen...
We probably need a ReadPatient
and ReadPatients
permission. New Appointments and Edit Appointments should require ReadPatients
maybe?
I think that is outside the scope of this issue though.
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.
Ok, added tests.
let newErrorMessage = '' | ||
if (isBefore(new Date(appointment.endDateTime), new Date(appointment.startDateTime))) { | ||
newErrorMessage += ` ${t('scheduling.appointment.errors.startDateMustBeBeforeEndDate')}` | ||
} | ||
|
||
if (newErrorMessage) { | ||
setErrorMessage(newErrorMessage.trim()) | ||
return | ||
} |
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'm not sure either, right now. Let's think on this and we can figure out the right abstraction later.
This should also be fixed now. |
@MatthewDorner I've accepted 2 PRs and there's been an "avalanche" of conflict. Sorry, dude! |
Should be fixed now. Not sure what's going on with the tests now, but they're passing for me locally (Ubuntu). |
Fixes #1807
Changes proposed in this pull request:
createAppointment
from appointments-slice to appointment-slicecreateAppointmentSuccess
action to setisLoading
to false and be consistent withcreatePatient
Typeahead
for patient instead of conditional typeahead / text field