OBPIH-5161+5246 Make user transition in/out of translation mode smoother #3738

kchelstowski · 2022-12-27T15:52:57Z

getting rid of useDebugLocale logic, which Justin mentioned in the ticket
not allowing user to set her/his default locale to translation mode locale
the localization mode is activated automatically when changing the language for Acoli
the localization mode is deactivated when changing language to any other than Acoli when being in localization mode
when disabling localization mode by button, we are brought back to the locale, that we had set before enabling localization mode

Bonus:

changing the webpack config, so all babel's and eslint's stuff works also fine for .js files, not only .jsx - example: optional chaining or nullish coalescing operator, which were added by me not a long time ago, but were not working for .js files

…ff is understood by the .js files, not only .jsx files

…ugLocale and return translationModeLocale to sessionReducer

… get rid of useDebugLocale for enable/disable localization mode

…e and add styling for buttons

…ult locale to translation mode locale

alannadolny · 2022-12-27T20:56:18Z

src/js/components/Layout/Footer.jsx

+                key={language.code}
+                href="/openboxes/user/enableLocalizationMode"
+              >
+                <Translate id={`react.default.${language.name.toLowerCase()}.label`} /> {''}


Is {''} necessary?

Probably not, but since it has not been code that I've written, I did not catch such details

alannadolny · 2022-12-27T21:08:53Z

src/js/components/Layout/Footer.jsx

+const mapDispatchToProps = {
+  changeLocale: changeCurrentLocale,
+  setLanguage: setActiveLanguage,


What do you think about using useDispatch() instead of mapDispatchToProps? I found an interesting discussion on stackoverflow about this two approach Should I use useselector/useDispatch instead of mapStateToProps and i am curious what do you think

I don't see any pros/cons of using one versus another. I think it's rather what you prefer and what is more useful in each scenario (for example you can't use mapDispatchToProps inside a custom hook, so you are obligated to use useDispatch().
What I'm sure about is that we shouldn't mix both if possible and probably in "regular" components stick with connect approach as we've done for whole project, as it might be easier to read and debug, because we are used to this convention.
@drodzewicz any additional thoughts?

jmiranda · 2022-12-28T05:17:09Z

I need a bit more time to review this one.

kchelstowski · 2022-12-28T09:41:03Z

Marking it as a draft for a while, as I will probably want to bring back useDebugLocale for some cases, because creating vars with locale == translationModeLocale for many GSP pages does not seem the best idea for me.

kchelstowski · 2022-12-28T13:06:44Z

@jmiranda it's now ready for review. I've brought back useDebugLocale in the latest commit for some use cases, let me know if it's better to keep it like I did, or remove it completely and inject logic locale == translationModeLocale for each GSP page where we used to use if (session.useDebugLocale).
I personally believe that the latest version should be more readable and less complexed, but I leave it for you to decide :)

jmiranda · 2022-12-28T22:05:23Z

grails-app/controllers/org/pih/warehouse/api/ApiController.groovy

@@ -88,6 +88,10 @@ class ApiController {
        def localizationMode
        def locale = localizationService.getCurrentLocale()
        Object[] emptyArgs = [] as Object []
+        def translationModeLocale = grailsApplication.config.openboxes.locale.translationModeLocale
+        // If current locale is equal to translation mode locale, we are in localization mode
+        session.useDebugLocale = locale == new Locale(translationModeLocale)


Instead of sprinkling this logic everywhere, we should probably have it done in a filter. InitializationFilters seems to be the best candidate but I'm open to suggestions.

sounds good to me, I haven't thought about such a solution

jmiranda · 2022-12-28T22:05:31Z

grails-app/controllers/org/pih/warehouse/api/ApiController.groovy

@@ -57,12 +57,12 @@ class ApiController {

    @CacheFlush(["megamenuCache"])
    def chooseLocale = {
-        Locale locale = localizationService.getLocale(params.id)
-        if (!locale) {
+        Locale newLocale = localizationService.getLocale(params.id)


This seems unnecessary.

what do you mean as unnecessary here? I'm a bit confused, because I only changed the naming from locale to newLocale and I'm not sure if either changing of name seems unnecessary or this part: localizationService.getLocale(params.id)

I meant changing the variable name isn't necessary and I want to avoid unnecessary changes as much as possible since it complicates the rebase process for OBGM.

jmiranda · 2022-12-28T22:07:24Z

grails-app/controllers/org/pih/warehouse/api/ApiController.groovy

@@ -197,7 +201,8 @@ class ApiController {
                currencyCode         : currencyCode,
                localizedHelpScoutKey: localizedHelpScoutKey,
                isHelpScoutEnabled   : isHelpScoutEnabled,
-                localizationModeEnabled : localizationModeEnabled
+                localizationModeEnabled : localizationModeEnabled,
+                translationModeLocale : translationModeLocale


This is my fault since I kept referring to this as "translation mode", but let's either use the existing name "localization mode" or convert everything to "translation mode".

jmiranda · 2022-12-28T22:12:32Z

grails-app/controllers/org/pih/warehouse/user/UserController.groovy

+        // If we are in localization mode and want to change the language through the footer on React side
+        // we want to make sure we set the language to the clicked one, not for the previous locale
+        // we want to set the prev locale only if we disable localization mode through "Disable localization mode" button
+        Locale localeToSet = params?.localeToSet ? new Locale(params.localeToSet) : session?.prevLocale


Let's just call this request parameter and variable "locale" (not localeToSet) since that's what it's been called. I'm worried that adding a new request parameter will cause confusion.

For now it is only used for React part, as I didn't have an idea how to determine whether I disable localization mode through button "Disable localization mode" or I just change the language.
When disabling the localization mode through button "Disable localization mode" we want to go back to previous chosen locale.
When changing the language while being in localization mode, we want to disable localization mode, but we don't want to go back to previous chosen locale, but to change it for the language we've just clicked, so that's why I had to figure out some way to determine this.
For GSP part it's unnecessary, because we always change the language through updateUserAuthLocale.
For React part I just add something like this when changing the language being in localization mode:

href={`/openboxes/user/disableLocalizationMode?localeToSet=${language.code}`}

I know it may cause confusion, but I couldn't figure out anything better and that's why I make the checking if this param is provided and that's why I added a comment above this.
If you have any idea how I could deal with this case, I'd love to hear from you

jmiranda · 2022-12-28T22:31:08Z

grails-app/controllers/org/pih/warehouse/user/UserController.groovy

    }

    def enableLocalizationMode = {
+        // We want to store the previous locale, so we can go back to it when disabling localization mode through button
+        session.prevLocale = localizationService.getCurrentLocale()


Sorry to be nit-picky but let's avoid using abbreviations. session.previousLocale is more readable.

Also is there any difference between getting the current locale versus using session.locale?

Yeah, let me explain - when you log in and do not neither change the language nor enable localization mode before, session.locale is still null - your "initial" locale is taken from session.user.locale. We set the session.locale when we either change the language or like you see line below, we enable the localization mode.
This is why I modified getCurrentLocale() method, because at the beginning, we do not have session.locale

jmiranda · 2022-12-28T22:44:48Z

grails-app/controllers/org/pih/warehouse/user/UserController.groovy

+        // if no locale specified, do nothing
+        if (!params.locale) {
+            redirect(controller: "dashboard", action: "index")
+        }


I know this isn't code you wrote, but we should probably remove the redirects since that's handled at the end. And instead of handling the

if (params.locale) { // convert the passed locale parameter to an actual locale Locale locale = new Locale(params.locale) if (locale) { // Change the locale session.locale = locale // remove all redirects since they aren't going to do anything // Check if chosen locale is equal to translation mode locale, if so, enable localization mode session.useDebugLocale = localizationService.isLocalizationModeEnabled() } } log.info "Redirecting to " + params?.targetUri if (params?.targetUri) { redirect(uri: params.targetUri) return } // redirect to the dashboard redirect(controller: "dashboard", action: "index")

LocalizationService

isLocalizationModeEnabled() { def localizationModeLocale = new Locale(grailsApplication.config.openboxes.locale.translationModeLocale) return currentLocale == localizationModeLocale

jmiranda · 2022-12-28T22:47:12Z

grails-app/taglib/org/pih/warehouse/MessageTagLib.groovy

@@ -28,7 +28,7 @@ class MessageTagLib {
        boolean databaseStoreEnabled = grailsApplication.config.openboxes.locale.custom.enabled
        if (!databaseStoreEnabled) {
            Locale defaultLocale = new Locale(grailsApplication.config.openboxes.locale.defaultLocale)
-            attrs.locale = attrs.locale ?: session?.user?.locale ?: session.locale ?: defaultLocale
+            attrs.locale = attrs.locale ?: session?.locale ?: session?.user?.locale ?: defaultLocale


jmiranda · 2022-12-28T22:47:25Z

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

+        // First search for session locale, if none set, then search for user's default locale, otherwise get the default locale
+        def session = RequestContextHolder.currentRequestAttributes().getSession()
+        if (session?.locale) {
+            return session.locale


jmiranda · 2023-01-04T16:32:58Z

chooseLocale should implement the same rules. but @kchelstowski says he's implemented the rules properly in the initialization filter. there's a weird case where the state is not set until we get to the controller and therefore if there's logic that depends on that state in the filters, then we'll have one request where the logic will be wrong. I'll pull this down and check it.

kchelstowski · 2023-01-05T09:33:52Z

@jmiranda I played with it again and I reminded myself why I couldn't just stick with the chooseLocale for every case: changing language, disabling/enabling localization mode for the React side. The problem is that api requests don't refresh the page in this case - it just dispatches an action in "a React way" and the problem is that we inject the crowdin script in the main.gsp file - if we are in localization mode, it injects the script, if we are not, it doesn't and for api requests it doesn't trigger the main.gsp to "reload" - it only happens if you refresh the page (which happens when you click a link that redirects us to the enable/disable localization mode function).

 <g:if test="${session.useDebugLocale}">
        <script type="text/javascript">
            var _jipt = [];
            _jipt.push(['project', 'openboxes']);
        </script>
        <script type="text/javascript" src="//cdn.crowdin.com/jipt/jipt.js"></script>
    </g:if>

So I believe that if we'd like to stick with the chooseLocale for React for each of those cases, we should find a way to inject the crowdin script inside React, because now it is handled by GSP.
Saying that, I think for now it's ready to be merged unless you have some more questions

kchelstowski added 9 commits December 27, 2022 16:24

OBPIH-5161 Add .js extension to babel loader, so all eslint/babel stu…

ed6bbca

…ff is understood by the .js files, not only .jsx files

OBPIH-5161 Add translation mode locale to session reducer

48dbbf4

OBPIH-5161 Add translation mode locale constant to Config

63375fc

OBPIH-5161 Modify getAppContext and chooseLocale to get rid of useDeb…

6bf118d

…ugLocale and return translationModeLocale to sessionReducer

OBPIH-5161 Modify getCurrentLocale method

17662bb

OBPIH-5161 Do not update the user locale when changing the locale and…

c0a5a94

… get rid of useDebugLocale for enable/disable localization mode

OBPIH-5161 React - modify footer buttons logic for changing the local…

a7f49ac

…e and add styling for buttons

OBPIH-5161 GSP - get rid of useDebugLocale

cadac87

OBPIH-5161 Add validation for user, not to allow setting her/his defa…

4bb824d

…ult locale to translation mode locale

kchelstowski requested review from jmiranda, drodzewicz and alannadolny December 27, 2022 15:53

alannadolny reviewed Dec 27, 2022

View reviewed changes

kchelstowski added work in progress do not merge labels Dec 28, 2022

kchelstowski marked this pull request as draft December 28, 2022 09:43

kchelstowski force-pushed the OBPIH-5161 branch from 2b8da66 to d497c9c Compare December 28, 2022 12:53

OBPIH-5161 Bring back useDebugLocale logic for some cases

964c581

kchelstowski force-pushed the OBPIH-5161 branch from d497c9c to 964c581 Compare December 28, 2022 13:02

kchelstowski marked this pull request as ready for review December 28, 2022 13:04

kchelstowski removed work in progress do not merge labels Dec 28, 2022

OBPIH-5161 Fix editing locale validation

07202aa

jmiranda requested changes Dec 28, 2022

View reviewed changes

kchelstowski force-pushed the OBPIH-5161 branch from 740be28 to 9f6e3ab Compare January 2, 2023 09:29

OBPIH-5161 Fixes after review

a3548c6

kchelstowski force-pushed the OBPIH-5161 branch from 9f6e3ab to a3548c6 Compare January 2, 2023 09:44

kchelstowski requested a review from jmiranda January 2, 2023 09:45

jmiranda added the needs attention label Jan 4, 2023

kchelstowski added the awaiting feedback label Jan 5, 2023

jmiranda approved these changes Jan 5, 2023

View reviewed changes

jmiranda merged commit 6bfc10d into develop Jan 5, 2023

jmiranda deleted the OBPIH-5161 branch January 5, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5161+5246 Make user transition in/out of translation mode smoother #3738

OBPIH-5161+5246 Make user transition in/out of translation mode smoother #3738

kchelstowski commented Dec 27, 2022

alannadolny Dec 27, 2022

kchelstowski Dec 28, 2022

alannadolny Dec 27, 2022

kchelstowski Dec 28, 2022

jmiranda commented Dec 28, 2022

kchelstowski commented Dec 28, 2022

kchelstowski commented Dec 28, 2022

jmiranda Dec 28, 2022

kchelstowski Dec 29, 2022

jmiranda Dec 28, 2022

kchelstowski Dec 29, 2022 •

edited

jmiranda Jan 3, 2023

jmiranda Dec 28, 2022

jmiranda Dec 28, 2022

kchelstowski Dec 29, 2022

jmiranda Dec 28, 2022

jmiranda Dec 28, 2022

kchelstowski Dec 29, 2022 •

edited

jmiranda Dec 28, 2022

jmiranda Dec 28, 2022

jmiranda Dec 28, 2022

jmiranda commented Jan 4, 2023

kchelstowski commented Jan 5, 2023 •

edited

OBPIH-5161+5246 Make user transition in/out of translation mode smoother #3738

OBPIH-5161+5246 Make user transition in/out of translation mode smoother #3738

Conversation

kchelstowski commented Dec 27, 2022

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

jmiranda commented Dec 28, 2022

kchelstowski commented Dec 28, 2022

kchelstowski commented Dec 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Dec 29, 2022 • edited

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

kchelstowski Dec 29, 2022 • edited

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

jmiranda commented Jan 4, 2023

kchelstowski commented Jan 5, 2023 • edited

kchelstowski Dec 29, 2022 •

edited

kchelstowski Dec 29, 2022 •

edited

kchelstowski commented Jan 5, 2023 •

edited