App icon indicating copy to clipboard operation
App copied to clipboard

[$1000] Chat - The group chat status does not show unread after the user goes online

Open kbecciv opened this issue 1 year ago • 18 comments

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:

  1. Open network tab on Account A and switch to offline mode
  2. Open a group chat and send a message on Account A
  3. Go to Account B and open the same group chat with Account A and send multiple messages
  4. Go back to Account A and open any chat other than the previous group chat
  5. Turn off offline mode
  6. Check the group chat on LHN

Expected Result:

The group chat should show unread status since there are new messages sent in the group while Account A was offline

Actual Result:

The group chat does not show unread status and when opening the group chat the new messages are visible.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • [x] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [x] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop

Version Number: 1.3.2.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

https://user-images.githubusercontent.com/93399543/233205950-860970fb-2d10-409e-8d0e-65ef6ce09497.mp4

https://user-images.githubusercontent.com/93399543/233206009-a92dead4-324b-45a6-8ad1-95d7134968ac.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Nathan-Mulugeta

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681902462599969

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01fbfa46985f3e9ea0
  • Upwork Job ID: 1650478391345414144
  • Last Price Increase: 2023-04-24

kbecciv avatar Apr 19 '23 21:04 kbecciv

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

MelvinBot avatar Apr 19 '23 21:04 MelvinBot

Bug0 Triage Checklist (Main S/O)

  • [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [x] 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
  • [x] 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.
  • [x] 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.
  • [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot avatar Apr 19 '23 21:04 MelvinBot

Looks good!

slafortune avatar Apr 24 '23 12:04 slafortune

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

MelvinBot avatar Apr 24 '23 12:04 MelvinBot

Triggered auto assignment to @miljakljajic (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

MelvinBot avatar Apr 24 '23 12:04 MelvinBot

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

MelvinBot avatar Apr 24 '23 12:04 MelvinBot

Triggered auto assignment to @neil-marcellini (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

MelvinBot avatar Apr 24 '23 12:04 MelvinBot

Proposal

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

When a user sends a message in a group chat while offline and receives new messages in the same chat during that time, the group chat status does not show unread after the user goes online

What is the root cause of that problem?

So basically this is what's happening:

  1. Device A(offline) sends a message, request is added into queue
  2. Device B(online) sends a message, goes to backend
  3. When device A comes online, it receives updates which include message from device B.
  4. Requests in queue in device A gets sent, backend pushes update with lastReadTime based on current request which is after device B's message

And you can see that, offline sent messages are ordered after messages that sent online

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

Well there is not much we can do about it, since based on current logic, message's created time is determined by when it is received by backend rather than when it is sent by client app and lastReadTime is updated when new message is received.

One thing we can do is, to supply lastReadTime in a AddComment request manually, and when backend receives this command it will create new messsage with current time, but updates lastReadTime with value we provided. But, this might lead to get unread for their own messages

What alternative solutions did you explore? (Optional)

alitoshmatov avatar Apr 24 '23 13:04 alitoshmatov

I am very happy to post my opinion on this issue. I can show more clear idea when I see your backend workflow. And now, I have a logical idea to resolve this issue with an experience when I got on previous project. Make a notification when receive a new message from other. And implement real-time messaging to get a new message status.

vadymdev716 avatar Apr 24 '23 13:04 vadymdev716

📣 @vadymtbl! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

MelvinBot avatar Apr 24 '23 13:04 MelvinBot

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

MelvinBot avatar Apr 24 '23 13:04 MelvinBot

⚠️ Invalid email. Please make sure to create an Expensify account with this email first here.

MelvinBot avatar Apr 24 '23 13:04 MelvinBot

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01394546b94c94ce62

vadymdev716 avatar Apr 24 '23 13:04 vadymdev716

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

MelvinBot avatar Apr 24 '23 13:04 MelvinBot

@kbecciv Wow that's quite a title 😆 would you please write a shorter one?

neil-marcellini avatar Apr 24 '23 16:04 neil-marcellini

@neil-marcellini Done.

kbecciv avatar Apr 24 '23 16:04 kbecciv

@neil-marcellini What are your thoughts on this proposal, which suggests implementing a backend fix?

I have observed that the AddComment command is queued before the OpenReport command when returning from offline mode, which seems to align with @alitoshmatov's explanation.

Screenshot 2023-04-27 at 1 13 37 AM

Also another point to mention that this internal PR https://github.com/Expensify/Web-Expensify/pull/37053 was deployed recently which fixes a similar issue https://github.com/Expensify/App/issues/17100

fedirjh avatar Apr 27 '23 01:04 fedirjh

Interesting, I think @alitoshmatov explains it well. I think we might have been aware of this problem when refactoring away from sequence numbers. I'll tag some people into the bug report thread in Slack and we can discuss potential solutions there.

neil-marcellini avatar Apr 27 '23 17:04 neil-marcellini

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

MelvinBot avatar May 01 '23 16:05 MelvinBot

After some discussion in Slack we decided that this will require a backend fix. I'll make this internal and either fix it myself or find someone how can take it over.

neil-marcellini avatar May 01 '23 17:05 neil-marcellini

Current assignee @fedirjh is eligible for the Internal assigner, not assigning anyone new.

MelvinBot avatar May 01 '23 17:05 MelvinBot

I have a draft PR up and I wrote a failing test for this bug. Next time I should be able to write the fix and put it up for review.

neil-marcellini avatar May 02 '23 01:05 neil-marcellini

I only had a bit of time for this yesterday. I made a little progress.

neil-marcellini avatar May 03 '23 17:05 neil-marcellini

@fedirjh I'm going to remove your assignment here because the fix is likely going to be on the backend only. If we need some App changes I'll re-assign you for a review.

neil-marcellini avatar May 03 '23 17:05 neil-marcellini

@slafortune @neil-marcellini this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

MelvinBot avatar May 03 '23 20:05 MelvinBot

Yes we're close to a solution more or less. This is one of my top priorities, but at the same time it's a bit of an edge case so it's not super important.

I got my integration test passing, but I'm going to have to make some other changes to actually get the flow working on NewDot. When the user comes back online they now see the group chat as unread, which is good progress, but the unread marker isn't in quite the right place yet. There are several API commands called when going back online so I'll have to make sure all of them are sending the proper lastReadTime in this case.

neil-marcellini avatar May 03 '23 23:05 neil-marcellini

I updated my test to make it have it include all the API commands that get called when the user goes back online and adds the comment. So the test is very close to reality now. I think I'll be able to get it up for review with a few more hours of work.

neil-marcellini avatar May 05 '23 00:05 neil-marcellini

@slafortune, @neil-marcellini Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar May 08 '23 20:05 melvin-bot[bot]

I'm not able to work on this today. I had many PRs to review and I'm focused on distance requests. I'll see if I can get to it tomorrow.

neil-marcellini avatar May 08 '23 22:05 neil-marcellini

Similar story today, oh well 🤷‍♂️

neil-marcellini avatar May 09 '23 22:05 neil-marcellini