Implement message-type Android push notifications #4270

HorusGoul · 2021-07-28T14:55:57Z

Details

Fixed Issues

$ #4007

Tests/QA Steps

Open three Expensify accounts. Let's call them (a), (b), and (c). In this case, (a) is opened on a real Android device.
From (a). Close the application.
From (b), send a message to (a).
From (a), make sure you receive a new notification that includes (b)'s avatar, name, and it's displayed in a similar way to the video under the Screenshots section of this PR. Don't dismiss the notification yet.
From (b), send another message to (a).
From (a), the notification should have been replaced with a new one that contains the two messages.
From (c), create a group and invite (a) and (b) to it.
From (c) and (b), send messages to that conversation.
From (a), make sure a new notification appears and that it shows the conversation in a similar way to the one you can see in the video under the Screenshots section of this PR.
From (a), dismiss all notifications.
From (b), send a new message to the conversation with (a).
From (a), make sure a new notification appears and that it only shows the last message sent in the conversation and not the rest of them.

Tested On

Screenshots

Android 10

Note that this video is recorded in Android 10. If you try to test this in a different Android version, the appearance of the notifications may vary.

Screencast_00002.mp4

HorusGoul · 2021-07-28T15:13:45Z

@roryabraham, could you do an early review of this? Also, I think we should wait until we finish #3844 to include the room/group support in this code.

…ge-type-android-push

Luke9389 · 2021-08-03T23:41:01Z

Interesting that this has to be written in java. I'm going to defer to @roryabraham's opinion on this review, given that he's got experience in java.

android/app/src/main/java/com/expensify/chat/CustomAirshipExtender.java

Julesssss · 2021-08-04T09:39:27Z

android/app/src/main/java/com/expensify/chat/CustomNotificationListener.java

+import androidx.annotation.NonNull;
+
+import com.urbanairship.push.NotificationActionButtonInfo;


android/app/src/main/java/com/expensify/chat/CustomAirshipExtender.java

android/app/src/main/java/com/expensify/chat/CustomNotificationListener.java

Julesssss · 2021-08-04T09:47:34Z

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

+    // Max wait time to resolve an icon. We have ~10 seconds to a little less
+    // to ensure the notification builds.


to ensure the notification builds

What does this mean? Is there a hard limit of 10 seconds?

It's related to this: https://developer.android.com/about/versions/12/behavior-changes-all#foreground-service-notification-delay

To provide a streamlined experience for short-running foreground services on Android 12, the system can delay the display of foreground service notifications by 10 seconds for certain foreground services. This change gives short-lived tasks a chance to complete before their notifications appear.

The Android 12 change only applies to foreground services. Regular notifications have no such limit and should always display -- unless the UA implementation is doing something very weird.

Android is happy for us to do stuff when the app is backgrounded, provided we display a long-running notification (like FB and WhatsApp 'Checking for messages...' notification). But that is very different from what we're trying to do here, which is just display a notification.

I'm not sure about this then; we should ask the UA team. Still, delaying notifications +10 seconds because we're fetching an avatar doesn't seem like a good idea even in bad network conditions.

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

Julesssss · 2021-08-04T10:15:23Z

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

+
+    @NonNull
+    private IconCompat fetchIcon(@NonNull String urlString, @DrawableRes int fallbackId) {
+        // TODO: Add disk LRU cache


We shouldn't leave TODO's. Does this still need to be done?

I'm not sure, and this was one of the things that I didn't touch at all. It would be interesting to store avatar data in the disk to avoid fetching from the network every time we receive a new notification that belongs to a different person.

Can you remove the TODO in that case, as it sounds like it is not a priority of ours

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

Julesssss · 2021-08-04T10:47:27Z

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

+
+        if (parsedUrl != null) {


Maybe return early instead, if parsedURl is null?

Julesssss · 2021-08-04T10:49:27Z

android/app/src/main/java/com/expensify/chat/CustomNotificationProvider.java

+                Log.e(TAG,"Failed to fetch icon", e);
+                Thread.currentThread().interrupt();


What is this for?

No idea. I copied this function from a Gist that Urban Airship provided us.

Julesssss

Looking better, but these comments remain unsolved:

This thread
This thread has a few
issues

My main worry is that we seem to be using a long-running service in order to style notifications based on your comments, which seems very wrong IMO. Notifications should fire once, there is no need to depend on a long-running background service to display notifications as this will definitely get killed by the OS and stop working.

To test this, I built the branch as a release build (compiled JS instead of depending on Chrome) using these steps (letting us background/kill the app as works in production) and I was unable to get one notification to show up. I wonder if you see the same thing when following these steps?

android.background.notification.styling.mov

HorusGoul · 2021-08-11T08:40:02Z

@Julesssss

Regarding the long-running service, no, sorry if I didn't explain myself. The service only gets to run when a new notification arrives. That's why I mentioned the need for persistence. So, in this case, we don't need to worry about having a long running service.

I've run into the same issue while testing; sometimes, I've needed to replace this line in Web-Expensify https://github.com/Expensify/Web-Expensify/blob/4e4da7aa858b7cb2eecb0a2bbd8409beaa815203/partners/UrbanAirship/push.php#L29 with:

        $isInChannel = false;

I'm not sure if it is a bug in the app, but I assumed it only happened in dev as I haven't run into the same issue in prod.

Julesssss · 2021-08-13T19:39:05Z

Regarding the long-running service, no, sorry if I didn't explain myself. The service only gets to run when a new notification arrives. That's why I mentioned the need for persistence. So, in this case, we don't need to worry about having a long-running service.

Okay, thanks for clarifying (and sorry for the slow response).

I'm still wary of the service though as other Urban Airship RN implementations used an out-of-date method of handling services which is directly responsible for our current problem where notifications are not immediately received. If they simply extend from FirebaseMessagingService then this is probably fine, but I'm worried about persistence being quite a hacky way of A) Styling notifications, and B) Persisting data in order to show the notification history.

Each service leads to background processing which will decrease the frequency of future notifications and consume users' battery, so we need be sure that this the only way to achieve styled notifications in a React Native app and that any unnecessary processing on the user's device is avoided. We already consume way more battery in comparison to similar native apps, so we really need to be careful here IMO.

I tried looking for the UA docs for this styled notification feature but wasn't able to find it, would you mind sharing the link? Maybe that will help me to understand why they solved the problem in this way.

Julesssss · 2021-08-13T20:35:57Z

I found the UA RN docs and looked into the ReactNative codebase -- but it doesn't look like we have a choice, after all 😞 It seems that React native is limited to using the old Services, which the OS will happily throttle.

Given that almost all of my comments have been resolved I'll run the tests again on Monday to see if I can get the notifications to show 👍

Julesssss

The notifications are looking good, here are a few small things I noticed:

I saw one notification icon for each unique chat, rather than one grouped notification with a single icon (NAB, we can fix this later)
Notification deep links are not working as expected. I think this is because we're a bit behind main now. Would you mind merging master so I can test this again, please?
I'm seeing some native exceptions related to the image library, but as the Expensify logo is generated it doesn't seem too critical 🤷‍♂️

Rejecting re-init on previously-failed class java.lang.Class<com.urbanairship.util.ImageUtils$2$1>: java.lang.NoClassDefFoundError: Failed resolution of: Landroid/graphics/ImageDecoder$OnHeaderDecodedListener;
I/zygote64:     at android.graphics.Bitmap com.urbanairship.util.ImageUtils.fetchScaledBitmap(android.content.Context, java.net.URL, int, int) (ImageUtils.java:112)
I/zygote64:     at android.graphics.Bitmap com.expensify.chat.CustomNotificationProvider.lambda$fetchIcon$0$CustomNotificationProvider(java.net.URL, int, int) (CustomNotificationProvider.java:207)
I/zygote64:     at java.lang.Object com.expensify.chat.-$$Lambda$CustomNotificationProvider$QwNE5Q7oUthdUtGtsD4dsFZ4bkc.call() (lambda:-1)
I/zygote64:     at void java.util.concurrent.FutureTask.run() (FutureTask.java:266)
I/zygote64:     at void java.util.concurrent.ThreadPoolExecutor.runWorker(java.util.concurrent.ThreadPoolExecutor$Worker) (ThreadPoolExecutor.java:1162)
I/zygote64:     at void java.util.concurrent.ThreadPoolExecutor$Worker.run() (ThreadPoolExecutor.java:636)
I/zygote64:     at void java.lang.Thread.run() (Thread.java:764)

HorusGoul · 2021-09-09T10:50:07Z

@Julesssss

Oh! I thought we wanted separate notification groups for each conversation. I think it'd be best to implement that after merging this one; the experience on Android right now deserves an improvement.
Done!
Uhmmm, it's either the logo or the user avatars, but they render correctly as far as I can see. If it's something internal, we won't be able to fix it.

Julesssss · 2021-09-09T13:41:59Z

Thanks for merging main. No problem, we can make that grouped notification change later.

I have only ever seen the small Expensify logo when while testing the feature, was I supposed to see user avatars? it might be that whatever image solution UA are using just isn't supported on Android 8 though.

Unfortunately, I see a regression since the main merge. I built a signed release to test the behaviour when the app is completely killed just like yesterday, but today I'm not seeing any notifications display:

HorusGoul · 2021-09-14T09:32:21Z

I haven't been able to reproduce that. Did you change this in Web-Expensify?

Julesssss

Sorry for the delay here!

Looking good now when building a release build and enabling the manual $isInChannel override. I still don't see the fully styled notifications, but this isn't worth blocking the PR. I'll create another issue to investigate whether this is expected for Android 8.1.

Julesssss · 2021-09-22T13:27:00Z

Holding for n6

Julesssss · 2021-10-18T14:01:12Z

Merging as the hold is over

OSBotify · 2021-10-18T14:02:16Z

✋ 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 · 2021-10-23T01:33:54Z

🚀 Deployed to staging by @Julesssss in version: 1.1.8-10 🚀

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

OSBotify · 2021-10-28T20:58:46Z

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

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

dangrous · 2023-02-17T17:49:02Z

To note: This PR I think introduced a bug that we didn't notice for 1.5 years, that the roomName in the notifications had a double hashtag because it was being added on both the backend and the frontend. In the future we should I guess make sure that we're not replicating behavior (unless it's on purpose) on both ends.

If I'm wrong about where that bug came from, reviewers let me know!

HorusGoul added 4 commits July 27, 2021 18:32

Start using the messaging style for the notifications

23d1661

Cache conversations to update update conversations

c3fc83e

Clear cache on notification dismiss

259c993

Add some comments

c00d7d9

HorusGoul self-assigned this Jul 28, 2021

HorusGoul requested a review from roryabraham July 28, 2021 15:11

HorusGoul added 2 commits August 2, 2021 10:25

Merge remote-tracking branch 'origin/main' into horus-implement-messa…

beaf6e0

…ge-type-android-push

Use room name as the conversation title

7534047

HorusGoul changed the title ~~[WIP] Implement message-type Android push notifications~~ Implement message-type Android push notifications Aug 2, 2021

HorusGoul marked this pull request as ready for review August 2, 2021 09:02

HorusGoul requested a review from a team as a code owner August 2, 2021 09:02

MelvinBot requested review from Luke9389 and removed request for a team August 2, 2021 09:03