(DRAFT) (feat): Allow adding different order types in the order basket #2109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -353 kB (-2.17%) Total Size: 15.9 MB
ℹ️ View Unchanged
|
ibacher
reviewed
Nov 22, 2024
| _type: Type.String, | ||
| _description: 'Order type UUID to be displayed on the order basket', | ||
| }, | ||
| orderableConcepts: { |
There was a problem hiding this comment.
Having to list all of the orderables for an order type is not going to be something we want to support.
There was a problem hiding this comment.
Agreed. This should be done via concept sets, ideally in a manner similar to what we do in the O2 ordering OWA (I don't remember exactly what that does, but you can look at that code to find out... it likely has a global property that points to a set of drug and lab orderables.) See: https://github.com/openmrs/openmrs-owa-orderentry
| _elements: { | ||
| orderTypeUuid: { | ||
| _type: Type.String, | ||
| _description: 'Order type UUID to be displayed on the order basket', |
There was a problem hiding this comment.
Uh... I hope we're not actually displaying the UUID.
| @@ -6,15 +6,53 @@ export const configSchema = { | |||
| _description: 'The encounter type of the encounter encapsulating orders', | |||
| _default: '39da3525-afe4-45ff-8977-c53b7b359158', | |||
| }, | |||
| debounceDelayInMs: { | |||
There was a problem hiding this comment.
I think this is too technical to be exposed via config. Do we have a use-case for this?
| @@ -30,7 +30,7 @@ const TestOrder: React.FC<TestOrderProps> = ({ testOrder }) => { | |||
| const tableHeaders: Array<{ key: string; header: string }> = [ | |||
| { | |||
| key: 'testType', | |||
| header: t('testType', 'Test type'), | |||
| header: testOrder?.orderType?.display, | |||
There was a problem hiding this comment.
Why would we render a header if the testOrder is undefined? Note that we assume above that it is defined anyways, so I think we can dispense with the unnecessary null-safety. Also, there needs to be some fallback if the orderType is undefined.
| @@ -0,0 +1,13 @@ | |||
| import type { OrderBasketItem, OrderUrgency } from './order'; | |||
|
|
|||
| export interface TestOrderBasketItem extends OrderBasketItem { | |||
There was a problem hiding this comment.
Basically all of the fields here are generic for an OpenMRS Order, so I'd assume that they are (with testType renamed to orderType) generic to any order basket item.
Also, it's not clear why test type is marked as nullable. Nothing is being ordered if there is no order concept.
| @@ -119,6 +120,15 @@ const OrderBasket: React.FC<DefaultPatientWorkspaceProps> = ({ | |||
| })} | |||
| name="order-basket-slot" | |||
| /> | |||
| {config?.orderTypes?.length > 0 && | |||
| config?.orderTypes?.map((orderType) => ( | |||
There was a problem hiding this comment.
Suggested change
| config?.orderTypes?.map((orderType) => ( | |
| config.orderTypes.map((orderType) => ( |
|
|
||
| .tabletTile { | ||
| @extend .desktopTile; | ||
| border-top: 1px solid #3778c1; |
There was a problem hiding this comment.
This feels like it should be a standard color, not a hex value.
There was a problem hiding this comment.
Also, does this match the designs? I can't recall seeing anything with blue borders...
|
|
||
| .chevron { | ||
| svg { | ||
| fill: black; |
There was a problem hiding this comment.
currentColor is probably more correct? We don't use black as the base text color, so this is likely visually too dark.
| import React, { type ComponentProps, useCallback, useEffect, useMemo, useState } from 'react'; | ||
| import { Button, Tile } from '@carbon/react'; | ||
| import classNames from 'classnames'; | ||
| import styles from './generic-order-panel.scss'; |
There was a problem hiding this comment.
Pay attention to the coding conventions.
| @@ -193,7 +197,9 @@ export function LabOrderForm({ | |||
| id="labReferenceNumberInput" | |||
| invalid={!!errors.accessionNumber} | |||
| invalidText={errors.accessionNumber?.message} | |||
| labelText={t('labReferenceNumber', 'Lab reference number')} | |||
| labelText={t('testOrderReferenceNumber', '{{orderType}} reference number', { | |||
| orderType: orderType?.display, | |||
There was a problem hiding this comment.
We need a fallback when the orderType.display has no value.
|
I also want to be clear that we cannot merge this in if it breaks the existing lab order functionality in any way, so be careful that things still work there with no code or configuration changes for implementers. Since this is widely-used in production, we have less freedom to break things "just because". |
vasharma05
force-pushed
the
feat/add-order-types
branch
from
6401632
to
f6899b7
Compare
vasharma05
force-pushed
the
feat/add-order-types
branch
from
01b63e0
to
2097c05
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR allows the implementer to add different order types in the order basket, which will be distinguished by the order's
javaClassnameScreenshots
None
Related Issue
Other