(feat) - O3-3096 service queues - configuration-driven queue table #1114
Conversation
chibongho
requested review from
mseaton,
mogoodrich,
ojwanganto,
makombe,
denniskigen,
donaldkibet and
ibacher
|
Size Change: -317 kB (-8%) ✅ Total Size: 3.53 MB
ℹ️ View Unchanged
|
| // There might be futher changes pending future design of the config schema. | ||
| const { defaultStatusConceptUuid, defaultTransitionStatus } = concepts ?? ({} as any); | ||
|
|
||
| const columns = [ |
There was a problem hiding this comment.
This (and similar columns arrays) are no longer hard-coded since they should be configuration-defined.
| return columns; | ||
| } | ||
|
|
||
| function getColumnFromDefinition(t: TFunction, columnDef: ColumnDefinition): QueueTableColumn { |
There was a problem hiding this comment.
Big switch statement here to map columnType (defined in config) to actual QueueTableColumns that are used to render header and cells of a column in the DataTable. The switch statement enumerates all the predefined columns we support. Implementators wishing to have a custom column can use the extension-column columnType.
| // returns the columns to display for a queue table of a particular queue + status. | ||
| // For a table displaying all entries of a particular queue, the status param should be null | ||
| // For a table displaying all entries from all queues, both params should be null | ||
| export function useColumns(queue: string, status: string): QueueTableColumn[] { |
There was a problem hiding this comment.
Function to determine the columns to display for a queue table of a particular queue + status
| import vitalsConfigSchema, { type VitalsConfigObject } from './current-visit/visit-details/vitals-config-schema'; | ||
| import biometricsConfigSchema, { | ||
| type BiometricsConfigObject, | ||
| } from './current-visit/visit-details/biometrics-config-schema'; | ||
|
|
||
| export const defaultTablesConfig: TablesConfig = { |
There was a problem hiding this comment.
Definition of default columns to show in the queue table of the service queues app main page.
| tableDefinitions: [ | ||
| { | ||
| columns: ['patient-name', 'priority', 'comingFrom', 'status', 'wait-time', 'actions'], | ||
| appliedTo: { queue: '3113f164-68f0-11ee-ab8d-0242ac120002' }, |
There was a problem hiding this comment.
2 different TableDefinitions here. The first one is for illustration purpose, to show that we can define the columns for a specific queue. The second one applies to all tables.
There was a problem hiding this comment.
@chibongho looks great. A few comments. I see this is still in Draft - would be good to get a sense of what is remaining before moving it into a state that could be merged.
| { | ||
| id: 'priority', | ||
| columnType: 'priority-column', | ||
| config: [], |
There was a problem hiding this comment.
Do we need to specify config here if the array is empty? Also, we may want to always have config be an object for future flexibility and overall consistency. If the only option right now is an array, we can just make that the only property on the config
| @@ -129,6 +180,11 @@ export const configSchema = { | |||
| _default: '', | |||
| _description: 'Custom URL to load default facility if it is not in the session', | |||
| }, | |||
| tablesConfig: { | |||
| _type: Type.Object, | |||
| _description: 'TODO', | |||
| { | ||
| id: 'status', | ||
| columnType: 'status-column', | ||
| config: [], |
There was a problem hiding this comment.
Same comment here as the one for priority
packages/esm-service-queues-app/src/queue-table/cells/columns.resource.ts
Show resolved
Hide resolved
|
|
||
| switch (columnType) { | ||
| case 'patient-name-column': { | ||
| return queueTableNameColumn(id, header ?? t('name', 'Name')); |
There was a problem hiding this comment.
The custom header here should also be run through the translation function, no? Potentially also with another optional property on columnDef to indicate which module the header translation string is defined in?
There was a problem hiding this comment.
ugh... not that difficult, but this is a bit more annoying than I hoped (the i18n namespace needs to be specified when calling useTranslation, which means I need to pass the module name in before the t function is generated). Filed https://openmrs.atlassian.net/browse/O3-3138
There was a problem hiding this comment.
There's also the framework's translateFrom() function, which might be easier than maintaining two t functions (since presumably the defaults should still come from this namespace).
| import React from 'react'; | ||
| import { type PatientIdColumnConfig } from '../../config-schema'; | ||
| import { type QueueEntry, type QueueTableColumnFunction, type QueueTableCellComponentProps } from '../../types'; | ||
|
|
There was a problem hiding this comment.
Id -> Identifier search/replace in this file
| @@ -172,9 +172,7 @@ | |||
| "program": "Program", | |||
There was a problem hiding this comment.
We probably don't actually want these message strings to be removed, and there may be some we need to see added from the different built-in columns. There is a trick here that you need to do to ensure that the build scripts behave correctly - you can see how this is done in the root component of patient-chart-app. Look in the codebase for a comment that reads something like: DO NOT REMOVE THIS COMMENT and THE TRANSLATION KEYS AND VALUES USED IN THE COMMON LIB IS WRITTEN HERE
There was a problem hiding this comment.
I got around this by changing the extract-translations git hook to read in *.ts files.
@mseaton There are a few more |
chibongho
changed the title
|
This is now ready for review. Thanks. |
There was a problem hiding this comment.
Looks great to me @chibongho . I'm approving, but there are some minor changes to be made, mostly where you've copied/pasted code around and failed to rename everything. I don't need to re-review assuming you clean up the comments indicated.
I would be good to get at least one or two reviews from some of the more experienced react developers
| headerModule?: string; // optional custom i18n translation module for the column's header. Must be used with the header option | ||
| } & ( | ||
| | { columnType: 'patient-name-column' } | ||
| | { columnType: 'patient-identifier-column'; config: PatientIdentifierColumnConfig } |
There was a problem hiding this comment.
This could probably be optional if we want, and if the type is not supplied, there is some default behavior (eg. get the first preferred identifier, or whatever is the primary identifier in the rest response, etc)
| import dayjs from 'dayjs'; | ||
| import { type QueueTableColumnFunction, type QueueTableCellComponentProps } from '../../types'; | ||
|
|
||
| export const QueueTableWaitTimeCell = ({ queueEntry }: QueueTableCellComponentProps) => { |
There was a problem hiding this comment.
This should be renamed to QueueTablePatientAgeCell
| export const queueTablePatientAgeColumn: QueueTableColumnFunction = (key, header) => ({ | ||
| key, | ||
| header, | ||
| CellComponent: QueueTableWaitTimeCell, |
There was a problem hiding this comment.
Same, rename once above is renamed
| ) => { | ||
| const { identifierType } = config; | ||
|
|
||
| const getPatientId = (queueEntry: QueueEntry) => { |
There was a problem hiding this comment.
getPatientId -> getPatientIdentifier
|
|
||
| const getPatientId = (queueEntry: QueueEntry) => { | ||
| for (const identifier of queueEntry.patient.identifiers) { | ||
| if (identifier.identifierType?.uuid == identifierType) { |
There was a problem hiding this comment.
We likely want to check beyond this. If a patient has > 1 identifier of the same type, then we should prioritize a) any that are "preferred = true" first, then likely the most recently created second. Ideally the API could give you this, but otherwise should be done here.
| return null; | ||
| }; | ||
|
|
||
| const QueueTablePatientIdCell = ({ queueEntry }: QueueTableCellComponentProps) => { |
There was a problem hiding this comment.
QueueTablePatientIdCell -> QueueTablePatientIdentifierCell
|
|
||
| // reprevents a column showing which queue a queue entry belongs to | ||
| export const QueueTableQueueCell = ({ queueEntry }: QueueTableCellComponentProps) => { | ||
| return <>{queueEntry.queue.display}</>; | ||
| }; | ||
|
|
||
| export const queueTableQueueColumn: QueueTableColumn = (t) => ({ | ||
| header: t('queue', 'Queue'), | ||
| export const queueTableQueueColumn: QueueTableColumnFunction = (key, header) => ({ |
There was a problem hiding this comment.
This is really queueName right? Let's rename the component to this
|
|
||
| export const queueTableVisitStartTimeColumn: QueueTableColumnFunction = (key, header) => { | ||
| function getVisitStartTime(queueEntry: QueueEntry) { | ||
| return queueEntry.visit.startDatetime; |
There was a problem hiding this comment.
visit is theoretically nullable on queueEntry, so may want to make this a ?.
| return queueEntry.visit.startDatetime; | ||
| } | ||
|
|
||
| const QueueTablePriorityCell = ({ queueEntry }: QueueTableCellComponentProps) => { |
There was a problem hiding this comment.
Copy/pasted code needs to be renamed.
| @@ -196,7 +197,6 @@ | |||
| "queueLocation": "Queue Location", | |||
| "queueLocationRequired": "Queue location is required", | |||
| "queueName": "Queue name", | |||
| "queueNumber": "Queue Number", | |||
There was a problem hiding this comment.
Turns out I forgot to have a case statement for this column. Thanks for catching.
chibongho
changed the title
Requirements
Summary
This is a draft proposal of how to address the problem describe in O3-3096:
We need to configure the queue table in different contexts. For example, there can be a view of queue entries from all queues, a view of queue entries in a specific queue, or even a view of queue entries in a specific queue AND status. Depending on the context, we want to configure the queue table to display different columns. Additionally, the queue table usually provides a way to perform actions on each queue entry. The set of available actions could be specific to a queue, specific to a status, or specific to a queue+status combination.
The PR proposes a solution that allows us to specify
TableDefinitions in the config. EachTableDefinitionspecifies a list of columns to use based on anappliedTocriteria, which can be applied to specific queue, a specific status, a specific queue+status combination, or (in the case whenappliedTonot specified) to any table.We predefine a set of
QueueTableColumns that we should reasonably support (like name, priority, status) and how they should get rendered. Some of these columns can further be configured. (For example, this PR shows how we can configure thepatientIdcolumn to display a particularpatientIdentifierType.) We will also define anotherQueueTableColumn(not currently in this PR) that only renders an extension slot, allowing implementors to custom-define whatever column they want.Also not currently in this PR yet, but we will also need more flexibility in how we define actions we can perform to each queue entry based on its current queue and status. We can predefined a set of
QueueEntryActions that we should reasonably support (like editing / transitioning / ending / undoing queue entries). Some of these actions can further be customized (For example, we can define a quick action that tranistions a queue entry to a particular status). In addition, we can also define anotherQueueEntryActionthat only renders an extension slot, allowing implementors to custom-define whatever action they want. We can use a very similar approach that we used for configuring columns and apply it toQueueEntryActionsScreenshots
Table listing queue entries from multiple queues:
Table listing queue entries from a specific queue. Note that the columns rendered are different.
Related Issue
Other