Skip to content
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) The lab order form should have a reason for ordering field #1458

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

makombe
Copy link
Contributor

@makombe makombe commented Nov 8, 2023

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

Screenshots

lab-order-reason

Related Issue

https://issues.openmrs.org/browse/O3-2361

Other

Sample config schema

{
  "@openmrs/esm-patient-labs-app": {
    "labTestsWithOrderReasons": [
      {
        "labTestUuid": "854AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
        "orderReasons": [
          "161236AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
          "162080AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
          "160032AAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
        ]
      }
    ]
  }
}

@makombe makombe force-pushed the O3-2361 branch 2 times, most recently from 88b37e5 to e47e35b Compare November 8, 2023 20:11
packages/esm-patient-labs-app/src/config-schema.ts Outdated Show resolved Hide resolved
_type: Type.UUID,
_description: 'Lab test concept UUID',
},
label: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule, we should avoid labels in configuration if we can. Labels like this are only usable in monolingual environments. In general, especially if this is concept-backed, we should prefer to use the display strings from the concepts themselves rather than inventing new text for them.

That said, what we store in the backend is just text, so I'm a little unclear on why we even have concepts involved in this at all? If we want to capture coded answers or coded or free text answers, we should actually update the backend and data model to reflect that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a minimum, labels should be run through the translation system.

Copy link
Contributor Author

@makombe makombe Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher orders model already provides the ability to capture order reason as coded or non coded/ free text(The two are very distinct and represented differently). In this case we are using coded answers which are represented by concepts
Screenshot from 2023-11-09 08-27-11

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makombe Thanks. I didn't realise it was using that pattern. If we're using concepts, then the label should come from the concept itself, right? Ideally, we want whatever we're using for drop-down labels to be translatable. Having strings in the configuration schema is, unfortunately, not translatable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @ibacher here, we should be getting labels from the concepts, right?

Copy link
Contributor Author

@makombe makombe Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandones we normally try to avoid editing of concept directly so as to be in sync with OCL/CIEL dictionary. We consume concepts as they come from OCL/CIEL and that is why I thought of providing label will help with local overrides without necessarily having to edit the concept. A better way of achieving the same is welcomed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can always request that CIEL adds a synonym or even preferred name for a concept. Alternatively, you can use Iniz to provide custom concept names or change the preferred name in a reproducible way without needing to deviate from CIEL.

In any case, labels must never appear in the configuration because it is unworkable in a multilingual environment. If we need something in the configuration to represent a user-facing string it needs to be a translation key rather than a label.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great points, thanks. I know adding synonyms in CIEL may be good but still not sustainable more so if preferences/synonyms differ across implementations. A major challenge we face with labels is trying to match the labels as they are provided in clinical forms or even reporting tools. Even though the concept is the same, we are forced to display it differently depending on the type of user, and how they commonly use the concept. What we are trying to avoid is having multiple concepts with the same meaning simply because of the display.

The concept to use the iniz module is great but I don't see how it will address the label overrides for concepts that may require 2 or 3 synonyms. Even if this could work, we still will need a way of instructing the config to use the synonym which am not sure already exists. I may be wrong, but I was hoping that the idea behind a config is to somehow address a local context. For instance, I don't expect the same config to apply in Kenya and Tanzania where English and Kiswahili are used. I would expect the Kenyanized config to be in English and that for Tanzania in Kiswahili. Sorry if I may have missed the point. @ibacher @brandones

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher @brandones I have removed the label from the config as suggested and made necessary adjustment. Thanks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @makombe. @ibacher @brandones @denniskigen kindly help with review. Thanks

@denniskigen denniskigen changed the title (feat): Lab form should have "reason for ordering (feat) The lab order form should have a reason for orderingfield Nov 9, 2023
@denniskigen denniskigen changed the title (feat) The lab order form should have a reason for orderingfield (feat) The lab order form should have a reason for ordering field Nov 9, 2023
@denniskigen denniskigen changed the title (feat) The lab order form should have a reason for ordering field (feat) The lab order form should have a reason for ordering field Nov 9, 2023
@makombe
Copy link
Contributor Author

makombe commented Feb 12, 2024

Apologies for the force-push update, prompted by extensive changes in lab orders. @brandones I will appreciate your input on the new lab order reason configuration, designed to specify reasons for specific orders. Thanks

@ojwanganto
Copy link

pinging @brandones

Comment on lines 76 to 79
const selectedLabTest =
config && config.labTestsWithOrderReasons
? config.labTestsWithOrderReasons.find((p) => p.labTestUuid === defaultValues?.testType?.conceptUuid)
: null;
Copy link
Contributor

@brandones brandones Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const selectedLabTest =
config && config.labTestsWithOrderReasons
? config.labTestsWithOrderReasons.find((p) => p.labTestUuid === defaultValues?.testType?.conceptUuid)
: null;
const orderReasons = config.labTestsWithOrderReasons.find((c) => c.labTestUuid === defaultValues?.testType?.conceptUuid).map(c => c.orderReasons);

Config elements will always be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

@makombe makombe force-pushed the O3-2361 branch 2 times, most recently from d0f4355 to d5d4b61 Compare February 15, 2024 21:33
@ojwanganto
Copy link

@denniskigen kindly help us review this. This PR will unblock the use of o3 lab order form for HIV monitoring lab tests

@denniskigen denniskigen merged commit 8be52b2 into openmrs:main Feb 16, 2024
6 checks passed
@brandones
Copy link
Contributor

Thank you @denniskigen for finishing the review, thanks @ojwanganto and @makombe for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants