-
Notifications
You must be signed in to change notification settings - Fork 224
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) O3-2825: Add "Discontinued" label for discontinued medications #2037
base: main
Are you sure you want to change the base?
Conversation
@brandones, @denniskigen, @dkayiwa, @ibacher , @njiddasalifu kindly this is the new PR. Ref #1691 |
Code seems fine. However, I think we want to make this more prominent. Instead of just saying "— DISCONTINUED" at the end of the block, I think we should put the text "Discontinued" in a grey pill in the first line (using the Carbon Tag element IIRC). |
@ibacher Kindly feel free to confirm the position of the tag, please! |
packages/esm-patient-medications-app/src/components/medications-details-table.component.tsx
Outdated
Show resolved
Hide resolved
<Tag type="gray" className={styles.tag}> | ||
{t('discontinued', 'Discontinued').toUpperCase()} | ||
</Tag> |
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.
@ibacher should this tag also contain the discontinued date?
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.
Maybe as a tooltip? I think the goal here was just to better flag that these requests are discontinued. PS, I'd probably leave out the toUpperCase()
and either change the translation here to t('discontinued_Caps', 'DISCONTINUED')
or just drop it altogether (my preference).
</span> | ||
{formatDate(new Date(medication.dateStopped))} | ||
<Tag type="gray" className={styles.tag}> |
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.
Add some left margin to the tag so it has some breathing room from the rest of the content.
@@ -23,7 +23,7 @@ export function usePatientOrders(patientUuid: string, status: 'ACTIVE' | 'any') | |||
'commentToFulfiller,drug:(uuid,display,strength,dosageForm:(display,uuid),concept),dose,doseUnits:ref,' + | |||
'frequency:ref,asNeeded,asNeededCondition,quantity,quantityUnits:ref,numRefills,dosingInstructions,' + | |||
'duration,durationUnits:ref,route:ref,brandName,dispenseAsWritten)'; | |||
const ordersUrl = `${restBaseUrl}/order?patient=${patientUuid}&careSetting=${careSettingUuid}&status=${status}&orderType=${drugOrderTypeUUID}&v=${customRepresentation}`; | |||
const ordersUrl = `${restBaseUrl}/order?patient=${patientUuid}&careSetting=${careSettingUuid}&orderTypes=${drugOrderTypeUUID}&v=${customRepresentation}`; |
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.
Missed that... orderType
... orderTypes
won't do anything.
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.
@ibacher, @denniskigen When I switch to orderType, we will primarily be requesting orders of a specific type (active medications) based on the UUID provided in drugOrderTypeUUID, which excludes other order types such as discontinued or past orders. However, the orderTypes parameter allows querying multiple types of orders (e.g., active, discontinued, or past orders). Since we are retrieving past medications, the API may expect multiple orderType values, such as both active and discontinued orders, with the plural form likely indicating an array of possible order types. I will therefore request we user orderTypes
Please feel free to correct me.
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.
Active / Discontinue is separate from order type. orderType=${drugOrderTypeUUID}
restricts things to drug orders, whether active or discontinued. There is no orderTypes
parameter (here is the code that parses it), so removing this now doesn't correctly filter things to just drug orders.
Actually looking at the code removing the status
parameter is also the wrong thing. It should actually be set to "any".
) : null} | ||
{medication.dateStopped ? ( | ||
)} | ||
{(medication.dateStopped || medication.autoStopDate) && ( |
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 the inclusion of medication.autoStopDate
here?
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.
@denniskigen It was one of the comments left by Ian on this PR this and I Quote @[ibacher](https://github.com/ibacher) ibacher [on Jul 5](https://github.com/openmrs/openmrs-esm-patient-chart/pull/1691#discussion_r1667011209) I don't know that it's guaranteed that dateStopped will always be set. That is, it will be set for medications that have been discontinued, but maybe not for those with a autoStopDate (sp. of property).
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.
However, if we're adding it to the condition, it should also be added to the tooltip.
<Tooltip | ||
content={`${t('discontinuedDate', 'Discontinued date')}: ${formatDate( | ||
new Date(medication.dateStopped || medication.autoStopDate), | ||
)}`} | ||
> | ||
<Tag type="gray" className={styles.tag}> | ||
{t('discontinued_Caps', 'DISCONTINUED')} | ||
</Tag> | ||
</Tooltip> |
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.
<Tooltip | |
content={`${t('discontinuedDate', 'Discontinued date')}: ${formatDate( | |
new Date(medication.dateStopped || medication.autoStopDate), | |
)}`} | |
> | |
<Tag type="gray" className={styles.tag}> | |
{t('discontinued_Caps', 'DISCONTINUED')} | |
</Tag> | |
</Tooltip> | |
<Tooltip align="top" label={formatDate(new Date(medication.dateStopped))}> | |
<Tag type="gray" className={styles.tag}> | |
{t('discontinued', 'Discontinued')} | |
</Tag> | |
</Tooltip> |
packages/esm-patient-medications-app/src/components/medications-details-table.scss
Outdated
Show resolved
Hide resolved
@denniskigen I believe this issue is occurring because of how the orders are fetched and rendered. After removing the 'ACTIVE' filter(on the api), When a medication is active, it is stored in the system as an active order. When it is discontinued, the same order might get updated or create a new record that reflects its discontinued status. This means: One entry represents the medication when it was active. Another entry represents the medication when it was discontinued. I think applying filtering to ensure that only the correct version (discontinued) is displayed in the past medications section will solve the issue if that is the way to go. |
) : null} | ||
{medication.dateStopped ? ( | ||
)} | ||
{(medication.dateStopped || medication.autoStopDate) && ( |
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.
However, if we're adding it to the condition, it should also be added to the tooltip.
<span className={styles.label01}> | ||
— {t('discontinuedDate', 'Discontinued date').toUpperCase()} | ||
</span> |
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 leave this in. Again, we're not trying to change the display, just make discontinued orders more prominent.
<Tooltip align="top" label={formatDate(new Date(medication.dateStopped))}> | ||
<Tag type="gray" className={styles.tag}> | ||
{t('discontinued', 'Discontinued')} | ||
</Tag> | ||
</Tooltip> |
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 specifically think this should go at the start of the card, at the end of the first line.
packages/esm-patient-medications-app/src/active-medications/active-medications.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-medications-app/src/active-medications/active-medications.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-medications-app/src/active-medications/active-medications.component.tsx
Outdated
Show resolved
Hide resolved
const activeMedications = activePatientOrders?.filter((order) => { | ||
const isActive = !order.dateStopped && order.action !== 'DISCONTINUE'; | ||
return isActive || (!order.autoExpireDate && order.action !== 'DISCONTINUE'); | ||
}); |
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.
Shouldn't this bit of logic factor in the order's:
dateStopped
- action (should not be
DISCONTINUE
) - autoExpireDate (should not have elapsed)
And if those are the conditions, should the logic be closer to something like:
const activeMedications = orders?.filter((order) => {
const currentDate = new Date();
const hasNotExpired = !order.autoExpireDate || new Date(order.autoExpireDate) > currentDate;
return !order.dateStopped && order.action !== 'DISCONTINUE' && hasNotExpired;
});
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 should also explain the discrepancy between the number of orders in the active medications table on the Summary page with this PR vs on dev3.
Additionally, because we're no longer leveraging the ACTIVE
parameter to do the filtering on the backend, perhaps we could move the filtering logic to the hook so that it knows to return both activeOrders
and pastOrders
so that consuming components can just focus on rendering what they want.
Requirements
Summary
I modified the tableRows mapping function to conditionally add the "Discontinued" label based on when medication is discontinued. To achieve this, I added a new constant isDiscontinued to check if the medication is discontinued then conditionally render the "Discontinued" label only when isDiscontinued is true. This way, the label will be displayed only for discontinued medications.
Screenshots
screencast.2024-10-01.6.PM-44-09.mp4
Related Issue
Other