[HOLD for payment 2024-06-13] [$250] Chat - Unread message marker disappeared for next message if user stays in chat

[lanitochka17]

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.61-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/cases/view/2990295
Issue reported by: Applause - Internal Team

Action Performed:

Preconditions; Two users logged in. User A is in another chat

1. User B send message to User A
2. Chat between users marked as unread (for user A)
3. User A navigates to chat with user B
4. User B send another message

Expected Result:

• After step 3, user A sees a new message marker with a green line above first message from user B. The new marker line is displayed over the original unread message
• After step 4, the new marker line remains, as messages come through. It won’t go away unless user switches to another chat.

Actual Result:

After step 4, a new message marker disappeared

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

Bug6442589_1712597097975.unread_message_marker.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017cac380aa031e55a
• Upwork Job ID: 1792396283373867008
• Last Price Increase: 2024-05-20
• Automatic offers:
• eh2077 | Reviewer | 0
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @Christinadobrzyn

[melvin-bot]

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

[lanitochka17]

@Christinadobrzyn FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[Christinadobrzyn]

Asking qa team for some clarification on how they are getting the unread line - https://expensify.slack.com/archives/C9YU7BX5M/p1712624387659049

[tienifr]

Proposal

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

a new message marker persist and it's still displayed over the original unread message

What is the root cause of that problem?

We have the logic to call readNewestAction API in

App/src/pages/home/report/ReportActionsList.tsx

Lines 248 to 251 in 4f67357

	if (ReportUtils.isUnread(report)) {
	if (Visibility.isVisible() && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
	Report.readNewestAction(report.reportID);
	} else {

This logic is triggered when there's new message comes in (lastVisibleActionCreated get changed), and the report is unread.

After calling we reset unread indicator here

App/src/libs/actions/Report.ts

Line 995 in 4f67357

DeviceEventEmitter.emit(`readNewestAction_${reportID}`, lastReadTime);

so the unread indicator disappear

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

This logic is added to reset unread indicator when users mark as read manually so we need to create the flag(shouldResetUnread) with default value is false and pass it true in here

Finally, reset unread indicator only if shouldResetUnread is true

What alternative solutions did you explore? (Optional)

NA

Result

web-resize.mp4

[Christinadobrzyn]

@tienifr do you know what the issue is here? I got this second video from QA but I still think this is behaving as expected. I don't understand what we need to change here...

green.line.repro.mp4

[Christinadobrzyn]

Asking the Vip-vsb team for a buddy check if this is a bug - https://expensify.slack.com/archives/C066HJM2CAZ/p1712802929200679

[tienifr]

@Christinadobrzyn I think if users are in the chat, and then there's new message come in, we shouldn't remove the unread marker. Users may be uncomfortable if it's removed while they're reading the older message

[Christinadobrzyn]

Based on the convo in slack it sounds like the behaviour in the video is behaving as expected.

@tienifr I agree that we could come back to the 'unread' behaviour changes as a feature request but for the time being, this doesn't appear to be a bug so closing.

[MonilBhavsar]

Commented in the thread because I feel like we should fix this

[Christinadobrzyn]

Ah thanks Monil, I updated the expected results in the OP of this GH based on the convo here. I think this can be external?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017cac380aa031e55a

[melvin-bot]

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

[MonilBhavsar]

Thanks Christina I'm self assigning myself

[MonilBhavsar]

@tienifr many things might have changed since your proposal. Do you want to take a look and share updated proposal?

[tienifr]

@MonilBhavsar @eh2077 I tested my proposal and it worked well without any updates

[MonilBhavsar]

Okay cool! Thanks for confirming. Since we're updating root logic, can we please make sure to test all cases to make sure we don't break it again

Tests

1. Different chat open:

• Given a chat report between user A and user B, and user A and user C, and user A has chat view with user B open
• When user C sends message to user A, and user A switches to chat view with user C
• Then user A should see the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.

2. Same chat open(no unread message):

• Given a chat report between user A and user B, and user A has chat view with user B open and no unread message and no unread marker
• When user B sends message to user A
• Then user A should not see any new marker line

3. Same chat open(with unread message):

• Given a chat report between user A and user B, and user A has chat view with user B open and user A has unread messages and new marker line
• When user B sends message to user A
• Then the new marker line should persist until userA switches to another chat view.

4. Same chat open(with user scrolled up):

• Given a chat report between user A and user B, and user A has chat view with user B open and has no unread messages and no new marker line
• When user A scrolls up to the old chats and user B sends message to user A
• Then user A should see the green badge with “New Messages” and the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.

5. Redirecting to chat from email notification:

• Given a chat report between user A and user B, and user A is not active on chat or offline
• When user B sends message to user A and user A receives email notification and clicks on the link in the message
• Then user A should be redirected to the chat view with user B and see the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.

6. Marking message as unread:

• Given a chat report between user A and user B, and user A is active on a chat view with user B
• When user A marks any report as unread i.e. click on “Mark as unread”
• Then user A should see the new marker line above that message, and it won’t be removed until the user revisits the chat and switches to another chat then.

7. Deleting a message(other than unread message):

• Given a chat report between user A and user B, and user A is active on a chat view with user B, and there is an unread message and unread marker
• When user B sends a new message and deletes that message (other then the message above which there is an unread marker)
• Then there should be no effect on the new marker line and user A should see the new marker line, and it won’t be removed until the user switches to another chat or the sender deletes all new messages.

8. Deleting an unread message:

• Given a chat report between user A and user B, and user A is active on a chat view with user B, and there is an unread message and unread marker
• When user B deletes a message above which there is an unread marker
• Then user A should should not see the unread marker and the new message that was deleted

9. Marking as unread and deleting a message

• Given a chat report between user A and user B
• User A sends message to user B and marks the last sent message as unread
• User A deletes the last sent message
• Then the new marker line above last message should be removed
• Now user A, sends three messages to user B - m1, m2, and m3 in order
• User A marks m2 as unread
• Then New marker line should be above m2
• Now user A deletes m2
• Then new marker line should be above next message i.e m3 in this case

[eh2077]

@MonilBhavsar Thanks for sharing rich test cases.

@tienifr 's proposal looks good. I agree with their RCA and solution. I think we can verify these tests in PR.

@MonilBhavsar All yours.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @MonilBhavsar is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[tienifr]

@eh2077 PR #42568 can be reviewed.

[Christinadobrzyn]

watching #42568

[Christinadobrzyn]

watching #42568

[melvin-bot]

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

[melvin-bot]

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

• fix Unread message marker disappeared for next message #42568

If no regressions arise, payment will be issued on 2024-06-13. 🎊

For reference, here are some details about the assignees on this issue:

• @tienifr requires payment automatic offer (Contributor)
• @eh2077 requires payment automatic offer (Reviewer)

[melvin-bot]

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:

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
• [@eh2077] 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:
• [@eh2077] 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:
• [@eh2077] Determine if we should create a regression test for this bug.
• [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@Christinadobrzyn] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Christinadobrzyn]

Payouts due:

• Contributor: $250 @tienifr (Upwork offer - https://www.upwork.com/nx/wm/workroom/37347265/overview)
• Contributor+: $250 @eh2077 (Upwork offer - https://www.upwork.com/nx/wm/workroom/37347174/overview)

Upwork job is here.

@eh2077 do we need a regression test?

[MonilBhavsar]

Just a heads up! We should correct the Testrail case that was updated previously

[Christinadobrzyn]

Just a heads up - I'm going to be ooo until June 24th so going to assign a teammate to pay this out once we have the regression test

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[Christinadobrzyn]

@puneetlath when we have a regression test, can you please pay out based on this payment summary?

@MonilBhavsar @eh2077 if I understand correctly, the new regression test is going to replace the existing one?

[MonilBhavsar]

We should correct the existing regression test with the correct expected behavior. Currently, the test is testing the incorrect behavior that marker should disappear when sending a message.

[puneetlath]

@eh2077 bump on the checklist so we can pay.

[eh2077]

Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I don't think there's a specific PR responsible for it as we adjusted the expected behaviour, see comment
• [@eh2077] 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: N/A
• [@eh2077] 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: N/A
• [@eh2077] Determine if we should create a regression test for this bug. Yes
• [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression test

1. User B send message to User A
2. Chat between users marked as unread (for user A)
3. User A navigates to chat with user B
4. User B send another message
5. Verify that after step 3, user A sees a new message marker with a green line above first message from user B. The new marker line is displayed over the original unread message
6. Verify that after step 4, the new marker line remains, as messages come through. It won’t go away unless user switches to another chat.

Note

Create or update existing regression tests as @MonilBhavsar mentioned #39871 (comment)

Do we agree 👍 or 👎

[puneetlath]

Regression test issue: https://github.com/Expensify/Expensify/issues/404987

All paid. Thanks everyone!