activitree / meteor-push

Meteor Push Notifications for Cordova and Web/PWA with Firebase (FCM).
MIT License
27 stars 19 forks source link

Push.send - validateDocument() check missing 'query' field? #6

Closed zuus-keith closed 4 years ago

zuus-keith commented 4 years ago

I'm looking at migrate away from raix:push to using this package and doing some basic inspection to see how much work there is to make the move over. Something that I noticed when looking through the example provided is that the query field is also used in the object that is passed into Push.send (similar to raix:push) but tracing this back to validateDocument() I can't see the 'query' field in the check. https://github.com/activitree/meteor-push/blob/45d97977c37d70d561fcdc4cd78e3af3bc910e88/lib/server/push.js#L17-L40 I assume this will produce the same error as mentioned in #5?

paulincai commented 4 years ago

Ok, I think you saw this in push_methods.js in the notification badge method (starting line 92)and ... that is a mistake. I try to remember what was my intention there. I think I was considering to give up entirely on the API part for sending to a token. I didn't see any application for this since in Meteor the token is tied up with the user Id. That query, initially was probably like { userId: ... } or { token: ... } whatever was available to you. However, you can send notifications or badge updates to only userIds and I think this covers all needs.

What do you think! Is that ok with you / your case? This package totally needs some polishing but the core tech is really ... current and good.

export const sendPushNotificationBadge = new ValidatedMethod({
  name: 'send.push.notification.badge',
  validate: new SimpleSchema({
    userId: { type: String },
    count: { type: Number }
  }).validator(),
  run ({ userId, count }) {
    Push.send({
      from: 'AppName',
      title: '',
      text: '',
      badge: count,
      query: { userId }
    })
  }
})
paulincai commented 4 years ago

@zuus-keith if you are ok with eliminating that querying userId or token, and if you could try to push a badge update to a userId directly and be successfully with it, I'd love to update this doc either by eliminating that query field from there, or by adding a query all the way. As is now is wrong anyway.

zuus-keith commented 4 years ago

My main use case for push notifications is to send some generic-looking notification to a group of users (e.g. a reminder). Based from that I can see the userId accepts a string but I assume its only expecting a single userId.

paulincai commented 4 years ago

Fixed in V2. The new API supports sending to userId, list of userIds, token, list of tokens, tokenId, list of tokenIds. V2 requires migration of tokens from APN to FCM since all messages are now going out via Firebase-Admin (Node server SDK for Firebase)