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

[HOLD for BE work] Workspace chats does not show as bold for the new expense report requests #38778

Closed
1 of 6 tasks
m-natarajan opened this issue Mar 21, 2024 · 26 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Mar 21, 2024

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.55-1
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
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710776229802789

Action Performed:

  1. Send a money request to a workspace chat
  2. Login as approver

Expected Result:

Message should appear bold in LHN

Actual Result:

Messages does not show in bold

Workaround:

unknonw

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

Add any screenshot/video evidence

Recording.2863.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ed3b85901af1a5cf
  • Upwork Job ID: 1772295775561621504
  • Last Price Increase: 2024-04-01
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 21, 2024
Copy link

melvin-bot bot commented Mar 21, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Mar 22, 2024

Proposal

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

  • Workspace chats does not show as bold for the new expense report requests

What is the root cause of that problem?

  • We use this logic to check if the report is unread or not. In case the last message is ReportPreview, the !!report.lastActorAccountID is falsewhich leads to it does not show as bold.

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

  • We can update this to:
    result.isUnread = ReportUtils.isUnread(report) && (ReportActionsUtils.isReportPreviewAction(visibleReportActionItems[report.reportID]) ? true : !!report.lastActorAccountID)

What alternative solutions did you explore? (Optional)

  • NA

@bernhardoj
Copy link
Contributor

Proposal

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

The chat item in LHN doesn't show as bold for the workspace admin/approver when there is an expense preview

What is the root cause of that problem?

#36950 changes the unread logic a bit by checking whether the report has a lastActorAccountID or not.

result.isUnread = ReportUtils.isUnread(report) && !!report.lastActorAccountID;

But in case the last action is a report preview, the last actor account ID is 0.
image

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

First of all, we should revert #36950 as it doesn't fix the root cause of the issue. The issue that the PR is trying to fix is that when we delete the last message in a room from device A, the lastVisibleActionCreated property of the report isn't updated in device B, so the logic of unread, that is

return lastReadTime < lastVisibleActionCreated || lastReadTime < lastMentionedTime;

stays true. It will be updated once the device B opens the room. So, the problem is in the pusher that doesn't send the update to device B. Instead of patching up the issue, we should fix the pusher.

fyi, when we delete the last message, we set the lastVisibleActionCreated to empty optimistically, that's why the device A doesn't experience the issue. I think it should be set to the CREATED action created value, but I guess that's another topic to discuss

@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Mar 25, 2024
@melvin-bot melvin-bot bot changed the title Workspace chats does not show as bold for the new expense report requests [$500] Workspace chats does not show as bold for the new expense report requests Mar 25, 2024
Copy link

melvin-bot bot commented Mar 25, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 25, 2024
Copy link

melvin-bot bot commented Mar 25, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Mar 25, 2024
@jayeshmangwani
Copy link
Contributor

jayeshmangwani commented Mar 27, 2024

Reviewing Proposals , Will update tomorrow

@jayeshmangwani
Copy link
Contributor

Thanks for the proposals @nkdengineer @bernhardoj ,
@nkdengineer Your proposal will fix the issue but I also feel same what @bernhardoj mentioned here that we're patching up the issue not fixing the root cause, instead we should fix the pusher data where lastVisibleActionCreated is updated that is less then or equal to lastReadTime

@sakluger Here we have to do 2 things:

  1. Assign an internal engineer to the issue so that they can investigate the Pusher issue in backend and fix that first
  2. Once Pusher issue will be fixed from the backend then we can assign @bernhardoj, so that they can revert the code of this PR

@melvin-bot melvin-bot bot added the Overdue label Apr 1, 2024
Copy link

melvin-bot bot commented Apr 1, 2024

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

@jayeshmangwani

This comment was marked as outdated.

@melvin-bot melvin-bot bot removed the Overdue label Apr 2, 2024
@jayeshmangwani
Copy link
Contributor

@bernhardoj's Proposal looks good to me. First, we have to revert the #36950, And then we need to fix from the backend of lastVisibleActionCreated data for the #announce room and Workspace Chat

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 2, 2024

Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@jayeshmangwani
Copy link
Contributor

@Julesssss , this is a summary of the selected proposal; we have added this code in this PR to fix this issue, #36013 was causing because of lastVisibleActionCreated data does not update correctly from the backend; That PR caused this issue; we should fix the lastVisibleActionCreated from the backend(Pusher) when we delete the message from the #announce room and workspace chat and should remove !!report.lastActorAccountID condition from setting the isUnread

@Julesssss
Copy link
Contributor

Thanks. The plan sounds good.

@sakluger FYI we'll need to get this attached to collect/control to keep this moving internally. It feels pretty minor and likely won't be in my list of priorities for a while

@sakluger
Copy link
Contributor

sakluger commented Apr 3, 2024

@Julesssss I added to wave-collect as polish. It seems appropriate for that wave because requesting money is a common collect action.

@Julesssss Julesssss removed their assignment Apr 4, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

@sakluger, @jayeshmangwani Eep! 4 days overdue now. Issues have feelings too...

@sakluger
Copy link
Contributor

sakluger commented Apr 9, 2024

@Julesssss are you able to review the proposal that @jayeshmangwani selected and give it a 👍 or 👎? Even if it's minor, I'd like to keep it moving forward since we have a contributor ready to fix it.

@Julesssss
Copy link
Contributor

Hey @sakluger, @jayeshmangwani's plan is good, but I don't want to approve it yet. It could be a while before the backend fix is applied and by then the front-end solution might not be relevant.

I will get to this eventually, but it could be a month. I'm also happy to unassign and we can find someone else with more time. but the backend work needs to be completed before we approve the proposal.

@melvin-bot melvin-bot bot added the Overdue label Apr 12, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

@sakluger, @Julesssss, @jayeshmangwani Huh... This is 4 days overdue. Who can take care of this?

@sakluger
Copy link
Contributor

Ah, thanks for clarifying @Julesssss, I didn't understand that the proposal was held on BE work. I'll move this to Monthly priority for now.

@melvin-bot melvin-bot bot removed the Overdue label Apr 16, 2024
@sakluger sakluger added Monthly KSv2 and removed Daily KSv2 labels Apr 16, 2024
@sakluger sakluger changed the title [$500] Workspace chats does not show as bold for the new expense report requests [HOLD for BE work][$500] Workspace chats does not show as bold for the new expense report requests Apr 16, 2024
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@sakluger
Copy link
Contributor

sakluger commented May 2, 2024

I asked in the original bug report thread if the bug is still occurring. If not, we can close.

@sakluger
Copy link
Contributor

sakluger commented May 2, 2024

@puneetlath (who reported the issue originally) says that this is still reproduceable. He said:

It seems that if there is a chat message and you mark it as unread, then it does show up as bold, but if it's just a money request, and you mark that as unread, it doesn't show up as bold.

@sakluger
Copy link
Contributor

No update

@Julesssss
Copy link
Contributor

This is unlikely to get fixed for a while TBH. Too many internal high-priority projects

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 8, 2024
@mallenexpensify mallenexpensify changed the title [HOLD for BE work][$500] Workspace chats does not show as bold for the new expense report requests [HOLD for BE work] Workspace chats does not show as bold for the new expense report requests Jul 8, 2024
@mallenexpensify
Copy link
Contributor

Trying to find a volunteer in  #wave-collect
https://expensify.slack.com/archives/C036QM0SLJK/p1720482169207219

@mallenexpensify
Copy link
Contributor

Closing per internal discussion, Puneet will reopen if the bugs reoccur

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 Monthly KSv2
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants