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

feat(messaging:) use prominent iOS push notification permission #3075

Closed
wants to merge 1 commit into from

Conversation

@anfriis
Copy link

anfriis commented Jan 7, 2020

This PR solves the "Provisional push authorization should be optional on iOS" post on the Feedback site: https://invertase.io/oss/react-native-firebase/feedback/

The requestPermissionWithoutProvisional() requests push permission for iOS without the UNAuthorizationOptionProvisional auth option set, showing the native permission alert to the user.
The current requestPermission() has no option for leaving out the provisional auth option.

Summary

It is currently not possible to explicitly ask for push permission for iOS 12+, because the UNAuthorizationOptionProvisional flag is set making push notifications silent for the user until the user actively accepts prominent permission (more info on Provisional Authorization here). In some cases, e.g. the commercial app I am currently working on, it is a requirement to explicitly ask for push permission.
This PR adds the possibility to explicitly ask for push permission with requestPermissionWithoutProvisional() without changing the current functionality of requestPermission().

Checklist

  • Supports Android (doesn't affect Android)
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

The "<app name> Would Like To Send You Notifications" permission alert is shown on iOS when calling requestPermissionWithoutProvisional(). When calling requestPermission() instead no alert is shown on iOS 12+ since it has the UNAuthorizationOptionProvisional auth option set.

Release Plan

[IOS] [ENHANCEMENT] {Messaging} - Add prominent push notification permission option to Messaging

This function requests push permission for iOS without the `UNAuthorizationOptionProvisional` auth option set, showing the native permission alert to the user.
The current `requestPermission()` has no option for leaving out the provisional auth option
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #3075 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3075      +/-   ##
==========================================
- Coverage   89.83%   89.75%   -0.07%     
==========================================
  Files         109      109              
  Lines        3381     3384       +3     
==========================================
  Hits         3037     3037              
- Misses        344      347       +3
@anfriis

This comment has been minimized.

Copy link
Author

anfriis commented Jan 7, 2020

I don't know what's going on with the claassistantio, but I have had my email address verified on my GitHub profile for ages and I have signed the agreement several times now.

@alittletf

This comment has been minimized.

Copy link

alittletf commented Jan 9, 2020

I don't know what the process is for merging new PR's but I would love to have this in my app. The default 'deliver quietly' notification by Apple sucks

@alexfoxy

This comment has been minimized.

Copy link

alexfoxy commented Jan 9, 2020

This is a key feature, currently it feels sort of broken, notifications are less useful if the user isn't notified!

Love to see this merged.

@mikehardy

This comment has been minimized.

Copy link
Collaborator

mikehardy commented Jan 9, 2020

out of curiosity - has anyone attempted to workaround via use of react-native-permissions?

@anfriis

This comment has been minimized.

Copy link
Author

anfriis commented Jan 11, 2020

@mikehardy No but it seems like react-native-permissions would do the trick as well, but I think i's a bit unnecessary to include a new dependency for just omitting the provisional flag. So I just patched my changes in along with the authorizationStatus check which I changed to be == UNAuthorizationStatusAuthorized instead of >= UNAuthorizationStatusAuthorized.
I didn't include that in this PR though

@mikehardy

This comment has been minimized.

Copy link
Collaborator

mikehardy commented Jan 11, 2020

The reason I was curious is that it seems permissions are a sore point for the library in general, and as they change from version to version of iOS and Android, it might make sense for the library in general to say "look, there's another library with laser focus on permissions that does the whole thing right and tracks changes, we should delegate to them completely for permissions". So it's material to know if react-native-permissions already handles this or not because it would be (for me) another data point saying yes, delegate.

Given the complexity of several of the permissions changes in iOS13 it's not in my opinion unnecessary to add a dependency that handles permissions well, I think it's actually the right direction, as I feel the primary problem this library has is constrained developer resources to track changes, but that's just opinion.

@Salakar Salakar added this to the v6.3.0 milestone Jan 13, 2020
@anfriis

This comment has been minimized.

Copy link
Author

anfriis commented Jan 14, 2020

I see your point @mikehardy and it makes completely sense to delegate this logic to react-native-permissions at some point.
However until that happens I think it would be great to have this option merged in, so you still get the swizzling functionality out of the box. Else there should be some documentation on how to do this using third party libraries and RNFB if that's the way to go.

@alittletf

This comment has been minimized.

Copy link

alittletf commented Jan 15, 2020

@anfriis @mikehardy @alexfoxy I ended up going to route of react-native-permissions and it works perfect. Of course I totally agree with @anfriis in not wanting an unneeded dependency since this is my only use for rn permissions but the installation was very easy. If this does get merged or solved in another way I will remove rn permissions and utilize the messaging package

@andrewbeckman

This comment has been minimized.

Copy link

andrewbeckman commented Feb 1, 2020

Hi @Salakar, I noticed you added this to the v6.3.0 milestone. That is super exciting. I am trying to schedule work for my team and I was wondering if you had any estimate on when a release will go out with this PR?

@Salakar Salakar modified the milestones: v6.3.0, v6.4.0 Feb 4, 2020
@ottob ottob mentioned this pull request Feb 7, 2020
5 of 5 tasks complete
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 4, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Anders Friis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Salakar

This comment has been minimized.

Copy link
Member

Salakar commented Mar 25, 2020

Quick note that #3339 (RC is also available on NPM - 6.4.0-rc2) now supersedes this, specifically these change log entries;

  • [FEAT] The permissions API has been upgraded to now support custom permissions. The permission API selects sensible defaults however allows you to fully customise them if required. Please note, provisional permissions is now disabled by default (previously, it was enabled by default for iOS 12+ devices):
// Default permissions:
await messaging().requestPermission({
  alert: true,
  announcement: false,
  badge: true,
  carPlay: true,
  provisional: false,
  sound: true,
});
  • [FEAT] The requestPermission & hasPermission API now returns the current authorization status, rather than a boolean value. This generally won't break existing apps as any "truthy" logic checks should still work:
const permission = await messaging().requestPermission();
// or const permission = await messaging().hasPermission();

if (permission === IOSAuthorizationStatus.NOT_DETERMINED) {
  // -1 =  No permission has been requested yet
}

if (permission === IOSAuthorizationStatus.DENIED) {
  // 0 =  User declined permission. They must enable permissions from the app settings
}

if (permission === IOSAuthorizationStatus.AUTHORIZED) {
  // 1 =  User granted permission
}

if (permission === IOSAuthorizationStatus.PROVISIONAL) {
  // 2 =  User accepted provisional permission
}
@Salakar Salakar changed the title Add prominent iOS push notification permission option to Messaging feat(messaging:) use prominent iOS push notification permission Mar 25, 2020
@Salakar

This comment has been minimized.

Copy link
Member

Salakar commented Mar 27, 2020

Closing as #3339 which supersedes this has now been merged and is available on an RC release (6.4.0-rc4).

Thank you for your contribution

@Salakar Salakar closed this Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.