Merge pull request #11259 from edx/jia/MA-1930-featured-rebase
MA-1930 add result count in paginated endpoints
This commit is contained in:
@@ -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")
|
||||
|
||||
@@ -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):
|
||||
"""
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)."}
|
||||
)
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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={}):
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user