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] LHN - Announce room on invited workspace member is not bold #35122

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 25, 2024 · 56 comments
Closed
1 of 6 tasks

[$500] LHN - Announce room on invited workspace member is not bold #35122

lanitochka17 opened this issue Jan 25, 2024 · 56 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 25, 2024

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


Version Number: 1.4.31-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: Applause - Internal Team
Slack conversation:

Action Performed:

  1. User A create workspace
  2. User A navigates to announce room> settings> set welcome message
  3. User A goes to Settings> Workspace> Members> Invite User B
  4. Login as User B

Expected Result:

Announce room on invited workspace member should be bold on LHN

Actual Result:

Announce room of invited workspace member is not bold on LHN, even green line on welcome message appears
Also, welcome message does not change on employee side when admin change the content of welcome message

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

Add any screenshot/video evidence

Bug6354010_1706142902654.Recording__1915.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016aee40f12650c4a6
  • Upwork Job ID: 1750329685857648640
  • Last Price Increase: 2024-02-15
  • Automatic offers:
    • situchan | Contributor | 28129326
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 25, 2024
@melvin-bot melvin-bot bot changed the title LHN - Announce room on invited workspace member is not bold [$500] LHN - Announce room on invited workspace member is not bold Jan 25, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016aee40f12650c4a6

Copy link

melvin-bot bot commented Jan 25, 2024

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

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

melvin-bot bot commented Jan 25, 2024

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

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Jan 25, 2024

Proposal

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

New workspaces for the user - does not show up as unread workspaces in LHN. They do get a notification.

What is the root cause of that problem?

Screenshot 2024-01-24 at 8 55 15 PM We currently create new reports with the same timestamp for lastRead and lastVisibleActionCreated

We only show something as unread if the last read time is before the last visible is created here:

return lastReadTime < lastVisibleActionCreated;

And the announced message, is merely metadata on the report. Not an actual message.

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

We should actually show all new channels as unread - this improves the customer experience by showing there's something new. Even if there are no messages in that channel.

We can make a small change here

return lastReadTime < lastVisibleActionCreated;
to also be unread if the lastRead equals lastVisibleActionCreated, indicating the newly created report condition.

Once a user actually clicks on the report - it will be marked as read as the lastRead timestamp updates:

Sample experience with new code:
https://github.com/Expensify/App/assets/157416545/b615bf89-7f34-4fe6-86fa-3579a5638c0f

This also covers the announce use case in this issue.

What alternative solutions did you explore? (Optional)

@dragnoir
Copy link
Contributor

I think it should be fixed here #31748

@Santhosh-Sellavel
Copy link
Collaborator

I think it should be fixed here #31748

@dragnoir Can you elaborate on why?
Is that regression from #31748?

@dragnoir
Copy link
Contributor

@Santhosh-Sellavel check this #33881
it's the same issue here and the same problem. The suggested solution is the same in boath #33881 and #31748 but the issue is always there. The solution is not complete and need to handle all the UNREAD cases.

PR update: https://github.com/Expensify/App/pull/34551/files
Second issue proposal: #33881 (comment)

c+ note from the same issue #33881 (comment) and #33881 (comment)

but the PR submitted there #34551 is not tested well.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jan 27, 2024

Waiting for feedback #31748 (comment)

I'm unavailable next week, Please assign a new C+ Issue here if required while I am away, thanks!

#31748 (comment)

cc: @lschurr

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
@lschurr
Copy link
Contributor

lschurr commented Jan 29, 2024

Asking for a volunteer in Slack - https://expensify.slack.com/archives/C01GTK53T8Q/p1706555426712459

@melvin-bot melvin-bot bot removed the Overdue label Jan 29, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 29, 2024

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

@lschurr lschurr added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 29, 2024
@lschurr
Copy link
Contributor

lschurr commented Jan 29, 2024

Assigning to @situchan - could you review?

@situchan
Copy link
Contributor

Seems like 2 bugs stated in OP. Checking reproducibility first

@situchan
Copy link
Contributor

Announce room of invited workspace member is not bold on LHN, even green line on welcome message appears

reproduced

Screenshot 2024-01-30 at 1 26 41 AM

Also, welcome message does not change on employee side when admin change the content of welcome message

not reproduced

@situchan
Copy link
Contributor

@jeremy-croff can you please explain the root cause in detail?

This has 2 bugs: (might have the same root cause though)

  • why LHN text is not bolded when login first time after getting invited?
  • why unread marker keeps showing even after read and revisit report?
Screen.Recording.2024-01-30.at.1.35.12.AM.mov

@jeremy-croff
Copy link
Contributor

@jeremy-croff can you please explain the root cause in detail?

This has 2 bugs: (might have the same root cause though)

  • why LHN text is not bolded when login first time after getting invited?
  • why unread marker keeps showing even after read and revisit report?

Screen.Recording.2024-01-30.at.1.35.12.AM.mov

While both are adressed with my fix,

I will report back soon why the unread marker keeps showing in current main.
It has to do with the logic that determines unread within the report is seperate from the LHN.

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Jan 29, 2024

@jeremy-croff can you please explain the root cause in detail?

This has 2 bugs: (might have the same root cause though)

  • why unread marker keeps showing even after read and revisit report?

We grab the last read time from the report

lastReadTimeRef.current = report.lastReadTime;

And compare the report actions against that timestamp for the unread marker in the chat. It sees the action has a later timestamp than the report read timestamp, and stores the ref.
const isCurrentMessageUnread = isMessageUnread(reportAction, lastReadTimeRef.current);

Then when going over the chats, it marks it as unread here

shouldDisplay = reportAction.reportActionID === currentUnreadMarker;

the welcome message has a later timestamp.

Following screenshots show this:
Screenshot 2024-01-29 at 4 21 22 PM

Screenshot 2024-01-29 at 4 19 15 PM

  • why LHN text is not bolded when login first time after getting invited?

This flow is different from the the chat unread unread.
LHN logic only looks at report:

return lastReadTime < lastVisibleActionCreated;

Where we just compare the timestamps from the report.
And as demonstrated are the same in this scenario.

Fixing the issue with my logic adresses both, as it will update both report and item timestamps.

@situchan
Copy link
Contributor

updating in a few hrs

@situchan
Copy link
Contributor

Ideally, this should be fixed in backend.
So report.lastVisibleActionCreated should be whisperAction.created when last visible action is whisperAction.
Currently, whisper actions are not considered as visible action in backend when calculate report.lastVisibleActionCreated.

If we wanna do frontend fix, @eh2077's proposal looks good.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 27, 2024

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

@situchan
Copy link
Contributor

I think this bug came from the fact that whisper action should not be visible in LHN as last message.

@thienlnam
Copy link
Contributor

Ah interesting - thanks for the summary. We should fix this in the BE - though the query for determining visible report actions is quite complex. Not an urgent bug so going to change the priority

@thienlnam thienlnam added Weekly KSv2 and removed Daily KSv2 labels Feb 28, 2024
@melvin-bot melvin-bot bot added the Overdue label Mar 7, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 7, 2024

Any update here @thienlnam?

@melvin-bot melvin-bot bot removed the Overdue label Mar 7, 2024
@thienlnam
Copy link
Contributor

I'm not currently working on this issue over other vip-vsb items, perhaps we can tie it to a wave and see if anyone is interested?

@melvin-bot melvin-bot bot added the Overdue label Mar 18, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 18, 2024

Ok, got it. I'll review this later today.

@melvin-bot melvin-bot bot removed the Overdue label Mar 18, 2024
@lschurr
Copy link
Contributor

lschurr commented Mar 19, 2024

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

lschurr commented Apr 2, 2024

Asking for a volunteer in Slack here: https://expensify.slack.com/archives/C066HJM2CAZ/p1712079432372279

@melvin-bot melvin-bot bot removed the Overdue label Apr 2, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 10, 2024
@lschurr
Copy link
Contributor

lschurr commented Apr 10, 2024

Haven't heard anything in the Slack thread - ideas on this one @thienlnam?

@melvin-bot melvin-bot bot removed the Overdue label Apr 10, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 19, 2024
@lschurr
Copy link
Contributor

lschurr commented Apr 22, 2024

Anything on this one @thienlnam? Are we cool leaving this unassigned since it's a low priority?

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
@thienlnam
Copy link
Contributor

Nope, hasn't been a focus - I think it's fine to leave unassigned or even close for other priorities at this time

@melvin-bot melvin-bot bot added the Overdue label Apr 30, 2024
@lschurr
Copy link
Contributor

lschurr commented Apr 30, 2024

Going to close this as not a priority for now.

@lschurr lschurr closed this as completed Apr 30, 2024
@melvin-bot melvin-bot bot removed the Overdue label Apr 30, 2024
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. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

8 participants