Use `default_payload` in Push Payloads #127

ismailgulek · 2020-06-10T11:53:38Z

Add fallback_content case handling from push data to include alert and mutable-content fields in the aps dictionary.

Part of element-hq/element-ios#3325

richvdh

Hey @ismailgulek, I've got a few questions I'm afraid...

richvdh · 2020-06-10T15:05:46Z

changelog.d/127.feature

@@ -0,0 +1 @@
+Add fallback_content handling from push data.


could you try to expand on this a bit? What is fallback_content? what does handling it involve? does this affect both iOS and Android, or just one of them?

Sure, fallback_content is needed because we need to add alert in the aps field of payload. The handling is this: if we see this value as true in the data, we're adding alert to the payload. It's because of this:

The system executes your notification content app extension only when a remote notification’s payload contains the following information:

The payload must include the mutable-content key with a value of 1.

The payload must include an alert dictionary with title, subtitle, or body information.

Detailed info can be found at https://developer.apple.com/documentation/usernotifications/modifying_content_in_newly_delivered_notifications

It only affects iOS and does not affect Android or other pusher types.

sygnal/apnspushkin.py

richvdh · 2020-06-10T15:08:22Z

sygnal/apnspushkin.py

@@ -299,6 +299,13 @@ def _get_payload_event_id_only(self, n):
        if n.counts.missed_calls is not None:
            payload["missed_calls"] = n.counts.missed_calls

+        payload["aps"] = {"mutable-content": 1}
+        if (
+            n.devices[0].data["fallback_content"] is not None


why do we only look at the first device? Shouldn't this be looking at the device we are preparing the payload for?

You're right. Updated.

richvdh · 2020-06-10T15:10:32Z

sygnal/apnspushkin.py

+        payload["aps"] = {"mutable-content": 1}
+        if (
+            n.devices[0].data["fallback_content"] is not None
+            and n.devices[0].data["fallback_content"]


Doesn't this need to be specced?

I'm not sure about that, the fallback_content will be an additional field in data property of https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-pushers-set. Plus homeserver just passes this data according to this:

A dictionary of additional pusher-specific data. For 'http' pushers, this is the data dictionary passed in at pusher creation minus the url key.

Detailed info can be found here: https://matrix.org/docs/spec/push_gateway/r0.1.1#post-matrix-push-v1-notify

The thing is, for client implementations to know what to set, and for pushgateway implementations to know how to interpret it, it has to be specced, otherwise this becomes secret knowledge which will only work with riot-ios and sygnal.

as per matrix-org/matrix-spec-proposals#2631 (comment): no, it doesn't need to be in the spec. Sorry!

That said, I do think it would be useful if we documented somewhere within the sygnal project what parameters the apnspushkin supports in its data dict and what they do. Perhaps you could add these details under https://github.com/matrix-org/sygnal#app-types ?

sygnal/apnspushkin.py

richvdh · 2020-06-11T11:48:57Z

sygnal/apnspushkin.py

+        payload["aps"] = {"mutable-content": 1}
+        if (
+            n.devices[0].data["fallback_content"] is not None
+            and n.devices[0].data["fallback_content"]


The thing is, for client implementations to know what to set, and for pushgateway implementations to know how to interpret it, it has to be specced, otherwise this becomes secret knowledge which will only work with riot-ios and sygnal.

ismailgulek · 2020-06-18T08:23:04Z

Decided to update this with a more general solution.

Offering use of default_payload in data which will be some JSON data, set by the clients. This will be even more helpful because in this way clients will have more freedom to use mutable/silent or any other type of notifications, and also have possibility to select alert string or dictionary in the way they want. These are some configurations for iOS side, but also Android side can have some default payload.

richvdh

A couple of things to fix here.

Please can you add some documentation for each of the two pushkins, which documents the structure of the data field they expect?

sygnal/apnspushkin.py

sygnal/gcmpushkin.py

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

ismailgulek · 2020-06-22T15:32:42Z

A couple of things to fix here.

Please can you add some documentation for each of the two pushkins, which documents the structure of the data field they expect?

Sure, is it ok to add these in Pushkin classes, like:

sygnal/sygnal/apnspushkin.py

Line 66 in d63e70f

Relays notifications to the Apple Push Notification Service.

richvdh

code looks good now. I look forward to seeing the docs!

ismailgulek · 2020-06-23T10:18:01Z

code looks good now. I look forward to seeing the docs!

thanks, README updated

richvdh

couple of suggestions

README.rst

richvdh · 2020-06-23T11:03:16Z

README.rst

+When calling https://matrix.org/docs/spec/client_server/r0.5.0#post-matrix-client-r0-pushers-set, clients
+can provide a ``default_payload`` data dictionary in ``PusherData``, which will be passed to Sygnal.
+This will be used by Sygnal to have a default data dictionary in the push payload, for both APNS and
+GCM pushers, if provided. Upon the default payload dictionary, Sygnal will make incremental changes.
+This is useful for clients to decide default push payload content. For instance, iOS clients will have
+freedom to use silent/mutable notifications and be able to set some default alert/sound/badge fields.
+


I think it would be clearer to start a list here.

The following parameters can be specified in the `data` dictionary which is given when configuring the pusher via `POST /_matrix/client/r0/pushers/set <https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-pushers-set>`_ : * ``default_payload``: a dictionary which defines the basic payload to be sent to the notification service. Sygnal will merge information specific to the push event into this dictionary. If unset, the empty dictionary is used. This can be useful for clients to specify default push payload content.... etc

Thanks for the suggestion, updated

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

richvdh · 2020-06-23T17:44:53Z

README.rst

+=======
+
+The following parameters can be specified in the `data` dictionary which is given when configuring the pusher
+via [POST /_matrix/client/r0/pushers/set](<https://matrix.org/docs/spec/client_server/latest#post-matrix-client-r0-pushers-set>) :


this is RST, so this format doesn't work. I think I got it right in my suggestion?

Ah sorry yes, you're right. I was bitten by my previewer. Updated.

richvdh

looks great, thank you for bearing with it!

ismailgulek · 2020-06-24T09:01:46Z

looks great, thank you for bearing with it!

Great, thanks for your valuable comments.

myoussef89 · 2020-06-28T22:12:45Z

I got this error when I send it empty, what shoud be sent inside ?:

Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: 2020-06-28 22:08:54,462 [4208] ERROR sygnal.http [40efe488-af22-41ee-8fc2-da59be7238b7] Exception whilst dispatching notification.
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: Traceback (most recent call last):
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: File "/home/ubuntu/sygnal/sygnal/http.py", line 265, in _handle_dispatch
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: result = await pushkin.dispatch_notification(notif, d, context)
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: File "/home/ubuntu/sygnal/sygnal/gcmpushkin.py", line 326, in dispatch_notification
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: data = GcmPushkin._build_data(n, device)
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: File "/home/ubuntu/sygnal/sygnal/gcmpushkin.py", line 410, in _build_data
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: data.update(device.data.get("default_payload", {}))
Jun 28 22:08:54 ip-172-31-11-241 python3[4208]: ValueError: dictionary update sequence element #0 has length 1; 2 is required

"data": {
"default_payload": {},
"format": "event_id_only",
"url": "https://********************/_matrix/push/v1/notify"
}

ismailgulek · 2020-06-29T07:38:06Z

I got this error when I send it empty, what shoud be sent inside ?:

Nothing actually. I've tested your data dictionary and it was fine in my Sygnal. I'm not sure what's the root cause but you may remove default_payload field.

myoussef89 · 2020-06-29T09:06:28Z

it's working without it, but I don't receive any notification on ios side.

ismailgulek added 3 commits June 10, 2020 14:47

Add fallback_content handling

d5aef73

Update tests to include fallback case

117fbfe

Add changelog for PR

474d2e1

ismailgulek requested a review from a team June 10, 2020 12:49

richvdh requested changes Jun 10, 2020

View reviewed changes

Add device parameter to payload method

d98ccee

ismailgulek requested a review from richvdh June 11, 2020 07:00

Update changelog

42ffb95

richvdh reviewed Jun 11, 2020

View reviewed changes

ismailgulek added 5 commits June 11, 2020 15:18

Add docstring for device param

b1f0ab7

Move redundant mutable-content only if alert is set

53abed2

Implement default_payload on APNS pushkin

c78daf7

Implement default_payload on GCM pushkin

beffc7b

Black formatting

daf2ef6

ismailgulek changed the title ~~Add Fallback Content Case Handling~~ Use default_payload in Push Payloads Jun 18, 2020

Update changelog

8cb8124

ismailgulek requested a review from richvdh June 18, 2020 08:26

richvdh mentioned this pull request Jun 19, 2020

MSC2631: Add default_payload to PusherData matrix-org/matrix-spec-proposals#2631

Closed

Add handling for badge only pushs

d1c91ad

richvdh requested changes Jun 22, 2020

View reviewed changes

ismailgulek and others added 8 commits June 22, 2020 18:07

Update sygnal/apnspushkin.py

c7fad91

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update sygnal/apnspushkin.py

e3a00a6

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update sygnal/apnspushkin.py

95c68bf

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update sygnal/apnspushkin.py

1d78aad

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update sygnal/gcmpushkin.py

77b4a4b

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Add setdefault for loc-args

2c07fee

Import copy module

88dfe21

Format with black

81b01d6

richvdh reviewed Jun 22, 2020

View reviewed changes

Update README.rst to include default payload

c15feca

richvdh reviewed Jun 23, 2020

View reviewed changes

ismailgulek and others added 2 commits June 23, 2020 14:10

Update README.rst

2d7c50c

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

Update doc for default payload

6f3eeb2

richvdh reviewed Jun 23, 2020

View reviewed changes

Update URL in README

fb262ec

richvdh approved these changes Jun 24, 2020

View reviewed changes

richvdh merged commit 34f60f6 into master Jun 24, 2020

richvdh deleted the riot_3325 branch June 24, 2020 09:00

richvdh mentioned this pull request Jun 30, 2020

Make it clear that Sygnal is Element-flavoured, and document what other apps can expect #128

Closed

DMRobertson mentioned this pull request Jan 24, 2022

Check if default_payload is a dict before using it as a dict #292

Merged

Johennes mentioned this pull request Oct 11, 2024

PusherData documentation doesn't mention default_payload field matrix-org/matrix-spec#921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `default_payload` in Push Payloads #127

Use `default_payload` in Push Payloads #127

Use default_payload in Push Payloads #127

Use default_payload in Push Payloads #127

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `default_payload` in Push Payloads #127

Use `default_payload` in Push Payloads #127