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.

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

Conversation

CEnnis91
Copy link

For situations when not using mutable-content and updating the badge count is not desired.
This also adds unread_count and missed_calls to the push payload when push_with_badge is False.

@CEnnis91 CEnnis91 requested a review from a team as a code owner January 26, 2024 23:27
Comment on lines +205 to +208
# #
# # 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
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.

@@ -551,14 +552,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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants