MA-1930 add result count in paginated endpoints

lms/djangoapps/discussion_api/api.py

@@ -328,24 +328,29 @@ def get_thread_list(
             })
 
     if following:
-        threads, result_page, num_pages = context["cc_requester"].subscribed_threads(query_params)
+        paginated_results = context["cc_requester"].subscribed_threads(query_params)
     else:
         query_params["course_id"] = unicode(course.id)
         query_params["commentable_ids"] = ",".join(topic_id_list) if topic_id_list else None
         query_params["text"] = text_search
-        threads, result_page, num_pages, text_search_rewrite = Thread.search(query_params)
+        paginated_results = Thread.search(query_params)
     # The comments service returns the last page of results if the requested
     # page is beyond the last page, but we want be consistent with DRF's general
     # behavior and return a PageNotFoundError in that case
-    if result_page != page:
+    if paginated_results.page != page:
         raise PageNotFoundError("Page not found (No results on this page).")
 
-    results = [ThreadSerializer(thread, context=context).data for thread in threads]
+    results = [ThreadSerializer(thread, context=context).data for thread in paginated_results.collection]
 
-    paginator = DiscussionAPIPagination(request, result_page, num_pages)
+    paginator = DiscussionAPIPagination(
+        request,
+        paginated_results.page,
+        paginated_results.num_pages,
+        paginated_results.thread_count
+    )
     return paginator.get_paginated_response({
         "results": results,
-        "text_search_rewrite": text_search_rewrite,
+        "text_search_rewrite": paginated_results.corrected_text,
     })
 
 
@@ -415,7 +420,7 @@ def get_comment_list(request, thread_id, endorsed, page, page_size):
     num_pages = (resp_total + page_size - 1) / page_size if resp_total else 1
 
     results = [CommentSerializer(response, context=context).data for response in responses]
-    paginator = DiscussionAPIPagination(request, page, num_pages)
+    paginator = DiscussionAPIPagination(request, page, num_pages, resp_total)
     return paginator.get_paginated_response(results)
 
 
@@ -751,11 +756,14 @@ def get_response_comments(request, comment_id, page, page_size):
 
         response_skip = page_size * (page - 1)
         paged_response_comments = response_comments[response_skip:(response_skip + page_size)]
+        if len(paged_response_comments) == 0 and page != 1:
+            raise PageNotFoundError("Page not found (No results on this page).")
+
         results = [CommentSerializer(comment, context=context).data for comment in paged_response_comments]
 
         comments_count = len(response_comments)
         num_pages = (comments_count + page_size - 1) / page_size if comments_count else 1
-        paginator = DiscussionAPIPagination(request, page, num_pages)
+        paginator = DiscussionAPIPagination(request, page, num_pages, comments_count)
         return paginator.get_paginated_response(results)
     except CommentClientRequestError:
         raise CommentNotFoundError("Comment not found")

lms/djangoapps/discussion_api/pagination.py

@@ -40,23 +40,22 @@ class DiscussionAPIPagination(NamespacedPageNumberPagination):
     Subclasses NamespacedPageNumberPagination to provide custom implementation of pagination metadata
     by overriding it's methods
     """
-    def __init__(self, request, page_num, num_pages):
+    def __init__(self, request, page_num, num_pages, result_count=0):
         """
         Overrides parent constructor to take information from discussion api
         essential for the parent method
         """
         self.page = _Page(page_num, num_pages)
         self.base_url = request.build_absolute_uri()
+        self.count = result_count
 
         super(DiscussionAPIPagination, self).__init__()
 
     def get_result_count(self):
         """
-        A count can't be calculated with the information coming from the
-        Forum's api, so returning 0 for the parent method to work
+        Returns total number of results
         """
-        # TODO: return actual count
-        return 0
+        return self.count
 
     def get_num_pages(self):
         """

lms/djangoapps/discussion_api/tests/test_api.py

@@ -695,7 +695,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
         ]
 
         expected_result = make_paginated_api_response(
-            results=expected_threads, count=0, num_pages=1, next_link=None, previous_link=None
+            results=expected_threads, count=2, num_pages=1, next_link=None, previous_link=None
         )
         expected_result.update({"text_search_rewrite": None})
         self.assertEqual(

lms/djangoapps/discussion_api/tests/test_views.py

@@ -308,18 +308,18 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
         }]
         self.register_get_threads_response(source_threads, page=1, num_pages=2)
         response = self.client.get(self.url, {"course_id": unicode(self.course.id), "following": ""})
-        expected_respoonse = make_paginated_api_response(
+        expected_response = make_paginated_api_response(
             results=expected_threads,
-            count=0,
+            count=1,
             num_pages=2,
             next_link="http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&page=2",
             previous_link=None
         )
-        expected_respoonse.update({"text_search_rewrite": None})
+        expected_response.update({"text_search_rewrite": None})
         self.assert_response_correct(
             response,
             200,
-            expected_respoonse
+            expected_response
         )
         self.assert_last_query_params({
             "user_id": [unicode(self.user.id)],
@@ -954,7 +954,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             response,
             200,
             make_paginated_api_response(
-                results=expected_comments, count=0, num_pages=10, next_link=next_link, previous_link=None
+                results=expected_comments, count=100, num_pages=10, next_link=next_link, previous_link=None
             )
         )
         self.assert_query_params_equal(
@@ -1440,3 +1440,29 @@ class CommentViewSetRetrieveTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase
         self.register_get_comment_error_response(self.comment_id, 404)
         response = self.client.get(self.url)
         self.assertEqual(response.status_code, 404)
+
+    def test_pagination(self):
+        """
+        Test that pagination parameters are correctly plumbed through to the
+        comments service and that a 404 is correctly returned if a page past the
+        end is requested
+        """
+        self.register_get_user_response(self.user)
+        cs_comment_child = self.make_comment_data("test_child_comment", self.comment_id, children=[])
+        cs_comment = self.make_comment_data(self.comment_id, None, [cs_comment_child])
+        cs_thread = make_minimal_cs_thread({
+            "id": self.thread_id,
+            "course_id": unicode(self.course.id),
+            "children": [cs_comment],
+        })
+        self.register_get_thread_response(cs_thread)
+        self.register_get_comment_response(cs_comment)
+        response = self.client.get(
+            self.url,
+            {"comment_id": self.comment_id, "page": "18", "page_size": "4"}
+        )
+        self.assert_response_correct(
+            response,
+            404,
+            {"developer_message": "Page not found (No results on this page)."}
+        )

lms/djangoapps/discussion_api/tests/utils.py

@@ -67,6 +67,7 @@ class CommentsServiceMockMixin(object):
                 "collection": threads,
                 "page": page,
                 "num_pages": num_pages,
+                "thread_count": len(threads),
             }),
             status=200
         )
@@ -81,6 +82,7 @@ class CommentsServiceMockMixin(object):
                 "page": 1,
                 "num_pages": num_pages,
                 "corrected_text": rewrite,
+                "thread_count": len(threads),
             }),
             status=200
         )
@@ -200,6 +202,7 @@ class CommentsServiceMockMixin(object):
                 "collection": threads,
                 "page": page,
                 "num_pages": num_pages,
+                "thread_count": len(threads),
             }),
             status=200
         )

lms/djangoapps/django_comment_client/forum/tests.py

@@ -6,6 +6,7 @@ from django.core.urlresolvers import reverse
 from django.http import Http404
 from django.test.client import Client, RequestFactory
 from django.test.utils import override_settings
+from lms.lib.comment_client.utils import CommentClientPaginatedResult
 from edxmako.tests import mako_middleware_process_request
 
 from django_comment_common.utils import ThreadContext
@@ -96,7 +97,7 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase):
 
         # Mock the code that makes the HTTP requests to the cs_comment_service app
         # for the profiled user's active threads
-        mock_threads.return_value = [], 1, 1
+        mock_threads.return_value = CommentClientPaginatedResult(collection=[], page=1, num_pages=1)
 
         # Mock the code that makes the HTTP request to the cs_comment_service app
         # that gets the current user's info

lms/djangoapps/django_comment_client/forum/views.py

@@ -146,7 +146,8 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
         )
     )
 
-    threads, page, num_pages, corrected_text = cc.Thread.search(query_params)
+    paginated_results = cc.Thread.search(query_params)
+    threads = paginated_results.collection
 
     # If not provided with a discussion id, filter threads by commentable ids
     # which are accessible to the current user.
@@ -162,9 +163,9 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
         if 'pinned' not in thread:
             thread['pinned'] = False
 
-    query_params['page'] = page
-    query_params['num_pages'] = num_pages
-    query_params['corrected_text'] = corrected_text
+    query_params['page'] = paginated_results.page
+    query_params['num_pages'] = paginated_results.num_pages
+    query_params['corrected_text'] = paginated_results.corrected_text
 
     return threads, query_params
 
@@ -336,7 +337,12 @@ def single_thread(request, course_key, discussion_id, thread_id):
     is_staff = has_permission(request.user, 'openclose_thread', course.id)
     if request.is_ajax():
         with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"):
-            annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info)
+            annotated_content_info = utils.get_annotated_content_infos(
+                course_key,
+                thread,
+                request.user,
+                user_info=user_info
+            )
         content = utils.prepare_content(thread.to_dict(), course_key, is_staff)
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
             add_courseware_context([content], course, request.user)
@@ -511,18 +517,26 @@ def followed_threads(request, course_key, user_id):
         if group_id is not None:
             query_params['group_id'] = group_id
 
-        threads, page, num_pages = profiled_user.subscribed_threads(query_params)
-        query_params['page'] = page
-        query_params['num_pages'] = num_pages
+        paginated_results = profiled_user.subscribed_threads(query_params)
+        print "\n \n \n paginated results \n \n \n "
+        print paginated_results
+        query_params['page'] = paginated_results.page
+        query_params['num_pages'] = paginated_results.num_pages
         user_info = cc.User.from_django_user(request.user).to_dict()
 
         with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"):
-            annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
+            annotated_content_info = utils.get_metadata_for_threads(
+                course_key,
+                paginated_results.collection,
+                request.user, user_info
+            )
         if request.is_ajax():
             is_staff = has_permission(request.user, 'openclose_thread', course.id)
             return utils.JsonResponse({
                 'annotated_content_info': annotated_content_info,
-                'discussion_data': [utils.prepare_content(thread, course_key, is_staff) for thread in threads],
+                'discussion_data': [
+                    utils.prepare_content(thread, course_key, is_staff) for thread in paginated_results.collection
+                ],
                 'page': query_params['page'],
                 'num_pages': query_params['num_pages'],
             })
@@ -533,7 +547,7 @@ def followed_threads(request, course_key, user_id):
                 'user': request.user,
                 'django_user': User.objects.get(id=user_id),
                 'profiled_user': profiled_user.to_dict(),
-                'threads': json.dumps(threads),
+                'threads': json.dumps(paginated_results.collection),
                 'user_info': json.dumps(user_info),
                 'annotated_content_info': json.dumps(annotated_content_info),
                 #                'content': content,

lms/lib/comment_client/thread.py

 import logging
 
 from eventtracking import tracker
-from .utils import merge_dict, strip_blank, strip_none, extract, perform_request
+from .utils import merge_dict, strip_blank, strip_none, extract, perform_request, CommentClientPaginatedResult
 from .utils import CommentClientRequestError
 import models
 import settings
@@ -94,7 +94,14 @@ class Thread(models.Model):
                     total_results=total_results
                 )
             )
-        return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1), response.get('corrected_text')
+
+        return CommentClientPaginatedResult(
+            collection=response.get('collection', []),
+            page=response.get('page', 1),
+            num_pages=response.get('num_pages', 1),
+            thread_count=response.get('thread_count', 0),
+            corrected_text=response.get('corrected_text', None)
+        )
 
     @classmethod
     def url_for_threads(cls, params={}):

lms/lib/comment_client/user.py

-from .utils import merge_dict, perform_request, CommentClientRequestError
+""" User model wrapper for comment service"""
+from .utils import merge_dict, perform_request, CommentClientRequestError, CommentClientPaginatedResult
 
 import models
 import settings
@@ -113,7 +114,12 @@ class User(models.Model):
             metric_tags=self._metric_tags,
             paged_results=True
         )
-        return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1)
+        return CommentClientPaginatedResult(
+            collection=response.get('collection', []),
+            page=response.get('page', 1),
+            num_pages=response.get('num_pages', 1),
+            thread_count=response.get('thread_count', 0)
+        )
 
     def _retrieve(self, *args, **kwargs):
         url = self.url(action='get', params=self.attributes)

lms/lib/comment_client/utils.py

+"""" Common utilities for comment client wrapper """
 from contextlib import contextmanager
 import dogstats_wrapper as dog_stats_api
 import logging
@@ -141,9 +142,9 @@ class CommentClientError(Exception):
 
 
 class CommentClientRequestError(CommentClientError):
-    def __init__(self, msg, status_code=400):
+    def __init__(self, msg, status_codes=400):
         super(CommentClientRequestError, self).__init__(msg)
-        self.status_code = status_code
+        self.status_code = status_codes
 
 
 class CommentClient500Error(CommentClientError):
@@ -152,3 +153,14 @@ class CommentClient500Error(CommentClientError):
 
 class CommentClientMaintenanceError(CommentClientError):
     pass
+
+
+class CommentClientPaginatedResult(object):
+    """ class for paginated results returned from comment services"""
+
+    def __init__(self, collection, page, num_pages, thread_count=0, corrected_text=None):
+        self.collection = collection
+        self.page = page
+        self.num_pages = num_pages
+        self.thread_count = thread_count
+        self.corrected_text = corrected_text