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

Improve documentation for pushers and push gateways #1506

Merged
merged 3 commits into from Aug 16, 2018

Conversation

2 participants
@turt2live
Member

turt2live commented Aug 14, 2018

Rendered: see 'docs' commit status


This fixes a number of formatting issues alongside a few documentation problems:

  • The push gateway can actually expect less parameters than previously advertised. This is for user privacy.
  • Introduction of the m.email pusher for email-capable homeservers.
  • Fields not being flagged as required on some endpoints.
  • Document the event_id_only format

Note: this does not attempt to document push rules, just pushers.

Fixes #1374
Fixes #1087

Improve documentation for pushers and push gateways
This fixes a number of formatting issues alongside a few documentation problems:
* The push gateway can actually expect less parameters than previously advertised. This is for user privacy.
* Introduction of the `m.email` pusher for email-capable homeservers.
* Fields not being flagged as required on some endpoints.
* Document the `event_id_only` format

Note: this does not attempt to document push rules, just pushers.

Fixes #1374
Fixes #1087

@turt2live turt2live requested a review from dbkr Aug 14, 2018

@turt2live turt2live added this to Needs review in August 2018 r0 Aug 14, 2018

many fields as it can in the push notification. If set to
``"event_id_only"``, the homeserver should only send the
absolute minimum amount of information possible to the push
gateway by not including optional fields.

This comment has been minimized.

@dbkr

dbkr Aug 15, 2018

Member

This feels like it probably needs a stricter definition. It should probably have a thing like url saying it's only valid if kind = http. It's not really, "as many fields as it can" if undefined, it's the fields in the push gateway spec at https://matrix.org/docs/spec/push_gateway/unstable.html. event_id_only should probably also be well defined as the event ID, room ID and counts (probably also in the push gateway spec rather than here?)

@@ -65,6 +65,9 @@ APNS or Google's GCM. This happens as follows:
notifications.
5. The Push Provider sends the notification to the device.
Homeservers may optionally support email notifications or other push kinds
of push, identified by the ``kind`` field of the pusher configuration.

This comment has been minimized.

@dbkr

dbkr Aug 15, 2018

Member

Too many 'push's in "other push kinds of push". Also, this another case of in what cases more types can be added. Can an HS just make up any type it feels like and implement it? If so, how do clients know how to use it? Personally I would say, what's defined in this version of the spec is valid, future versions of the spec might define more. We probably should have namespaced it, otherwise the risk is this implies any HS can support whatever they like here but it risks a namespace collision with a future specced thing.

This comment has been minimized.

@turt2live

turt2live Aug 15, 2018

Member

I'll take out this bit of text - it was supposed to help describe that email is a valid kind of pusher, although the swagger seems to do a pretty good job of saying that.

@@ -149,14 +146,15 @@ paths:
title: EventContent
description: |-
The ``content`` field from the event, if present. If the
event had no content field, this field is omitted.
event had no content field or the pusher wishes to not include
it, this field is omitted.

This comment has been minimized.

@dbkr

dbkr Aug 15, 2018

Member

Hmm, this is slightly debatable (and possibly not very relevant now we have event_id_only). I guess in principle there is no reason it shouldn't be optional. I would probably just say the HS may omit it for any other reason ('wishes not to include it' feels a bit unintentionally vague).

@turt2live turt2live requested a review from dbkr Aug 15, 2018

August 2018 r0 automation moved this from In review to Reviewer approved Aug 16, 2018

@dbkr

dbkr approved these changes Aug 16, 2018

Thank you!

@turt2live turt2live merged commit 94091a1 into matrix-org:master Aug 16, 2018

4 checks passed

ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 16, 2018

@turt2live turt2live deleted the turt2live:travis/general/pushers branch Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment