Add Receive Receipt delay

[Jeasmine]

• Add random delay from 0 to 25 seconds
• Add OSDelayTaskController for future delays
• Add MockDelayTaskController for test approaches
• Refactor Notification Work Manager to extends ListenableWorker. This adds future completion. Complete future after sending receive receipts
• Refactor Restoration Work Manager to not init another work manager, since Notification Work Manager nows expects a future when Restoration work is done, it will finish notification work manager that will end on a FutureGarbageCollectedException, avoid this exception by running everything on the same work thread

Note:

HMS and ADM handling always ends calling startNotificationProcessing which ends using OSNotificationWorkManager worker thread

---

This change is 

[Dash63]

nice meet

[jkasten2]

Most comments i left on specific lines however in addition the testshouldSendReceivedReceiptWhenEnabled is failing on TravisCI and needs to be updated for the changes made in this PR.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jeasmine)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSDelayTaskController.kt, line 9 at r1 (raw file):
I think we will probably need to use the WorkManager for this implementation. This could be started from an onRecieved of a BroadcastReceiver.

See this Android documentation on BroadcastReceiver behavior that applies here:
https://developer.android.com/guide/components/broadcasts#security-and-best-practices

Because a receiver's onReceive(Context, Intent) method runs on the main thread, it should execute and return quickly. If you need to perform long running work, be careful about spawning threads or starting background services because the system can kill the entire process after onReceive() returns. For more information, see Effect on process state To perform long running work, we recommend:

• Calling goAsync() in your receiver's onReceive() method and passing the BroadcastReceiver.PendingResult to a background thread. This keeps the broadcast active after returning from onReceive(). However, even with this approach the system expects you to finish with the broadcast very quickly (under 10 seconds). It does allow you to move work to another thread to avoid glitching the main thread.
  Scheduling a job with the JobScheduler. For more information, see Intelligent Job Scheduling.

Due note that a OneSignal notification is only fully displayed and process from a onRecieved only if there isn't a NSE setup and no remote images to pull down. In those cases we already use the WorkManager I believe but we need to ensure we only mark the work done once we send the confirmed delivery network call.

---

OneSignalSDK/unittest/src/test/java/com/onesignal/MockDelayTaskController.java, line 3 at r1 (raw file):

package com.onesignal;

import org.jetbrains.annotations.NotNull;

Use AndroidX's NonNull instead.

---

OneSignalSDK/unittest/src/test/java/com/test/onesignal/MainOneSignalClassRunner.java, line 1353 at r1 (raw file):

   @Test
   @Config(shadows = { ShadowGenerateNotification.class })
   public void testNotificationReceivedNoSendReceivedRequest_Delay() throws Exception {

This new test is failing on TravisCI
https://travis-ci.com/github/OneSignal/OneSignal-Android-SDK/builds/228260020#L2494-L2519

[Jeasmine]

Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @Jeasmine and @jkasten2)

---

OneSignalSDK/unittest/src/test/java/com/onesignal/MockDelayTaskController.java, line 3 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Use AndroidX's NonNull instead.

This object is outside the main and test package it doesn't have android x access, Should we add android x dependency to mock testing files?

---

OneSignalSDK/unittest/src/test/java/com/test/onesignal/MainOneSignalClassRunner.java, line 1353 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This new test is failing on TravisCI
https://travis-ci.com/github/OneSignal/OneSignal-Android-SDK/builds/228260020#L2494-L2519

Yeah I was still working on some problem with thee main looper, this is fixed, but will be working on the work manager change

[Jeasmine]

Reviewable status: 6 of 8 files reviewed, 3 unresolved discussions (waiting on @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSDelayTaskController.kt, line 9 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

I think we will probably need to use the WorkManager for this implementation. This could be started from an onRecieved of a BroadcastReceiver.

See this Android documentation on BroadcastReceiver behavior that applies here:
https://developer.android.com/guide/components/broadcasts#security-and-best-practices

Because a receiver's onReceive(Context, Intent) method runs on the main thread, it should execute and return quickly. If you need to perform long running work, be careful about spawning threads or starting background services because the system can kill the entire process after onReceive() returns. For more information, see Effect on process state To perform long running work, we recommend:

• Calling goAsync() in your receiver's onReceive() method and passing the BroadcastReceiver.PendingResult to a background thread. This keeps the broadcast active after returning from onReceive(). However, even with this approach the system expects you to finish with the broadcast very quickly (under 10 seconds). It does allow you to move work to another thread to avoid glitching the main thread.
  Scheduling a job with the JobScheduler. For more information, see Intelligent Job Scheduling.

Due note that a OneSignal notification is only fully displayed and process from a onRecieved only if there isn't a NSE setup and no remote images to pull down. In those cases we already use the WorkManager I believe but we need to ensure we only mark the work done once we send the confirmed delivery network call.

Totally right!

[jkasten2]

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jeasmine and @jkasten2)

---

OneSignalSDK/unittest/src/test/java/com/onesignal/MockDelayTaskController.java, line 3 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

This object is outside the main and test package it doesn't have android x access, Should we add android x dependency to mock testing files?

androidx.annotation.NonNull should work here. This file is in the unittest folder which uses the unittest/build.gradle which has implementation(project(':onesignal')) which will includes all it's dependencies.

[jkasten2]

Reviewable status: 6 of 18 files reviewed, 7 unresolved discussions (waiting on @Jeasmine and @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSDelayTaskController.kt, line 18 at r3 (raw file):

    }

    protected open fun getRandomNumber(): Int {

I think getRandomDelay as a method name is more fitting here.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationRestoreWorkManager.java, line 91 at r3 (raw file):

            OneSignalDbHelper dbHelper,
            StringBuilder dbQuerySelection,
            boolean needsWorkerThread) {

It looks like we only call this method in one palace and needsWorkerThread is always false here.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationWorkManager.java, line 17 at r3 (raw file):

import com.google.common.util.concurrent.ListenableFuture;

import org.jetbrains.annotations.NotNull;

This should be AndroidX's NonNull instead.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSReceiveReceiptController.java, line 89 at r3 (raw file):

        OneSignal.Log(OneSignal.LOG_LEVEL.DEBUG, "Receive receipt ending with success callback completer: " + callbackCompleter);
        if (callbackCompleter != null)
            callbackCompleter.set(ListenableWorker.Result.success());

I am not sure I understand moving to a WorkerListener from a Worker gives us here?
This receive receipt network call before we finish and this worker result could be set before a notification is even displayed. In the case where the random delay time is for the received receipt and the notification has an remote image that takes a while to download.

To solve this we need to wait for both events to complete before we count the Worker as complete. (In JS land thinking of received receipt and notification display as two Promises and we need to await for both to complete.)

---

OneSignalSDK/unittest/src/test/java/com/onesignal/MockDelayTaskController.java, line 3 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

androidx.annotation.NonNull should work here. This file is in the unittest folder which uses the unittest/build.gradle which has implementation(project(':onesignal')) which will includes all it's dependencies.

In most projects any references you use directly in a project should be added to it, so you don't rely on a sub-dependency that could change. However since this unittest project is a superset of the onesignal project it really should always contain at least those dependencies.
Anyway, so I am saying if AndroidX's annotations are being used in the main project they should always be accessible to tests.

[Jeasmine]

Reviewable status: 6 of 18 files reviewed, 7 unresolved discussions (waiting on @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSDelayTaskController.kt, line 18 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

I think getRandomDelay as a method name is more fitting here.

Done.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationRestoreWorkManager.java, line 91 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

It looks like we only call this method in one palace and needsWorkerThread is always false here.

Yeah, I wanted to make it parametrized for the future, but it can be simplified for now

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationWorkManager.java, line 17 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This should be AndroidX's NonNull instead.

Done.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSReceiveReceiptController.java, line 89 at r3 (raw file):

WorkerListener
Worker Listener is added to ensure that the received receipt call is done and avoid being finished by the operating system. I have checked that the show notification is done before receive receipt is called. The method processJobForDisplay handles all the notification construction and image pulling before calling processNotification.

processNotification also is in charge of saving the notification on the DB before OSReceiveReceiptController is called. So ReceiveReceip is the last thing called under the notifications building. Are the notification images being handle in another thread?

The only bitmap networking call I see is the following.

      try {
         return BitmapFactory.decodeStream(new URL(location).openConnection().getInputStream());
      } catch (Throwable t) {
         OneSignal.Log(OneSignal.LOG_LEVEL.WARN, "Could not download image!", t);
      }

      return null;

And this is handle in the same thread. Let me know If I'm missing something

---

OneSignalSDK/unittest/src/test/java/com/onesignal/MockDelayTaskController.java, line 3 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

In most projects any references you use directly in a project should be added to it, so you don't rely on a sub-dependency that could change. However since this unittest project is a superset of the onesignal project it really should always contain at least those dependencies.
Anyway, so I am saying if AndroidX's annotations are being used in the main project they should always be accessible to tests.

I was confusing NotNull with Nonnull 🤦 this is done!

[jkasten2]

Reviewed 3 of 12 files at r3.
Reviewable status: 8 of 18 files reviewed, 3 unresolved discussions (waiting on @Jeasmine and @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSReceiveReceiptController.java, line 89 at r3 (raw file):

Previously, Jeasmine (JNahui) wrote…

WorkerListener
Worker Listener is added to ensure that the received receipt call is done and avoid being finished by the operating system. I have checked that the show notification is done before receive receipt is called. The method processJobForDisplay handles all the notification construction and image pulling before calling processNotification.

processNotification also is in charge of saving the notification on the DB before OSReceiveReceiptController is called. So ReceiveReceip is the last thing called under the notifications building. Are the notification images being handle in another thread?

The only bitmap networking call I see is the following.

      try {
         return BitmapFactory.decodeStream(new URL(location).openConnection().getInputStream());
      } catch (Throwable t) {
         OneSignal.Log(OneSignal.LOG_LEVEL.WARN, "Could not download image!", t);
      }

      return null;

And this is handle in the same thread. Let me know If I'm missing something

ah my mistake, thought the notification generation and / or downloading was done on its own thread.

Just to confirm the sequence here:

1. notification is generated (image is downloaded if needed)
2. scheduledThreadPoolExecutor is used to schedule the confirmed delivery 0 to 25 seconds in the future
3. Confirmed Delivery network completes and callbackCompleter.set() is called

[Jeasmine]

Reviewable status: 8 of 18 files reviewed, 3 unresolved discussions (waiting on @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSReceiveReceiptController.java, line 89 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

ah my mistake, thought the notification generation and / or downloading was done on its own thread.

Just to confirm the sequence here:

1. notification is generated (image is downloaded if needed)
2. scheduledThreadPoolExecutor is used to schedule the confirmed delivery 0 to 25 seconds in the future
3. Confirmed Delivery network completes and callbackCompleter.set() is called

That sequence is correct. For cases we don't send receive receipt, the callbackCompleter.set() is also called

[jkasten2]

Reviewed 5 of 12 files at r3, 3 of 5 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jkasten2)

[jkasten2]

Reviewed 6 of 6 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)