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

[5.0.0] Fixes to detecting native permissions #1229

Merged

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Feb 17, 2023

Description

One Line Summary

Fixes to sending permission change information and notification types to the user module.

Details

Motivation

The SDK was behaving oddly and getting into states where the enabled flag does not match the notification_types when an update subscription request is sent to the server. This happens when permissions are changed on the operating system level, and then returning to the app. It also seems like when returning to the app before these changes, we get the notification settings but too early and it contains the old information. These PR changes seem to remedy the problem mentioned.

Scope

  • Simplify by passing notification types only to user manager, instead of also accounting for permission observer changes firing or passing the device's reachable property separately.
  • Notification types changes will drive other changes on the subscription model, like setting reachable, optedIn, and enabled.

Known Issues

  • The app has notification permission and optOut is called. The push subscription observer is not fired in this case.
  • The app does not have notification permission and optOut was also called. Calling optIn does not change the notification_types from -2. This does not effect behavior.

Testing

Manual testing

Example app with iPhone 13 on iOS 16.

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 changed the base branch from main to 5.0.0/onesignal_wrapper February 17, 2023 10:35
* Simplify by passing notification types to user manager, instead of also accounting for permission observer changes and setting the `reachable` property separately.
* Notification types changes will drive other changes on the subscription model, like setting reachable, optedin, and enabled.
@nan-li nan-li force-pushed the 5.0.0/detecting_native_permission_changes branch from b5aee76 to d9b816f Compare February 17, 2023 10:49
@nan-li nan-li changed the base branch from 5.0.0/onesignal_wrapper to major_release_5.0.0 February 17, 2023 21:08
@nan-li nan-li changed the base branch from major_release_5.0.0 to 5.0.0/onesignal_wrapper February 17, 2023 21:08
Base automatically changed from 5.0.0/onesignal_wrapper to major_release_5.0.0 February 17, 2023 21:10
@nan-li nan-li merged commit 454856a into major_release_5.0.0 Feb 17, 2023
@nan-li nan-li deleted the 5.0.0/detecting_native_permission_changes branch February 17, 2023 21:11
nan-li added a commit that referenced this pull request Oct 30, 2023
…sion_changes

[5.0.0] Fixes to detecting native permissions
nan-li added a commit that referenced this pull request Oct 30, 2023
…sion_changes

[5.0.0] Fixes to detecting native permissions
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