(fix) O3-2807: Quantity Units should be required when a quantity to dispense is specified #1636

mccarthyaaron · 2024-02-03T22:35:01Z

Requirements

My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

In the drug-order-form component, the dispensing quantity units will default to the dosage units if the quantity to be dispensed is greater than 0 while still giving the user the option to select their preferred quantity units. I attempt to achieve the result in two ways. Firstly changing the dosage units will trigger a corresponsing change in the quantity units but only if the quantity to be dispensed is not 0. Secondly, a change in the quantity to be dispensed to a value greater than 0 will trigger the quantity units to take on the value of the dosage units at the time. In both cases the user can still change the quantity units to a value other than the one defined in the dosing units.

Related Issue

https://openmrs.atlassian.net/browse/O3-2807

Screenshots

drug-order-form.mp4

…h user still able to change the dispensing units

…en the quantity to dispense is greater than 0

…quantity is greater than 0

vasharma05 · 2024-02-06T07:11:50Z

only if the quantity to be dispensed is not 0.
Any specific reason why we are looking into quantity here?
Thanks!

vasharma05 · 2024-02-06T07:29:52Z

The Dose unit and Quantity units don't need to always be the same. E.g. the Dose unit can be Tablets, but the quantity units might be 3 boxes of Tablet, so it's wrong to assume that these will always be the same. So, the right condition I feel should be: If dose unit === quantityUnit then only the quantityUnit should be updated with the new doseUnit, else it shouldn't.

vasharma05 · 2024-02-06T07:34:06Z

Also, the ticket is not understood properly:
The title says: Quantity Units should be required when a quantity to dispense is specified, which is more about the form validation, which is not taken care of here.

denniskigen · 2024-02-06T10:31:47Z

Could you add a test?

vasharma05 · 2024-02-06T11:44:45Z

Hi, @denniskigen!
Currently, there are no tests written for the drug order form right now.
I can write the tests in a separate file.
Thanks!

ibacher · 2024-02-06T15:54:32Z

If dose unit === quantityUnit then only the quantityUnit should be updated with the new doseUnit, else it shouldn't.

In general, the way I would expect this to work is:

By default neither dose unit nor quantity unit is set (unless specified in an order template)
If the dose unit is set (including by order template), but the quantity unit is unset, the quantity unit is set to the dose unit
In no other circumstance do we automatically change either the dose unit or quantity unit
Dispensing instructions are optional, so validation of anything here should only happen if some quantity to dispense is specified (it's ok to update the quantity unit as long as this won't trigger validation).

…defaults to the dose unit

… defaults the quantity unit to the selected dose unit

…fied

mccarthyaaron · 2024-02-08T15:54:25Z

@ibacher could you kindly take a look at the changes I have a pushed and suggest any modifications

vasharma05 · 2024-02-09T07:52:30Z

Hi @ibacher!

By default neither dose unit nor quantity unit is set (unless specified in an order template)

No, it's also defined in the drug's object, we define the default dose unit from drug's dosageForm:

openmrs-esm-patient-chart/packages/esm-patient-medications-app/src/add-drug-order/drug-search/drug-search.resource.tsx

Line 136 in 7222e81

unit: drug?.dosageForm

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

vasharma05 · 2024-02-09T07:53:54Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

@@ -158,13 +171,29 @@ export function DrugOrderForm({ initialOrderBasketItem, onSave, onCancel }: Drug
      duration: initialOrderBasketItem?.duration,
      durationUnit: initialOrderBasketItem?.durationUnit,
      pillsDispensed: initialOrderBasketItem?.pillsDispensed,
-      quantityUnits: initialOrderBasketItem?.quantityUnits,
+      quantityUnits: initialOrderBasketItem?.quantityUnits ?? initialOrderBasketItem?.unit,


This is already handled here:

openmrs-esm-patient-chart/packages/esm-patient-medications-app/src/add-drug-order/drug-search/drug-search.resource.tsx

Line 136 in 7222e81

unit: drug?.dosageForm

vasharma05 · 2024-02-09T07:54:58Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

@@ -177,8 +177,8 @@ export function DrugOrderForm({ initialOrderBasketItem, onSave, onCancel }: Drug
    newValue: MedicationOrderFormData['unit'],
    prevValue: MedicationOrderFormData['unit'],
  ) => {
-    if (prevValue?.valueCoded === watch('quantityUnits')?.valueCoded) {
-      setValue('quantityUnits', newValue);
+    if (!watch('quantityUnits')) {


Why was this condition changed?

If the dose unit is set (including by order template), but the quantity unit is unset, the quantity unit is set to the dose unit

In no other circumstance do we automatically change either the dose unit or quantity unit

@vasharma05 with the previous condition, if the dose unit and quantity unit were the same, then changing the dose units was also changing the quantity unit to the new value of the dose unit. Yet according to @ibacher the dose unit should only change the quantity unit if the quantity unit is unset. Also bullet 2 above emphasizes it again

@vasharma05 @ibacher could you kindly offer some clarity on this; should a change in the dose unit trigger a change in the quantity unit if they the values of both are the same? Which happens to be what @vasharma05 wants. What I understood from @ibacher is that the dose unit should only change the quantity unit if the quantity unit is not set.

Dosing units are only rarely dispensing units. For example, a relatively common in-patient order might be for 1000 ml of saline, which is commonly dispensed as either 1 or 2 bags, depending on availability. Here the dosing is 1000 ml, but the dispensing units are in "bags". Similarly, for paracetamol, you might be given a dose of 650 mg, but that's dispensed in the form of 2 325 mg tablets per dose. Again, the dosing is in mg, but the dispensing is in tablets. There are a few narrow cases, though, where the dose ordered is the same units as the thing dispensed. This feature is targeted at those narrow cases.

So, my opinion is that once a user has selected a dispensing unit, we should not change that, because in most cases, the dispensing unit is not going to be the same as the dosing unit. Prior to the user making that choice, however, its fine to automatically update, as for a few cases this will make people's lives easier.

Understood. Thanks

…rder-form.component.tsx

…ient-chart into drug-order-form

denniskigen · 2024-02-09T11:45:29Z

Tangential to this, but the forms inputs ought to have an error outline when validation fails, which is not the case here:

Additionally, the error messages need a bit of breathing room. They're too close to the inputs.

Could you help ticket that out under the bug fix epic, @vasharma05?

denniskigen · 2024-02-09T11:51:49Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

+      return true;
+    },
+    {
+      message: 'Please select Quantity unit',


Suggested change

message: 'Please select Quantity unit',

message: 'Please select a quantity unit',

@vasharma05, could you guide @mccarthyaaron on how to make these error messages translatable? We should probably add an example to our Coding Conventions.

Will be eager to learn how to do that. Thanks

Hi @mccarthyaaron!
You can refer #1652.
Thanks!

denniskigen · 2024-02-09T11:52:35Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

+    prevValue: MedicationOrderFormData[keyof MedicationOrderFormData],
+  ) => void;
+  control: Control<MedicationOrderFormData>;
+  [x: string]: any;


Can we get a better type here than any? any completely bails out of the type system.

Properly typing this requires a discriminated union, since the values of restProps depends on the type. Something like:

type ControlledFieldInputProps = BaseControlledFieldInputProps & ( ToggleFieldProps | CheckboxFieldProps | NumberFieldProps | TextAreaProps | TextInputProps | ComboBoxProps ) interface BaseControlledFieldInputProps { name: keyof MedicationOrderFormData; handleAfterChange?: ( newValue: MedicationOrderFormData[keyof MedicationOrderFormData], prevValue: MedicationOrderFormData[keyof MedicationOrderFormData], ) => void; control: Control<MedicationOrderFormData>; } interface ToggleFieldProps extends ToggleProps /* part of Carbon */ { type: 'toggle', } // repeat for other types

And that should give us something fully typed.

denniskigen

Thanks for working on this, @mccarthyaaron!

@ibacher I've tested things out locally against the functional requirements you laid out, and they seem to work OK for me. Do you have any qualms about the approach?

ibacher

I'm mostly fine with the logic. A few small nits and one edge-case requirement. Thanks for the great work @mccarthyaaron!

ibacher · 2024-02-09T13:02:58Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

@@ -143,7 +156,7 @@ export function DrugOrderForm({ initialOrderBasketItem, onSave, onCancel }: Drug
    return initialOrderBasketItem?.startDate as Date;
  }, [initialOrderBasketItem?.startDate]);

-  const { handleSubmit, control, watch, setValue } = useForm<MedicationOrderFormData>({
+  const methods = useForm<MedicationOrderFormData>({


Is there a reason for the methods variable?

ibacher · 2024-02-09T13:12:05Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

+    newValue: MedicationOrderFormData['unit'],
+    prevValue: MedicationOrderFormData['unit'],
+  ) => {
+    if (prevValue?.valueCoded === watch('quantityUnits')?.valueCoded) {


This can probably be substituted (more correctly) with getValues() (also returned from useForm(). watch() is intended to subscribe to input changes, but here it's used to get a point-in-time value.

Since this is passed as a prop to a component, it would be best to wrap this in useCallback(). Otherwise, each render will re-render the ControlledFieldInput.

So, I don't think the logic here avoids the edge-case where the user has selected a quantityUnits value that happens to be the same as the dosingUnits, but then changes the dosingUnits. I'm ok with automatically changing the value as long as its been automatically determined, but user input (especially for medication instructions) needs to always override automatic determinations.

ibacher · 2024-02-09T13:21:22Z

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

+    prevValue: MedicationOrderFormData[keyof MedicationOrderFormData],
+  ) => void;
+  control: Control<MedicationOrderFormData>;
+  [x: string]: any;


Properly typing this requires a discriminated union, since the values of restProps depends on the type. Something like:

type ControlledFieldInputProps = BaseControlledFieldInputProps & ( ToggleFieldProps | CheckboxFieldProps | NumberFieldProps | TextAreaProps | TextInputProps | ComboBoxProps ) interface BaseControlledFieldInputProps { name: keyof MedicationOrderFormData; handleAfterChange?: ( newValue: MedicationOrderFormData[keyof MedicationOrderFormData], prevValue: MedicationOrderFormData[keyof MedicationOrderFormData], ) => void; control: Control<MedicationOrderFormData>; } interface ToggleFieldProps extends ToggleProps /* part of Carbon */ { type: 'toggle', } // repeat for other types

And that should give us something fully typed.

packages/esm-patient-medications-app/src/add-drug-order/drug-order-form.component.tsx

and wrapped handlers in a useCallback()

mccarthyaaron · 2024-02-09T16:33:58Z

@ibacher pushed the requested changes. anymore recommended changes?

ibacher · 2024-02-09T17:52:45Z

@mccarthyaaron Could you take a look into the Typescript thing?

mccarthyaaron · 2024-02-09T19:58:05Z

@mccarthyaaron Could you take a look into the Typescript thing?

@ibacher could you kindly direct me to a resource with the carbon types if possible?

ibacher · 2024-02-09T20:00:25Z

@mccarthyaaron The Carbon source code is quite readable and has the types. Just make sure you're looking at the v11.37.0 tag.

mccarthyaaron · 2024-02-09T20:19:25Z

@mccarthyaaron The Carbon source code is quite readable and has the types. Just make sure you're looking at the v11.37.0 tag.

For educative purposes, is there a reason it has to be that specific tag?

ibacher · 2024-02-09T20:20:27Z

Is the version of Carbon we're currently using. The current version of Carbon may have any number of differences.

mccarthyaaron · 2024-02-09T20:31:46Z

@ibacher I am running into this error: Cannot use namespace 'ToggleProps' as a type. I think I imported the correct type because the import statement bringing it in isn't throwing any errors. I don't know whats causing that. Does it need downloading the @types/carbon-components-react package

mccarthyaaron · 2024-02-10T15:15:01Z

@ibacher I have something for the types thing. Feels makeshift-ish especially with the importing of the types. For some reason I can't use the carbon types imported from '@carbon/react'. I get the error Cannot use namespace as type so I imported each of them individualy in a format like this;
import { type ComboBoxProps } from '@carbon/react/lib/components/ComboBox/ComboBox';
There are some other details that would need some polishing and feedback from you. Can I push the saidchanges here?

vasharma05

LGTM!

…ient-chart into drug-order-form

ibacher · 2024-02-13T14:05:08Z

@mccarthyaaron Oh right... we kind of tell TS to ignore the types from Carbon... Although the preferred fallback solution would've been to mark those properties as unknown in this case, I think.

mccarthyaaron added 3 commits February 3, 2024 23:58

(fix) dispensing units in drug-order-form default to dosage units wit…

844de36

…h user still able to change the dispensing units

changes in dosage unit only trigger changes in the dispensing unit wh…

772d757

…en the quantity to dispense is greater than 0

the dispensing units default to the dosing units when the dispensing …

7d91e9e

…quantity is greater than 0

denniskigen requested a review from ibacher February 5, 2024 11:01

denniskigen mentioned this pull request Feb 5, 2024

fix O3-2807 : Quantity unit made default for dose unit when quantity is not zero #1637

Closed

3 tasks

vasharma05 added 4 commits February 6, 2024 14:09

Quantity units should be same as dosing units by default

08ea98b

Fixed the Form Provider path

46413eb

Removed Form Provider

61b35d9

Quantity unit is required field

cdeb02a

Better error handling for quantity units

c4eeb10

mccarthyaaron and others added 4 commits February 8, 2024 11:27

Merge branch 'openmrs:main' into drug-order-form

068fcb3

if the default value of the quantity units is not specified, then it …

12a1aac

…defaults to the dose unit

if the quantity unit is not specified, manually setting the dose unit…

cb9b02c

… defaults the quantity unit to the selected dose unit

quantity unit is required only when the quantity to dispense is speci…

b4e9648

…fied

vasharma05 reviewed Feb 9, 2024

View reviewed changes

Update packages/esm-patient-medications-app/src/add-drug-order/drug-o…

282d82d

…rder-form.component.tsx

vasharma05 changed the title ~~(fix) O3-2807: Quantity units default to dose unit when the quantity is not zero~~ (fix) O3-2807: Quantity Units should be required when a quantity to dispense is specified Feb 9, 2024

Final changes

fb1adb5

vasharma05 approved these changes Feb 9, 2024

View reviewed changes

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

f380d44

…ient-chart into drug-order-form

denniskigen reviewed Feb 9, 2024

View reviewed changes

ibacher reviewed Feb 9, 2024

View reviewed changes

mccarthyaaron added 2 commits February 9, 2024 17:15

removed the methods variable

c087559

Replaced watch() with getValues() where necessary

624173c

and wrapped handlers in a useCallback()

vasharma05 approved these changes Feb 13, 2024

View reviewed changes

vasharma05 added 2 commits February 13, 2024 16:15

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

0315e57

…ient-chart into drug-order-form

Quantity untis should be translated

936a825

vasharma05 merged commit a659a8e into openmrs:main Feb 13, 2024
6 checks passed

mccarthyaaron deleted the drug-order-form branch March 6, 2024 08:06

	message: 'Please select Quantity unit',
	message: 'Please select a quantity unit',

(fix) O3-2807: Quantity Units should be required when a quantity to dispense is specified #1636

(fix) O3-2807: Quantity Units should be required when a quantity to dispense is specified #1636

Conversation

mccarthyaaron commented Feb 3, 2024 • edited by denniskigen

Requirements

Summary

Related Issue

Screenshots

vasharma05 commented Feb 6, 2024

vasharma05 commented Feb 6, 2024

vasharma05 commented Feb 6, 2024

denniskigen commented Feb 6, 2024 • edited

vasharma05 commented Feb 6, 2024

ibacher commented Feb 6, 2024

mccarthyaaron commented Feb 8, 2024

vasharma05 commented Feb 9, 2024

vasharma05 Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen commented Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denniskigen left a comment • edited

Choose a reason for hiding this comment

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccarthyaaron commented Feb 9, 2024

ibacher commented Feb 9, 2024

mccarthyaaron commented Feb 9, 2024

ibacher commented Feb 9, 2024

mccarthyaaron commented Feb 9, 2024

ibacher commented Feb 9, 2024

mccarthyaaron commented Feb 9, 2024 • edited

mccarthyaaron commented Feb 10, 2024

vasharma05 left a comment

Choose a reason for hiding this comment

ibacher commented Feb 13, 2024 • edited

mccarthyaaron commented Feb 3, 2024 •

edited by denniskigen

denniskigen commented Feb 6, 2024 •

edited

vasharma05 Feb 9, 2024 •

edited

denniskigen commented Feb 9, 2024 •

edited

denniskigen Feb 9, 2024 •

edited

denniskigen left a comment •

edited

mccarthyaaron commented Feb 9, 2024 •

edited

ibacher commented Feb 13, 2024 •

edited