Remove aps size check Notification#background_notification?

[danielmorrison]

Notifications intended to be background were not getting set as expected due to other attributes (for example, custom_payload).

I explained this a bit more in: #90.

We're testing this now on real-world code. I don't see any "gotchas" yet, but our use cases are fairly limited.

[ostinelli]

@noefroidevaux any thoughts on this? I'm not using those so feedback is welcome.

[noefroidevaux]

@ostinelli I think my code was ok. The Apple documentation about this say:

You may include custom keys in the payload, but the aps dictionary must not contain any keys that would trigger user interactions.

https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/pushing_background_updates_to_your_app#2980040

My code tested that content-available is the only key in apsdictionary, which follows the Apple documentation (and the test too). As custom_payload is added at the root of the JSON, outside the aps, so I don't how the PR will fix something about that?

Maybe I miss something, but I don't see the problem. I use the current version of the Gem with background notifications and custom payload without problem.

/cc @danielmorrison

[danielmorrison]

@noefroidevaux I looked at how we're using it (in production since this PR, no problems) and I see that the app sets alert = {} (we add to that in other notifications, but not these ones).

So it is very possible this is on my side and easily fixed.

With that in mind, seems like others could fall into this trap too. Should we check for specific key usage, or maybe even error. Explicitly fail here if count > 1?

[noefroidevaux]

Yes, you are right @danielmorrison , as the documentation from Apple that the "aps dictionary must not contain any keys that would trigger user interactions". We should raise an error when the aps dictionary use content-available AND something else which trigger user interactions (alert, badge, ...).

[ostinelli]

@danielmorrison and @noefroidevaux would you like to create a PR that addresses these concerns?

[benubois]

If there is going to be validation it should not be based on content-available because it is valid to set content-available to 1 along with other supported keys:

To support a background update notification, make sure that the payload’s aps dictionary includes the content-available key with a value of 1. If there are user-visible updates that go along with the background update, you can set the alert, sound, or badge keys in the aps dictionary, as appropriate.

However, it is NOT valid to have apns-push-type set to background AND include any other supported keys.

To send a background notification, create a remote notification with an aps dictionary that includes only the content-available key, as shown in Listing 1. You may include custom keys in the payload, but the aps dictionary must not contain any keys that would trigger user interactions.

I think the distinction here is around a "silent push" only, where you're requesting that your app be woken up in the background vs an "alert push", where there is a visual notification displayed to the recipient, either with or without a wakeup request.

If there is any validation, it should be based on the apns-push-type value. I don't think there's anything to do here right now, since apns-push-type cannot be set.

Longer term, I think it would be useful to fully support apns-push-type as outlined in #88. If anyone wants to take apnotic in this direction, then it should also validate or appropriately set the apns-priority to 5.