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 requestPermission to return false when permission is denied #1591

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Oct 24, 2023

Description

One Line Summary

Return the correct boolean for the requestPermission method when permission denied, and resolve correctly if permission is already granted.

Details

Motivation

This was reported by a GitHub issue and I was able to reproduce. This method was always returning true previously on denial.

In addition, we were not resolving if permission is already granted due to the Android implementation of suspend functions.

Scope

  • We used to return result.isSuccess() which always returns true because this is whether the coroutine call was successful.
  • Instead return result.getData() which holds the value returned by the coroutine.
  • If the coroutine is not successful, we get the Throwable's message and pass that along.
  • Also add a button to get permission native in example app to facilitate testing.
  • Now, we also resolve correctly when permission has already been granted.

Testing

Unit testing

None

Manual testing

Android emulator API 33

  • Tested the boolean returned when permission is accepted and denied and it returns true and false respectively.
  • Tested combinations of toggling permissions in app settings and calls to requestPermission.

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

* result.isSuccess() always returns true because this is Whether the coroutine call was successful
* result.getData() holds the value
* Also fix syntax for requestPermission
Problem:
If permission is already enabled, the native call to `OneSignal.getNotifications().requestPermission(fallbackToSettings, Continue.with(...)` never suspends and the Continuation code block never runs. As a result, we would not be able to resolve the promise over the bridge.

Solution:
Before calling that method, do a permission check and return true early.
@nan-li nan-li force-pushed the fix/request_permission_deny branch from fe96e72 to 9680d81 Compare November 6, 2023 21:48
@nan-li nan-li merged commit f410b06 into major_release_5.0.0 Nov 9, 2023
4 of 5 checks passed
@jennantilla jennantilla mentioned this pull request Nov 9, 2023
@nan-li nan-li deleted the fix/request_permission_deny branch March 16, 2024 01:45
@nan-li nan-li restored the fix/request_permission_deny branch March 16, 2024 01:45
@nan-li nan-li deleted the fix/request_permission_deny branch March 16, 2024 01:46
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.

3 participants