From 283d8eda4fb36dbbf2c84e84df8ec72b24933289 Mon Sep 17 00:00:00 2001 From: chdorner Date: Mon, 3 Apr 2017 17:16:42 +0200 Subject: [PATCH 1/4] Add `hidden` method to annotation moderation service For checking if a given annotation id is moderated or not. --- h/services/annotation_moderation.py | 19 +++++++++++++---- .../h/services/annotation_moderation_test.py | 21 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/h/services/annotation_moderation.py b/h/services/annotation_moderation.py index 2006be51f0e..3abad080163 100644 --- a/h/services/annotation_moderation.py +++ b/h/services/annotation_moderation.py @@ -9,6 +9,20 @@ class AnnotationModerationService(object): def __init__(self, session): self.session = session + def hidden(self, annotation_id): + """ + Check if an annotation id is hidden. + + :param annotation_id: The id of the annotation to check. + :type annotation: unicode + + :returns: true/false boolean + :rtype: bool + """ + q = self.session.query(models.AnnotationModeration) \ + .filter_by(annotation_id=annotation_id) + return self.session.query(q.exists()).scalar() + def hide(self, annotation): """ Hide an annotation from other users. @@ -23,10 +37,7 @@ def hide(self, annotation): :type annotation: h.models.Annotation """ - query = self.session.query(models.AnnotationModeration) \ - .filter_by(annotation=annotation) - - if query.count() > 0: + if self.hidden(annotation.id): return mod = models.AnnotationModeration(annotation=annotation) diff --git a/tests/h/services/annotation_moderation_test.py b/tests/h/services/annotation_moderation_test.py index 596207c27ed..bd4b342ddfb 100644 --- a/tests/h/services/annotation_moderation_test.py +++ b/tests/h/services/annotation_moderation_test.py @@ -9,6 +9,18 @@ from h.services.annotation_moderation import annotation_moderation_service_factory +class TestAnnotationModerationServiceHidden(object): + def test_it_returns_true_for_moderated_annotation(self, svc, factories): + mod = factories.AnnotationModeration() + + assert svc.hidden(mod.annotation.id) is True + + def test_it_returns_false_for_non_moderated_annotation(self, svc, factories): + annotation = factories.Annotation() + + assert svc.hidden(annotation.id) is False + + class TestAnnotationModerationServiceHide(object): def test_it_creates_annotation_moderation(self, svc, factories, db_session): annotation = factories.Annotation() @@ -31,10 +43,6 @@ def test_it_skips_creating_moderation_when_already_exists(self, svc, factories, assert count == 1 - @pytest.fixture - def svc(self, db_session): - return AnnotationModerationService(db_session) - class TestAnnotationNipsaServiceFactory(object): def test_it_returns_service(self, pyramid_request): @@ -44,3 +52,8 @@ def test_it_returns_service(self, pyramid_request): def test_it_provides_request_db_as_session(self, pyramid_request): svc = annotation_moderation_service_factory(None, pyramid_request) assert svc.session == pyramid_request.db + + +@pytest.fixture +def svc(db_session): + return AnnotationModerationService(db_session) From 65daa05b8d3ab3ac5df993d17abbfe6bc900ab1c Mon Sep 17 00:00:00 2001 From: chdorner Date: Tue, 4 Apr 2017 12:44:59 +0200 Subject: [PATCH 2/4] Mark moderated annotations as nipsa'd in search index For now, this is done the same way we already check if the user is nipsa'd, and then add a `"nipsa": true` item to the search index payload. Ideally, we would also get rid of the `AnnotationTransformEvent` system and build on top of the recently introduced formatters (or something similar to that). But since the card for annotation moderation is already quite big I've opted for extending the current system instead. --- h/nipsa/subscribers.py | 25 ++++++++--- tests/h/nipsa/subscribers_test.py | 73 ++++++++++++++++++++----------- 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/h/nipsa/subscribers.py b/h/nipsa/subscribers.py index ccda031100e..2e5217415fb 100644 --- a/h/nipsa/subscribers.py +++ b/h/nipsa/subscribers.py @@ -1,9 +1,24 @@ # -*- coding: utf-8 -*- +from __future__ import unicode_literals + def transform_annotation(event): - """Add a {"nipsa": True} field on annotations whose users are flagged.""" - annotation = event.annotation_dict - nipsa_service = event.request.find_service(name='nipsa') - if 'user' in annotation and nipsa_service.is_flagged(annotation['user']): - annotation['nipsa'] = True + """Add a {"nipsa": True} field on moderated annotations or whose users are flagged.""" + payload = event.annotation_dict + + nipsa = _user_nipsa(event.request, payload) + nipsa = nipsa or _annotation_moderated(event.request, payload) + + if nipsa: + payload['nipsa'] = True + + +def _user_nipsa(request, payload): + nipsa_service = request.find_service(name='nipsa') + return 'user' in payload and nipsa_service.is_flagged(payload['user']) + + +def _annotation_moderated(request, payload): + svc = request.find_service(name='annotation_moderation') + return svc.hidden(payload['id']) diff --git a/tests/h/nipsa/subscribers_test.py b/tests/h/nipsa/subscribers_test.py index 656262bd301..51e63977eaf 100644 --- a/tests/h/nipsa/subscribers_test.py +++ b/tests/h/nipsa/subscribers_test.py @@ -10,28 +10,51 @@ FakeEvent = namedtuple('FakeEvent', ['request', 'annotation_dict']) -@pytest.mark.usefixtures('nipsa_service') -@pytest.mark.parametrize("ann,flagged", [ - ({"user": "george"}, True), - ({"user": "georgia"}, False), - ({}, False), -]) -def test_transform_annotation(ann, flagged, nipsa_service, pyramid_request): - nipsa_service.is_flagged.return_value = flagged - event = FakeEvent(request=pyramid_request, - annotation_dict=ann) - - subscribers.transform_annotation(event) - - if flagged: - assert ann["nipsa"] is True - else: - assert "nipsa" not in ann - - -@pytest.fixture -def nipsa_service(pyramid_config): - service = mock.Mock(spec_set=['is_flagged']) - service.is_flagged.return_value = False - pyramid_config.register_service(service, name='nipsa') - return service +@pytest.mark.usefixtures('nipsa_service', 'moderation_service') +class TestTransformAnnotation(object): + @pytest.mark.parametrize('ann,flagged', [ + ({'id': 'ann-1', 'user': 'george'}, True), + ({'id': 'ann-2', 'user': 'georgia'}, False), + ({'id': 'ann-3'}, False), + ]) + def test_with_user_nipsa(self, ann, flagged, nipsa_service, pyramid_request): + nipsa_service.is_flagged.return_value = flagged + event = FakeEvent(request=pyramid_request, + annotation_dict=ann) + + subscribers.transform_annotation(event) + + if flagged: + assert ann['nipsa'] is True + else: + assert 'nipsa' not in ann + + @pytest.mark.parametrize('ann,moderated', [ + ({'id': 'normal'}, False), + ({'id': 'moderated'}, True) + ]) + def test_with_moderated_annotation(self, ann, moderated, moderation_service, pyramid_request): + moderation_service.hidden.return_value = moderated + event = FakeEvent(request=pyramid_request, + annotation_dict=ann) + + subscribers.transform_annotation(event) + + if moderated: + assert ann['nipsa'] is True + else: + assert 'nipsa' not in ann + + @pytest.fixture + def nipsa_service(self, pyramid_config): + service = mock.Mock(spec_set=['is_flagged']) + service.is_flagged.return_value = False + pyramid_config.register_service(service, name='nipsa') + return service + + @pytest.fixture + def moderation_service(self, pyramid_config): + service = mock.Mock(spec_set=['hidden']) + service.hidden.return_value = False + pyramid_config.register_service(service, name='annotation_moderation') + return service From cdbb82be89c1480b93c67e7223fedc3e4c30887e Mon Sep 17 00:00:00 2001 From: chdorner Date: Tue, 4 Apr 2017 14:09:30 +0200 Subject: [PATCH 3/4] Reindex annotation after moderating --- h/views/api_moderation.py | 5 +++++ tests/h/views/api_moderation_test.py | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/h/views/api_moderation.py b/h/views/api_moderation.py index d6f6896c093..11bae16e5c5 100644 --- a/h/views/api_moderation.py +++ b/h/views/api_moderation.py @@ -5,6 +5,7 @@ from pyramid import security from pyramid.httpexceptions import HTTPNoContent, HTTPNotFound +from h import events from h.views.api import api_config @@ -19,4 +20,8 @@ def create(context, request): svc = request.find_service(name='annotation_moderation') svc.hide(context.annotation) + + event = events.AnnotationEvent(request, context.annotation.id, 'update') + request.notify_after_commit(event) + return HTTPNoContent() diff --git a/tests/h/views/api_moderation_test.py b/tests/h/views/api_moderation_test.py index 7666e33ff8d..9b65050860e 100644 --- a/tests/h/views/api_moderation_test.py +++ b/tests/h/views/api_moderation_test.py @@ -17,6 +17,15 @@ def test_it_hides_the_annotation(self, pyramid_request, resource, moderation_ser moderation_service.hide.assert_called_once_with(resource.annotation) + def test_it_publishes_update_event(self, pyramid_request, resource, events): + views.create(resource, pyramid_request) + + events.AnnotationEvent.assert_called_once_with( + pyramid_request, resource.annotation.id, 'update') + + pyramid_request.notify_after_commit.assert_called_once_with( + events.AnnotationEvent.return_value) + def test_it_renders_no_content(self, pyramid_request, resource): response = views.create(resource, pyramid_request) assert isinstance(response, HTTPNoContent) @@ -45,3 +54,12 @@ def has_permission(self, pyramid_request): func = mock.Mock(return_value=True) pyramid_request.has_permission = func return func + + @pytest.fixture + def events(self, patch): + return patch('h.views.api_moderation.events') + + @pytest.fixture + def pyramid_request(self, pyramid_request): + pyramid_request.notify_after_commit = mock.Mock() + return pyramid_request From 1888860c0b4c8b824620aef083361de46157a055 Mon Sep 17 00:00:00 2001 From: Nick Stenning Date: Wed, 5 Apr 2017 10:47:54 +0200 Subject: [PATCH 4/4] Tiny wording change to docstring --- h/nipsa/subscribers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/h/nipsa/subscribers.py b/h/nipsa/subscribers.py index 2e5217415fb..390bfbd57b6 100644 --- a/h/nipsa/subscribers.py +++ b/h/nipsa/subscribers.py @@ -4,7 +4,7 @@ def transform_annotation(event): - """Add a {"nipsa": True} field on moderated annotations or whose users are flagged.""" + """Add a {"nipsa": True} field on moderated annotations or those whose users are flagged.""" payload = event.annotation_dict nipsa = _user_nipsa(event.request, payload)