Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new APNs configuration option push_with_badge to suppress sending aps.badge in APNs messages. #358

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add a new APNs configuration option push_with_badge to suppress sending aps.badge in APNs messages. #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0924eb8
Add a new APNs configuration option `push_with_badge` to suppress sen…
CEnnis91 Jan 25, 2024
12c0b05
Update changelog.d
CEnnis91 Jan 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/358.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a new APNs configuration option `push_with_badge` to suppress sending `aps.badge` in APNs messages.
4 changes: 4 additions & 0 deletions sygnal.yaml.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ apps:
# # Defaults to True, set this to False if your client library provides a
# # push token in hex format.
# #convert_device_token_to_hex: false
# #
# # Specifies whether to send the badge key in the APNs message.
# # Defaults to True, set this to False to suppress sending the badge count.
# #push_with_badge: false
Comment on lines +205 to +208
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# #
# # Specifies whether to send the badge key in the APNs message.
# # Defaults to True, set this to False to suppress sending the badge count.
# #push_with_badge: false
# #
# # Specifies whether to update the notification count badge on the app upon sending
# # an APNs message (if notification counts can be calculated). Specifically,
# # whether or not the `aps.badge` field is set.
# #
# # If this is False, additional `unread_count` and `missed_calls` fields will be added to
# # the APNS message.
# #
# # Defaults to True, set this to False to suppress sending the badge count.
# #push_with_badge: false

Some slight wording adjustments. You may also want to explain why this could be desirable when "not using mutable-content"?

Though after writing this out, I feel like we're conflating two different behaviours in a single config option:

  1. Suppression of aps.badge field.
  2. Inclusion of additional unread_count and missed_calls fields.

Could you say why you want the unread_count and missed_calls fields? Or even what the overall goal for this PR is with respect to your implementation?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this comes off as a bit confusing.

The primary focus of this commit to suppress the aps.badge field so iOS does not automatically draw the badge on the home screen. In the event an iOS app is receiving APNS from multiple independent sources, the badge value is incorrect. Whether the other source is sending its own aps.badge or not, sygnal sending its own aps.badge still conflicts with the true badge value expected by the user.

When suppressing that field, I wanted to ensure the underlying data was still available to the iOS client, so it could be used, if desired. The inclusion of the unread_count and missed_calls fields were to represent the expanded form of what aps.badge represents here .

I agree the inclusion of unread_count and missed_calls could become their own setting, but I was unsure about APNS message length assumptions.

  • If length assumptions are not a concern, it seems like always including unread_count and missed_calls in APNS would be reasonable (I can submit a separate PR). Then this PR would simply be about toggling the inclusion of aps.badge.
  • If length assumptions are a concern, I'm open to opinions of how to move forward. We can drop the extra fields entirely, or add multiple settings controlling the inclusion of fields. But neither of these options feel very elegant.


# This is an example GCM/FCM push configuration.
#
Expand Down
9 changes: 8 additions & 1 deletion sygnal/apnspushkin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class ApnsPushkin(ConcurrencyLimitedPushkin):
"topic",
"push_type",
"convert_device_token_to_hex",
"push_with_badge",
} | ConcurrencyLimitedPushkin.UNDERSTOOD_CONFIG_FIELDS

APNS_PUSH_TYPES = {
Expand Down Expand Up @@ -555,14 +556,20 @@ def _get_payload_full(
if loc_args:
payload["aps"].setdefault("alert", {})["loc-args"] = loc_args

if badge is not None:
if self.get_config("push_with_badge", bool, True) and badge is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than pulling this option value out of the config on each request, please do so in __init__ and set to a class variable, then reference that class variable instead. i.e.:

def __init__(self, name: str, sygnal: "Sygnal", config: Dict[str, Any]) -> None:
    # ...
    self.push_with_badge = self.get_config("push_with_badge", bool, True)
def _get_payload_event_id_only(
    self, n: Notification, default_payload: Dict[str, Any]
) -> Dict[str, Any]:
    # ...
    if self.push_with_badge and badge is not None:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 will be in the next commit

payload["aps"]["badge"] = badge

if loc_key and n.room_id:
payload["room_id"] = n.room_id
if loc_key and n.event_id:
payload["event_id"] = n.event_id

if not self.get_config("push_with_badge", bool, True):
if n.counts.unread is not None:
payload["unread_count"] = n.counts.unread
if n.counts.missed_calls is not None:
payload["missed_calls"] = n.counts.missed_calls

return payload

async def _send_notification(
Expand Down
59 changes: 59 additions & 0 deletions tests/test_apns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

PUSHKIN_ID = "com.example.apns"
PUSHKIN_ID_WITH_PUSH_TYPE = "com.example.apns.push_type"
PUSHKIN_ID_WITH_NO_BADGE = "com.example.apns.no_badge"

TEST_CERTFILE_PATH = "/path/to/my/certfile.pem"

Expand Down Expand Up @@ -55,6 +56,12 @@
"pushkey_ts": 42,
}

DEVICE_EXAMPLE_FOR_NO_BADGE_PUSHKIN = {
"app_id": "com.example.apns.no_badge",
"pushkey": "spqr",
"pushkey_ts": 42,
}


class ApnsTestCase(testutils.TestCase):
def setUp(self) -> None:
Expand All @@ -73,10 +80,12 @@ def setUp(self) -> None:
self.apns_pushkin_snotif = MagicMock()
test_pushkin = self.get_test_pushkin(PUSHKIN_ID)
test_pushkin_push_type = self.get_test_pushkin(PUSHKIN_ID_WITH_PUSH_TYPE)
test_pushkin_with_no_badge = self.get_test_pushkin(PUSHKIN_ID_WITH_NO_BADGE)
# type safety: using ignore here due to mypy not handling monkeypatching,
# see https://github.com/python/mypy/issues/2427
test_pushkin._send_notification = self.apns_pushkin_snotif # type: ignore[assignment] # noqa: E501
test_pushkin_push_type._send_notification = self.apns_pushkin_snotif # type: ignore[assignment] # noqa: E501
test_pushkin_with_no_badge._send_notification = self.apns_pushkin_snotif # type: ignore[assignment] # noqa: E501

def get_test_pushkin(self, name: str) -> ApnsPushkin:
test_pushkin = self.sygnal.pushkins[name]
Expand All @@ -91,6 +100,12 @@ def config_setup(self, config: Dict[str, Any]) -> None:
"certfile": TEST_CERTFILE_PATH,
"push_type": "alert",
}
config["apps"][PUSHKIN_ID_WITH_NO_BADGE] = {
"type": "apns",
"certfile": TEST_CERTFILE_PATH,
"push_type": "alert",
"push_with_badge": False,
}

def test_payload_truncation(self) -> None:
"""
Expand Down Expand Up @@ -391,3 +406,47 @@ def test_expected_with_push_type(self) -> None:
self.assertEqual(PushType.ALERT, notification_req.push_type)

self.assertEqual({"rejected": []}, resp)

def test_expected_with_no_badge(self) -> None:
"""
Tests the expected case with no badge: a good response from APNS means
we pass on a good response to the homeserver.
"""
# Arrange
method = self.apns_pushkin_snotif
method.side_effect = testutils.make_async_magic_mock(
NotificationResult("notID", "200")
)

# Act
resp = self._request(
self._make_dummy_notification([DEVICE_EXAMPLE_FOR_NO_BADGE_PUSHKIN])
)

# Assert
self.assertEqual(1, method.call_count)
((notification_req,), _kwargs) = method.call_args

self.assertEqual(
{
"room_id": "!slw48wfj34rtnrf:example.com",
"event_id": "$qTOWWTEL48yPm3uT-gdNhFcoHxfKbZuqRVnnWWSkGBs",
"missed_calls": 1,
"unread_count": 2,
"aps": {
"alert": {
"loc-key": "MSG_FROM_USER_IN_ROOM_WITH_CONTENT",
"loc-args": [
"Major Tom",
"Mission Control",
"I'm floating in a most peculiar way.",
],
},
},
},
notification_req.message,
)

self.assertEqual(PushType.ALERT, notification_req.push_type)

self.assertEqual({"rejected": []}, resp)