allow voip type #88
allow voip type #88
Conversation
lib/apnotic/request.rb
Outdated
| if notification.type | ||
| h.merge!('apns-push-type' => notification.type) | ||
| else | ||
| h.merge!('apns-push-type' => notification.background_notification? ? 'background' : 'alert' ) |
There was a problem hiding this comment.
I would defer to the owner here but here we have two ways of doing the if statement for the same function... I would suggest...
if notification.type
..
elsif notification.background_notification?
...
else
...
end
There was a problem hiding this comment.
Yes, I was trying to maintain some backwards compatibility in cases where the type is not set, using the approach you're proposing allows us to be more specific and would allow us to just set voip type if we want to eliminate generic type
lib/apnotic/notification.rb
Outdated
| @@ -3,7 +3,8 @@ | |||
| module Apnotic | |||
|
|
|||
| class Notification < AbstractNotification | |||
| attr_accessor :alert, :badge, :sound, :content_available, :category, :custom_payload, :url_args, :mutable_content, :thread_id | |||
| attr_accessor :alert, :badge, :sound, :content_available, :category, :custom_payload, :url_args, | |||
| :mutable_content, :thread_id, :type | |||
There was a problem hiding this comment.
looking at the rest of the project, I think type might be a bit too generic.... what about voip as a boolean?
There was a problem hiding this comment.
from reading the apple documentation here: https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns I think only one apns-push-type is allowed. Due to that, I think we should ensure that we are not allowing more push type values to be set. This is why I decided to go with type, and it could be set to the following values
- voip
- mdm
- fileprovider
- complication
- alert
- background
Since we are only concerned with voip we could limit the scope to just that, if we think that works better?
ostinelli
force-pushed
the
master
branch
2 times, most recently
from
90a8855
to
49e105d
Compare
|
Hi @kelleyduran, Thanks for your work on this! What I'd like to see is a proper wrapper around the The I know it's been a long time so if you're not interested or available to do this work I would understand and we can close this PR. Ben |
|
Hi, I need to setup voip push notifications. |
proposed fix for invalidpushtype error #87