[HOLD for payment 2023-12-15] MEDIUM: [$500] Notifications - Reports removed from LHN can still show notifications leading to unremovable notifications

[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:

For Web and Desktop:

1. Send a message in any report
2. Hover over the message and reply in thread
3. Right click on the thread and click mark as unread
4. Click out of the thread and notice a notification that you can no longer remove
5. Additionally if you remove the original message there's is no longer any possible way of removing the notification

For mobile

1. Go to any report and hold press on any message and reply in thread
2. While in the thread, click the three dots and click "pin"
3. Go back to the LHN list of reports and hold press the empty pinned thread and choose mark as unread
4. Hold press again on the pinned thread and choose unpin
5. Notice the thread getting hidden but the notification remaining

Expected Result:

The user should not be stuck with notifications that are impossible to remove.

Actual Result:

User is stuck with notifications that are impossible to remove, even unintentionally, I have notifications that I can't remove on all of my accounts.

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.75.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

Screen.Recording.2023-09-29.at.10.22.55.PM.mov

Screen.Recording.2023-09-29.at.10.13.09.PM.mov

WhatsApp.Video.2023-09-29.at.10.15.41.PM.mp4

Screen.Recording.2023-09-28.at.6.29.50.PM.mov

Recording.4808.mp4

Expensify/Expensify Issue URL:
Issue reported by: @AmjedNazzal
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695916782976289

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01082d000584ab5766
• Upwork Job ID: 1708128118997794816
• Last Price Increase: 2023-09-30
• Automatic offers:
• cubuspl42 | Reviewer | 27217153
• AmjedNazzal | Reporter | 27217154

[melvin-bot]

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

[melvin-bot]

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

[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

[melvin-bot]

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

[kbecciv]

Proposal by: @AmjedNazzal
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695916782976289

Proposal

AmjedNazzal on GH

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

Reports removed from LHN can still show notifications leading to unremovable notifications

What is the root cause of that problem?

This is a problem with the logic of the notifications system colliding with the logic of LHN, in LHN we remove any report/thread that does not have replies in it by using isEmptyChat in ReportUtils.shouldReportBeInOptionList, on the other hand, we are passing ReportUtils.isUnread all of the reports regardless if they are hidden or not and just shows notifications for any report where lastReadTime < lastVisibleActionCreated leading to this issue.

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

Firstly we should make changes to UnreadIndicatorUpdater to correctly handle hidden reports.

callback: (reportsFromOnyx) => {
    if (navigationRef.isReady()) {
        const filteredReports = _.filter(reportsFromOnyx, (report) => ReportUtils.shouldReportBeInOptionList(report, Navigation.getTopmostReportId(), isInGSDMode, betas, policies, allReportActions, true));
        const unreadReports = _.filter(filteredReports, ReportUtils.isUnread);
        updateUnread(_.size(unreadReports));
    }
},

We can then make use of SidebarLinksData to keep the notifications and the state of the visible reports consistent by watching for changes in optionListItemsWithCurrentReport to account for cases where reports are hidden by performing an update on the notifications whenever something is added or removed from the LHN insuring that nothing hidden could push a notification while at the same time preserving the ability to mark any report as unread and seeing real time changes in the notifications count.

useEffect(() => {
    if (currentReportID) {
        const isInGSDMode = priorityMode === CONST.PRIORITY_MODE.GSD;
        const filteredReports = _.filter(chatReports, (report) => ReportUtils.shouldReportBeInOptionList(report, currentReportID, isInGSDMode, betas, policies, allReportActions, true));
        const unreadReports = _.filter(filteredReports, ReportUtils.isUnread);
        updateUnread(_.size(unreadReports));
    }
  }, [optionListItemsWithCurrentReport]);

This will ensure that we have an extra safeguard for unexpected inconsistencies between the LHN and the unread status

Result

Screen.Recording.2023-10-01.at.2.35.11.AM.mov

[allroundexperts]

Proposal

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

Reports not visible still show up in notifications

What is the root cause of that problem?

The root cause of the issue is that we're just relying on the unread function to check if we should show a notification or not. This can be seen here.

Using this proposal would cause a regression where unread option won't work for new chats. Here's a video of the problem that it introduces:

Screen.Recording.2023-09-30.at.8.06.44.PM.mov

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

Instead of using unread function here, we should use the following:

        const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.unread(report) && ReportUtils.isEmptyReport(report));

In order to implement above, we'll have to create a new utility function called isEmptyReport having the following logic:

function isEmptyReport(report) {
   const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
   return !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && 
   !lastVisibleMessage.lastMessageTranslationKey;
}

Alternatively, we can also use the following to mimic it more accurately with LHN.

        const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.shouldReportBeInOptionList(report, null, true, ..., true));

What alternative solutions did you explore? (Optional)

Alternatively, we can also hide the mark as unread option for empty chats. In order to do this, we need to make use of the isEmptyReport utility function defined above and add it here.

[AmjedNazzal]

@allroundexperts What's the point of letting the user mark something as unread when the user is A) Viewing that thread already, and B) That thread is empty? if anything we should just remove mark as unread for empty threads or at least ignore the marking as unread to begin with because otherwise there will be a whole bunch of hidden reports that are marked as unread in the Onyx store

[allroundexperts]

@allroundexperts What's the point of letting the user mark something as unread when the user is A) Viewing that thread already, and B) That thread is empty? if anything we should just remove mark as unread for empty threads or at least ignore the marking as unread to begin with because otherwise there will be a whole bunch of hidden reports that are marked as unread in the Onyx store

I would agree with that. That's what I wrote in my alternate proposal.

[cubuspl42]

@AmjedNazzal @allroundexperts

What's the use of those reports that are hidden in the LHN? Does the app reason about them at all? In which places we want not to exclude them from any processing?

[AmjedNazzal]

@cubuspl42 I don't know but what we seem to be doing is just hide empty threads rather than removing them from the store:

    const isEmptyChat = !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;
    const canHideReport = shouldHideReport(report, currentReportId);

    // Hide only chat threads that haven't been commented on (other threads are actionable)
    if (isChatThread(report) && canHideReport && isEmptyChat) {
        return false;
    }```

[allroundexperts]

I think we're not removing it because there might be whispers that are in it.

[cubuspl42]

@allroundexperts What are whispers?

Actually, I'm not pushing to delete them from the persistent storage, as this is a big decision to do so, but I'm wondering if we're not missing some model-level API like ReportUtils.filterExisting(reportsFromOnyx)... Because it seems like there might be a non-explicitly-modeled concept of hidden / archived / inaccessible reports that some / most / all (?) algorithms processing reports should use.

I believe that both LHN and unread indicators logic (and possibly more parts of the domain logic) should operate on the same model of user-perceivable reports, or at least be allowed to do so.

The fact that withOnyx gives React components (the UI layer) direct access to the data persistence layer that can be later optionally processed by domain/business logic *Utils operator is itself a smell, but we won't change that.

[AmjedNazzal]

@cubuspl42 If removing empty threads is a better solution, what we can do is create a utility to check if there are threads that should be removed:

function shouldRemoveThread(report, currentReportId) {
    const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
    const isEmptyChat = !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;
    const isCurentReport = report.reportID === currentReportId;

    if (isChatThread(report) && isEmptyChat && !isCurentReport) {
        return true;
    }  
}

We can call this in SidebarLinksData whenever a change happens in the LHN meaning an empty thread could've been created and then hidden, and from there we could remove the empty thread from the onyx:

useEffect(() => {
        
        if (currentReportID) {
        const threadsToRemove = _.filter(chatReports, (report) => ReportUtils.shouldRemoveThread(report, currentReportID));
        
        threadsToRemove.forEach((thread) => {
            const { reportID } = thread;
            
            Report.leaveRoom(reportID);
          });
        }
      }, [optionListItemsWithCurrentReport]);

[allroundexperts]

@allroundexperts What are whispers?

Whispers are sort of notification messages from Concierge. You get one for example, when you join a public room.

[AmjedNazzal]

I don't think whispers are relevant here, we are hiding empty threads even when there's a whisper in them.

[allroundexperts]

I don't think whispers are relevant here, we are hiding empty threads even when there's a whisper in them.

I think the same. I said that we're saving the empty chat in local storage because of it.

[AmjedNazzal]

As you can see in the video, the user can still leave a thread that has a whisper in it so I don't see why we can't just call leaveRoom for empty threads, at least if there is a reason it doesn't seem like it would be whispers. According to tests I've done the notifications from these empty threads do go away when we call leaveRoom.

Screen.Recording.2023-10-02.at.7.28.16.PM.mov

[allroundexperts]

@AmjedNazzal Apologies for the confusion. I mean't something very different.

[tjferriss]

Coming in late, but I am able to reproduce the bug.

[cubuspl42]

The existing proposals are still not satisfying. In my opinion, we should introduce a notion of active reports and:

• Only present the active reports on the LHN
• Only consider the active reports when displaying the unread indicator
• ...and possibly only consider active reports in other places (?)

In one of the previous comments, I used the adjective "existing", but "active" may be a better name. Still, it's more about a concept. And pragmatically, it would end up as a helper in ReportUtils.

I'm not suggesting something with significant observable differences from what was proposed so far, but rather I disagree with the way things were modeled in the solutions proposed so far.

Please let me know if you see any flaw in this model.

[AmjedNazzal]

@cubuspl42 we can consolidate both calls into one utility like so:

function isActiveAndUnread(report, currentReportID = null) {
    return shouldReportBeInOptionList(report, currentReportID) && isUnread(report);
}

And from there we can call this whenever needed, for example:

Onyx.connect({
   key: ONYXKEYS.COLLECTION.REPORT,
   waitForCollectionCallback: true,
   callback: (reportsFromOnyx) => {
       const unreadReports = _.filter(reportsFromOnyx, report => ReportUtils.isActiveAndUnread(report));
       updateUnread(_.size(unreadReports));
   },
});

useEffect(() => {
        if (currentReportID) {
            const unreadReports = _.filter(chatReports, report => ReportUtils.isActiveAndUnread(report, currentReportID));
            updateUnread(_.size(unreadReports));
        }
}, [optionListItemsWithCurrentReport]);

To clarify, the reason I'm calling this utility in SidebarLinksData is to insure that any changes in the list of reports such as a report being added or the currentReportID changing is captured and used to determine the shown notifications.

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

• fix: notification count for empty reports #30133

If no regressions arise, payment will be issued on 2023-12-15. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @cubuspl42 requires payment automatic offer (Reviewer)
• @allroundexperts requires payment through NewDot Manual Requests
• @AmjedNazzal requires payment automatic offer (Reporter)

[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:

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

[tjferriss]

Waiting on hold period before sending payment. @cubuspl42 / @allroundexperts can you get started on the checklist?

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • This can be treated as a clarification of the expected behavior; the notifications referred to existing entities that just couldn't be navigated to
• 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:
  • N/A
• 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 for additional discussion
• Determine if we should create a regression test for this bug.
  • Up to the QA team
• 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.
  • Note your notification count (let's say it's 10)
  • Send a message in any report
  • Replay in a thread to that message
  • Mark the first message in the thread (the one you're replying to) as unread
    • Verify that the notification count increased (in our case, to 11)
  • Navigate out of the thread
    • Verify that you can't see the thread just left in the LHN
    • Verify that the notification count is now as initially (in our case, 10)

[tjferriss]

@cubuspl42 @AmjedNazzal payments have been made via Upworks

[allroundexperts]

Hi @tjferriss!

This got merged after about a month. Back then, we had a late merge penalty. So this is not eligible for any payment to me and @cubuspl42.

However, I think the delay was on my part and @cubuspl42 was pretty prompt throughout. I would request to not apply the late penalty for him. It's fine if it applies to me (Although this was a tough PR with a lot of back and forth conversations).

[melvin-bot]

@cubuspl42, @tjferriss, @Gonals, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[tjferriss]

However, I think the delay was on my part and @cubuspl42 was pretty prompt throughout. I would request to not apply the late penalty for him.

I'll agree with you. I think we're all set here.

[cubuspl42]

@allroundexperts Thank you for these words.

From my perspective, this is a workflow thing. My primary tool for handling all issues I'm assigned to is the GitHub inbox. When some action is needed on the Contributor side, I inform them or ask a question and mark the notification as "Done".

Very rarely (it might be the first or second time), the Contributor misses the question and I move on. The issue and PR get forgotten.

I'm not sure if it's Melvin's malfunction or just a blind spot in Melvin's logic. It would be great if he could remind us about such stalled issues.

[allroundexperts]

That does happen but in this case, I think it was more of my fault. I saw your reminder but was at an airport (preparing for a 5h flight) and just completely forgot about this 😄

[cubuspl42]

Taking into consideration that this caused a regression, and the time it took to merge, let's just agree we're all set and payments do not apply.

Let's move on.