The course creator view adds granted users course creator status.

Includes unit tests and modifications to authz.py.

cms/djangoapps/auth/authz.py

@@ -209,19 +209,11 @@ def is_user_in_creator_group(user):
     return True
 
 
-def _grant_instructors_creator_access(caller):
+def get_users_with_instructor_role():
     """
-    This is to be called only by either a command line code path or through an app which has already
-    asserted permissions to do this action.
-
-    Gives all users with instructor role course creator rights, and returns the set of
-    such users.
-    This is only intended to be run once on a given environment.
+    Returns all users with the role 'instructor'
     """
-    instructors = _get_users_with_role(INSTRUCTOR_ROLE_NAME)
-    for user in instructors:
-        add_user_to_creator_group(caller, user)
-    return instructors
+    return _get_users_with_role(INSTRUCTOR_ROLE_NAME)
 
 
 def get_users_with_staff_role():

cms/djangoapps/auth/tests/test_authz.py

@@ -9,8 +9,8 @@ from django.core.exceptions import PermissionDenied
 
 from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, is_user_in_creator_group,\
     create_all_course_groups, add_user_to_course_group, STAFF_ROLE_NAME, INSTRUCTOR_ROLE_NAME,\
-    is_user_in_course_group_role, remove_user_from_course_group, _grant_instructors_creator_access,\
-    get_users_with_staff_role
+    is_user_in_course_group_role, remove_user_from_course_group, get_users_with_staff_role,\
+    get_users_with_instructor_role
 
 
 class CreatorGroupTest(TestCase):
@@ -182,48 +182,22 @@ class CourseGroupTest(TestCase):
         add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
 
         location2 = 'i4x', 'mitX', '103', 'course2', 'test2'
-        staff2 = User.objects.create_user('teststaff2', 'teststaff+courses@edx.org', 'foo')
+        staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
         create_all_course_groups(self.creator, location2)
         add_user_to_course_group(self.creator, staff2, location2, STAFF_ROLE_NAME)
 
         self.assertSetEqual({self.staff, staff2, self.creator}, get_users_with_staff_role())
 
+    def test_get_instructor(self):
+        # Do this test with creators in 2 different classes.
+        create_all_course_groups(self.creator, self.location)
+        add_user_to_course_group(self.creator, self.staff, self.location, STAFF_ROLE_NAME)
+
+        location2 = 'i4x', 'mitX', '103', 'course2', 'test2'
+        creator2 = User.objects.create_user('testcreator2', 'testcreator2+courses@edx.org', 'foo')
+        staff2 = User.objects.create_user('teststaff2', 'teststaff2+courses@edx.org', 'foo')
+        create_all_course_groups(creator2, location2)
+        add_user_to_course_group(creator2, staff2, location2, STAFF_ROLE_NAME)
+
+        self.assertSetEqual({self.creator, creator2}, get_users_with_instructor_role())
 
-class GrantInstructorsCreatorAccessTest(TestCase):
-    """
-    Tests granting existing instructors course creator rights.
-    """
-    def create_course(self, index):
-        """
-        Creates a course with one instructor and one staff member.
-        """
-        creator = User.objects.create_user('testcreator' + str(index), 'testcreator+courses@edx.org', 'foo')
-        staff = User.objects.create_user('teststaff' + str(index), 'teststaff+courses@edx.org', 'foo')
-        location = 'i4x', 'mitX', str(index), 'course', 'test'
-        create_all_course_groups(creator, location)
-        add_user_to_course_group(creator, staff, location, STAFF_ROLE_NAME)
-        return [creator, staff]
-
-    def test_grant_creator_access(self):
-        """
-        Test for _grant_instructors_creator_access.
-        """
-        [creator1, staff1] = self.create_course(1)
-        [creator2, staff2] = self.create_course(2)
-        with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}):
-            # Initially no creators.
-            self.assertFalse(is_user_in_creator_group(creator1))
-            self.assertFalse(is_user_in_creator_group(creator2))
-            self.assertFalse(is_user_in_creator_group(staff1))
-            self.assertFalse(is_user_in_creator_group(staff2))
-
-            admin = User.objects.create_user('populate_creators_command', 'grant+creator+access@edx.org', 'foo')
-            admin.is_staff = True
-            instructors = _grant_instructors_creator_access(admin)
-            self.assertSetEqual({creator1, creator2}, instructors)
-
-            # Now instructors only are creators.
-            self.assertTrue(is_user_in_creator_group(creator1))
-            self.assertTrue(is_user_in_creator_group(creator2))
-            self.assertFalse(is_user_in_creator_group(staff1))
-            self.assertFalse(is_user_in_creator_group(staff2))

cms/djangoapps/contentstore/management/commands/populate_creators.py

@@ -3,7 +3,7 @@ Script for granting existing course instructors course creator privileges.
 
 This script is only intended to be run once on a given environment.
 """
-from auth.authz import _grant_instructors_creator_access, get_users_with_staff_role
+from auth.authz import get_users_with_instructor_role, get_users_with_staff_role
 from course_creators.views import add_user_with_status_granted, add_user_with_status_unrequested
 from django.core.management.base import BaseCommand
 
@@ -32,15 +32,14 @@ class Command(BaseCommand):
             # the admin user will already exist.
             admin = User.objects.get(username=username, email=email)
 
-        creators = _grant_instructors_creator_access(admin)
-        for user in creators:
-            add_user_with_status_granted(user)
+        for user in get_users_with_instructor_role():
+            add_user_with_status_granted(admin, user)
 
         # Some users will be both staff and instructors. Those folks have been
         # added with status granted above, and add_user_with_status_unrequested
         # will not try to add them again if they already exist in the course creator database.
         for user in get_users_with_staff_role():
-            add_user_with_status_unrequested(user)
+            add_user_with_status_unrequested(admin, user)
 
         # There could be users who are not in either staff or instructor (they've
         # never actually done anything in Studio). I plan to add those as unrequested

cms/djangoapps/course_creators/models.py

@@ -6,19 +6,18 @@ from django.db.models.signals import post_init, post_save
 from django.dispatch import receiver
 from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, get_user_by_email
 
-import datetime
+from django.utils import timezone
 
 
 class CourseCreator(models.Model):
     """
     Creates the database table model.
     """
-    STATES = (
-        (u'u', u'unrequested'),
-        (u'p', u'pending'),
-        (u'g', u'granted'),
-        (u'd', u'denied'),
-        )
+    STATES = ((u'u', u'unrequested'),
+              (u'p', u'pending'),
+              (u'g', u'granted'),
+              (u'd', u'denied'),
+             )
 
     username = models.CharField(max_length=64, blank=False, help_text="Studio username", primary_key=True, unique=True)
     email = models.CharField(max_length=128, blank=False, help_text="Registered e-mail address")
@@ -29,14 +28,16 @@ class CourseCreator(models.Model):
     note = models.CharField(max_length=512, blank=True, help_text='Optional notes about this user (for example, '
                                                                   'why course creation access was denied)')
 
-
     def __unicode__(self):
-        s = "%s %s | %s [%s] | %s" % (self.username, self.email, self.state, self.state_changed, self.note)
+        s = "%str %str | %str [%str] | %str" % (self.username, self.email, self.state, self.state_changed, self.note)
         return s
 
 
 @receiver(post_init, sender=CourseCreator)
-def post_init_callback(sender, **kwargs):
+def post_init_callback(_sender, **kwargs):
+    """
+    Extend to remove deleted users and store previous state.
+    """
     instance = kwargs['instance']
     user = get_user_by_email(instance.email)
     if user is None:
@@ -47,19 +48,26 @@ def post_init_callback(sender, **kwargs):
 
 
 @receiver(post_save, sender=CourseCreator)
-def post_save_callback(sender, **kwargs):
+def post_save_callback(_sender, **kwargs):
+    """
+    Extend to remove deleted users, update state_changed time,
+    and modify the course creator group in authz.py.
+    """
     instance = kwargs['instance']
     # We only wish to modify the state_changed time if the state has been modified. We don't wish to
     # modify it for changes to the notes field.
     if instance.state != instance.orig_state:
-        instance.state_changed = datetime.datetime.now()
-        # We removed bad users in post_init.
         user = get_user_by_email(instance.email)
-        if instance.state == 'g':
-            # We have granted access, add to course group
-            add_user_to_creator_group(instance.admin, user)
+        if user is None:
+            # User has been removed, delete from this table.
+            instance.delete()
         else:
-            remove_user_from_creator_group(instance.admin, user)
+            instance.state_changed = timezone.now()
+            if instance.state == 'g':
+                # We have granted access, add to course group
+                add_user_to_creator_group(instance.admin, user)
+            else:
+                remove_user_from_creator_group(instance.admin, user)
 
-        instance.orig_state = instance.state
-        instance.save()
+            instance.orig_state = instance.state
+            instance.save()

cms/djangoapps/course_creators/tests/test_admin.py

+"""
+Tests course_creators.admin.py.
+"""
+
+from django.test import TestCase
+from django.contrib.auth.models import User
+from django.contrib.admin.sites import AdminSite
+from django.http import HttpRequest
+import mock
+
+from course_creators.admin import CourseCreatorAdmin
+from course_creators.models import CourseCreator
+from auth.authz import is_user_in_creator_group
+
+
+class CourseCreatorAdminTest(TestCase):
+    """
+    Tests for course creator admin.
+    """
+
+    def setUp(self):
+        """ Test case setup """
+        self.user = User.objects.create_user('test_user', 'test_user+courses@edx.org', 'foo')
+        self.table_entry = CourseCreator(username=self.user.username, email=self.user.email)
+        self.table_entry.save()
+
+        self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo')
+        self.admin.is_staff = True
+
+        self.request = HttpRequest()
+        self.request.user = self.admin
+
+        self.creator_admin = CourseCreatorAdmin(self.table_entry, AdminSite())
+
+    def test_change_status(self):
+        """
+        Tests that updates to state impact the creator group maintained in authz.py.
+        """
+        def change_state(state, is_creator):
+            """ Helper method for changing state """
+            self.table_entry.state = state
+            self.creator_admin.save_model(self.request, self.table_entry, None, True)
+            self.assertEqual(is_creator, is_user_in_creator_group(self.user))
+
+        with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}):
+            # User is initially unrequested.
+            self.assertFalse(is_user_in_creator_group(self.user))
+
+            # change state to 'g' (granted)
+            change_state('g', True)
+
+            # change state to 'd' (denied)
+            change_state('d', False)
+
+            # and change state back to 'g' (granted)
+            change_state('g', True)
+
+            # change state to 'p' (pending)
+            change_state('p', False)
+
+            # and change state back to 'g' (granted)
+            change_state('g', True)
+
+            # and change state back to 'u' (unrequested)
+            change_state('u', False)
+
+
+    def test_delete_bad_user(self):
+        """
+        Tests that users who no longer exist are deleted from the table.
+        """
+        with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}):
+            self.assertEqual('test_user', self.table_entry.username)
+            self.user.delete()
+            # Go through the post-save update, which will delete users who no longer exist.
+            self.table_entry.state = 'g'
+            self.creator_admin.save_model(self.request, self.table_entry, None, True)
+            self.assertEqual(None, self.table_entry.username)

cms/djangoapps/course_creators/tests/test_views.py

+"""
+Tests course_creators.views.py.
+"""
+
+from django.test import TestCase
+from django.contrib.auth.models import User
+from django.core.exceptions import PermissionDenied
+
+from course_creators.views import add_user_with_status_unrequested, add_user_with_status_granted
+from course_creators.views import get_course_creator_status
+from course_creators.models import CourseCreator
+from auth.authz import is_user_in_creator_group
+import mock
+
+
+class CourseCreatorView(TestCase):
+    """
+    Tests for modifying the course creator table.
+    """
+
+    def setUp(self):
+        """ Test case setup """
+        self.user = User.objects.create_user('test_user', 'test_user+courses@edx.org', 'foo')
+        self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo')
+        self.admin.is_staff = True
+
+    def test_staff_permission_required(self):
+        """
+        Tests that add methods must be called with staff permissions.
+        """
+        with self.assertRaises(PermissionDenied):
+            add_user_with_status_granted(self.user, self.user)
+
+        with self.assertRaises(PermissionDenied):
+            add_user_with_status_unrequested(self.user, self.user)
+
+    def test_table_initially_empty(self):
+        with self.assertRaises(AssertionError):
+            get_course_creator_status(self.user)
+
+    def test_add_unrequested(self):
+        add_user_with_status_unrequested(self.admin, self.user)
+        self.assertEqual('u', get_course_creator_status(self.user))
+
+        # Calling add again will be a no-op (even if state is different).
+        add_user_with_status_granted(self.admin, self.user)
+        self.assertEqual('u', get_course_creator_status(self.user))
+
+    def test_add_granted(self):
+        with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}):
+            # Calling add_user_with_status_granted impacts is_user_in_course_group_role.
+            self.assertFalse(is_user_in_creator_group(self.user))
+
+            add_user_with_status_granted(self.admin, self.user)
+            self.assertEqual('g', get_course_creator_status(self.user))
+
+            # Calling add again will be a no-op (even if state is different).
+            add_user_with_status_unrequested(self.admin, self.user)
+            self.assertEqual('g', get_course_creator_status(self.user))
+
+            self.assertTrue(is_user_in_creator_group(self.user))
+
+    def test_delete_bad_user(self):
+        """
+        Tests that users who no longer exist are deleted from the table.
+        """
+        add_user_with_status_unrequested(self.admin, self.user)
+        self.user.delete()
+        # Ensure that the post-init callback runs (removes the entry from the table).
+        users = CourseCreator.objects.filter(username=self.user.username)
+        if users.count() == 1:
+            users[0].__init__()
+        with self.assertRaises(AssertionError):
+            get_course_creator_status(self.user)