add csv & enrollment & access api tests, disallow instructors from un-instructoring themselves, rename mode to action for access

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))

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
+@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
-# # modify_access
+class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase):
-# # list_course_role_members
+    """
+    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

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"

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']
+  # action should be one of ['allow', 'revoke']
-  access_change: (email, mode, cb) ->
+  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."