(feat) O3-2793 add actions to transition queue entries in queue table #997
Conversation
chibongho
requested review from
mseaton,
mogoodrich,
ojwanganto,
makombe,
denniskigen,
donaldkibet and
ibacher
|
Size Change: -54.6 kB (-2%) Total Size: 2.97 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
@chibongho - The code for this looks well done. My question is more around the requirements. I generally would not expect to have different menu items to modify each property of a queue entry individually, and I would not want to transition twice if I wanted to transition both the status and the priority of a queue entry.
There are also several additional properties of a Queue Entry that are not handled here, and we need to think about where these would fit in for implementations that might want them, namely locationWaitingFor, providerWaitingFor, and priorityComments.
The way I've been thinking of it is that we need to clearly present 2 basic options - one option is to modify the existing queue entry in place (eg. edit the status or priority, or these other fields, without creating a new queue entry) - and the other is to transition to a new queue entry.
Both of these options could likely use the same form, and just limit allowing one to change the queue itself to the "transition" case.
This likely needs a bit more analysis, but I think having 2 options (eg. Edit and Transition) that both open a form that provides the ability to enter all of the available fields, provides the most flexibility and avoid creating more queue entries than is needed/desired.
| }, | ||
| ); | ||
|
|
||
| export const transferQueueModal = getAsyncLifecycle( |
There was a problem hiding this comment.
Why does the naming call this a Modal and the others are Dialog?
There was a problem hiding this comment.
Prefer Modal for consistency
| import { useTranslation } from 'react-i18next'; | ||
| import { type QueueTableCellComponentProps, type QueueTableColumn } from '../../types'; | ||
|
|
||
| export function QueueTableActionsCell({ queueEntry }: QueueTableCellComponentProps) { |
There was a problem hiding this comment.
Aren't there really 2 different cells here? One is a cell containing a "Transfer" button. The other is a cell containing an "Actions" menu. I could see different implementations wanting one and not the other.
@mogoodrich and I discussed this a little (albeit in a vacuum just between us). The idea is that the chance of us having to change both status and priority at the same time is slim, and that having to transition twice is not that terrible. You brought up a good point with the other possible fields to change (like priorityComments, locationWaitingFor, providerWaitingFor and sortWeight). So maybe we should have one form that's flexible enough to modify any/all fields we want, and also be able to distinguish between a transition and an edit.
What is locationWaitingFor and providerWaitingFor (and why do we need those in addition to which queue they are in?) Is that documented somewhere?
Yeah, I like that. Will need more clarification from Paul/Ciaran on UI to distinguish between edits and transition. |
If you like gory details, most of the discussion around how queues were designed is on Talk. However, the idea was basically that a queue is waiting for a resource; in the discussions, the "resources" that people might be waiting for were: a service (which is what this app defaults to assuming everything is about), a location (specifically something like "Room 123"), or a specific provider. Technically, the queue module was supposed to be an evolution of the patientqueueing module, which always represents the resource the queue is waiting for as a location. |
There was a problem hiding this comment.
Thanks, @chibongho. Your code looks good. I've left a few comments.
| const apiUrl = '/ws/rest/v1/queue' + (locationUuid ? `?location=${locationUuid}` : ''); | ||
| const customRepresentation = | ||
| 'custom:(uuid,display,name,description,allowedPriorities:(uuid,display),allowedStatuses:(uuid,display),location:(uuid,display))'; | ||
| const apiUrl = `/ws/rest/v1/queue?v=${customRepresentation}` + (locationUuid ? `&location=${locationUuid}` : ''); |
There was a problem hiding this comment.
| const apiUrl = `/ws/rest/v1/queue?v=${customRepresentation}` + (locationUuid ? `&location=${locationUuid}` : ''); | |
| const apiUrl = `${restBaseUrl}/queue?v=${customRepresentation}` + (locationUuid ? `&location=${locationUuid}` : ''); |
| const { mutate } = useSWRConfig(); | ||
| const mutateQueueEntries = () => | ||
| mutate((key) => { | ||
| return typeof key === 'string' && key.startsWith(`/ws/rest/v1/queue-entry`); |
There was a problem hiding this comment.
| return typeof key === 'string' && key.startsWith(`/ws/rest/v1/queue-entry`); | |
| return typeof key === 'string' && key.startsWith(`${restBaseUrl}/queue-entry`); |
| }, | ||
| ); | ||
|
|
||
| export const transferQueueModal = getAsyncLifecycle( |
There was a problem hiding this comment.
Prefer Modal for consistency
| <OverflowMenuItem | ||
| itemText={t('transitionStatus', 'Transition status')} | ||
| onClick={() => { | ||
| const dispose = showModal('transition-queue-entry-status-dialog', { | ||
| closeModal: () => dispose(), | ||
| queueEntry, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
You'll likely want to give these OverflowMenuItem components a className which applies a max-width of 'none':
.menuItem {
max-width: none;
}| useConfig: jest.fn(() => ({ | ||
| customPatientChartUrl: 'someUrl', | ||
| })), |
There was a problem hiding this comment.
@brandones introduced an alternative approach for overriding config schemas. Worth checking out.
There was a problem hiding this comment.
Yeah I'd suggest only mocking useConfig if you actually are testing config options. If you just need to get your components to render with the default config, you can just add defineConfigSchema('@openmrs/esm-service-queues-app', configSchema) at the top of the file and the default esm-framework mock will handle it for you. mockedUseConfig.mockReturnValue(getDefaultsFromConfigSchema(configSchema)); is a good alternative if you'll want to test configurability later.
| .then(({ status }) => { | ||
| if (status === 200) { | ||
| showSnackbar({ | ||
| isLowContrast: true, |
There was a problem hiding this comment.
Just as an FYI, this prop can be omitted if you're on the latest version of the framework. It defaults to true if omitted. Same for the success prop.
| invalidText="Required" | ||
| value={formState.selectedQueue} | ||
| onChange={(event) => setSelectedQueueUuid(event.target.value)}> | ||
| {queues?.map((queue) => ( |
There was a problem hiding this comment.
May be worthwhile adding a default state.
There was a problem hiding this comment.
After discussing with @mseaton more, we are consolidating transitions (of queues and/or status) into a single form. I think this means we should default to the currently selected queue (in case the user only wants to transition the status).
| @@ -0,0 +1,78 @@ | |||
| import { openmrsFetch, showSnackbar } from '@openmrs/esm-framework'; | |||
| import '@testing-library/jest-dom'; | |||
There was a problem hiding this comment.
This is already included in setup-tests.ts.
|
|
||
| renderWithSwr(<TransferQueueDialog queueEntry={queueEntry} closeModal={closeModal} />); | ||
| const cancelButton = screen.getByText('Cancel'); | ||
| cancelButton.click(); |
There was a problem hiding this comment.
await user.click(cancelButton);
|
|
||
| return ( | ||
| <div> | ||
| <Form onSubmit={transferQueue}> |
There was a problem hiding this comment.
| <Form onSubmit={transferQueue}> | |
| <Form onSubmit={transferQueue}> | |
| <Stack gap={4}> |
I recommend using a Stack to space out the form fields in all of these modals.
|
PR updated. Addressed PR comments. After discussing with @mseaton, we are also consolidating the transitions into one single form. The form validates that either the status or the queue is different from current values for the submit button to be enabled. |
Requirements
Summary
This PR adds "transition" actions to the queue table. A transition ends a queue entry and immediately starts a new one, albeit with (slightly) different values. This allows for us to keep a history of all queue entries over transitions in the patient's visit. The backend support for this is recently added by this. This PR contains actions to transition the queue entry status, queue entry priority and which queue a patient is on (i.e. a transfer to a different queue).
Screenshots
The modal dialogs for the transitions are very similar to the dialogs already there for queue transfers in the older
<ActiveVisitsTable />(but makes different backend calls).Related Issue
Other