rewrite test_access, add stricter argument to access

lms/djangoapps/instructor/access.py

@@ -23,7 +23,7 @@ def list_with_level(course, level):
     There could be other levels specific to the course.
     If there is no Group for that course-level, returns an empty list
     """
-    if level in ['beta']:
+    if level is 'beta':
         grpname = course_beta_test_group_name(course.location)
     else:
         grpname = get_access_group_name(course, level)
@@ -60,10 +60,12 @@ def _change_access(course, user, level, mode):
     mode is one of ['allow', 'revoke']
     """
 
-    if level in ['beta']:
+    if level is 'beta':
         grpname = course_beta_test_group_name(course.location)
-    else:
+    elif level in ['instructor', 'staff']:
         grpname = get_access_group_name(course, level)
+    else:
+        raise ValueError("unrecognized level '{}'".format(level))
     group, _ = Group.objects.get_or_create(name=grpname)
 
     if mode == 'allow':
@@ -78,9 +80,11 @@ def update_forum_role_membership(course_id, user, rolename, mode):
     """
     Change forum access of user.
 
-    rolename is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]
+    `rolename` is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]
+    `mode` is one of ['allow', 'revoke']
 
-    mode is one of ['allow', 'revoke']
+    if `mode` is bad, raises ValueError
+    if `rolename` does not exist, raises Role.DoesNotExist
     """
     role = Role.objects.get(course_id=course_id, name=rolename)
 

lms/djangoapps/instructor/tests/test_access.py

@@ -11,183 +11,181 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from django.test.utils import override_settings
 from courseware.tests.modulestore_config import TEST_DATA_MONGO_MODULESTORE
 
-from courseware.access import get_access_group_name
+from courseware.access import (get_access_group_name,
+                               course_beta_test_group_name)
 from django_comment_common.models import (Role,
-                                          # FORUM_ROLE_ADMINISTRATOR,
-                                          FORUM_ROLE_MODERATOR,
-                                          FORUM_ROLE_COMMUNITY_TA)
-from instructor.access import allow_access, revoke_access, list_with_level, update_forum_role_membership
+                                          FORUM_ROLE_MODERATOR)
+from instructor.access import (allow_access,
+                               revoke_access,
+                               list_with_level,
+                               update_forum_role_membership)
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
-class TestInstructorAccessControlDB(ModuleStoreTestCase):
-    """ Test instructor access administration against database effects """
-
+class TestInstructorAccessList(ModuleStoreTestCase):
+    """ Test access listings. """
     def setUp(self):
         self.course = CourseFactory.create()
 
-    def test_allow(self):
-        user = UserFactory()
-        level = 'staff'
+        self.instructors = [UserFactory.create() for _ in xrange(4)]
+        for user in self.instructors:
+            allow_access(self.course, user, 'instructor')
+        self.beta_testers = [UserFactory.create() for _ in xrange(4)]
+        for user in self.beta_testers:
+            allow_access(self.course, user, 'beta')
 
-        allow_access(self.course, user, level)
+    def test_list_instructors(self):
+        instructors = list_with_level(self.course, 'instructor')
+        self.assertEqual(set(instructors), set(self.instructors))
 
-        self.assertIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
+    def test_list_beta(self):
+        beta_testers = list_with_level(self.course, 'beta')
+        self.assertEqual(set(beta_testers), set(self.beta_testers))
 
-    def test_allow_twice(self):
-        user = UserFactory()
-        level = 'staff'
 
-        allow_access(self.course, user, level)
-        self.assertIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-        allow_access(self.course, user, level)
-        self.assertIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
+@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
+class TestInstructorAccessAllow(ModuleStoreTestCase):
+    """ Test access allow. """
+    def setUp(self):
+        self.course = CourseFactory.create()
 
-    def test_allow_revoke(self):
-        user = UserFactory()
-        level = 'staff'
-
-        allow_access(self.course, user, level)
-        self.assertIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-        revoke_access(self.course, user, level)
-        self.assertNotIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-        allow_access(self.course, user, level)
-        self.assertIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-        revoke_access(self.course, user, level)
-        self.assertNotIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-
-    def test_revoke_without_group(self):
+    def test_allow(self):
         user = UserFactory()
-        level = 'staff'
+        allow_access(self.course, user, 'staff')
+        group = Group.objects.get(
+            name=get_access_group_name(self.course, 'staff')
+        )
+        self.assertIn(user, group.user_set.all())
 
-        revoke_access(self.course, user, level)
-        self.assertNotIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
+    def test_allow_twice(self):
+        user = UserFactory()
+        allow_access(self.course, user, 'staff')
+        allow_access(self.course, user, 'staff')
+        group = Group.objects.get(
+            name=get_access_group_name(self.course, 'staff')
+        )
+        self.assertIn(user, group.user_set.all())
+
+    def test_allow_beta(self):
+        """ Test allow beta against list beta. """
+        user = UserFactory()
+        allow_access(self.course, user, 'beta')
+        self.assertIn(user, list_with_level(self.course, 'beta'))
 
-    def test_revoke_with_group(self):
+    @raises(ValueError)
+    def test_allow_badlevel(self):
         user = UserFactory()
-        level = 'staff'
-
-        Group(name=get_access_group_name(self.course, level))
-        revoke_access(self.course, user, level)
-        self.assertNotIn(user, Group.objects.get(name=get_access_group_name(self.course, 'staff')).user_set.all())
-
-    def test_allow_disallow_multiuser(self):
-        users = [UserFactory() for _ in xrange(3)]
-        levels = ['staff', 'instructor', 'staff']
-        antilevels = ['instructor', 'staff', 'instructor']
-
-        allow_access(self.course, users[0], levels[0])
-        allow_access(self.course, users[1], levels[1])
-        allow_access(self.course, users[2], levels[2])
-        self.assertIn(users[0], Group.objects.get(name=get_access_group_name(self.course, levels[0])).user_set.all())
-        self.assertIn(users[1], Group.objects.get(name=get_access_group_name(self.course, levels[1])).user_set.all())
-        self.assertIn(users[2], Group.objects.get(name=get_access_group_name(self.course, levels[2])).user_set.all())
-
-        revoke_access(self.course, users[0], levels[0])
-        revoke_access(self.course, users[0], antilevels[0])
-        self.assertNotIn(users[0], Group.objects.get(name=get_access_group_name(self.course, levels[0])).user_set.all())
-        self.assertIn(users[1], Group.objects.get(name=get_access_group_name(self.course, levels[1])).user_set.all())
-        self.assertIn(users[2], Group.objects.get(name=get_access_group_name(self.course, levels[2])).user_set.all())
-
-        revoke_access(self.course, users[1], levels[1])
-        allow_access(self.course, users[0], antilevels[0])
-        self.assertNotIn(users[0], Group.objects.get(name=get_access_group_name(self.course, levels[0])).user_set.all())
-        self.assertIn(users[0], Group.objects.get(name=get_access_group_name(self.course, antilevels[0])).user_set.all())
-        self.assertNotIn(users[1], Group.objects.get(name=get_access_group_name(self.course, levels[1])).user_set.all())
-        self.assertIn(users[2], Group.objects.get(name=get_access_group_name(self.course, levels[2])).user_set.all())
+        allow_access(self.course, user, 'robot-not-a-level')
+        group = Group.objects.get(name=get_access_group_name(self.course, 'robot-not-a-level'))
+        self.assertIn(user, group.user_set.all())
 
+    @raises(Exception)
+    def test_allow_noneuser(self):
+        user = None
+        allow_access(self.course, user, 'staff')
+        group = Group.objects.get(name=get_access_group_name(self.course, 'staff'))
+        self.assertIn(user, group.user_set.all())
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
-class TestInstructorAccessControlPrefilledDB(ModuleStoreTestCase):
-    """
-    Test access with existing users.
-    """
+class TestInstructorAccessRevoke(ModuleStoreTestCase):
+    """ Test access revoke. """
     def setUp(self):
         self.course = CourseFactory.create()
 
-        # setup instructors
-        self.instructors = set([UserFactory.create(), UserFactory.create()])
-        for user in self.instructors:
-            allow_access(self.course, user, 'instructor')
-
-    def test_list_with_level(self):
-        instructors = set(list_with_level(self.course, 'instructor'))
-        self.assertEqual(instructors, self.instructors)
-
-    def test_list_with_level_not_yet_group(self):
-        instructors = set(list_with_level(self.course, 'staff'))
-        self.assertEqual(instructors, set())
-
-    def test_list_with_level_bad_group(self):
-        self.assertEqual(set(list_with_level(self.course, 'robot-definitely-not-a-group')), set())
+        self.staff = [UserFactory.create() for _ in xrange(4)]
+        for user in self.staff:
+            allow_access(self.course, user, 'staff')
+        self.beta_testers = [UserFactory.create() for _ in xrange(4)]
+        for user in self.beta_testers:
+            allow_access(self.course, user, 'beta')
 
-    def test_list_with_level_beta(self):
-        beta_testers_result = set(list_with_level(self.course, 'beta'))
-        self.assertEqual(set(), beta_testers_result)
+    def test_revoke(self):
+        user = self.staff[0]
+        revoke_access(self.course, user, 'staff')
+        group = Group.objects.get(
+            name=get_access_group_name(self.course, 'staff')
+        )
+        self.assertNotIn(user, group.user_set.all())
+
+    def test_revoke_twice(self):
+        user = self.staff[0]
+        revoke_access(self.course, user, 'staff')
+        group = Group.objects.get(
+            name=get_access_group_name(self.course, 'staff')
+        )
+        self.assertNotIn(user, group.user_set.all())
+
+    def test_revoke_beta(self):
+        user = self.beta_testers[0]
+        revoke_access(self.course, user, 'beta')
+        self.assertNotIn(user, list_with_level(self.course, 'beta'))
 
-        beta_testers = set([UserFactory.create(), UserFactory.create()])
-        for user in beta_testers:
-            allow_access(self.course, user, 'beta')
-        beta_testers_result = set(list_with_level(self.course, 'beta'))
-        self.assertEqual(beta_testers, beta_testers_result)
+    @raises(ValueError)
+    def test_revoke_badrolename(self):
+        user = UserFactory()
+        revoke_access(self.course, user, 'robot-not-a-level')
+        group = Group.objects.get(
+            name=get_access_group_name(self.course, 'robot-not-a-level')
+        )
+        self.assertNotIn(user, group.user_set.all())
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
-class TestInstructorAccessForumDB(ModuleStoreTestCase):
+class TestInstructorAccessForum(ModuleStoreTestCase):
     """
     Test forum access control.
     """
     def setUp(self):
         self.course = CourseFactory.create()
 
-        self.moderators = set([UserFactory.create() for _ in xrange(4)])
-        self.mod_role = Role.objects.create(course_id=self.course.id, name=FORUM_ROLE_MODERATOR)
+        self.mod_role = Role.objects.create(
+            course_id=self.course.id,
+            name=FORUM_ROLE_MODERATOR
+        )
+        self.moderators = [UserFactory.create() for _ in xrange(4)]
         for user in self.moderators:
             self.mod_role.users.add(user)
 
-    def test_update_forum_membership_allow_existing_role(self):
+    def test_allow(self):
         user = UserFactory.create()
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
         self.assertIn(user, self.mod_role.users.all())
 
-    def test_update_forum_membership_allow_existing_role_allowed_user(self):
+    def test_allow_twice(self):
         user = UserFactory.create()
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
+        self.assertIn(user, self.mod_role.users.all())
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'allow')
         self.assertIn(user, self.mod_role.users.all())
 
     @raises(Role.DoesNotExist)
-    def test_update_forum_membership_allow_not_existing_role(self):
+    def test_allow_badrole(self):
         user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_COMMUNITY_TA, 'allow')
+        update_forum_role_membership(self.course.id, user, 'robot-not-a-real-role', 'allow')
 
-    def test_update_forum_membership_revoke_existing_role(self):
-        user = iter(self.moderators).next()
+    def test_revoke(self):
+        user = self.moderators[0]
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
 
-    def test_update_forum_membership_existing_role_revoked_user(self):
-        user = iter(self.moderators).next()
+    def test_revoke_twice(self):
+        user = self.moderators[0]
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        self.assertNotIn(user, self.mod_role.users.all())
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
         self.assertNotIn(user, self.mod_role.users.all())
 
-    @raises(Role.DoesNotExist)
-    def test_update_forum_membership_revoke_not_existing_role(self):
-        user = iter(self.moderators).next()
-        update_forum_role_membership(self.course.id, user, FORUM_ROLE_COMMUNITY_TA, 'revoke')
-
-    @raises(Role.DoesNotExist)
-    def test_update_forum_membership_bad_role_allow(self):
-        user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, 'robot-definitely-not-a-forum-role', 'allow')
+    def test_revoke_notallowed(self):
+        user = UserFactory()
+        update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'revoke')
+        self.assertNotIn(user, self.mod_role.users.all())
 
     @raises(Role.DoesNotExist)
-    def test_update_forum_membership_bad_role_revoke(self):
-        user = UserFactory.create()
-        update_forum_role_membership(self.course.id, user, 'robot-definitely-not-a-forum-role', 'revoke')
+    def test_revoke_badrole(self):
+        user = self.moderators[0]
+        update_forum_role_membership(self.course.id, user, 'robot-not-a-real-role', 'allow')
 
     @raises(ValueError)
-    def test_update_forum_membership_bad_mode(self):
-        user = iter(self.moderators).next()
+    def test_bad_mode(self):
+        user = UserFactory()
         update_forum_role_membership(self.course.id, user, FORUM_ROLE_MODERATOR, 'robot-not-a-mode')