react-native-onesignal icon indicating copy to clipboard operation
react-native-onesignal copied to clipboard

Fix requestPermission to reject the promise when permission is denied

Open Basil-Code opened this issue 2 years ago • 4 comments

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

  • [x] Notifications
    • [ ] Display
    • [ ] Open
    • [ ] Push Processing
    • [ ] Confirm Deliveries
  • [ ] Outcomes
  • [ ] Sessions
  • [ ] In-App Messaging
  • [ ] REST API requests
  • [ ] Public API changes

Checklist

Overview

  • [x] I have filled out all REQUIRED sections above
  • [x] PR does one thing
  • [x] Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • [x] I have included test coverage for these changes, or explained why they are not needed
  • [x] All automated tests pass, or I explained why that is not possible
  • [x] I have personally tested this on my device, or explained why that is not possible

Final pass

  • [x] Code is as readable as possible.
  • [x] I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

Basil-Code avatar Jan 12 '24 03:01 Basil-Code

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?

nan-li avatar Jan 12 '24 06:01 nan-li

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?

Basil-Code avatar Jan 12 '24 16:01 Basil-Code

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.

nan-li avatar Feb 12 '24 18:02 nan-li

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.

Basil-Code avatar Feb 16 '24 19:02 Basil-Code