From 3564fca35b658f586909343e7b345319e4f9a8ba Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 1 Feb 2022 01:29:58 -0800 Subject: [PATCH 1/4] Unit test for display notif with NSE and foreground handler When the NSE calls `complete` with `notification.mutableCopy()` rather than the original notification, and the foreground handler is also set, the notification will not display. This test is added to replicate that behavior. We will address the bug in the following commit to get the test to pass. --- .../onesignal/GenerateNotificationRunner.java | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java b/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java index 3d85a84ca..4f34ade14 100644 --- a/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java +++ b/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java @@ -1983,6 +1983,64 @@ public void remoteNotificationReceived(Context context, OSNotificationReceivedEv } } + @Test + @Config(shadows = { ShadowGenerateNotification.class }) + public void testNotificationProcessingAndForegroundHandler_callCompleteWithMutableNotification_displays() throws Exception { + // 1. Setup correct notification extension service class + startRemoteNotificationReceivedHandlerService( + RemoteNotificationReceivedHandler_notificationReceivedCallCompleteWithMutableNotification + .class + .getName() + ); + + // 2. Init OneSignal + OneSignal.setAppId("b2f7f966-d8cc-11e4-bed1-df8f05be55ba"); + OneSignal.initWithContext(blankActivity); + OneSignal.setNotificationWillShowInForegroundHandler(notificationReceivedEvent -> { + lastForegroundNotificationReceivedEvent = notificationReceivedEvent; + + // Call complete to end without waiting default 30 second timeout + notificationReceivedEvent.complete(notificationReceivedEvent.getNotification()); + }); + threadAndTaskWait(); + + blankActivityController.resume(); + threadAndTaskWait(); + + // 3. Receive a notification in foreground + FCMBroadcastReceiver_processBundle(blankActivity, getBaseNotifBundle()); + threadAndTaskWait(); + + // 4. Make sure service was called + assertNotNull(lastServiceNotificationReceivedEvent); + + // 5. Make sure foreground handler was called + assertNotNull(lastForegroundNotificationReceivedEvent); + + // 6. Make sure running on main thread check is called, this is only called for showing the notification + assertTrue(ShadowGenerateNotification.isRunningOnMainThreadCheckCalled()); + + // 7. Check badge count to represent the notification is displayed + assertEquals(1, ShadowBadgeCountUpdater.lastCount); + } + + /** + * @see #testNotificationProcessingAndForegroundHandler_callCompleteWithMutableNotification_displays + */ + public static class RemoteNotificationReceivedHandler_notificationReceivedCallCompleteWithMutableNotification implements OneSignal.OSRemoteNotificationReceivedHandler { + + @Override + public void remoteNotificationReceived(final Context context, OSNotificationReceivedEvent receivedEvent) { + lastServiceNotificationReceivedEvent = receivedEvent; + + OSNotification notification = receivedEvent.getNotification(); + OSMutableNotification mutableNotification = notification.mutableCopy(); + + // Complete is called to end NotificationProcessingHandler + receivedEvent.complete(mutableNotification); + } + } + @Test @Config(shadows = { ShadowGenerateNotification.class }) public void testNotificationWillShowInForegroundHandlerIsCallWhenReceivingNotificationInForeground() throws Exception { From fa5b17cb380e7b24dd61bd40f7fc79fcd79b38cf Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 1 Feb 2022 01:35:11 -0800 Subject: [PATCH 2/4] Add missing properties in `OSNotification` constructor - `OSNotification(OSNotification notification)` is used as the constructor for `OSMutableNotification` - `ttl` and `sentTime` were missing from this constructor so that these values defaulted to 0, leading to some problems with determining ttl logic later. - Unrelated to what this PR is going to address, but also added `smallIcon` here since that was also missing. --- .../onesignal/src/main/java/com/onesignal/OSNotification.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java index e607d333c..f9efd4061 100644 --- a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java +++ b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java @@ -117,6 +117,7 @@ protected OSNotification(OSNotification notification) { this.title = notification.title; this.body = notification.body; this.additionalData = notification.additionalData; + this.smallIcon = notification.smallIcon; this.largeIcon = notification.largeIcon; this.bigPicture = notification.bigPicture; this.smallIconAccentColor = notification.smallIconAccentColor; @@ -132,6 +133,8 @@ protected OSNotification(OSNotification notification) { this.collapseId = notification.collapseId; this.priority = notification.priority; this.rawPayload = notification.rawPayload; + this.sentTime = notification.sentTime; + this.ttl = notification.ttl; } private void initPayloadData(JSONObject currentJsonPayload) { From ad2227c8249f7cf26ac9d86d9fec88fa36ba6667 Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 1 Feb 2022 01:46:25 -0800 Subject: [PATCH 3/4] Use `currentTime` instead of `currentThreadTime` When we check `isNotificationWithinTTL` to determine if we should display it, we should use `currentTimeMillis` (milliseconds since epoch) instead of `currentThreadTimeMillis` to determine the current time that will be used in the logic. The google sent_time is also in milliseconds since epoch. During some testing, `currentThreadTimeMillis` is equivalent to almost 0 seconds due to it being the time a thread has been active. While working on this, I looked at other places that used thread time inaccurately and changed to using `currentTimeMillis` instead. --- .../main/java/com/onesignal/NotificationBundleProcessor.java | 2 +- .../onesignal/src/main/java/com/onesignal/OSNotification.java | 2 +- .../src/main/java/com/onesignal/OSNotificationController.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java b/OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java index 026cc7f32..b579451e3 100644 --- a/OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java +++ b/OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java @@ -236,7 +236,7 @@ private static void saveNotification(OSNotificationGenerationJob notificationJob values.put(NotificationTable.COLUMN_NAME_MESSAGE, notificationJob.getBody().toString()); // Set expire_time - long sentTime = jsonPayload.optLong(OSNotificationController.GOOGLE_SENT_TIME_KEY, OneSignal.getTime().getCurrentThreadTimeMillis()) / 1_000L; + long sentTime = jsonPayload.optLong(OSNotificationController.GOOGLE_SENT_TIME_KEY, OneSignal.getTime().getCurrentTimeMillis()) / 1_000L; int ttl = jsonPayload.optInt(OSNotificationController.GOOGLE_TTL_KEY, OSNotificationRestoreWorkManager.DEFAULT_TTL_IF_NOT_IN_PAYLOAD); long expireTime = sentTime + ttl; values.put(NotificationTable.COLUMN_NAME_EXPIRE_TIME, expireTime); diff --git a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java index f9efd4061..b2cb7082b 100644 --- a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java +++ b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotification.java @@ -146,7 +146,7 @@ private void initPayloadData(JSONObject currentJsonPayload) { return; } - long currentTime = OneSignal.getTime().getCurrentThreadTimeMillis(); + long currentTime = OneSignal.getTime().getCurrentTimeMillis(); if (currentJsonPayload.has(GOOGLE_TTL_KEY)) { sentTime = currentJsonPayload.optLong(GOOGLE_SENT_TIME_KEY, currentTime) / 1_000; ttl = currentJsonPayload.optInt(GOOGLE_TTL_KEY, OSNotificationRestoreWorkManager.DEFAULT_TTL_IF_NOT_IN_PAYLOAD); diff --git a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java index 04cd0b3e9..4b43ff95a 100644 --- a/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java +++ b/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java @@ -125,7 +125,7 @@ public boolean isNotificationWithinTTL() { if (!useTtl) return true; - long currentTimeInSeconds = OneSignal.getTime().getCurrentThreadTimeMillis() / 1_000; + long currentTimeInSeconds = OneSignal.getTime().getCurrentTimeMillis() / 1_000; long sentTime = notificationJob.getNotification().getSentTime(); // If available TTL times comes in seconds, by default is 3 days in seconds int ttl = notificationJob.getNotification().getTtl(); From 8dd2cdcbe31bbf82f04b2d6110738c66eca962e4 Mon Sep 17 00:00:00 2001 From: Nan Date: Thu, 3 Feb 2022 10:33:17 -0800 Subject: [PATCH 4/4] Modify now failing tests after changes These two tests now fail after moving away from using thread time. - notShowNotificationPastTTL - shouldSetExpireTimeCorrectlyWhenMissingFromPayload Modify the test code to not use thread time --- .../java/com/test/onesignal/GenerateNotificationRunner.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java b/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java index 4f34ade14..5365a1a56 100644 --- a/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java +++ b/OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java @@ -1252,7 +1252,7 @@ public void shouldSetExpireTimeCorrectlyFromGoogleTTL() throws Exception { @Test @Config (sdk = 23, shadows = { ShadowGenerateNotification.class }) public void notShowNotificationPastTTL() throws Exception { - long sentTime = time.getCurrentThreadTimeMillis(); + long sentTime = time.getCurrentTimeMillis(); long ttl = 60L; Bundle bundle = getBaseNotifBundle(); @@ -1260,7 +1260,7 @@ public void notShowNotificationPastTTL() throws Exception { bundle.putLong(OneSignalPackagePrivateHelper.GOOGLE_TTL_KEY, ttl); // Go forward just past the TTL of the notification - time.advanceThreadTimeBy(ttl + 1); + time.advanceSystemTimeBy(ttl + 1); NotificationBundleProcessor_ProcessFromFCMIntentService(blankActivity, bundle); threadAndTaskWait(); @@ -1275,7 +1275,7 @@ public void shouldSetExpireTimeCorrectlyWhenMissingFromPayload() throws Exceptio threadAndTaskWait(); long expireTime = (Long)TestHelpers.getAllNotificationRecords(dbHelper).get(0).get(NotificationTable.COLUMN_NAME_EXPIRE_TIME); - assertEquals((SystemClock.currentThreadTimeMillis() / 1_000L) + 259_200, expireTime); + assertEquals((System.currentTimeMillis() / 1_000L) + 259_200, expireTime); } // TODO: Once we figure out the correct way to process notifications with high priority using the WorkManager