I18n support for all classes extending BaseOpenmrsMetadata

Generally, i find it unintuitive and hacky to use the Concept class for this. If it were a new class, that would be easier to take in. I do not think Erick Evans would find this to be in line with Domain-Driven Design. :slight_smile:

I’m not enough of a medical informatics expert to settle whether it is hacky or appropriate to use terminology concepts to describe pieces of EMR metadata.

I would say yes, unless there was a specific and yet other reason for them to be described by a concept.

I agree with the idea that we should keep getters and setters simple, but this is actually how we implement Attributable#getPossibleValues(). Granted that goes through the service layer, so it likely avoids some of the LazyInitializationExceptions.

What do you think should happen in the case that we commonly have where translations would be only very rarely changed at runtime? For example, the number of VisitTypes and their corresponding names are likely to change only very infrequently, but I’m not sure that keeping a link between a possibly implementation-specific VisitType and a configuration file is the best idea.

This is the argument that really gets me. This really does end up changing what a concept is in OpenMRS (I read a “concept” in OpenMRS as a semantic label for a piece of data). Plus, it seems a little odd to me that we end up with a concept for determining what to call a “concept” in a given implementation.

That said, I take the overall point: i18n support is pretty limited in OpenMRS. I think it might be better to approach this from the perspective of creating a i18n framework from which we could (re-)build support for localization of concept names and metadata items, e.g., by implementing a version of ResourceBundle that can read from the DB (or wherever). Of course, this does get away from being a “simple way” to implement i18n for metadata.

We could introduce a specific data class for this. For instance “metadata descriptor”, to segregate them away. Again, such ad-hoc classes had been introduced for programs, workflows and states.

I’m not sure I understood this, would you mind rephrasing?


I was indeed contemplating a relatively inexpensive way to solve this old problem. Concept is great because it’s been there forever and it will plug nicely without too much overhead. Every implementer by now has a way to handle them.

I just realized that what I was thinking doesn’t actually apply (I naively assume Concept extends BaseOpenmrsMetadata).

Nor me. My understanding is that Concepts have the explicit name structure that they have due to more than just localization, and that it is an important feature of the system to be able to record exactly what concept name was asked and answered (eg. the specific wording) on a clinical observation. This is why, for example, Obs has a reference to valueCodedName in addition to valueCoded. As @dkayiwa pointed out, there may be value in trying to limit Concepts to clinical terms and value sets rather than a generic multi-lingual dictionary. Though I defer to @burke or @akanter on this.

Taking this a step further, one could ask why we have different types of metadata at all, if everything is ultimately able to link back to Concept. What design decisions led us to create distict “drug” table and independent Drug object rather to have Drug extends Concept and an explicit relationship between the “concept” and “concept_drug” table? The answers to that might be informative.

Mike

At Regenstrief, the concept dictionary started out much like in OpenMRS but later evolved to a model where the “local” concept dictionary (equivalent to OpenMRS’ Concept) became just one source (one dictionary) and other sources/dictionaries could easily be added & used as needed for things like metadata.

In effect, imagine adding a concept.source attribute and populating it with “1” for all existing concepts. In turn, what we see as the dictionary now becomes the “primary dictionary” and code that uses concepts evolves from asking for “concept 123” to asking for “concept 123 from the primary dictionary.” When you need a new value set or metadata dictionary, you add a new source (e.g., “2” or “3”). Eventually, a universal concept ID was added that could map to a source + concept ID (similar to how we use UUIDs to map to concepts).

The challenge with using concepts for metadata (and why there is ongoing debate on our choice to leverage them for i18n of programs/workflows/states) is conflating local metadata with a central/primary concept dictionary that typically needs higher rigor than local metadata.

I bring this up because this thread reminds me of my attempts many years ago to evolve the OpenMRS model toward Regenstrief’s – i.e., adding a “source” column to our concept table. Unfortunately, the extra complexity wasn’t justifiable at the time. That said, if we had managed to do it, I don’t think we’d be having this conversation; rather, people would just be adding a new source for metadata when needed.

Thanks @burke.

In the absence of the source column, do you think it would be acceptable enough to introduce ad-hoc classes (again, this is what was done with programs/workflows/states)? See here. That would be the discriminator between first class metadata and local/second class metadata.

If yes then the proposition would be:

  1. Each piece of metadata could be described by a concept.
  2. Such concepts would belong to a “metadata descriptor” class.

Bumping this one up, once again :wink: A much more lightweight approach could be to:

  1. Add a new member - say - i18nCode to each subclass of BaseOpenmrsMetadata.
  2. Expand on all REST metadata resources to have a label property that would basically be the L10n value of i18nCode.

Eg. with VisitType:

VisitType vt = new VisitType();
vt.setName("Outpatient Visit");
vt.setI18nCode("vt.opdvisit");

(That’d be typically done through an OpenMRS config, not through code of course.)

The properties files would do the usual:

messages_en.properties

vt.opdvisit=OPD Visit

messages_fr.properties

vt.opdvisit=Visite ambulatoire

Then the REST resource:

In locale ‘en’:

{
  "uuid": ...,
  "name": "Outpatient Visit",
  "label": "OPD Visit",
  ...
}

In locale ‘fr’:

{
  "uuid": ...,
  "name": "Outpatient Visit",
  "label": "Visite ambulatoire",
  ...
}

Thoughts, @burke , @dkayiwa, @ibacher, @mogoodrich, @mseaton?

I would even be ok to assume that BaseOpenmrsMetadata#name could be used as the i18n code rather than actually adding a new member to that class (and it’s myriad of subclasses). Hackier of course, but quite affordable IMO.

I would simply update the implementation of this method to take advantage of your new proposed member property: https://github.com/openmrs/openmrs-module-webservices.rest/blob/2.29.0/omod-common/src/main/java/org/openmrs/module/webservices/rest/web/resource/impl/MetadataDelegatingCrudResource.java#L158-L176

Unless we are changing the implementation of getName() to be aware of the i18n code, i would not be in favour of this. And even then, i would rather introduce a new method like getLocalizedName(), than change getName().

1 Like

Going slightly further, as a minor refinement REST resources could provision for all backend supported locales, so typically we’d get something like this:

In locale ‘en’:

{
  "uuid": ...,
  "name": "Outpatient Visit",
  "label_en": "OPD Visit",
  "label_fr": "Visite ambulatoire",
  ...
}

In locale ‘fr’:

{
  "uuid": ...,
  "name": "Outpatient Visit",
  "label_en": "OPD Visit",
  "label_fr": "Visite ambulatoire",
  ...
}

And the frontend code would attempt to find a label in accordance to the user’s session locale, or default to the metadata name. On the frontend this would allow to switch between supported locales while minimizing server calls.

1 Like

Am fine with this as long as it is not the default.

Is there a reason why you are using label instead of name? For instance, label_en instead of name_en?

Because at first I was imagining this display string to be the L10n’d value of the new i18n code, that’s how it was differing from the name.

But it could as well be name_xy. Perhaps label_xy or display_xy is more explicit as to how it is supposed to be used by client side code.

I didn’t realise that PIH’s technique of i18n of metadata was already supported! As far as we are concerned, this is good enough to quite a large extent…

Although, having a way to provide the display values for all supported locales would be a really cool thing to have to enable a frontend logic to switch between locales without making much server calls.

@mksrom so this means that if for each piece of metadata we provide a value for the i18n key built as

Java class name + "." + entity UUID

then REST WS will provide a L10n’d display property accordingly for that metadata entity.

Example for the location “Amani Hospital” whose UUID is aff27d58-a15c-49a6-9beb-d30dcfc0c66e. Here is how the ‘messageproperties’ domain would be set up in the OpenMRS config:

locations_en.properties

org.openmrs.Location.aff27d58-a15c-49a6-9beb-d30dcfc0c66e=Amani Hospital

locations_kh.properties

org.openmrs.Location.aff27d58-a15c-49a6-9beb-d30dcfc0c66e=មន្ទីរពេទ្យអាម៉ានី

locations_fr.properties

org.openmrs.Location.aff27d58-a15c-49a6-9beb-d30dcfc0c66e=Hôpital Amani

And therefore here is how the metadata resources would look like:

REST resource in ‘en’

{
  "uuid":  "aff27d58-a15c-49a6-9beb-d30dcfc0c66e",
  "name": "Amani Hospital",
  "display": "Amani Hospital"
  ...
}

REST resource in ‘kh’

{
  "uuid":  "aff27d58-a15c-49a6-9beb-d30dcfc0c66e",
  "name": "Amani Hospital",
  "display": "មន្ទីរពេទ្យអាម៉ានី"
  ...
}

REST resource in ‘fr’

{
  "uuid":  "aff27d58-a15c-49a6-9beb-d30dcfc0c66e",
  "name": "Amani Hospital",
  "display": "Hôpital Amani"
  ...
}

That should be good enough for OpenMRS 3.0 right?

FWIW: UI Framework Reference Guide - Documentation - OpenMRS Wiki

@mksd this makes sense to me.

You could also imagine it being quite cheap to also support names as message codes, so in additional to checking message codes for fqn.uuid for a given piece of metadata, you also just tried checking message codes for “name” of a given piece of metadata. And having “display” property on the REST resource that returns this.

One the Iniz side, would be cool if each metadata domain could abstract this away too. For example, on the location domain if you add columns like “name_en”, “name_fr”, etc, then Iniz would basically add these with the appropriate keys to a message source that could return them at runtime for the given location.

This would greatly reduce errors and annoyances of having to copy lots of uuids around and use them as keys in configuration files…

Mike

2 Likes

I had forgotten we had implemented that for the REST web services module as well…

Longer-term, I think I’d be in favor of adding a “i18nCode” code property to BaseOpenmrsMetadata, and then support returning “display_en”, “display_fr”, etc as @mksd . My initial thought would be to use “display_" over "label_” or “name_*” but could be swayed by arguments…

Take care, Mark

Nice, I like that, it would be quite easy I think to make this a generic process for all domains. TBH when this exists in Iniz, there is not even much incentive to even touch REST WS further (in reference to supporting using the metadata name as a i18n code.)

@mogoodrich since there is already a display property, I think we should just stick with it.

Great stuff, thanks all for your input. Taking away:

  1. Ensure that Iniz makes it easy to provide i18n names as @mseaton suggested.
  2. Expand REST WS so that it can fill display_xy for all supported locales.
1 Like

Thankfully we have an encyclopedia among us :slight_smile: :point_right: Issue with OAuth2login user creation - #11 by mksd

I created the following ticket: