Skip to content

requestAuthorizationWithOptions before registerForRemoteNotifications #3017

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

Conversation

cark1
Copy link

@cark1 cark1 commented Dec 17, 2019

Summary

Fixes #2657

This pull request solves the problems with request permission and registerForRemoteNotifications in messaging module.
When these methods are called, they simply fail without throwing any warnings or errors. Because of this, it was impossible to get the FCMToken.

The problem was that [[UIApplication sharedApplication] registerForRemoteNotifications]; should be called after [[UNUserNotificationCenter currentNotificationCenter] requestAuthorizationWithOptions as described in the documentation

There is some code duplication in this commit, but I think that is better for now, to help the reviewers.

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

After calling request permission and registerForRemoteNotifications a FCMToken is returned by messaging().getToken()

Release Plan

[CATEGORY][type] [LOCATION] - Message

[ IOS] [ BUGFIX] [MESSAGING] - Solves the problems with request permission and registerForRemoteNotifications in messaging module.


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@claassistantio
Copy link

claassistantio commented Dec 17, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Salakar
❌ Simone Carcone


Simone Carcone 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.

@mikehardy
Copy link
Collaborator

Fabulous - I think there are many reasons for the issue linked but if you've tested this as a fix for some of them, great! Thanks for the PR

@mikehardy mikehardy requested a review from Salakar December 17, 2019 18:39
@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. type: bug New bug report plugin: messaging FCM only - ( messaging() ) - do not use for Notifications Type: Bug Fix labels Dec 17, 2019
@cark1
Copy link
Author

cark1 commented Dec 17, 2019

Fabulous - I think there are many reasons for the issue linked but if you've tested this as a fix for some of them, great! Thanks for the PR

Yes, I think so, but after this commit of Salakar, probably this bug is the most frequent (reading the last messages)

@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #3017 into master will increase coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #3017      +/-   ##
=========================================
+ Coverage   89.83%   90.1%   +0.27%     
=========================================
  Files         109     109              
  Lines        3381    3381              
=========================================
+ Hits         3037    3046       +9     
+ Misses        344     335       -9

Copy link
Contributor

@Salakar Salakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for figuring this out, duplication is fine for now to get this fixed. I'll make a note on our internal tracker to see if we can de-dupe after we release Notifications - as we'll look at adding some internal changes to messaging to make it integrate better.

@Salakar Salakar modified the milestones: v6.3.0, v6.4.0 Feb 4, 2020
@russellwheatley russellwheatley merged commit ea66c68 into invertase:master Mar 12, 2020
@russellwheatley russellwheatley removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: messaging FCM only - ( messaging() ) - do not use for Notifications type: bug New bug report
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(FIXED) "[messaging/unknown] The operation couldn’t be completed".
5 participants