mekomsolutions / openmrs-module-initializer

The OpenMRS Initializer module is an API-only module that processes the content of the configuration folder when it is found inside OpenMRS' application data directory.
MIT License
23 stars 79 forks source link

Support ordered loading of message.properties files #145

Closed mogoodrich closed 2 years ago

mogoodrich commented 3 years ago

Like the "_order" property that can be used to specify the order in which csv-based metadata is loaded, we'd like to be able to order the way messages.properties files are loaded by Iniz.

Use case: In the PIH EMR, country-specific implementations want to override a limited number of messages properties... ie the Peru implementation wants a slightly different Spanish translation for Consult Note than our Mexico implementation. We'd like to ship a standard "messages_encountertypes_es.properties" with all our implementations, and then ship a specific "messages_encountertypes_peru_es.properties" that overrides the name of this single encounter type. I think we can do by specifying the order in which messages properties are loaded, and then enforcing this loading order here.

We could support this by allowing the first field in a messages.properties file to be "_order", a la:

_order:1000
my.code: My Translation
my.other.code: My Other Translation
brandones commented 2 years ago

@mks-d What do you think of this? I think we (PIH) could implement it if it seems good to you.

mks-d commented 2 years ago

It's like CSV files right, there is no proper way to attach metadata to Java properties file, so we need to hack it into its content.

Will anybody ever need the key _order? I have to admit that most likely not.

I guess my issue is more that we generate an actual key-value pair. In the case of CSVs we don't interfere with the values under the header, so that if anyone ever needed that header to provide values, they could. The Java properties equivalent would be more something like:

_order_1000:

And if anybody ever needed that improbable key, well they still could.

I am certainly overthinking this, feel free to implement the suggested solution.

mseaton commented 2 years ago

@mogoodrich and @brandones - so we still need this feature? With the new message source implemented in #155 I would think that, at least for our needs at PIH, we can achieve this easily by just having messages_es.properties that can be specified further via messages_es_PE.properties and messages_es_MX.properties, and then ensuring that the default and allowed locales used by Peru and Mexico are es_PE and es_MX respectively. This wouldn't have worked reliably before some of the revisions to the message source in #155 but should do so if/when that is merged.

brandones commented 2 years ago

We don't, since we only run one implementation per country. We're sort of abusing the country coded files, because the things in "es_PE" aren't actually differences between Peruvian and Mexican Spanish, but idiosyncrasies of the Peru implementation of OpenMRS. I think having an organic way to define message code "overrides" would be valuable.

mogoodrich commented 2 years ago

yeah, I tend to agree with @brandones ... it would still be nice to support this because "the things in "es_PE" aren't actually differences between Peruvian and Mexican Spanish, but idiosyncrasies of the Peru implementation of OpenMRS", but not a blocker for us at this point.

mogoodrich commented 2 years ago

This has come up again and is potentially going to end up in the next PIH sprint... unless there are objections, let's go with the pattern that @mks-d suggests:

_order_1000:

mogoodrich commented 2 years ago

I removed the code link I provided above, as I think it is outdated, I think the correct link is here:

https://github.com/mekomsolutions/openmrs-module- initializer/blob/master/api/src/main/java/org/openmrs/module/initializer/InitializerMessageSource.java#L195

Also, it's quite possible there's other code we could steal from in Iniz that handles this for other domains.

mseaton commented 2 years ago

I'm still on the fence about this @mogoodrich . There is no standard domain loader in Iniz for message properties, there is just the custom message source that consumes them. Let's take this offline until we decide.

mseaton commented 2 years ago

I have a PR for this here: #183 . I did not take the above approach, but rather just used a normal message property key/value pair. I'm OK with reserving the "_order" code for this. If we are worried about this, we could come up with a more unique code variant, but I'm inclined to stick with _order. Handling this as a key/value pair is a lot more straightforward and intuitive.

mseaton commented 2 years ago

@mks-d and @rbuisson I tagged you on the PR, not sure if you are the right people. Please include others as appropriate. @mogoodrich FYI (not able to add you as a reviewer for some reason)