From f53de2c04ab674eb4138ff6f7761c508dfb6d4de Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Fri, 20 Nov 2015 21:59:54 +0000 Subject: [PATCH] Paginate results of Courses API list endpoint * Catalog results are now paginated * Implements the new namespaced pagination described at https://openedx.atlassian.net/wiki/pages/viewpage.action?pageId=47481813 * API level code returns pythonic business objects * View layer performs serialize at the view layer * Convert views to use DRF generic views * Removes an unintentional authentication decorator that caused the detail endpoint to return a 401 for anonymous users MA-1724 --- lms/djangoapps/course_api/api.py | 9 +- lms/djangoapps/course_api/serializers.py | 5 +- lms/djangoapps/course_api/tests/test_api.py | 89 ++++++++----------- .../course_api/tests/test_serializers.py | 67 ++++++++++++++ lms/djangoapps/course_api/tests/test_views.py | 7 +- lms/djangoapps/course_api/views.py | 41 +++++---- openedx/core/lib/api/paginators.py | 32 +++++++ openedx/core/lib/api/tests/test_paginators.py | 51 ++++++++++- 8 files changed, 219 insertions(+), 82 deletions(-) create mode 100644 lms/djangoapps/course_api/tests/test_serializers.py diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index b6089b2f1b..8fd36b1a2d 100644 --- a/lms/djangoapps/course_api/api.py +++ b/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 diff --git a/lms/djangoapps/course_api/serializers.py b/lms/djangoapps/course_api/serializers.py index 1114b0f6bc..02349049bf 100644 --- a/lms/djangoapps/course_api/serializers.py +++ b/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) diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index 8d4b446e52..d5b0413367 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/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) diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py new file mode 100644 index 0000000000..936dc42b8a --- /dev/null +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -0,0 +1,67 @@ +""" +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' + ) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index ab85274423..55a54ff30e 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/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) diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index b730bd5cc5..2fcb700652 100644 --- a/lms/djangoapps/course_api/views.py +++ b/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): - """ - GET /api/courses/v1/courses/ - """ - username = request.query_params.get('username', request.user.username) + pagination_class = NamespacedPageNumberPagination + serializer_class = CourseSerializer - content = list_courses(request, username) - return Response(content) + def get_queryset(self): + """ + Return a list of courses visible to the user. + """ + username = self.request.query_params.get('username', self.request.user.username) + return list_courses(self.request, username) diff --git a/openedx/core/lib/api/paginators.py b/openedx/core/lib/api/paginators.py index 373845bc19..3fa9e0831c 100644 --- a/openedx/core/lib/api/paginators.py +++ b/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 diff --git a/openedx/core/lib/api/tests/test_paginators.py b/openedx/core/lib/api/tests/test_paginators.py index 3e18283737..c34a54e5cf 100644 --- a/openedx/core/lib/api/tests/test_paginators.py +++ b/openedx/core/lib/api/tests/test_paginators.py @@ -1,10 +1,15 @@ """ 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()