From e723fb6aecd817ca5b99882f0ae911e95a191b58 Mon Sep 17 00:00:00 2001 From: wajeeha-khalid Date: Mon, 18 Jan 2016 13:07:16 +0500 Subject: [PATCH] MA-1930 add result count in paginated endpoints --- lms/djangoapps/discussion_api/api.py | 24 ++++++++----- lms/djangoapps/discussion_api/pagination.py | 9 +++-- .../discussion_api/tests/test_api.py | 2 +- .../discussion_api/tests/test_views.py | 36 ++++++++++++++++--- lms/djangoapps/discussion_api/tests/utils.py | 3 ++ .../django_comment_client/forum/tests.py | 3 +- .../django_comment_client/forum/views.py | 36 +++++++++++++------ lms/lib/comment_client/thread.py | 11 ++++-- lms/lib/comment_client/user.py | 10 ++++-- lms/lib/comment_client/utils.py | 16 +++++++-- 10 files changed, 113 insertions(+), 37 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 825d23f4ad..05241a314d 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/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") diff --git a/lms/djangoapps/discussion_api/pagination.py b/lms/djangoapps/discussion_api/pagination.py index 0a705e8e88..099459510f 100644 --- a/lms/djangoapps/discussion_api/pagination.py +++ b/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): """ diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 3a4373b548..9ce1a9708d 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/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( diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index a92181fd54..355a834a56 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/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)."} + ) diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 030d33f8a4..e0613dc9d0 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/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 ) diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index a052fed399..2ac39cc644 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 7f4ce025a9..1459d4c39c 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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, diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 559ba35a3b..4316b97122 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -1,7 +1,7 @@ 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={}): diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 0326dea214..e88fe9ecd4 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -1,4 +1,5 @@ -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) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 626880c626..5c133fed8a 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -1,3 +1,4 @@ +"""" 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