(fix) O3-2995: Allow empty quantity to dispense and prescription refills in Drug Order Form

[mccarthyaaron]

Requirements

• This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
• My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
• My work includes tests or is validated by existing tests.

Summary

The suggested solution is two fold:
1.Adding the {allowEmpty=true} prop so that Carbon allows the field to be empty without adding a warning icon to the input field that suggests that the field should have a value
2.Previously, leaving the field empty returned NaN due to how parseFloat() deals with invalid values i.e parseFloat('') returns NaN. Now we first check for that and if NaN is returned the field value becomes 0 otherwise we proceed as before.

Screenshots

O3-2995.mp4

Related Issue

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

[piyushmishra1416]

Thanks, @mccarthyaaron, this is a very thoughtful touch to the UX designs.

[denniskigen]

What contract does the model provide for these fields, @dkayiwa?

[mogoodrich]

One small question, but generally LGTM...

[mogoodrich]

Is the refill field? (Setting refills to 0 if the user doesn't enter anything makes sense, but I would think that that quantity to dispense should be a set to null, not 0, if the user doesn't enter anything).

[mccarthyaaron]

Is the refill field? (Setting refills to 0 if the user doesn't enter anything makes sense, but I would think that that quantity to dispense should be a set to null, not 0, if the user doesn't enter anything).

@mogoodrich makes sense. Even the zod schema expects a number or null. Should I go ahead and change it null if the user does not fill in anything?

[mccarthyaaron]

@mogoodrich How far with this PR. Should we close it?

[ibacher]

What contract does the model provide for these fields, @dkayiwa?

It somewhat varies, but quantity and number of refills are required for orders placed with an outpatient care setting unless the drugOrder.requireOutpatientQuantity global property is set to false (it defaults to true).

I am all in favour of these fields being emptyable, but that should be done by setting the value sent to the backend to null or omitting it altogether. Defaulting things to 0 seems to me to be a non-configurable CDS rule (it is the computer making an inference about what the user intended and filling something in.

[mogoodrich]

@ibacher I believe @mccarthyaaron has updated this to set the value to null?

Who is the appropriate person to merge this? As PIH is not actively using the order basket I'm reluctant to it myself.

[ojwanganto]

@mogoodrich will this ticket affect the dispensing module? I know the dispensing module optionally uses the quantity to dispense to mark an order as complete

[ibacher]

This should definitely be something that implementations can opt-out of. Ideally it'd be tied directly to the global property I mentioned.

[mogoodrich]

It's a good point @ojwanganto and yes this should be configurable, ideally via the GP Ian mentions.
Then, if implememtations were going to enable the dispensing functionality to limit the total quantity dispensed, they'd need to make sure that total quantity was mandatory.

[denniskigen]

@mccarthyaaron is it clear what the next steps for this are?

[mccarthyaaron]

@mccarthyaaron is it clear what the next steps for this are?
@denniskigen Honsetly no. Does the code have to be updated to take into account the global property and if so how

[denniskigen]

The first part would be to add a hook that fetches the GP. Something like:

/**
 * Hook to fetch the system setting for whether to require quantity, quantity units,
 * and number of refills for outpatient drug orders.
 *
 * @returns {Object} An object containing:
 * - requireOutpatientQuantity: A boolean indicating if outpatient quantity is required.
 * - error: Any error encountered during the fetch operation.
 * - isLoading: A boolean indicating if the fetch operation is in progress.
 */
export function useRequireOutpatientQuantity() {
  const url = `${restBaseUrl}/systemsetting/drugOrder.requireOutpatientQuantity?v=custom:(value)`;
  const { data, error, isLoading } = useSWRImmutable<FetchResponse<{ value: 'true' | 'false' }>, Error>(
    url,
    openmrsFetch,
  );

  const results = useMemo(
    () => ({
      requireOutpatientQuantity: data?.data?.value === 'true',
      error,
      isLoading,
    }),
    [data?.data?.value, error, isLoading],
  );

  return results;
}

@ibacher is your impression that the current behaviour is fine and implementations should be ok with it as the default and that we'd want to make this new behaviour configurable via a config property? Then this config property would check the GP and enable this new behaviour based on the value of the GP?

[mogoodrich]

The first part would be to add a hook that fetches the GP. Something like:

/**
 * Hook to fetch the system setting for whether to require quantity, quantity units,
 * and number of refills for outpatient drug orders.
 *
 * @returns {Object} An object containing:
 * - requireOutpatientQuantity: A boolean indicating if outpatient quantity is required.
 * - error: Any error encountered during the fetch operation.
 * - isLoading: A boolean indicating if the fetch operation is in progress.
 */
export function useRequireOutpatientQuantity() {
  const url = `${restBaseUrl}/systemsetting/drugOrder.requireOutpatientQuantity?v=custom:(value)`;
  const { data, error, isLoading } = useSWRImmutable<FetchResponse<{ value: 'true' | 'false' }>, Error>(
    url,
    openmrsFetch,
  );

  const results = useMemo(
    () => ({
      requireOutpatientQuantity: data?.data?.value === 'true',
      error,
      isLoading,
    }),
    [data?.data?.value, error, isLoading],
  );

  return results;
}

@ibacher is your impression that the current behaviour is fine and implementations should be ok with it as the default and that we'd want to make this new behaviour configurable via a config property? Then this config property would check the GP and enable this new behaviour based on the value of the GP?

This makes sense to me @denniskigen

[mccarthyaaron]

@ibacher @denniskigen @mogoodrich could you kindly have a look at the recent changes pushed.

Design discussion is ongoing

[brandones]

@denniskigen @mogoodrich Why have the frontend config property, why not just have the GP control it?

[mogoodrich]

See my comment.

[mogoodrich]

As @brandones suggests, we don't need the front end config here, just use the global property. For existing consistently, we can default to a value of "true" (ie require outpatient quantity properties) if the GP is not defined.

[denniskigen]

So, the drug-order E2E test is failing because the requireOutpatientQuantity GP defaults to true, which means that the Quantity to dispense, Quantity unit , and Prescription refills fields will be marked required in the test environment. I've changed the test to fill out these fields to get it passing. Does that sound reasonable? Also, is 0 a valid value for Prescription refills? @ibacher @mogoodrich

[denniskigen]

GP set to true (the default)

with.requireOutpatientQuantity.GP.set.to.true.default.mp4

[denniskigen]

GP set to false

with.requireQuantityUnits.GP.set.to.false.mp4

[ibacher]

Does that sound reasonable?

Yes

Also, is 0 a valid value for Prescription refills?

Yes; that just means "no refills", which is a valid quantity (no quantity to dispense is a little different because having that blank means something like the quantity is "unspecified").

[denniskigen]

Also, circling back to the configuration issue, do we still want to make this the default behaviour out of the box for the refapp with the GP as the only source of truth (i.e. sans a configuration property like we normally do)?

[ibacher]

This all looks like steps in the right direction. I feel like I might be missing something from the comments I've left, so please let me know if they're off-base.

[mogoodrich]

Also, circling back to the configuration issue, do we still want to make this the default behaviour out of the box for the refapp with the GP as the only source of truth (i.e. sans a configuration property like we normally do)?

Yes, that would be my vote, to just defer to the GP

Your fix to the tests sounds perfect, thanks @denniskigen , and, yes, as Ian said refills can be zero.

[ojwanganto]

Also, circling back to the configuration issue, do we still want to make this the default behaviour out of the box for the refapp with the GP as the only source of truth (i.e. sans a configuration property like we normally do)?

I support the use of the GP.

[ibacher]

Also, circling back to the configuration issue, do we still want to make this the default behaviour out of the box for the refapp with the GP as the only source of truth (i.e. sans a configuration property like we normally do)?

IMO, the rule is this:

• If its configurable via a backend property, we defer to the backend property
• If its not configurable via a backend property, we add a frontend property

The overall goal here is that for any given configuration option, there should be a single source of truth.

[bmamlin]

It's common that quantity to dispense is required in the outpatient setting and not required (or even omitted from the form) for inpatient medications. In an outpatient setting, the patient is typically going to a pharmacy to fill the prescription and they need to know how much to give. In an inpatient setting, medications are assumed to be given until orders are changed and dispensed through hospital workflows (e.g., daily from pharmacy to floor/nurse) and quantity/refills are unnecessary.

Using a global property assumes a system will only be used for outpatient or inpatient setting (i.e., require quantity/refills for outpatient and not for inpatient). In reality, even if this were true, any inpatient system still needs to be able to place outpatient orders (i.e., medications included in a "discharge orders" encounter are treated as outpatient orders). So, a global property approach will still not quite work.

Ideally, whether or not to include (and require) dispensing details (quantity & refills) for medication orders would be driven by the care setting of the encounter (which doesn't yet exist in the data model). We introduced the notion of care_setting into the OpenMRS data model and added it to orders in anticipation of this need as well as the anticipated need of filtering orders by care setting, but it has not been adopted/used yet AFAIK. The FHIR-aligned approach would be Encounter.class.

So, I would consider use of global property to control the use of required dispensing fields on medication orders to be a hack that will work for most of our current settings (all outpatient), but will not truly serve any inpatient needs (setting the global property to false will only mean that providers will still unnecessary see but can skip these fields for inpatient medication orders and will likely continue to skip them when they really should be required for outpatient medication orders like with discharge orders).

Rather than a global property that will only work for outpatient systems, I would favor an approach that assumes the current Encounter's care setting is outpatient/ambulatory and displays & requires dispensing fields (quantity, quantity units, and number of refills) on medication orders. Then any work to support inpatient medication orders could undertake adding encounter.care_setting (FHIR's Encounter.class) to our Encounter and evolve to having care setting populated & used for encounters & associated orders. Then the medication orders form could omit dispensing fields when placing orders for inpatient encounters.

[ibacher]

@bmamlin The goal here is just to align the orders frontend with the existing backend validations. For better or for worse, core currently validates drug orders based off the care setting and the mentioned GP, so we're really just trying to ensure that our validation rules are in sync. By itself, the GP doesn't say "all orders are outpatient orders", but rather "if the order is an outpatient order, should we require a quantity and number of refills".

There's a secondary issue where, yes, we need some way of being able to determine the care setting. EMRAPI adds the care setting to its EncounterTransaction model, but it's not clear to me how we'd determine that, i.e., should this be a property of a visit? An encounter? A location? Currently, I think the code in the order basket just assumes that the care setting is always outpatient, and that's obviously incorrect.

[mogoodrich]

IMO, the rule is this:

• If its configurable via a backend property, we defer to the backend property
• If its not configurable via a backend property, we add a frontend property

The overall goal here is that for any given configuration option, there should be a single source of truth.

I'd add one other asterisk-- if it not configurable via a backend property, consider if it should be and we should add a new backend property.

[denniskigen]

@ibacher @mogoodrich I think this is ready to be reviewed anew

[mogoodrich]

Generally looks good, one question