[HOLD for payment 2024-06-28] [$250] Get rid of MISSING TRANSLATION errors once and for all

[iwiznia]

Context https://expensify.slack.com/archives/C01GTK53T8Q/p1702316632335369

Problem

We've had lots (also search in:#expensify-bugs "missing translation" in slack) of MISSING TRANSLATIONbugs.

These bugs should really not exist ever, they solely exist because we've added a method translateIfPhraseKey that really should never exist, because at every point in our code, we should know if what we have is a translation key or a text to show to the user as is.

Solution

Remove the method.
Change all its usages to:

• Show the text directly, if the text we have is a text ready to show to the user
• Call the regular translate method if we have a translation key

cc @StevenKKC @fedirjh

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013d70ff8202e9ba0e
• Upwork Job ID: 1793684887920951296
• Last Price Increase: 2024-05-23
• Automatic offers:
• fedirjh | Contributor | 102487870
• nkdengineer | Contributor | 102497969

Issue Owner

Current Issue Owner: @nkdengineer

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013d70ff8202e9ba0e

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 (External)

[hunxjunedo]

Requesting assignment.

[melvin-bot]

📣 @hunxjunedo! 📣
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.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove MISSING TRANSLATION ERROR

What is the root cause of that problem?

We've had of MISSING TRANSLATION bugs, they exist because we've added a method translateIfPhraseKey that really should never exist, because at every point in our code, we should know if what we have is a translation key or a text to show to the user as is.

What changes do you think we should make in order to solve the problem?

remove the translateIfPhraseKey function and change all of its usages to:

1. Call the regular translate method if we have a translation key, for this we need to define a new function inside localize util that would return true if the key exist in the translation file and false otherwise
2. show the text as its if we dont have the key in the translation file.

[cretadn22]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Get rid of MISSING TRANSLATION errors once and for all

What is the root cause of that problem?

This is a refactor task

What changes do you think we should make in order to solve the problem?

App/src/components/DotIndicatorMessage.tsx

Line 53 in c19eea6

const uniqueMessages: Array<ReceiptError | string> = [...new Set(sortedMessages)].map((message) => (isReceiptError(message) ? message : Localize.translateIfPhraseKey(message)));

App/src/components/FormHelpMessage.tsx

Line 36 in c19eea6

const translatedMessage = Localize.translateIfPhraseKey(message);

App/src/libs/ErrorUtils.ts

Line 158 in c19eea6

const translatedMessage = Localize.translateIfPhraseKey(message);

Currently, we utilize "translateIfPhraseKey" in three places. I propose using the message directly in these three places; there's no need for translation. Then we can identify which cases require translation and update them to use the message directly. This will help maintain consistency across these common components.

What alternative solutions did you explore? (Optional)

[hungvu193]

@iwiznia I saw you tagged @fedirjh, let me know if @fedirjh will handle this one, if yes please unassign me 😄

[iwiznia]

I tagged him because IIRC he added that method so he can have context on all this, but I don't think he necessarily has to handle this one.

[iwiznia]

Call the regular translate method if we have a translation key, for this we need to define a new function inside localize util that would return true if the key exist in the translation file and false otherwise

I don't think this is good, its improving on what we have, but still does not resolve the main problem which is: whoever is adding a call to translate should know if what they are passing is a text or a key.

Currently, we utilize "translateIfPhraseKey" in three places. I propose using the message directly in these three places; there's no need for translation. Then we can identify which cases require translation and update them to use the message directly. This will help maintain consistency across these common components.

This is not saying much about the solution, since those 3 places are used in like 100 different places.

[nkdengineer]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We want to remove the translateIfPhraseKey method

What is the root cause of that problem?

The reason translateIfPhraseKey was added is that for components like FormHelpMessage, DotIndicatorMessage and when adding form errors, we wanted it to support both the use cases of front-end errors and back-end errors.

The idea was to have a data structure MaybePhraseKey for that use case.

However this is not a good pattern, as agreed to in the OP. The components should receive only translated error, whether it's back-end error or not will be controlled by whichever parent components/forms that use those components.

What changes do you think we should make in order to solve the problem?

Already-translated errors will always be a pure string.

1. We need to update DotIndicatorMessage, FormHelpMessage, addErrorMessage so that they will only receive already-translated errors as props/params

That means for DotIndicatorMessage, messages here will be a map of string and string only

For FormHelpMessage, message here will strictly be a string

For addErrorMessage, message here will strictly be a string

And we'll update the implementations of them accordingly to remove the translation logic. The Localize.MaybePhraseKey should no longer be used in those places.

2. Most of the places that use DotIndicatorMessage, FormHelpMessage, and 100% of the cases with addErrorMessage, we're dealing with client-side-only errors.

In this case, instead of constructing a MaybePhraseKey and delegate the responsibility of translating it to the lower level components, we should handle the translation in the parent components and just pass the results to those lower level components/methods.

For example, in this case, update to

message={translate("iou.error.splitExpenseMultipleParticipantsErrorMessage")}

3. There're a few places that use back-end errors/composite client-side errors for those components, in this case, we pass those user-ready errors as is to those components. For example, in this case update to

message={policy?.alertMessage ?? ''}

4. In case of combination between before-translate error and translated error, like in this, convert it to an array of all translated errors (if error is before-translate, translate it, if error is translated error, do nothing) and use it.

We'll do so across the app, all cases to be updated are mostly of the above 3 categories.

What alternative solutions did you explore? (Optional)

NA

[hungvu193]

I'll review proposals this weekend.

[fedirjh]

cc @hungvu193 @iwiznia I can take over this one.

As for the proposals, I think @nkdengineer proposal looks good

[melvin-bot]

@hungvu193, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hungvu193]

Hmm not sure what should I do in this case.
Cc @iwiznia

[iwiznia]

Ah sorry, going to assign @fedirjh as C+ if you don't mind, since he has more context on this.
@fedirjh make sure to add the comment of the review so it assigns an engineer.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.85-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Get rid of missing translation #42970

If no regressions arise, payment will be issued on 2024-06-28. 🎊

For reference, here are some details about the assignees on this issue:

• @fedirjh requires payment automatic offer (Contributor)
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

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] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[kadiealexander]

Payouts due:

• Contributor: $250 @fedirjh (offer)
• Contributor+: $250 @nkdengineer (offer)

Upwork job is here.

[kadiealexander]

@fedirjh please complete the checklist!

[fedirjh]

BugZero Checklist:

• Link to the PR: N/A: This is was a code refactoring
• Link to comment: N/A
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: N/A

[fedirjh]

@kadiealexander I will request payment on new Dot.

[JmillsExpensify]

$250 approved for @fedirjh

[kadiealexander]

@dukenv0307 please let me know when you've accepted the Upwork offer, thanks!

[dukenv0307]

@dukenv0307 please let me know when you've accepted the Upwork offer, thanks!

@kadiealexander Maybe a mistake here, I'm not C or C+ on this issue 😄.

[kadiealexander]

Whoops, you're totally right! Sorry, been a long day. @nkdengineer please let me know when you've accepted the Upwork offer, thanks!

[nkdengineer]

@nkdengineer please let me know when you've accepted the Upwork offer, thanks!

@kadiealexander I've done that, thanks

[melvin-bot]

@francoisl, @fedirjh, @kadiealexander, @nkdengineer Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[francoisl]

@kadiealexander can you confirm all payments have been issued and we can close this please? Thanks!

[kadiealexander]

Yup! All done.