From 7905e7d2891c7f7593cd2c8982f0877bde06d7a5 Mon Sep 17 00:00:00 2001 From: Miles Steele Date: Wed, 17 Jul 2013 09:56:23 -0400 Subject: [PATCH] add csv & enrollment & access api tests, disallow instructors from un-instructoring themselves, rename mode to action for access --- lms/djangoapps/instructor/access.py | 22 +- lms/djangoapps/instructor/tests/test_api.py | 270 +++++++++++++++++- lms/djangoapps/instructor/views/api.py | 48 +++- .../instructor_dashboard/membership.coffee | 6 +- 4 files changed, 312 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 7b42d58556..99bebdee4a 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -52,12 +52,12 @@ def revoke_access(course, user, level): _change_access(course, user, level, 'revoke') -def _change_access(course, user, level, mode): +def _change_access(course, user, level, action): """ Change access of user. level is one of ['instructor', 'staff', 'beta'] - mode is one of ['allow', 'revoke'] + action is one of ['allow', 'revoke'] NOTE: will NOT create a group that does not yet exist. """ @@ -70,29 +70,29 @@ def _change_access(course, user, level, mode): raise ValueError("unrecognized level '{}'".format(level)) group, _ = Group.objects.get_or_create(name=grpname) - if mode == 'allow': + if action == 'allow': user.groups.add(group) - elif mode == 'revoke': + elif action == 'revoke': user.groups.remove(group) else: - raise ValueError("unrecognized mode '{}'".format(mode)) + raise ValueError("unrecognized action '{}'".format(action)) -def update_forum_role_membership(course_id, user, rolename, mode): +def update_forum_role_membership(course_id, user, rolename, action): """ Change forum access of user. `rolename` is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA] - `mode` is one of ['allow', 'revoke'] + `action` is one of ['allow', 'revoke'] - if `mode` is bad, raises ValueError + if `action` is bad, raises ValueError if `rolename` does not exist, raises Role.DoesNotExist """ role = Role.objects.get(course_id=course_id, name=rolename) - if mode == 'allow': + if action == 'allow': role.users.add(user) - elif mode == 'revoke': + elif action == 'revoke': role.users.remove(user) else: - raise ValueError("unrecognized mode '{}'".format(mode)) + raise ValueError("unrecognized action '{}'".format(action)) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index f4bfdbde7f..4468f9864c 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -9,6 +9,7 @@ from nose.tools import raises from django.test.utils import override_settings from django.core.urlresolvers import reverse +from django.contrib.auth.models import User from courseware.tests.modulestore_config import TEST_DATA_MONGO_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from courseware.tests.helpers import LoginEnrollmentTestCase @@ -17,6 +18,7 @@ from student.tests.factories import UserFactory, AdminFactory from student.models import CourseEnrollment +from instructor.access import allow_access from instructor.views.api import _split_input_list, _msk_from_problem_urlname @@ -75,12 +77,260 @@ class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): response = self.client.get(url, {}) self.assertEqual(response.status_code, 403) -# class TestInstructorAPILevelsEnrollment -# # students_update_enrollment -# class TestInstructorAPILevelsAccess -# # modify_access -# # list_course_role_members +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test enrollment modification endpoint. + + This test does NOT exhaustively test state changes, that is the + job of test_enrollment. This tests the response and action switch. + """ + def setUp(self): + self.instructor = AdminFactory.create() + self.course = CourseFactory.create() + self.client.login(username=self.instructor.username, password='test') + + self.enrolled_student = UserFactory() + CourseEnrollment.objects.create( + user=self.enrolled_student, + course_id=self.course.id + ) + self.notenrolled_student = UserFactory() + + self.notregistered_email = 'robot-not-an-email-yet@robot.org' + self.assertEqual(User.objects.filter(email=self.notregistered_email).count(), 0) + + # enable printing of large diffs + # from failed assertions in the event of a test failure. + self.maxDiff = None + + def test_missing_params(self): + """ Test missing all query parameters. """ + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + + def test_bad_action(self): + """ Test with an invalid action. """ + action = 'robot-not-an-action' + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'emails': self.enrolled_student.email, 'action': action}) + self.assertEqual(response.status_code, 400) + + def test_enroll(self): + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'emails': self.notenrolled_student.email, 'action': 'enroll'}) + print "type(self.notenrolled_student.email): {}".format(type(self.notenrolled_student.email)) + self.assertEqual(response.status_code, 200) + # test that the user is now enrolled + + self.assertEqual( + self.notenrolled_student.courseenrollment_set.filter( + course_id=self.course.id + ).count(), + 1 + ) + + # test the response data + expected = { + "action": "enroll", + "auto_enroll": False, + "results": [ + { + "email": self.notenrolled_student.email, + "before": { + "enrollment": False, + "auto_enroll": False, + "user": True, + "allowed": False, + }, + "after": { + "enrollment": True, + "auto_enroll": False, + "user": True, + "allowed": False, + } + } + ] + } + + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + + def test_unenroll(self): + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'emails': self.enrolled_student.email, 'action': 'unenroll'}) + print "type(self.enrolled_student.email): {}".format(type(self.enrolled_student.email)) + self.assertEqual(response.status_code, 200) + # test that the user is now unenrolled + + self.assertEqual( + self.enrolled_student.courseenrollment_set.filter( + course_id=self.course.id + ).count(), + 0 + ) + + # test the response data + expected = { + "action": "unenroll", + "auto_enroll": False, + "results": [ + { + "email": self.enrolled_student.email, + "before": { + "enrollment": True, + "auto_enroll": False, + "user": True, + "allowed": False, + }, + "after": { + "enrollment": False, + "auto_enroll": False, + "user": True, + "allowed": False, + } + } + ] + } + + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test endpoints whereby instructors can change permissions + of other users. + + This test does NOT test whether the actions had an effect on the + database, that is the job of test_access. + This tests the response and action switch. + Actually, modify_access does not having a very meaningful + response yet, so only the status code is tested. + """ + def setUp(self): + self.instructor = AdminFactory.create() + self.course = CourseFactory.create() + self.client.login(username=self.instructor.username, password='test') + + self.other_instructor = UserFactory() + allow_access(self.course, self.other_instructor, 'instructor') + self.other_staff = UserFactory() + allow_access(self.course, self.other_staff, 'staff') + self.other_user = UserFactory() + + def test_modify_access_noparams(self): + """ Test missing all query parameters. """ + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + + def test_modify_access_bad_action(self): + """ Test with an invalid action parameter. """ + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'email': self.other_staff.email, + 'rolename': 'staff', + 'action': 'robot-not-an-action', + }) + self.assertEqual(response.status_code, 400) + + def test_modify_access_allow(self): + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'email': self.other_instructor.email, + 'rolename': 'staff', + 'action': 'allow', + }) + self.assertEqual(response.status_code, 200) + + def test_modify_access_revoke(self): + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'email': self.other_staff.email, + 'rolename': 'staff', + 'action': 'revoke', + }) + self.assertEqual(response.status_code, 200) + + def test_modify_access_revoke_not_allowed(self): + """ Test revoking access that a user does not have. """ + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'email': self.other_staff.email, + 'rolename': 'instructor', + 'action': 'revoke', + }) + self.assertEqual(response.status_code, 200) + + def test_modify_access_revoke_self(self): + """ + Test that an instructor cannot remove instructor privelages from themself. + """ + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'email': self.instructor.email, + 'rolename': 'instructor', + 'action': 'revoke', + }) + self.assertEqual(response.status_code, 400) + + def test_list_course_role_members_noparams(self): + """ Test missing all query parameters. """ + url = reverse('list_course_role_members', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + + def test_list_course_role_members_bad_rolename(self): + """ Test with an invalid rolename parameter. """ + url = reverse('list_course_role_members', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'rolename': 'robot-not-a-rolename', + }) + print response + self.assertEqual(response.status_code, 400) + + def test_list_course_role_members_staff(self): + url = reverse('list_course_role_members', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'rolename': 'staff', + }) + print response + self.assertEqual(response.status_code, 200) + + # check response content + expected = { + 'course_id': self.course.id, + 'staff': [ + { + 'username': self.other_staff.username, + 'email': self.other_staff.email, + 'first_name': self.other_staff.first_name, + 'last_name': self.other_staff.last_name, + } + ] + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + + def test_list_course_role_members_beta(self): + url = reverse('list_course_role_members', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'rolename': 'beta', + }) + print response + self.assertEqual(response.status_code, 200) + + # check response content + expected = { + 'course_id': self.course.id, + 'beta': [] + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) @@ -93,7 +343,6 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa self.course = CourseFactory.create() self.client.login(username=self.instructor.username, password='test') - # self.students = [UserFactory(email="foobar{}@robot.org".format(i)) for i in xrange(6)] self.students = [UserFactory() for _ in xrange(6)] for student in self.students: CourseEnrollment.objects.create(user=student, course_id=self.course.id) @@ -115,6 +364,15 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa self.assertEqual(student_json['username'], student.username) self.assertEqual(student_json['email'], student.email) + def test_get_students_features_csv(self): + """ + Test that some minimum of information is formatted + correctly in the response to get_students_features. + """ + url = reverse('get_students_features', kwargs={'course_id': self.course.id}) + response = self.client.get(url + '/csv', {}) + self.assertEqual(response['Content-Type'], 'text/csv') + def test_get_distribution_no_feature(self): """ Test that get_distribution lists available features diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 0d02121e11..3db4269c30 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -134,7 +134,7 @@ def students_update_enrollment(request, course_id): Returns an analog to this JSON structure: { "action": "enroll", - "auto_enroll": false + "auto_enroll": false, "results": [ { "email": "testemail@test.org", @@ -202,17 +202,19 @@ def students_update_enrollment(request, course_id): @require_query_params( email="user email", rolename="'instructor', 'staff', or 'beta'", - mode="'allow' or 'revoke'" + action="'allow' or 'revoke'" ) def modify_access(request, course_id): """ - Modify staff/instructor access. + Modify staff/instructor access of other user. Requires instructor access. + NOTE: instructors cannot remove their own instructor access. + Query parameters: email is the target users email rolename is one of ['instructor', 'staff', 'beta'] - mode is one of ['allow', 'revoke'] + action is one of ['allow', 'revoke'] """ course = get_course_with_access( request.user, course_id, 'instructor', depth=None @@ -220,7 +222,7 @@ def modify_access(request, course_id): email = request.GET.get('email') rolename = request.GET.get('rolename') - mode = request.GET.get('mode') + action = request.GET.get('action') if not rolename in ['instructor', 'staff', 'beta']: return HttpResponseBadRequest( @@ -229,17 +231,23 @@ def modify_access(request, course_id): user = User.objects.get(email=email) - if mode == 'allow': + # disallow instructors from removing their own instructor access. + if rolename == 'instructor' and user == request.user and action != 'allow': + return HttpResponseBadRequest( + "An instructor cannot remove their own instructor access." + ) + + if action == 'allow': access.allow_access(course, user, rolename) - elif mode == 'revoke': + elif action == 'revoke': access.revoke_access(course, user, rolename) else: - raise ValueError("unrecognized mode '{}'".format(mode)) + return HttpResponseBadRequest("unrecognized action '{}'".format(action)) response_payload = { 'email': email, 'rolename': rolename, - 'mode': mode, + 'action': action, 'success': 'yes', } response = HttpResponse( @@ -258,6 +266,18 @@ def list_course_role_members(request, course_id): Requires instructor access. rolename is one of ['instructor', 'staff', 'beta'] + + Returns JSON of the form { + "course_id": "some/course/id", + "staff": [ + { + "username": "staff1", + "email": "staff1@example.org", + "first_name": "Joe", + "last_name": "Shmoe", + } + ] + } """ course = get_course_with_access( request.user, course_id, 'instructor', depth=None @@ -627,7 +647,7 @@ def list_forum_members(request, course_id): @require_query_params( email="the target users email", rolename="the forum role", - mode="'allow' or 'revoke'", + action="'allow' or 'revoke'", ) @common_exceptions_400 def update_forum_role_membership(request, course_id): @@ -637,24 +657,24 @@ def update_forum_role_membership(request, course_id): Query parameters: email is the target users email rolename is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA] - mode is one of ['allow', 'revoke'] + action is one of ['allow', 'revoke'] """ email = request.GET.get('email') rolename = request.GET.get('rolename') - mode = request.GET.get('mode') + action = request.GET.get('action') if not rolename in [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]: return HttpResponseBadRequest() try: user = User.objects.get(email=email) - access.update_forum_role_membership(course_id, user, rolename, mode) + access.update_forum_role_membership(course_id, user, rolename, action) except Role.DoesNotExist: return HttpResponseBadRequest("Role does not exist.") response_payload = { 'course_id': course_id, - 'mode': mode, + 'action': action, } response = HttpResponse( json.dumps(response_payload), content_type="application/json" diff --git a/lms/static/coffee/src/instructor_dashboard/membership.coffee b/lms/static/coffee/src/instructor_dashboard/membership.coffee index e1e68d739e..48e2531932 100644 --- a/lms/static/coffee/src/instructor_dashboard/membership.coffee +++ b/lms/static/coffee/src/instructor_dashboard/membership.coffee @@ -251,15 +251,15 @@ class AuthList # update the access of a user. # (add or remove them from the list) - # mode should be one of ['allow', 'revoke'] - access_change: (email, mode, cb) -> + # action should be one of ['allow', 'revoke'] + access_change: (email, action, cb) -> $.ajax dataType: 'json' url: @$add_section.data 'endpoint' data: email: email rolename: @rolename - mode: mode + action: action success: (data) -> cb?(data) error: std_ajax_err => @$request_response_error.text "Error changing user's permissions."