Standardizing Boolean Concepts via "OpenMRS" Source Mappings (TRUNK-6232)

Development
, ,
1

Hi everyone,

I am working on TRUNK-6232 to resolve a long-standing issue regarding how OpenMRS identifies boolean concepts. Following a review of my Pull Request, @dkayiwa suggested I start this thread to share the approach and specifically gather feedback from @akanter.

The Context (discussed here)

Historically, OpenMRS defined boolean concepts using Global Properties (concept.true and concept.false) pointing to database IDs 1 and 2. In many distributions, especially those initialized with the full CIEL dictionary, IDs 1 and 2 actually refer to “Anemia due to blood loss” and “Anemia, hemolysis”.

This has led to a long-standing issue where boolean observations are technically stored as “Anemia” in the database. While the backend often handles this contextually, it creates significant issues for data integrity and REST API consumers.

The Proposed Solution

To resolve this, the Platform Team has decided to move away from ID-based identification in favor of string-based mappings within a new “Official” namespace. My Pull Request implements the following:

  • Official “OpenMRS” Concept Source: Introduced a new concept source with a unique version 4 UUID (913e06c5-4ca1-46e3-9ecf-afbe11bbd0d9) to represent official OpenMRS core metadata.
  • Standardized Reference Terms: Added two concept reference terms to the base data under the “OpenMRS” source with codes TRUE and FALSE.
  • Alignment on 1065/1066: We are aligning on using the concepts traditionally known as CIEL:1065 (Yes/True) and CIEL:1066 (No/False).
  • New Core Mappings: Within the base dataset, I added SAME-AS mappings linking these core boolean concepts (UUIDs 1065AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA and 1066AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA) to OpenMRS:TRUE and OpenMRS:FALSE.
  • Deprecation Strategy: Marked the concept.true and concept.false global properties as deprecated in both the code and base dataset descriptions, pointing instead to these new mappings.

Feedback Requested

I’d like to confirm with the community, and specifically @akanter:

  1. Does the introduction of the “OpenMRS” source for core metadata align with the long-term vision for handling platform-specific mappings?
  2. Are there any concerns with using the codes OpenMRS:TRUE and OpenMRS:FALSE for these core terms?

Looking forward to your thoughts!

cc: @ibacher @burke @mseaton

2

Wow, I had no idea that instance data was being stored with the CIEL concept IDs of 1 and 2. That clearly is a problem which needs a data migration clean up to prevent introduction of Anemia into patient records.’

I also thought that we had addressed this in current versions by pointing the boolean true/false to the Yes/No concepts 1065/1066, which would be in alignment to your suggestion @dkayiwa can also confirm.

For question concepts, at least, CIEL favors a Yes, No, Unknown approach so as to also identify when an answer is neither yes or no but information is not available.

As for the proper strategy, I would leave that to a more technical group to discuss (as has been tagged on this post).

4

We’ve generally actually deprecated boolean concepts for more or less this reason.

The other option is to add something like a “metadata term mapping” which I think would do this without the need to change CIEL itself.

6

One quick thought: I saw that the openmrs-content-referenceapplication-demo repo is optional for setup, so If I put the mappings in the -demo repo, won’t that leave users (who don’t use demo data) without the concept.true mapping?

Should we consider placing the metadatatermmappings.csv in the main referenceapplication config instead? That way, the mapping becomes the standard for all Reference Application installations, not just the demo.

7

Should I go ahead with the implementation @ibacher?

8

Yes, but the point of that repo is more to allow us to segregate absolutely required metadata from metadata that can be customized to an implementation. Different implementations will have their own concept dictionaries and may have their own concepts they want to serve as “true” and “false”. Hence, it belongs in the demo data rather than the other one.

9

Apologies for the delay. I was sick.

@ibacher Got it, that makes sense regarding where the data should live.

However, I have a question about the implementation logic in openmrs-core. I have gone through the relevant code and found that the logic is currently hardcoded to look up Global Properties:

    private void setBooleanConcepts() {
      
      try {
         trueConcept = Context.getConceptService().getConcept(
             Integer.parseInt(Context.getAdministrationService().getGlobalProperty(
                 OpenmrsConstants.GLOBAL_PROPERTY_TRUE_CONCEPT)));
         initializeLazyPropertiesForConcept(trueConcept);
         
         falseConcept = Context.getConceptService().getConcept(
             Integer.parseInt(Context.getAdministrationService().getGlobalProperty(
                 OpenmrsConstants.GLOBAL_PROPERTY_FALSE_CONCEPT)));
         initializeLazyPropertiesForConcept(falseConcept);
      }
      catch (NumberFormatException e) {
         log.warn("Concept ids for boolean concepts should be numbers");
      }
   }

   @Override
   @Transactional(readOnly = true)
   public Concept getTrueConcept() {
      if (trueConcept == null) {
         setBooleanConcepts();
      }
      
      return trueConcept;
   }

   @Override
   @Transactional(readOnly = true)
   public Concept getFalseConcept() {
      if (falseConcept == null) {
         setBooleanConcepts();
      }
      
      return falseConcept;
   }

   public void setValueBoolean(Boolean valueBoolean) {
      if (getConcept() != null && getConcept().getDatatype() != null && getConcept().getDatatype().isBoolean()) {
         if (valueBoolean != null) {
            setValueCoded(valueBoolean ? Context.getConceptService().getTrueConcept() : Context.getConceptService()
                    .getFalseConcept());
         } else {
            setValueCoded(null);
         }
      }
   }

Since the code explicitly calls getGlobalProperty, I don’t understand how the metadatatermmappings will get utilised instead.

Should we modify the core logic to first try looking up the concept via the new mapping and then fall back to the Global Property if that mapping isn’t found?

10

It would be an alternative implementation but I was only mentioning it as an option.