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 reject the promise when permission is denied #1632

Open
wants to merge 2 commits into
base: rel/5.0.4
Choose a base branch
from

Conversation

Basil-Code
Copy link

@Basil-Code Basil-Code commented Jan 12, 2024

Description

One Line Summary

Fix the requestPermission method when the permission is denied, and resolve ONLY if the permission is granted. Also fixing NullPointerException when attempting to access the data and/or result.getThrowable().getMessage().

Details

Motivation

I faced this issue while implementing the notification permissions, when I deny the permission request the promise is never rejected.

Testing

Manual testing

Tested on Samsung A33

  1. Tested the promise response when the permission is accepted and denied.
  2. Tested changes to the permissions in the app settings and called requestPermission.
  3. Tested changing the permissions within the app by toggling requestPermission and optOut.

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 self-requested a review January 12, 2024 06:50
@nan-li
Copy link
Contributor

nan-li commented Jan 12, 2024

Hi @Basil-Code,

Thanks for submitting this PR.

Fix the requestPermission method when the permission is denied, and resolve ONLY if the permission is granted.

I think this description in the PR changed after your second commit? It looks like these changes are primarily making null checks on result.getData() and result.getThrowable().

Did you encounter these as issues?

@Basil-Code
Copy link
Author

Basil-Code commented Jan 12, 2024

Hi @Basil-Code,

Thanks for submitting this PR.

Fix the requestPermission method when the permission is denied, and resolve ONLY if the permission is granted.

I think this description in the PR changed after your second commit? It looks like these changes are primarily making null checks on result.getData() and result.getThrowable().

Did you encounter these as issues?

Hi @nan-li,
I'm facing NullPointerException crashes on Sentry related to OneSignal, but I'm not sure if it's related to this method yet. But do you think the null checks can cause other issues?

@nan-li
Copy link
Contributor

nan-li commented Feb 12, 2024

Hi @Basil-Code, sorry for the delay.

The null checks you added are good for safety and don't have any side effects, so they look good to me.

Can you tell me more about the NPE crashes you experienced?

I'm facing NullPointerException crashes on Sentry related to OneSignal

Feel free to open an issue with your crashes.

@Basil-Code
Copy link
Author

Hi @Basil-Code, sorry for the delay.

The null checks you added are good for safety and don't have any side effects, so they look good to me.

Can you tell me more about the NPE crashes you experienced?

I'm facing NullPointerException crashes on Sentry related to OneSignal

Feel free to open an issue with your crashes.

Sure, I'll open an issue if the crash still happens.

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