-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(ios, badge): use new JSON location for FCM badge information #4560
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/aalzy8q1h |
Codecov Report
@@ Coverage Diff @@
## master #4560 +/- ##
==========================================
+ Coverage 73.54% 76.72% +3.19%
==========================================
Files 109 109
Lines 3710 3710
Branches 346 346
==========================================
+ Hits 2728 2846 +118
+ Misses 857 763 -94
+ Partials 125 101 -24 |
Hi there! Thanks for taking the time to PR this - that is certainly unexpected though, do you have any documentation or upstream links for it? I believe you and am inclined to merge but this wasn't noted anywhere and it apparently doesn't have any test coverage so I don't have any way to verify it |
Hi mike according to this doc https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification the badge count for ios is not within alert, but it is at the same level as alert. |
Yes the test cover here is really hard, but the docs link is great, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - seems to correlate with where the docs say the badging information is - thanks @yunarta !
…ation (invertase#4560) Co-authored-by: Yunarta Kartawahyudi <[email protected]>
Description
APS badging information was being pulled from the wrong JSON chunk, this moves it to align with docs
https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification
Related issues
Fixes #4559
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter