OBPIH-6410 Allow configuring country specific localization #4681

awalkowiak · 2024-06-18T18:23:28Z

"Some" context https://pihemr.atlassian.net/browse/OBPIH-6410?focusedCommentId=153567

jmiranda · 2024-06-18T18:26:40Z

@awalkowiak Please give me a few days to get to this one.

EWaterman · 2024-06-18T19:07:35Z

grails-app/utils/org/pih/warehouse/LocalizationUtil.groovy

@@ -26,9 +26,41 @@ class LocalizationUtil {
 return localizationService.currentLocale
 }

+ /**


An alternative to this would be to extend the Locale class and give it another constructor but I don't know if there's much advantage of that over what you've done

EWaterman · 2024-06-18T19:16:38Z

grails-app/services/org/pih/warehouse/product/ProductService.groovy

@@ -1449,7 +1449,7 @@ class ProductService {

 Product addSynonymToProduct(String productId, String synonymTypeCodeName, String synonymValue, String localeName) {
 Product product = Product.get(productId)
- Locale locale = localeName ? new Locale(localeName) : null
+ Locale locale = localeName ? LocalizationUtil.getLocale(localeName) : null


Not a question to be resolved here, but I'm wondering if there's a fancier way to do this so that we don't need to bake all this localization logic into our services.

It'd be awesome if we could mark a Domain field as "localized" and then whenever we do a look up on that field it'd automatically also search for the synonyms.

Yeah, I guess this is a more Synonyms related topic suiting for a TD discovery or some OBIG

EWaterman · 2024-06-18T19:22:43Z

grails-app/utils/org/pih/warehouse/LocalizationUtil.groovy

+ locale.setDefault(new Locale(Holders.config.openboxes.locale.defaultLocale ?: "en"))
+ return locale
+ }
+
 static List<Locale> getSupportedLocales() {


Since locales are static, we could consider caching them in a map (maybe in the LocalizationService) so that we don't need to reconstruct them every time we do a lookup. Currently, every time we call this method we're re-initializing all of the Locale objects. That's not a big deal since it's not really a lot of computation work, but still a good practice IMO.

If we wanted to get really fancy, we could even build that map on app startup based on the locales we have defined in our properties.

I think this is also potentially something that I would move to the separate "further improvements in the supported locale" ticket, but hold until more reviews

EWaterman · 2024-06-18T19:23:52Z

grails-app/i18n/messages_es_MX.properties

@@ -0,0 +1 @@
+default.number.format=\# \# \#, \# \# \#, \# \# \#0.00


is this being used yet?

This was an "initial" request from the ticket, so I just added it here to not leave it empty

EWaterman · 2024-06-18T19:25:01Z

grails-app/services/org/pih/warehouse/core/LocalizationService.groovy

@@ -69,51 +69,11 @@ class LocalizationService {
 */
 Properties getMessagesProperties(Locale locale) {
 Properties messagesProperties = new Properties()
- def messagesPropertiesFilename = (locale && locale.language != "en" && locale.language != 'null') ? "messages_${locale.language}.properties" : "messages.properties"
+ def messagesPropertiesFilename = (locale && locale.language != "en" && locale.language != 'null') ? "messages_${locale.toString()}.properties" : "messages.properties"


in what situations do we want locale.toString() vs locale.getDisplayName()? I see we use both

We want to find a message properties file corresponding to the current locale. toString() concatenates language code with country code together with an underscore (or returns just language code if no country code). So for Spanish, we are getting just es -> messages_es.properties and for Mexican Spanish, we will get es_MX -> messages_es_MX.properties

EWaterman · 2024-06-18T19:25:49Z

grails-app/services/org/pih/warehouse/core/LocalizationService.groovy


 def resource = grailsResourceLocator.findResourceForURI('classpath:' + messagesPropertiesFilename)
 messagesProperties.load(resource.inputStream)

 return messagesProperties
 }
-


unused code?

kchelstowski

I would make sure none adjustments have to be done in the InitializationInterceptor, but looking quickly at the code, I think none have to be done.

kchelstowski · 2024-06-19T07:39:39Z

grails-app/utils/org/pih/warehouse/LocalizationUtil.groovy

+ Locale locale
+
+ if (localeCode?.contains("_")) {
+ String[] localeCodeParts = localeCode.split("_")


nitpicky: I'd add the "_" separator to the Constants.

kchelstowski · 2024-06-19T07:41:58Z

grails-app/utils/org/pih/warehouse/LocalizationUtil.groovy

+ }
+
+ locale = new Locale(localeCode)
+ locale.setDefault(new Locale(Holders.config.openboxes.locale.defaultLocale ?: "en"))


I would move the new Locale(...) ?: "en" to a separate variable for a better readability.

kchelstowski · 2024-06-19T07:43:37Z

grails-app/views/admin/showSettings.gsp

@@ -1,4 +1,4 @@
-<%@ page import="org.pih.warehouse.core.RoleType" %>
+<%@ page import="org.pih.warehouse.LocalizationUtil; org.pih.warehouse.core.RoleType" %>


nitpicky: I know it was done automaticallty, but we used to separate the imports in gsp templates to be one import per line

Yeah this is my laziness since I was doing the automatic import and noticed it while creating the PR it's in one line, I'll fix that

kchelstowski · 2024-06-19T07:44:01Z

grails-app/views/user/create.gsp

@@ -1,5 +1,5 @@

-<%@ page import="org.pih.warehouse.core.User" %>
+<%@ page import="org.pih.warehouse.LocalizationUtil; org.pih.warehouse.core.User" %>


same as above

awalkowiak · 2024-06-19T08:01:12Z

@kchelstowski

I would make sure none adjustments have to be done in the InitializationInterceptor, but looking quickly at the code, I think none have to be done.

Initially, I was making the changes there too, because I was pulling through this new util every "defaultLocale" as well, but I decided to skip that one for three reasons:

I just assumed localizationModeLocale is crowdin-specific and it will always be ach for us
I just assumed no one would have country-specific as the default for now (at least for now)
I want to improve the supportedLocales in some further (ideally) post-release ticket/changes

kchelstowski · 2024-06-19T08:03:07Z

@awalkowiak I think even if the users have country-specific language, this should be handled already by the:

session?.locale ?: session.user?.locale

assuming the session locale would be set correctly (which you've probably done)
so I think the only edge case I saw there is the last fallback:

new Locale(grailsApplication.config.openboxes.locale.defaultLocale ?: "en")

but as long as no one has their defaultLocale set to es (assuming someone has overwritten the config), we should be fine.

alannadolny · 2024-06-19T09:27:38Z

grails-app/utils/org/pih/warehouse/LocalizationUtil.groovy

+ }
+
+ locale = new Locale(localeCode)
+ locale.setDefault(new Locale(Holders.config.openboxes.locale.defaultLocale ?: "en"))


I think it would be better to use "en" from the Locale class instead of hardcoding it.

awalkowiak · 2024-06-20T13:48:26Z

@jmiranda I am merging this one, but feel free to leave a post-merge review

OBPIH-6410 Allow configuring country specific localization

321fb00

awalkowiak requested review from jmiranda, EWaterman, drodzewicz, alannadolny and kchelstowski June 18, 2024 18:23

EWaterman reviewed Jun 18, 2024

View reviewed changes

kchelstowski reviewed Jun 19, 2024

View reviewed changes

alannadolny reviewed Jun 19, 2024

View reviewed changes

OBPIH-6410 Minor improvements after review

92c1e10

kchelstowski approved these changes Jun 20, 2024

View reviewed changes

alannadolny approved these changes Jun 20, 2024

View reviewed changes

awalkowiak merged commit ad787a2 into develop Jun 20, 2024
2 checks passed

awalkowiak deleted the OBPIH-6410 branch June 20, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-6410 Allow configuring country specific localization #4681

OBPIH-6410 Allow configuring country specific localization #4681

awalkowiak commented Jun 18, 2024

jmiranda commented Jun 18, 2024

EWaterman Jun 18, 2024

EWaterman Jun 18, 2024

awalkowiak Jun 19, 2024

EWaterman Jun 18, 2024

awalkowiak Jun 19, 2024

EWaterman Jun 18, 2024

awalkowiak Jun 19, 2024

EWaterman Jun 18, 2024 •

edited

Loading

awalkowiak Jun 19, 2024

EWaterman Jun 18, 2024

awalkowiak Jun 19, 2024

kchelstowski left a comment

kchelstowski Jun 19, 2024

kchelstowski Jun 19, 2024

kchelstowski Jun 19, 2024

awalkowiak Jun 19, 2024

kchelstowski Jun 19, 2024

awalkowiak commented Jun 19, 2024

kchelstowski commented Jun 19, 2024 •

edited

Loading

alannadolny Jun 19, 2024

awalkowiak commented Jun 20, 2024

		@@ -0,0 +1 @@
		default.number.format=\# \# \#, \# \# \#, \# \# \#0.00

		@@ -1,4 +1,4 @@
		<%@ page import="org.pih.warehouse.core.RoleType" %>
		<%@ page import="org.pih.warehouse.LocalizationUtil; org.pih.warehouse.core.RoleType" %>

		@@ -1,5 +1,5 @@

		<%@ page import="org.pih.warehouse.core.User" %>
		<%@ page import="org.pih.warehouse.LocalizationUtil; org.pih.warehouse.core.User" %>

OBPIH-6410 Allow configuring country specific localization #4681

OBPIH-6410 Allow configuring country specific localization #4681

Conversation

awalkowiak commented Jun 18, 2024

jmiranda commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EWaterman Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalkowiak commented Jun 19, 2024

kchelstowski commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

awalkowiak commented Jun 20, 2024

EWaterman Jun 18, 2024 •

edited

Loading

kchelstowski commented Jun 19, 2024 •

edited

Loading