Added Permission check before setting Preferred Language by parasharrajat · Pull Request #5943 · Expensify/App

parasharrajat · 2021-10-19T02:45:17Z

Details

Fixed Issues

$ #5286

Tests | QA Steps

Launch the app
Log in with an account which is Spanish (expensifail domain account)
Log out from account
Log in with an account which is English (Expensifail or gmail account)
Go to Settings
Check the language of the app

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat · 2021-10-19T15:09:44Z

        .then(() => {
-            if (preferredLocale) {
+            // Betas are only present when user is Logined
+            if (Permissions.canUseInternationalization([]) && preferredLocale) {


Moved to review early. I wanted to have feedback on this piece of the code.

In reality, we don't have beta access when the user is logged out but I used this check instead of isDevelopment as it seems more meaningful.

Ah hmm not sure how isDevelopment relates here. I think if we are logging out it maybe makes sense to just clear the preferredLocale? Why are we trying to persist it? Maybe @iwiznia knows.

If I have selected spanish locale and log out (or get logged out), then I want to see the login screen in spanish, not english. In any case it's fine to only check for the betas in the UI, if a user not on the beta sets the locale manually via developer console or something, then it's ok if the localization feature works for them,

Permissions.canUseInternationalization([]) internally also checks for isDevelopment.

We can check for isDevelopment if we want, but I don't think it's necessary

It's needed. Otherwise, the user will be locked in one language. If we are not showing a LocalePicker on the login page in staging then the language should not change to Spanish at all after logout, if the logged-in user was using spanish.

We determine whether the locale picker is shown or not, is via beta permissions check so I have used the same here.

I think this check is confusing. Calling canUseInternationalization passing an empty betas list is just a confusing way to check isDevelopment() in this case? I think we should completely remove this check from here

Hmm. It can be. I will test this way.

marcaaron · 2021-10-19T18:55:20Z

-                Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
-            }
-        });
+        if (Permissions.canUseInternationalization(this.props.betas)) {


I think this is a race condition for a fresh login. How can we guarantee that this.props.betas is set? We are calling User.getBetas() after so surely this won't work.

Same comment here, we don't need to check for the betas here, let's only check when presenting the UI to change the language.

But, we need to check that. For example,

a user with beta access login, changed the language to Spanish.

He logs out.

Page is still in Spanish.

Now he logs in with another normal user without beta.

Langauge is set to Spanish but there is no option to change and this user speaks English.

This is the issue we are trying to solve with PR.

Well, yeah there is a race condition here. I look into it.

That's an edge case we don't really care about in any scenario other than us testing things, so I think it's fine.

wait, are you saying the issue we are fixing is an edge case? 😕

Yes..... 🤭

Just checked the issue (should've done it before) and yes, I think so. But not sure if that will be a problem for QA... I guess not because they can still log in to an account with the beta, change the language to english then log out and log in with the gmail account which is not in the beta.

marcaaron · 2021-10-19T18:58:07Z

        .then(() => {
-            if (preferredLocale) {
+            // Betas are only present when user is Logined
+            if (Permissions.canUseInternationalization([]) && preferredLocale) {


Ah hmm not sure how isDevelopment relates here. I think if we are logging out it maybe makes sense to just clear the preferredLocale? Why are we trying to persist it? Maybe @iwiznia knows.

…age-beta

parasharrajat · 2021-10-29T16:22:52Z

-                Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
-            }
-        });
+        if (Permissions.canUseInternationalization(this.props.betas)) {


Looking to get some help in choosing the right technique for removing the race condition.

Create a key ONYXKEYS.IS_LOADING_BETAS set to false when beta is loaded.

When this is set to true, run the above logic.

Subscribing to Onyx.beta here seems unnecessary to me as we only want to run one thing once. so I can abstract this code to a separate function and use Onyx.connect in the local scope.

What do you say? @marcaaron

parasharrajat · 2021-10-30T02:00:53Z

This is ready for review.

Currently, I have used the Onyx approach for unlocking the race conditions as we are following a rule that actions should not have then chains #6093.

But I will argue that using a then on User.getBetas() will be optimal here. Thus I can create an action getBetasAndSetPreferredLocale which handles both things getting betas and then setting the locale. Please let me know how do we want it to be?

parasharrajat · 2021-10-31T06:07:20Z

I think the getBetasAndSetPreferredLocale approach is better thus I have updated the code with that approach.

If you want to see the Onyx approach, then see the second last commit d8d4417 (#5943) .

iwiznia

I think I got lost on what we are trying to do on this PR... 😕

iwiznia · 2021-11-01T11:22:21Z

        .then(() => {
-            if (preferredLocale) {
+            // Betas are only present when user is Logined
+            if (Permissions.canUseInternationalization([]) && preferredLocale) {


I think this check is confusing. Calling canUseInternationalization passing an empty betas list is just a confusing way to check isDevelopment() in this case? I think we should completely remove this check from here

iwiznia · 2021-11-01T11:22:53Z

        PersonalDetails.fetchPersonalDetails();
        User.getUserDetails();
-        User.getBetas();
+        User.fetchBetasAndSetPreferredLocale(this.props.preferredLocale);


I don't get why we are setting the preferredLocale here, the user is logging in, their locale can't change in this flow.

Could you please clarify this for me? I didn't get it.

I think, if the user does not have access to betas then we don't want the language to be changed from En as the locale is a beta feature.

Could you please clarify this for me? I didn't get it.

I don't get why we need to set a locale here nor where the value from the locale will come. The only place where the user can set the locale is from the preferences page, no?

I think, if the user does not have access to betas then we don't want the language to be changed from En as the locale is a beta feature.

The way I see it, regardless of the beta, the locale should be what's in preferred locale or english if nothing is set in preferred locale.

Here, the locale is picked from Onyx if the user has set a locale from the login page picker.

I am following the existing code. Just added a check for beta so that we can break the loop if the user does not have localization beta access.

The way I see it, regardless of the beta, the locale should be what's in preferred locale or english if nothing is set in the preferred locale.

Yup, it does but with one exception that it also picks the locale preference from the login page.

If we kill the login page language selection , would that mean we can remove this?

Then we will have to remove the extra code.

marcaaron · 2021-11-01T17:11:26Z

+ * Fetches betas and User's preferred Locale, then set the app locale.
+ * @param {String} currentPreferredLocale
+ */
+function fetchBetasAndSetPreferredLocale(currentPreferredLocale) {


I'm having trouble figuring out why we are combining these. It's not obvious from looking at the code so I don't think it's the right solution to the problem.

If I'm understanding correctly, there is some race condition where we want to "set preferred locale" but only if we are on the beta and the betas have not loaded yet...?

This seems very easy to fix at the API layer by just returning the preferred locale if the user is on the beta and do the check in the server. So basically have something like API.User_GetPreferredLocale() and then this would become:

API.User_GetPreferredLocale() .then(({preferredLocale}) => { Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale); });

Another POV is that we should never prevent getting or setting this NVP depending on the beta.
But in order to use it we should have the correct beta.

marcaaron · 2021-11-01T17:12:10Z

+        ) {
+            setLocale(currentPreferredLocale);
+        } else {
+            Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);


What's the difference between setLocale() and Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, value) 🤔 ?

marcaaron

I think maybe it would be better handle some things at the API layer than combine "get betas" and "get preferred locale" into one synchronous check. That will set up a weird practice where anything that requires the beta to load first will need be put into the "wait for the betas" method.

marcaaron · 2021-11-01T17:21:47Z

unlocking the race conditions as we are following a rule that actions should not have then chains

Heh I'm not sure this is the right interpretation of that rule. We still do not want to encourage promise chains or time dependent code. Anything that can happen in parallel should happen in parallel.

What is the race condition? I've seen it mentioned a few times, but don't have a good understanding of what it is.

parasharrajat · 2021-11-02T15:27:34Z

I will follow up on each comment one by one shortly. But looking at the comments I think we need to discuss a solution before jumping on the implementation.

parasharrajat · 2021-11-03T01:12:37Z

Ok, things are moving fast #6177 is also trying to fix this. But looks like in a good way.

parasharrajat · 2021-11-09T04:49:13Z

Now, I think we don't need the changes hereafter https://github.com/Expensify/App/pull/6177/files.
Please let me know what should I do next?

Added Permission check

e7bf8f7

parasharrajat marked this pull request as ready for review October 19, 2021 15:04

parasharrajat requested a review from a team as a code owner October 19, 2021 15:04

MelvinBot removed the request for review from a team October 19, 2021 15:04

botify requested a review from marcaaron October 19, 2021 15:04

parasharrajat changed the title ~~Added Permission check before setting Preferred Language~~ [WIP] Added Permission check before setting Preferred Language Oct 19, 2021

parasharrajat commented Oct 19, 2021

View reviewed changes

marcaaron reviewed Oct 19, 2021

View reviewed changes

Merge branch 'main' of github.com:Expensify/Expensify.cash into langu…

9990cc2

…age-beta

parasharrajat commented Oct 29, 2021

View reviewed changes

parasharrajat added 2 commits October 30, 2021 06:29

race conditions resolved

783182d

allow render when betas are loaded in Authscreen

d8d4417

parasharrajat changed the title ~~[WIP] Added Permission check before setting Preferred Language~~ Added Permission check before setting Preferred Language Oct 30, 2021

Refactor code to resolve race conditions

8f9007a

iwiznia requested changes Nov 1, 2021

View reviewed changes

marcaaron reviewed Nov 1, 2021

View reviewed changes

marcaaron requested changes Nov 1, 2021

View reviewed changes

MonilBhavsar mentioned this pull request Nov 3, 2021

Only set locale language from preferences #6177

Merged

5 tasks

Christinadobrzyn mentioned this pull request Nov 15, 2021

Chat - After switching account from Spanish to English the app still shows in Spanish and no option to change language for gmail accounts #5286

Closed

parasharrajat closed this Nov 23, 2021

Conversation

parasharrajat commented Oct 19, 2021

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasharrajat commented Oct 31, 2021

Uh oh!

iwiznia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Nov 1, 2021

Uh oh!

parasharrajat commented Oct 30, 2021 •

edited

Loading