Skip to content
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

Fix ttl and related current time logic #1528

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Feb 3, 2022

Description

One Line Summary

Add ttl and sentTime (and smallIcon) to an OSNotification constructor and change usages of currentThreadTimeMillis (time thread has been active) to currentTimeMillis (epoch time) instead.

Details

Motivation

This PR is related to some features of #1479.

The protected OSNotification(OSNotification notification) constructor was missing ttl and sentTime so that when the NSE is used along with the foreground handler, and the NSE passes on a mutableCopy(), the notification that is processed by the foreground handler next has ttl and sentTime missing, and thus defaulted to 0, which ends up not displaying due to isNotificationWithinTTL() check returning false erroneously.

Unrelated, but smallIcon was also missing so I added it as well.

While making the above changes, I noticed that the currentTimeInSeconds for isNotificationWithinTTL() would be close to zero. When we check isNotificationWithinTTL() to determine if we should display it, we should use currentTimeMillis (milliseconds since epoch) instead of currentThreadTimeMillis (the time a thread has been active) to determine the current time that will be used in the logic. Note that the google.sent_time is also in milliseconds since epoch.

While working on this, I looked at other places that used thread time inaccurately and changed to using currentTimeMillis instead.

Scope

I think using currentThreadTimeMillis was not a problem in the past because it was only the backup value when google.sent_time was not in the payload.

Testing

Unit testing

Added test testNotificationProcessingAndForegroundHandler_callCompleteWithMutableNotification_displays() and handler RemoteNotificationReceivedHandler_notificationReceivedCallCompleteWithMutableNotification. The test NSE calls complete() with a mutableCopy() of the notification that is used by the foreground handler later. We test that the notification will display.

The two tests notShowNotificationPastTTL and shouldSetExpireTimeCorrectlyWhenMissingFromPayload are now failing because they were using thread time. I modified these to use current non-thread time and they now pass.

  • All unit tests pass

Manual testing

Tested using Android emulator Pixel 5 on API 30 with the demo app in the SDK. Sent notification to the device in the foreground and it didn't display before. Now it displays. Also regularly checked values of sentTime, ttl and currentTimeInSeconds throughout testing.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li requested a review from a team February 4, 2022 02:03
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good to me! Made one comment on a test improvement, feel free to make the fix and merge without another approval.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nan-li)


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

   public void testNotificationProcessingAndForegroundHandler_callCompleteWithMutableNotification_displays() throws Exception {
      // 1. Setup correct notification extension service class
      startRemoteNotificationReceivedHandlerService("com.test.onesignal.GenerateNotificationRunner$" +

Recommend using a hard reference to this class so if any code refactor is done in the future this will be a compile error instead of a failed test.

startRemoteNotificationReceivedHandlerService(RemoteNotificationReceivedHandler_notificationReceivedCallCompleteWithMutableNotification.getClass().getCanonicalName());

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.
- `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.
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.
These two tests now fail after moving away from using thread time.
- notShowNotificationPastTTL
- shouldSetExpireTimeCorrectlyWhenMissingFromPayload

Modify the test code to not use thread time
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants