fix: notification count for empty reports #30133

allroundexperts · 2023-10-21T21:27:14Z

Details

This PR fixes the notification count going up when an empty report is unread.

NOTE @cubuspl42 I've used isEmpty along with isUnread instead of shouldReportBeInOptionList function because I found the former to be more simpler. The later would require us to fetch all the policies, betas and report actions before hand which I think is in-efficient and can be easily avoided by using isEmpty instead.

Fixed Issues

$ #28536
PROPOSAL: #28536 (comment)

Tests

Send a message in any report
Hover over the message and reply in thread
Right click on the thread and click mark as unread
Click out of the thread and verify that the notification count does not increase.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Send a message in any report
Hover over the message and reply in thread
Right click on the thread and click mark as unread
Click out of the thread and verify that the notification count does not increase.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

Screen.Recording.2023-10-22.at.2.03.19.AM.mov

iOS: Native

Screen.Recording.2023-11-17.at.1.08.47.AM.mov

iOS: mWeb Safari

Screen.Recording.2023-10-22.at.4.08.44.AM.mov

MacOS: Chrome / Safari

Screen.Recording.2023-10-22.at.1.56.09.AM.mov

MacOS: Desktop

melvin-bot · 2023-10-21T21:27:19Z

@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

cubuspl42 · 2023-10-23T07:47:30Z

src/libs/ReportUtils.js

+ * @param {Object} report
+ * @returns {Boolean}
+ */
+function isEmptyReport(report) {


Isn't this unused?

@allroundexperts

Forgot about this. Removed now!

cubuspl42 · 2023-10-23T07:56:13Z

@allroundexperts

NOTE @cubuspl42 I've used isEmpty along with isUnread instead of shouldReportBeInOptionList function because I found the former to be more simpler. The later would require us to fetch all the policies, betas and report actions before hand which I think is in-efficient and can be easily avoided by using isEmpty instead.

Well, this would undermine the whole proposal selection process...

Are you entirely sure that these two conditions are 100% equivalent? I wanted to achieve the effect when it's impossible to have a notification for a report that's not visible in the LHN, as I explicitly stated in my comments on the issue.

If it's currently ineffective performance-wise to achieve this effect, we could figure out if there are any ways to speed things up, possibly by creating a single operator with the filtering semantics, like getReportsThatShouldReportBeInOptionList?

Still, it would be great to get some numbers first to ensure we have a performance problem here.

I see that this note is struck-through, you can let me know what made you change your mind in this matter

allroundexperts · 2023-10-23T15:17:20Z

@allroundexperts

NOTE @cubuspl42 I've used isEmpty along with isUnread instead of shouldReportBeInOptionList function because I found the former to be more simpler. The later would require us to fetch all the policies, betas and report actions before hand which I think is in-efficient and can be easily avoided by using isEmpty instead.

Well, this would undermine the whole proposal selection process...

Are you entirely sure that these two conditions are 100% equivalent? I wanted to achieve the effect when it's impossible to have a notification for a report that's not visible in the LHN, as I explicitly stated in my comments on the issue.

If it's currently ineffective performance-wise to achieve this effect, we could figure out if there are any ways to speed things up, possibly by creating a single operator with the filtering semantics, like getReportsThatShouldReportBeInOptionList?

Still, it would be great to get some numbers first to ensure we have a performance problem here.

I see that this note is struck-through, you can let me know what made you change your mind in this matter

@cubuspl42 I initially thought that we will need to supply policies, betas and allReportActions to the getReportsThatShouldReportBeInOptionList function. However, later I realised that those are not required. That's why I changed the approach again and went on with the one originally decided in the proposal.

allroundexperts · 2023-10-26T04:36:39Z

Bump @cubuspl42

cubuspl42 · 2023-10-26T08:41:12Z

@allroundexperts Ouch, I was 100% sure I left a comment about my finding. I either didn't submit it properly, or it landed in some other random issue 😶

So here it is:

wrong-notification-count-ios-web.mp4

wrong-notification-count-ios.mp4

Do you see the problem? Things aren't completely reactive. You need to "touch" the system in a way so the count updates properly. Do you also observe this?

allroundexperts · 2023-10-26T08:45:18Z

@cubuspl42 Can you please confirm if this isn't an issue on Staging?

cubuspl42 · 2023-10-26T09:18:41Z

@allroundexperts As I see it, this is related to the new behavior we're implementing.

Please test this on iOS, and preferably provide a video, as from your screenshot, it's really not clear that the testing on iOS succeeded. The notification count can't be seen at all.

cubuspl42 · 2023-11-14T10:44:22Z

@allroundexperts Bump!

cubuspl42 · 2023-11-15T09:51:19Z

@allroundexperts From my perspective, we're still on:

Please test this on iOS, and preferably provide a video, as from your screenshot, it's really not clear that the testing on iOS succeeded. The notification count can't be seen at all.

Also, there are conflicts to be resolved.

MonilBhavsar · 2023-11-16T13:23:37Z

@allroundexperts if you could please take a look at @cubuspl42 comments. This fixes a high priority project issue #31276

allroundexperts · 2023-11-16T19:34:38Z

I think I've missed this bug. Apologies. Fixing right now.

allroundexperts · 2023-11-16T20:11:17Z

@cubuspl42 Added iOS native screen recording as requested.

OSBotify · 2023-11-16T20:13:26Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30133/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30133/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30133/NewExpensify.dmg	https://30133.pr-testing.expensify.com

AndrewGable · 2023-11-16T20:23:10Z

FYI this fixed the unread bug for me. Staging build on the left tab, AdHoc on the right tab.

cubuspl42 · 2023-11-21T15:42:51Z

@allroundexperts
I can still reproduce the problem.
wrong-notification-count-2-ios.mp4
This is a problem with the notification count not reducing (in this particular case, from 11 to 10), even when it should. As reducing the notification count is the subject of this PR (the issue we're solving was that it does not reduce), I don't know what you'd expect me to test on staging.
If you know this is a problem on main/staging, please provide a video. I'm not sure what the testing steps would be, but it would clear up any misunderstanding we might have.

@cubuspl42 Initially, I see the count as 10. At 15 seconds, you mark the thread as unread. After that, you go back and close the app. At this point the count is 11. Is it your expectation here that pressing the back button should again mark the thread as read? If not, then where are you marking it as read?

After going back to LHN, that thread becomes a "ghost" thread, right? 11 is a valid number only for a short period of time, right after I mark it as unread. When I leave the thread, it should go down to 10 again. Isn't this the whole point of this issue/PR?

I believed we're aiming to introduce an invariant: "A notification count should only include threads that are accessible, i.e. can be navigated to". As I see it, I demonstrate the invariant being broken.

cubuspl42 · 2023-11-27T09:47:18Z

@allroundexperts Bump. Let me know if this explanation is still not clear.

AmjedNazzal · 2023-11-27T10:32:20Z

@cubuspl42 The issue you are currently having is the reason my proposal focused on hooking into optionListItemsWithCurrentReport in SidebarLinksData because that would make sure the unread indicator update is responsive against changes in the LHN as items get added and removed because that's where we handle the LHN visible reports on a higher level.

allroundexperts · 2023-11-30T00:13:37Z

@cubuspl42 Fixed the issue.

Screen.Recording.2023-11-30.at.5.10.52.AM.mov

cubuspl42 · 2023-11-30T11:08:43Z

src/libs/UnreadIndicatorUpdater/index.js

-let previousUnreadCount = 0;
+let allReports = [];
+
+const triggerUnreadUpdate = (reports = allReports) => {


What's the deal with reports = allReports? Is it ever non-default?

It's never non-default. I've updated the code and removed this parameter.

cubuspl42 · 2023-11-30T12:44:40Z

src/libs/UnreadIndicatorUpdater/index.js

+let allReports = [];
+
+const triggerUnreadUpdate = () => {
+    const currentReportID = navigationRef.isReady() ? Navigation.getTopmostReportId() : '';


Have you tested this '' case? I understand the defence-in-depth strategy, but I'm not convinced by this specific implementation of it.

Maybe...

const currentReportID = navigationRef.isReady() ? Navigation.getTopmostReportId() : null; const unreadReports = _.filter(allReports, (report) => { const isAccessible = currentReportID !== null ? ReportUtils.shouldReportBeInOptionList(report, currentReportID) : true; return ReportUtils.isUnread(report) && isAccessible; });

or : false, depending on what we want to do in the corner case

This is in accordance with how we're saving currentReportID in CurrentReportIdContext

cubuspl42 · 2023-11-30T12:47:29Z

src/libs/UnreadIndicatorUpdater/index.js

-         * More info: https://reactnative.dev/docs/interactionmanager
-         */
-        InteractionManager.runAfterInteractions(() => {
-            const unreadReports = _.filter(reportsFromOnyx, (report) => ReportUtils.isUnread(report) && report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN);


Why was the report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN condition removed?

Because we wanted to sync this exactly with the LHN.

I don't know, did we?

I summed it up before as...

"A notification count should only include threads that are accessible, i.e. can be navigated to".

...in other words...

"A notification count should not include threads that are inaccessible, i.e. cannot be navigated to".

...and I stand with this. It's my personal summary based on my understanding of the issue.

Notification preferences are a user-accessible feature:

We don't want to break this, right? It sounds like a different topic.

I think this is something different. By setting this preference, you're setting if you want to receive a notification for a new message. Changing this won't really effect the count.

In other words, do you suggest that the old code checking for report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN was a mistake (an unnecessary / irrelevant check)?

Or maybe that the user setting I added a screenshot of is not actually related to report.notificationPreference?

@marcaaron Im OoO for two more days. I'll raise a PR for this after then if that's fine.

Do we have a GH issue for this mentioned regression?

I think #33506 could be regression from this PR.

@allroundexperts

Im OoO for two more days. I'll raise a PR for this after then if that's fine.

Would you provide an update?

We can move the further discussion to #33506

PR created already!

cubuspl42 · 2023-12-01T12:37:23Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

notification-count-empty-reports-web.mp4

Mobile Web - Chrome

notification-count-empty-reports-android-web-compressed.mp4

Mobile Web - Safari

notification-count-empty-reports-ios-web.mp4

Desktop

iOS

notification-count-empty-reports-ios.mp4

Android

notification-count-empty-reports-android-compressed.mp4

OSBotify · 2023-12-04T16:43:06Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/30133/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/30133/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/30133/NewExpensify.dmg	https://30133.pr-testing.expensify.com

cubuspl42 · 2023-12-05T11:19:57Z

@Gonals Friendly bump

OSBotify · 2023-12-05T12:03:14Z

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

OSBotify · 2023-12-06T22:17:10Z

🚀 Deployed to staging by https://github.com/Gonals in version: 1.4.9-0 🚀

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

OSBotify · 2023-12-06T23:11:20Z

🚀 Deployed to staging by https://github.com/Gonals in version: 1.4.9-0 🚀

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

OSBotify · 2023-12-08T01:08:27Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.9-5 🚀

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

marcaaron · 2023-12-21T18:34:15Z

As mentioned here. This caused a regression.

allroundexperts added 2 commits October 22, 2023 00:40

fix: unread logic

b0c9b95

fix: empty condition

90c915b

allroundexperts requested a review from a team as a code owner October 21, 2023 21:27

melvin-bot bot removed the request for review from a team October 21, 2023 21:27

melvin-bot bot requested a review from cubuspl42 October 21, 2023 21:27

allroundexperts changed the title ~~Fix 28536~~ fix: notification count for empty reports Oct 21, 2023

allroundexperts mentioned this pull request Oct 21, 2023

[HOLD for payment 2023-12-15] MEDIUM: [$500] Notifications - Reports removed from LHN can still show notifications leading to unremovable notifications #28536

Closed

6 tasks

feat: mimic lhn

15294be

cubuspl42 reviewed Oct 23, 2023

View reviewed changes

allroundexperts added 2 commits October 24, 2023 19:26

fix: remove unused variable

7fba759

Merge branch 'main' into fix-28536

208d572

allroundexperts requested a review from cubuspl42 October 24, 2023 14:29

cubuspl42 mentioned this pull request Nov 16, 2023

MEDIUM: [$500] Unread indicator is showing in the fevicon even though there is nothing in LHN #31276

Closed

6 tasks

Merge branch 'main' into fix-28536

d5391d9

AndrewGable added the Ready To Build label Nov 16, 2023

fix lint errors

04119fd

allroundexperts added 3 commits November 30, 2023 04:04

merge with main

b5987fe

lint fix

1cdb174

fix: sync unread count with lhn

50580cb

allroundexperts requested a review from cubuspl42 November 30, 2023 00:13

allroundexperts added 2 commits November 30, 2023 05:22

fix: unit tests

5bff1bd

fix: added comment

59cfe6f

cubuspl42 reviewed Nov 30, 2023

View reviewed changes

fix: remove report param since its stale

bbe1a98

allroundexperts requested a review from cubuspl42 November 30, 2023 11:29

cubuspl42 reviewed Nov 30, 2023

View reviewed changes

cubuspl42 approved these changes Dec 1, 2023

View reviewed changes

melvin-bot bot requested a review from Gonals December 1, 2023 12:37

Gonals approved these changes Dec 5, 2023

View reviewed changes

Gonals merged commit a7af90e into Expensify:main Dec 5, 2023
16 of 29 checks passed

github-actions bot mentioned this pull request Dec 6, 2023

Deploy Checklist: New Expensify 2023-12-06 #32589

Closed

74 tasks

mananjadhav mentioned this pull request Dec 7, 2023

LOW: [$500] Unread notification in fevicon does not update when the user is in another tab #32225

Closed

6 tasks

marcaaron mentioned this pull request Jan 4, 2024

[HOLD for payment 2024-02-01] MEDIUM: Notification counter has phantom unreads #33506

Closed

6 tasks

fix: notification count for empty reports #30133

fix: notification count for empty reports #30133

Conversation

allroundexperts commented Oct 21, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cubuspl42 commented Oct 23, 2023

allroundexperts commented Oct 23, 2023

allroundexperts commented Oct 26, 2023

cubuspl42 commented Oct 26, 2023

allroundexperts commented Oct 26, 2023

cubuspl42 commented Oct 26, 2023

cubuspl42 commented Nov 14, 2023

cubuspl42 commented Nov 15, 2023 • edited Loading

MonilBhavsar commented Nov 16, 2023

allroundexperts commented Nov 16, 2023

allroundexperts commented Nov 16, 2023

OSBotify commented Nov 16, 2023

AndrewGable commented Nov 16, 2023 • edited Loading

cubuspl42 commented Nov 21, 2023 • edited Loading

cubuspl42 commented Nov 27, 2023

AmjedNazzal commented Nov 27, 2023

allroundexperts commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cubuspl42 commented Dec 1, 2023

Reviewer Checklist

Screenshots/Videos

OSBotify commented Dec 4, 2023

cubuspl42 commented Dec 5, 2023

OSBotify commented Dec 5, 2023

OSBotify commented Dec 6, 2023

OSBotify commented Dec 6, 2023

OSBotify commented Dec 8, 2023

marcaaron commented Dec 21, 2023

allroundexperts commented Oct 21, 2023 •

edited

Loading

cubuspl42 commented Nov 15, 2023 •

edited

Loading

AndrewGable commented Nov 16, 2023 •

edited

Loading

cubuspl42 commented Nov 21, 2023 •

edited

Loading