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 #15212][$1000] Web - New marker does not go away when Report is automatically marked as read #21875

Closed
1 of 6 tasks
kbecciv opened this issue Jun 29, 2023 · 52 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 Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Jun 29, 2023

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 a chat with history;
  2. Mark any message as unread;
  3. Refresh the page OR switch the tab and come back;
  4. On left, see that the report has been marked as read, but the New marker is still visible;
  5. Repeat Step 3;
  6. See that the New marker did go away the second time.

Expected Result:

The New marker should go away the first time.

Actual Result:

The New marker goes away the second time.

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.33-4
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
Notes/Photos/Videos: Any additional supporting documentation

marker_twice.mp4
Recording.3310.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0107af640eb3758e2b
  • Upwork Job ID: 1675894728590426112
  • Last Price Increase: 2023-07-03
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 29, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 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

@kbecciv
Copy link
Author

kbecciv commented Jun 29, 2023

Proposal

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

New marker does not go away when Report is automatically marked as read

What is the root cause of that problem?

  1. In ReportActionsView.js, if we switch the chrome tab and come back, or refresh the current tab, the report's lastReadTime updates to a new value;
  2. In ReportUtils.js, the method isUnread does return lastReadTime < lastVisibleActionCreated, so it would retun false;
  3. But, in ReportActionsView.js inside componentDidUpdate, we do not check if the report automatically became read in order to clear this.state.newMarkerReportActionID.

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

We should add this logic:

if (!ReportUtils.isUnread(this.props.report) && this.state.newMarkerReportActionID != '') {
    this.setState({
        newMarkerReportActionID: '',
    });
}

inside componentDidUpdate in ReportActionsView.js.

@mollfpr
Copy link
Contributor

mollfpr commented Jun 29, 2023

@sonialiap please hold triaging this. I will try investigating this, maybe can be solved by #21557.

@mollfpr
Copy link
Contributor

mollfpr commented Jun 30, 2023

@sonialiap It's okay now to triage this. We have different root causes for the issue in the PR above. Thank you!

@melvin-bot melvin-bot bot added the Overdue label Jul 3, 2023
@sonialiap
Copy link
Contributor

The unread line remains for me in both scenarios. Maybe it's some general delay with the page updating?

Screen.Recording.2023-07-03.at.5.48.05.PM.mov

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2023
@sonialiap sonialiap added the External Added to denote the issue can be worked on by a contributor label Jul 3, 2023
@melvin-bot melvin-bot bot changed the title Web - New marker does not go away when Report is automatically marked as read [$1000] Web - New marker does not go away when Report is automatically marked as read Jul 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0107af640eb3758e2b

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

melvin-bot bot commented Jul 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

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

@fedirjh
Copy link
Contributor

fedirjh commented Jul 3, 2023

@sonialiap I think this should be hold for #15212

@hwakstar
Copy link

hwakstar commented Jul 3, 2023

Hello
I checked your job description and I am sure that can solve it.
I hope that work with you.
Warm regards

@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

📣 @hwakstar! 📣
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>

@DanutGavrus
Copy link
Contributor

@fedirjh Should we ask on Slack about holding this one for 15212?

I'm asking as 20759 was recently completed instead of also being held and seems related.

Thanks!

@coeur85
Copy link

coeur85 commented Jul 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 3, 2023

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

@fedirjh
Copy link
Contributor

fedirjh commented Jul 4, 2023

I'm asking as #20759 was recently completed instead of also being held and seems related.

There is an tracker issue for all unread marker issue, and all related issues, should be added to it. It doesn’t make sense to proceed with solution while we are working on a refactor.

@DanutGavrus
Copy link
Contributor

DanutGavrus commented Jul 4, 2023

There is an tracker issue for all unread marker issue, and all related issues, should be added to it. It doesn’t make sense to proceed with solution while we are working on a refactor.

Ok, I understand, thanks!

@hwakstar
Copy link

hwakstar commented Jul 4, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2023

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

@hwakstar
Copy link

hwakstar commented Jul 4, 2023

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

@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@sonialiap sonialiap reopened this Oct 23, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 31, 2023
@sonialiap
Copy link
Contributor

PR for #15212 is super close to being approved and merged, exciting! Hopefully soon we'll be able to re-test this one and either close or look for proposals

@melvin-bot melvin-bot bot removed the Overdue label Oct 31, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 9, 2023
@sonialiap
Copy link
Contributor

PR for #15212 is close but not yet merged. Waiting

@melvin-bot melvin-bot bot removed the Overdue label Nov 10, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2023
@sonialiap
Copy link
Contributor

Waiting for #25935 to test this again

@melvin-bot melvin-bot bot removed the Overdue label Nov 20, 2023
@MonilBhavsar
Copy link
Contributor

Expected Result: The New marker should go away the first time.

When marking the message as unread manually, the new marker should go second time.

That said, this issue should be fixed. We can retest and most probably close it

@fedirjh
Copy link
Contributor

fedirjh commented Nov 21, 2023

When marking the message as unread manually, the new marker should go second time.

@MonilBhavsar I just retested and found that :

  • When refreshing the report, the New marker goes away the first time and the report is marked as read in LHN.
  • When switching between tabs, the new marker is not dismissed and the report is still marked as unread in LHN (displayed as bold).

is that expected?

CleanShot.2023-11-21.at.18.51.32.mp4

@melvin-bot melvin-bot bot added the Overdue label Dec 1, 2023
@sonialiap
Copy link
Contributor

@MonilBhavsar bumping Fedi's question, do you know the answer?

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2023
@MonilBhavsar
Copy link
Contributor

Sorry, missed it

When marking the message as unread manually, the new marker should go second time.

@MonilBhavsar I just retested and found that :

  • When refreshing the report, the New marker goes away the first time and the report is marked as read in LHN.
  • When switching between tabs, the new marker is not dismissed and the report is still marked as unread in LHN (displayed as bold).

is that expected?

CleanShot.2023-11-21.at.18.51.32.mp4

I would say it is expected. When refreshing the page, the entire component is remounted losing the marker state. We do not store marker data in local storage or Onyx DB, and also OpenApp and OpenReport updates the lastReadTime. To persist unread data between refresh is out of scope and the losing of marker during refresh is expected

@melvin-bot melvin-bot bot added the Overdue label Dec 13, 2023
@sonialiap
Copy link
Contributor

@fedirjh sounds like we still have an issue here and need proposals, is that correct?

@melvin-bot melvin-bot bot removed the Overdue label Dec 14, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Dec 14, 2023

sounds like we still have an issue here and need proposals, is that correct?

@sonialiap I think refreshing the report marks the report as unread , that's expected and that fine. Additionally, I would expect that switching between tabs should also mark the report as read. What are your thoughts @MonilBhavsar ?

If we should not mark the report as unread when switching between tabs (or when the app goes into the background), then I believe everything is working as expected, and we can consider closing this issue.

@MonilBhavsar
Copy link
Contributor

As per the design, we should not remove marker when user switches the tab. For the refresh I think it is fine as the components are remounted.

@fedirjh
Copy link
Contributor

fedirjh commented Dec 18, 2023

That looks good , I think this can be closed.

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2023
@fedirjh
Copy link
Contributor

fedirjh commented Dec 28, 2023

In case a regression test is required :

Regression Test Proposal

  1. Open a chat with history
  2. Mark any message as unread
  3. Refresh the page
  4. Verify that the report has been marked as read on LHN and New marker did go away
  5. Repeat Step 2
  6. Switch the tab and come back
  7. Verify that the report is still marked as unread on LHN and New marker is still visible inside the report
  • Do we agree 👍 or 👎

@melvin-bot melvin-bot bot removed the Overdue label Dec 28, 2023
@melvin-bot melvin-bot bot added the Overdue label Jan 5, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 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. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants