Skip to content

E
edx-platform
Commit 9d6df1e1 by wajeeha-khalid
Options

Merge pull request #11189 from edx/ekafeel/discussion-api-namespace-pagination 

Ekafeel/discussion api namespace pagination
parents cf5dc107 83206535
Showing with 261 additions and 137 deletions
lms/djangoapps/discussion_api/api.py
......@@ -18,7 +18,6 @@ from courseware.courses import get_course_with_access
from discussion_api.exceptions import ThreadNotFoundError, CommentNotFoundError, DiscussionDisabledError
from discussion_api.forms import CommentActionsForm, ThreadActionsForm
from discussion_api.pagination import get_paginated_data
from discussion_api.permissions import (
can_delete,
get_editable_fields,
......@@ -38,6 +37,7 @@ from django_comment_common.signals import (
comment_deleted,
)
from django_comment_client.utils import get_accessible_discussion_modules, is_commentable_cohorted
from lms.djangoapps.discussion_api.pagination import DiscussionAPIPagination
from lms.lib.comment_client.comment import Comment
from lms.lib.comment_client.thread import Thread
from lms.lib.comment_client.utils import CommentClientRequestError
......@@ -328,22 +328,30 @@ 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]
ret = get_paginated_data(request, results, page, num_pages)
ret["text_search_rewrite"] = text_search_rewrite
return ret
results = [ThreadSerializer(thread, context=context).data for thread in paginated_results.collection]
paginator = DiscussionAPIPagination(
request,
paginated_results.page,
paginated_results.num_pages,
paginated_results.thread_count
)
return paginator.get_paginated_response({
"results": results,
"text_search_rewrite": paginated_results.corrected_text,
})
def get_comment_list(request, thread_id, endorsed, page, page_size):
......@@ -412,7 +420,8 @@ 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]
return get_paginated_data(request, results, page, num_pages)
paginator = DiscussionAPIPagination(request, page, num_pages, resp_total)
return paginator.get_paginated_response(results)
def _check_fields(allowed_fields, data, message):
......@@ -747,11 +756,15 @@ 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
return get_paginated_data(request, results, 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
......@@ -2,6 +2,7 @@
Discussion API pagination support
"""
from rest_framework.utils.urls import replace_query_param
from openedx.core.lib.api.paginators import NamespacedPageNumberPagination
class _Page(object):
......@@ -9,12 +10,11 @@ class _Page(object):
Implements just enough of the django.core.paginator.Page interface to allow
PaginationSerializer to work.
"""
def __init__(self, object_list, page_num, num_pages):
def __init__(self, page_num, num_pages):
"""
Create a new page containing the given objects, with the given page
number and number of pages
"""
self.object_list = object_list
self.page_num = page_num
self.num_pages = num_pages
......@@ -35,33 +35,50 @@ class _Page(object):
return self.page_num - 1
def get_paginated_data(request, results, page_num, per_page):
class DiscussionAPIPagination(NamespacedPageNumberPagination):
"""
Return a dict with the following values:
next: The URL for the next page
previous: The URL for the previous page
results: The results on this page
Subclasses NamespacedPageNumberPagination to provide custom implementation of pagination metadata
by overriding it's methods
"""
# Note: Previous versions of this function used Django Rest Framework's
# paginated serializer. With the upgrade to DRF 3.1, paginated serializers
# have been removed. We *could* use DRF's paginator classes, but there are
# some slight differences between how DRF does pagination and how we're doing
# pagination here. (For example, we respond with a next_url param even if
# there is only one result on the current page.) To maintain backwards
# compatability, we simulate the behavior that DRF used to provide.
page = _Page(results, page_num, per_page)
next_url, previous_url = None, None
base_url = request.build_absolute_uri()
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
if page.has_next():
next_url = replace_query_param(base_url, "page", page.next_page_number())
super(DiscussionAPIPagination, self).__init__()
if page.has_previous():
previous_url = replace_query_param(base_url, "page", page.previous_page_number())
def get_result_count(self):
"""
Returns total number of results
"""
return self.count
return {
"next": next_url,
"previous": previous_url,
"results": results,
}
def get_num_pages(self):
"""
Returns total number of pages the response is divided into
"""
return self.page.num_pages
def get_next_link(self):
"""
Returns absolute url of the next page if there's a next page available
otherwise returns None
"""
next_url = None
if self.page.has_next():
next_url = replace_query_param(self.base_url, "page", self.page.next_page_number())
return next_url
def get_previous_link(self):
"""
Returns absolute url of the previous page if there's a previous page available
otherwise returns None
"""
previous_url = None
if self.page.has_previous():
previous_url = replace_query_param(self.base_url, "page", self.page.previous_page_number())
return previous_url
lms/djangoapps/discussion_api/tests/test_pagination.py
......@@ -5,7 +5,8 @@ from unittest import TestCase
from django.test import RequestFactory
from discussion_api.pagination import get_paginated_data
from discussion_api.pagination import DiscussionAPIPagination
from discussion_api.tests.utils import make_paginated_api_response
class PaginationSerializerTest(TestCase):
......@@ -16,55 +17,45 @@ class PaginationSerializerTest(TestCase):
parameters returns the expected result
"""
request = RequestFactory().get("/test")
actual = get_paginated_data(request, objects, page_num, num_pages)
self.assertEqual(actual, expected)
paginator = DiscussionAPIPagination(request, page_num, num_pages)
actual = paginator.get_paginated_response(objects)
self.assertEqual(actual.data, expected)
def test_empty(self):
self.do_case(
[], 1, 0,
{
"next": None,
"previous": None,
"results": [],
}
[], 1, 0, make_paginated_api_response(
results=[], count=0, num_pages=0, next_link=None, previous_link=None
)
)
def test_only_page(self):
self.do_case(
["foo"], 1, 1,
{
"next": None,
"previous": None,
"results": ["foo"],
}
["foo"], 1, 1, make_paginated_api_response(
results=["foo"], count=0, num_pages=1, next_link=None, previous_link=None
)
)
def test_first_of_many(self):
self.do_case(
["foo"], 1, 3,
{
"next": "http://testserver/test?page=2",
"previous": None,
"results": ["foo"],
}
["foo"], 1, 3, make_paginated_api_response(
results=["foo"], count=0, num_pages=3, next_link="http://testserver/test?page=2", previous_link=None
)
)
def test_last_of_many(self):
self.do_case(
["foo"], 3, 3,
{
"next": None,
"previous": "http://testserver/test?page=2",
"results": ["foo"],
}
["foo"], 3, 3, make_paginated_api_response(
results=["foo"], count=0, num_pages=3, next_link=None, previous_link="http://testserver/test?page=2"
)
)
def test_middle_of_many(self):
self.do_case(
["foo"], 2, 3,
{
"next": "http://testserver/test?page=3",
"previous": "http://testserver/test?page=1",
"results": ["foo"],
}
["foo"], 2, 3, make_paginated_api_response(
results=["foo"],
count=0,
num_pages=3,
next_link="http://testserver/test?page=3",
previous_link="http://testserver/test?page=1"
)
)
lms/djangoapps/discussion_api/tests/test_views.py
......@@ -23,6 +23,7 @@ from discussion_api.tests.utils import (
CommentsServiceMockMixin,
make_minimal_cs_comment,
make_minimal_cs_thread,
make_paginated_api_response
)
from student.tests.factories import CourseEnrollmentFactory, UserFactory
from util.testing import UrlResetMixin, PatchMediaTypeMixin
......@@ -307,15 +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_response = make_paginated_api_response(
results=expected_threads,
count=1,
num_pages=2,
next_link="http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&page=2",
previous_link=None
)
expected_response.update({"text_search_rewrite": None})
self.assert_response_correct(
response,
200,
{
"results": expected_threads,
"next": "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&page=2",
"previous": None,
"text_search_rewrite": None,
}
expected_response
)
self.assert_last_query_params({
"user_id": [unicode(self.user.id)],
......@@ -374,15 +378,20 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
def test_text_search(self):
self.register_get_user_response(self.user)
self.register_get_threads_search_response([], None)
self.register_get_threads_search_response([], None, num_pages=0)
response = self.client.get(
self.url,
{"course_id": unicode(self.course.id), "text_search": "test search string"}
)
expected_response = make_paginated_api_response(
results=[], count=0, num_pages=0, next_link=None, previous_link=None
)
expected_response.update({"text_search_rewrite": None})
self.assert_response_correct(
response,
200,
{"results": [], "next": None, "previous": None, "text_search_rewrite": None}
expected_response
)
self.assert_last_query_params({
"user_id": [unicode(self.user.id)],
......@@ -398,7 +407,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
@ddt.data(True, "true", "1")
def test_following_true(self, following):
self.register_get_user_response(self.user)
self.register_subscribed_threads_response(self.user, [], page=1, num_pages=1)
self.register_subscribed_threads_response(self.user, [], page=1, num_pages=0)
response = self.client.get(
self.url,
{
......@@ -406,10 +415,15 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
"following": following,
}
)
expected_response = make_paginated_api_response(
results=[], count=0, num_pages=0, next_link=None, previous_link=None
)
expected_response.update({"text_search_rewrite": None})
self.assert_response_correct(
response,
200,
{"results": [], "next": None, "previous": None, "text_search_rewrite": None}
expected_response
)
self.assertEqual(
urlparse(httpretty.last_request().path).path,
......@@ -933,16 +947,15 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
"resp_total": 100,
})
response = self.client.get(self.url, {"thread_id": self.thread_id})
next_link = "http://testserver/api/discussion/v1/comments/?page=2&thread_id={}".format(
self.thread_id
)
self.assert_response_correct(
response,
200,
{
"results": expected_comments,
"next": "http://testserver/api/discussion/v1/comments/?page=2&thread_id={}".format(
self.thread_id
),
"previous": None,
}
make_paginated_api_response(
results=expected_comments, count=100, num_pages=10, next_link=next_link, previous_link=None
)
)
self.assert_query_params_equal(
httpretty.httpretty.latest_requests[-2],
......@@ -1427,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,11 +67,12 @@ class CommentsServiceMockMixin(object):
"collection": threads,
"page": page,
"num_pages": num_pages,
"thread_count": len(threads),
}),
status=200
)
def register_get_threads_search_response(self, threads, rewrite):
def register_get_threads_search_response(self, threads, rewrite, num_pages=1):
"""Register a mock response for GET on the CS thread search endpoint"""
httpretty.register_uri(
httpretty.GET,
......@@ -79,8 +80,9 @@ class CommentsServiceMockMixin(object):
body=json.dumps({
"collection": threads,
"page": 1,
"num_pages": 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
)
......@@ -371,3 +374,18 @@ def make_minimal_cs_comment(overrides=None):
}
ret.update(overrides or {})
return ret
def make_paginated_api_response(results=None, count=0, num_pages=0, next_link=None, previous_link=None):
"""
Generates the response dictionary of paginated APIs with passed data
"""
return {
"pagination": {
"next": next_link,
"previous": previous_link,
"count": count,
"num_pages": num_pages,
},
"results": results or []
}
lms/djangoapps/discussion_api/views.py
......@@ -261,19 +261,17 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
form = ThreadListGetForm(request.GET)
if not form.is_valid():
raise ValidationError(form.errors)
return Response(
get_thread_list(
request,
form.cleaned_data["course_id"],
form.cleaned_data["page"],
form.cleaned_data["page_size"],
form.cleaned_data["topic_id"],
form.cleaned_data["text_search"],
form.cleaned_data["following"],
form.cleaned_data["view"],
form.cleaned_data["order_by"],
form.cleaned_data["order_direction"],
)
return get_thread_list(
request,
form.cleaned_data["course_id"],
form.cleaned_data["page"],
form.cleaned_data["page_size"],
form.cleaned_data["topic_id"],
form.cleaned_data["text_search"],
form.cleaned_data["following"],
form.cleaned_data["view"],
form.cleaned_data["order_by"],
form.cleaned_data["order_direction"],
)
def retrieve(self, request, thread_id=None):
......@@ -443,14 +441,12 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet):
form = CommentListGetForm(request.GET)
if not form.is_valid():
raise ValidationError(form.errors)
return Response(
get_comment_list(
request,
form.cleaned_data["thread_id"],
form.cleaned_data["endorsed"],
form.cleaned_data["page"],
form.cleaned_data["page_size"]
)
return get_comment_list(
request,
form.cleaned_data["thread_id"],
form.cleaned_data["endorsed"],
form.cleaned_data["page"],
form.cleaned_data["page_size"]
)
def retrieve(self, request, comment_id=None):
......@@ -460,13 +456,11 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet):
form = _PaginationForm(request.GET)
if not form.is_valid():
raise ValidationError(form.errors)
return Response(
get_response_comments(
request,
comment_id,
form.cleaned_data["page"],
form.cleaned_data["page_size"]
)
return get_response_comments(
request,
comment_id,
form.cleaned_data["page"],
form.cleaned_data["page_size"]
)
def create(self, request):
......
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
openedx/core/lib/api/paginators.py
......@@ -39,6 +39,18 @@ class NamespacedPageNumberPagination(pagination.PageNumberPagination):
page_size_query_param = "page_size"
def get_result_count(self):
"""
Returns total number of results
"""
return self.page.paginator.count
def get_num_pages(self):
"""
Returns total number of pages the results are divided into
"""
return self.page.paginator.num_pages
def get_paginated_response(self, data):
"""
Annotate the response with pagination information
......@@ -46,8 +58,8 @@ class NamespacedPageNumberPagination(pagination.PageNumberPagination):
metadata = {
'next': self.get_next_link(),
'previous': self.get_previous_link(),
'count': self.page.paginator.count,
'num_pages': self.page.paginator.num_pages,
'count': self.get_result_count(),
'num_pages': self.get_num_pages(),
}
if isinstance(data, dict):
if 'results' not in data:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment