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.

Sign up for GitHub

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

Jump to bottom

Fix ttl and related current time logic #1528

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Fix ttl and related current time logic #1528

merged 4 commits into from
Feb 9, 2022

Commits on Feb 8, 2022

  1. 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.
    @nan-li
    nan-li committed Feb 8, 2022
    Configuration menu
    Copy the full SHA
    3564fca View commit details
    Browse the repository at this point in the history

  2. 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.
    @nan-li
    nan-li committed Feb 8, 2022
    Configuration menu
    Copy the full SHA
    fa5b17c View commit details
    Browse the repository at this point in the history

  3. 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.
    @nan-li
    nan-li committed Feb 8, 2022
    Configuration menu
    Copy the full SHA
    ad2227c View commit details
    Browse the repository at this point in the history

  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
    @nan-li
    nan-li committed Feb 8, 2022
    Configuration menu
    Copy the full SHA
    8dd2cdc View commit details
    Browse the repository at this point in the history