From 28115af48887eecc535228bba0c3e5add216b8b8 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:08:06 -0400 Subject: [PATCH] The course creator view adds granted users course creator status. Includes unit tests and modifications to authz.py. --- cms/djangoapps/auth/authz.py | 14 +--- cms/djangoapps/auth/tests/test_authz.py | 52 ++++--------- .../management/commands/populate_creators.py | 9 +-- cms/djangoapps/course_creators/models.py | 46 ++++++----- .../course_creators/tests/test_admin.py | 78 +++++++++++++++++++ .../course_creators/tests/test_views.py | 74 ++++++++++++++++++ 6 files changed, 199 insertions(+), 74 deletions(-) create mode 100644 cms/djangoapps/course_creators/tests/test_admin.py create mode 100644 cms/djangoapps/course_creators/tests/test_views.py diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index d5179da05e..0f2e60dd6e 100644 --- a/cms/djangoapps/auth/authz.py +++ b/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(): diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index dd04a7d41d..b6baa3d728 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/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) -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] + 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) - 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)) + self.assertSetEqual({self.creator, creator2}, get_users_with_instructor_role()) - 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)) diff --git a/cms/djangoapps/contentstore/management/commands/populate_creators.py b/cms/djangoapps/contentstore/management/commands/populate_creators.py index e20d418abb..90d8b3c668 100644 --- a/cms/djangoapps/contentstore/management/commands/populate_creators.py +++ b/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 diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 48e56c63d6..6b07b88222 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/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() diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py new file mode 100644 index 0000000000..94886fbdb3 --- /dev/null +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -0,0 +1,78 @@ +""" +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) diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py new file mode 100644 index 0000000000..16c8299649 --- /dev/null +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -0,0 +1,74 @@ +""" +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)