Hide notifications for the currently open report #15858

arosiclair · 2023-03-10T19:40:07Z

Details

Adds context awareness to push notifications from Airship to hide notifications for a report the user is already reading (or messages the user may have already read). Reuses our existing logic for local notifications

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/259220

Tests

No testing needed for mWeb Safari or mWeb Chrome. Notifications are not supported on those platforms.

Log into NewDot
Accept any prompt for notification permissions
Open a chat that is NOT from user A
Send a message from user A to your user
Verify a notification displays for the message
Open the chat for user A
Send another message
Verify a notification does NOT display for the message

Offline tests

None

QA Steps

Same as Tests

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (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 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
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- 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 correct English and 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 followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
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
I verified that if a function's arguments changed that all usages have also been updated correctly
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 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 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 author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

iOS

Android

…e wrong platform

arosiclair · 2023-03-17T19:44:35Z

src/libs/Notification/PushNotification/subscribeToReportCommentPushNotifications.js

This is an existing function pulled from actions/Report.js to fix a require cycle from PushNotification libs

src/libs/fileDownload/index.ios.js

MelvinBot · 2023-03-17T20:00:56Z

@parasharrajat @neil-marcellini One of you needs to 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]

android/app/build.gradle

arosiclair · 2023-04-06T17:54:42Z

@arosiclair is it cool with you if I wait to review until tomorrow?

Yeah no worries

I think someone needs to trigger the builds.

Sorry forgot to manually run the workflow. It should be done in an hour or so.

OSBotify · 2023-04-06T18:54:35Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

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

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

parasharrajat · 2023-04-06T23:15:22Z

Might be a bug: Android: After a chat is active and the user goes back to LHN, no notifications are shown for that chat.

Screenrecorder-2023-04-07-04-41-04-994.mp4

arosiclair · 2023-04-07T14:25:15Z

Might be a bug: Android: After a chat is active and the user goes back to LHN, no notifications are shown for that chat.

Yup known issue. When swiping back to the LHN, the chat is still "open" to the right. You have to deliberately select another chat to switch. This will be fixed by the navigation reboot.

parasharrajat · 2023-04-07T15:20:18Z

Ok, glad to hear that. Do we have an issue added to the main tracking issue of the navigation reboot? or Should we create a new one?

Web: Should the notification be hidden for the active chat even if the window is not focused/active?

I am going to test this a little more to understand how this works overall.

neil-marcellini

Great work thanks! It tests well and the code looks good.

neil-marcellini · 2023-04-07T17:17:10Z

src/libs/ReportActionsUtils.js

+    const reportActionUpdate = _.find(onyxData, onyxUpdate => onyxUpdate.key.startsWith(ONYXKEYS.COLLECTION.REPORT_ACTIONS));
+
+    if (!reportActionUpdate) {
+        return null;


NAB: In general I think we prefer keeping the return type consistent by using an empty object instead, but it's not a big deal.

arosiclair · 2023-04-07T18:48:13Z

Ok, glad to hear that. Do we have an issue added to the main tracking issue of the navigation reboot? or Should we create a new one?

I don't think another issue is necessary to track this specifically. It should be fixed by #15847.

@parasharrajat Could we up the urgency for the checklist? This PR has been sitting around in limbo for a month now. It's a big UX improvement especially for busy rooms so let's try pushing this through ASAP.

parasharrajat · 2023-04-07T20:31:34Z

@arosiclair Can you please merge main into this? I am gonna finish the checklist.

parasharrajat · 2023-04-07T20:32:28Z

Screenshots

🔲 iOS / native

🔲 MacOS / Desktop

screen-2023-04-08_04.59.04.mp4

🔲 MacOS / web

screen-2023-04-08_04.56.20.mp4

🔲 Android / native

Screenrecorder-2023-04-07-04-41-04-994.mp4

…ip-15

parasharrajat · 2023-04-07T23:35:06Z

This #15858 (comment) is not happening on the Desktop. The desktop still shows the notification for the not active window.

It means that it is a bug on the Web. @arosiclair

Check the video for Desktop and web. The web does not show notifications for an active chat even when the window is not focused. But the Desktop shows the notification.

Either both should show or both should hide.

arosiclair · 2023-04-10T15:39:59Z

The web does not show notifications for an active chat even when the window is not focused. But the Desktop shows the notification.

Yes this is pre-existing behavior I checked that here. The visibility behavior between web, desktop and native is not quite the same (code). We can log a separate bug to address that.

parasharrajat

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 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 correct English and 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 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 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.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @Julesssss

🎀 👀 🎀 C+ reviewed

MelvinBot · 2023-04-10T15:50:18Z

🎯 @parasharrajat, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #17219.

parasharrajat · 2023-04-10T15:50:33Z

Got it. Thanks, @arosiclair . I am going to report that to Slack.

Reported https://expensify.slack.com/archives/C049HHMV9SM/p1681142429319059

OSBotify · 2023-04-10T17:02:56Z

✋ 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-04-10T19:26:49Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.2.98-0 🚀

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

OSBotify · 2023-04-11T21:27:21Z

🚀 Deployed to production by https://github.com/francoisl in version: 1.2.98-2 🚀

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

s77rt · 2023-04-13T03:46:28Z

This PR caused a regression. CONST here is incorrectly imported: expensify-common/lib/CONST does not have DEEPLINK_BASE_URL. It should have been imported from E/App.

parasharrajat · 2023-04-13T10:33:20Z

Ok I didn't notice during the review.

arosiclair self-assigned this Mar 10, 2023

arosiclair added 13 commits March 14, 2023 16:47

pull out notification checks into helper function

6a9ae47

use shouldShowReportActionNotification for push notifications on android

3c3dc24

show notifications from concierge

e0626ea

use casing consistent with other logging

be57390

don't show notifications for report actions that were already read

d283491

Merge branch 'main' into arosiclair-airship-15

085e59d

fix camera-roll import

8916cdb

fix iOS notification presentation options for v15

f03189f

use context awareness for Android and iOS push notifications

6abdfcd

update comments

c074d49

update pods

a9647a1

clearer logging for debugging

1616a47

handle push notifications with no reportAction in payload

f3279a2

arosiclair changed the title ~~Migrate to Airship v.15.2.0~~ Silence notifications for report actions the user has already seen Mar 16, 2023

arosiclair added 5 commits March 16, 2023 18:05

use report action from onyxData

20b3c82

use platform modules to fix errors about using iOS/Android SDKs on th…

48128a8

…e wrong platform

add airship mocks for foreground callbacks

9fa14dd

mock new camera roll lib

fc6ebf8

fix require cycle between PushNotification libs and Report lib

9dc1ec4

arosiclair commented Mar 17, 2023

View reviewed changes

src/libs/fileDownload/index.ios.js Outdated Show resolved Hide resolved

arosiclair changed the title ~~Silence notifications for report actions the user has already seen~~ Hide notifications for the currently open report Mar 17, 2023

arosiclair marked this pull request as ready for review March 17, 2023 20:00

arosiclair requested a review from a team as a code owner March 17, 2023 20:00

melvin-bot bot requested review from neil-marcellini and parasharrajat and removed request for a team March 17, 2023 20:00

Julesssss reviewed Mar 20, 2023

View reviewed changes

android/app/build.gradle Outdated Show resolved Hide resolved

neil-marcellini previously approved these changes Apr 7, 2023

View reviewed changes

Merge branch 'main' of github.com:Expensify/App into arosiclair-airsh…

fd0afba

…ip-15

arosiclair dismissed neil-marcellini’s stale review via fd0afba April 7, 2023 20:44

arosiclair requested review from neil-marcellini and parasharrajat April 7, 2023 20:45

parasharrajat approved these changes Apr 10, 2023

View reviewed changes

melvin-bot bot mentioned this pull request Apr 10, 2023

[HOLD for payment 4/18] Payment issue for E/App PR#15858 #17219

Closed

neil-marcellini approved these changes Apr 10, 2023

View reviewed changes

neil-marcellini merged commit bcada91 into main Apr 10, 2023

neil-marcellini deleted the arosiclair-airship-15 branch April 10, 2023 16:59

OSBotify mentioned this pull request Apr 10, 2023

Deploy Checklist: New Expensify 2023-04-10 #17230

Closed

11 tasks

francoisl mentioned this pull request Apr 11, 2023

[HOLD for payment 2023-05-16] [$1000] Web does not show not show notifications for not active window. #17242

Closed

6 tasks

s77rt mentioned this pull request Apr 13, 2023

[HOLD for payment 2023-04-24] [HOLD for payment 2023-04-20] [$2000] Tapping on notification /switching chats does not take to correct chat / closing immediately #15758

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide notifications for the currently open report #15858

Hide notifications for the currently open report #15858

arosiclair commented Mar 10, 2023 •

edited

Loading

arosiclair Mar 17, 2023

MelvinBot commented Mar 17, 2023

arosiclair commented Apr 6, 2023

OSBotify commented Apr 6, 2023

parasharrajat commented Apr 6, 2023

arosiclair commented Apr 7, 2023

parasharrajat commented Apr 7, 2023

neil-marcellini left a comment

neil-marcellini Apr 7, 2023

arosiclair commented Apr 7, 2023

parasharrajat commented Apr 7, 2023

parasharrajat commented Apr 7, 2023 •

edited

Loading

parasharrajat commented Apr 7, 2023

arosiclair commented Apr 10, 2023

parasharrajat left a comment

MelvinBot commented Apr 10, 2023

parasharrajat commented Apr 10, 2023 •

edited

Loading

OSBotify commented Apr 10, 2023

OSBotify commented Apr 10, 2023

OSBotify commented Apr 11, 2023

s77rt commented Apr 13, 2023

parasharrajat commented Apr 13, 2023

Hide notifications for the currently open report #15858

Hide notifications for the currently open report #15858

Conversation

arosiclair commented Mar 10, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

arosiclair Mar 17, 2023

Choose a reason for hiding this comment

MelvinBot commented Mar 17, 2023

arosiclair commented Apr 6, 2023

OSBotify commented Apr 6, 2023

parasharrajat commented Apr 6, 2023

arosiclair commented Apr 7, 2023

parasharrajat commented Apr 7, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Apr 7, 2023

Choose a reason for hiding this comment

arosiclair commented Apr 7, 2023

parasharrajat commented Apr 7, 2023

parasharrajat commented Apr 7, 2023 • edited Loading

Screenshots

🔲 iOS / native

🔲 MacOS / Desktop

🔲 MacOS / web

🔲 Android / native

parasharrajat commented Apr 7, 2023

arosiclair commented Apr 10, 2023

parasharrajat left a comment

Choose a reason for hiding this comment

Reviewer Checklist

MelvinBot commented Apr 10, 2023

parasharrajat commented Apr 10, 2023 • edited Loading

OSBotify commented Apr 10, 2023

OSBotify commented Apr 10, 2023

OSBotify commented Apr 11, 2023

s77rt commented Apr 13, 2023

parasharrajat commented Apr 13, 2023

arosiclair commented Mar 10, 2023 •

edited

Loading

parasharrajat commented Apr 7, 2023 •

edited

Loading

parasharrajat commented Apr 10, 2023 •

edited

Loading