Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Desktop - Unread count is incorrect when joining a thread #27512

Closed
1 of 6 tasks
kbecciv opened this issue Sep 15, 2023 · 19 comments
Closed
1 of 6 tasks

[$500] Desktop - Unread count is incorrect when joining a thread #27512

kbecciv opened this issue Sep 15, 2023 · 19 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Sep 15, 2023

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:

  1. Clicked on a thread made by other users that already had comments
  2. Didn't add any comment
  3. Left the thread
  4. The thread disappeared from LHN

Expected Result:

New messages inside those threads should not be counted on my unread indications

Actual Result:

New messages on the threads that I can't see on my LHN are counting towards the unread counter on the app icon

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.67-1
Reproducible in staging?: n/a
Reproducible in production?: n./a
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

n/a

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a81e245f41f24d05
  • Upwork Job ID: 1703973226245242880
  • Last Price Increase: 2023-09-19
  • Automatic offers:
    • fedirjh | Reviewer | 26746291
    • dukenv0307 | Contributor | 26746293
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 15, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 15, 2023

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

@roryabraham
Copy link
Contributor

This is super annoying and front-end fixable so I'm going to make it external

@roryabraham roryabraham self-assigned this Sep 19, 2023
@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Sep 19, 2023
@melvin-bot melvin-bot bot changed the title Desktop - Unread count is incorrect when joining a thread [$500] Desktop - Unread count is incorrect when joining a thread Sep 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

@roryabraham
Copy link
Contributor

roryabraham commented Sep 19, 2023

At a high level, I think we maybe just need to call Report.leaveRoom when you navigate away from a thread that you have not commented on.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Sep 19, 2023

Proposal

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

New messages on the threads that I can't see on my LHN are counting towards the unread counter on the app icon

What is the root cause of that problem?

When leaving a thread without adding any comments, the thread is still in the user list of reports and the notifications are still sent.

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

@roryabraham 's suggestion is correct for this case.

We should:

  • When the report screen unmounts/lose focus/no longer the top report, check if the user has any comments on the thread, if not, call Report.leaveRoom

There could be cases where the user does not leave the thread, but closes the tab and opens another report by direct link, then the thread is not left. It's an edge case but if we want to fix it, at the place we fetch the reports, we can check if there's any thread the user doesn't have any comments on and call Report.leaveRoom on those threads.

What alternative solutions did you explore? (Optional)

Instead of checking if users have comments on the thread, we can check if "notificationPreference": "hidden", instead, since if users didn't comment, the notification will be hidden, but this has an edge case where user comments on it but manually mark the thread as notificationPreference: hidden, so I still prefer the above.

@roryabraham
Copy link
Contributor

roryabraham commented Sep 19, 2023

I think we should address those edge cases in the back-end – maybe in FixAccount, which I think still runs as part of OpenApp?

Edit: Maybe FixAccount isn't run from NewDot anymore? Anyways, let's address that edge case separately...

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

📣 @fedirjh 🎉 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
Copy link

melvin-bot bot commented Sep 19, 2023

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

@dukenv0307
Copy link
Contributor

@roryabraham sorry I don't have access to that repo for FixAccount so I don't know exactly how it's related to OpenApp. But I think that will work. If the user's last chat is the thread where there's no comment, the OpenReport will still be called. So If OpenApp clears it before, it won't cause any issue.

I agree that edge case can be handled separately.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 20, 2023
@dukenv0307
Copy link
Contributor

@fedirjh The PR is ready for review

@garrettmknight
Copy link
Contributor

Bumped the PR to investigate the checks that weren't successful.

@dukenv0307
Copy link
Contributor

@garrettmknight @roryabraham Should we process payment for this issue? We spent lots of effort in the PR to discuss and implement my main solution and everything is almost done. Additionally, the PR that fixed this issue is the same as my alternative solution.

@garrettmknight
Copy link
Contributor

Since plenty of work was done here and we got pretty deep into the PR I think this counts for pay.

Summary of payments on this issue:

@garrettmknight
Copy link
Contributor

@fedirjh accept the new offer and I'll pay out.

@fedirjh
Copy link
Contributor

fedirjh commented Nov 2, 2023

@garrettmknight Done.

@garrettmknight
Copy link
Contributor

All paid up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests

5 participants