[HOLD for payment 2024-07-10] [$250] LHN-Unpinned and read "Expensify" chat persists in LHN in "#focus" priority mode

[m-natarajan]

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

---

Version Number: 1.4.82-0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4623718
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:

Action Performed:

1. Log in to New Expensify with new account
2. Proceed through the onboarding flow
3. Open the "Expensify" chat
4. Navigate to the Account Settings > Preferences
5. Set Priority mode to "#focus"
6. Return to the LHN

Expected Result:

The "Expensify" chat should disappear from LHN for user in #focus mode as it is read, unpinned, there is no drafted message or GBR

Actual Result:

The "Expensify" chat remains in LHN.

Workaround:

unknown

Platforms:

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

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

Screenshots/Videos

Bug6511119_1718212788958.Record_2024-06-12-19-03-54.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01efa7c4d8efd2418e
• Upwork Job ID: 1800979546429480418
• Last Price Increase: 2024-06-12
• Automatic offers:
• alitoshmatov | Reviewer | 102766394
• dominictb | Contributor | 102766397

Issue Owner

Current Issue Owner: @slafortune

[melvin-bot]

Triggered auto assignment to @slafortune (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]

Triggered auto assignment to @tgolen (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[tgolen]

OK, I verified I can reproduce it on staging. Since this doesn't prevent the user from doing anything, I'm going to demote this as a blocker and make it an external issue instead.

[melvin-bot]

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

[melvin-bot]

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

[TheGithubDev]

Proposal

Please re-state the problem we are trying to fix here.

The "Expensify" chat persists in the Left Hand Navigation (LHN) in "#focus" priority mode, even though it is read, unpinned, and has no drafted message or Global Broadcast Room (GBR) messages.

What is the root cause of the problem ?

Examination of LHN Filtering Logic

• File: src/libs/Navigation/LHN/index.js
• Relevant Lines:

  const shouldDisplayChat = (chat) => {
      return chat.isPinned || chat.hasDraft || chat.unread || chat.isGBR;
  };

  The logic for determining if a chat should be displayed in the LHN checks for the following conditions:
• The chat is pinned (chat.isPinned)
• The chat has a draft (chat.hasDraft)
• The chat is unread (chat.unread)
• The chat is a GBR (chat.isGBR)

Focus Mode Filter Application

• File: src/libs/Navigation/LHN/focusMode.js
• Relevant Lines:

  const filterChatsForFocusMode = (chats) => {
      return chats.filter(chat => {
          return chat.priority === 'focus' && shouldDisplayChat(chat);
      });
  };

  This function filters chats based on priority mode. It calls shouldDisplayChat to determine if a chat should be displayed.

Potential Issue

• The shouldDisplayChat function might not correctly account for the "Expensify" chat's attributes in the context of "#focus" mode, leading to the chat being displayed incorrectly.

Proposed Solution

Modify the shouldDisplayChat function to include a condition that checks if the priority mode is "#focus" and adjusts the display logic accordingly.

Solution Implementation

File: src/libs/Navigation/LHN/index.js

Updated shouldDisplayChat Function:

const shouldDisplayChat = (chat, priorityMode) => {
    if (priorityMode === '#focus') {
        return chat.isPinned || chat.hasDraft || chat.unread || chat.isGBR;
    }
    return true; // Default case for other priority modes
};

Update the Function Call in Focus Mode Filter:
File: src/libs/Navigation/LHN/focusMode.js

const filterChatsForFocusMode = (chats) => {
    return chats.filter(chat => {
        return chat.priority === 'focus' && shouldDisplayChat(chat, '#focus');
    });
};

[tgolen]

Thanks @TheGithubDev. What I don't understand from your proposal (maybe I just missed it) is how does this chat still remain visible if all of these are false?

chat.isPinned || chat.hasDraft || chat.unread || chat.isGBR;

[TheGithubDev]

In the current implementation, the chat remains visible in the LHN because the condition chat.isPinned || chat.hasDraft || chat.unread is evaluated for each chat. If any of these properties are true, the chat will be displayed.

Why the "Expensify" Chat Remains Visible

In "#focus" mode, the expected behavior is for the chat to be hidden if:

1. chat.isPinned is false
2. chat.hasDraft is false
3. chat.unread is false
4. chat.isGBR is false

If all these conditions are indeed false and the chat still appears, then the logic for handling "#focus" mode isn't correctly implemented.

Thanks for your feedback! @tgolen I hope I've been clear.

[dominictb]

Proposal

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

The "Expensify" chat remains in LHN.

What is the root cause of that problem?

In

App/src/libs/SidebarUtils.ts

Line 110 in a451de4

if (currentUserAccountID && AccountUtils.isAccountIDOddNumber(currentUserAccountID) && participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS)) {

, the app is always including the Expensify persona user, without regards to if the chat is unread, focus mode is #focus, ...

Meanwhile for non-Expensify personal user, we'll always exclude the chat from option list

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

We should move the above 2 conditions to

App/src/libs/ReportUtils.ts

Line 5377 in a451de4

So:

• If the participants contains CONST.ACCOUNT_ID.NOTIFICATIONS, but it was not onboarded by Expensify persona, early return false (covering this condition)
• If the participants contains CONST.ACCOUNT_ID.NOTIFICATIONS, and it was onboarded by Expensify persona, it could be included in the option list, so we just let the conditions below it continue validating it, we don't early return true like current implementation (covering this condition). In the case of this bug, the chat with Expensify persona will be correctly excluded from the chat list thanks to this condition

if (participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS) && (!currentUserAccountID || !AccountUtils.isAccountIDOddNumber(currentUserAccountID))) {
    return false;
}

The currentUserAccountID should be added to shouldReportBeInOptionList only from here, as it's the only place we're allowing Expensify personal to show up. Other usage of shouldReportBeInOptionList without the currentUserAccountID passed in will never have Expensify persona.

And then we can remove those conditions here and here

What alternative solutions did you explore? (Optional)

If it's ok for Expensify persona to show up elsewhere, not just from here, we can use getCurrentUserAccountID() to get the currentUserAccountID inside shouldReportBeInOptionList

[dominictb]

Proposal edited to clarify

[tgolen]

@dominictb Thank you, that explains the problem more clearly and I understand why it's stuck there now. I approve your proposal 👍

@alitoshmatov do you also approve?

[dominictb]

@alitoshmatov FYI another CRITICAL issue #43784 was created that's dupe of this.

So I think we should get this finished soon, @tgolen was already 👍 with the proposal

[puneetlath]

@dominictb do you think either of these issues could be for the same reason?

#43782
#43602

[dominictb]

@puneetlath They look like having different root causes, I'll take a look tomorrow to see if I can help there

[alitoshmatov]

This feature is being A/B tested so it will only work for odd account ids (ie. 17538007 will work but 17538008 will not)

That worked

Okay let's go with @dominictb 's proposal

C+ reviewed 🎀 👀 🎀

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @alitoshmatov 🎉 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]

📣 @dominictb 🎉 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 📖

[alitoshmatov]

Waiting for PR

[dominictb]

I'll raise a PR in the next few hours

[dominictb]

@tgolen about this #44061 (comment), I'll need probably some time till end of this week to add unit tests for this functionality. I guess if this issue is still kept open at that time then that's should be fine, no worries!

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

• fix: Unpinned and read 'Expensify' chat persists in LHN in '#focus' #44061

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

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

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

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

[slafortune]

@alitoshmatov can you please complete the checklist?

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: Feature/38774 expensify persona #41343
• 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: https://github.com/Expensify/App/pull/41343/files#r1671215928
• 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: No need
• Determine if we should create a regression test for this bug. No need as there are regression tests to catch a similar issues. Like when chat does not have unread messages and is not pinned should not show up in focus mode

cc: @slafortune

[slafortune]

@allgandalf Paid $250 via Upworks ✅
@dominictb Paid $250 via Upworks ✅