[#248] Support for loading-time L10n of metadata names and descriptions based on Context's locale. #249
Conversation
…ta from translations in the context locale
There was a problem hiding this comment.
Ok sorry, just read the issue description...
So, at Iniz-time, the metadata name and description are localised as per the system's locale, correct?
This should not have any impact in the UX (in theory), but is useful in the local context? As you describe when flattening data directly out of the database, etc?
If that's right then this really a metadata regionalization/localization feature so that the database looks "local" to consumers in country. Then maybe, rather than the _translate_ directive, we could use _localize_ (or _L10n_)?
Sure, we can have this named whatever we want. I'll take direction from you on this, and make it so. |
mks-d
changed the title
| String name = line.getName(true); | ||
| if (TRANSLATE.equals(name)) { | ||
| String code = "ui.i18n." + metadata.getClass().getSimpleName() + ".name." + metadata.getUuid(); | ||
| name = messageSourceService.getMessage(code); |
There was a problem hiding this comment.
What locale is going to be used here? My question is at a functional level, OpenMRS' default locale I would assume?
There was a problem hiding this comment.
Based on the functioning of the unit tests, it is whatever is returned from Context.getLocale(), which will end up as LocaleUtility.getDefaultLocale(). This will ultimately be whatever is in the GP named "default_locale", and if this is either unset or can't be retrieved, will fall back to "en_GB"
There was a problem hiding this comment.
Does that describe the feature appropriately:
Support for loading-time L10n of metadata names and descriptions based on Context's locale.
?
It would need to be documented well of course, but is that functional intent?
There was a problem hiding this comment.
Yes, but it raises a good point. We might want to change this and also be more explicit in the code. Because if someone were to decide to re-run an initializer load themselves (eg. we have an admin feature to allow this for development), then this will pick up their locale, rather than a standard system locale. So maybe the translation here should explicitly pass in the locale that is returned from LocaleUtility.getDefaultLocale(), so that this will always behave consistently in the same system. Then, the documentation can describe this as "...based on the system default locale (default_locale global property)" or something.
There was a problem hiding this comment.
@mseaton we all left this PR and thread hanging, what's your take? Should we resume reviewing it, or should be close it for now?
mks-d
changed the title
Note, if this approach is approved, then there are likely additional changes to expand this beyond the encounter type domain, by changing other metadata loaders to extend the
BaseMetadataLineProcessorrather than theBaseLineProcessorand then update the way the set the name and description properties.For now, this works and is tested against the
EncounterTypedomain.We'll also need to update the README and Release Notes. I can add that in if this is agreed on as a general approach. I expect there may be alternative ideas for what the constant is named (currently "
_translate_", but I assume others may have better ideas here).