fix: allow Truthy values to be returned correctly from translation #38667
Conversation
|
cc @fedirjh as you were C+ on the offending PR. |
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / SafariCleanShot.2024-03-20.at.16.00.22.mp4MacOS: DesktopCleanShot.2024-03-20.at.16.39.27.mp4 |
src/libs/Localize/index.ts
Outdated
| @@ -152,7 +152,7 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' | | |||
| const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es'; | |||
|
|
|||
| const translatedPhrase = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters); | |||
| if (translatedPhrase) { | |||
| if (translatedPhrase !== null && translatedPhrase !== undefined) { | |||
There was a problem hiding this comment.
@hurali97 I believe the correct way to fix this bug is to return the result of the translation as it is, similar to what was previously done: Here is a simple code diff, please give me your thoughts:
code
diff --git a/src/libs/Localize/index.ts b/src/libs/Localize/index.ts
index 39a8741990..c54f07dacb 100644
--- a/src/libs/Localize/index.ts
+++ b/src/libs/Localize/index.ts
@@ -95,7 +95,7 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
phraseKey: TKey,
fallbackLanguage: 'en' | 'es' | null = null,
...phraseParameters: PhraseParameters<Phrase<TKey>>
-): string | null {
+): string {
// Get the cache for the above locale
const cacheForLocale = translationCache.get(language);
@@ -121,7 +121,17 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
}
if (!fallbackLanguage) {
- return null;
+ // Phrase is not found in default language, on production and staging log an alert to server
+ // on development throw an error
+ if (Config.IS_IN_PRODUCTION || Config.IS_IN_STAGING) {
+ const phraseString: string = Array.isArray(phraseKey) ? phraseKey.join('.') : phraseKey;
+ Log.alert(`${phraseString} was not found in the en locale`);
+ if (userEmail.includes(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {
+ return CONST.MISSING_TRANSLATION;
+ }
+ return phraseString;
+ }
+ throw new Error(`${phraseKey} was not found in the default language`);
}
// Phrase is not found in full locale, search it in fallback language e.g. es
@@ -151,22 +161,7 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' |
// Phrase is not found in full locale, search it in fallback language e.g. es
const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es';
- const translatedPhrase = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
- if (translatedPhrase !== null && translatedPhrase !== undefined) {
- return translatedPhrase;
- }
-
- // Phrase is not found in default language, on production and staging log an alert to server
- // on development throw an error
- if (Config.IS_IN_PRODUCTION || Config.IS_IN_STAGING) {
- const phraseString: string = Array.isArray(phraseKey) ? phraseKey.join('.') : phraseKey;
- Log.alert(`${phraseString} was not found in the en locale`);
- if (userEmail.includes(CONST.EMAIL.EXPENSIFY_EMAIL_DOMAIN)) {
- return CONST.MISSING_TRANSLATION;
- }
- return phraseString;
- }
- throw new Error(`${phraseKey} was not found in the default language`);
+ return getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
}
/**There was a problem hiding this comment.
Actually, I think we should refactor the code in getTranslatedPhrase , Previously we were checking the existence of a key inside the Translations object, but now we check the value of the translation which doesn't look correct IMO.
There was a problem hiding this comment.
cc @hurali97 Here is my latest attempt to address the concerns raised in the previous comment: I believe we should refactor the getTranslatedPhrase code and introduce a new flag isValidKey in its response. This way, the function should return both the value and the validity of the key. Here is the updated code:
Code
diff --git a/src/libs/Localize/index.ts b/src/libs/Localize/index.ts
index 39a8741990..993561a0e6 100644
--- a/src/libs/Localize/index.ts
+++ b/src/libs/Localize/index.ts
@@ -72,6 +72,12 @@ const translationCache = new Map<ValueOf<typeof CONST.LOCALES>, Map<TranslationP
}, [] as Array<[ValueOf<typeof CONST.LOCALES>, Map<TranslationPaths, string>]>),
);
+
+type CacheValue = {
+ value: string;
+ isValidKey: boolean
+}
+
/**
* Helper function to get the translated string for given
* locale and phrase. This function is used to avoid
@@ -95,7 +101,7 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
phraseKey: TKey,
fallbackLanguage: 'en' | 'es' | null = null,
...phraseParameters: PhraseParameters<Phrase<TKey>>
-): string | null {
+): CacheValue {
// Get the cache for the above locale
const cacheForLocale = translationCache.get(language);
@@ -105,30 +111,30 @@ function getTranslatedPhrase<TKey extends TranslationPaths>(
// If the phrase is already translated, return the translated value
if (valueFromCache) {
- return valueFromCache;
+ return {value: valueFromCache, isValidKey: true}
}
const translatedPhrase = translations?.[language]?.[phraseKey] as Phrase<TKey>;
if (translatedPhrase) {
if (typeof translatedPhrase === 'function') {
- return translatedPhrase(...phraseParameters);
+ return {value: translatedPhrase(...phraseParameters), isValidKey: true};
}
// We set the translated value in the cache only for the phrases without parameters.
cacheForLocale?.set(phraseKey, translatedPhrase);
- return translatedPhrase;
+ return {value: translatedPhrase, isValidKey: true};
}
if (!fallbackLanguage) {
- return null;
+ return {value: '', isValidKey: false}
}
// Phrase is not found in full locale, search it in fallback language e.g. es
const fallbacktranslatedPhrase = getTranslatedPhrase(fallbackLanguage, phraseKey, null, ...phraseParameters);
- if (fallbacktranslatedPhrase) {
- return fallbacktranslatedPhrase;
+ if (fallbacktranslatedPhrase.isValidKey) {
+ return fallbacktranslatedPhrase
}
if (fallbackLanguage !== CONST.LOCALES.DEFAULT) {
@@ -151,9 +157,9 @@ function translate<TKey extends TranslationPaths>(desiredLanguage: 'en' | 'es' |
// Phrase is not found in full locale, search it in fallback language e.g. es
const languageAbbreviation = desiredLanguage.substring(0, 2) as 'en' | 'es';
- const translatedPhrase = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
- if (translatedPhrase !== null && translatedPhrase !== undefined) {
- return translatedPhrase;
+ const {value, isValidKey} = getTranslatedPhrase(language, phraseKey, languageAbbreviation, ...phraseParameters);
+ if (isValidKey) {
+ return value;
}
// Phrase is not found in default language, on production and staging log an alert to serverThere was a problem hiding this comment.
@fedirjh Your latest code looks nice and I have incorporated it with just renaming the type name from CacheValue to TranslatedValue. Is that fine with you?
I believe the correct way to fix this bug is to return the result of the translation as it is, similar to what was previously done
If we did that we will have the same bug where we show the translation key in UI for the value which is an empty string. To fill you in, previously we were checking for Truthiness of translatedPhrase and if it's an empty string, which can happen because of variables, we will have an empty string as translatedPhrase, which is what is expected. So in such cases, returning phraseKey instead would be a violation.
But I guess it's also fixed by your latest code, so that's fine 👍
There was a problem hiding this comment.
So in such cases, returning phraseKey instead would be a violation.
Yes, we are in agreement. By "the result of the translation as it is" I meant the value, even if it's empty or null, not the key.
There was a problem hiding this comment.
I actually think that the code was simpler and just as clear before - strings (including empty strings) are valid results, but null or undefined are not.
There was a problem hiding this comment.
i.e: if the translation isn't found, getTranslatedPhrase should just return null, rather than {value: '', isValidKey: false}. If the translation is found and happens to be an empty string, that's fine
There was a problem hiding this comment.
are valid results, but null or undefined are not.
@roryabraham Yes, but this won't tell us if the key exists or not. with previous code , null or undefined means that it's either :
- the key doesn't exit.
- the key exists, but its value is
nullorundefined. this can happen if we have a function maybe?
In the second case, In staging or production it will log an alert to the server about a key not found, which is not what we want?
There was a problem hiding this comment.
There are no test or QA steps on this PR, which is never acceptable under any circumstances. If it should be No QA you can mark the PR as such, but always you should leave an explanation as to why it can't or shouldn't be tested or QA'd.
Also, Reassure tests have been fixed on main, can we merge main into this branch?
edit: I see that you actually had "follow the steps in the linked issue", which is better than nothing. but let's just take the 30 seconds to copy/paste the test steps next time so a reviewer coming in can clearly see them.
|
A new PR was made here. Closing this PR for now 🙇 @fedirjh, you were assigned as a C+ reviewer in the new PR |
@roryabraham I have explicitly added a video of my testing in PR description, if that's not an evidence of testing then I am not sure what is. |
|
My apologies - it was not in the screenshots section of the PR description. |
Details
This fixes the issue where static
common. zipCodeExampleFormatwas visible just below the Zip/ Post code.Screen.Recording.2024-03-20.at.5.30.13.PM.mov
Fixed Issues
$ #38660
PROPOSAL: #38660 (comment)
Tests / QA Steps
Precondition:
common.zipCodeExampleFormaton the screen.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop