Skip to content

Added Permission check before setting Preferred Language #5943

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.

Sign up for GitHub

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

Closed
parasharrajat wants to merge 5 commits into from
Closed

Added Permission check before setting Preferred Language #5943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7bf8f7
Added Permission check
parasharrajat Oct 19, 2021
9990cc2
Merge branch 'main' of github.com:Expensify/Expensify.cash into langu…
parasharrajat Oct 29, 2021
783182d
race conditions resolved
parasharrajat Oct 30, 2021
d8d4417
allow render when betas are loaded in Authscreen
parasharrajat Oct 30, 2021
8f9007a
Refactor code to resolve race conditions
parasharrajat Oct 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/withLocalize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const withLocalizePropTypes = {

/** Returns an internationally converted phone number with the country code */
fromLocalPhone: PropTypes.func.isRequired,

/** The user's preferred locale e.g. 'en', 'es-ES' */
preferredLocale: PropTypes.string.isRequired,
};

const localeProviderPropTypes = {
Expand Down
26 changes: 4 additions & 22 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,8 @@ import SCREENS from '../../../SCREENS';
import Timers from '../../Timers';
import LogInWithShortLivedTokenPage from '../../../pages/LogInWithShortLivedTokenPage';
import defaultScreenOptions from './defaultScreenOptions';
import * as API from '../../API';
import {setLocale} from '../../actions/App';
import {cleanupSession} from '../../actions/Session';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';

Onyx.connect({
key: ONYXKEYS.MY_PERSONAL_DETAILS,
Expand All @@ -83,12 +82,6 @@ Onyx.connect({
},
});

let currentPreferredLocale;
Onyx.connect({
key: ONYXKEYS.NVP_PREFERRED_LOCALE,
callback: val => currentPreferredLocale = val || CONST.DEFAULT_LOCALE,
});

const RootStack = createCustomModalStackNavigator();

// We want to delay the re-rendering for components(e.g. ReportActionCompose)
Expand All @@ -111,6 +104,7 @@ const propTypes = {
isOffline: PropTypes.bool,
}),

...withLocalizePropTypes,
...windowDimensionsPropTypes,
};

Expand Down Expand Up @@ -141,22 +135,9 @@ class AuthScreens extends React.Component {
// Fetch some data we need on initialization
NameValuePair.get(CONST.NVP.PRIORITY_MODE, ONYXKEYS.NVP_PRIORITY_MODE, 'default');
NameValuePair.get(CONST.NVP.IS_FIRST_TIME_NEW_EXPENSIFY_USER, ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER, true);

API.Get({
returnValueList: 'nameValuePairs',
nvpNames: ONYXKEYS.NVP_PREFERRED_LOCALE,
}).then((response) => {
const preferredLocale = lodashGet(response, ['nameValuePairs', 'preferredLocale'], CONST.DEFAULT_LOCALE);
if ((currentPreferredLocale !== CONST.DEFAULT_LOCALE) && (preferredLocale !== currentPreferredLocale)) {
setLocale(currentPreferredLocale);
} else {
Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
}
});

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

@iwiznia iwiznia Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@parasharrajat parasharrajat Nov 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iwiznia iwiznia Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@parasharrajat parasharrajat Nov 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

@iwiznia iwiznia Nov 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@parasharrajat parasharrajat Nov 2, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we will have to remove the extra code.

User.getDomainInfo();
PersonalDetails.fetchLocalCurrency();
fetchAllReports(true, true);
Expand Down Expand Up @@ -382,6 +363,7 @@ AuthScreens.propTypes = propTypes;
AuthScreens.defaultProps = defaultProps;
export default compose(
withWindowDimensions,
withLocalize,
withOnyx({
network: {
key: ONYXKEYS.NETWORK,
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/SignInRedirect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Onyx from 'react-native-onyx';
import SignoutManager from '../SignoutManager';
import ONYXKEYS from '../../ONYXKEYS';
import Permissions from '../Permissions';

let currentActiveClients;
Onyx.connect({
Expand All @@ -26,7 +27,8 @@ function clearStorageAndRedirect(errorMessage) {
// Clearing storage discards the authToken. This causes a redirect to the SignIn screen
Onyx.clear()
.then(() => {
if (preferredLocale) {
// Betas are only present when user is Logined
if (Permissions.canUseInternationalization([]) && preferredLocale) {

@parasharrajat parasharrajat Oct 19, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@marcaaron marcaaron Oct 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iwiznia iwiznia Oct 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

@parasharrajat parasharrajat Oct 20, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@iwiznia iwiznia Oct 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@parasharrajat parasharrajat Oct 30, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iwiznia iwiznia Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@parasharrajat parasharrajat Nov 1, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
}
if (activeClients && activeClients.length > 0) {
Expand Down
32 changes: 31 additions & 1 deletion src/libs/actions/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import Log from '../Log';
import NetworkConnection from '../NetworkConnection';
import NameValuePair from './NameValuePair';
import getSkinToneEmojiFromIndex from '../../pages/home/report/EmojiPickerMenu/getSkinToneEmojiFromIndex';
import Permissions from '../Permissions';
import {setLocale} from './App';


let sessionAuthToken = '';
let sessionEmail = '';
Expand Down Expand Up @@ -58,9 +61,35 @@ function changePassword(oldPassword, password) {
}

function getBetas() {
API.User_GetBetas().then((response) => {
return API.User_GetBetas().then((response) => {
if (response.jsonCode === 200) {
Onyx.set(ONYXKEYS.BETAS, response.betas);
return response.betas;
}
return [];
});
}

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

@marcaaron marcaaron Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);    
    });

@marcaaron marcaaron Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Promise.all([
getBetas(),
API.Get({
returnValueList: 'nameValuePairs',
nvpNames: ONYXKEYS.NVP_PREFERRED_LOCALE,
}),
]).then((betas, response) => {
const preferredLocale = lodashGet(response, ['nameValuePairs', 'preferredLocale'], CONST.DEFAULT_LOCALE);
if (currentPreferredLocale !== CONST.DEFAULT_LOCALE
&& preferredLocale !== currentPreferredLocale
&& Permissions.canUseInternationalization(betas)
) {
setLocale(currentPreferredLocale);
} else {
Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);

@marcaaron marcaaron Nov 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
});
}
Expand Down Expand Up @@ -312,4 +341,5 @@ export {
setPreferredSkinTone,
setShouldUseSecureStaging,
clearUserErrorMessage,
fetchBetasAndSetPreferredLocale,
};