[HOLD for payment 2023-02-24] [$2000] Chat -If you mark a message as unread, then delete it -"New messages" button does not dissapear

[kbecciv]

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 staging.new.expensify.com
2. Log i with any account
3. Navigate to a 1:1 conversation that has a large history of messages
4. Press and hold/hover over your message and mark the message as "Unread"
5. Delete marked message
6. Scroll the conversation a little

Expected Result:

The Green line should not be shown after deleting a marked message, also the "New messages" button at the top of the page when scrolling.

Actual Result:

If you mark a message as unread and then delete it, a "New messages" button does not disappear. Or if you delete messages not at the end but in the middle of the chat, the green line does not disappear.

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,2,61,0

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

Bug5915033_Recording__259.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01418691c14f3167db
• Upwork Job ID: 1621665339139534848
• Last Price Increase: 2023-02-11

[mateocole]

@NikkiWines is this something we will want to work on fixing internally?

[NikkiWines]

Hmm, yeah I'm not sure what the intended behavior is here to be honest. Not sure if we'd consider this part of the same issue (or an issue at all) but the line and banner seem to persist even if you don't delete the message and scroll past the message marked as unread. cc: @roryabraham tagging you cause slack history shows you've mentioned the unread indicator a couple of times

Screen.Recording.2023-02-01.at.13.04.44.mov

[roryabraham]

Trying my best to clarify the expected behavior here:

• If you mark a comment as unread, then generally the only thing that should make that comment unread or cause the new message indicator to disappear is if you leave the report and come back.
• If you mark a comment as unread, then delete it, the expected behavior depends:
  • If the comment you marked as unread then deleted is the last reportAction in a report, then the report should go back to the read state, and the new message indicator should disappear
  • If the comment you marked as unread then deleted had reportActions after it in the report, then the report should remain in the unread state, and the new message indicator should appear on the next-oldest reportAction.

Does that make sense?

[roryabraham]

So @NikkiWines what you've shown in this screenshot is expected. 👍🏼

[roryabraham]

Looking closer at the video in the OP, it looks like this is happening as expected:

1. Mark an older comment as unread
2. Delete that comment

Expected behavior: The report remains unread and the new message indicator appears on the next-oldest reportAction ✅

However, this is not happening as expected:

Steps to reproduce:

1. Send a message in a chat
2. Mark it as unread
3. Delete it.

Expected behavior

• The report should go back to the read state
• The new message indicator should disappear
• If you scroll up, then the "new message" pill should not be present.

Actual behavior

• The report goes back to the read state ✅
• The new message indicator disappears ✅
• If you scroll up, then the "new message" pill is visible ❌

[NikkiWines]

Awesome, thanks for clarifying @roryabraham 🙌 From what you've written it sounds like there is still something to fix here and that the issue can be External.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01418691c14f3167db

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

This is because we're missing a case when deciding whether to update the newMarkerReportActionID, when a comment was deleted.

When the report was unread then the unread message is deleted, the report will become read, but we're only recalculating the newMarkerReportActionID when the report is unread, thus missing this case.

The fix is to remove that condition.

diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js
index 4bbeaa997a..87a81ab1a0 100755
--- a/src/pages/home/report/ReportActionsView.js
+++ b/src/pages/home/report/ReportActionsView.js
@@ -207,7 +207,7 @@ class ReportActionsView extends React.Component {
 
         // If the report is unread, we want to check if the number of actions has decreased. If so, then it seems that one of them was deleted. In this case, if the deleted action was the
         // one marking the unread point, we need to recalculate which action should be the unread marker.
-        if (ReportUtils.isUnread(this.props.report) && ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.sortedAndFilteredReportActions.length) {
+        if (ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.sortedAndFilteredReportActions.length) {
             this.setState({newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions)});
         }
 

[tienifr]

Proposal 2

We can add the condition to only run the update when there's a current newMarkerReportActionID, to minimize the time the code run.

diff --git a/src/pages/home/report/ReportActionsView.js b/src/pages/home/report/ReportActionsView.js
index 4bbeaa997a..87a81ab1a0 100755
--- a/src/pages/home/report/ReportActionsView.js
+++ b/src/pages/home/report/ReportActionsView.js
@@ -207,7 +207,7 @@ class ReportActionsView extends React.Component {
 
         // If the report is unread, we want to check if the number of actions has decreased. If so, then it seems that one of them was deleted. In this case, if the deleted action was the
         // one marking the unread point, we need to recalculate which action should be the unread marker.
-        if (ReportUtils.isUnread(this.props.report) && ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.sortedAndFilteredReportActions.length) {
+        if (this.state.newMarkerReportActionID && ReportActionsUtils.filterReportActionsForDisplay(prevProps.reportActions).length > this.sortedAndFilteredReportActions.length) {
             this.setState({newMarkerReportActionID: ReportUtils.getNewMarkerReportActionID(this.props.report, this.sortedAndFilteredReportActions)});
         }
 

There could be a number of optimization to be done on this to minimize the trigger time for the setState that we might choose to add, like we filter out what reportAction was deleted and only trigger if that reportAction matches the newMarkerReportActionID. But the setState is not a huge operation so we might not want to make it too complicated. In any case the approach to fix it is the same. If there's interest to optimize further I'd be happy to add as well.

[anupp-11]

PROPOSAL

If the report is marked as unread and then deleted, it would still show as having a new message even though the report is no longer present. To fix this issue, we could add a check to see if the report still exists before trying to access its properties and actions. If the report has been deleted, you could reset the state of the component.

[tienifr]

Both proposal 1 and proposal 2 working well after the fix:

Screen.Recording.2023-02-04.at.11.37.10.mp4

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.73-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Recalculate new marker indicator if current one is deleted #15144

If no regressions arise, payment will be issued on 2023-02-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@thesahindia / @neil-marcellini] The PR that introduced the bug has been identified. Link to the PR:
• [@thesahindia / @neil-marcellini] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@thesahindia / @neil-marcellini] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Propose regression test steps to ensure the same bug will not reach production again.
• [@mateocole] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

Regression Test Proposal

1. Login to any account
2. Go to any chat
3. Send a message
4. Mark that message as unread
5. Delete that message
6. Scroll up
7. Verify that you don't see the "New messages" button at the top

[neil-marcellini]

Regarding the first 3 checklist items, I think this bug may have been expected after the project to deprecate sequence numbers, so I don't think it's worth tracking down the PR and commenting there. I also don't think we need to raise a discussion because we expected some small bugs after such a large refactor.

@s77rt I think the regression test looks good. @mateocole would you please create an issue to add the regression test and handle payment?

[neil-marcellini]

@mateocole bump

[neil-marcellini]

@mateocole bump again

[neil-marcellini]

Bumped again in Slack. I also asked if someone can take over.

[mateocole]

Alright offers sent!

[thesahindia]

Accepted, thanks!

[mateocole]

Okay, both payments have been sent

[thesahindia]

@mateocole, it is also eligible for the 50% bonus.

[mateocole]

got it one sec

[mateocole]

50% Bonus sent