[$1000] Inconsistency bug - App does not dynamically translate 'No activity yet' in search (works fine after revisit and dynamically updates in LHN)

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the app (device 1) and login with user A
2. Open another device (device 2) and login with user A
3. Open search in device 1
4. From device 2, change language from english to spanish
5. In device 1 observe that 'No activity yet' is not updated in Spanish in search but it is updated in LHN and other terms too are updated dynamically in search and reports

Expected Result:

App should dynamically update 'No activity yet' in search to Spanish on language change

Actual Result:

App does not dynamically update 'No activity yet' in search to Spanish on language change

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.29-2
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

Not.updated.dynamically.to.spanish.no.activity.yet.1.mp4

Recording.5133.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687199616837069

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c556dc52d6e81d71
• Upwork Job ID: 1673861548329369600
• Last Price Increase: 2023-06-28

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[Pujan92]

Proposal

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

Inconsistency bug - App does not dynamically translate 'No activity yet' in search

What is the root cause of that problem?

For the SearchPage we are not updating options when the preferredLocale gets updated and that is the reason for this issue.

App/src/pages/SearchPage.js

Lines 72 to 75 in 820fa93

	if (_.isEqual(prevProps.reports, this.props.reports) && _.isEqual(prevProps.personalDetails, this.props.personalDetails)) {
	return;
	}
	this.updateOptions();

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

Add preferredLocale condition here in componentDidUpdate for early return if the preferredLocale isn't gets changed otherwise it should call updateOptions.

App/src/pages/SearchPage.js

Lines 72 to 75 in 820fa93

	if (_.isEqual(prevProps.reports, this.props.reports) && _.isEqual(prevProps.personalDetails, this.props.personalDetails)) {
	return;
	}
	this.updateOptions();

&& this.props.preferredLocale === prevProps.preferredLocale

OR

_.isEqual(this.props.preferredLocale, prevProps.preferredLocale)

[trjExpensify]

Hm, this sounds like a similar root cause as this one and this one. @abdulrahuman5196 can you offer an opinion on that?

[abdulrahuman5196]

@trjExpensify. No, It seems both are different actually for this case atleast. Here we are having the data but not translating properly. Other bugs are kind of case where the text shown itself is wrong regardless of the language choosen.

For eg: We only show this 'No activity yet' in search page for money requests. There can be cases where 'No activiy yet' is shown wrongly instead of a user text message which is a different issue. But this issue is only concerned about translation.

While checking on this bug I also ended up reviewing this proposal by @Pujan92 to check on cause #21537 (comment), the proposal is working and make sense for this issue.
If possible and if this bug is planed to go external, can I be assigned C+?

[trjExpensify]

Okay got it, thanks for confirming and explaining that. Assigning you as the C+. If you're happy with @Pujan92's proposal green light it and someone from our side will get assigned for a secondary. 👍

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c556dc52d6e81d71

[melvin-bot]

Current assignee @trjExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

[tienifr]

Proposal

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

App does not dynamically update 'No activity yet' in search to Spanish on language change

What is the root cause of that problem?

It's here

App/src/pages/SearchPage.js

Line 75 in 52a1891

this.updateOptions();

where we do not updateOptions if the preferredLocale change.

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

I've seen this kind of issues many times, let's see how the current flow for such messages is designed:

1. We do some calculation to get the translated text message
2. We display the text message
3. If the preferredLocale, we do those calculations again to get another text message with the new preferredLocale

We can see that the calculation step is now concerned with what languages the user is using (since it will return different result). Also when the language changes, we're doing all the calculations again, even though the result ('No activity yet') is the same, the only difference is the language. And in this case the calculation (updateOptions) is a compute-intensive one so can lead to performance issue.

So the thing to note here is that we're conflating "calculation of message" and "display of message", which will lead to many similar issues in the future if we continue this way.

So I propose a new design:

1. We do some calculation and return which messageKey (and relevant params) we want to display, now the calculation is not concerned about what language the user is using. In case of No activity yet the function will return 'report.noActivityYet', not No activity yet if English (and Sin actividad todavía if Spanish), since it's not the calculation's concern.
2. When we display the message, we'll translate it accordingly, since language is the display's concern.
3. When the language changes, we don't need to run the calculation again, because the calculation is not concerned about the language. Only where we display it, we display it in the correct language.

It's not a big change, but will help us avoid such issues in the future, and also improve the performance a bit since calculation doesn't need to be rerun.

The implementation details are mentioned below here and here

What alternative solutions did you explore? (Optional)

NA

[abdulrahuman5196]

@tienifr Thank you for the proposal, but I don't think i would be aligning with the proposal here since, it is not just No activity yet will be sent, but it could also have text messages so it would not be wise to say we can only send the translation key and translate while rendering the item.
And I don't think the re-computation is super heavy, and we are not going to be doing it during every render, it is only going to be done during the locale changes which shouldn't be high numbers comparing the renders.

[abdulrahuman5196]

@Pujan92 's proposal here #21537 (comment) looks good and works well.

🎀👀🎀
C+ Reviewed

🎀 👀 🎀 C+ reviewed

cc: @trjExpensify

[melvin-bot]

Triggered auto assignment to @youssef-lr, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[abdulrahuman5196]

@youssef-lr Just for reference. The C+ reviewed comment is here - #21537 (comment)

[tienifr]

it is not just No activity yet will be sent, but it could also have text messages

@abdulrahuman5196 can you share some examples of the text messages that might show in this case? I don't find any in the getHeaderMessage method.

@abdulrahuman5196 if there's no such case of user-generated text messages, do you agree that my approach is better both in terms of code design and performance?

@abdulrahuman5196 there's another similar case where we're conflating the text message and the system message in the chat here, for which it's also universally agreed to use the translationKey as the best design.

cc @youssef-lr

[abdulrahuman5196]

@tienifr

can you share some examples of the text messages that might show in this case? I don't find any in the getHeaderMessage method.

Screen.Recording.2023-06-29.at.1.06.31.PM.mov

if there's no such case of user-generated text messages, do you agree that #21537 (comment) is better both in terms of code design and performance?

Even still I would question this. I don't think we are optimizing some super intensive task into a super light weight solution. The functionality is already light weight and the accepted proposal is also not going to increase it to super intensive task.

thttps://github.com//issues/19449#issuecomment-1561043031's another similar case where we're conflating the text message and the system message in the chat here, for which it's also universally agreed to use the translationKey as the best design.

I don't think both are similar. Even in the PR of the linked issue, you are actually translating with the translationKey and providing the translated value to the UI to render, it is not like render place is translating. The same is currently working in this case already, only issue here is its not re-rendering on locale change from other device.

So I would still go with the current accepted solution.

[tienifr]

can you share some examples of the text messages that might show in this case? I don't find any in the getHeaderMessage method.

@abdulrahuman5196 I think the above you shared is not the header message (or Expensify-defined message) that we're addressing 😅, in that case no translation will take place.

Even still I would question this. I don't think we are optimizing some super intensive task into a super light weight solution.

The updateOptions calls getOptions (a 300 LOC function) and it's even debounced, see here

App/src/pages/SearchPage.js

Line 58 in 562a838

this.debouncedUpdateOptions = _.debounce(this.updateOptions.bind(this), 75);

. So I think it's intensive as per our definition (otherwise we would not debounce it).

So I would still go with the current accepted solution.

Yup would love to hear what @youssef-lr thinks as well, thanks!

[trjExpensify]

@youssef-lr thoughts on this one, please? Let's keep it moving! :)

[Christinadobrzyn]

Hey @abdulrahuman5196 - will this job resolve the issue reported here #22051

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[trjExpensify]

@youssef-lr is back tomorrow!

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[trjExpensify]

@tienifr there are a bunch of conflicts in the PR to fix. What's the ETA on getting to them? Thanks!

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr 10 days overdue. Is anyone even seeing these? Hello?

[melvin-bot]

@youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr 12 days overdue. Walking. Toward. The. Light...

[melvin-bot]

This issue has not been updated in over 14 days. @youssef-lr, @trjExpensify, @abdulrahuman5196, @tienifr eroding to Weekly issue.

[trjExpensify]

Hey @tienifr @youssef-lr what's going on here? The PR has been open since July, we now have conflicts again. Thanks!

[tienifr]

I'm trying to resolve the conflicts

[trjExpensify]

As discussed in the open-source thread, we're going to close this issue. We've hit an impasse with the PR not going anywhere, and overall this is pretty minor.

[tienifr]

@trjExpensify Should I and @abdulrahuman5196 get paid? We spent a lot of time to create/review the PR. Thanks

[abdulrahuman5196]

@trjExpensify Should I and @abdulrahuman5196 get paid? We spent a lot of time to create/review the PR. Thanks

+1 on this.

[tienifr]

@trjExpensify Wdyt?

[trjExpensify]

Oh, sorry guys. This fell into the abyss after the close. Let me reopen as we discuss.

• The PR was opened mid-July 2023
• The last time the PR had commits was early November 2023.
• It sat dormant until the end of Jan 2024 when we decided to close it.

I think the months of inactivity on this issue isn't a great showing, and we should be mindful to approach the issues we take on with #urgency and #focus. We've closed "abandoned" PRs for inactivity of way less time than this one historically, so I'm curious what you both as tenured members of the community think is fair compensation and a reflection of the timeline on this issue?

[abdulrahuman5196]

Hi, @trjExpensify . I think this issue is one off cases, since it was created and had PR movements before the latest shift in focus. And both the contributor task and C+ approval was done on the PR and was waiting on internal review for some time.
I think we provide 25% and 50% of the payment on similar cases if considerable work was done on the issue and transferred or moved due to internal circumstances.

Anyways this is all IMO, do kindly take in your best judgement on this case.

[trjExpensify]

Cool, thanks for your input. I'm happy to pay out $250 (50% the current rate) for the work so far.

if considerable work was done on the issue and transferred or moved due to internal circumstances.

I will add on this that I don't think it was transferred elsewhere due to internal drivers, but the focus on this issue likely dropped off for other things. So let's just be careful to address that going forward all round.

Sound good?

[abdulrahuman5196]

Hi @trjExpensify , Sorry I missed this. Yes. I am fine the callout.

[trjExpensify]

Cool, offers have been sent to you both.

[abdulrahuman5196]

Thank you @trjExpensify . Accepted the offer.

[trjExpensify]

@abdulrahuman5196 paid!

[trjExpensify]

@tienifr paid, closing!