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

[Regression in PR][$250] Clicking on "New Messages" jumps to newest messages, skipping over any messages marked unread report by @chauchausoup #11584

Closed
kavimuru opened this issue Oct 4, 2022 · 66 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff

Comments

@kavimuru
Copy link

kavimuru commented Oct 4, 2022

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. Go to any chat
  2. Click at Mark as Unread
  3. Click on New Messages button

Expected Result:

Clicking "new messages" should jump directly to the message marked as Unread

Actual Result:

Clicking "new messages" jumps to the bottom of the message thread, skipping over the message marked as unread

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.11-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/193866710-b8402785-759a-4f1f-aa86-80b0878cb3c9.mov
https://user-images.githubusercontent.com/43996225/193866742-10e455e5-acd7-4d2d-862a-b61a85c3e9c5.mp4
Expensify/Expensify Issue URL:
Issue reported by: @chauchausoup
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664725500869019
View all open jobs on GitHub

@kavimuru kavimuru added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Oct 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2022

Triggered auto assignment to @greg-schroeder (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 4, 2022
@greg-schroeder greg-schroeder changed the title Bug: New Message logic is ambiguous reported by @chauchausoup Clicking on "New Messages" jumps to newest messages, skipping over any messages marked unread report by @chauchausoup Oct 4, 2022
@greg-schroeder
Copy link
Contributor

greg-schroeder commented Oct 4, 2022

I reproduced this on the desktop app:

2022-10-04_14-57-19.mp4

Adjusted some of the OP to make it clearer

@greg-schroeder greg-schroeder added Engineering Improvement Item broken or needs improvement. labels Oct 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2022

Triggered auto assignment to @srikarparsi (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@srikarparsi srikarparsi added Weekly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Oct 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2022

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2022

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

@melvin-bot melvin-bot bot changed the title Clicking on "New Messages" jumps to newest messages, skipping over any messages marked unread report by @chauchausoup [$250] Clicking on "New Messages" jumps to newest messages, skipping over any messages marked unread report by @chauchausoup Oct 6, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 10, 2022

@conorpendergrast, @eVoloshchak, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Oct 10, 2022
@conorpendergrast
Copy link
Contributor

Oops, I'll get to this today, sorry!

@melvin-bot melvin-bot bot removed the Overdue label Oct 10, 2022
@JmillsExpensify
Copy link

This is just my opinion, but to me that's worse than any bug. It signals that we're no longer in control and likely need to change something about our process.

I agree with @marcaaron here. @conorpendergrast @srikarparsi I think we need to treat this an a top priority for today and get the RCA published. We've got to figure out how we avoid this in the future, and especially changing defined/designed behaviors in the app.

@conorpendergrast
Copy link
Contributor

Before we do that, let's agree the behaviour we expect. Discussing that in Slack now!

@conorpendergrast
Copy link
Contributor

conorpendergrast commented Nov 30, 2022

Based on the Slack discussion and the original design (Expensify internal doc link), I agree that I triaged this incorrectly and identified it as a bug when it was not a bug, and we should revert with an RCA 👍

This is a “feature bug” (I disagree with the design) not a “bug” (the design is working as expected), because the function of the new messages button is:

Tapping it will bring you to the bottom of the chat

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2022
@conorpendergrast
Copy link
Contributor

Reversion PR is here: #13230
And @srikarparsi is working on an RCA!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 5, 2022
@conorpendergrast
Copy link
Contributor

Scroll up, Melv

@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 8, 2022

@conorpendergrast, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Dec 8, 2022
@srikarparsi
Copy link
Contributor

Issue reverted here: #13230

Will close after the PR gets deployed and I respond to @conorpendergrast's comments in the RCA

@melvin-bot melvin-bot bot removed the Overdue label Dec 8, 2022
@conorpendergrast conorpendergrast added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@conorpendergrast
Copy link
Contributor

I'm re-assigning to another BugZero team member in case there are any further steps, as I'm going on sabbatical in 30 minutes!

@conorpendergrast conorpendergrast removed their assignment Dec 9, 2022
@melvin-bot melvin-bot bot added the Overdue label Dec 12, 2022
@srikarparsi
Copy link
Contributor

PR to address one change in the RCA: #13518
Will bring up the other proposed changes from the RCA in slack

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 12, 2022
@trjExpensify
Copy link
Contributor

👋 Hey everyone! Can you give me an update on why this issue is still open, and what we need to do to close it out? I'm trying to piece it together:

  1. The PR to revert the change has been merged
  2. The linked PR with some action from the RCA has been closed add comment to explain scroll to bottom #13518
  3. Potentially something about other proposed changed from the RCA are in slack, but not sure where that is or what (if any) actions we need to take from that?

Thanks!

@melvin-bot
Copy link

melvin-bot bot commented Dec 15, 2022

@tjferriss, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

@srikarparsi
Copy link
Contributor

hey @trjExpensify! I think this issue is ready to be closed out, the PR has been reverted, the RCA has been completed, and I'm talking to marc about the proposed changes in the RCA but I believe that can be continued with this issue closed. @tjferriss @tjferriss please reopen it though if there's something that I'm missing.

@melvin-bot melvin-bot bot removed the Overdue label Dec 18, 2022
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. Daily KSv2 Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests