diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 7645b21575..b370ac423a 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -54,20 +54,23 @@ def post_init_callback(sender, **kwargs): @receiver(post_save, sender=CourseCreator) def post_save_callback(sender, **kwargs): """ - Extend to update state_changed time and modify the course creator group in authz.py. + Extend to update state_changed time and fire event to update course creator group, if appropriate. """ 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: - if hasattr(instance, 'admin'): + # If either old or new state is 'granted', we must manipulate the course creator + # group maintained by authz. That requires staff permissions (stored admin). + if instance.state == CourseCreator.GRANTED or instance.orig_state == CourseCreator.GRANTED: + assert hasattr(instance, 'admin'), 'Must have stored staff user to change course creator group' update_creator_state.send( sender=sender, caller=instance.admin, user=instance.user, add=instance.state == CourseCreator.GRANTED ) - # TODO: Else must be sure that state change does not switch to or from granted + instance.state_changed = timezone.now() instance.orig_state = instance.state instance.save() diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index bd91208b9c..943120f3c0 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -7,7 +7,7 @@ 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, update_course_creator_group +from course_creators.views import get_course_creator_status, update_course_creator_group, user_requested_access from course_creators.models import CourseCreator from auth.authz import is_user_in_creator_group import mock @@ -26,14 +26,11 @@ class CourseCreatorView(TestCase): def test_staff_permission_required(self): """ - Tests that add methods and course creator group method must be called with staff permissions. + Tests that any method changing the course creator authz group 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) - with self.assertRaises(PermissionDenied): update_course_creator_group(self.user, self.user, True) @@ -41,7 +38,7 @@ class CourseCreatorView(TestCase): self.assertIsNone(get_course_creator_status(self.user)) def test_add_unrequested(self): - add_user_with_status_unrequested(self.admin, self.user) + add_user_with_status_unrequested(self.user) self.assertEqual('unrequested', get_course_creator_status(self.user)) # Calling add again will be a no-op (even if state is different). @@ -57,7 +54,7 @@ class CourseCreatorView(TestCase): self.assertEqual('granted', 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) + add_user_with_status_unrequested(self.user) self.assertEqual('granted', get_course_creator_status(self.user)) self.assertTrue(is_user_in_creator_group(self.user)) @@ -69,3 +66,17 @@ class CourseCreatorView(TestCase): self.assertTrue(is_user_in_creator_group(self.user)) update_course_creator_group(self.admin, self.user, False) self.assertFalse(is_user_in_creator_group(self.user)) + + def test_user_requested_access(self): + add_user_with_status_unrequested(self.user) + self.assertEqual('unrequested', get_course_creator_status(self.user)) + user_requested_access(self.user) + self.assertEqual('pending', get_course_creator_status(self.user)) + + def test_user_requested_already_granted(self): + add_user_with_status_granted(self.admin, self.user) + self.assertEqual('granted', get_course_creator_status(self.user)) + # Will not "downgrade" to pending because that would require removing the + # user from the authz course creator group (and that can only be done by an admin). + user_requested_access(self.user) + self.assertEqual('granted', get_course_creator_status(self.user)) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 12993d1be4..6b2d91acb2 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -20,10 +20,11 @@ def add_user_with_status_granted(caller, user): """ Adds a user to the course creator table with status 'granted'. - If the user is already in the table, this method is a no-op - (state will not be changed). Caller must have staff permissions. - This method also adds the user to the course creator group maintained by authz.py. + Caller must have staff permissions. + + If the user is already in the table, this method is a no-op + (state will not be changed). """ _add_user(user, CourseCreator.GRANTED) update_course_creator_group(caller, user, True) @@ -64,11 +65,13 @@ def user_requested_access(user): """ User has requested course creator access. - This changes the user state to CourseCreator.PENDING. + This changes the user state to CourseCreator.PENDING, unless the user + state is already CourseCreator.GRANTED, in which case this method is a no-op. """ user = CourseCreator.objects.get(user=user) - user.state = CourseCreator.PENDING - user.save() + if user.state != CourseCreator.GRANTED: + user.state = CourseCreator.PENDING + user.save() def _add_user(user, state):