From 42331464eda1887d9bb43499c67fc1ea45f83e43 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 24 Jul 2013 10:39:27 -0400 Subject: [PATCH] Can't remove last instructor of a course --- .../contentstore/tests/test_users.py | 60 +++++++++++++++++++ cms/djangoapps/contentstore/views/user.py | 46 ++++++++++---- cms/templates/manage_users.html | 26 ++++---- 3 files changed, 106 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_users.py b/cms/djangoapps/contentstore/tests/test_users.py index 327bbfcf64..2fe88490ee 100644 --- a/cms/djangoapps/contentstore/tests/test_users.py +++ b/cms/djangoapps/contentstore/tests/test_users.py @@ -105,6 +105,29 @@ class UsersTestCase(CourseTestCase): self.assertIn(self.staff_groupname, groups) self.assertNotIn(self.inst_groupname, groups) + def test_detail_post_staff_other_inst(self): + inst_group, _ = Group.objects.get_or_create(name=self.inst_groupname) + self.user.groups.add(inst_group) + self.user.save() + + resp = self.client.post( + self.detail_url, + data=json.dumps({"role": "staff"}), + content_type="application/json", + HTTP_ACCEPT="application/json", + ) + self.assert2XX(resp.status_code) + # reload user from DB + ext_user = User.objects.get(email=self.ext_user.email) + groups = [g.name for g in ext_user.groups.all()] + self.assertIn(self.staff_groupname, groups) + self.assertNotIn(self.inst_groupname, groups) + # check that other user is unchanged + user = User.objects.get(email=self.user.email) + groups = [g.name for g in user.groups.all()] + self.assertNotIn(self.staff_groupname, groups) + self.assertIn(self.inst_groupname, groups) + def test_detail_post_instructor(self): resp = self.client.post( self.detail_url, @@ -171,7 +194,9 @@ class UsersTestCase(CourseTestCase): def test_detail_delete_instructor(self): group, _ = Group.objects.get_or_create(name=self.inst_groupname) + self.user.groups.add(group) self.ext_user.groups.add(group) + self.user.save() self.ext_user.save() resp = self.client.delete( @@ -183,3 +208,38 @@ class UsersTestCase(CourseTestCase): ext_user = User.objects.get(email=self.ext_user.email) groups = [g.name for g in ext_user.groups.all()] self.assertNotIn(self.inst_groupname, groups) + + def test_delete_last_instructor(self): + group, _ = Group.objects.get_or_create(name=self.inst_groupname) + self.ext_user.groups.add(group) + self.ext_user.save() + + resp = self.client.delete( + self.detail_url, + HTTP_ACCEPT="application/json", + ) + self.assertEqual(resp.status_code, 400) + result = json.loads(resp.content) + self.assertIn("error", result) + # reload user from DB + ext_user = User.objects.get(email=self.ext_user.email) + groups = [g.name for g in ext_user.groups.all()] + self.assertIn(self.inst_groupname, groups) + + def test_post_last_instructor(self): + group, _ = Group.objects.get_or_create(name=self.inst_groupname) + self.ext_user.groups.add(group) + self.ext_user.save() + + resp = self.client.post( + self.detail_url, + data={"role": "staff"}, + HTTP_ACCEPT="application/json", + ) + self.assertEqual(resp.status_code, 400) + result = json.loads(resp.content) + self.assertIn("error", result) + # reload user from DB + ext_user = User.objects.get(email=self.ext_user.email) + groups = [g.name for g in ext_user.groups.all()] + self.assertIn(self.inst_groupname, groups) diff --git a/cms/djangoapps/contentstore/views/user.py b/cms/djangoapps/contentstore/views/user.py index 6945d75da4..b7fe313226 100644 --- a/cms/djangoapps/contentstore/views/user.py +++ b/cms/djangoapps/contentstore/views/user.py @@ -87,9 +87,15 @@ def manage_users(request, org, course, name): course_module = modulestore().get_item(location) + staff_groupname = get_course_groupname_for_role(location, "staff") + staff_group, __ = Group.objects.get_or_create(name=staff_groupname) + inst_groupname = get_course_groupname_for_role(location, "instructor") + inst_group, __ = Group.objects.get_or_create(name=inst_groupname) + return render_to_response('manage_users.html', { 'context_course': course_module, - 'staff': get_users_in_course_group_by_role(location, STAFF_ROLE_NAME), + 'staff': staff_group.user_set.all(), + 'instructors': inst_group.user_set.all(), 'allow_actions': has_access(request.user, location, role=INSTRUCTOR_ROLE_NAME), }) @@ -137,8 +143,22 @@ def course_team_user(request, org, course, name, email): } return JsonResponse(msg, 400) + # make sure that the role groups exist + staff_groupname = get_course_groupname_for_role(location, "staff") + staff_group, __ = Group.objects.get_or_create(name=staff_groupname) + inst_groupname = get_course_groupname_for_role(location, "instructor") + inst_group, __ = Group.objects.get_or_create(name=inst_groupname) + if request.method == "DELETE": - # remove all roles in this course from this user + # remove all roles in this course from this user: but fail if the user + # is the last instructor in the course team + instructors = set(inst_group.user_set.all()) + if user in instructors and len(instructors) == 1: + msg = { + "error": _("You may not remove the last instructor from a course") + } + return JsonResponse(msg, 400) + for role in roles: remove_user_from_course_group(request.user, user, location, role) return JsonResponse() @@ -152,24 +172,26 @@ def course_team_user(request, org, course, name, email): try: role = payload["role"] except KeyError: - return JsonResponse({"error": "`role` is required"}, 400) + return JsonResponse({"error": _("`role` is required")}, 400) else: if not "role" in request.POST: - return JsonResponse({"error": "`role` is required"}, 400) + return JsonResponse({"error": _("`role` is required")}, 400) role = request.POST["role"] - # make sure that the role group exists - groupname = get_course_groupname_for_role(location, role) - Group.objects.get_or_create(name=groupname) - if role == "instructor": add_user_to_course_group(request.user, user, location, role) elif role == "staff": - add_user_to_course_group(request.user, user, location, role) - # should *not* be an instructor - inst_groupname = get_course_groupname_for_role(location, "instructor") - if any(g.name == inst_groupname for g in user.groups.all()): + # if we're trying to downgrade a user from "instructor" to "staff", + # make sure we have at least one other instructor in the course team. + instructors = set(inst_group.user_set.all()) + if user in instructors: + if len(instructors) == 1: + msg = { + "error": _("You may not remove the last instructor from a course") + } + return JsonResponse(msg, 400) remove_user_from_course_group(request.user, user, location, "instructor") + add_user_to_course_group(request.user, user, location, role) return JsonResponse() diff --git a/cms/templates/manage_users.html b/cms/templates/manage_users.html index 8baa9854c9..41738b1e6d 100644 --- a/cms/templates/manage_users.html +++ b/cms/templates/manage_users.html @@ -39,9 +39,9 @@
- - - + + +
%endif @@ -58,15 +58,13 @@ ${user.email} % if allow_actions:
- % if request.user.id != user.id: - % if is_user_in_course_group_role(user, context_course.location, 'instructor', check_staff=False): - <% admin_class = "remove-admin" %> - <% admin_text = "Remove Admin" %> - % else: - <% admin_class = "add-admin" %> - <% admin_text = "Add Admin" %> - % endif - ${admin_text} + <% is_instuctor = is_user_in_course_group_role(user, context_course.location, 'instructor', check_staff=False) %> + % if is_instuctor and len(instructors) == 1: + Admin + % else: + ${_("Remove Admin") if is_instuctor else _("Add Admin")} + % endif + % if request.user.id != user.id: ## can't remove yourself % endif
@@ -146,10 +144,10 @@ }); }); - $(".toggle-admin").click(function(e) { + $(".toggle-admin-role").click(function(e) { e.preventDefault() var type; - if($(this).hasClass("add-admin")) { + if($(this).hasClass("add-admin-role")) { role = 'instructor'; } else { role = 'staff';