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

@AmjedNazzal

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

Action Performed:

For Web and Desktop:

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

For mobile

Go to any report and hold press on any message and reply in thread
While in the thread, click the three dots and click "pin"
Go back to the LHN list of reports and hold press the empty pinned thread and choose mark as unread
Hold press again on the pinned thread and choose unpin
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?

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

@tjferriss

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

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

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

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

@AmjedNazzal

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

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.

@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

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

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)

@AmjedNazzal

@cubuspl42 @AmjedNazzal payments have been made via Upworks

@tjferriss

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

@tjferriss

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

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.

@allroundexperts

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

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 😄

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.

Navigation Menu

Repository navigation

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

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

Result

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

70 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Activity

melvin-bot commented on Sep 30, 2023

melvin-bot commented on Sep 30, 2023

melvin-bot commented on Sep 30, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot commented on Sep 30, 2023

kbecciv commented on Sep 30, 2023

Proposal

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

What is the root cause of that problem?

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

Result

allroundexperts commented on Sep 30, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

AmjedNazzal commented on Sep 30, 2023

allroundexperts commented on Sep 30, 2023

70 remaining items

jakub-trzebiatowski commented on Dec 15, 2023

tjferriss commented on Dec 15, 2023

allroundexperts commented on Dec 15, 2023

melvin-bot commented on Dec 19, 2023

tjferriss commented on Dec 19, 2023

jakub-trzebiatowski commented on Dec 20, 2023

allroundexperts commented on Dec 20, 2023

jakub-trzebiatowski commented on Dec 20, 2023

jakub-trzebiatowski commented on Dec 22, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions