-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: show error message depend on the language preference #19324
fix: show error message depend on the language preference #19324
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StevenKKC As this is your first time contribution , you need to sign the CLA , check this #19324 (comment) . Also please update the checklist , the template should’ve been maintained . There are some lint error that you should fix as well.
Please include all screenshots for all platforms.
@@ -64,7 +65,7 @@ const DotIndicatorMessage = (props) => { | |||
key={i} | |||
style={styles.offlineFeedback.text} | |||
> | |||
{message} | |||
{Localize.translateError(message)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{Localize.translateError(message)} | |
{message} |
Let's move this to line 51 as bellow :
const sortedMessages = _.chain(props.messages)
.keys()
.sortBy()
.map((key) => props.messages[key])
// Using uniq here since some fields are wrapped by the same OfflineWithFeedback component (e.g. WorkspaceReimburseView)
// and can potentially pass the same error.
.uniq()
.map((message) => Localize.translateError(message))
.value();
src/libs/Localize/index.js
Outdated
@@ -91,6 +91,24 @@ function translateLocal(phrase, variables) { | |||
return translate(BaseLocaleListener.getPreferredLocale(), phrase, variables); | |||
} | |||
|
|||
/** | |||
* Return translated message for phrase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Return translated message for phrase. | |
* Return translated string for given error |
@@ -81,7 +81,7 @@ function getVBBADataForOnyx() { | |||
value: { | |||
isLoading: false, | |||
errors: { | |||
[DateUtils.getMicroseconds()]: Localize.translateLocal('paymentsPage.addBankAccountFailure'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can extract this part to ErrorUtils.js
as below :
/**
* Method used to generate errors object for optimistic
* @param {String} error - error key or message to be saved
* @return {Object} - Object to be optimistically saved to Onyx
*
*/
function getOptimisticErrors(error){
return ({[DateUtils.getMicroseconds()]: error});
}
// Use It as
errors: ErrorUtils.getOptimisticErrors('paymentsPage.addBankAccountFailure'),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fedirjh @chiragsalian @StevenKKC We're passing 'paymentsPage.addBankAccountFailure'
to the utility method, but all it's doing is storing that key as the error instead of translating it. So this is what the error looks like:
but it should be:
1685721717986000: "An unexpected error occurred while trying to add your bank account. Please try again."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins Yes , We save the key then we translate it inside DotIndicatorMessage.js
App/src/components/DotIndicatorMessage.js
Line 52 in 465b788
.map((message) => Localize.translateIfPhraseKey(message)) |
We check if the key exists in the translation directory , then we translate it , If it's from backend then it will be displayed as it it's
App/src/libs/Localize/index.js
Lines 100 to 106 in 465b788
function translateIfPhraseKey(phrase) { | |
try { | |
return translateLocal(phrase); | |
} catch (error) { | |
return phrase; | |
} | |
} |
Is there any issue with that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins Storing the key is correct, and when display that error message, the key will be translated to the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only calling translateIfPhraseKey
in DotIndicator, but there are other places displaying the error text, e.g. Forms, that don't use DotIndicator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! We missed that case.
@luacmartins should we raise a new PR to address that or should we revert ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in prod already, so it's a regression and we should raise a PR to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @luacmartins I have raised a draft PR #20093 , can you please check it
I have read the CLA Document and I hereby sign the CLA |
@StevenKKC Please fix the failing tests as well , you need to update them with the new approach. |
@fedirjh Updated. |
@StevenKKC Can you please merge main. Please update tests step as well
Verify that the error messages is shown in English when the language was already changed to English. |
Reviewer Checklist
Screenshots/VideosWebWeb.movMobile Web - ChromeChrome.movMobile Web - SafariSafari.movDesktopDesktop.moviOSIOS.movAndroidAndroid.mov |
@StevenKKC The changes look good. Please include the missing screenshots and address my comment #19324 (comment) |
@fedirjh Uploaded the missing screenshots and tests step. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fedirjh Thanks for your review. @chiragsalian Could you please review my PR? |
@fedirjh @chiragsalian Because of conflict, I have merged main branch into my branch. |
@chiragsalian Updated files according to your suggestion. |
@chiragsalian @fedirjh All checks have passed. What can I do for this now? |
@chiragsalian Could you please review the changes that you requested? I hope to merge this PR as soon as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chiragsalian I have updated files according to your request. Could you please review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed addressing this comment of mine.
Specifically this one,
There is an extra eslint comment here on line 43 that should be removed as well |
@chiragsalian @fedirjh Updated. |
@chiragsalian All checks have passed. Please let me know what I should do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm going ahead and merging since fedi already approved it earlier and his most recent review comment was minor. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.18-0 🚀
|
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.18-0 🚀
|
🚀 Deployed to staging by https://github.com/chiragsalian in version: 1.3.18-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.18-2 🚀
|
I have checked the Deploy checklist (#19565) and the issue #19651 (#19651). But I think it's not related to this PR at all. When user change the language preference, the following code will be executed. Lines 85 to 112 in 0f1188d
In the offline mode (or the user have already changed the force offline mode to online, but the app is still in the offline status, as you can see in your video), API.write only change the local Onyx data, and the request API that update the language preference does not send to the server. After send the comment, the server will send the response error message and the Onyx data that is stored in the server to the app, and these data will contain the original language preference, as you can see in the below screenshot. So, after app receive the server response and merge the server's Onyx data, the language preference will be changed to the previous state, that is Spanish, and the error message is shown in Spanish. That is, this PR is working as expected. And now, there is a new issue opened. |
@StevenKKC you should post that in the the other issue. That would be helpful to identify the root cause of the other issue, And if we find that the root cause is related to this PR , then we can flag it as regression and proceed with a fix. Edit : I agree with your analysis , this issue is not related to the changes with this PR |
Details
Fixed Issues
$ #18203
PROPOSAL: #18203(COMMENT)
Tests
Offline tests
QA Steps
Same as Tests step.
PR Author Checklist
I linked the correct issue in the
### Fixed Issues
section aboveI wrote clear testing steps that cover the changes made in this PR
Tests
sectionOffline steps
sectionQA steps
sectionI included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
In web and desktop version, there's a console error that is not related to this PR. A console error is generated in src/styles/getTooltipStyles.js and src/libs/BootSplash/index.js, and display error screen.
I think should open an issue for it.
I followed proper code patterns (see Reviewing the code)
toggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedIf a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar
, I verified the components usingAvatar
are working as expected)I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)If any new file was added I verified that:
If a new CSS style is added I verified that:
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avatar
is modified, I verified thatAvatar
is working as expected in all cases)If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the
ScrollView
component to make it scrollable when more elements are added to the page.If the
main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.
Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
mobile.web.chrome.mp4
Mobile Web - Safari
mobile.web.safari.mov
Desktop
desktop.mov
iOS
iOS.mov
Android
Android.mp4