Closed mmazzarolo closed 7 years ago
Did you test it thoroughly?
Only on Android, two devices, one app.
No need to rush with the merge though, I'll test it even more in next few days (I must ship it to production soon).
If anyone else want to test it too it would be even better :+1:
Awesome! Hopefully your testing will be conclusive ;)
Great work @mmazzarolo I will test it tomorrow. Thanks for taking the time!
FYI I handled iOS push notification too, just specify the 'fcm' push type when you define the adapter.
Seems that the tests are still failing, can you address that before I spend more time in reviewing?
@mmazzarolo seems like tests are only failing because they were tied to the old version of the GCM sender. If you see the errors here it is comparing the new output to the old input: Expected [ 'ios', 'android', 'fcm' ] to equal [ 'ios', 'android' ] Expected 'high' to equal 'normal'.
Should be just a matter of updating the test cases in here
Let me know if you need any help (was on holidays so I'm just catching up with all you did)
Hey @felimartina, I'm currently using this adapter, which is just a repo built from this pull in a small production app that I launched just today. I tested it a bit and it seemed to work, I'm still waiting for user feedback tough :)
@mmazzarolo I tested your repo and seems to be working fine. Wanna go ahead and proceed with the pull request?
@felimartina Done, please keep in mind that I also did the following breaking changes:
priority
is not 'high' because the 'normal' one doesn't trigger the notification with app in background on some devicespayloadData.data
is not stringified anymore, it was causing some issue on FCM side last weekGreat @mmazzarolo. I guess the pull request is ready to be merged. @flovilmart
I had been using my own FCM adapter when I added pushType support to classification. I wasn't aware that node-gcm had started supporting FCM payload.
Might I add that the new change be merged + a new change to support Badge updates? Because if you let fcm pushType be the sole push adapter, iOS will receive push notification, but badge updates would be ignored.
This would require including badge value from installations and pass it to "GCM.prototype.send" devices argument.
You can make a Pr on this Pr for the badge support for iOS
@jeffjen are you adding the support for the badge or do we merge that one?
Please go ahead and merge this PR.
Hi! I'm having a problem while sending notifications through Parse integrated with FCM.
My config:
. . . push: { "fcm": { "senderId": "[senderId]", "apiKey": "[apiKey]" } }
It is relevant to mention that when I send notifications through the FCM console, it does work.
I've tried to set the deviceToken
field in the ParseInstallation object and it has just crashed.
I've also tried changing the pushType
field to 'fcm' instead of 'gcm', and nothing.
My iOS gets the notifications sent through Parse.Push but the Android doesn't.
Thanks!
@EtgarSH did you follow the configuration steps as described in the documentation?
How does one update the documentation? The notification field should be mentioned in it. http://parseplatform.org/Parse-SDK-JS/api/classes/Parse.Push.html#methods
The docs are generated from the source code, and the api docs haven’t been correctly updated for a while, as we didn’t spent enough time in tooling for those.
Related to #32.
You should be able to send notifications this way:
Why do you need a notification field? Because with the recent gcm -> fcm migration the notification field is required for showing the notification when the app is in background.
From my testing it works on both Android and iOS.