Setup push notifications #424

roryabraham · 2020-09-08T21:27:37Z

Fixes: #288

Details

Adds Airship as a library (urbanairship-react-native)
- Airship's react native library is only compatible with iOS 11+, so I had to bump the iOS platform target
Configures iOS and Android with Airship API keys, capabilities, permissions, etc...
Refactors the Notification library to separate out LocalNotifications vs PushNotifications
- LocalNotifications
  - Are triggered by the client.
  - Are currently only implemented for web/desktop using browser notifications. In the future, LocalNotifications could be implemented for mobile via a growl, snackbar, or using the native notification system (which I think could be done pretty easily using wix/react-native-notifications
- PushNotifications
  - Are triggered remotely by the Airship API.
  - By default, are only displayed if the app is in the background.
  - Are only implemented for native platforms, and don't really have an equivalent on web/desktop.
  - Furthermore, if the app is in the foreground, any callbacks associated with push notifications will not be executed. We're assuming that if we're receiving a push notification, then we must be connected to the internet, and if we're connected to the internet, then we must be connected to Pusher. And if we're connected to pusher, then the events having to do with Push notifications will be caught and handled via pusher.
Gets permission to display push notifications when the app first loads
- If user has already granted or denied permissions for user notifications, this code not affect the user at all.
- From the Airship docs:
  
  The Airship module makes a distinction between 'user notifications,' which can be seen by the user, and 'invisible notifications' that carry only data for the app to process.
- Even if the user denies permission for 'user notifications', we will still be able to send push notifications to trigger update actions in the background.
On sign-in, registers the device for push notifications (associates the device and accountID with a named_user in the Airship API) On sign-out, deregisters the device for push notifications (associates the device with an undefined named_user in the Airship API)
- To implement this, we simply subscribe the push notification library to IONKEYS.SESSION.accountID. When the user signs in, the SESSION.accountID is assigned, and the registration is triggered. When the user signs out, we clear Ion, the SESSION.accountID is nullified, and the deregistration is triggered. However, we weren't executing the callbacks for Ion subscribers when we called Ion.clear(), so this PR implements that as well.

Tests / QA

Build the mobile app to a physical iPhone device (instructions here). (dev) Or just open the iPhone app 😉 (QA)
Log into an account and background the app.
Go to expensify.com.dev as a different user, and submit an expense report to the account you used in step 2.
Comment on the report, and verify that you receive a push notification on your device with the comment information.
Open the app, and verify that you see the report comment from step 4.
Sign into an account.
Run npm run desktop, and use the desktop UI to sign into another account.
Send a chat from the desktop account to the account you used on mobile, with the app open on mobile. Ensure that you do not see a push notification for the chat.
Send a chat from the desktop account to the account you used on mobile, with the app backgrounded on mobile. Ensure that you see a push notification.
Close the app on mobile. (i.e: actually close it by swiping up and then swiping it away, don't just background it)
Send a chat from the desktop account to the account you used on mobile. Ensure that you see a push notification.
Click on the push notification, and see that it opens up the app to the correct chat.
(dev) Find the accountID of the account you signed in on in mobile, and find a report ID for any of its reports. Then run $EXPENSIDEV/script/clitools push:reportComment, follow the prompts, and verify that you see the push notification show up.
Log out on mobile.
Send a chat from the web account to the account you used on mobile. Ensure that you don't see a push notification.
Build the mobile app to an Android emulator, and repeat steps 2-15.

…Mount

# Conflicts: # ios/Podfile.lock # ios/ReactNativeChat.xcodeproj/project.pbxproj # src/Expensify.js # src/lib/Notification/BrowserNotifications.js

AndrewGable · 2020-09-15T21:35:22Z

Regarding your discussion: I'd suggest we just follow what the API currently does which triggers native (iOS/Android) notifications from our Expensify API to the Urban Airship API. Then keep the web browser the same using local notifications via pusher.

The only difference I'd suggest is let's create a new project in Airship and send these pushes separately from our main Expensify app/Airship keys.

ios/Podfile

roryabraham · 2020-09-22T22:42:32Z

So today has been pretty successful in that I've been able to get test push notifications showing up on the device. I've also set up notifications to appear in the foreground by default, but we will likely not want to show push notifications for chat reports which are already visible.

I've also laid the groundwork by which the currently signed-in user registers for Airship notifications. We probably don't need to worry about edge cases where a copilot logs in, right?

I'm unsure how to reconcile differences between pusher and Airship, because they serve similar purposes in our united codebase. For example, if a chat is sent, our API would send a notification via pusher and Airship, and the app needs to be able to determine which notification to respond to/display. Here are some ideas:

Only use pusher for web/desktop, and Airship for mobile. We could lump both of these into a single "notification" library, with the other areas of code being unaware of which is used under the hood.
Ignore/don't show any Airship notifications if the app is connected to Pusher. If connected to pusher, trigger native notifications locally (I think I have figured out how to do this using another library)

tgolen · 2020-09-22T23:00:59Z

I think 1 sounds good, but the biggest problem with that is it will only work when notifications are enabled on mobile (and those can be easily disabled), so I think we need to favor relying on Pusher as much as we can (this would favor 2).

marcaaron · 2020-09-22T23:01:41Z

I think we might also experience a lot of latency since all data would be arriving via Airship on mobile which I'd guess is "less real-time" than Pusher (but open to being proved wrong on that).

AndrewGable · 2020-09-22T23:02:33Z

We probably don't need to worry about edge cases where a copilot logs in, right?

Agreed, I don't think co-pilot is on the road map.

Ignore/don't show any Airship notifications if the app is connected to Pusher. If connected to pusher, trigger native notifications locally

This feels like the better option to me (and it sounds like others agree on this being better as well)

AndrewGable · 2020-09-22T23:03:09Z

How do we handle 2 on the current Expensify app? We have both Airship and Pusher co-exist.

marcaaron · 2020-09-22T23:04:17Z

Ignore/don't show any Airship notifications if the app is connected to Pusher. If connected to pusher, trigger native notifications locally (I think I have figured out how to do this using another library)

Actually, I don't really even think this is an option. Local notifications are only relevant if the app is not in the foreground. If it is then we don't want to show them. At least last time we looked into this Pusher doesn't seem to work at all once an app is backgrounded.

marcaaron · 2020-09-22T23:07:33Z

IIRC here's what we do on mobile today:

Connect to Pusher when the app is in the foreground (and maybe only when a report is in view?)
When the app is not in the foreground device notifications are shown
When the app is in the foreground and receives an urban airship notification we conditionally display it as a snack bar notification, but never show it as a local notification. If we are looking at the report the update belongs to we do nothing with the airship notification.

roryabraham · 2020-09-22T23:15:09Z

Actually, I don't really even think this is an option. Local notifications are only relevant if the app is not in the foreground. If it is then we don't want to show them.

I was able to get remote push notifications displaying in the foreground earlier today (although I read it's only possible on iOS 10+). I think we could get this working with local notifications too.

Jag96 · 2020-10-14T23:28:47Z

android/build.gradle

@@ -14,6 +14,8 @@ buildscript {
    }
    dependencies {
        classpath("com.android.tools.build:gradle:3.5.3")
+        classpath("com.google.gms:google-services:4.2.0")


Any reason not to use 4.3.4 here? Just asking b/c I'm also adding this in this PR

I have no opinion here - again, I didn't write any of this config stuff manually.

# Conflicts: # ios/ReactNativeChat.xcodeproj/project.pbxproj # src/lib/API.js # src/lib/actions/Report.js

roryabraham · 2020-10-14T23:53:57Z

Went through and fixed a few more bugs, retested on all platforms, and then merged master. Ready for more reviews 👍

Jag96

Code LGTM, found an issue during tests:

Push notifications are showing up on my physical iPhone even when I have the chat open. If I am on Account A on mobile, viewing a chat with Account B, and I type a message on web from Account B, the message shows up on mobile and then a few seconds later a push notification appears for that message.
If I am on Account A viewing a chat with Account B, and Account C sends me a message, I see a push notification. Tapping on that push notification doesn't bring me to the chat w/ Account C. I'm assuming there just shouldn't be a notification shown in this case, and if so the fix for 1 will remove this case

roryabraham · 2020-10-16T01:00:48Z

Hmmm that's pretty strange: at one point I had to intentionally write code to make notifications appear in the foreground, and since removing it I haven't seen any appear in the foreground. I'll have to look into that. #2 is feature, not a bug haha (if we receive a push notification while the app is in the foreground, we assume we're connected to pusher so we don't execute any callbacks)

roryabraham · 2020-10-20T06:07:11Z

Push notifications are showing up on my physical iPhone even when I have the chat open.

Okay, so I'm seeing the same thing on my end, but I'm not sure how to resolve it. This is controlled by the OS, and I think it is a problem with UrbanAirship's React Native SDK. So I opened a GH issue to hopefully get a solution. You can read up on the issue for steps I took to attempt to resolve this, and if you have any other ideas for things I could try I'm all ears.

In the meantime, maybe what we should do is allow callbacks to execute for notifications received in the foreground so that tapping on the notification will open the correct report, then merge this PR and create a separate issue to disable foreground notifications. In my opinion, the benefits of having functional push notifications probably outweigh the potential inconveniences of having them display in the foreground while I investigate and try to resolve it.

marcaaron · 2020-10-20T16:45:47Z

the benefits of having functional push notifications probably outweigh the potential inconveniences of having them display in the foreground while I investigate and try to resolve it

Does this mean that when you are chatting with someone each new message from them will trigger a notification in the foreground?

roryabraham · 2020-10-20T17:16:38Z

Does this mean that when you are chatting with someone each new message from them will trigger a notification in the foreground?

Yes, on iOS

marcaaron · 2020-10-20T22:45:27Z

src/libs/Notification/PushNotification/index.native.js

+    if (AppState.currentState === 'active' && eventType === EventType.PushReceived) {
+        console.debug('[PUSH_NOTIFICATION] Push received while app is in foreground, not executing any callback.');
+        return;
+    }


If I'm understanding this problem correctly... I think we could handle this before calling pushNotificationEventCallback(EventType.PushReceived, notification).

There's a lot of explanation going on here and I think at the end of the day all that needs to be said is what was there already. The selected event should always do the callback. Said another way, whether or not notifications show in the foreground wouldn't make a difference - they just happen to be showing and they should not show.

Scaled back this comment because you're right, we should only repress the callback for PushReceived events because those are the only ones that will be duplicates with pusher

To clarify I was suggesting we do something like this...

UrbanAirship.addListener(EventType.PushReceived, (notification) => { // If a push notification is received while the app is in foreground, // we'll assume pusher is connected so we'll ignore is and not fetch the same data twice. if (AppState.currentState === 'active') { console.debug('[PUSH_NOTIFICATION] Push received while app is in foreground, not executing any callback.'); return; } pushNotificationEventCallback(EventType.PushReceived, notification); });

and remove everything from this method.

roryabraham · 2020-10-20T22:59:10Z

Following this discussion I've set up push notification callbacks so that NotificationResponse event callbacks can be triggered when the app is in the foreground. So if you have chat A open and you get a foreground notification for chat B, then tapping it will open chat B. If you have chat A open and you get a foreground notification for chat A, then tapping it has no effect. We still do not allow callbacks to execute on a PushReceived event when the app is in the foreground, because we are operating under the assumption that pusher is also connected so we don't need to fetch the same data twice.

All this is to say, this is ready for another round of reviews. After a big merge, I retested all platforms and aside from the known issue with foreground notifications everything is working as expected.

roryabraham · 2020-10-20T23:44:55Z

Actually putting this on HOLD because it sounds like a fix for this should be pretty imminent, according to this comment

marcaaron · 2020-10-21T00:00:50Z

FYI don't mind where this landed and would merge as is. Once the lib is patched I'm pretty sure the solution will be to just "update" it.

roryabraham · 2020-10-21T00:19:37Z

Okay, sure. Let's do it 👍

marcaaron · 2020-10-21T18:41:38Z

src/pages/home/HomePage.js

-        this.animationTranslateX = new Animated.Value(!this.state.hamburgerShown ? -300 : 0);
+
+        // Note: This null check is only necessary because withIon passes null for bound props
+        //       that are null-initialized initialized in Ion, and defaultProps only replaces for `undefined` values


We should make a follow up issue for this and discuss. This keeps popping up.

Tim is OOO

roryabraham added 3 commits September 2, 2020 08:55

setup airship in RN, iOS, Android

3ce3814

port code from BrowserNotifications to Notification/index.js

deab109

export enableUserNotifications and call it in Expensify::componentDid…

9ac89a5

…Mount

roryabraham requested review from AndrewGable, marcaaron and a team September 8, 2020 21:27

roryabraham requested a review from tgolen as a code owner September 8, 2020 21:27

roryabraham self-assigned this Sep 8, 2020

botify requested review from thienlnam and removed request for a team September 8, 2020 21:27

roryabraham added 2 commits September 8, 2020 14:35

isClientTheLeader is a promise

f4b3617

Merge branch 'master' into Rory-PushNotifications

42ce96a

# Conflicts: # ios/Podfile.lock # ios/ReactNativeChat.xcodeproj/project.pbxproj # src/Expensify.js # src/lib/Notification/BrowserNotifications.js

roryabraham changed the title ~~[WIP] Setup push notifications~~ Setup push notifications Sep 15, 2020

roryabraham changed the title ~~Setup push notifications~~ [WIP] Setup push notifications Sep 15, 2020

roryabraham added 4 commits September 16, 2020 11:38

revert react-native-notification libary add and remove blank file

7714a1c

update browser notifications to match master

9dbe08e

merge master into Rory-PushNotifications

f93400b

got it running on a physical device

4a98da3

roryabraham commented Sep 17, 2020

View reviewed changes

ios/Podfile Show resolved Hide resolved

present notifications in the foreground on iOS

1572e4d

Jag96 reviewed Oct 14, 2020

View reviewed changes

Merge branch 'master' into Rory-PushNotifications

6b8b7d6

# Conflicts: # ios/ReactNativeChat.xcodeproj/project.pbxproj # src/lib/API.js # src/lib/actions/Report.js

roryabraham requested review from Jag96 and marcaaron October 14, 2020 23:53

roryabraham added 3 commits October 14, 2020 16:57

clean pods (but leave manual signing

344343f

Merge branch 'master' into Rory-PushNotifications

a1a1c35

remove native import bug

b49e7d4

Jag96 reviewed Oct 16, 2020

View reviewed changes

roryabraham added 2 commits October 20, 2020 14:07

allow notification response callbacks when app is in foreground

27553a9

try to fix bad merge

60865d7

roryabraham force-pushed the Rory-PushNotifications branch from 4a1e56c to 60865d7 Compare October 20, 2020 22:46

marcaaron reviewed Oct 20, 2020

View reviewed changes

roryabraham requested review from marcaaron and Jag96 October 20, 2020 22:59

roryabraham changed the title ~~Setup push notifications~~ [HOLD] Setup push notifications Oct 20, 2020

roryabraham changed the title ~~[HOLD] Setup push notifications~~ Setup push notifications Oct 21, 2020

Jag96 approved these changes Oct 21, 2020

View reviewed changes

marcaaron approved these changes Oct 21, 2020

View reviewed changes

marcaaron merged commit f9b6077 into master Oct 21, 2020

marcaaron deleted the Rory-PushNotifications branch October 21, 2020 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup push notifications #424

Setup push notifications #424

roryabraham commented Sep 8, 2020 •

edited

Loading

AndrewGable commented Sep 15, 2020

roryabraham commented Sep 22, 2020 •

edited

Loading

tgolen commented Sep 22, 2020

marcaaron commented Sep 22, 2020

AndrewGable commented Sep 22, 2020 •

edited

Loading

AndrewGable commented Sep 22, 2020 •

edited

Loading

marcaaron commented Sep 22, 2020

marcaaron commented Sep 22, 2020

roryabraham commented Sep 22, 2020

Jag96 Oct 14, 2020

roryabraham Oct 14, 2020 •

edited

Loading

roryabraham commented Oct 14, 2020 •

edited

Loading

Jag96 left a comment

roryabraham commented Oct 16, 2020

roryabraham commented Oct 20, 2020

marcaaron commented Oct 20, 2020

roryabraham commented Oct 20, 2020

marcaaron Oct 20, 2020

roryabraham Oct 20, 2020

marcaaron Oct 21, 2020

roryabraham commented Oct 20, 2020

roryabraham commented Oct 20, 2020 •

edited

Loading

marcaaron commented Oct 21, 2020

roryabraham commented Oct 21, 2020

marcaaron Oct 21, 2020

Setup push notifications #424

Setup push notifications #424

Conversation

roryabraham commented Sep 8, 2020 • edited Loading

Details

Tests / QA

AndrewGable commented Sep 15, 2020

roryabraham commented Sep 22, 2020 • edited Loading

tgolen commented Sep 22, 2020

marcaaron commented Sep 22, 2020

AndrewGable commented Sep 22, 2020 • edited Loading

AndrewGable commented Sep 22, 2020 • edited Loading

marcaaron commented Sep 22, 2020

marcaaron commented Sep 22, 2020

roryabraham commented Sep 22, 2020

Jag96 Oct 14, 2020

Choose a reason for hiding this comment

roryabraham Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

roryabraham commented Oct 14, 2020 • edited Loading

Jag96 left a comment

Choose a reason for hiding this comment

roryabraham commented Oct 16, 2020

roryabraham commented Oct 20, 2020

marcaaron commented Oct 20, 2020

roryabraham commented Oct 20, 2020

marcaaron Oct 20, 2020

Choose a reason for hiding this comment

roryabraham Oct 20, 2020

Choose a reason for hiding this comment

marcaaron Oct 21, 2020

Choose a reason for hiding this comment

roryabraham commented Oct 20, 2020

roryabraham commented Oct 20, 2020 • edited Loading

marcaaron commented Oct 21, 2020

roryabraham commented Oct 21, 2020

marcaaron Oct 21, 2020

Choose a reason for hiding this comment

roryabraham commented Sep 8, 2020 •

edited

Loading

roryabraham commented Sep 22, 2020 •

edited

Loading

AndrewGable commented Sep 22, 2020 •

edited

Loading

AndrewGable commented Sep 22, 2020 •

edited

Loading

roryabraham Oct 14, 2020 •

edited

Loading

roryabraham commented Oct 14, 2020 •

edited

Loading

roryabraham commented Oct 20, 2020 •

edited

Loading