Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

Chat - Text (archived) is not translated in Spanish after switch language setting #9018

Closed kavimuru closed 2 years ago

kavimuru commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue is found when executing the PR #8436

Action Performed:

  1. Go https://staging.new.expensify.com/

  2. Log in with expensifail account A

  3. Create a Workspace

  4. Invite another user B to workspace

  5. Go to OldDot and delete the Workspace

  6. Log in with account B

  7. Go to Setting - Preferences

  8. Change language to Spanish

  9. Go back to LHN and find your Workspace

Expected Result:

Text (archived) is translated in Spanish after switch the langue settings

Actual Result:

Text (archived) is not translated in Spanish after switch the language setting

Workaround:

After refresh it gets updated

Platform:

Where is this issue occurring?

Version Number: 1.1-60-1 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Bug5571379_Screenshot_20220514-112416_Chrome

https://user-images.githubusercontent.com/43996225/168439287-28e9b337-4acc-433d-8095-3edcc4eff6b3.mp4

https://user-images.githubusercontent.com/43996225/168439290-9819d992-c1e4-4749-aa3b-630caee60c04.mp4 Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

View all open jobs on GitHub

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @cead22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

roryabraham commented 2 years ago

The solution to this issue should use ReportUtils.getReportName to populate the report names in SidebarLinks

stitesExpensify commented 2 years ago

@cead22 I'm going to hand this over to @srikarparsi to take on!

cead22 commented 2 years ago

Thanks!

srikarparsi commented 2 years ago

I've been working on this issue and currently have a question regarding code practices and have also ran into a problem with getting data from Onyx.

Currently, when you change the language preference, most of the text in the chat is changing because it is listening for changes made to the locale setting in Onyx and updates the text based on that.

However, the title which includes "(archived)" is different because it only fetches data from Onyx at the beginning and doesn't update based on changes to locale. A fix could be to listen to these changes similar to the text but the problem is that the title text occurs in multiple places and it would just be better to update the title in Onyx directly.

Screen Shot 2022-05-19 at 2 07 54 AM

As shown in the screenshot above, the language is English, but the title says "archivado" since in Onyx, report_16 also has archivado appended to the end. When the page is refreshed from this state, the text changes from archivado to archived which is good, but in Onyx the report name still has (archivado) appended to the end since changing the locale doesn't update Onyx's individual reports.

Screen Shot 2022-05-19 at 2 27 36 AM

To fix this issue, we need to update each report in Onyx to have the right title name when the language preference is changed. A possible solution is below.

Screen Shot 2022-05-19 at 2 15 12 AM

First, I wanted to ask if it was an ok coding practice to update Onyx's reports in the setLocale method as shown above since the other text that changes based on language does it by listening to the language preference setting in Onyx.

Second, I'm running into an issue with the above code. I tried running the commented code but after not finding a way to get all the reports and iterate through them, I wanted to try and get just report_16. When I run that code, it logs undefined which I think means it did not find "report_16" as a key even though it is in Onyx as shown in the first two screenshots. I was thinking this is because that key isn't registered as one of the ones you can get data from but I'm not completely sure. Would anyone happen to know a workaround for this problem?

roryabraham commented 2 years ago

Hi @srikarparsi, thanks for the write-up. We discussed this a bit in slack here, but here's my summary:

The reportName is a "composite" value that comes from multiple sources:

And given our new API design that prohibits side-effects from API requests, we had to choose between:

We settled on the second approach. So I recently wrote a function, ReportUtils.getReportName, that is throughly unit-tested and generates the correct display name from those various values. What this means is that we should not directly reference the reportName field from the report stored in Onyx directly, anywhere in the application. Even setting the correct report name in Onyx on first load is problematic, because we have to ensure that all of those composite values are loaded before we set the report name.

So I think the approach you're taking (updating the reportName for all reports in Onyx, whenever the locale changes), isn't the best approach. Instead, here's what I recommend:

  1. Make components use ReportUtils.getReportName to get the report name they display, not the reportName from Onyx
  2. Ensure those components re-render when any of the four values used to create the "composite" reportName change. The best way to do this is probably to:
    1. extract the preferredLocale Onyx connection out of ReportUtils, and instead take the current locale as a parameter to any functions using preferredLocale in the utils functions
    2. Wrap any components using those functions in withLocalize
    3. Pass the locale from withLocalize into the ReportUtils functions.
    4. That way, whenever the locale changes, withLocalize will re-render any components consuming that locale, and the reportName (or the result of any ReportUtils function dependent upon that locale) will also update
srikarparsi commented 2 years ago

Stuck on understanding which methods are outdated. Messaged to setup a call.

melvin-bot[bot] commented 2 years ago

@srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

roryabraham commented 2 years ago

@srikarparsi – anything I can do to help get this over the finish line? I'm here to help + review when you need 🙂

melvin-bot[bot] commented 2 years ago

@srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 years ago

@srikarparsi Still overdue 6 days?! Let's take care of this!

srikarparsi commented 2 years ago

looked at test cases but believe we might have to change the test cases and not the code so messaged Rory

srikarparsi commented 2 years ago

Fixed by this pr: https://github.com/Expensify/App/pull/8569