diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 9eb10d10f0..864ddf7756 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -904,6 +904,38 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase }) self.assertEqual(response.status_code, 200) + def test_modify_access_with_fake_user(self): + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'unique_student_identifier': 'GandalfTheGrey', + 'rolename': 'staff', + 'action': 'revoke', + }) + self.assertEqual(response.status_code, 200) + expected = { + 'unique_student_identifier': 'GandalfTheGrey', + 'userDoesNotExist': True, + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + + def test_modify_access_with_inactive_user(self): + self.other_user.is_active = False + self.other_user.save() # pylint: disable=no-member + url = reverse('modify_access', kwargs={'course_id': self.course.id}) + response = self.client.get(url, { + 'unique_student_identifier': self.other_user.username, + 'rolename': 'beta', + 'action': 'allow', + }) + self.assertEqual(response.status_code, 200) + expected = { + 'unique_student_identifier': self.other_user.username, + 'inactiveUser': True, + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) + 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}) @@ -924,7 +956,16 @@ class TestInstructorAPILevelsAccess(ModuleStoreTestCase, LoginEnrollmentTestCase 'rolename': 'instructor', 'action': 'revoke', }) - self.assertEqual(response.status_code, 400) + self.assertEqual(response.status_code, 200) + # check response content + expected = { + 'unique_student_identifier': self.instructor.username, + 'rolename': 'instructor', + 'action': 'revoke', + 'removingSelfAsInstructor': True, + } + res_json = json.loads(response.content) + self.assertEqual(res_json, expected) def test_list_course_role_members_noparams(self): """ Test missing all query parameters. """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 4021bedfb4..70c4fbcc75 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -395,8 +395,25 @@ def modify_access(request, course_id): course = get_course_with_access( request.user, course_id, 'instructor', depth=None ) + try: + user = get_student_from_identifier(request.GET.get('unique_student_identifier')) + except User.DoesNotExist: + response_payload = { + 'unique_student_identifier': request.GET.get('unique_student_identifier'), + 'userDoesNotExist': True, + } + return JsonResponse(response_payload) + + # Check that user is active, because add_users + # in common/djangoapps/student/roles.py fails + # silently when we try to add an inactive user. + if not user.is_active: + response_payload = { + 'unique_student_identifier': user.username, + 'inactiveUser': True, + } + return JsonResponse(response_payload) - user = get_student_from_identifier(request.GET.get('unique_student_identifier')) rolename = request.GET.get('rolename') action = request.GET.get('action') @@ -407,9 +424,13 @@ def modify_access(request, course_id): # 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." - ) + response_payload = { + 'unique_student_identifier': user.username, + 'rolename': rolename, + 'action': action, + 'removingSelfAsInstructor': True, + } + return JsonResponse(response_payload) if action == 'allow': allow_access(course, user, rolename) diff --git a/lms/static/coffee/src/instructor_dashboard/membership.coffee b/lms/static/coffee/src/instructor_dashboard/membership.coffee index d66e4c5e11..c385fc3507 100644 --- a/lms/static/coffee/src/instructor_dashboard/membership.coffee +++ b/lms/static/coffee/src/instructor_dashboard/membership.coffee @@ -156,9 +156,23 @@ class AuthListWidget extends MemberListWidget unique_student_identifier: unique_student_identifier rolename: @rolename action: action - success: (data) => cb? null, data + success: (data) => @member_response data error: std_ajax_err => cb? gettext "Error changing user's permissions." + member_response: (data) -> + @clear_errors() + @clear_input() + if data.userDoesNotExist + msg = gettext("Could not find a user with username or email address '<%= identifier %>'.") + @show_errors _.template(msg, {identifier: data.unique_student_identifier}) + else if data.inactiveUser + msg = gettext("Error: User '<%= username %>' has not yet activated their account. Users must create and activate their accounts before they can be assigned a role.") + @show_errors _.template(msg, {username: data.unique_student_identifier}) + else if data.removingSelfAsInstructor + @show_errors gettext "Error: You cannot remove yourself from the Instructor group!" + else + @reload_list() + class BetaTesterBulkAddition constructor: (@$container) ->