-
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
Check TTL notification to Account for App Standby Buckets #1479
Conversation
ed2de36
to
7663d85
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 5 files at r1, 10 of 14 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java, line 95 at r3 (raw file):
if (notification != null) { boolean display = isStringNotEmpty(notification.getBody()); boolean ttl = isNotificationInsideTTL();
ttl
doesn't read like a boolean type. Could change name to withinTtl
or something else that reflects a bool type?
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java, line 140 at r3 (raw file):
int ttl; if (jsonPayload.has(GOOGLE_TTL_KEY)) {
I would recommending abstracting reading specific keys for HMS vs FCM to be done outside of OSNotificationController
class. If there is not clear way to do this maybe we defer doing so though.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationReceivedEvent.java, line 87 at r3 (raw file):
processNotification(notification); } }, COMPLETE_NOTIFICATION_THREAD).start();
This change isn't related to this PR.
Also I believe @nan-li might be changing this line in a PR so best not to change if not needed.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationWorkManager.java, line 3 at r3 (raw file):
package com.onesignal; import static com.onesignal.OSUtils.isStringNotEmpty;
Reorder doesn't seem to be related to PR.
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowBadgeCountUpdater.java, line 46 at r3 (raw file):
public static void resetStatics() { lastCount = 0;
Badge count seems unrelated to this PR.
OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java, line 30 at r3 (raw file):
package com.test.onesignal; import static com.onesignal.OneSignalPackagePrivateHelper.FCMBroadcastReceiver_onReceived_withIntent;
Is this import order we want?
Also this change and these import don't seem to be related to the PR.
* Notifications where being batches and displayed by android standBy * For cases where notification sent time + ttl is more than the current time, don't display notification
7663d85
to
473b6c7
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.
Reviewable status: 13 of 16 files reviewed, 6 unresolved discussions (waiting on @emawby, @jkasten2, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java, line 95 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
ttl
doesn't read like a boolean type. Could change name towithinTtl
or something else that reflects a bool type?
What do you think about isNotificationWithinTTL?
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java, line 140 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
I would recommending abstracting reading specific keys for HMS vs FCM to be done outside of
OSNotificationController
class. If there is not clear way to do this maybe we defer doing so though.
Agree!
OneSignalSDK/unittest/src/test/java/com/onesignal/ShadowBadgeCountUpdater.java, line 46 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Badge count seems unrelated to this PR.
Actually is needed for tests to be independent, the count wasn't being reset, so new tests were failing even though updateCount was not being called, this is because the lastCount was not being reset.
OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java, line 30 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Is this import order we want?
Also this change and these import don't seem to be related to the PR.
Oh, this has been done by the AS. Since I always clean the imports of the file I have been using and this seems like the format the AS has by default. I can go back to the way it was. Let me know if you agree to keep it this way or to go back to avoid conflicts with the current work maybe @nan-li is doing.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationReceivedEvent.java, line 87 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
This change isn't related to this PR.
Also I believe @nan-li might be changing this line in a PR so best not to change if not needed.
Done.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationWorkManager.java, line 3 at r3 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Reorder doesn't seem to be related to PR.
Done.
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.
small optional nit left on naming. Feel free to change and merge in without requiring approval on that specific change.
Reviewed 1 of 3 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSNotificationController.java, line 95 at r3 (raw file):
Previously, Jeasmine (JNahui) wrote…
What do you think about isNotificationWithinTTL?
ah yes, isNotificationWithinTTL
sounds more correct than isNotificationInsideTTL
. However I was talking about boolean ttl
here, thinking it should be boolean withinTtl
.
fb2c8c6
to
e8e806d
Compare
e8e806d
to
3800e63
Compare
Description
Details
One-line Summary
Notifications were being shown independent of the TTL due to Android App Standby Buckets
Motivation
App Standby Buckets avoid users being spammed by unused applications. If notification is not set with high priority and the app is marked as not used, the notification will be received by the app, but showed the next time the user opens it, this is not the same as restoring, since they will vibrate and behave as they were just received. Because of that we need to add a TTL check before display logic.
Scope
Notification complete logic now checks if notification is between TTL time.
HMS and GCM tests for TTL have been added
Background context
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is