-
Notifications
You must be signed in to change notification settings - Fork 368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use two Workers to display notification and send receive receipt #1484
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Turn off data/wifi, send notification, wait a while, turn on data/wifi and receive notification. Then turn off data/wifi shortly after notification comes through, receive receipt is sent with failure due to "device is offline" but Worker is completed successfully regardless of response and won't rerun. I think this is ok, because we haven't accounted for this in the previous 2 implementations anyway (on failure, we don't retry).
To account for the device going offline we can add a constraint that device must be online before the received receipt job can fire.
This can be done with the following code.
Constraints constraints = new Constraints.Builder()
.setRequiredNetworkType(NetworkType.CONNECTED)
.build();
OneTimeWorkRequest onetimeJob = new OneTimeWorkRequest.Builder(YourJob.class)
.setConstraints(constraints).build(); // or PeriodicWorkRequest
Reviewed 18 of 19 files at r1, all commit messages.
Reviewable status: 18 of 19 files reviewed, 2 unresolved discussions (waiting on @jkasten2 and @nan-li)
OneSignalSDK/onesignal/build.gradle, line 51 at r1 (raw file):
implementation 'com.google.android.gms:play-services-base:[17.0.0, 17.6.99]' implementation("androidx.concurrent:concurrent-futures:$androidConcurrentFutures")
Nice, good catch removing a dependency when the code using it was removed.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSUtils.java, line 653 at r1 (raw file):
static int getRandomDelay(int minDelay, int maxDelay) { return (int) Math.round(Math.random() * (maxDelay - minDelay + 1) + minDelay);
I believe this has a bug where the value could be 1 higher than max if Math.random()
is 1 or close enough to it.
Recommend using this instead as it is simpler:
return random.nextInt(max + 1 - min) + min;
5ec456d
to
68f365c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)
68f365c
to
86ebee3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)
- Reverts `NotificationWorker` from being a `ListenableWorker` back to `Worker` - Removed all code related to the ListenableWorker implementation of NotificationWorker
- Add a `ReceiveReceiptWorker` to call `sendReceiveReceipt(notificationId)`
- removed OSDelayTaskController.kt since no longer used now that we use Worker with a delay - removed MockDelayTaskController.java which is used in unit tests - removed other code related to these two classes
- unit tests fail when ReceiveReceiptWorker runs with a delay so make no delay for unit tests - a few unit tests related to sending receive receipts were failing due to "Main looper has queued unexecuted runnables. This might be the cause of the test failure." even after awaiting a long time (flaky pass/fail) - removed test `testNotificationReceivedNoSendReceivedRequest_Delay` because no longer testing the delay in unit tests and other receive receipt tests are sufficient
86ebee3
to
d4458ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @nan-li)
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowReceiveReceiptController.java, line 27 at r4 (raw file):
@Implementation public void beginEnqueueingWork(Context context, String osNotificationId) { if (!isReceiveReceiptEnabled()) {
This isReceiveReceiptEnabled()
check and the inputData
below is duplicated with the code in the actual class.
To clean this up we can make another method in OSReceiveReceiptController
which splits these up so we have to override less.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @nan-li)
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowReceiveReceiptController.java, line 27 at r4 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
This
isReceiveReceiptEnabled()
check and theinputData
below is duplicated with the code in the actual class.
To clean this up we can make another method inOSReceiveReceiptController
which splits these up so we have to override less.
Since this is only to clean up the tests we can differ to another PR.
- unit tests sending receive receipts were failing when the ReceiveReceiptWorker has constraint of `setRequiredNetworkType(NetworkType.CONNECTED)` - they were failing even though a check to `NetworkInfo.isConnected()` returns `true` - so, refactor `buildConstraints` out of the method `beginEnqueueingWork` so it can be shadowed to return no constraints when building the work request for unit tests
552f467
to
d80b60b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowReceiveReceiptController.java, line 27 at r4 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Since this is only to clean up the tests we can differ to another PR.
Nice, abstracted exactly what is different, the constraints!
Description
One Line Summary
Make (1) displaying a notification and (2) sending the receive receipt (with delay) two separate Android
Workers
instead of oneListenableWorker
.Details
Motivation
We were getting reports of duplicated notifications such as in this issue, as well as a few other reports. I was able to reproduce this and confirm the notification is only sent from the dashboard once.
Example of the Duplicated Notification
Background Context
Prior, the
NotificationWorker
was aListenableWorker
. Its work is completed after the receive receipt is sent, for which there is a delay of 0-25 seconds. The work is enqueued only once, but if anything happens before the work is done (during the delay before receive receipt is sent), it will re-start itsstartWork
, which is where the notification is processed and displayed, leading to displaying a duplicated notification on the device.Even prior to that, the
NotificationWorker
was aWorker
, and was changed to aListenableWorker
for the feature of future completion when the receive receipt delay was implemented in the SDK.Scope
Even with testing and trying different scenarios, there can always be edge cases or situations that aren't foreseen and cause a problem.
In terms of receive receipt behavior, I don't believe we are changing the behavior.
Removed the
OSDelayTaskController
andMockDelayTaskController
as these two classes are not needed anymore.The reversion back to using
Worker
instead ofListenableWorker
may also fix #1444 as well.Testing
Unit testing
Three of the existing unit tests for sending receive receipts failed after this change to a delayed
Worker
instead of theOSDelayTaskController
. The receive receive request is not sent (but is queued), leading to unit test failure.After trying a few different solutions with unreliable results:
shadowOf(getMainLooper()).idle()
callsthreadAndTaskWait()
callstime.advanceSystemAndElapsedTimeBy()
The conclusion is to omit the delay, as thus also not test the delay, in unit tests. I am open to other ideas for addressing this problem that lets us have the delay.
In addition, the constraint of
setRequiredNetworkType(NetworkType.CONNECTED)
was causing these same tests to fail as well even though checkingisConnected()
returnstrue
. The methodbeginEnqueueingWork
is shadowed to not use this constraint, and theShadowReceiveReceiptController
is used in all tests related to sending or not sending receive receipts.Removed test:
testNotificationReceivedNoSendReceivedRequest_Delay
Manual Testing
The receive receipt delay is sent after a random delay of 0-25 seconds. For testing only, I bumped up that range to 24-25 seconds.
✅ Ran example app on Pixel Emulator API 30
✅ Ran example app on Pixel Emulator API 31
complete(null)
complete(null)
complete(notif)
complete(notif)
complete(notif)
complete(null)
Also tested manually on Pixel Emulator API 28, 30, 31:
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is