Merge pull request #5332 from hypothesis/revert-5321-filters

Revert "Change the NipsaFilter to HiddenFilter to filter annotations which are moderated or belong to a nipsaed user."

h/nipsa/subscribers.py

@@ -5,12 +5,21 @@
 
 def transform_annotation(event):
     """Add a {"nipsa": True} field on moderated annotations or those whose users are flagged."""
+    annotation = event.annotation
     payload = event.annotation_dict
 
-    if _user_nipsa(event.request, payload):
+    nipsa = _user_nipsa(event.request, payload)
+    nipsa = nipsa or _annotation_moderated(event.request, annotation)
+
+    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, annotation):
+    svc = request.find_service(name='annotation_moderation')
+    return svc.hidden(annotation)

h/presenters/annotation_searchindex.py

@@ -43,10 +43,14 @@ def asdict(self):
 
         # Mark an annotation as hidden if it and all of it's children have been
         # moderated and hidden.
-        parents_and_replies = [self.annotation.id] + self.annotation.thread_ids
-
-        ann_mod_svc = self.request.find_service(name='annotation_moderation')
-        result['hidden'] = len(ann_mod_svc.all_hidden(parents_and_replies)) == len(parents_and_replies)
+        result['hidden'] = False
+        if self.annotation.thread_ids:
+            ann_mod_svc = self.request.find_service(name='annotation_moderation')
+            annotation_hidden = ann_mod_svc.hidden(self.annotation)
+            thread_ids_hidden = len(ann_mod_svc.all_hidden(self.annotation.thread_ids)) == len(self.annotation.thread_ids)
+
+            if annotation_hidden and thread_ids_hidden:
+                result['hidden'] = True
 
         return result
 

h/search/core.py

@@ -48,7 +48,7 @@ def __init__(self, request, separate_replies=False, stats=None, _replies_limit=2
                            query.GroupFilter(),
                            query.GroupAuthFilter(request),
                            query.UserFilter(),
-                           query.HiddenFilter(request),
+                           query.NipsaFilter(request),
                            query.AnyMatcher(),
                            query.TagsMatcher(),
                            query.KeyValueMatcher()]

h/search/query.py

@@ -386,18 +386,19 @@ def __call__(self, search, _):
         return search.exclude("exists", field="deleted")
 
 
-class HiddenFilter(object):
-    """Return an Elasticsearch filter for filtering out moderated or NIPSA'd annotations."""
+class NipsaFilter(object):
+    """Return an Elasticsearch filter for filtering out NIPSA'd annotations."""
 
     def __init__(self, request):
         self.group_service = request.find_service(name='group')
         self.user = request.user
 
     def __call__(self, search, _):
-        """Filter out all hidden and NIPSA'd annotations except the current user's."""
-        # If any one of the "should" clauses is true then the annotation will
-        # get through these filter.
-        should_clauses = [Q("bool", must_not=[Q("term", nipsa=True), Q("term", hidden=True)])]
+        """Filter out all NIPSA'd annotations except the current user's."""
+        # If any one of these "should" clauses is true then the annotation will
+        # get through the filter.
+        should_clauses = [Q("bool", must_not=Q("term", nipsa=True)),
+                          Q("exists", field="thread_ids")]
 
         if self.user is not None:
             # Always show the logged-in user's annotations even if they have nipsa.

tests/h/nipsa/subscribers_test.py

@@ -20,7 +20,7 @@ def id(self):
         return self.data['id']
 
 
-@pytest.mark.usefixtures('nipsa_service')
+@pytest.mark.usefixtures('nipsa_service', 'moderation_service')
 class TestTransformAnnotation(object):
     @pytest.mark.parametrize('ann,flagged', [
         (FakeAnnotation({'id': 'ann-1', 'user': 'george'}), True),
@@ -40,9 +40,33 @@ def test_with_user_nipsa(self, ann, flagged, nipsa_service, pyramid_request):
         else:
             assert 'nipsa' not in ann.data
 
+    @pytest.mark.parametrize('ann,moderated', [
+        (FakeAnnotation({'id': 'normal'}), False),
+        (FakeAnnotation({'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=ann,
+                          annotation_dict=ann.data)
+
+        subscribers.transform_annotation(event)
+
+        if moderated:
+            assert ann.data['nipsa'] is True
+        else:
+            assert 'nipsa' not in ann.data
+
     @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

tests/h/presenters/annotation_searchindex_test.py

@@ -76,7 +76,8 @@ def test_it_marks_annotation_hidden_when_it_and_all_children_are_moderated(self,
             userid='acct:luke@hypothes.is',
             thread_ids=thread_ids)
 
-        moderation_service.all_hidden.return_value = [annotation.id] + thread_ids
+        moderation_service.hidden.return_value = True
+        moderation_service.all_hidden.return_value = thread_ids
 
         annotation_dict = AnnotationSearchIndexPresenter(annotation, pyramid_request).asdict()
 
@@ -104,7 +105,8 @@ def test_it_does_not_mark_annotation_hidden_when_children_are_not_moderated(self
             userid='acct:luke@hypothes.is',
             thread_ids=thread_ids)
 
-        moderation_service.all_hidden.return_value = [annotation.id] + thread_ids[1:]
+        moderation_service.all_hidden.return_value = thread_ids[1:]
+        moderation_service.hidden.return_value = True
 
         annotation_dict = AnnotationSearchIndexPresenter(annotation, pyramid_request).asdict()
 
@@ -121,11 +123,9 @@ def test_it_does_not_mark_annotation_hidden_when_not_moderated_and_no_replies(se
         assert annotation_dict['hidden'] is False
 
     def test_it_marks_annotation_hidden_when_moderated_and_no_replies(self, pyramid_request, moderation_service):
-        thread_ids = []
-        annotation = mock.MagicMock(userid='acct:luke@hypothes.is',
-                                    thread_ids=thread_ids)
+        annotation = mock.MagicMock(userid='acct:luke@hypothes.is')
 
-        moderation_service.all_hidden.return_value = [annotation.id] + thread_ids
+        moderation_service.hidden.return_value = True
 
         annotation_dict = AnnotationSearchIndexPresenter(annotation, pyramid_request).asdict()
 
@@ -142,6 +142,7 @@ def DocumentSearchIndexPresenter(self, patch):
 def moderation_service(pyramid_config):
     svc = mock.create_autospec(AnnotationModerationService, spec_set=True, instance=True)
     svc.all_hidden.return_value = []
+    svc.hidden.return_value = False
     pyramid_config.register_service(svc, name='annotation_moderation')
     return svc
 

tests/h/search/conftest.py

@@ -6,7 +6,6 @@
 
 import h.search.index
 from h.services.group import GroupService
-from h.services.annotation_moderation import AnnotationModerationService
 
 
 @pytest.fixture(autouse=True)
@@ -17,14 +16,6 @@ def group_service(pyramid_config):
     return group_service
 
 
-@pytest.fixture(autouse=True)
-def moderation_service(pyramid_config):
-    svc = mock.create_autospec(AnnotationModerationService, spec_set=True, instance=True)
-    svc.all_hidden.return_value = []
-    pyramid_config.register_service(svc, name='annotation_moderation')
-    return svc
-
-
 @pytest.fixture
 def Annotation(factories, index):
     """Create and index an annotation.

tests/h/search/index_test.py

@@ -10,11 +10,12 @@
 import pytest
 
 import h.search.index
+from h.services.annotation_moderation import AnnotationModerationService
 
 from tests.common.matchers import Matcher
 
 
-@pytest.mark.usefixtures("annotations")
+@pytest.mark.usefixtures("annotations", "moderation_service")
 class TestIndex(object):
     def test_annotation_ids_are_used_as_elasticsearch_ids(self, es_client,
                                                           factories,
@@ -494,3 +495,12 @@ def _get(annotation_id):
             index=es_client.index, doc_type=es_client.mapping_type,
             id=annotation_id)["_source"]
     return _get
+
+
+@pytest.fixture
+def moderation_service(pyramid_config):
+    svc = mock.create_autospec(AnnotationModerationService, spec_set=True, instance=True)
+    svc.all_hidden.return_value = []
+    svc.hidden.return_value = False
+    pyramid_config.register_service(svc, name='annotation_moderation')
+    return svc

tests/h/search/query_test.py

@@ -623,50 +623,13 @@ def search(self, search):
 
 
 @pytest.mark.usefixtures('pyramid_config')
-class TestHiddenFilter(object):
-
-    @pytest.mark.parametrize('nipsa,hidden,should_show_annotation', [
-        # both nipsa and hidden fields are set, so don't show the annotation
-        (True, True, False),
-        # nipsa field is set, so don't show the annotation
-        (True, False, False),
-        # hidden field is set, so don't show  the annotation
-        (False, True, False),
-        # neither field is set, show  the annotation
-        (False, False, True),
-    ])
-    def test_visibility_of_moderated_and_nipsaed_annotations(
-        self, index, Annotation, pyramid_request, search, user,
-        AnnotationSearchIndexPresenter, nipsa, hidden, should_show_annotation
-    ):
-
-        pyramid_request.user = user
-        search.append_modifier(query.HiddenFilter(pyramid_request))
-        presenter = AnnotationSearchIndexPresenter.return_value
-
-        presenter.asdict.return_value = {'id': 'ann1',
-                                         'hidden': hidden,
-                                         'nipsa': nipsa}
-        Annotation(id='ann1')
-
-        presenter.asdict.return_value = {'id': 'ann2',
-                                         'hidden': False,
-                                         'nipsa': False}
-        Annotation(id='ann2', userid=user.userid)
-
-        expected_ids = ['ann2']
-        if should_show_annotation:
-            expected_ids.append('ann1')
-
-        result = search.run({})
-
-        assert sorted(result.annotation_ids) == sorted(expected_ids)
+class TestNipsaFilter(object):
 
     def test_hides_banned_users_annotations_from_other_users(
         self, pyramid_request, search, banned_user, user, Annotation
     ):
         pyramid_request.user = user
-        search.append_modifier(query.HiddenFilter(pyramid_request))
+        search.append_modifier(query.NipsaFilter(pyramid_request))
         Annotation(userid=banned_user.userid)
         expected_ids = [Annotation(userid=user.userid).id]
 
@@ -678,7 +641,7 @@ def test_shows_banned_users_annotations_to_banned_user(
         self, pyramid_request, search, banned_user, user, Annotation
     ):
         pyramid_request.user = banned_user
-        search.append_modifier(query.HiddenFilter(pyramid_request))
+        search.append_modifier(query.NipsaFilter(pyramid_request))
         expected_ids = [Annotation(userid=banned_user.userid).id]
 
         result = search.run(webob.multidict.MultiDict({}))
@@ -691,7 +654,7 @@ def test_shows_banned_users_annotations_in_groups_they_created(
     ):
         pyramid_request.user = user
         group_service.groupids_created_by.return_value = ["created_by_banneduser"]
-        search.append_modifier(query.HiddenFilter(pyramid_request))
+        search.append_modifier(query.NipsaFilter(pyramid_request))
         expected_ids = [Annotation(groupid="created_by_banneduser",
                                    userid=banned_user.userid).id]
 
@@ -975,10 +938,3 @@ def es_dsl_search(pyramid_request):
         using=pyramid_request.es.conn,
         index=pyramid_request.es.index,
     )
-
-
-@pytest.fixture
-def AnnotationSearchIndexPresenter(patch):
-    AnnotationSearchIndexPresenter = patch('h.search.index.presenters.AnnotationSearchIndexPresenter')
-    AnnotationSearchIndexPresenter.return_value.asdict.return_value = {'test': 'val'}
-    return AnnotationSearchIndexPresenter