-
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
[PAID] [$1000] The error is displayed in Spanish on invalid number chat even when the language preference has changed to English #18203
Comments
Triggered auto assignment to @kevinksullivan ( |
Bug0 Triage Checklist (Main S/O)
|
Job added to Upwork: https://www.upwork.com/jobs/~01e6c0bedde567e47c |
Current assignee @kevinksullivan is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh ( |
Triggered auto assignment to @chiragsalian ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.The error is displayed in Spanish on invalid number chat even when the language preference has changed to English What is the root cause of that problem?The user's comment is saved by onSubmitComment. App/src/pages/home/ReportScreen.js Lines 176 to 178 in d0f7955
And addComment function calls addActions. App/src/libs/actions/Report.js Lines 316 to 318 in d0f7955
In addActions function, if an error occurs, the failure message is generated by Localize.translateLocal('report.genericAddCommentFailureMessage'), and is saved to Onyx. That is, the error will be saved in Spanish. App/src/libs/actions/Report.js Lines 265 to 271 in d0f7955
So if send the message fails, the translated Spanish error message is saved. And the translated message is displayed by App/src/components/DotIndicatorMessage.js Lines 58 to 62 in d0f7955
So even though the language preference has changed to English, the error will be displayed in Spanish. What changes do you think we should make in order to solve the problem?If the error occurs, instead of saving the translated error message, it would just save the error key directly to Onyx. (ex. report.genericAddCommentFailureMessage).
When showing the error message, we first check the error key exists in the user's translation dictionary. For this we would create a utility function inside This utility function checks whether the error key exists in the translation dictionary of the user.
Now we can use this utility function to show the error message. We can use this approach to cover all cases where we save the translation error to Onyx. What alternative solutions did you explore? (Optional)None. |
📣 @StevenKKC! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Format:
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@StevenKKC Thank you for the proposal. I believe the root cause analysis is correct, this looks like a regression from #14736. However, I am unclear about the main solution you provided. Could you please elaborate on it further? As for the optional solution, it seems more promising, but it could still use some improvement. One suggestion would be to save the error key <Text key={i} style={styles.offlineFeedback.text}>
{message === 'report.genericAddCommentFailureMessage'
? Localize.translateLocal('report.genericAddCommentFailureMessage')
: message}
</Text> @chiragsalian and @iwiznia I am wondering if Overall, I think we have two options:
|
@fedirjh Thank you for your review. I haven't thought about it much. I'll pay more attention in the future. I appreciate your advice. |
@fedirjh I think now that the language preference is set to Spanish, the backend error should be displayed in Spanish also. |
In fact, backend errors are not translated and are only displayed in English. |
What can I do for this now? @fedirjh |
We know it is inconsistent, but spanish is a beta feature for that reason. The goal is to have the app be localized and for that, we need to have that message in spanish too. |
@iwiznia Thank you for the input. Do we have any other generic errors that we should consider in addition to this issue? If |
Sorry, I don't get the question. What is there to consider? Any message generated in the App needs to be translated. |
@iwiznia I mean to ask whether we should consider other cases in which translated error messages are saved optimistically in |
Oh, no idea, I assume there are indeed more cases like that one. |
Let me make a thorough check of the codebase to determine whether additional instances exist. |
Thanks @chiragsalian, have a great weekend! |
@chiragsalian @fedirjh @strepanier03 PR(#19324) is ready for review. |
PR is up and reviews are happening, moving forward well here. |
PR is merged and awaiting push to staging and then prod. In a good holding spot on this one for now. |
PR was merged to Production, however the automation wasn’t not triggered, I think the title should be updated manually. |
@chiragsalian @fedirjh When does the title update? Because of the regression test mentioned in the issue (#19565), have not updated the title yet? |
@chiragsalian |
@StevenKKC it's not related to regression test, the automation fails sometimes when we don't follow the right format in the PR template, the bot fails to find linked issue , hence the automation fails triggers. |
@fedirjh Thanks for you reply. |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.18-2 🚀 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: #19324 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
Speed bonus analysis: Contributor hired on May 18 / PR merged on May 23 = 3 business days = Eligible As a reminder, here are the bonuses/penalties that should be applied for any External issue: Merged PR within 3 business days of assignment - 50% bonus |
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@fedirjh and @StevenKKC - I did what should have happened manually and will prep for the hold being removed. |
The job originally created for this GH has been closed by Upwork so I had to make a new one. @fedirjh - I have hired you for the job. Please feel free to complete the checklist items and I'll update it and make the reg test GH if needed. @StevenKKC - I am not confident that I'm hiring the right "Steven" in Upwork. Can you apply for the job here and link me to your Upwork profile so I know for sure? @priya-zha - I created a reporting job for you in Upwork, can you please apply here. I am not 100% confident that the Priya I'm finding in Upwork is you and would also appreciate a link to your Upwork profile so I am sure. Thank you all, the hold is released tomorrow and I'll take what action I can then. |
❌ - Link to the PR : This is a new feature Regression Test Proposal
Do we agree 👍 or 👎 |
@strepanier03 |
@strepanier03 Submitted the proposal. Here's my upwork profile : https://www.upwork.com/freelancers/~011e32317dbbd489e0 Thanks |
📣 @kristianbarth! 📣
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.18-2 🚀 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: #19324 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done. External issue reporter - @priya-zha - $250 As a reminder, here are the bonuses/penalties that should be applied for any External issue: Merged PR within 3 business days of assignment - 50% bonus |
I finished up payment and the reg test GH is done. Thanks all! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The error message should be shown in English when the language was changed in the preference
Actual Result:
The error is displayed in Spanish on invalid number chat even when the language preference has changed to English
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.8.8
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
error.mp4
az_recorder_20230429_234232.1.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682764542821759
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: