Remove `unreadActionCount` & Improve Global Indicators #10739

marcaaron · 2022-09-01T09:22:28Z

Details

Removes unreadActionCount in favor of boolean unread status

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/226118
$ #10768

Tests

Testing Global Unread Indicator

Start a chat between User A and User B.
Ensure User A is not looking at the chat.
Leave a comment on the chat as User B.
Verify the global indicator (app icon or browser page title) has increased by 1
Leave another comment
Verify the global indicator has not changed
As User C add another comment to a chat that User A is not looking at
Verify the global indicator increases by 1
As User A visit the chat with User B
Verify the global indicator decreases by 1
As user A visit the chat with User C
Verify the global indicator decreases by 1

Testing Unread LHN Status, “New Messages” badge, and New Line Indicator

Start a chat between User A and User B.
Ensure User A is not looking at the chat.
Send a message as User B
Verify the LHN row shows in bold for User A to indicate unread messages
Visit the chat as User A and verify the LHN row no longer shows in bold
Verify there is a “----- New” line above the message sent by User B
Scroll up and verify that the “New Messages” button appears floating above the content.
Verify that tapping this button will scroll you to the bottom of the chat but not clear the “---- New” line marker.
Mark a comment as “unread”
Verify the “---- New” line marker changes to the place above the comment marked as unread
Mark as unread comments above and below the existing new marker
Verify the marker appears in the correct position each time
While the new marker is active leave a comment as User A
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Verify the chat in the LHN shows as read
Verify the New Line marker and new messages badge is still present
Navigate away one more time and back
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Minimize the app and reopen it
Verify the new marker and new message badge both disappear
Switch to a mobile view (small screen) and mark a message as unread
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “unread”
Tap the chat
Verify the new marker and badge are present
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “read”
Tap the chat
Verify the new marker and badge are no longer present.
Scroll up in the chat so that last comment is out of view
Leave a comment as User B
Verify the “new messages badge” shows for User A
Scroll down and verify the “new line marker” shows for User A in the correct position.
Scroll back up and leave another comment as User B
Scroll down as User A
Verify the new marker is in the same position as it was before

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

…not in view at all

marcaaron · 2022-09-02T09:04:43Z

Okaiee this is ready for reviews now :D

mountiny

code looks clean, great job, going to test now!

cead22 · 2022-09-02T09:38:47Z

src/libs/UnreadIndicatorUpdater/index.js


-// Stash the unread action counts for each report
-const unreadActionCounts = {};
+// Stash the reports


Suggested change

// Stash the reports

cead22 · 2022-09-02T09:40:09Z

src/libs/UnreadIndicatorUpdater/index.js

    updateUnread(totalCount);
-}, 1000, {leading: false});
+}, 300, {leading: false});


Curious why this changed from 1000 to 300

Oh no particular reason - this is leftover from testing and I think not needed.

cead22 · 2022-09-02T09:42:19Z

src/libs/UnreadIndicatorUpdater/index.js

-        return;
-    }
-
+    const totalCount = _.reduce(reports, (total, report) => total + (ReportUtils.isUnread(report) ? 1 : 0), 0);


Suggested change

const totalCount = _.reduce(reports, (total, report) => total + (ReportUtils.isUnread(report) ? 1 : 0), 0);

const totalCount = _.filter(reports, ReportUtils.isUnread).length

NAB, just a suggestion

cead22 · 2022-09-02T09:47:14Z

src/pages/home/report/ReportActionsView.js

@@ -214,6 +212,9 @@ class ReportActionsView extends React.Component {
                } else if (this.state.newMarkerSequenceNumber === 0) {
                    this.setState({newMarkerSequenceNumber: currentLastSequenceNumber});
                }
+            } else if (this.state.newMarkerSequenceNumber === 0) {
+                // The report is not in view so we will update the newMarkerSequenceNumber


Update to the last one so nothing shows as unread?

Ah so, the condition here is:

report isn't in view

a new message appeared from someone besides the current user

no previous marker is set

So we will update the new marker with this sequenceNumber. This basically fixes a bug found by QA here: #10768

mountiny · 2022-09-02T10:19:47Z

@marcaaron Tests well 🎉

But a bit confused about these steps:

Verify that tapping this button will scroll you to the bottom of the chat but not clear the “---- New” line marker.
Mark a comment as “unread”
Verify the “---- New” line marker appears
Mark as unread comments above and below the existing new marker
Verify the marker appears in the correct position each time
While the new marker is active leave a comment as User A

So click the New Message, scorll down, dont errase the ---- New line marker. Mark comment as unread and the marker should appear? Did you mean the marker should change to the place above the comment marked as unread.

What is a bit unintuitive for me is that I mark old comment as unread and then I mark newer comment as unread but the New marker is moved to the newer comment. Should the marker not stay with the older comment marked as unread as it is technically the oldest comment "I did not read"?

cead22 · 2022-09-02T10:20:20Z

src/pages/home/report/ReportActionsView.js

@@ -214,6 +212,10 @@ class ReportActionsView extends React.Component {
                } else if (this.state.newMarkerSequenceNumber === 0) {
                    this.setState({newMarkerSequenceNumber: currentLastSequenceNumber});
                }
+            } else if (this.state.newMarkerSequenceNumber === 0) {
+                // The report is not in view and we received a comment from another user while the new marker is not set
+                // so we wil set the new marker now.


mountiny

NAB: Typo

src/pages/home/report/ReportActionsView.js

marcaaron · 2022-09-02T10:25:44Z

Mark comment as unread and the marker should appear? Did you mean the marker should change to the place above the comment marked as unread.

yes exactly this! I updated the steps to make it clearer, thanks!

What is a bit unintuitive for me is that I mark old comment as unread and then I mark newer comment as unread but the New marker is moved to the newer comment. Should the marker not stay with the older comment marked as unread as it is technically the oldest comment "I did not read"?

No, in the case of manually marking a comment as unread the user is telling us they want to manually override the unread position.

marcaaron · 2022-09-02T10:27:41Z

Updated thanks for the reviews! 🙇‍♂️

danieldoglas

LGTM

thienlnam

This has been simplified quite a bit, thanks for these changes!

mountiny

No, in the case of manually marking a comment as unread the user is telling us they want to manually override the unread position.

Coolio! Thanks

melvin-bot · 2022-09-02T11:13:42Z

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-09-02T11:18:27Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

marcaaron · 2022-09-02T11:44:37Z

Did pass the tests.

OSBotify · 2022-09-05T09:13:38Z

🚀 Deployed to staging by @marcaaron in version: 1.1.97-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

Julesssss · 2022-09-07T23:43:21Z

Tested flawlessly for me on staging. Nice work.

marcaaron added 7 commits September 1, 2022 08:42

Add isUnread() method

fee7041

Remove calculateUnreadActionCount()

2e71cf2

Remove getUnreadActionCount() method

0635b3d

Fix propTypes and various usages

a77b62f

Remove unread count tracking in global indicator

85c57f7

Remove unreadActionCount from tests

5701418

Update title on state change

211bd86

marcaaron self-assigned this Sep 1, 2022

marcaaron added 3 commits September 1, 2022 19:04

Update tests

9402317

Make sure missing report shows up in list as it is unread

f194516

Fix the tests!

5e38362

marcaaron marked this pull request as ready for review September 2, 2022 07:33

marcaaron requested a review from a team as a code owner September 2, 2022 07:33

melvin-bot bot requested review from danieldoglas and removed request for a team September 2, 2022 07:33

marcaaron added 4 commits September 2, 2022 10:17

Use constant for test

2dd3b35

Fix #10768

d54abfa

Update newMarkerSequenceNumber when new action appears and report is …

fd76532

…not in view at all

fix conflict

4b1a3db

mountiny reviewed Sep 2, 2022

View reviewed changes

cead22 previously approved these changes Sep 2, 2022

View reviewed changes

Add filter remove delay change

f6b02b4

marcaaron dismissed cead22’s stale review via f6b02b4 September 2, 2022 10:19

mountiny approved these changes Sep 2, 2022

View reviewed changes

cead22 previously approved these changes Sep 2, 2022

View reviewed changes

mountiny previously approved these changes Sep 2, 2022

View reviewed changes

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

fix typo

e92d5c1

marcaaron dismissed stale reviews from mountiny and cead22 via e92d5c1 September 2, 2022 10:26

remove useless comment

d088e8c

danieldoglas approved these changes Sep 2, 2022

View reviewed changes

thienlnam approved these changes Sep 2, 2022

View reviewed changes

mountiny approved these changes Sep 2, 2022

View reviewed changes

marcaaron merged commit 8ce2774 into main Sep 2, 2022

marcaaron deleted the marcaaron-removeUnreadActionCount branch September 2, 2022 11:13

melvin-bot bot added the Emergency label Sep 2, 2022

marcaaron removed the Emergency label Sep 2, 2022

OSBotify mentioned this pull request Sep 5, 2022

Deploy Checklist: New Expensify 2022-09-05 #10808

Closed

33 tasks

This was referenced Sep 5, 2022

mWeb - Global Unread Indicator - Global Unread Indicator doesn't decreases #10828

Closed

Android - Global Unread Indicator - There is no Global Unread Indicator in Android App #10829

Closed

DanutGavrus mentioned this pull request Jun 21, 2023

[HOLD for payment 2023-07-12] [$1000] Web -Thread - When deleting the last unread message, the "New" marker briefly appears for a second before disappearing #20759

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `unreadActionCount` & Improve Global Indicators #10739

Remove `unreadActionCount` & Improve Global Indicators #10739

marcaaron commented Sep 1, 2022 •

edited by Julesssss

Loading

marcaaron commented Sep 2, 2022

mountiny left a comment

cead22 Sep 2, 2022

cead22 Sep 2, 2022

marcaaron Sep 2, 2022

cead22 Sep 2, 2022

cead22 Sep 2, 2022

marcaaron Sep 2, 2022

mountiny commented Sep 2, 2022

cead22 Sep 2, 2022

mountiny left a comment

marcaaron commented Sep 2, 2022

marcaaron commented Sep 2, 2022

danieldoglas left a comment

thienlnam left a comment

mountiny left a comment

melvin-bot bot commented Sep 2, 2022

OSBotify commented Sep 2, 2022

marcaaron commented Sep 2, 2022

OSBotify commented Sep 5, 2022

Julesssss commented Sep 7, 2022

	const totalCount = _.reduce(reports, (total, report) => total + (ReportUtils.isUnread(report) ? 1 : 0), 0);
	const totalCount = _.filter(reports, ReportUtils.isUnread).length

Remove unreadActionCount & Improve Global Indicators #10739

Remove unreadActionCount & Improve Global Indicators #10739

Conversation

marcaaron commented Sep 1, 2022 • edited by Julesssss Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Sep 2, 2022

mountiny left a comment

Choose a reason for hiding this comment

cead22 Sep 2, 2022

Choose a reason for hiding this comment

cead22 Sep 2, 2022

Choose a reason for hiding this comment

marcaaron Sep 2, 2022

Choose a reason for hiding this comment

cead22 Sep 2, 2022

Choose a reason for hiding this comment

cead22 Sep 2, 2022

Choose a reason for hiding this comment

marcaaron Sep 2, 2022

Choose a reason for hiding this comment

mountiny commented Sep 2, 2022

cead22 Sep 2, 2022

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

marcaaron commented Sep 2, 2022

marcaaron commented Sep 2, 2022

danieldoglas left a comment

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 2, 2022

OSBotify commented Sep 2, 2022

marcaaron commented Sep 2, 2022

OSBotify commented Sep 5, 2022

Julesssss commented Sep 7, 2022

Remove `unreadActionCount` & Improve Global Indicators #10739

Remove `unreadActionCount` & Improve Global Indicators #10739

marcaaron commented Sep 1, 2022 •

edited by Julesssss

Loading