Merge pull request #3028 from edx/sarina/LMS-2262

More helpful UX in beta membership widget

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. """

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)

lms/static/coffee/src/instructor_dashboard/membership.coffee

@@ -157,9 +157,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) ->