search

j3k0 / ganomede-notifications

Long-pull notification service for Ganomede
0 stars 0 forks source link

Push API Fixes #18

Closed elmigranto closed 9 years ago

elmigranto commented 9 years ago

Should I expect to see some error logs if it failed?

I added some more logging in this PR for failed sends. It is delayed sometime and might not show up before process exits due to how apn works. (There seems to be no way to know whether notification was sent successfully and accepted.)

Concurrency code states (not sure if it works with APN atm) (?)

When specifying options.maxConnections for new apn.Connection, I thought it would open multiple connections to Apple server, but it fires up connected event with only one. Should not be a problem, though, possibly it manages it internally and only adds more when 1 isn't enough.

Is it possible that the process exits before the payload has been sent?

Seems like this was the case, should be fixed now.

elmigranto commented 9 years ago

Okay, @j3k0, try to send notifcation now, if it doesn't work, can you please show what it looks like and what redis contents are?

Also, bunyan's log.debug doesn't show up in the output for me, any specific level I must set?

j3k0 commented 9 years ago

@elmigranto DEBUG is level 20. (https://github.com/trentm/node-bunyan#levels)

I will try and let you know!

j3k0 commented 9 years ago

Works!

I also made sure notification.push is removed from APN's payload (it would be redondant). Actually, I think a safer way would be to select which fields to copy from notification to payload 1 by 1 (in case we later add other sensitive informations in notification, we may forgot to hide it from the push notification, with security implications)

Fields to copy: data, to, timestamp, from, type, id, push

j3k0 commented 9 years ago

While writing the client-side push handling code, I realized having the notification.push object available is actually useful, because it is "provider-independant".

This allows to write more generic handling code.