Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Notification system #488

Merged
merged 4 commits into from
Oct 23, 2015
Merged

Notification system #488

merged 4 commits into from
Oct 23, 2015

Conversation

tarekziade
Copy link
Contributor

Stuff to add before it's ready :

  • Agree on the payload v1 in cliquet/events.py
  • test : No event when POST on collection that returns 200 (existing id provided)
  • test : PUT that create
  • test : PUT that replace
  • test : PATCH whose fields are identical to existing ones
  • Update documentation (add new section?)
  • Update the roadmap, we now have notifications !
  • Add some logging during the setup of listeners

@@ -678,3 +683,31 @@ def test_next_page_url_relies_on_headers_information(self):
resp = self.app.get(self.collection_url + '?_limit=1',
headers=headers)
self.assertIn('https://server.name:443', resp.headers['Next-Page'])


class EventsTest(BaseWebTest, unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

move to separate test_events file :)

class CollectionChanged(object):
"""Triggered when a collection is changed.
"""
def __init__(self, action, collection, request):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: collection is actually a resource here

@Natim
Copy link
Contributor

Natim commented Oct 21, 2015

I have created #492 and #493 to handle some more stuff after this PR.

def __init__(self, action, resource, request):
self.request = request
service = current_service(request)
resource_id = service.viewset.get_name(resource.__class__) + '_id'
Copy link
Contributor

Choose a reason for hiding this comment

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

With what @Natim suggested in #492 it becomes clear that we will want the resource name in the payload.

In the case of kinto, they are bucket, group, collection, record.

It can still be shipped as is, but if there is still some room for last-minute changes :)

@leplatrem
Copy link
Contributor

Among the remaining tasks, I think of :

  • Update documentation (add new section?)
  • Update the roadmap, we now have notifications !
  • Add some logging during the setup of listeners
  • Add some statsd for listeners ?
  • Add impacted records to payload

Do you agree with that ? Should I create individual issues for each of them ?

The original issue for this was #32. But it mentions «pre»/«post» storage hooks. Should we close it with this PR ? And open another issue for hooks where records can be manipulated before/after interactions with storage backend ?

@leplatrem leplatrem modified the milestone: 2.9 Oct 22, 2015
@tarekziade
Copy link
Contributor Author

I would like to merge the PR asap before it rots, so I guess the most important stuff (besides the small tasks you added) is to agree on the payload v1. Let's talk about this today and decide

@tarekziade
Copy link
Contributor Author

I am adding #501 where I am listing stuff to add in the payload later

@leplatrem
Copy link
Contributor

FYI the roadmap I had in mind was this : http://cliquet.readthedocs.org/en/latest/rationale.html#roadmap

@tarekziade tarekziade changed the title [wip] Events Notification system Oct 23, 2015
@tarekziade
Copy link
Contributor Author

I can't see any statsd used anywhere in cliquet. I am create a new bug for adding statsd not only in notifications

@Natim
Copy link
Contributor

Natim commented Oct 23, 2015

It looks good to me apart from the failing test.

@tarekziade tarekziade merged commit d6228f6 into master Oct 23, 2015
@almet almet removed the in progress label Oct 23, 2015
@tarekziade tarekziade deleted the events branch October 23, 2015 09:34
glasserc pushed a commit that referenced this pull request May 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants