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

Added notification field for fcm [WIP] #41

Merged
merged 5 commits into from
Dec 1, 2016
Merged

Added notification field for fcm [WIP] #41

merged 5 commits into from
Dec 1, 2016

Conversation

mmazzarolo
Copy link
Contributor

@mmazzarolo mmazzarolo commented Nov 2, 2016

Related to #32.
You should be able to send notifications this way:

POST /api/push HTTP/1.1
Host: localhost:1337
X-Parse-Application-Id: TEST_APP_ID
X-Parse-Master-Key: TEST_MASTER_KEY
Content-Type: application/json
Cache-Control: no-cache
Postman-Token: 277e9e75-565d-1a81-b80b-1575809fbac6

{
    "where": {
	    "objectId" : "yTgONHk2il"
    },
    "data": {
        "data" : "Hello World!"
    },
    "notification": {
    	"title": "I am a title",
    	"body": "I am the body"
    }
}

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.

// push adapter config
  push: {
    fcm: {
      senderId: keys.FCM_SENDER_ID,
      apiKey: keys.FCM_API_KEY
    }
  }

From my testing it works on both Android and iOS.

@mmazzarolo mmazzarolo mentioned this pull request Nov 2, 2016
@flovilmart
Copy link
Contributor

Did you test it thoroughly?

@mmazzarolo
Copy link
Contributor Author

mmazzarolo commented Nov 2, 2016

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 👍

@flovilmart
Copy link
Contributor

Awesome! Hopefully your testing will be conclusive ;)

@felimartina
Copy link

Great work @mmazzarolo I will test it tomorrow. Thanks for taking the time!

@mmazzarolo
Copy link
Contributor Author

FYI I handled iOS push notification too, just specify the 'fcm' push type when you define the adapter.

@mmazzarolo mmazzarolo changed the title Added notification field for fcm Added notification field for fcm [WIP] Nov 9, 2016
@flovilmart
Copy link
Contributor

Seems that the tests are still failing, can you address that before I spend more time in reviewing?

@felimartina
Copy link

@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)

@mmazzarolo
Copy link
Contributor Author

mmazzarolo commented Nov 14, 2016

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 :)

@felimartina
Copy link

@mmazzarolo I tested your repo and seems to be working fine. Wanna go ahead and proceed with the pull request?

@mmazzarolo
Copy link
Contributor Author

@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 devices
  • payloadData.data is not stringified anymore, it was causing some issue on FCM side last week

@felimartina
Copy link

Great @mmazzarolo. I guess the pull request is ready to be merged. @flovilmart

@jeffjen
Copy link
Contributor

jeffjen commented Nov 20, 2016

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.

@flovilmart
Copy link
Contributor

You can make a Pr on this Pr for the badge support for iOS

@flovilmart
Copy link
Contributor

@jeffjen are you adding the support for the badge or do we merge that one?

@jeffjen
Copy link
Contributor

jeffjen commented Dec 1, 2016

Please go ahead and merge this PR.

@flovilmart flovilmart merged commit 8e087cf into parse-community:master Dec 1, 2016
@EtgarSH
Copy link

EtgarSH commented Jul 2, 2017

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!

@flovilmart
Copy link
Contributor

@EtgarSH did you follow the configuration steps as described in the documentation?

@julianvogels
Copy link

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

@flovilmart
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants