ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 206 forks source link

Notification not working with new Ionic Push #181

Closed SarasArya closed 8 years ago

SarasArya commented 8 years ago

I was actually trying to run the latest Node-gcm with Ionic Push Library. Seems to me that if I just paste var message = new gcm.Message(); the notification is received but without the payload and if I try and add the addNotification() field the notification is not delivered. Server side code

message.addData('message', "This is my notification");
message.addNotification({
  title: 'Alert!!!',
  body: 'Abnormal data access',
  icon: 'icon',
  message : 'This is my last try'
 });
var sender = new gcm.Sender('SENDERS_API_KEY'); 

var regTokens =  [];
regTokens.push(req.body.regId);

    sender.send(message, { registrationTokens: regTokens }, function (err, result) {
        if (err) {
            console.error(err);
        }
        else {

             console.log(result);
             res.status(200);
             res.set('Content-Type', 'application/json');
             res.send(result);
        }
    });   

front end code

app.run(function($ionicPlatform, $ionicPush,$http) {
$ionicPlatform.ready(function() {
var push = new Ionic.Push({
"onNotification": function(notification) {
var payload = notification.payload;
console.log(payload);
console.log(notification);
console.log("notification received");
},
"onRegister": function(data) {
console.log(data.token);
var reg = {
regId : data.token
 };
$http.post('http://app.metaiot.com/sendPush', reg)
.then(function(response){
 console.log(response);
 }, function (error){
console.log(error);
 }); 
}
});

So I wanted to confirm first. Is this an issue from the node-gcm end or Ionic End? Can somebody implement it and let us know? becuase I have boiled it down to the very fact that it's just and issue with addNotification function.

eladnava commented 8 years ago

@SarasArya I don't think that you should be adding a message: key to the addNotification() call. Can you try without it?

  message : 'This is my last try'

Also, check out this issue which reports similar behavior with PhoneGap:

You are right, Its not node gcm issue. However i debugged and was able to solve it. Just incase anyone stumbles here, The Phonegap plugin needs the 3 notification fields. phonegap plugin reads the title and the icon from the "notification" object however the message needs to be set in the "data" object.

hypesystem commented 8 years ago

As @eladnava points out, there are generally some issues with a lot of client-side libraries not yet supporting the notification field. It was added with Android M (quite recently).

It all else fails, I would recommend you stick to the data field and build and show the notifications client-side (instead of relying on automatic showing).

SarasArya commented 8 years ago

Sorry for the issue it was something from ionic side.... They had given everything in their docs just for one thing. They were initialising a constructor with 2 parameter and the docs asked us to set only one. I have reported the issue. Lets hope someone will find it useful if it's there Keep up the good work. I am closing the issue. Thanks @hypesystem and @eladnava

eladnava commented 8 years ago

@SarasArya Can you please link to your report of this bug to Ionic so we can list it in the Compatibility Documentation? (#155)

hypesystem commented 8 years ago

As I read it, the Ionic implementation actually worked, but something was documented poorly. It seems this was a false negative, not a bug.

eladnava commented 8 years ago

They were initialising a constructor with 2 parameter and the docs asked us to set only one

@hypesystem I guess we'll never know unless @SarasArya responds.

SarasArya commented 8 years ago

@hypesystem This wasn't a bug. Just poor documentation. I don't know how it worked for others. @eladnava How do I link my report to Ionic?

eladnava commented 8 years ago

@SarasArya You stated this:

They were initialising a constructor with 2 parameter and the docs asked us to set only one. I have reported the issue.

How did you report the issue? GitHub? E-mail?

SarasArya commented 8 years ago

I emailed them... They have a suggest edit in the docs. I gave my suggestion there. That's that

eladnava commented 8 years ago

@SarasArya Cool, do you know if they updated the docs to fix the mistake?