From 17c0d6d59f67025f70c93ddcb0fbcc9bbe41fe09 Mon Sep 17 00:00:00 2001 From: Paulo Viadanna Date: Wed, 19 Sep 2018 11:33:46 -0300 Subject: [PATCH] Add an alternative cohort management API This is primarily intended to be used for server to server communication. The endpoints support OAuth2 authentication. --- lms/djangoapps/instructor/views/api.py | 92 +++- lms/urls.py | 3 + openedx/core/djangoapps/course_groups/api.py | 27 ++ .../djangoapps/course_groups/serializers.py | 21 + .../course_groups/tests/test_api_views.py | 457 ++++++++++++++++++ openedx/core/djangoapps/course_groups/urls.py | 40 ++ .../core/djangoapps/course_groups/views.py | 409 +++++++++++++++- 7 files changed, 1015 insertions(+), 34 deletions(-) create mode 100644 openedx/core/djangoapps/course_groups/api.py create mode 100644 openedx/core/djangoapps/course_groups/serializers.py create mode 100644 openedx/core/djangoapps/course_groups/tests/test_api_views.py create mode 100644 openedx/core/djangoapps/course_groups/urls.py diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index b2c50c4036..d2ef3385b1 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -30,13 +30,18 @@ from django.core.validators import validate_email from django.db import IntegrityError, transaction from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotFound from django.shortcuts import redirect +from django.utils.decorators import method_decorator from django.utils.html import strip_tags from django.utils.translation import ugettext as _ from django.views.decorators.cache import cache_control from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_http_methods, require_POST +from edx_rest_framework_extensions.authentication import JwtAuthentication, SessionAuthenticationAllowInactiveUser from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from rest_framework import permissions, status +from rest_framework.response import Response +from rest_framework.views import APIView from six import text_type import instructor_analytics.basic @@ -85,6 +90,8 @@ from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api.preferences.api import get_user_preference, set_user_preference from openedx.core.djangolib.markup import HTML, Text +from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from shoppingcart.models import ( Coupon, CourseMode, @@ -346,7 +353,8 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man else: general_errors.append({ 'username': '', 'email': '', - 'response': _('Make sure that the file you upload is in CSV format with no extraneous characters or rows.') + 'response': _( + 'Make sure that the file you upload is in CSV format with no extraneous characters or rows.') }) except Exception: # pylint: disable=broad-except @@ -1341,6 +1349,25 @@ def get_students_who_may_enroll(request, course_id): return JsonResponse({"status": success_status}) +def _cohorts_csv_validator(file_storage, file_to_validate): + """ + Verifies that the expected columns are present in the CSV used to add users to cohorts. + """ + with file_storage.open(file_to_validate) as f: + reader = unicodecsv.reader(UniversalNewlineIterator(f), encoding='utf-8') + try: + fieldnames = next(reader) + except StopIteration: + fieldnames = [] + msg = None + if "cohort" not in fieldnames: + msg = _("The file must contain a 'cohort' column containing cohort names.") + elif "email" not in fieldnames and "username" not in fieldnames: + msg = _("The file must contain a 'username' column, an 'email' column, or both.") + if msg: + raise FileValidationException(msg) + + @transaction.non_atomic_requests @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -1356,29 +1383,11 @@ def add_users_to_cohorts(request, course_id): course_key = CourseKey.from_string(course_id) try: - def validator(file_storage, file_to_validate): - """ - Verifies that the expected columns are present. - """ - with file_storage.open(file_to_validate) as f: - reader = unicodecsv.reader(UniversalNewlineIterator(f), encoding='utf-8') - try: - fieldnames = next(reader) - except StopIteration: - fieldnames = [] - msg = None - if "cohort" not in fieldnames: - msg = _("The file must contain a 'cohort' column containing cohort names.") - elif "email" not in fieldnames and "username" not in fieldnames: - msg = _("The file must contain a 'username' column, an 'email' column, or both.") - if msg: - raise FileValidationException(msg) - __, filename = store_uploaded_file( request, 'uploaded-file', ['.csv'], course_and_time_based_filename_generator(course_key, "cohorts"), max_file_size=2000000, # limit to 2 MB - validator=validator + validator=_cohorts_csv_validator ) # The task will assume the default file storage. lms.djangoapps.instructor_task.api.submit_cohort_students(request, course_key, filename) @@ -1388,6 +1397,49 @@ def add_users_to_cohorts(request, course_id): return JsonResponse() +# The non-atomic decorator is required because this view calls a celery +# task which uses the 'outer_atomic' context manager. +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class CohortCSV(DeveloperErrorViewMixin, APIView): + """ + **Use Cases** + + Submit a CSV file to assign users to cohorts + + **Example Requests**: + + POST /api/cohorts/v1/courses/{course_id}/users/ + + **Response Values** + * Empty as this is executed asynchronously. + """ + authentication_classes = ( + JwtAuthentication, + OAuth2AuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (permissions.IsAuthenticated, permissions.IsAdminUser) + + def post(self, request, course_key_string): + """ + View method that accepts an uploaded file (using key "uploaded-file") + containing cohort assignments for users. This method spawns a celery task + to do the assignments, and a CSV file with results is provided via data downloads. + """ + course_key = CourseKey.from_string(course_key_string) + try: + __, file_name = store_uploaded_file( + request, 'uploaded-file', ['.csv'], + course_and_time_based_filename_generator(course_key, 'cohorts'), + max_file_size=2000000, # limit to 2 MB + validator=_cohorts_csv_validator + ) + lms.djangoapps.instructor_task.api.submit_cohort_students(request, course_key, file_name) + except (FileValidationException, ValueError) as e: + raise self.api_error(status.HTTP_400_BAD_REQUEST, str(e), 'failed-validation') + return Response(status=status.HTTP_204_NO_CONTENT) + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') diff --git a/lms/urls.py b/lms/urls.py index 4653abe183..eb68fd63b0 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -514,6 +514,9 @@ urlpatterns += [ name='course_discussions_settings', ), + # Cohorts management API + url(r'^api/cohorts/', include('openedx.core.djangoapps.course_groups.urls', namespace='api_cohorts')), + # Cohorts management url( r'^courses/{}/cohorts/settings$'.format( diff --git a/openedx/core/djangoapps/course_groups/api.py b/openedx/core/djangoapps/course_groups/api.py new file mode 100644 index 0000000000..c742b4ce30 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/api.py @@ -0,0 +1,27 @@ +""" +course_groups API +""" +from django.contrib.auth.models import User + +from openedx.core.djangoapps.course_groups.models import CohortMembership + + +def remove_user_from_cohort(course_key, username, cohort_id=None): + """ + Removes an user from a course group. + """ + if username is None: + raise ValueError('Need a valid username') + user = User.objects.get(username=username) + if cohort_id is not None: + membership = CohortMembership.objects.get( + user=user, course_id=course_key, course_user_group__id=cohort_id + ) + membership.delete() + else: + try: + membership = CohortMembership.objects.get(user=user, course_id=course_key) + except CohortMembership.DoesNotExist: + pass + else: + membership.delete() diff --git a/openedx/core/djangoapps/course_groups/serializers.py b/openedx/core/djangoapps/course_groups/serializers.py new file mode 100644 index 0000000000..3867a776c5 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/serializers.py @@ -0,0 +1,21 @@ +""" +Cohorts API serializers. +""" +from rest_framework import serializers + +from django.contrib.auth.models import User + + +class CohortUsersAPISerializer(serializers.ModelSerializer): + """ + Serializer for cohort users. + """ + name = serializers.SerializerMethodField('get_full_name') + + def get_full_name(self, model): + """Return the full name of the user.""" + return '{} {}'.format(model.first_name, model.last_name) + + class Meta: + model = User + fields = ('username', 'email', 'name') diff --git a/openedx/core/djangoapps/course_groups/tests/test_api_views.py b/openedx/core/djangoapps/course_groups/tests/test_api_views.py new file mode 100644 index 0000000000..1e8bf27a9b --- /dev/null +++ b/openedx/core/djangoapps/course_groups/tests/test_api_views.py @@ -0,0 +1,457 @@ +""" +Tests for Cohort API +""" +import json +import tempfile + +import ddt +from django.urls import reverse +from edx_oauth2_provider.tests.factories import AccessTokenFactory, ClientFactory +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import ToyCourseFactory + +from openedx.core.djangolib.testing.utils import skip_unless_lms +from .helpers import CohortFactory +from .. import cohorts + +USERNAME = 'honor' +USER_MAIL = 'honor@example.com' +SETTINGS_PAYLOAD = '{"is_cohorted": true}' +HANDLER_POST_PAYLOAD = '{"name":"Default","user_count":0,"assignment_type":"random","user_partition_id":null\ +,"group_id":null}' +HANDLER_PATCH_PAYLOAD = '{"name":"Default Group","group_id":null,"user_partition_id":null,"assignment_type":"random"}' +ADD_USER_PAYLOAD = json.dumps({'users': [USER_MAIL, ]}) +CSV_DATA = '''email,cohort\n{},DEFAULT'''.format(USER_MAIL) + + +@skip_unless_lms +@ddt.ddt +class TestCohortOauth(SharedModuleStoreTestCase): + """ + Tests for cohort API oauth authentication + """ + + password = 'password' + + @classmethod + def setUpClass(cls): + super(TestCohortOauth, cls).setUpClass() + cls.user = UserFactory(username=USERNAME, email=USER_MAIL, password=cls.password) + cls.staff_user = UserFactory(is_staff=True, password=cls.password) + cls.course_key = ToyCourseFactory.create().id + cls.course_str = unicode(cls.course_key) + + @ddt.data({'path_name': 'api_cohorts:cohort_settings'}, + {'path_name': 'api_cohorts:cohort_handler'}, ) + @ddt.unpack + def test_oauth_list(self, path_name): + """ Verify the endpoints supports OAuth, and only allows authorization for staff users. """ + path = reverse(path_name, kwargs={'course_key_string': self.course_str}) + user = UserFactory(is_staff=False) + oauth_client = ClientFactory.create() + access_token = AccessTokenFactory.create(user=user, client=oauth_client).token + headers = { + 'HTTP_AUTHORIZATION': 'Bearer ' + access_token + } + + # Non-staff users should not have access to the API + response = self.client.get(path=path, **headers) + self.assertEqual(response.status_code, 403) + + # Staff users should have access to the API + user.is_staff = True + user.save() + response = self.client.get(path=path, **headers) + self.assertEqual(response.status_code, 200) + + def test_oauth_users(self): + """ Verify the endpoint supports OAuth, and only allows authorization for staff users. """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + path = reverse('api_cohorts:cohort_users', kwargs={'course_key_string': self.course_str, 'cohort_id': 1}) + user = UserFactory(is_staff=False) + oauth_client = ClientFactory.create() + access_token = AccessTokenFactory.create(user=user, client=oauth_client).token + headers = { + 'HTTP_AUTHORIZATION': 'Bearer ' + access_token + } + data = { + 'users': [user.username] + } + + # Non-staff users should not have access to the API + response = self.client.post(path=path, data=data, **headers) + self.assertEqual(response.status_code, 403) + + # Staff users should have access to the API + user.is_staff = True + user.save() + response = self.client.post(path=path, data=data, **headers) + self.assertEqual(response.status_code, 200) + + def test_oauth_csv(self): + """ Verify the endpoint supports OAuth, and only allows authorization for staff users. """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + path = reverse('api_cohorts:cohort_users_csv', kwargs={'course_key_string': self.course_str}) + user = UserFactory(is_staff=False) + oauth_client = ClientFactory.create() + access_token = AccessTokenFactory.create(user=user, client=oauth_client).token + headers = { + 'HTTP_AUTHORIZATION': 'Bearer ' + access_token + } + + # Non-staff users should not have access to the API + response = self.client.post(path=path, **headers) + self.assertEqual(response.status_code, 403) + + # Staff users should have access to the API + user.is_staff = True + user.save() + response = self.client.post(path=path, **headers) + self.assertEqual(response.status_code, 400) + + +@skip_unless_lms +@ddt.ddt +class TestCohortApi(SharedModuleStoreTestCase): + """ + Tests for cohort API endpoints + """ + + password = 'password' + + @classmethod + def setUpClass(cls): + super(TestCohortApi, cls).setUpClass() + cls.user = UserFactory(username=USERNAME, email=USER_MAIL, password=cls.password) + cls.staff_user = UserFactory(is_staff=True, password=cls.password) + cls.course_key = ToyCourseFactory.create().id + cls.course_str = unicode(cls.course_key) + + @ddt.data( + {'is_staff': True, 'status': 200}, + {'is_staff': False, 'status': 403}, + ) + @ddt.unpack + def test_cohort_settings_staff_access_required(self, is_staff, status): + """ + Test that staff access is required for the endpoints. + """ + path = reverse('api_cohorts:cohort_settings', kwargs={'course_key_string': self.course_str}) + user = self.staff_user if is_staff else self.user + self.client.login(username=user.username, password=self.password) + + response = self.client.get(path=path) + assert response.status_code == status + + response = self.client.put(path=path, data=SETTINGS_PAYLOAD, content_type='application/json') + assert response.status_code == status + + def test_cohort_settings_non_existent_course(self): + """ + Test getting and updating the cohort settings of a non-existent course. + """ + path = reverse('api_cohorts:cohort_settings', kwargs={'course_key_string': 'e/d/X'}) + self.client.login(username=self.staff_user.username, password=self.password) + + response = self.client.get(path=path) + assert response.status_code == 404 + + response = self.client.put(path=path, data=SETTINGS_PAYLOAD, content_type='application/json') + assert response.status_code == 404 + + @ddt.data( + {'data': '', 'status': 400}, + {'data': 'abcd', 'status': 400}, + {'data': {'is_course_cohorted': 'abcd'}, 'status': 400}, + ) + @ddt.unpack + def test_put_cohort_settings_invalid_request(self, data, status): + """ + Test the endpoint with invalid requests + """ + path = reverse('api_cohorts:cohort_settings', kwargs={'course_key_string': self.course_str}) + self.client.login(username=self.staff_user.username, password=self.password) + + response = self.client.put(path=path, data=data, content_type='application/json') + assert response.status_code == status + + @ddt.data( + {'data': '', 'kwargs': {'cohort_id': 1}, 'status': 405}, + {'data': '{"a": 1}', 'kwargs': {}, 'status': 400}, + {'data': '{"name": "c1"}', 'kwargs': {}, 'status': 400}, + {'data': '{"assignment_type": "manual"}', 'kwargs': {}, 'status': 400}, + ) + @ddt.unpack + def test_post_cohort_handler_invalid_requests(self, data, kwargs, status): + """ + Test the endpoint with invalid requests. + """ + url_kwargs = {'course_key_string': self.course_str} + if kwargs: + url_kwargs.update(kwargs) + + path = reverse('api_cohorts:cohort_handler', kwargs=url_kwargs) + user = self.staff_user + assert self.client.login(username=user.username, password=self.password) + + response = self.client.post(path=path, data=data, content_type='application/json') + assert response.status_code == status + + @ddt.data({'is_staff': False, 'payload': HANDLER_POST_PAYLOAD, 'status': 403}, + {'is_staff': True, 'payload': HANDLER_POST_PAYLOAD, 'status': 200}, + {'is_staff': False, 'payload': '', 'status': 403}, + {'is_staff': True, 'payload': '', 'status': 200}, ) + @ddt.unpack + def test_cohort_handler(self, is_staff, payload, status): + """ + Test GET and POST methods of cohort handler endpoint + """ + path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str}) + user = self.staff_user if is_staff else self.user + assert self.client.login(username=user.username, password=self.password) + if payload: + response = self.client.post( + path=path, + data=payload, + content_type='application/json') + else: + response = self.client.get(path=path) + assert response.status_code == status + + def test_cohort_handler_patch_without_cohort_id(self): + path = reverse('api_cohorts:cohort_handler', kwargs={'course_key_string': self.course_str}) + self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.patch(path=path, data=HANDLER_PATCH_PAYLOAD, content_type='application/json') + assert response.status_code == 405 + + @ddt.data( + {'payload': '', 'status': 400}, + {'payload': '{"name": "C2"}', 'status': 400}, + {'payload': '{"assignment_type": "automatic"}', 'status': 400}, + ) + @ddt.unpack + def test_cohort_handler_patch_invalid_request(self, payload, status): + """ + Test the endpoint with invalid requests. + """ + cohorts.add_cohort(self.course_key, "C1", "random") + cohorts.add_cohort(self.course_key, "C2", "automatic") + path = reverse( + 'api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': 1} + ) + self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.patch(path=path, data=payload, content_type='application/json') + assert response.status_code == status + + @ddt.data({'is_staff': False, 'payload': HANDLER_PATCH_PAYLOAD, 'status': 403}, + {'is_staff': True, 'payload': HANDLER_PATCH_PAYLOAD, 'status': 204}, + {'is_staff': False, 'payload': '', 'status': 403}, + {'is_staff': True, 'payload': '', 'status': 200}, ) + @ddt.unpack + def test_cohort_handler_patch(self, is_staff, payload, status): + """ + Test GET and PATCH methods of cohort handler endpoint for a specific cohort + """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + cohort_id = 1 + path = reverse('api_cohorts:cohort_handler', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort_id}) + user = self.staff_user if is_staff else self.user + assert self.client.login(username=user.username, password=self.password) + if payload: + response = self.client.patch( + path=path, + data=payload, + content_type='application/json') + else: + response = self.client.get(path=path) + assert response.status_code == status + + def test_list_users_in_cohort_non_existent_cohort(self): + """ + Test listing the users in a non-existent cohort. + """ + path = reverse( + 'api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': 99} + ) + assert self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.get(path=path) + assert response.status_code == 404 + + @ddt.data( + {'is_staff': False, 'status': 403}, + {'is_staff': True, 'status': 200}, + ) + @ddt.unpack + def test_list_users_in_cohort(self, is_staff, status): + """ + Test GET method for listing users in a cohort. + """ + users = [UserFactory() for _ in range(5)] + cohort = CohortFactory(course_id=self.course_key, users=users) + path = reverse( + 'api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id} + ) + self.user = self.staff_user if is_staff else self.user + assert self.client.login(username=self.user.username, password=self.password) + response = self.client.get( + path=path + ) + assert response.status_code == status + + if status == 200: + results = json.loads(response.content)['results'] + expected_results = [{ + 'username': user.username, + 'email': user.email, + 'name': '{} {}'.format(user.first_name, user.last_name) + } for user in users] + assert results == expected_results + + def test_add_users_to_cohort_non_existent_cohort(self): + """ + Test adding users to a non-existent cohort. + """ + path = reverse( + 'api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': 99} + ) + assert self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.post( + path=path, + data=ADD_USER_PAYLOAD, + content_type='application/json' + ) + assert response.status_code == 404 + + def test_add_users_to_cohort_username_in_url(self): + """ + Test adding a user to cohort by passing the username in URL. + """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + path = reverse( + 'api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': 1, 'username': self.staff_user.username} + ) + assert self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.post(path=path, data='', content_type='application/json') + assert response.status_code == 200 + + def test_add_users_to_cohort_missing_users(self): + """ + Test adding users to cohort without providing the users. + """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + + path = reverse('api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': 1}) + assert self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.post(path=path, data='', content_type='application/json') + assert response.status_code == 400 + + @ddt.data({'is_staff': False, 'payload': ADD_USER_PAYLOAD, 'status': 403}, + {'is_staff': True, 'payload': ADD_USER_PAYLOAD, 'status': 200}, ) + @ddt.unpack + def test_add_users_to_cohort(self, is_staff, payload, status): + """ + Test POST method for adding users to a cohort + """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + cohort_id = 1 + path = reverse('api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort_id}) + user = self.staff_user if is_staff else self.user + assert self.client.login(username=user.username, password=self.password) + response = self.client.post( + path=path, + data=payload, + content_type='application/json') + assert response.status_code == status + + def test_add_users_to_cohort_different_types_of_users(self): + """ + Test adding users of different types - invalid, existing, preassigned, unassigned, to a cohort. + """ + cohort = cohorts.add_cohort(self.course_key, "DEFAULT", "random") + cohort2 = cohorts.add_cohort(self.course_key, "C2", "random") + user1 = UserFactory(username='user1') + user2 = UserFactory(username='user2') + user3 = UserFactory(username='user3') + cohorts.add_user_to_cohort(cohort2, user1) + cohorts.add_user_to_cohort(cohort, user2) + path = reverse('api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort.id}) + assert self.client.login(username=self.staff_user.username, password=self.password) + data = '{"users": ["foo@example.com", "user1", "", "user4", "user3", "user2", "foo@bar"]}' + response = self.client.post( + path=path, + data=data, + content_type='application/json' + ) + assert response.status_code == 200 + + expected_response = { + "preassigned": ["foo@example.com"], + "added": [{"username": "user3", "email": user3.email}], + "success": True, + "unknown": ["user4"], + "changed": [{"username": "user1", "email": user1.email, "previous_cohort": "C2"}], + "invalid": ["foo@bar"], + "present": ["user2"] + } + assert json.loads(response.content) == expected_response + + def test_remove_user_from_cohort_missing_username(self): + """ + Test removing a user from cohort without providing the username. + """ + path = reverse('api_cohorts:cohort_users', kwargs={'course_key_string': self.course_str, 'cohort_id': 1}) + assert self.client.login(username=self.staff_user.username, password=self.password) + response = self.client.delete(path) + assert response.status_code == 405 + + @ddt.data({'is_staff': False, 'username': USERNAME, 'status': 403}, + {'is_staff': True, 'username': USERNAME, 'status': 204}, + {'is_staff': True, 'username': 'doesnotexist', 'status': 404}, + {'is_staff': False, 'username': None, 'status': 403}, + {'is_staff': True, 'username': None, 'status': 404}, ) + @ddt.unpack + def test_remove_user_from_cohort(self, is_staff, username, status): + """ + Test DELETE method for removing an user from a cohort. + """ + cohort = cohorts.add_cohort(self.course_key, "DEFAULT", "random") + cohorts.add_user_to_cohort(cohort, USERNAME) + cohort_id = 1 + path = reverse('api_cohorts:cohort_users', + kwargs={'course_key_string': self.course_str, 'cohort_id': cohort_id, 'username': username}) + user = self.staff_user if is_staff else self.user + assert self.client.login(username=user.username, password=self.password) + response = self.client.delete(path=path) + assert response.status_code == status + + @ddt.data({'is_staff': False, 'payload': CSV_DATA, 'status': 403}, + {'is_staff': True, 'payload': CSV_DATA, 'status': 204}, + {'is_staff': True, 'payload': '', 'status': 400}, + {'is_staff': False, 'payload': '', 'status': 403}, ) + @ddt.unpack + def test_add_users_csv(self, is_staff, payload, status): + """ + Test adding users to cohorts using a CSV file + """ + cohorts.add_cohort(self.course_key, "DEFAULT", "random") + # this temporary file will be removed in `self.tearDown()` + __, file_name = tempfile.mkstemp(suffix='.csv', dir=tempfile.mkdtemp()) + with open(file_name, 'w') as file_pointer: + file_pointer.write(payload.encode('utf-8')) + path = reverse('api_cohorts:cohort_users_csv', kwargs={'course_key_string': self.course_str}) + user = self.staff_user if is_staff else self.user + assert self.client.login(username=user.username, password=self.password) + with open(file_name, 'r') as file_pointer: + response = self.client.post(path=path, + data={'uploaded-file': file_pointer}) + assert response.status_code == status diff --git a/openedx/core/djangoapps/course_groups/urls.py b/openedx/core/djangoapps/course_groups/urls.py new file mode 100644 index 0000000000..32ce124e60 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/urls.py @@ -0,0 +1,40 @@ +""" +Cohort API URLs +""" + +from django.conf import settings +from django.conf.urls import url + +import openedx.core.djangoapps.course_groups.views +import lms.djangoapps.instructor.views.api + +urlpatterns = [ + url( + r'^v1/settings/{}$'.format( + settings.COURSE_KEY_PATTERN, + ), + openedx.core.djangoapps.course_groups.views.CohortSettings.as_view(), + name='cohort_settings', + ), + url( + r'^v1/courses/{}/cohorts/(?P[0-9]+)?$'.format( + settings.COURSE_KEY_PATTERN, + ), + openedx.core.djangoapps.course_groups.views.CohortHandler.as_view(), + name='cohort_handler', + ), + url( + r'^v1/courses/{}/cohorts/(?P[0-9]+)/users/(?P.+)?$'.format( + settings.COURSE_KEY_PATTERN, + ), + openedx.core.djangoapps.course_groups.views.CohortUsers.as_view(), + name='cohort_users', + ), + url( + r'^v1/courses/{}/users?$'.format( + settings.COURSE_KEY_PATTERN, + ), + lms.djangoapps.instructor.views.api.CohortCSV.as_view(), + name='cohort_users_csv', + ), +] diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index caf8ebf37a..45318d57f7 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -14,15 +14,26 @@ from django.http import Http404, HttpResponseBadRequest from django.utils.translation import ugettext from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_http_methods, require_POST +from edx_rest_framework_extensions.authentication import JwtAuthentication, SessionAuthenticationAllowInactiveUser +from edx_rest_framework_extensions.paginators import NamespacedPageNumberPagination from opaque_keys.edx.keys import CourseKey +from rest_framework import status, permissions +from rest_framework.generics import GenericAPIView +from rest_framework.response import Response from six import text_type from courseware.courses import get_course_with_access from edxmako.shortcuts import render_to_response from util.json_request import JsonResponse, expect_json -from . import cohorts -from .models import CohortMembership, CourseUserGroup, CourseUserGroupPartitionGroup +from openedx.core.djangoapps.course_groups.models import CohortMembership +from openedx.core.lib.api.authentication import OAuth2AuthenticationAllowInactiveUser +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin +from . import api, cohorts +from .models import CourseUserGroup, CourseUserGroupPartitionGroup +from .serializers import CohortUsersAPISerializer + +MAX_PAGE_SIZE = 100 log = logging.getLogger(__name__) @@ -71,6 +82,16 @@ def _get_course_cohort_settings_representation(cohort_id, is_cohorted): } +def _cohort_settings(course_key): + """ + Fetch a course current cohort settings. + """ + return _get_course_cohort_settings_representation( + cohorts.get_course_cohort_id(course_key), + cohorts.is_course_cohorted(course_key) + ) + + def _get_cohort_representation(cohort, course): """ Returns a JSON representation of a cohort. @@ -345,22 +366,13 @@ def remove_user_from_cohort(request, course_key_string, cohort_id): username = request.POST.get('username') if username is None: - return json_http_response({'success': False, - 'msg': 'No username specified'}) + return json_http_response({'success': False, 'msg': 'No username specified'}) try: - user = User.objects.get(username=username) + api.remove_user_from_cohort(course_key, username) except User.DoesNotExist: log.debug('no user') - return json_http_response({'success': False, - 'msg': "No user '{0}'".format(username)}) - - try: - membership = CohortMembership.objects.get(user=user, course_id=course_key) - membership.delete() - - except CohortMembership.DoesNotExist: - pass + return json_http_response({'success': False, 'msg': "No user '{0}'".format(username)}) return json_http_response({'success': True}) @@ -379,3 +391,372 @@ def debug_cohort_mgmt(request, course_key_string): kwargs={'course_key': text_type(course_key)} )} return render_to_response('/course_groups/debug.html', context) + + +def _get_course_with_access(request, course_key_string, action='staff'): + """ + Fetching a course with expected permission level + """ + course_key = CourseKey.from_string(course_key_string) + return course_key, get_course_with_access(request.user, action, course_key) + + +def _get_cohort_response(cohort, course): + """ + Helper method that returns APIView Response of a cohort representation + """ + return Response(_get_cohort_representation(cohort, course), status=status.HTTP_200_OK) + + +def _get_cohort_settings_response(course_key): + """ + Helper method to return a serialized response for the cohort settings. + """ + return Response(_cohort_settings(course_key)) + + +class APIPermissions(GenericAPIView): + """ + Helper class defining the authentication and permission class for the subclass views. + """ + authentication_classes = ( + JwtAuthentication, + OAuth2AuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (permissions.IsAuthenticated, permissions.IsAdminUser) + + +class CohortSettings(DeveloperErrorViewMixin, APIPermissions): + """ + **Use Cases** + + Get the cohort setting for a course. + Set the cohort setting for a course. + + **Example Requests**: + + GET /api/cohorts/v1/settings/{course_id} + PUT /api/cohorts/v1/settings/{course_id} + + **Response Values** + + * is_cohorted: current status of the cohort setting + """ + + def get(self, request, course_key_string): + """ + Endpoint to fetch the course cohort settings. + """ + course_key, _ = _get_course_with_access(request, course_key_string) + return _get_cohort_settings_response(course_key) + + def put(self, request, course_key_string): + """ + Endpoint to set the course cohort settings. + """ + course_key, _ = _get_course_with_access(request, course_key_string) + + if 'is_cohorted' not in request.data: + raise self.api_error(status.HTTP_400_BAD_REQUEST, + 'Missing field "is_cohorted".') + try: + cohorts.set_course_cohorted(course_key, request.data.get('is_cohorted')) + except ValueError as err: + raise self.api_error(status.HTTP_400_BAD_REQUEST, err) + return _get_cohort_settings_response(course_key) + + +class CohortHandler(DeveloperErrorViewMixin, APIPermissions): + """ + **Use Cases** + + Get the current cohorts in a course. + Create a new cohort in a course. + Modify a cohort in a course. + + **Example Requests**: + + GET /api/cohorts/v1/courses/{course_id}/cohorts + POST /api/cohorts/v1/courses/{course_id}/cohorts + GET /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id} + PATCH /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id} + + **Response Values** + + * cohorts: List of cohorts. + * cohort: A cohort representation: + * name: The string identifier for a cohort. + * id: The integer identifier for a cohort. + * user_count: The number of students in the cohort. + * assignment_type: The string representing the assignment type. + * user_partition_id: The integer identified of the UserPartition. + * group_id: The integer identified of the specific group in the partition. + """ + + def get(self, request, course_key_string, cohort_id=None): + """ + Endpoint to get either one or all cohorts. + """ + course_key, course = _get_course_with_access(request, course_key_string) + if not cohort_id: + all_cohorts = cohorts.get_course_cohorts(course) + paginator = NamespacedPageNumberPagination() + paginator.max_page_size = MAX_PAGE_SIZE + page = paginator.paginate_queryset(all_cohorts, request) + response = [_get_cohort_representation(c, course) for c in page] + return Response(response, status=status.HTTP_200_OK) + else: + cohort = cohorts.get_cohort_by_id(course_key, cohort_id) + return _get_cohort_response(cohort, course) + + def post(self, request, course_key_string, cohort_id=None): + """ + Endpoint to create a new cohort, must not include cohort_id. + """ + if cohort_id is not None: + raise self.api_error(status.HTTP_405_METHOD_NOT_ALLOWED, + 'Please use the parent endpoint.', + 'wrong-endpoint') + course_key, course = _get_course_with_access(request, course_key_string) + name = request.data.get('name') + if not name: + raise self.api_error(status.HTTP_400_BAD_REQUEST, + '"name" must be specified to create cohort.', + 'missing-cohort-name') + assignment_type = request.data.get('assignment_type') + if not assignment_type: + raise self.api_error(status.HTTP_400_BAD_REQUEST, + '"assignment_type" must be specified to create cohort.', + 'missing-assignment-type') + return _get_cohort_response( + cohorts.add_cohort(course_key, name, assignment_type), course) + + def patch(self, request, course_key_string, cohort_id=None): + """ + Endpoint to update a cohort name and/or assignment type. + """ + if cohort_id is None: + raise self.api_error(status.HTTP_405_METHOD_NOT_ALLOWED, + 'Request method requires cohort_id in path', + 'missing-cohort-id') + name = request.data.get('name') + assignment_type = request.data.get('assignment_type') + if not any((name, assignment_type)): + raise self.api_error(status.HTTP_400_BAD_REQUEST, + 'Request must include name and/or assignment type.', + 'missing-fields') + course_key, _ = _get_course_with_access(request, course_key_string) + cohort = cohorts.get_cohort_by_id(course_key, cohort_id) + if name is not None and name != cohort.name: + if cohorts.is_cohort_exists(course_key, name): + raise self.api_error(status.HTTP_400_BAD_REQUEST, + 'A cohort with the same name already exists.', + 'cohort-name-exists') + cohort.name = name + cohort.save() + if assignment_type is not None: + try: + cohorts.set_assignment_type(cohort, assignment_type) + except ValueError as e: + raise self.api_error(status.HTTP_400_BAD_REQUEST, str(e), 'last-random-cohort') + return Response(status=status.HTTP_204_NO_CONTENT) + + +class CohortUsers(DeveloperErrorViewMixin, APIPermissions): + """ + **Use Cases** + List users in a cohort + Removes an user from a cohort. + Add a user to a specific cohort. + + **Example Requests** + + GET /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id}/users + DELETE /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id}/users/{username} + POST /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id}/users/{username} + POST /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id}/users + + **GET list of users in a cohort request parameters** + + * course_id (required): The course id of the course the cohort belongs to. + * cohort_id (required): The cohort id of the cohort to list the users in. + * page_size: A query string parameter with the number of results to return per page. + Optional. Default: 10. Maximum: 100. + * page: A query string parameter with the page number to retrieve. Optional. Default: 1. + + ** POST add a user to a cohort request parameters** + + * course_id (required): The course id of the course the cohort belongs to. + * cohort_id (required): The cohort id of the cohort to list the users in. + * users (required): A body JSON parameter with a list of usernames/email addresses of users + to be added to the cohort. + + ** DELETE remove a user from a cohort request parameters** + + * course_id (required): The course id of the course the cohort belongs to. + * cohort_id (required): The cohort id of the cohort to list the users in. + * username (required): The username of the user to be removed from the given cohort. + + **GET Response Values** + + Returns a HTTP 404 Not Found response status code when: + * The course corresponding to the corresponding course id could not be found. + * The requesting user does not have staff access to the course. + * The cohort corresponding to the given cohort id could not be found. + Returns a HTTP 200 OK response status code to indicate success. + + * count: Number of users enrolled in the given cohort. + * num_pages: Total number of pages of results. + * current_page: Current page number. + * start: The list index of the first item in the response. + * previous: The URL of the previous page of results or null if it is the first page. + * next: The URL of the next page of results or null if it is the last page. + * results: A list of users in the cohort. + * username: Username of the user. + * email: Email address of the user. + * name: Full name of the user. + + **POST Response Values** + + Returns a HTTP 404 Not Found response status code when: + * The course corresponding to the corresponding course id could not be found. + * The requesting user does not have staff access to the course. + * The cohort corresponding to the given cohort id could not be found. + Returns a HTTP 200 OK response status code to indicate success. + + * success: Boolean indicating if the operation was successful. + * added: Usernames/emails of the users that have been added to the cohort. + * changed: Usernames/emails of the users that have been moved to the cohort. + * present: Usernames/emails of the users already present in the cohort. + * unknown: Usernames/emails of the users with an unknown cohort. + * preassigned: Usernames/emails of unenrolled users that have been preassigned to the cohort. + * invalid: Invalid emails submitted. + + Adding multiple users to a cohort, send a request to: + POST /api/cohorts/v1/courses/{course_id}/cohorts/{cohort_id}/users + + With a payload such as: + { + "users": [username1, username2, username3...] + } + + **DELETE Response Values** + + Returns a HTTP 404 Not Found response status code when: + * The course corresponding to the corresponding course id could not be found. + * The requesting user does not have staff access to the course. + * The cohort corresponding to the given cohort id could not be found. + * The user corresponding to the given username could not be found. + Returns a HTTP 204 No Content response status code to indicate success. + """ + serializer_class = CohortUsersAPISerializer + + def _get_course_and_cohort(self, request, course_key_string, cohort_id): + """ + Return the course and cohort for the given course_key_string and cohort_id. + """ + course_key, _ = _get_course_with_access(request, course_key_string) + + try: + cohort = cohorts.get_cohort_by_id(course_key, cohort_id) + except CourseUserGroup.DoesNotExist: + msg = 'Cohort (ID {cohort_id}) not found for {course_key_string}'.format( + cohort_id=cohort_id, + course_key_string=course_key_string + ) + raise self.api_error(status.HTTP_404_NOT_FOUND, msg, 'cohort-not-found') + return course_key, cohort + + def get(self, request, course_key_string, cohort_id, username=None): # pylint: disable=unused-argument + """ + Lists the users in a specific cohort. + """ + + _, cohort = self._get_course_and_cohort(request, course_key_string, cohort_id) + queryset = cohort.users.all() + page = self.paginate_queryset(queryset) + + if page is not None: + serializer = self.get_serializer(page, many=True) + return self.get_paginated_response(serializer.data) + + return Response(self.get_serializer(queryset, many=True).data) + + def delete(self, request, course_key_string, cohort_id, username=None): + + """ + Removes and user from a specific cohort. + + Note: It's better to use the post method to move users between cohorts. + """ + if username is None: + raise self.api_error(status.HTTP_405_METHOD_NOT_ALLOWED, + 'Missing username in path', + 'missing-username') + course_key, cohort = self._get_course_and_cohort(request, course_key_string, cohort_id) + + try: + api.remove_user_from_cohort(course_key, username, cohort.id) + except User.DoesNotExist: + raise self.api_error(status.HTTP_404_NOT_FOUND, 'User does not exist.', 'user-not-found') + except CohortMembership.DoesNotExist: # pylint: disable=duplicate-except + raise self.api_error( + status.HTTP_400_BAD_REQUEST, + 'User not assigned to the given cohort.', + 'user-not-in-cohort' + ) + return Response(status=status.HTTP_204_NO_CONTENT) + + def post(self, request, course_key_string, cohort_id, username=None): + """ + Add given users to the cohort. + """ + _, cohort = self._get_course_and_cohort(request, course_key_string, cohort_id) + users = request.data.get('users') + if not users: + if username is not None: + users = [username] + else: + raise self.api_error(status.HTTP_400_BAD_REQUEST, 'Missing users key in payload', 'missing-users') + + added, changed, present, unknown, preassigned, invalid = [], [], [], [], [], [] + for username_or_email in users: + if not username_or_email: + continue + + try: + # A user object is only returned by add_user_to_cohort if the user already exists. + (user, previous_cohort, preassignedCohort) = cohorts.add_user_to_cohort(cohort, username_or_email) + + if preassignedCohort: + preassigned.append(username_or_email) + elif previous_cohort: + info = { + 'email': user.email, + 'previous_cohort': previous_cohort, + 'username': user.username + } + changed.append(info) + else: + info = { + 'username': user.username, + 'email': user.email + } + added.append(info) + except User.DoesNotExist: + unknown.append(username_or_email) + except ValidationError: + invalid.append(username_or_email) + except ValueError: + present.append(username_or_email) + + return Response({ + 'success': True, + 'added': added, + 'changed': changed, + 'present': present, + 'unknown': unknown, + 'preassigned': preassigned, + 'invalid': invalid + })