Make returning replies separately optional

Add a new, undocumented separate_replies=True option to the search API.

If separate_replies=True option is _not_ given to the search API, then
it reverts to its previous behaviour: _do_ include replies in the "rows"
list returned. This is the same behaviour that the search API had befor:
it returns both top-level annotations and replies in the one "rows"
list, but without any guarantee that if some annotations/replies from a
given thread are in the list then all annotations/replies from that
thread will be in it.

If separate_replies=True _is_ given then the API follows the new
behaviour: "rows" contains top-level annotations only, and a separate
"replies" list containing all replies to the annotations in rows is also
inserted into the result.

docs/api.rst

@@ -70,10 +70,6 @@ search
 
    Search for annotations.
 
-   Top-level annotations (but not replies) that match the search query are
-   returned in the "rows" field. The "total" field counts the number of these
-   top-level annotations.
-
    **Example request**::
 
       GET /api/search?limit=1000&user=gluejar@hypothes.is

h/api/search/core.py

@@ -9,7 +9,7 @@
 log = logging.getLogger(__name__)
 
 
-def search(request, params, private=True):
+def search(request, params, private=True, separate_replies=False):
     """
     Search with the given params and return the matching annotations.
 
@@ -23,8 +23,17 @@ def search(request, params, private=True):
         results
     :type private: bool
 
-    :returns: a dict with keys "rows" (the list of matching annotations, as
-        dicts) and "total" (the number of matching annotations, an int)
+    :param separate_replies: Whether or not to include a "replies" key in the
+        result containing a list of all replies to the annotations in the
+        "rows" key. If this is True then the "rows" key will include only
+        top-level annotations, not replies.
+    :type private: bool
+
+    :returns: A dict with keys:
+      "rows" (the list of matching annotations, as dicts)
+      "total" (the number of matching annotations, an int)
+      "replies": (the list of all replies to the annotations in "rows", if
+        separate_replies=True was passed)
     :rtype: dict
     """
     def make_builder():
@@ -39,30 +48,37 @@ def make_builder():
         return builder
 
     builder = make_builder()
-    builder.append_filter(query.TopLevelAnnotationsFilter())
+    if separate_replies:
+        builder.append_filter(query.TopLevelAnnotationsFilter())
     results = models.Annotation.search_raw(builder.build(params),
                                            raw_result=True,
                                            authorization_enabled=False)
 
-    # Do a second query for all replies to the annotations from the first
-    # query.
-    builder = make_builder()
-    builder.append_matcher(query.RepliesMatcher(
-        [h['_id'] for h in results['hits']['hits']]))
-    reply_results = models.Annotation.search_raw(builder.build({'limit': 100}),
-                                                 raw_result=True,
-                                                 authorization_enabled=False)
-
-    if len(reply_results['hits']['hits']) < reply_results['hits']['total']:
-        log.warn("The number of reply annotations exceeded the page size of "
-                 "the Elasticsearch query. We currently don't handle this, "
-                 "our search API doesn't support pagination of the reply set.")
-
     total = results['hits']['total']
     docs = results['hits']['hits']
     rows = [models.Annotation(d['_source'], id=d['_id']) for d in docs]
-    reply_docs = reply_results['hits']['hits']
-    reply_rows = [models.Annotation(d['_source'], id=d['_id'])
-                  for d in reply_docs]
+    return_value = {"rows": rows, "total": total}
+
+    if separate_replies:
+        # Do a second query for all replies to the annotations from the first
+        # query.
+        builder = make_builder()
+        builder.append_matcher(query.RepliesMatcher(
+            [h['_id'] for h in results['hits']['hits']]))
+        reply_results = models.Annotation.search_raw(
+            builder.build({'limit': 100}), raw_result=True,
+            authorization_enabled=False)
+
+        if len(reply_results['hits']['hits']) < reply_results['hits']['total']:
+            log.warn("The number of reply annotations exceeded the page size "
+                     "of the Elasticsearch query. We currently don't handle "
+                     "this, our search API doesn't support pagination of the "
+                     "reply set.")
+
+        reply_docs = reply_results['hits']['hits']
+        reply_rows = [models.Annotation(d['_source'], id=d['_id'])
+                      for d in reply_docs]
+
+        return_value["replies"] = reply_rows
 
-    return {"rows": rows, "total": total, "replies": reply_rows}
+    return return_value

h/api/search/test/core_test.py

@@ -13,8 +13,19 @@ def test_search_passes_private_to_AuthFilter(query):
 
     core.search(request, mock.Mock(), private=True)
 
-    assert query.AuthFilter.call_args_list == [
-        mock.call(request, private=True), mock.call(request, private=True)]
+    query.AuthFilter.assert_called_once_with(request, private=True)
+
+
+@search_fixtures
+def test_search_does_not_exclude_replies(query):
+    result = core.search(mock.Mock(), mock.Mock())
+
+    assert not query.TopLevelAnnotationsFilter.called, (
+        "Replies should not be filtered out of the 'rows' list if "
+        "separate_replies=True is not given")
+    assert 'replies' not in result, (
+        "The separate 'replies' list should not be included in the result if "
+        "separate_replies=True is not given")
 
 
 @search_fixtures
@@ -55,7 +66,7 @@ def test_search_queries_for_replies(query, models):
         },
     ]
 
-    core.search(mock.Mock(), mock.Mock())
+    core.search(mock.Mock(), mock.Mock(), separate_replies=True)
 
     # It should construct a RepliesMatcher for replies to the annotations from
     # the first search.
@@ -108,7 +119,7 @@ def test_search_returns_replies(models):
         mock.sentinel.reply_annotation_object_3,
     ]
 
-    result = core.search(mock.Mock(), mock.Mock())
+    result = core.search(mock.Mock(), mock.Mock(), separate_replies=True)
 
     assert result['replies'] == [
         mock.sentinel.reply_annotation_object_1,
@@ -141,7 +152,7 @@ def test_search_logs_a_warning_if_there_are_too_many_replies(models, log):
         },
     ]
 
-    core.search(mock.Mock(), mock.Mock())
+    core.search(mock.Mock(), mock.Mock(), separate_replies=True)
 
     assert log.warn.call_count == 1
 
@@ -171,7 +182,7 @@ def test_search_does_not_log_a_warning_if_there_are_not_too_many_replies(
         },
     ]
 
-    core.search(mock.Mock(), mock.Mock())
+    core.search(mock.Mock(), mock.Mock(), separate_replies=True)
 
     assert not log.warn.called
 

h/api/test/views_test.py

@@ -45,7 +45,8 @@ def test_search_searches(search_lib):
 
     views.search(request)
 
-    search_lib.search.assert_called_once_with(request, request.params)
+    search_lib.search.assert_called_once_with(
+        request, request.params, separate_replies=False)
 
 
 def test_search_renders_results(search_lib):
@@ -61,7 +62,6 @@ def test_search_renders_results(search_lib):
     assert result == {
         'total': 3,
         'rows': ['A', 'B', 'C'],
-        'replies': []
     }
 
 

h/api/views.py

@@ -92,14 +92,23 @@ def index(context, request):
 @api_config(context=Root, name='search')
 def search(request):
     """Search the database for annotations matching with the given query."""
-    results = search_lib.search(request, request.params)
+    params = request.params.copy()
 
-    return {
+    separate_replies = params.pop('_separate_replies', False)
+    results = search_lib.search(request, params,
+                                separate_replies=separate_replies)
+
+    return_value = {
         'total': results['total'],
-        'rows': [search_lib.render(a) for a in results['rows']],
-        'replies': [search_lib.render(a) for a in results['replies']]
+        'rows': [search_lib.render(a) for a in results['rows']]
     }
 
+    if separate_replies:
+        return_value['replies'] = [
+            search_lib.render(a) for a in results['replies']]
+
+    return return_value
+
 
 @api_config(route_name='access_token')
 def access_token(request):

h/static/scripts/annotation-viewer-controller.coffee

@@ -22,9 +22,14 @@ module.exports = class AnnotationViewerController
     $scope.search.update = (query) ->
       $location.path('/stream').search('q', query)
 
-    store.SearchResource.get _id: id, ({rows, replies}) ->
+    search_params = {
+      _id: id,
+      _separate_replies: true
+    }
+    store.SearchResource.get(search_params, ({rows, replies}) ->
       annotationMapper.loadAnnotations(rows, replies)
       $scope.threadRoot = {children: [threading.idTable[id]]}
+    )
 
     streamFilter
       .setMatchPolicyIncludeAny()

h/static/scripts/stream-controller.coffee

@@ -19,6 +19,7 @@ module.exports = class StreamController
       options = {offset, limit}
       searchParams = searchFilter.toObject($routeParams.q)
       query = angular.extend(options, searchParams)
+      query._separate_replies = true
       store.SearchResource.get(query, load)
 
     load = ({rows, replies}) ->

h/static/scripts/test/annotation-mapper-test.js

@@ -43,6 +43,15 @@ describe('annotationMapper', function() {
       assert.calledWith($rootScope.$emit, 'annotationsLoaded', [{}, {}, {}]);
     });
 
+    it('also includes replies in the annotationLoaded event', function () {
+      sandbox.stub($rootScope, '$emit');
+      var annotations = [{id: 1}]
+      var replies = [{id: 2}, {id: 3}];
+      annotationMapper.loadAnnotations(annotations, replies);
+      assert.called($rootScope.$emit);
+      assert.calledWith($rootScope.$emit, 'annotationsLoaded', [{}, {}, {}]);
+    });
+
     it('triggers the annotationUpdated event for each annotation in the threading cache', function () {
       sandbox.stub($rootScope, '$emit');
       var annotations = [{id: 1}, {id: 2}, {id: 3}];
@@ -54,6 +63,17 @@ describe('annotationMapper', function() {
       assert.calledWith($rootScope.$emit, 'annotationUpdated', cached.message);
     });
 
+    it('also triggers annotationUpdated for cached replies', function () {
+      sandbox.stub($rootScope, '$emit');
+      var annotations = [{id: 1}];
+      var replies = [{id: 2}, {id: 3}, {id: 4}];
+      var cached = {message: {id: 3, $$tag: 'tag3'}};
+      fakeThreading.idTable[3] = cached;
+
+      annotationMapper.loadAnnotations(annotations, replies);
+      assert($rootScope.$emit.calledWith("annotationUpdated", {id: 3}))
+    });
+
     it('replaces the properties on the cached annotation with those from the loaded one', function () {
       sandbox.stub($rootScope, '$emit');
       var annotations = [{id: 1, url: 'http://example.com'}];

h/static/scripts/test/annotation-viewer-controller-test.coffee

@@ -49,7 +49,7 @@ describe "AnnotationViewerController", ->
     {store} = createAnnotationViewerController({})
     assert store.SearchResource.get.calledOnce
     assert store.SearchResource.get.calledWith(
-      {_id: "test_annotation_id"})
+      {_id: "test_annotation_id", _separate_replies: true})
 
   it "passes the annotations and replies from search into loadAnnotations", ->
     {annotationMapper} = createAnnotationViewerController({

h/static/scripts/test/stream-controller-test.coffee

@@ -86,6 +86,11 @@ describe 'StreamController', ->
   afterEach ->
     sandbox.restore()
 
+  it 'calls the search API with _separate_replies: true', ->
+    createController()
+    assert.equal(
+      fakeStore.SearchResource.get.firstCall.args[0]._separate_replies, true)
+
   it 'passes the annotations and replies from search to loadAnnotations()', ->
     fakeStore.SearchResource.get = (query, func) ->
       func({

h/static/scripts/test/widget-controller-test.coffee

@@ -104,6 +104,15 @@ describe 'WidgetController', ->
       assert.calledWith(loadSpy, [60..79])
       assert.calledWith(loadSpy, [80..99])
 
+    it 'passes _separate_replies: true to the search API', ->
+      fakeStore.SearchResource.get = sandbox.stub()
+      fakeCrossFrame.frames.push({uri: 'http://example.com'})
+
+      $scope.$digest()
+
+      assert.equal(
+        fakeStore.SearchResource.get.firstCall.args[0]._separate_replies, true)
+
     it 'passes annotations and replies from search to loadAnnotations()', ->
       fakeStore.SearchResource.get = (query, callback) ->
         callback({

h/static/scripts/widget-controller.coffee

@@ -32,6 +32,7 @@ module.exports = class WidgetController
         order: 'asc'
         group: groups.focused().id
       q = angular.extend(queryCore, query)
+      q._separate_replies = true
 
       store.SearchResource.get q, (results) ->
         total = results.total