Merge pull request #10727 from edx/cdyer/pagination

Add pagination support to course catalog API

lms/djangoapps/course_api/api.py

@@ -9,7 +9,6 @@ from rest_framework.exceptions import NotFound, PermissionDenied
 from lms.djangoapps.courseware.courses import get_courses, get_course_with_access
 
 from .permissions import can_view_courses_for_username
-from .serializers import CourseSerializer
 
 
 def get_effective_user(requesting_user, target_username):
@@ -41,14 +40,14 @@ def course_detail(request, username, course_key):
         course_key (CourseKey): Identifies the course of interest
 
     Return value:
-        CourseSerializer object representing the requested course
+        `CourseDescriptor` object representing the requested course
     """
     user = get_effective_user(request.user, username)
     try:
         course = get_course_with_access(user, 'see_exists', course_key)
     except Http404:
         raise NotFound()
-    return CourseSerializer(course, context={'request': request}).data
+    return course
 
 
 def list_courses(request, username):
@@ -69,8 +68,8 @@ def list_courses(request, username):
 
 
     Return value:
-        A CourseSerializer object representing the collection of courses.
+        List of `CourseDescriptor` objects representing the collection of courses.
     """
     user = get_effective_user(request.user, username)
     courses = get_courses(user)
-    return CourseSerializer(courses, context={'request': request}, many=True).data
+    return courses

lms/djangoapps/course_api/serializers.py

@@ -42,7 +42,7 @@ class CourseSerializer(serializers.Serializer):  # pylint: disable=abstract-meth
     Serializer for Course objects
     """
 
-    course_id = serializers.CharField(source='id', read_only=True)  # pylint: disable=invalid-name
+    course_id = serializers.CharField(source='id', read_only=True)
     name = serializers.CharField(source='display_name_with_default')
     number = serializers.CharField(source='display_number_with_default')
     org = serializers.CharField(source='display_org_with_default')
@@ -88,7 +88,8 @@ class CourseSerializer(serializers.Serializer):  # pylint: disable=abstract-meth
         """
         Get the representation for SerializerMethodField `blocks_url`
         """
-        return '?'.join([
+        base_url = '?'.join([
             reverse('blocks_in_course'),
             urllib.urlencode({'course_id': course.id}),
         ])
+        return self.context['request'].build_absolute_uri(base_url)

lms/djangoapps/course_api/tests/test_api.py

@@ -2,14 +2,14 @@
 Test for course API
 """
 
-from datetime import datetime
 from django.contrib.auth.models import AnonymousUser
-from django.test import RequestFactory
 from rest_framework.exceptions import NotFound, PermissionDenied
+from rest_framework.request import Request
+from rest_framework.test import APIRequestFactory
 
 from opaque_keys.edx.keys import CourseKey
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ModuleStoreTestCase
-from xmodule.course_module import DEFAULT_START_DATE
+from xmodule.course_module import CourseDescriptor
 
 from ..api import course_detail, list_courses
 from .mixins import CourseApiFactoryMixin
@@ -42,10 +42,20 @@ class CourseApiTestMixin(CourseApiFactoryMixin):
         'blocks_url': '/api/courses/v1/blocks/?course_id=edX%2Ftoy%2F2012_Fall',
     }
 
+    def verify_course(self, course, course_id=None):
+        """
+        Ensure that the returned course is the course we just created
+        """
+
+        if course_id is None:
+            course_id = self.expected_course_data['course_id']
+        self.assertIsInstance(course, CourseDescriptor)
+        self.assertEqual(course_id, str(course.id))
+
     @classmethod
     def setUpClass(cls):
         super(CourseApiTestMixin, cls).setUpClass()
-        cls.request_factory = RequestFactory()
+        cls.request_factory = APIRequestFactory()
 
 
 class CourseDetailTestMixin(CourseApiTestMixin):
@@ -57,9 +67,9 @@ class CourseDetailTestMixin(CourseApiTestMixin):
         Call the `course_detail` api endpoint to get information on the course
         identified by `course_key`.
         """
-        request = self.request_factory.get('/')
+        request = Request(self.request_factory.get('/'))
         request.user = requesting_user
-        return course_detail(request, target_user, course_key)
+        return course_detail(request, target_user.username, course_key)
 
 
 class TestGetCourseDetail(CourseDetailTestMixin, SharedModuleStoreTestCase):
@@ -75,54 +85,25 @@ class TestGetCourseDetail(CourseDetailTestMixin, SharedModuleStoreTestCase):
         cls.staff_user = cls.create_user('staff', is_staff=True)
 
     def test_get_existing_course(self):
-        result = self._make_api_call(self.honor_user, self.honor_user.username, self.course.id)
-        self.assertEqual(self.expected_course_data, result)
+        course = self._make_api_call(self.honor_user, self.honor_user, self.course.id)
+        self.verify_course(course)
 
     def test_get_nonexistent_course(self):
         course_key = CourseKey.from_string(u'edX/toy/nope')
         with self.assertRaises(NotFound):
-            self._make_api_call(self.honor_user, self.honor_user.username, course_key)
+            self._make_api_call(self.honor_user, self.honor_user, course_key)
 
     def test_hidden_course_for_honor(self):
         with self.assertRaises(NotFound):
-            self._make_api_call(self.honor_user, self.honor_user.username, self.hidden_course.id)
+            self._make_api_call(self.honor_user, self.honor_user, self.hidden_course.id)
 
     def test_hidden_course_for_staff(self):
-        result = self._make_api_call(self.staff_user, self.staff_user.username, self.hidden_course.id)
-        self.assertIsInstance(result, dict)
-        self.assertEqual(result['course_id'], u'edX/hidden/2012_Fall')
+        course = self._make_api_call(self.staff_user, self.staff_user, self.hidden_course.id)
+        self.verify_course(course, course_id=u'edX/hidden/2012_Fall')
 
     def test_hidden_course_for_staff_as_honor(self):
         with self.assertRaises(NotFound):
-            self._make_api_call(self.staff_user, self.honor_user.username, self.hidden_course.id)
-
-
-class TestGetCourseDetailStartDate(CourseDetailTestMixin, ModuleStoreTestCase):
-    """
-    Test variations of start_date field responses
-    """
-
-    def setUp(self):
-        super(TestGetCourseDetailStartDate, self).setUp()
-        self.staff_user = self.create_user('staff', is_staff=True)
-
-    def test_course_with_advertised_start(self):
-        course = self.create_course(
-            course=u'custom',
-            start=datetime(2015, 3, 15),
-            advertised_start=u'The Ides of March'
-        )
-        result = self._make_api_call(self.staff_user, self.staff_user.username, course.id)
-        self.assertEqual(result['course_id'], u'edX/custom/2012_Fall')
-        self.assertEqual(result['start_type'], u'string')
-        self.assertEqual(result['start_display'], u'The Ides of March')
-
-    def test_course_with_empty_start_date(self):
-        course = self.create_course(start=DEFAULT_START_DATE, course=u'custom2')
-        result = self._make_api_call(self.staff_user, self.staff_user.username, course.id)
-        self.assertEqual(result['course_id'], u'edX/custom2/2012_Fall')
-        self.assertEqual(result['start_type'], u'empty')
-        self.assertIsNone(result['start_display'])
+            self._make_api_call(self.staff_user, self.honor_user, self.hidden_course.id)
 
 
 class CourseListTestMixin(CourseApiTestMixin):
@@ -134,15 +115,23 @@ class CourseListTestMixin(CourseApiTestMixin):
         Call the list_courses api endpoint to get information about
         `specified_user` on behalf of `requesting_user`.
         """
-        request = self.request_factory.get('/')
+        request = Request(self.request_factory.get('/'))
         request.user = requesting_user
         return list_courses(request, specified_user.username)
 
+    def verify_courses(self, courses):
+        """
+        Verify that there is one course, and that it has the expected format.
+        """
+        self.assertEqual(len(courses), 1)
+        self.verify_course(courses[0])
+
 
 class TestGetCourseList(CourseListTestMixin, SharedModuleStoreTestCase):
     """
     Test the behavior of the `list_courses` api function.
     """
+
     @classmethod
     def setUpClass(cls):
         super(TestGetCourseList, cls).setUpClass()
@@ -153,17 +142,15 @@ class TestGetCourseList(CourseListTestMixin, SharedModuleStoreTestCase):
     def test_as_staff(self):
         courses = self._make_api_call(self.staff_user, self.staff_user)
         self.assertEqual(len(courses), 1)
-        self.assertEqual(courses[0], self.expected_course_data)
+        self.verify_courses(courses)
 
     def test_for_honor_user_as_staff(self):
         courses = self._make_api_call(self.staff_user, self.honor_user)
-        self.assertEqual(len(courses), 1)
-        self.assertEqual(courses[0], self.expected_course_data)
+        self.verify_courses(courses)
 
     def test_as_honor(self):
         courses = self._make_api_call(self.honor_user, self.honor_user)
-        self.assertEqual(len(courses), 1)
-        self.assertEqual(courses[0], self.expected_course_data)
+        self.verify_courses(courses)
 
     def test_for_staff_user_as_honor(self):
         with self.assertRaises(PermissionDenied):
@@ -172,8 +159,7 @@ class TestGetCourseList(CourseListTestMixin, SharedModuleStoreTestCase):
     def test_as_anonymous(self):
         anonuser = AnonymousUser()
         courses = self._make_api_call(anonuser, anonuser)
-        self.assertEqual(len(courses), 1)
-        self.assertEqual(courses[0], self.expected_course_data)
+        self.verify_courses(courses)
 
     def test_for_honor_user_as_anonymous(self):
         anonuser = AnonymousUser()
@@ -209,5 +195,4 @@ class TestGetCourseListExtras(CourseListTestMixin, ModuleStoreTestCase):
     def test_hidden_course_for_staff(self):
         self.create_course(visible_to_staff_only=True)
         courses = self._make_api_call(self.staff_user, self.staff_user)
-        self.assertEqual(len(courses), 1)
-        self.assertEqual(courses[0], self.expected_course_data)
+        self.verify_courses(courses)

lms/djangoapps/course_api/tests/test_serializers.py

+"""
+Test data created by CourseSerializer
+"""
+
+from datetime import datetime
+
+from rest_framework.test import APIRequestFactory
+from rest_framework.request import Request
+
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.course_module import DEFAULT_START_DATE
+
+from ..serializers import CourseSerializer
+from .mixins import CourseApiFactoryMixin
+
+
+class TestCourseSerializerFields(CourseApiFactoryMixin, ModuleStoreTestCase):
+    """
+    Test variations of start_date field responses
+    """
+
+    def setUp(self):
+        super(TestCourseSerializerFields, self).setUp()
+        self.staff_user = self.create_user('staff', is_staff=True)
+        self.honor_user = self.create_user('honor', is_staff=False)
+        self.request_factory = APIRequestFactory()
+
+    def _get_request(self, user=None):
+        """
+        Build a Request object for the specified user
+        """
+        if user is None:
+            user = self.honor_user
+        request = Request(self.request_factory.get('/'))
+        request.user = user
+        return request
+
+    def test_advertised_start(self):
+        course = self.create_course(
+            course=u'custom',
+            start=datetime(2015, 3, 15),
+            advertised_start=u'The Ides of March'
+        )
+        result = CourseSerializer(course, context={'request': self._get_request()}).data
+        self.assertEqual(result['course_id'], u'edX/custom/2012_Fall')
+        self.assertEqual(result['start_type'], u'string')
+        self.assertEqual(result['start_display'], u'The Ides of March')
+
+    def test_empty_start(self):
+        course = self.create_course(start=DEFAULT_START_DATE, course=u'custom')
+        result = CourseSerializer(course, context={'request': self._get_request()}).data
+        self.assertEqual(result['course_id'], u'edX/custom/2012_Fall')
+        self.assertEqual(result['start_type'], u'empty')
+        self.assertIsNone(result['start_display'])
+
+    def test_description(self):
+        course = self.create_course()
+        result = CourseSerializer(course, context={'request': self._get_request()}).data
+        self.assertEqual(result['description'], u'A course about toys.')
+
+    def test_blocks_url(self):
+        course = self.create_course()
+        result = CourseSerializer(course, context={'request': self._get_request()}).data
+        self.assertEqual(
+            result['blocks_url'],
+            u'http://testserver/api/courses/v1/blocks/?course_id=edX%2Ftoy%2F2012_Fall'
+        )

lms/djangoapps/course_api/tests/test_views.py

@@ -4,7 +4,6 @@ Tests for Blocks Views
 
 from django.core.urlresolvers import reverse
 from django.test import RequestFactory
-from rest_framework.exceptions import NotFound
 
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
 
@@ -121,7 +120,7 @@ class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase
         self.verify_response(params={'username': self.honor_user.username})
 
     def test_as_anonymous_user(self):
-        self.verify_response(expected_status_code=401)
+        self.verify_response(expected_status_code=200)
 
     def test_hidden_course_as_honor(self):
         self.setup_user(self.honor_user)
@@ -142,5 +141,5 @@ class CourseDetailViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase
         request = request_factory.get('/')
         request.query_params = {}
         request.user = self.staff_user
-        with self.assertRaises(NotFound):
-            CourseDetailView().get(request, 'a:b:c')
+        response = CourseDetailView().dispatch(request, course_key_string='a:b:c')
+        self.assertEqual(404, response.status_code)

lms/djangoapps/course_api/views.py

@@ -3,19 +3,18 @@ Course API Views
 """
 
 from rest_framework.exceptions import NotFound
-from rest_framework.views import APIView, Response
+from rest_framework.generics import ListAPIView, RetrieveAPIView
 
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 
-
-from openedx.core.lib.api.view_utils import view_auth_classes
+from openedx.core.lib.api.paginators import NamespacedPageNumberPagination
 
 from .api import course_detail, list_courses
+from .serializers import CourseSerializer
 
 
-@view_auth_classes()
-class CourseDetailView(APIView):
+class CourseDetailView(RetrieveAPIView):
     """
     **Use Cases**
 
@@ -89,21 +88,26 @@ class CourseDetailView(APIView):
             }
     """
 
-    def get(self, request, course_key_string):
+    serializer_class = CourseSerializer
+    lookup_url_kwarg = 'course_key_string'
+
+    def get_object(self):
         """
-        GET /api/courses/v1/courses/{course_key}/
+        Return the requested course object, if the user has appropriate
+        permissions.
         """
 
-        username = request.query_params.get('username', request.user.username)
+        username = self.request.query_params.get('username', self.request.user.username)
+        course_key_string = self.kwargs[self.lookup_url_kwarg]
         try:
             course_key = CourseKey.from_string(course_key_string)
         except InvalidKeyError:
             raise NotFound()
-        content = course_detail(request, username, course_key)
-        return Response(content)
+
+        return course_detail(self.request, username, course_key)
 
 
-class CourseListView(APIView):
+class CourseListView(ListAPIView):
     """
     **Use Cases**
 
@@ -158,11 +162,12 @@ class CourseListView(APIView):
             ]
     """
 
-    def get(self, request):
+    pagination_class = NamespacedPageNumberPagination
+    serializer_class = CourseSerializer
+
+    def get_queryset(self):
         """
-        GET /api/courses/v1/courses/
+        Return a list of courses visible to the user.
         """
-        username = request.query_params.get('username', request.user.username)
-
-        content = list_courses(request, username)
-        return Response(content)
+        username = self.request.query_params.get('username', self.request.user.username)
+        return list_courses(self.request, username)

openedx/core/lib/api/paginators.py

@@ -29,6 +29,38 @@ class DefaultPagination(pagination.PageNumberPagination):
         })
 
 
+class NamespacedPageNumberPagination(pagination.PageNumberPagination):
+    """
+    Pagination scheme that returns results with pagination metadata
+    embedded in a "pagination" attribute.  Can be used with data
+    that comes as a list of items, or as a dict with a "results"
+    attribute that contains a list of items.
+    """
+
+    page_size_query_param = "page_size"
+
+    def get_paginated_response(self, data):
+        """
+        Annotate the response with pagination information
+        """
+        metadata = {
+            'next': self.get_next_link(),
+            'previous': self.get_previous_link(),
+            'count': self.page.paginator.count,
+            'num_pages': self.page.paginator.num_pages,
+        }
+        if isinstance(data, dict):
+            if 'results' not in data:
+                raise TypeError(u'Malformed result dict')
+            data['pagination'] = metadata
+        else:
+            data = {
+                'results': data,
+                'pagination': metadata,
+            }
+        return Response(data)
+
+
 def paginate_search_results(object_class, search_results, page_size, page):
     """
     Takes edx-search results and returns a Page object populated

openedx/core/lib/api/tests/test_paginators.py

 """ Tests paginator methods """
+
+from collections import namedtuple
+
 import ddt
 from mock import Mock, MagicMock
 from unittest import TestCase
 from django.http import Http404
+from django.test import RequestFactory
+from rest_framework import serializers
 
-from openedx.core.lib.api.paginators import paginate_search_results
+from openedx.core.lib.api.paginators import NamespacedPageNumberPagination, paginate_search_results
 
 
 @ddt.ddt
@@ -118,6 +123,50 @@ class PaginateSearchResultsTestCase(TestCase):
             paginate_search_results(self.mock_model, self.search_results, self.default_size, page_num)
 
 
+class NamespacedPaginationTestCase(TestCase):
+    """
+    Test behavior of `NamespacedPageNumberPagination`
+    """
+
+    TestUser = namedtuple('TestUser', ['username', 'email'])
+
+    class TestUserSerializer(serializers.Serializer):  # pylint: disable=abstract-method
+        """
+        Simple serializer to paginate results from
+        """
+        username = serializers.CharField()
+        email = serializers.CharField()
+
+    expected_data = {
+        'results': [
+            {'username': 'user_5', 'email': 'user_5@example.com'},
+            {'username': 'user_6', 'email': 'user_6@example.com'},
+            {'username': 'user_7', 'email': 'user_7@example.com'},
+            {'username': 'user_8', 'email': 'user_8@example.com'},
+            {'username': 'user_9', 'email': 'user_9@example.com'},
+        ],
+        'pagination': {
+            'next': 'http://testserver/endpoint?page=3&page_size=5',
+            'previous': 'http://testserver/endpoint?page_size=5',
+            'count': 25,
+            'num_pages': 5,
+        }
+    }
+
+    def setUp(self):
+        super(NamespacedPaginationTestCase, self).setUp()
+        self.paginator = NamespacedPageNumberPagination()
+        self.users = [self.TestUser('user_{}'.format(idx), 'user_{}@example.com'.format(idx)) for idx in xrange(25)]
+        self.request_factory = RequestFactory()
+
+    def test_basic_pagination(self):
+        request = self.request_factory.get('/endpoint', data={'page': 2, 'page_size': 5})
+        request.query_params = {'page': 2, 'page_size': 5}
+        paged_users = self.paginator.paginate_queryset(self.users, request)
+        results = self.TestUserSerializer(paged_users, many=True).data
+        self.assertEqual(self.expected_data, self.paginator.get_paginated_response(results).data)
+
+
 def build_mock_object(obj_id):
     """ Build a mock object with the passed id"""
     mock_object = Mock()