Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OBPIH-5161 Set locale for every request #3763

Merged
merged 1 commit into from
Jan 10, 2023
Merged

OBPIH-5161 Set locale for every request #3763

merged 1 commit into from
Jan 10, 2023

Conversation

kchelstowski
Copy link
Collaborator

@kchelstowski kchelstowski commented Jan 9, 2023

…olving the big spike), fix menu (translations) fetching when switching locales
After a long and exhausting round of investigation I have a very interesting summary of known for the long time issue with <g:message> tags not being translatable by the crowdin vs <warehouse:message> being translatable.

After we lost this "workaround" I found for translating <g:message> tags when merging the changes to transition in/out to/from localization mode, we again faced an issue with <g:message> not being translatable by crowdin anywhere (because the “workaround” which we did not really know what it is about, we thought it’s just a random thing was lost). It was a big regression “issue”, because since we had a workaround to translate those and suddenly lost that workaround, it would mean that our previous work (especially Manon’s work with translating) had no-sense. Why I marked the “issue” word inside the quotation mark is that actually it was only a workaround which we didn’t really understand.
After many attempts from each of us to find the real cause of <g:message> not always being translatable by the crowdin (the spike ticket), I finally found what really caused that issue of <g:message> not being translatable by the crowdin.
Let me explain:
What turned out to be this "workaround" was actually passing the lang param to the updateAuthUserLocale method (we called this method when changing the language via links in the footer on the GSP side) and it was changing "grails' language", so that the grails and all its tags including <g:message> knew what the current language was.
From what I've read from the grails_i18n's docs:

You can switch locales by simply passing a parameter called lang to Grails as a request parameter:

/?lang=es

Grails will automatically switch the user’s locale and store it in a cookie so subsequent requests will have the new header.

source: link (point 3.2)

and since I changed logic a bit for changing the language, so that when changing the language to Acoli we were not using updateAuthUserLocale, but actually calling the enableLocalizationMode explicitly via footer language link/button, we were not passing the lang param to that action, so the <g:message> and whole grails still didn't know that the language is set to ach, as this information is stored somehow in a cookie. This lang param and our session.locale is NOT the same and grails' built-in tags did not "pull" the language from session.locale, but they were looking on actual lang.
This is also why I recommended not a long time ago to change the language to Acoli on the GSP side in order to make the "workaround" work (because on React side we did not pass the lang as param when changing the language and that explains why when changing the language to Acoli on the React side and then going to some GSP pages, the <g:message> tags were still not translatable - because the lang param was not passed to the method on the React side).
Every issue that we've faced with crowdin so far, especially with this <g:message> stuff seems really logical to me after understanding what really caused the issue.

Going to what actually needed to be done in the shortest way was to call the RCU and setting the locale via InitializationFilters (so we don't have to pass lang param for each request of language change).
This line:

RCU.getLocaleResolver(request).setLocale(request, response, locale)

is actually equal to passing ?lang as param to a request, so that's why I thought doing in inside the InitializationFilters is the best idea to have this logic in one place.

As for changing the language on obdev and having menu translated to the locale that is equal to user's default locale I couldn't spot it earlier locally, because I think (I “think”, because I do not have access to obdev storage to check it and as Justin was out today and Artur is out, I had to “blind guess” it basing on my experience and intuition) that on obdev this setting in config:

openboxes.locale.custom.enabled

is set to true (it has to be overwritten via openboxes-config.properties, because by default it is set to false as I have locally on develop branch) and that's why the first if was not executed and in the further if's inside MessageTagLib the user.locale was "higher" in order than session.locale which should be checked firstly in order to choose the proper translation.
What needed to be done is just changing the order, so that session.locale is checked before the session.user.locale.

With this fix I also answered for the spike we had for the g:message vs warehouse:message stuff: so that we do not need any workarounds and can change the language/enable localization mode WHEREVER we want and everything should be working fine. I really do feel that this will improve the work with crowdin for Manon and the users, as it’s a big game changer going further.

…olving the big spike), fix menu (translations) fetching when switching locales
@jmiranda jmiranda changed the title OBPIH-5161 'HOTFIX' - Make all g.message translatable by crowdin (res… OBPIH-5161 Set locale for every request Jan 10, 2023
@jmiranda jmiranda merged commit 2a6fecc into develop Jan 10, 2023
@jmiranda jmiranda deleted the OBPIH-5161-fix branch January 10, 2023 03:13
@drodzewicz
Copy link
Collaborator

greate work!

@jmiranda
Copy link
Member

😊 Gosh. Thank you @drodzewicz. But all I did was merge the PR.

Oh nevermind ... you were talking to @kchelstowski. My bad.

@drodzewicz
Copy link
Collaborator

you too @jmiranda, great work! The best merge I've seen in a while

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants