Merge pull request #50 from edx/mushtaq/edxnotes-multiple-usageids

Support bulk search in /search endpoint

AUTHORS

@@ -2,3 +2,4 @@ Oleg Marshev <oleg@edx.org>
 Tim Babych <tim.babych@gmail.com>
 Christina Roberts <christina@edx.org>
 Ben McMorran <ben.mcmorran@gmail.com>
+Mushtaq Ali <mushtaak@gmail.com>

notesapi/v1/tests/test_views.py

@@ -680,6 +680,32 @@ class AnnotationSearchViewTests(BaseAnnotationViewTests):
         search_and_verify("First", "First one", [])
         search_and_verify("Second", "Second note", ["tag1", "tag2"])
 
+    @ddt.data(True, False)
+    def test_usage_id_search(self, is_es_disabled):
+        """
+        Verifies the search with usage id.
+        """
+        self._create_annotation(text=u'First one', usage_id='test-1')
+        self._create_annotation(text=u'Second note', usage_id='test-2')
+        self._create_annotation(text=u'Third note', usage_id='test-3')
+
+        @patch('django.conf.settings.ES_DISABLED', is_es_disabled)
+        def verify_usage_id_search(usage_ids):
+            """
+            Verify search results based on usage id operation.
+
+            Arguments:
+                usage_ids: List. The identifier string of the annotations XBlock(s).
+            """
+            results = self._get_search_results(usage_id=usage_ids)
+            self.assertEqual(len(results), len(usage_ids))
+            # Here we are reverse-traversing usage_ids because response has descending ordered rows.
+            for index, u_id in enumerate(usage_ids[::-1]):
+                self.assertEqual(results[index]['usage_id'], u_id)
+
+        verify_usage_id_search(usage_ids=['test-1'])
+        verify_usage_id_search(usage_ids=['test-1', 'test-2', 'test-3'])
+
     def test_search_deleted(self):
         """
         Tests for search method to not return deleted notes.

notesapi/v1/views.py

@@ -35,10 +35,12 @@ class AnnotationSearchView(GenericAPIView):
     """
     **Use Case**
 
-        * Search and return a paginated list of annotations for a user.
+        * Search and return a list of annotations for a user.
 
             The annotations are always sorted in descending order by updated date.
 
+            Response is paginated by default except usage_id based search.
+
             Each page in the list contains 25 annotations by default. The page
             size can be altered by passing parameter "page_size=<page_size>".
 
@@ -58,6 +60,8 @@ class AnnotationSearchView(GenericAPIView):
 
         GET /api/v1/search/
         GET /api/v1/search/?course_id={course_id}&user={user_id}
+        GET /api/v1/search/?course_id={course_id}&user={user_id}&usage_id={usage_id}
+        GET /api/v1/search/?course_id={course_id}&user={user_id}&usage_id={usage_id}&usage_id={usage_id} ...
 
     **Query Parameters for GET**
 
@@ -67,6 +71,8 @@ class AnnotationSearchView(GenericAPIView):
 
         * user: Anonymized user id.
 
+        * usage_id: The identifier string of the annotations XBlock.
+
         * text: Student's thoughts on the quote
 
         * highlight: dict. Only used when search from ElasticSearch. It contains two keys:
@@ -109,31 +115,49 @@ class AnnotationSearchView(GenericAPIView):
 
             * updated: DateTime. When was the last time annotation was updated.
     """
+    params = {}
+    query_params = {}
+    search_with_usage_id = False
 
     def get(self, *args, **kwargs):  # pylint: disable=unused-argument
         """
         Search annotations in most appropriate storage
         """
+        self.query_params = {}
+        self.search_with_usage_id = False
+        self.params = self.request.query_params.dict()
+
+        usage_ids = self.request.query_params.getlist('usage_id')
+        if len(usage_ids) > 0:
+            self.search_with_usage_id = True
+            self.query_params['usage_id__in'] = usage_ids
+
+        if 'course_id' in self.params:
+            self.query_params['course_id'] = self.params['course_id']
+
         # search in DB when ES is not available or there is no need to bother it
-        if settings.ES_DISABLED or 'text' not in self.request.query_params.dict():
+        if settings.ES_DISABLED or 'text' not in self.params:
+            if 'user' in self.params:
+                self.query_params['user_id'] = self.params['user']
             return self.get_from_db(*args, **kwargs)
         else:
+            if 'user' in self.params:
+                self.query_params['user'] = self.params['user']
             return self.get_from_es(*args, **kwargs)
 
     def get_from_db(self, *args, **kwargs):  # pylint: disable=unused-argument
         """
         Search annotations in database.
         """
-        params = self.request.query_params.dict()
-        query = Note.objects.filter(
-            **{f: v for (f, v) in params.items() if f in ('course_id', 'usage_id')}
-        ).order_by('-updated')
+        query = Note.objects.filter(**self.query_params).order_by('-updated')
 
-        if 'user' in params:
-            query = query.filter(user_id=params['user'])
+        if 'text' in self.params:
+            query = query.filter(Q(text__icontains=self.params['text']) | Q(tags__icontains=self.params['text']))
 
-        if 'text' in params:
-            query = query.filter(Q(text__icontains=params['text']) | Q(tags__icontains=params['text']))
+        # Do not send paginated result if usage id based search.
+        if self.search_with_usage_id:
+            serializer = NoteSerializer(query, many=True)
+            return Response(serializer.data, status=status.HTTP_200_OK)
 
         page = self.paginate_queryset(query)
         serializer = NoteSerializer(page, many=True)
@@ -144,16 +168,13 @@ class AnnotationSearchView(GenericAPIView):
         """
         Search annotations in ElasticSearch.
         """
-        params = self.request.query_params.dict()
-        query = SearchQuerySet().models(Note).filter(
-            **{f: v for (f, v) in params.items() if f in ('user', 'course_id', 'usage_id')}
-        )
+        query = SearchQuerySet().models(Note).filter(**self.query_params)
 
-        if 'text' in params:
-            clean_text = query.query.clean(params['text'])
+        if 'text' in self.params:
+            clean_text = query.query.clean(self.params['text'])
             query = query.filter(SQ(data=clean_text))
 
-        if params.get('highlight'):
+        if self.params.get('highlight'):
             opts = {
                 'pre_tags': ['{elasticsearch_highlight_start}'],
                 'post_tags': ['{elasticsearch_highlight_end}'],
@@ -161,6 +182,11 @@ class AnnotationSearchView(GenericAPIView):
             }
             query = query.highlight(**opts)
 
+        # Do not send paginated result if usage id based search.
+        if self.search_with_usage_id:
+            serializer = NotesElasticSearchSerializer(query, many=True)
+            return Response(serializer.data, status=status.HTTP_200_OK)
+
         page = self.paginate_queryset(query)
         serializer = NotesElasticSearchSerializer(page, many=True)
         response = self.get_paginated_response(serializer.data)

notesserver/test_views.py

+import json
 import datetime
 from unittest import skipIf
 from mock import patch, Mock
@@ -17,7 +18,7 @@ class OperationalEndpointsTest(APITestCase):
         """
         response = self.client.get(reverse('heartbeat'))
         self.assertEquals(response.status_code, 200)
-        self.assertEquals(response.data, {"OK": True})
+        self.assertEquals(json.loads(response.content), {"OK": True})
 
     @skipIf(settings.ES_DISABLED, "Do not test if Elasticsearch service is disabled.")
     @patch('notesserver.views.get_es')
@@ -28,7 +29,7 @@ class OperationalEndpointsTest(APITestCase):
         mocked_get_es.return_value.ping.return_value = False
         response = self.client.get(reverse('heartbeat'))
         self.assertEquals(response.status_code, 500)
-        self.assertEquals(response.data, {"OK": False, "check": "es"})
+        self.assertEquals(json.loads(response.content), {"OK": False, "check": "es"})
 
     @patch("django.db.backends.utils.CursorWrapper")
     def test_heartbeat_failure_db(self, mocked_cursor_wrapper):
@@ -38,7 +39,7 @@ class OperationalEndpointsTest(APITestCase):
         mocked_cursor_wrapper.side_effect = Exception
         response = self.client.get(reverse('heartbeat'))
         self.assertEquals(response.status_code, 500)
-        self.assertEquals(response.data, {"OK": False, "check": "db"})
+        self.assertEquals(json.loads(response.content), {"OK": False, "check": "db"})
 
     def test_root(self):
         """

notesserver/views.py

@@ -3,6 +3,8 @@ import datetime
 
 from django.db import connection
 from django.conf import settings
+from django.http import JsonResponse
+
 from rest_framework import status
 from rest_framework.permissions import AllowAny
 from rest_framework.response import Response
@@ -38,12 +40,12 @@ def heartbeat(request):  # pylint: disable=unused-argument
     try:
         db_status()
     except Exception:
-        return Response({"OK": False, "check": "db"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+        return JsonResponse({"OK": False, "check": "db"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
 
     if not settings.ES_DISABLED and not get_es().ping():
-        return Response({"OK": False, "check": "es"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+        return JsonResponse({"OK": False, "check": "es"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)
 
-    return Response({"OK": True})
+    return JsonResponse({"OK": True}, status=status.HTTP_200_OK)
 
 
 @api_view(['GET'])