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

Change id -> event_id and include push section #306

Merged
merged 6 commits into from
Apr 6, 2016

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Apr 6, 2016

  • Change id in the push gateway poke to be event_id and spec that it's the Matrix event ID of the message. This is much more useful since it will allow a client to fetch just the event it's being notified about, or know when this event comes down its event stream.
  • Correct the spec for badge count pushes which omit fields previously described as mandatory.
  • Add more detail about when to use event_id to suppress dupes.
  • Also add the push gateway doc so it's actually included in the spec.

…t's the Matrix event ID of the message. Correct the spec for badge count pushes which omit fields previously described as mandatory. Add more detail about when to use event_id to suppress dupes. Also add the push gateway doc so it's actually included in the spec.
alerted in some way. It is therefore necessary to perform duplicate
suppression for such notifications using the `event_id` field to avoid
retries of this HTTP API causing duplicate alerts. The operation of
updating counts of unread notifications should be and therefore do not
Copy link
Member

Choose a reason for hiding this comment

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

should be ... idempotent?

@richvdh
Copy link
Member

richvdh commented Apr 6, 2016

I think notification, counts and devices.items each need a title to make the generated spec right.

@richvdh
Copy link
Member

richvdh commented Apr 6, 2016

By the way, could you try to give your commit messages short summary lines ?

@richvdh richvdh assigned dbkr and unassigned richvdh Apr 6, 2016
dbkr added 4 commits April 6, 2016 19:49
They are actually within the notification object rather than the top level object. Add titles to objects so it works.
@dbkr
Copy link
Member Author

dbkr commented Apr 6, 2016

Noticed some other problems too. ptal.

@dbkr dbkr assigned richvdh and unassigned dbkr Apr 6, 2016
@richvdh
Copy link
Member

richvdh commented Apr 6, 2016

lgtm now.

@richvdh richvdh assigned dbkr and unassigned richvdh Apr 6, 2016
@dbkr dbkr merged commit 0411cf5 into master Apr 6, 2016
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