Merge pull request #5327 from hypothesis/fix-unperformant-activity-pa…

…ge-db-query

Use elasticsearch to get group annotation count

h/services/annotation_stats.py

@@ -3,28 +3,31 @@
 from __future__ import unicode_literals
 
 import sqlalchemy as sa
+from webob.multidict import MultiDict
 
 from h.models import Annotation
+from h.search import Search
+from h.search import TopLevelAnnotationsFilter
 
 
 class AnnotationStatsService(object):
     """A service for retrieving annotation stats for users and groups."""
 
-    def __init__(self, session):
-        self.session = session
+    def __init__(self, request):
+        self.request = request
 
     def user_annotation_counts(self, userid):
         """Return the count of annotations for this user."""
 
-        annotations = self.session.query(Annotation). \
+        annotations = self.request.db.query(Annotation). \
             filter_by(userid=userid, deleted=False). \
             options(sa.orm.load_only('groupid', 'shared')).subquery()
         grouping = sa.case([
             (sa.not_(annotations.c.shared), 'private'),
             (annotations.c.groupid == '__world__', 'public'),
         ], else_='group')
 
-        result = dict(self.session.query(grouping, sa.func.count(annotations.c.id)).group_by(grouping).all())
+        result = dict(self.request.db.query(grouping, sa.func.count(annotations.c.id)).group_by(grouping).all())
         for key in ['public', 'group', 'private']:
             result.setdefault(key, 0)
 
@@ -36,15 +39,17 @@ def user_annotation_counts(self, userid):
 
     def group_annotation_count(self, pubid):
         """
-        Return the count of shared annotations for this group.
+        Return the count of searchable top level annotations for this group.
         """
+        search = Search(self.request, stats=self.request.stats)
+        search.append_modifier(TopLevelAnnotationsFilter())
 
-        return (
-            self.session.query(Annotation)
-            .filter_by(groupid=pubid, shared=True, deleted=False)
-            .count())
+        params = MultiDict({'limit': 0, 'group': pubid})
+
+        search_result = search.run(params)
+        return search_result.total
 
 
 def annotation_stats_factory(context, request):
     """Return an AnnotationStatsService instance for the passed context and request."""
-    return AnnotationStatsService(session=request.db)
+    return AnnotationStatsService(request)

h/views/activity.py

@@ -156,7 +156,7 @@ def user_annotation_count(aggregation, userid):
                               for u in [self.group.creator]]
                 moderators = sorted(moderators, key=lambda k: k['username'].lower())
 
-        group_annotation_count = self.request.find_service(name='annotation_stats').group_annotation_count(self.group.pubid)
+        group_annotation_count = self._get_total_annotations_in_group(result, q, self.request)
 
         result['stats'] = {
             'annotation_count': group_annotation_count,
@@ -205,6 +205,19 @@ def user_annotation_count(aggregation, userid):
 
         return result
 
+    def _get_total_annotations_in_group(self, result, q, request):
+        """
+        Get number of annotations in group.
+
+        If the search result already has this number don't run a query, just re-use it.
+        """
+        group_annotation_count = result['search_results'].total
+        if len(q) > 1:
+            group_annotation_count = (self.request
+                                      .find_service(name='annotation_stats')
+                                      .group_annotation_count(self.group.pubid))
+        return group_annotation_count
+
     @view_config(request_method='POST',
                  request_param='group_join')
     def join(self):

tests/h/services/annotation_stats_test.py

@@ -7,6 +7,8 @@
 
 from h.services.annotation_stats import AnnotationStatsService
 from h.services.annotation_stats import annotation_stats_factory
+from h.search import Search
+from h.search import TopLevelAnnotationsFilter
 
 
 class TestAnnotationStatsService(object):
@@ -69,21 +71,36 @@ def test_user_annotation_counts_ignores_deleted_annotations(self, svc, factories
         assert results['group'] == 0
         assert results['total'] == 0
 
-    def test_annotation_count_returns_count_of_shared_annotations_for_group(self, svc, factories):
-        pubid = 'abc123'
-        for i in range(3):
-            factories.Annotation(groupid=pubid, shared=True)
-        for i in range(2):
-            factories.Annotation(groupid=pubid, shared=False)
+    def test_group_annotation_count_calls_search_with_request_and_stats(
+        self, svc, search, pyramid_request,
+    ):
+        svc.group_annotation_count('groupid')
 
-        assert svc.group_annotation_count(pubid) == 3
+        search.assert_called_with(pyramid_request, stats=pyramid_request.stats)
 
-    def test_group_annotation_count_excludes_deleted_annotations(self, svc, factories):
-        pubid = 'abc123'
-        for i in range(3):
-            factories.Annotation(groupid=pubid, shared=True, deleted=True)
+    def test_group_annotation_count_calls_run_with_groupid_and_limit(
+        self, svc, search,
+    ):
+        svc.group_annotation_count('groupid')
+
+        search.return_value.run.assert_called_with({"limit": 0, "group": "groupid"})
+
+    def test_group_annotation_count_excludes_replies(
+        self, svc, search, top_level_annotation_filter,
+    ):
+        svc.group_annotation_count('groupid')
 
-        assert svc.group_annotation_count(pubid) == 0
+        search.return_value.append_modifier.assert_called_with(
+            top_level_annotation_filter.return_value)
+
+    def test_group_annotation_count_returns_total(
+        self, svc, search,
+    ):
+        search.return_value.run.return_value.total = 3
+
+        anns = svc.group_annotation_count('groupid')
+
+        assert anns == 3
 
 
 class TestAnnotationStatsFactory(object):
@@ -92,13 +109,32 @@ def test_returns_service(self):
 
         assert isinstance(svc, AnnotationStatsService)
 
-    def test_sets_session(self):
+    def test_sets_request(self):
         request = mock.Mock()
         svc = annotation_stats_factory(mock.Mock(), request)
 
-        assert svc.session == request.db
+        assert svc.request == request
+
+
+@pytest.fixture
+def svc(pyramid_request):
+    return AnnotationStatsService(request=pyramid_request)
+
+
+@pytest.fixture
+def pyramid_request(pyramid_request):
+    pyramid_request.es = mock.Mock()
+    pyramid_request.stats = mock.Mock()
+    return pyramid_request
+
+
+@pytest.fixture
+def search(patch):
+    return patch('h.services.annotation_stats.Search',
+                 autospec=Search, spec_set=True)
 
 
 @pytest.fixture
-def svc(db_session):
-    return AnnotationStatsService(session=db_session)
+def top_level_annotation_filter(patch):
+    return patch('h.services.annotation_stats.TopLevelAnnotationsFilter',
+                 autospec=TopLevelAnnotationsFilter, spec_set=True)

tests/h/views/activity_test.py

@@ -6,10 +6,12 @@
 import pytest
 import mock
 from pyramid import httpexceptions
+from webob.multidict import MultiDict
 
 from h.activity.query import ActivityResults
 from h.views import activity
 from h.models import Organization
+from h.services.annotation_stats import AnnotationStatsService
 
 
 GROUP_TYPE_OPTIONS = ('group', 'open_group', 'restricted_group')
@@ -573,10 +575,36 @@ def test_search_passes_the_group_annotation_count_to_the_template(self,
                                                                       factories,
                                                                       test_group,
                                                                       test_user,
+                                                                      query,
+                                                                      annotation_stats_service,
                                                                       pyramid_request):
+        query.extract.return_value = MultiDict({"user": test_user, "group": test_group})
         result = controller.search()['stats']
+        annotation_stats_service.group_annotation_count.assert_called_with(test_group.pubid)
         assert result['annotation_count'] == 5
 
+    @pytest.mark.parametrize('test_group,test_user',
+                             [('group', 'member')],
+                             indirect=['test_group', 'test_user'])
+    def test_search_reuses_group_annotation_count_if_able(self,
+                                                         controller,
+                                                         factories,
+                                                         test_group,
+                                                         test_user,
+                                                         query,
+                                                         annotation_stats_service,
+                                                         pyramid_request):
+        """
+        In cases where the annotation count returned from search is the same calc
+        as the annotation count that would be returned from the stats service,
+        re-use that value rather than executing another query inside the stats
+        service.
+        """
+        query.extract.return_value = MultiDict({"group": test_group})
+        result = controller.search()['stats']
+        annotation_stats_service.group_annotation_count.assert_not_called()
+        assert result['annotation_count'] == 200
+
     @pytest.mark.parametrize('test_group, test_user, test_heading, test_subtitle, test_share_msg',
                              [('group', 'member', 'Members', 'Invite new members', 'Sharing the link lets people join this group:'),
                               ('open_group', 'user', 'Members', 'Share group', 'Sharing the link lets people view this group:'),
@@ -832,6 +860,10 @@ def toggle_user_facet_request(self, pyramid_request):
         pyramid_request.params['toggle_user_facet'] = 'acct:fred@hypothes.is'
         return pyramid_request
 
+    @pytest.fixture
+    def query(self, patch):
+        return patch('h.views.activity.query')
+
 
 @pytest.mark.usefixtures('annotation_stats_service', 'user_service', 'routes', 'search')
 class TestUserSearchController(object):
@@ -1174,14 +1206,6 @@ def user_search_controller(self, user, pyramid_request):
         return activity.UserSearchController(user, pyramid_request)
 
 
-class FakeAnnotationStatsService(object):
-    def user_annotation_counts(self, userid):
-        return {'public': 1, 'private': 3, 'group': 2, 'total': 6}
-
-    def group_annotation_count(self, pubid):
-        return 5
-
-
 @pytest.fixture
 def group(factories):
     group = factories.Group()
@@ -1256,7 +1280,15 @@ def routes(pyramid_config):
 
 @pytest.fixture
 def annotation_stats_service(pyramid_config):
-    pyramid_config.register_service(FakeAnnotationStatsService(), name='annotation_stats')
+    ann_stat_svc = mock.create_autospec(AnnotationStatsService, instance=True, spec_set=True)
+
+    ann_stat_svc.user_annotation_counts.return_value = (
+        {'public': 1, 'private': 3, 'group': 2, 'total': 6})
+    ann_stat_svc.group_annotation_count.return_value = 5
+
+    pyramid_config.register_service(ann_stat_svc, name="annotation_stats")
+
+    return ann_stat_svc
 
 
 @pytest.fixture

tests/h/views/admin/users_test.py

@@ -2,7 +2,7 @@
 
 from __future__ import unicode_literals
 
-from mock import Mock
+import mock
 from mock import MagicMock
 from pyramid import httpexceptions
 import pytest
@@ -233,22 +233,35 @@ def models(patch):
 
 @pytest.fixture
 def user_service(pyramid_config, db_session):
-    service = Mock(spec_set=UserService(default_authority='example.com',
-                                        session=db_session))
+    service = mock.create_autospec(
+        UserService,
+        instance=True,
+        spec_set=True)
+    service.return_value.default_authority = 'example.com'
+    service.return_value.session = db_session
+
     pyramid_config.register_service(service, name='user')
     return service
 
 
 @pytest.fixture
-def annotation_stats_service(pyramid_config, db_session):
-    service = Mock(spec_set=AnnotationStatsService(session=db_session))
+def annotation_stats_service(pyramid_config, pyramid_request):
+    service = mock.create_autospec(
+        AnnotationStatsService,
+        instance=True,
+        spec_set=True)
+    service.return_value.request = pyramid_request
     service.user_annotation_counts.return_value = {'total': 0}
     pyramid_config.register_service(service, name='annotation_stats')
     return service
 
 
 @pytest.fixture
 def delete_user_service(pyramid_config, pyramid_request):
-    service = Mock(spec_set=DeleteUserService(request=pyramid_request))
+    service = mock.create_autospec(
+        DeleteUserService,
+        instance=True,
+        spec_set=True)
+    service.return_value.request = pyramid_request
     pyramid_config.register_service(service, name='delete_user')
     return service