diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index df27af994d81ddd0437f0b595ec4417ff6595058..8963a27805b3ba15c57b65974d61fe3625fa13a1 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -3,7 +3,6 @@ Course API """ import logging -from edx_django_utils.monitoring import function_trace from edx_when.api import get_dates_for_course from django.conf import settings from django.contrib.auth.models import AnonymousUser, User @@ -18,7 +17,9 @@ from lms.djangoapps.courseware.courses import ( get_courses, get_permission_for_course_about ) +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.view_utils import LazySequence +from student.roles import GlobalStaff from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -138,6 +139,50 @@ def list_courses(request, username, org=None, filter_=None, search_term=None): return course_qs +def list_course_keys(request, username, role): + """ + Yield all available CourseKeys for the user having the given role. + + The courses returned include those for which the user identified by + `username` has the given role. Additionally, the logged in user + should have permission to view courses available to that user. + + Note: This function does not use branding to determine courses. + + Arguments: + request (HTTPRequest): + Used to identify the logged-in user and to instantiate the course + module to retrieve the course about description + username (string): + The name of the user the logged-in user would like to be + identified as + + Keyword Arguments: + role (string): + Course keys are filtered such that only those for which the + user has the specified role are returned. + + Return value: + Yield `CourseKey` objects representing the collection of courses. + + """ + user = get_effective_user(request.user, username) + + course_keys = CourseOverview.get_all_course_keys() + + # Global staff have access to all courses. Filter courses for non-global staff. + if GlobalStaff().has_user(user): + return course_keys + + return LazySequence( + ( + course_key for course_key in course_keys + if has_access(user, role, course_key) + ), + est_len=len(course_keys) + ) + + def get_due_dates(request, course_key, user): """ Get due date information for a user for blocks in a course. diff --git a/lms/djangoapps/course_api/forms.py b/lms/djangoapps/course_api/forms.py index cc6c3593fdb82f1e031531b90f58054c83bbd517..3340dba413559f83f91de1f64fc600467a9fe575 100644 --- a/lms/djangoapps/course_api/forms.py +++ b/lms/djangoapps/course_api/forms.py @@ -75,3 +75,11 @@ class CourseListGetForm(UsernameValidatorMixin, Form): cleaned_data['filter_'] = filter_ or None return cleaned_data + + +class CourseIdListGetForm(UsernameValidatorMixin, Form): + """ + A form to validate query parameters in the course list retrieval endpoint + """ + username = CharField(required=False) + role = CharField(required=True) diff --git a/lms/djangoapps/course_api/serializers.py b/lms/djangoapps/course_api/serializers.py index 120ef5a8700dd0786dc8d5152f335ba757dd90da..69fa5c33ffdf8c0fc5608b37614c81a781e3a90a 100644 --- a/lms/djangoapps/course_api/serializers.py +++ b/lms/djangoapps/course_api/serializers.py @@ -121,3 +121,11 @@ class CourseDetailSerializer(CourseSerializer): # pylint: disable=abstract-meth # fields from CourseSerializer, which get their data # from the CourseOverview object in SQL. return CourseDetails.fetch_about_attribute(course_overview.id, 'overview') + + +class CourseKeySerializer(serializers.BaseSerializer): # pylint:disable=abstract-method + """ + Serializer that takes a CourseKey and serializes it to a string course_id. + """ + def to_representation(self, instance): + return str(instance) diff --git a/lms/djangoapps/course_api/tests/test_forms.py b/lms/djangoapps/course_api/tests/test_forms.py index faaf90e0837391c1d2fdd2edd5496f4f541398f1..ed2c2e095115dfaa01c0363b0096dfa4e0670fc2 100644 --- a/lms/djangoapps/course_api/tests/test_forms.py +++ b/lms/djangoapps/course_api/tests/test_forms.py @@ -1,7 +1,7 @@ """ Tests for Course API forms. """ - +# pylint: disable=missing-docstring from itertools import product @@ -16,7 +16,7 @@ from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from ..forms import CourseDetailGetForm, CourseListGetForm +from ..forms import CourseDetailGetForm, CourseIdListGetForm, CourseListGetForm class UsernameTestMixin(object): @@ -101,6 +101,42 @@ class TestCourseListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreT self.assert_valid(self.cleaned_data) +class TestCourseIdListGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreTestCase): + FORM_CLASS = CourseIdListGetForm + + @classmethod + def setUpClass(cls): + super(TestCourseIdListGetForm, cls).setUpClass() + + cls.course = CourseFactory.create() + + def setUp(self): + super(TestCourseIdListGetForm, self).setUp() + + self.student = UserFactory.create() + self.set_up_data(self.student) + + def set_up_data(self, user): + """ + Sets up the initial form data and the expected clean data. + """ + self.initial = {'requesting_user': user} + self.form_data = QueryDict( + urlencode({ + 'username': user.username, + 'role': 'staff', + }), + mutable=True, + ) + self.cleaned_data = { + 'username': user.username, + 'role': 'staff', + } + + def test_basic(self): + self.assert_valid(self.cleaned_data) + + class TestCourseDetailGetForm(FormTestMixin, UsernameTestMixin, SharedModuleStoreTestCase): """ Tests for CourseDetailGetForm diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py index bf14af6183c2c676616786e922ef48d352d61a6a..587c42043d6f0025368e61c74257aa8c8ba9e265 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -4,11 +4,13 @@ Test data created by CourseSerializer and CourseDetailSerializer from datetime import datetime +from unittest import TestCase import ddt from rest_framework.request import Request from rest_framework.test import APIRequestFactory from xblock.core import XBlock +from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.models.course_details import CourseDetails @@ -16,7 +18,7 @@ from xmodule.course_module import DEFAULT_START_DATE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import check_mongo_calls -from ..serializers import CourseDetailSerializer, CourseSerializer +from ..serializers import CourseDetailSerializer, CourseKeySerializer, CourseSerializer from .mixins import CourseApiFactoryMixin @@ -155,3 +157,11 @@ class TestCourseDetailSerializer(TestCourseSerializer): about_descriptor = XBlock.load_class('about') overview_template = about_descriptor.get_template('overview.yaml') self.expected_data['overview'] = overview_template.get('data') + + +class TestCourseKeySerializer(TestCase): + + def test_course_key_serializer(self): + course_key = CourseLocator(org='org', course='course', run='2020_Q3') + serializer = CourseKeySerializer(course_key) + self.assertEqual(serializer.data, str(course_key)) diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 44b8b7d26b1331d57b6c89dd75b73820fbec2fd9..96df1654482c24e7e0514ea23ab73de2f0fbb8ed 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -5,6 +5,7 @@ Tests for Course API views. from datetime import datetime from hashlib import md5 +from unittest import TestCase import ddt import six @@ -21,12 +22,16 @@ from waffle.testutils import override_switch from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory +from openedx.core.lib.api.view_utils import LazySequence from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig +from student.auth import add_users +from student.roles import CourseInstructorRole, CourseStaffRole +from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from ..views import CourseDetailView, CourseListUserThrottle +from ..views import CourseDetailView, CourseListUserThrottle, LazyPageNumberPagination from .mixins import TEST_PASSWORD, CourseApiFactoryMixin @@ -350,13 +355,12 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear def test_list_all_with_search_term(self): """ - Test with search, should only the course that matches the search term. + Test with search, should list only the course that matches the search term. """ res = self.verify_response(params={'search_term': 'unique search term'}) self.assertIn('results', res.data) self.assertNotEqual(res.data['results'], []) - # Returns a count of 3 courses because that's the estimate before filtering - self.assertEqual(res.data['pagination']['count'], 3) + self.assertEqual(res.data['pagination']['count'], 1) self.assertEqual(len(res.data['results']), 1) # Should return a single course def test_too_many_courses(self): @@ -408,3 +412,118 @@ class CourseListSearchViewTest(CourseApiTestViewMixin, ModuleStoreTestCase, Sear [c['id'] for c in response.data['results']], ordered_course_ids[(page - 1) * 30:page * 30] ) + + +class CourseIdListViewTestCase(CourseApiTestViewMixin, ModuleStoreTestCase): + """ + Test responses returned from CourseIdListView (with tests that modify the courseware). + """ + ENABLED_SIGNALS = ['course_published'] + + def setUp(self): + super(CourseIdListViewTestCase, self).setUp() + self.course = self.create_course() + self.url = reverse('course-id-list') + self.staff_user = self.create_user(username='staff', is_staff=True) + self.honor_user = self.create_user(username='honor', is_staff=False) + self.global_admin = AdminFactory() + + def test_filter_by_roles_global_staff(self): + """ + Verify that global staff are always returned all courses irregardless of role filter. + """ + self.setup_user(self.staff_user) + + # Request the courses as the staff user with the different roles specified. + for role in ('staff', 'instructor'): + filtered_response = self.verify_response(params={'username': self.staff_user.username, 'role': role}) + self.assertEqual(len(filtered_response.data['results']), 1) + + def test_filter_by_roles_non_staff(self): + """ + Verify that a non-staff user can't access course_ids by role. + """ + self.setup_user(self.honor_user) + + # Request the courses as the non-staff user with the different roles should *not* be allowed. + for role in ('staff', 'instructor'): + filtered_response = self.verify_response(params={'username': self.honor_user.username, 'role': role}) + self.assertEqual(len(filtered_response.data['results']), 0) + + def test_filter_by_roles_course_staff(self): + """ + Verify that course_ids are filtered by the provided roles. + """ + # Make this user a course staff user for the course. + course_staff_user = self.create_user(username='course_staff', is_staff=False) + add_users(self.global_admin, CourseStaffRole(self.course.id), course_staff_user) + + # Create a second course, along with an instructor user for it. + alternate_course = self.create_course(org='test') + course_instructor_user = self.create_user(username='course_instructor', is_staff=False) + add_users(self.global_admin, CourseInstructorRole(alternate_course.id), course_instructor_user) + + # Requesting the courses for which the course staff user is staff should return *only* the single course. + self.setup_user(self.staff_user) + filtered_response = self.verify_response(params={ + 'username': course_staff_user.username, + 'role': 'staff' + }) + self.assertEqual(len(filtered_response.data['results']), 1) + self.assertTrue(filtered_response.data['results'][0].startswith(self.course.org)) + + # The course staff user does *not* have the course instructor role on any courses. + filtered_response = self.verify_response(params={ + 'username': course_staff_user.username, + 'role': 'instructor' + }) + self.assertEqual(len(filtered_response.data['results']), 0) + + # The course instructor user only has the course instructor role on one course. + filtered_response = self.verify_response(params={ + 'username': course_instructor_user.username, + 'role': 'instructor' + }) + self.assertEqual(len(filtered_response.data['results']), 1) + self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org)) + + # The course instructor user has the inferred course staff role on one course. + self.setup_user(course_instructor_user) + filtered_response = self.verify_response(params={ + 'username': course_instructor_user.username, + 'role': 'staff' + }) + self.assertEqual(len(filtered_response.data['results']), 1) + self.assertTrue(filtered_response.data['results'][0].startswith(alternate_course.org)) + + +class LazyPageNumberPaginationTestCase(TestCase): + + def test_lazy_page_number_pagination(self): + number_sequence = range(20) + even_numbers_lazy_sequence = LazySequence( + ( + number for number in number_sequence + if (number % 2) == 0 + ), + est_len=len(number_sequence) + ) + + expected_response = { + 'results': [10, 12, 14, 16, 18], + 'pagination': { + 'previous': 'http://testserver/endpoint?page_size=5', + 'num_pages': 2, + 'next': None, + 'count': 10} + } + + request = RequestFactory().get('/endpoint', data={'page': 2, 'page_size': 5}) + request.query_params = {'page': 2, 'page_size': 5} + + pagination = LazyPageNumberPagination() + pagination.max_page_size = 5 + pagination.page_size = 5 + paginated_queryset = pagination.paginate_queryset(even_numbers_lazy_sequence, request) + paginated_response = pagination.get_paginated_response(paginated_queryset) + self.assertDictEqual(expected_response, paginated_response.data) diff --git a/lms/djangoapps/course_api/urls.py b/lms/djangoapps/course_api/urls.py index 3c2894886a85dfa0811fa5024527f8908208680b..f155222156827b1bb3057e198271c86a89a39a7d 100644 --- a/lms/djangoapps/course_api/urls.py +++ b/lms/djangoapps/course_api/urls.py @@ -6,10 +6,11 @@ Course API URLs from django.conf import settings from django.conf.urls import include, url -from .views import CourseDetailView, CourseListView +from .views import CourseDetailView, CourseIdListView, CourseListView urlpatterns = [ url(r'^v1/courses/$', CourseListView.as_view(), name="course-list"), url(r'^v1/courses/{}'.format(settings.COURSE_KEY_PATTERN), CourseDetailView.as_view(), name="course-detail"), + url(r'^v1/course_ids/$', CourseIdListView.as_view(), name="course-id-list"), url(r'', include('course_api.blocks.urls')) ] diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index efed2458b3baf90c715d9a43d2f0601da98dc4dd..5c507dc2cadfe2c440fd6898b9a30fb8fd69d072 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -4,8 +4,6 @@ Course API Views from django.core.exceptions import ValidationError -from edx_django_utils.monitoring import set_custom_metric - from edx_rest_framework_extensions.paginators import NamespacedPageNumberPagination from rest_framework.generics import ListAPIView, RetrieveAPIView from rest_framework.throttling import UserRateThrottle @@ -13,9 +11,9 @@ from rest_framework.throttling import UserRateThrottle from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from . import USE_RATE_LIMIT_2_FOR_COURSE_LIST_API, USE_RATE_LIMIT_10_FOR_COURSE_LIST_API -from .api import course_detail, list_courses -from .forms import CourseDetailGetForm, CourseListGetForm -from .serializers import CourseDetailSerializer, CourseSerializer +from .api import course_detail, list_course_keys, list_courses +from .forms import CourseDetailGetForm, CourseIdListGetForm, CourseListGetForm +from .serializers import CourseDetailSerializer, CourseKeySerializer, CourseSerializer @view_auth_classes(is_authenticated=False) @@ -161,6 +159,25 @@ class CourseListUserThrottle(UserRateThrottle): return super(CourseListUserThrottle, self).allow_request(request, view) +class LazyPageNumberPagination(NamespacedPageNumberPagination): + """ + NamespacedPageNumberPagination that works with a LazySequence queryset. + + The paginator cache uses ``@cached_property`` to cache the property values for + count and num_pages. It assumes these won't change, but in the case of a + LazySquence, its count gets updated as we move through it. This class clears + the cached property values before reporting results so they will be recalculated. + + """ + + def get_paginated_response(self, data): + # Clear the cached property values to recalculate the estimated count from the LazySequence + del self.page.paginator.__dict__['count'] + del self.page.paginator.__dict__['num_pages'] + + return super(LazyPageNumberPagination, self).get_paginated_response(data) + + @view_auth_classes(is_authenticated=False) class CourseListView(DeveloperErrorViewMixin, ListAPIView): """ @@ -177,6 +194,7 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): Body comprises a list of objects as returned by `CourseDetailView`. **Parameters** + search_term (optional): Search term to filter courses (used by ElasticSearch). @@ -227,9 +245,10 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): } ] """ + class CourseListPageNumberPagination(LazyPageNumberPagination): + max_page_size = 100 - pagination_class = NamespacedPageNumberPagination - pagination_class.max_page_size = 100 + pagination_class = CourseListPageNumberPagination serializer_class = CourseSerializer throttle_classes = (CourseListUserThrottle,) @@ -248,3 +267,98 @@ class CourseListView(DeveloperErrorViewMixin, ListAPIView): filter_=form.cleaned_data['filter_'], search_term=form.cleaned_data['search_term'] ) + + +class CourseIdListUserThrottle(UserRateThrottle): + """Limit the number of requests users can make to the course list id API.""" + + THROTTLE_RATES = { + 'user': '20/minute', + 'staff': '40/minute', + } + + def allow_request(self, request, view): + # Use a special scope for staff to allow for a separate throttle rate + user = request.user + if user.is_authenticated and (user.is_staff or user.is_superuser): + self.scope = 'staff' + self.rate = self.get_rate() + self.num_requests, self.duration = self.parse_rate(self.rate) + + return super(CourseIdListUserThrottle, self).allow_request(request, view) + + +@view_auth_classes() +class CourseIdListView(DeveloperErrorViewMixin, ListAPIView): + """ + **Use Cases** + + Request a list of course IDs for all courses the specified user can + access based on the provided parameters. + + **Example Requests** + + GET /api/courses/v1/courses_ids/ + + **Response Values** + + Body comprises a list of course ids and pagination details. + + **Parameters** + + username (optional): + The username of the specified user whose visible courses we + want to see. + + role (required): + Course ids are filtered such that only those for which the + user has the specified role are returned. Role can be "staff" + or "instructor". + Case-insensitive. + + **Returns** + + * 200 on success, with a list of course ids and pagination details + * 400 if an invalid parameter was sent or the username was not provided + for an authenticated request. + * 403 if a user who does not have permission to masquerade as + another user who specifies a username other than their own. + * 404 if the specified user does not exist, or the requesting user does + not have permission to view their courses. + + Example response: + + { + "results": + [ + "course-v1:edX+DemoX+Demo_Course" + ], + "pagination": { + "previous": null, + "num_pages": 1, + "next": null, + "count": 1 + } + } + + """ + class CourseIdListPageNumberPagination(LazyPageNumberPagination): + max_page_size = 1000 + + pagination_class = CourseIdListPageNumberPagination + serializer_class = CourseKeySerializer + throttle_classes = (CourseIdListUserThrottle,) + + def get_queryset(self): + """ + Returns CourseKeys for courses which the user has the provided role. + """ + form = CourseIdListGetForm(self.request.query_params, initial={'requesting_user': self.request.user}) + if not form.is_valid(): + raise ValidationError(form.errors) + + return list_course_keys( + self.request, + form.cleaned_data['username'], + role=form.cleaned_data['role'], + )