[HOLD for payment 2024-10-14] [$250] LHN - Members' room names are not updated in LHN after cache deletion

[IuliiaHerets]

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

---

Version Number: 9.0.34-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: https://expensify.testrail.io/index.php?/tests/view/4960406
Email or phone of affected tester (no customers): sustinov@applausemail.com
Issue reported by: Applause Internal Team

Action Performed:

Prerequisites:
Create a WS and add two employees to it

Steps:

1. Open NewExpensify app
2. Log in with a WS administrator account
3. Go to the account settings
4. Go to the Troubleshoot menu
5. Click on “Clear cache and restart” and clear the cache
6. Go back to the LHN Inbox

Expected Result:

Members' room names should be updated and display the room name in LHN after the cache is deleted

Actual Result:

Members' room names are not updated in LHN after cache deletion

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6602770_1726253910821.Recording__1601.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021836832143423032683
• Upwork Job ID: 1836832143423032683
• Last Price Increase: 2024-09-26
• Automatic offers:
• ikevin127 | Reviewer | 104231232
• truph01 | Contributor | 104231233

Issue Owner

Current Issue Owner: @zanyrenney

[melvin-bot]

Triggered auto assignment to @zanyrenney (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.

[IuliiaHerets]

@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[melvin-bot]

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

[zanyrenney]

whoops!!!

[zanyrenney]

yeh i agree this is a bug. we should look at this in ND quality.

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

💰 Looking for proposal (if issue is reproducible).

[huult]

Proposal

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

Members' room names are not updated in LHN after cache deletion

What is the root cause of that problem?

When we clear the cache and restart, personalDetails will return {isOptimisticPersonalDetail: true}, and reportOwnerDisplayName will be set to report?.reportName because getDisplayNameForParticipant(ownerAccountID) or login will not have a value. As a result, we will see the name as Chat Report

App/src/libs/ReportUtils.ts

Line 2681 in 9160fa5

const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName;

And when we send a message, personalDetails will be updated after the AddComment call is completed. At this point, getDisplayNameForParticipant(ownerAccountID) will return the participant's display name, and login will also have a value. As a result, the report name will be set based on the logic we check. Therefore, reportOwnerDisplayName will return the value from getDisplayNameForParticipant(ownerAccountID), and this issue will occur.

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

I think this is not a bug, but we need to confirm the expected behavior in this case.

Solution 1

If we want to keep the report name as the members' room name after clearing the cache or restarting, we can check it like this.

// .src/libs/ReportUtils.ts#L560
+ let hasResetOccurredBefore: boolean = false;
+ Onyx.connect({
+    key: ONYXKEYS.HAS_RESET_OCCURRED_BEFORE,
+    waitForCollectionCallback: true,
+    callback: (value) => {
+        hasResetOccurredBefore = value;
+    },
+ });

// .src/libs/ReportUtils.ts#L2681
- const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName;
+    const reportOwnerDisplayName = hasResetOccurredBefore 
+    ? report?.reportName ?? getDisplayNameForParticipant(ownerAccountID) ?? login 
+    : getDisplayNameForParticipant(ownerAccountID) ?? login ?? report?.reportName;

// .src/pages/settings/Troubleshoot/TroubleshootPage.tsx#L161
         Onyx.clear(App.KEYS_TO_PRESERVE).then(() => {
+                                        Onyx.set(ONYXKEYS.HAS_RESET_OCCURRED_BEFORE, true);
                                        App.openApp();
                                    });

With that solution, we need backend support to store the key by account ID.

Solution 2

We should use the report name instead of the participant's name.

// .src/libs/ReportUtils.ts#L2681
- const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName;
+ const reportOwnerDisplayName = report?.reportName;

[melvin-bot]

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

[zanyrenney]

@ikevin127 please could you review the proposal above?

[ikevin127]

@huult Thanks for your proposal. Unfortunately, I don't see how any of the proposed solutions would fulfil the Expected result because even if we involve BE -> the new key would still not preserve personalDetails in order to display the room name in LHN after the cache is cleared.

Correct me if I'm wrong in any way or if I misunderstood the issue's expected result.

Otherwise, what I'd do would be to preserve ONYXKEYS.PERSONAL_DETAILS_LIST as well here:

App/src/pages/settings/Troubleshoot/TroubleshootPage.tsx

Lines 156 to 158 in dc74ee3

	Onyx.clear(App.KEYS_TO_PRESERVE).then(() => {
	App.openApp();
	});

by extending [...App.KEYS_TO_PRESERVE, ONYXKEYS.PERSONAL_DETAILS_LIST], which would ensure that personalDetails would not be removed upon cache deletion, therefore displaying the room name in LHN after the cache is cleared.

[truph01]

Proposal

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

• Members' room names are not updated in LHN after cache deletion

What is the root cause of that problem?

• We implemented a cache mechanism to store report names by reportID, eliminating redundant computations for frequently used reports in PR. We will use the cache data if we meet the condition:
  

  App/src/libs/ReportUtils.ts

  Line 3696 in 9160fa5

  if (reportNameFromCache?.reportName && reportNameFromCache.reportName === report?.reportName) {

• In this bug, when clearing the cache data, the report name is set to Chat Report because in:

App/src/libs/ReportUtils.ts

Line 2681 in 9160fa5

const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName;

getDisplayNameForParticipant(ownerAccountID) and login is empty so we fallback to report?.reportName.

• Then, that data is stored to the report name cache.

• In the next time, because the condition:

App/src/libs/ReportUtils.ts

Line 3696 in 9160fa5

if (reportNameFromCache?.reportName && reportNameFromCache.reportName === report?.reportName) {

is true, so we always use the report name from cache data instead of calculating a new one.

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

• We need to update above condition to:

       if (reportNameFromCache?.reportName && reportNameFromCache.reportName === report?.reportName && reportNameFromCache.reportName !== CONST.REPORT.DEFAULT_REPORT_NAME ) {

so if the report name in cache data is Chat Report, we always calculate a new one because we don't want to display the report name as Chat Report.

What alternative solutions did you explore? (Optional)

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[huult]

@ikevin127 , thank you for review.

As part of the RCA, I explained that after “Clear cache and restart”, getDisplayNameForParticipant(ownerAccountID) and login return undefined, causing the value of report?.reportName to be used instead.

// .src/libs/ReportUtils.ts#L2681
const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName; 

1. Click `Clear cache and restart`.
2. getDisplayNameForParticipant(ownerAccountID) is undefined and login is undefined
3. report?.reportName is `Chat room`

const reportOwnerDisplayName = undefined || undefined || "Chat roo.";

So reportOwnerDisplayName will be `Chat room`

After logging out and adding a new comment, getDisplayNameForParticipant(ownerAccountID) has a value because we store the value from the API response locally. This issue occurs when the member's room is restored.

// .src/libs/ReportUtils.ts#L2681
const reportOwnerDisplayName = getDisplayNameForParticipant(ownerAccountID) || login || report?.reportName; 

1. Add new comment.
2. getDisplayNameForParticipant(ownerAccountID) is "applausetester+sucategoy...." that is participant name
3. report?.reportName is `Chat room`

const reportOwnerDisplayName = "applausetester+sucategoy...." || "" || "Chat room";

So reportOwnerDisplayName will be `applausetester+sucategoy....`

The two points described above explain why the issue occurred.

Suggestions and further explanation of my proposal

Case: If the user clicks 'Clear cache and restart', we use reportName. After that, if the user sends a new comment to this room, should we switch to using the participant's name for the room name, or should we continue displaying the report name as the room name?

1. User clears cache and restarts. 
2. Room name displays the report name. 
3. User sends a new comment. 
4. Should the room name display the report name or the participant's name?

Solution 1, as I mentioned in my proposal, is to continue using the report name for the room name even after the user sends a new comment. (This solution is to use the report name to display the room name).

The ideal solution is to create a new key to monitor the status of whether it has been clear cache or restart. After that, I will use that key to detect reportOwnerDisplayName in order to use the report name for the room name after 'Clear cache and restart' has been used.

// if used `'Clear cache and restart'` ? report?.reportName : getDisplayNameForParticipant(ownerAccountID) ?? login ?? report?.reportName

+    const reportOwnerDisplayName = hasResetOccurredBefore 
+    ? report?.reportName ?? getDisplayNameForParticipant(ownerAccountID) ?? login 
+    : getDisplayNameForParticipant(ownerAccountID) ?? login ?? report?.reportName;

Regarding the backend update, we will store that key on the server so that when a user logs in on another device, they can retrieve that key and apply the same logic for displaying the room name.

[ikevin127]

@huult I appreciate the detailed response to my review. The reason I'm reserved in proceeding with a solution which involves BE is because, based on reviewing and testing I don't think BE changes are required to fix this issue since that would only add more computation to the issue of cache clearing which is what we're dealing with here.

[MariaHCD]

I reviewed the original issue for the intention behind this cache and it looks like it is purely a frontend optimization. Is the suggestion to have the caching mechanism on the BE instead? I would imagine that this would worsen the timing for getReportName.

Additionally, in the scenario outlined, why would Device B not have a cache?

[MariaHCD]

@truph01's proposal makes sense to me - if the default report name is found in the cache, recalculate to fetch the actual report name

[melvin-bot]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[huult]

@MariaHCD
Currently, our solution for this ticket is as follows: if an account has cleared the cache or restarted, we will use the room name as the reportName. Conversely, if the account has not cleared the cache or restarted, we will use the room name as the name of the participant.

Additionally, in the outlined scenario, why wouldn't Device B have a cache?
Let me provide a scenario to explain this:

• User Huu logs into Device A (or browser).
• Next, User Huu creates a workspace and adds a member. At this point, the room name will be set to the participant's name.
• After that, User Huu clears the cache or restarts.
• The room name will then be set to reportName.
• Now, User Huu logs into Device B.
• At this point, Account Huu on Device B will not know whether Account Huu has cleared the cache or restarted because we haven't stored this information on the server. As a result, the room name will default to the participant's name, which will be different from the room name on Account Huu on Device A.

And my proposal can solve this problem without needing backend changes, and I used BE to resolve the issue in the scenario above.

[MariaHCD]

Account Huu on Device B will not know whether Account Huu has cleared the cache or restarted

Once the user logs in on Device B, we call the API that fetches the latest information anyway, correct?

[huult]

Yes, that's correct! That approach covers this case.

[truph01]

PR is ready

[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 9.0.45-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: LHN - Members' room names are displayed incorrectly #50082

If no regressions arise, payment will be issued on 2024-10-14. 🎊

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

• @ikevin127 requires payment automatic offer (Reviewer)
• @truph01 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:

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@ikevin127] 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:
• [@ikevin127] 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:
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.
• [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ikevin127]

• [@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Add cache to getReportName #47156.
• [@ikevin127] 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: Add cache to getReportName #47156 (comment).
• [@ikevin127] 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: N/A.
• [@ikevin127] Determine if we should create a regression test for this bug.
• [@ikevin127] 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.

Regression Test Proposal

Prerequisites:

• Create a WS and add two employees to it.

1. Open NewExpensify app.
2. Log in with a WS admin account.
3. Go to the account settings.
4. Go to the Troubleshoot menu.
5. Click on Clear cache and restart to clear the cache.
6. Go back to the LHN Inbox.
7. Verify that members' room names should be updated and display the room name in LHN after the cache is deleted, instead of showing default Chat Report name.

Do we agree 👍 or 👎.

[zanyrenney]

#50727

[zanyrenney]

payment summary

@ikevin127 requires payment automatic offer (Reviewer) - paid via Upwork $250
@truph01 requires payment automatic offer (Contributor) - paid via Upwork $250