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

Poll for notification permission changes #2112

Merged
merged 7 commits into from
Jun 14, 2024
Merged

Poll for notification permission changes #2112

merged 7 commits into from
Jun 14, 2024

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Jun 6, 2024 •
edited
Loading

Description

One Line Summary

This PR adds polling for notification permission changes so that we can detect changes that happen from outside of the SDK in the current session.

Details

Spawn a new thread where we check notification permission every second. If the permission has changed we fire the permission observer.
This means that we need to track the permission value to know if it has changed.

This is done using an _enabled boolean that gets set when using OneSignal's prompting or when the polling detects a change.

When the app loses focus we change the polling interval to 1 day to have the thread sleep until the app is brought into focus.

Motivation

Currently if an app prompts for permissions without using OneSignal or if the user changes permissions without closing the app (notification center long press on notification), we don't detect those permission changes until the next session begins.

Scope

notification permission prompting

Testing

Unit testing

Added a unit testing file for the PermissionController. Currently has 3 tests around the new polling behavior.

Manual testing

Tested by prompting directly through Android native APIs to test that the SDK detects the change.
Tested that backgrounding that properly sleeps the thread to ensure we aren't polling while the app is not in focus.

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@emawby emawby force-pushed the fix/poll_for_notification_permission branch from a175fec to e8277d7 Compare June 6, 2024 21:16
@emawby emawby force-pushed the fix/poll_for_notification_permission branch from e8277d7 to 63e5cbf Compare June 6, 2024 22:41
@emawby emawby requested a review from jkasten2 June 6, 2024 22:41
@emawby emawby requested a review from jkasten2 June 10, 2024 21:37
jkasten2
jkasten2 requested changes Jun 12, 2024
View reviewed changes
Copy link
Member

@jkasten2 jkasten2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Comments are for if we keep this change the polling rate for the background to a value so high it essentially stops polling.

Still for up for stopping the polling loop and starting a new one when foregrounding if you want to go that route though, as the intent in the code would be more clear.

...va/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt Outdated Show resolved Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt Outdated Show resolved Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt Show resolved Hide resolved
@emawby emawby force-pushed the fix/poll_for_notification_permission branch from 5c28104 to 54bb02d Compare June 13, 2024 17:06
@emawby emawby requested a review from jkasten2 June 13, 2024 17:07
@emawby emawby changed the title WiP Poll for notification permissions Poll for notification permission changes Jun 13, 2024
@emawby emawby changed the title Poll for notification permission changes WiP Poll for notification permission changes Jun 13, 2024
jkasten2
jkasten2 reviewed Jun 13, 2024
View reviewed changes
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.

The implementation itself looks good to me now.

This PR is still noted as WiP, so I'll wait to approve until it's considered complete.

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java Outdated Show resolved Hide resolved
@emawby emawby force-pushed the fix/poll_for_notification_permission branch 2 times, most recently from 5d8d226 to 5b159f7 Compare June 13, 2024 19:55
@emawby
Poll for the permission status every second
b74049b 
We will track the permission state as enabled
and fire the handler when the state changes
Use a constant from the ConfigModel to control the permission polling interval
@emawby
Use a waiter with a lifecycle listener to change the polling period
7b71e1e
@emawby
have background polling interval set to 1 day to effectively pause it
daae10b 
Use a waiter instead of waiterWithValue
@emawby
Create NotificationPermissionControllerTests
4ee0250 
Adding an initial test for the polling behavior
Adding mocking for setting notifications as enabled/disabled in ShadowRoboNotificationManager
Adding foregroundFetchNotificationPermissionInterval to the MockHelper configModel with a value of 1 ms
@emawby
Add tests for pausing and resuming polling based on app focus
8feeda8
@emawby emawby force-pushed the fix/poll_for_notification_permission branch from 5b159f7 to 8feeda8 Compare June 13, 2024 20:14
@emawby emawby changed the title WiP Poll for notification permission changes Poll for notification permission changes Jun 13, 2024
@emawby emawby requested a review from jkasten2 June 13, 2024 20:17
jkasten2
jkasten2 requested changes Jun 13, 2024
View reviewed changes
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.

After looking through the test I have a concern about the default pollingWaitInterval value, left more details on the line of code.

...va/com/onesignal/notifications/internal/permissions/impl/NotificationPermissionController.kt Outdated Show resolved Hide resolved
@emawby emawby requested a review from jkasten2 June 14, 2024 17:46
@emawby
Start with polling interval as the background value
4cb7cb1 
This protects init in the background
onFocus is called immediately if the app is in focus and wakes so we start polling correctly

update unit test to account for this
@emawby emawby force-pushed the fix/poll_for_notification_permission branch from 7104d3a to 4cb7cb1 Compare June 14, 2024 17:51
jkasten2
jkasten2 requested changes Jun 14, 2024
View reviewed changes
Copy link
Member

@jkasten2 jkasten2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

New "NotificationPermissionController permission polling fires permission changed event" test is failing on CI from the last commit. I believe the test needs to be updated to account for the new polling rate default.

jkasten2
jkasten2 approved these changes Jun 14, 2024
View reviewed changes
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.

Test passes locally now for me, so it's most likely good now, but let's wait until CI finishes before merging to be sure.

@emawby emawby merged commit 91fc398 into main Jun 14, 2024
2 checks passed
@emawby emawby deleted the fix/poll_for_notification_permission branch June 14, 2024 19:15
@nan-li nan-li mentioned this pull request Jun 18, 2024
Merged
nan-li added a commit that referenced this pull request Jun 18, 2024
@nan-li
update an override method signature
8930137 
* This code was originally created in #2112
* When making the 5.1.15 release, 2 errors showed up for the `onFocus` method.
* Odd that our PR CI build and unit testing succeeded
@nan-li nan-li mentioned this pull request Jun 18, 2024
Merged
nan-li added a commit that referenced this pull request Jun 18, 2024
@nan-li
update an override method signature
9482a93 
* This code was originally created in #2112
* A parallel PR made a change to the method signature of `onFocus`
This was referenced Jun 21, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

Assignees

@jkasten2 jkasten2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@emawby @jkasten2