From 72999e8417e22acee431457b5858c375238a7a38 Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 28 Jun 2013 12:19:51 -0400 Subject: [PATCH 01/21] Initial model. --- cms/djangoapps/course_creators/__init__.py | 0 .../migrations/0001_initial.py | 36 +++++++++++++++++++ .../course_creators/migrations/__init__.py | 0 cms/djangoapps/course_creators/models.py | 29 +++++++++++++++ cms/envs/common.py | 1 + 5 files changed, 66 insertions(+) create mode 100644 cms/djangoapps/course_creators/__init__.py create mode 100644 cms/djangoapps/course_creators/migrations/0001_initial.py create mode 100644 cms/djangoapps/course_creators/migrations/__init__.py create mode 100644 cms/djangoapps/course_creators/models.py diff --git a/cms/djangoapps/course_creators/__init__.py b/cms/djangoapps/course_creators/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cms/djangoapps/course_creators/migrations/0001_initial.py b/cms/djangoapps/course_creators/migrations/0001_initial.py new file mode 100644 index 0000000000..a5668bbe3c --- /dev/null +++ b/cms/djangoapps/course_creators/migrations/0001_initial.py @@ -0,0 +1,36 @@ +# -*- coding: utf-8 -*- +import datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'CourseCreators' + db.create_table('course_creators_coursecreators', ( + ('username', self.gf('django.db.models.fields.CharField')(unique=True, max_length=64, primary_key=True)), + ('state_changed', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), + ('state', self.gf('django.db.models.fields.CharField')(default='u', max_length=1)), + ('note', self.gf('django.db.models.fields.CharField')(max_length=512, blank=True)), + )) + db.send_create_signal('course_creators', ['CourseCreators']) + + + def backwards(self, orm): + # Deleting model 'CourseCreators' + db.delete_table('course_creators_coursecreators') + + + models = { + 'course_creators.coursecreators': { + 'Meta': {'object_name': 'CourseCreators'}, + 'note': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), + 'state': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}), + 'state_changed': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '64', 'primary_key': 'True'}) + } + } + + complete_apps = ['course_creators'] \ No newline at end of file diff --git a/cms/djangoapps/course_creators/migrations/__init__.py b/cms/djangoapps/course_creators/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py new file mode 100644 index 0000000000..b9814a6033 --- /dev/null +++ b/cms/djangoapps/course_creators/models.py @@ -0,0 +1,29 @@ +""" +Table for storing information about whether or not Studio users have course creation privileges. +""" +from django.db import models + + +class CourseCreators(models.Model): + """ + Creates the database table model. + """ + 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) + state_changed = models.DateTimeField('state last updated', auto_now_add=True, + help_text='The date when state was last updated') + state = models.CharField(max_length=1, blank=False, choices=STATES, default='u', + help_text='Current course creator state') + 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" % (self.username, self.state, self.state_changed, self.note) + return s + diff --git a/cms/envs/common.py b/cms/envs/common.py index 87c130a4b5..73d19f5674 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -331,6 +331,7 @@ INSTALLED_APPS = ( # For CMS 'contentstore', 'auth', + 'course_creators', 'student', # misleading name due to sharing with lms 'course_groups', # not used in cms (yet), but tests run From 2970f242456a39b90b4d8616fc68af2f3bb1ca97 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 3 Jul 2013 09:24:02 -0400 Subject: [PATCH 02/21] Initial admin table view. --- cms/djangoapps/auth/authz.py | 4 +++ cms/djangoapps/course_creators/admin.py | 22 ++++++++++++++ .../migrations/0001_initial.py | 14 ++++----- cms/djangoapps/course_creators/models.py | 30 ++++++++++++++++++- cms/djangoapps/course_creators/views.py | 20 +++++++++++++ cms/envs/common.py | 3 ++ cms/urls.py | 8 +++-- 7 files changed, 91 insertions(+), 10 deletions(-) create mode 100644 cms/djangoapps/course_creators/admin.py create mode 100644 cms/djangoapps/course_creators/views.py diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 23dde88e94..77e4d5037d 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -3,6 +3,7 @@ from django.core.exceptions import PermissionDenied from django.conf import settings from xmodule.modulestore import Location +from course_creators import views ''' This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story @@ -221,3 +222,6 @@ def _grant_instructors_creator_access(caller): if group.name.startswith(INSTRUCTOR_ROLE_NAME + "_"): for user in group.user_set.all(): add_user_to_creator_group(caller, user) + views.add_user_with_status_granted(user) + elif group.name.startswith(STAFF_ROLE_NAME + "_"): + views.add_user_with_status_unrequested(user) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py new file mode 100644 index 0000000000..6cea8c6235 --- /dev/null +++ b/cms/djangoapps/course_creators/admin.py @@ -0,0 +1,22 @@ +""" +django admin page for the course creators table +""" + +from course_creators.models import CourseCreator +from django.contrib import admin + + +class CourseCreatorAdmin(admin.ModelAdmin): + list_display = ('username', 'state', 'state_changed', 'note') + + def has_add_permission(self, request): + return False + + def has_delete_permission(self, request, obj=None): + return False + + def has_change_permission(self, request, obj=None): + return request.user.is_staff + + +admin.site.register(CourseCreator, CourseCreatorAdmin) diff --git a/cms/djangoapps/course_creators/migrations/0001_initial.py b/cms/djangoapps/course_creators/migrations/0001_initial.py index a5668bbe3c..7e91e08261 100644 --- a/cms/djangoapps/course_creators/migrations/0001_initial.py +++ b/cms/djangoapps/course_creators/migrations/0001_initial.py @@ -8,24 +8,24 @@ from django.db import models class Migration(SchemaMigration): def forwards(self, orm): - # Adding model 'CourseCreators' - db.create_table('course_creators_coursecreators', ( + # Adding model 'CourseCreator' + db.create_table('course_creators_coursecreator', ( ('username', self.gf('django.db.models.fields.CharField')(unique=True, max_length=64, primary_key=True)), ('state_changed', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), ('state', self.gf('django.db.models.fields.CharField')(default='u', max_length=1)), ('note', self.gf('django.db.models.fields.CharField')(max_length=512, blank=True)), )) - db.send_create_signal('course_creators', ['CourseCreators']) + db.send_create_signal('course_creators', ['CourseCreator']) def backwards(self, orm): - # Deleting model 'CourseCreators' - db.delete_table('course_creators_coursecreators') + # Deleting model 'CourseCreator' + db.delete_table('course_creators_coursecreator') models = { - 'course_creators.coursecreators': { - 'Meta': {'object_name': 'CourseCreators'}, + 'course_creators.coursecreator': { + 'Meta': {'object_name': 'CourseCreator'}, 'note': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), 'state': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}), 'state_changed': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index b9814a6033..c8159b66b1 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -2,9 +2,14 @@ Table for storing information about whether or not Studio users have course creation privileges. """ from django.db import models +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 + +import datetime -class CourseCreators(models.Model): +class CourseCreator(models.Model): """ Creates the database table model. """ @@ -23,7 +28,30 @@ class CourseCreators(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" % (self.username, self.state, self.state_changed, self.note) return s + +@receiver(post_init, sender=CourseCreator) +def post_init_callback(sender, **kwargs): + instance = kwargs['instance'] + instance.orig_state = instance.state + +@receiver(post_save, sender=CourseCreator) +def post_save_callback(sender, **kwargs): + 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() + if instance.state == 'g': + # add to course group + instance.state = 'g' + else: + # remove from course group + instance.state = instance.state + + instance.orig_state = instance.state + instance.save() diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py new file mode 100644 index 0000000000..acdd50a0ca --- /dev/null +++ b/cms/djangoapps/course_creators/views.py @@ -0,0 +1,20 @@ +from course_creators.models import CourseCreator + + +def add_user_with_status_unrequested(user): + """ + Adds a user to the course creator table with status 'unrequested'. + """ + _add_user(user, 'u') + + +def add_user_with_status_granted(user): + """ + Adds a user to the course creator table with status 'granted'. + """ + _add_user(user, 'g') + +def _add_user(user, state): + if CourseCreator.objects.filter(username=user.username).count() == 0: + entry = CourseCreator(username=user.username, state=state) + entry.save() \ No newline at end of file diff --git a/cms/envs/common.py b/cms/envs/common.py index 73d19f5674..c8840e9634 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -346,6 +346,9 @@ INSTALLED_APPS = ( # comment common 'django_comment_common', + + # for course creator table + 'django.contrib.admin' ) ################# EDX MARKETING SITE ################################## diff --git a/cms/urls.py b/cms/urls.py index d04c311161..6a061c3a03 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -6,8 +6,8 @@ from django.conf.urls import patterns, include, url from . import one_time_startup # Uncomment the next two lines to enable the admin: -# from django.contrib import admin -# admin.autodiscover() +from django.contrib import admin +admin.autodiscover() urlpatterns = ('', # nopep8 url(r'^$', 'contentstore.views.howitworks', name='homepage'), @@ -146,6 +146,10 @@ if settings.MITX_FEATURES.get('ENABLE_SERVICE_STATUS'): url(r'^status/', include('service_status.urls')), ) +urlpatterns += ( + url(r'^admin/', include(admin.site.urls)), + ) + urlpatterns = patterns(*urlpatterns) # Custom error pages From f1f6acb5c70a7e08baac3783cd8e272cb7762bff Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 3 Jul 2013 17:07:06 -0400 Subject: [PATCH 03/21] Updates to admin table view. Also hooks up the granted and deny events to the course creator group. --- cms/djangoapps/auth/authz.py | 30 ++++++++++++++----- cms/djangoapps/auth/tests/test_authz.py | 18 +++++++++-- .../management/commands/populate_creators.py | 18 +++++++++-- cms/djangoapps/course_creators/admin.py | 22 +++++++++++++- .../migrations/0001_initial.py | 2 ++ cms/djangoapps/course_creators/models.py | 22 +++++++++----- cms/djangoapps/course_creators/views.py | 11 +++++-- 7 files changed, 102 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index 77e4d5037d..d5179da05e 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -3,7 +3,6 @@ from django.core.exceptions import PermissionDenied from django.conf import settings from xmodule.modulestore import Location -from course_creators import views ''' This code is somewhat duplicative of access.py in the LMS. We will unify the code as a separate story @@ -215,13 +214,30 @@ def _grant_instructors_creator_access(caller): 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. + 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. """ + instructors = _get_users_with_role(INSTRUCTOR_ROLE_NAME) + for user in instructors: + add_user_to_creator_group(caller, user) + return instructors + + +def get_users_with_staff_role(): + """ + Returns all users with the role 'staff' + """ + return _get_users_with_role(STAFF_ROLE_NAME) + + +def _get_users_with_role(role): + """ + Returns all users with the specified role. + """ + users = set() for group in Group.objects.all(): - if group.name.startswith(INSTRUCTOR_ROLE_NAME + "_"): + if group.name.startswith(role + "_"): for user in group.user_set.all(): - add_user_to_creator_group(caller, user) - views.add_user_with_status_granted(user) - elif group.name.startswith(STAFF_ROLE_NAME + "_"): - views.add_user_with_status_unrequested(user) + users.add(user) + return users diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index 658c176498..dd04a7d41d 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -9,7 +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 + is_user_in_course_group_role, remove_user_from_course_group, _grant_instructors_creator_access,\ + get_users_with_staff_role class CreatorGroupTest(TestCase): @@ -175,6 +176,18 @@ class CourseGroupTest(TestCase): with self.assertRaises(PermissionDenied): remove_user_from_course_group(self.staff, self.staff, self.location, STAFF_ROLE_NAME) + def test_get_staff(self): + # Do this test with staff 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' + staff2 = User.objects.create_user('teststaff2', 'teststaff+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()) + class GrantInstructorsCreatorAccessTest(TestCase): """ @@ -206,7 +219,8 @@ class GrantInstructorsCreatorAccessTest(TestCase): admin = User.objects.create_user('populate_creators_command', 'grant+creator+access@edx.org', 'foo') admin.is_staff = True - _grant_instructors_creator_access(admin) + instructors = _grant_instructors_creator_access(admin) + self.assertSetEqual({creator1, creator2}, instructors) # Now instructors only are creators. self.assertTrue(is_user_in_creator_group(creator1)) diff --git a/cms/djangoapps/contentstore/management/commands/populate_creators.py b/cms/djangoapps/contentstore/management/commands/populate_creators.py index f627df88f5..e20d418abb 100644 --- a/cms/djangoapps/contentstore/management/commands/populate_creators.py +++ b/cms/djangoapps/contentstore/management/commands/populate_creators.py @@ -3,7 +3,8 @@ 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 +from auth.authz import _grant_instructors_creator_access, 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 from django.contrib.auth.models import User @@ -31,5 +32,18 @@ class Command(BaseCommand): # the admin user will already exist. admin = User.objects.get(username=username, email=email) - _grant_instructors_creator_access(admin) + creators = _grant_instructors_creator_access(admin) + for user in creators: + add_user_with_status_granted(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) + + # 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 + # when they first go to their dashboard. + admin.delete() diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 6cea8c6235..e9bc6b33d2 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -7,7 +7,22 @@ from django.contrib import admin class CourseCreatorAdmin(admin.ModelAdmin): - list_display = ('username', 'state', 'state_changed', 'note') + # Fields to display on the overview page. + list_display = ('username', 'email', 'state', 'state_changed') + readonly_fields = ('username', 'email', 'state_changed') + # Controls the order on the edit form (without this, read-only fields appear at the end). + fieldsets = ( + (None, { + 'fields': list_display + }), + ) + # Fields that filtering support + list_filter = list_display + # Fields that search supports. Note that the search term for state has to be + # its key (ie, 'g' instead of 'granted'). + search_fields = ['username', 'email', 'state'] + # Turn off the action bar (we have no bulk actions) + actions = None def has_add_permission(self, request): return False @@ -18,5 +33,10 @@ class CourseCreatorAdmin(admin.ModelAdmin): def has_change_permission(self, request, obj=None): return request.user.is_staff + def save_model(self, request, obj, form, change): + # Store who is making the request. + obj.admin = request.user + obj.save() + admin.site.register(CourseCreator, CourseCreatorAdmin) diff --git a/cms/djangoapps/course_creators/migrations/0001_initial.py b/cms/djangoapps/course_creators/migrations/0001_initial.py index 7e91e08261..dea4a160c7 100644 --- a/cms/djangoapps/course_creators/migrations/0001_initial.py +++ b/cms/djangoapps/course_creators/migrations/0001_initial.py @@ -11,6 +11,7 @@ class Migration(SchemaMigration): # Adding model 'CourseCreator' db.create_table('course_creators_coursecreator', ( ('username', self.gf('django.db.models.fields.CharField')(unique=True, max_length=64, primary_key=True)), + ('email', self.gf('django.db.models.fields.CharField')(max_length=128)), ('state_changed', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), ('state', self.gf('django.db.models.fields.CharField')(default='u', max_length=1)), ('note', self.gf('django.db.models.fields.CharField')(max_length=512, blank=True)), @@ -26,6 +27,7 @@ class Migration(SchemaMigration): models = { 'course_creators.coursecreator': { 'Meta': {'object_name': 'CourseCreator'}, + 'email': ('django.db.models.fields.CharField', [], {'max_length': '128'}), 'note': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), 'state': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}), 'state_changed': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index c8159b66b1..48e56c63d6 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -4,7 +4,7 @@ Table for storing information about whether or not Studio users have course crea from django.db import models 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 +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group, get_user_by_email import datetime @@ -21,6 +21,7 @@ class CourseCreator(models.Model): ) 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") state_changed = models.DateTimeField('state last updated', auto_now_add=True, help_text='The date when state was last updated') state = models.CharField(max_length=1, blank=False, choices=STATES, default='u', @@ -30,14 +31,20 @@ class CourseCreator(models.Model): def __unicode__(self): - s = "%s | %s [%s] | %s" % (self.username, self.state, self.state_changed, self.note) + s = "%s %s | %s [%s] | %s" % (self.username, self.email, self.state, self.state_changed, self.note) return s @receiver(post_init, sender=CourseCreator) def post_init_callback(sender, **kwargs): instance = kwargs['instance'] - instance.orig_state = instance.state + user = get_user_by_email(instance.email) + if user is None: + # User has been removed, delete from this table. + instance.delete() + else: + instance.orig_state = instance.state + @receiver(post_save, sender=CourseCreator) def post_save_callback(sender, **kwargs): @@ -46,12 +53,13 @@ def post_save_callback(sender, **kwargs): # 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': - # add to course group - instance.state = 'g' + # We have granted access, add to course group + add_user_to_creator_group(instance.admin, user) else: - # remove from course group - instance.state = instance.state + remove_user_from_creator_group(instance.admin, user) instance.orig_state = instance.state instance.save() diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index acdd50a0ca..8d2769d440 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -1,3 +1,6 @@ +""" +Methods for interacting programmatically with the user creator table. +""" from course_creators.models import CourseCreator @@ -14,7 +17,11 @@ def add_user_with_status_granted(user): """ _add_user(user, 'g') + def _add_user(user, state): + """ + Adds a user to the course creator table with the specified state. + """ if CourseCreator.objects.filter(username=user.username).count() == 0: - entry = CourseCreator(username=user.username, state=state) - entry.save() \ No newline at end of file + entry = CourseCreator(username=user.username, email=user.email, state=state) + entry.save() From 293ee0b0ae390e385c9149044128d484009e0a1b Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 12:09:53 -0400 Subject: [PATCH 04/21] Update comment. --- cms/urls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/urls.py b/cms/urls.py index 6a061c3a03..3121dbea9b 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -5,7 +5,7 @@ from django.conf.urls import patterns, include, url # pylint: disable=W0611 from . import one_time_startup -# Uncomment the next two lines to enable the admin: +# There is a course creators admin table. from django.contrib import admin admin.autodiscover() From 2cceda948c97e3c69eb01ab6cd8dd7fa55cd0c94 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 13:12:44 -0400 Subject: [PATCH 05/21] Disable test point when run under CMS. The template exists when django admin is enabled, but the mappings in CMS' urls.py do not exist. --- cms/envs/test.py | 3 +++ common/djangoapps/student/tests/tests.py | 11 +---------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/cms/envs/test.py b/cms/envs/test.py index 86925caff6..6479c938e9 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -143,3 +143,6 @@ MITX_FEATURES['ENABLE_SERVICE_STATUS'] = True # Enabling SQL tracking logs for testing on common/djangoapps/track MITX_FEATURES['ENABLE_SQL_TRACKING_LOGS'] = True + +# This is to disable a test under the common directory that will not pass when run under CMS +MITX_FEATURES['DISABLE_PASSWORD_RESET_EMAIL_TEST'] = True diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 844ddb536e..3e36016316 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -9,15 +9,12 @@ import json import re import unittest -from django import forms from django.conf import settings from django.test import TestCase from django.test.client import RequestFactory from django.contrib.auth.models import User from django.contrib.auth.hashers import UNUSABLE_PASSWORD from django.contrib.auth.tokens import default_token_generator -from django.template.loader import render_to_string, get_template, TemplateDoesNotExist -from django.core.urlresolvers import is_valid_path from django.utils.http import int_to_base36 @@ -33,12 +30,6 @@ COURSE_2 = 'edx/full/6.002_Spring_2012' log = logging.getLogger(__name__) -try: - get_template('registration/password_reset_email.html') - project_uses_password_reset = True -except TemplateDoesNotExist: - project_uses_password_reset = False - class ResetPasswordTests(TestCase): """ Tests that clicking reset password sends email, and doesn't activate the user @@ -75,7 +66,7 @@ class ResetPasswordTests(TestCase): self.assertEquals(bad_email_resp.content, json.dumps({'success': False, 'error': 'Invalid e-mail or user'})) - @unittest.skipUnless(project_uses_password_reset, + @unittest.skipUnless(not settings.MITX_FEATURES.get('DISABLE_PASSWORD_RESET_EMAIL_TEST', False), dedent("""Skipping Test because CMS has not provided necessary templates for password reset. If LMS tests print this message, that needs to be fixed.""")) @patch('django.core.mail.send_mail') From 5e8e3353ca6ec843e8e0bc13ff1d266ad013edf4 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:07:02 -0400 Subject: [PATCH 06/21] Pylint/pep8 cleanup. --- cms/djangoapps/course_creators/admin.py | 4 +++ cms/djangoapps/course_creators/views.py | 37 +++++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index e9bc6b33d2..36e74420fd 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -7,6 +7,10 @@ from django.contrib import admin class CourseCreatorAdmin(admin.ModelAdmin): + """ + Admin for the course creator table. + """ + # Fields to display on the overview page. list_display = ('username', 'email', 'state', 'state_changed') readonly_fields = ('username', 'email', 'state_changed') diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 8d2769d440..550ed9f548 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -2,26 +2,53 @@ Methods for interacting programmatically with the user creator table. """ from course_creators.models import CourseCreator +from django.core.exceptions import PermissionDenied + +from auth.authz import add_user_to_creator_group -def add_user_with_status_unrequested(user): +def add_user_with_status_unrequested(caller, user): """ Adds a user to the course creator table with status 'unrequested'. + + Caller must have staff permissions. """ - _add_user(user, 'u') + _add_user(caller, user, 'u') -def add_user_with_status_granted(user): +def add_user_with_status_granted(caller, user): """ Adds a user to the course creator table with status 'granted'. + + Caller must have staff permissions. This method also adds the user + to the course creator group maintained by authz.py. """ - _add_user(user, 'g') + _add_user(caller, user, 'g') + add_user_to_creator_group(caller, user) -def _add_user(user, state): +def get_course_creator_status(user): + """ + Returns the status for a particular user. + + Possible return values are: + 'g' = 'granted' + 'u' = 'unrequested' + 'p' = 'pending' + 'd' = 'denied' + """ + user = CourseCreator.objects.filter(username=user.username) + assert user.count() == 1, "The user does not exist in the table." + return user[0].state + + +def _add_user(caller, user, state): """ Adds a user to the course creator table with the specified state. """ + if not caller.is_active or not caller.is_authenticated or not caller.is_staff: + raise PermissionDenied + if CourseCreator.objects.filter(username=user.username).count() == 0: entry = CourseCreator(username=user.username, email=user.email, state=state) entry.save() From 28115af48887eecc535228bba0c3e5add216b8b8 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:08:06 -0400 Subject: [PATCH 07/21] 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) From 69a998780a9eff42c3c075fca08fbf24e581cb74 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:10:43 -0400 Subject: [PATCH 08/21] Remove test case moved to test_admin. --- cms/djangoapps/course_creators/tests/test_views.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index 16c8299649..0ea5656e21 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -60,15 +60,3 @@ class CourseCreatorView(TestCase): 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) From 73f4b1c676733a430c5a5b6b424ba060aced5d58 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:43:30 -0400 Subject: [PATCH 09/21] Overloading methods does not work with variable names changed. --- cms/djangoapps/course_creators/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 6b07b88222..7b75af35c8 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -34,7 +34,7 @@ class CourseCreator(models.Model): @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. """ @@ -48,7 +48,7 @@ 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. From 23ffa2a616e5be224b27537c235a81f16b4328fd Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 8 Jul 2013 16:55:32 -0400 Subject: [PATCH 10/21] Show note field, update filterable. --- cms/djangoapps/course_creators/admin.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 36e74420fd..75cde2ebd9 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -12,8 +12,8 @@ class CourseCreatorAdmin(admin.ModelAdmin): """ # Fields to display on the overview page. - list_display = ('username', 'email', 'state', 'state_changed') - readonly_fields = ('username', 'email', 'state_changed') + list_display = ['username', 'email', 'state', 'state_changed', 'note'] + readonly_fields = ['username', 'email', 'state_changed'] # Controls the order on the edit form (without this, read-only fields appear at the end). fieldsets = ( (None, { @@ -21,10 +21,10 @@ class CourseCreatorAdmin(admin.ModelAdmin): }), ) # Fields that filtering support - list_filter = list_display + list_filter = ['state', 'state_changed'] # Fields that search supports. Note that the search term for state has to be # its key (ie, 'g' instead of 'granted'). - search_fields = ['username', 'email', 'state'] + search_fields = ['username', 'email', 'state', 'note'] # Turn off the action bar (we have no bulk actions) actions = None From 0bad62942e66408cb019ad446eaec0538af8e8b1 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 09:36:28 -0400 Subject: [PATCH 11/21] Update migrate and syncdb commands so they run on CMS as well, delete obsolete migrate task. --- rakelib/django.rake | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/rakelib/django.rake b/rakelib/django.rake index b1adf24050..41265beeac 100644 --- a/rakelib/django.rake +++ b/rakelib/django.rake @@ -57,18 +57,20 @@ task :resetdb, [:env] do |t, args| sh(django_admin(:lms, args.env, 'migrate')) end -desc "Update the relational database to the latest migration" -task :migrate, [:env] do |t, args| - args.with_defaults(:env => 'dev') - sh(django_admin(:lms, args.env, 'migrate')) -end - task :runserver => :lms desc "Run django-admin against the specified system and environment" task "django-admin", [:action, :system, :env, :options] do |t, args| + # If no system was explicitly set, we want to run both CMS and LMS for migrate and syncdb. + no_system_set = true + if args.system + no_system_set = false + end args.with_defaults(:env => 'dev', :system => 'lms', :options => '') sh(django_admin(args.system, args.env, args.action, args.options)) + if no_system_set and (args.action == 'migrate' or args.action == 'syncdb') + sh(django_admin('cms', args.env, args.action, args.options)) + end end desc "Set the staff bit for a user" From 5a3a03c0d4d83edfee9b679cb68ac5799387d772 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 10:23:38 -0400 Subject: [PATCH 12/21] Change user to foreign key (tracks deletions). --- cms/djangoapps/course_creators/admin.py | 15 +++++-- .../migrations/0001_initial.py | 44 +++++++++++++++++-- cms/djangoapps/course_creators/models.py | 39 ++++++---------- .../course_creators/tests/test_admin.py | 14 +----- .../course_creators/tests/test_views.py | 3 +- cms/djangoapps/course_creators/views.py | 16 ++++--- 6 files changed, 77 insertions(+), 54 deletions(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 75cde2ebd9..f2d014a2c4 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -6,25 +6,32 @@ from course_creators.models import CourseCreator from django.contrib import admin +def get_email(obj): + """ Returns the email address for a user """ + return obj.user.email + +get_email.short_description = 'email' + + class CourseCreatorAdmin(admin.ModelAdmin): """ Admin for the course creator table. """ # Fields to display on the overview page. - list_display = ['username', 'email', 'state', 'state_changed', 'note'] - readonly_fields = ['username', 'email', 'state_changed'] + list_display = ['user', get_email, 'state', 'state_changed', 'note'] + readonly_fields = ['user', 'state_changed'] # Controls the order on the edit form (without this, read-only fields appear at the end). fieldsets = ( (None, { - 'fields': list_display + 'fields': ['user', 'state', 'state_changed', 'note'] }), ) # Fields that filtering support list_filter = ['state', 'state_changed'] # Fields that search supports. Note that the search term for state has to be # its key (ie, 'g' instead of 'granted'). - search_fields = ['username', 'email', 'state', 'note'] + search_fields = ['user__username', 'user__email', 'state', 'note'] # Turn off the action bar (we have no bulk actions) actions = None diff --git a/cms/djangoapps/course_creators/migrations/0001_initial.py b/cms/djangoapps/course_creators/migrations/0001_initial.py index dea4a160c7..dd36b79fb3 100644 --- a/cms/djangoapps/course_creators/migrations/0001_initial.py +++ b/cms/djangoapps/course_creators/migrations/0001_initial.py @@ -10,8 +10,8 @@ class Migration(SchemaMigration): def forwards(self, orm): # Adding model 'CourseCreator' db.create_table('course_creators_coursecreator', ( - ('username', self.gf('django.db.models.fields.CharField')(unique=True, max_length=64, primary_key=True)), - ('email', self.gf('django.db.models.fields.CharField')(max_length=128)), + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], unique=True)), ('state_changed', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), ('state', self.gf('django.db.models.fields.CharField')(default='u', max_length=1)), ('note', self.gf('django.db.models.fields.CharField')(max_length=512, blank=True)), @@ -25,13 +25,49 @@ class Migration(SchemaMigration): models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + }, 'course_creators.coursecreator': { 'Meta': {'object_name': 'CourseCreator'}, - 'email': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), 'note': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), 'state': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}), 'state_changed': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), - 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '64', 'primary_key': 'True'}) + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'}) } } diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 7b75af35c8..e67c57b1be 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -4,7 +4,8 @@ Table for storing information about whether or not Studio users have course crea from django.db import models 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 +from django.contrib.auth.models import User +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group from django.utils import timezone @@ -19,8 +20,7 @@ class CourseCreator(models.Model): (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") + user = models.ForeignKey(User, help_text="Studio user", unique=True) state_changed = models.DateTimeField('state last updated', auto_now_add=True, help_text='The date when state was last updated') state = models.CharField(max_length=1, blank=False, choices=STATES, default='u', @@ -29,45 +29,34 @@ class CourseCreator(models.Model): 'why course creation access was denied)') def __unicode__(self): - s = "%str %str | %str [%str] | %str" % (self.username, self.email, self.state, self.state_changed, self.note) + s = "%str %str | %str [%str] | %str" % (self.user.username, self.user.email, self.state, self.state_changed, self.note) return s @receiver(post_init, sender=CourseCreator) def post_init_callback(sender, **kwargs): """ - Extend to remove deleted users and store previous state. + Extend to store previous state. """ instance = kwargs['instance'] - user = get_user_by_email(instance.email) - if user is None: - # User has been removed, delete from this table. - instance.delete() - else: - instance.orig_state = instance.state + instance.orig_state = instance.state @receiver(post_save, sender=CourseCreator) def post_save_callback(sender, **kwargs): """ - Extend to remove deleted users, update state_changed time, - and modify the course creator group in authz.py. + Extend to 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: - user = get_user_by_email(instance.email) - if user is None: - # User has been removed, delete from this table. - instance.delete() + instance.state_changed = timezone.now() + if instance.state == 'g': + # We have granted access, add to course group + add_user_to_creator_group(instance.admin, instance.user) else: - 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) + remove_user_from_creator_group(instance.admin, instance.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 index 94886fbdb3..d06b9066c4 100644 --- a/cms/djangoapps/course_creators/tests/test_admin.py +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -21,7 +21,7 @@ class CourseCreatorAdminTest(TestCase): 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 = CourseCreator(user=self.user) self.table_entry.save() self.admin = User.objects.create_user('Mark', 'admin+courses@edx.org', 'foo') @@ -64,15 +64,3 @@ class CourseCreatorAdminTest(TestCase): # 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 index 0ea5656e21..71d3e1523b 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -35,8 +35,7 @@ class CourseCreatorView(TestCase): 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) + self.assertIsNone(get_course_creator_status(self.user)) def test_add_unrequested(self): add_user_with_status_unrequested(self.admin, self.user) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 550ed9f548..4cf1693a97 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -29,17 +29,21 @@ def add_user_with_status_granted(caller, user): def get_course_creator_status(user): """ - Returns the status for a particular user. + Returns the status for a particular user, or None if user is not in the table. Possible return values are: 'g' = 'granted' 'u' = 'unrequested' 'p' = 'pending' 'd' = 'denied' + None = user does not exist in the table """ - user = CourseCreator.objects.filter(username=user.username) - assert user.count() == 1, "The user does not exist in the table." - return user[0].state + user = CourseCreator.objects.filter(user=user) + if user.count() == 0: + return None + else: + # User is defined to be unique, can assume a single entry. + return user[0].state def _add_user(caller, user, state): @@ -49,6 +53,6 @@ def _add_user(caller, user, state): if not caller.is_active or not caller.is_authenticated or not caller.is_staff: raise PermissionDenied - if CourseCreator.objects.filter(username=user.username).count() == 0: - entry = CourseCreator(username=user.username, email=user.email, state=state) + if CourseCreator.objects.filter(user=user).count() == 0: + entry = CourseCreator(user=user, state=state) entry.save() From 5cd90d58f52741fac0cb4167941c5b62e396e4fe Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 10:33:37 -0400 Subject: [PATCH 13/21] Clarify documentation. --- cms/djangoapps/course_creators/views.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 4cf1693a97..9380a9a412 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -11,7 +11,8 @@ def add_user_with_status_unrequested(caller, user): """ Adds a user to the course creator table with status 'unrequested'. - Caller must have staff permissions. + If the user is already in the table, this method is a no-op + (state will not be changed). Caller must have staff permissions. """ _add_user(caller, user, 'u') @@ -20,8 +21,10 @@ def add_user_with_status_granted(caller, user): """ Adds a user to the course creator table with status 'granted'. - Caller must have staff permissions. This method also adds the user - to the course creator group maintained by authz.py. + 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. """ _add_user(caller, user, 'g') add_user_to_creator_group(caller, user) @@ -49,6 +52,9 @@ def get_course_creator_status(user): def _add_user(caller, user, state): """ Adds a user to the course creator table with the specified state. + + If the user is already in the table, this method is a no-op + (state will not be changed). """ if not caller.is_active or not caller.is_authenticated or not caller.is_staff: raise PermissionDenied From 6a49980582c018bcc28c9bc6f1bf108356b6a13b Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 11:02:59 -0400 Subject: [PATCH 14/21] Add tests for permissions. --- .../course_creators/tests/test_admin.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py index d06b9066c4..8a7c6cbb79 100644 --- a/cms/djangoapps/course_creators/tests/test_admin.py +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -64,3 +64,26 @@ class CourseCreatorAdminTest(TestCase): # and change state back to 'u' (unrequested) change_state('u', False) + + def test_add_permission(self): + """ + Tests that staff cannot add entries + """ + self.assertFalse(self.creator_admin.has_add_permission(self.request)) + + + def test_delete_permission(self): + """ + Tests that staff cannot delete entries + """ + self.assertFalse(self.creator_admin.has_delete_permission(self.request)) + + + def test_change_permission(self): + """ + Tests that only staff can change entries + """ + self.assertTrue(self.creator_admin.has_change_permission(self.request)) + + self.request.user = self.user + self.assertFalse(self.creator_admin.has_change_permission(self.request)) From 99584aab23e81dd53c88c7d1141b9a395f9f1b27 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 11:08:27 -0400 Subject: [PATCH 15/21] pep8 cleanup --- cms/djangoapps/auth/tests/test_authz.py | 1 - cms/djangoapps/course_creators/models.py | 13 +++++++------ cms/djangoapps/course_creators/tests/test_views.py | 1 - cms/urls.py | 4 +--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/auth/tests/test_authz.py b/cms/djangoapps/auth/tests/test_authz.py index b6baa3d728..e04c108250 100644 --- a/cms/djangoapps/auth/tests/test_authz.py +++ b/cms/djangoapps/auth/tests/test_authz.py @@ -200,4 +200,3 @@ class CourseGroupTest(TestCase): add_user_to_course_group(creator2, staff2, location2, STAFF_ROLE_NAME) self.assertSetEqual({self.creator, creator2}, get_users_with_instructor_role()) - diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index e67c57b1be..7df1580987 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -14,11 +14,12 @@ 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'), + ) user = models.ForeignKey(User, help_text="Studio user", unique=True) state_changed = models.DateTimeField('state last updated', auto_now_add=True, @@ -29,7 +30,7 @@ class CourseCreator(models.Model): 'why course creation access was denied)') def __unicode__(self): - s = "%str %str | %str [%str] | %str" % (self.user.username, self.user.email, self.state, self.state_changed, self.note) + s = "%str | %str [%str] | %str" % (self.user, self.state, self.state_changed, self.note) return s diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index 71d3e1523b..b3495c4d76 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -58,4 +58,3 @@ class CourseCreatorView(TestCase): self.assertEqual('g', get_course_creator_status(self.user)) self.assertTrue(is_user_in_creator_group(self.user)) - diff --git a/cms/urls.py b/cms/urls.py index 3121dbea9b..1bd9850d4b 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -146,9 +146,7 @@ if settings.MITX_FEATURES.get('ENABLE_SERVICE_STATUS'): url(r'^status/', include('service_status.urls')), ) -urlpatterns += ( - url(r'^admin/', include(admin.site.urls)), - ) +urlpatterns += (url(r'^admin/', include(admin.site.urls)),) urlpatterns = patterns(*urlpatterns) From 01557b26be1168bc31a04bfe610df809a5d267a0 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 12:47:54 -0400 Subject: [PATCH 16/21] Move call into authz methods out of model class. --- cms/djangoapps/course_creators/admin.py | 13 ++++++++++++- cms/djangoapps/course_creators/models.py | 17 +++++++++-------- .../course_creators/tests/test_views.py | 15 +++++++++++++-- cms/djangoapps/course_creators/views.py | 16 ++++++++++++++-- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index f2d014a2c4..097e4f95a2 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -2,8 +2,11 @@ django admin page for the course creators table """ -from course_creators.models import CourseCreator +from course_creators.models import CourseCreator, update_creator_state +from course_creators.views import update_course_creator_group + from django.contrib import admin +from django.dispatch import receiver def get_email(obj): @@ -51,3 +54,11 @@ class CourseCreatorAdmin(admin.ModelAdmin): admin.site.register(CourseCreator, CourseCreatorAdmin) + + +@receiver(update_creator_state, sender=CourseCreator) +def update_creator_group_callback(sender, **kwargs): + """ + Callback for when the model's creator status has changed. + """ + update_course_creator_group(kwargs['caller'], kwargs['user'], kwargs['add']) diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index 7df1580987..d0aaa4bec1 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -3,12 +3,13 @@ Table for storing information about whether or not Studio users have course crea """ from django.db import models from django.db.models.signals import post_init, post_save -from django.dispatch import receiver +from django.dispatch import receiver, Signal from django.contrib.auth.models import User -from auth.authz import add_user_to_creator_group, remove_user_from_creator_group from django.utils import timezone +# A signal that will be sent when users should be added or removed from the creator group +update_creator_state = Signal(providing_args=["caller", "user", "add"]) class CourseCreator(models.Model): """ @@ -52,12 +53,12 @@ def post_save_callback(sender, **kwargs): # 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: + update_creator_state.send( + sender=sender, + caller=instance.admin, + user=instance.user, + add=instance.state == 'g' + ) instance.state_changed = timezone.now() - if instance.state == 'g': - # We have granted access, add to course group - add_user_to_creator_group(instance.admin, instance.user) - else: - remove_user_from_creator_group(instance.admin, instance.user) - 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 b3495c4d76..ad37208d23 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 +from course_creators.views import get_course_creator_status, update_course_creator_group from course_creators.models import CourseCreator from auth.authz import is_user_in_creator_group import mock @@ -26,7 +26,7 @@ class CourseCreatorView(TestCase): def test_staff_permission_required(self): """ - Tests that add methods must be called with staff permissions. + Tests that add methods and course creator group method must be called with staff permissions. """ with self.assertRaises(PermissionDenied): add_user_with_status_granted(self.user, self.user) @@ -34,6 +34,9 @@ class CourseCreatorView(TestCase): 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) + def test_table_initially_empty(self): self.assertIsNone(get_course_creator_status(self.user)) @@ -58,3 +61,11 @@ class CourseCreatorView(TestCase): self.assertEqual('g', get_course_creator_status(self.user)) self.assertTrue(is_user_in_creator_group(self.user)) + + def test_update_creator_group(self): + with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): + self.assertFalse(is_user_in_creator_group(self.user)) + update_course_creator_group(self.admin, self.user, True) + 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)) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 9380a9a412..4fc273cf33 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -4,7 +4,7 @@ Methods for interacting programmatically with the user creator table. from course_creators.models import CourseCreator from django.core.exceptions import PermissionDenied -from auth.authz import add_user_to_creator_group +from auth.authz import add_user_to_creator_group, remove_user_from_creator_group def add_user_with_status_unrequested(caller, user): @@ -27,7 +27,19 @@ def add_user_with_status_granted(caller, user): This method also adds the user to the course creator group maintained by authz.py. """ _add_user(caller, user, 'g') - add_user_to_creator_group(caller, user) + update_course_creator_group(caller, user, True) + + +def update_course_creator_group(caller, user, add): + """ + Method for adding and removing users from the creator group. + + Caller must have staff permissions. + """ + if add: + add_user_to_creator_group(caller, user) + else: + remove_user_from_creator_group(caller, user) def get_course_creator_status(user): From 283e2b2e6085b1d735dde663fa2df38bf5d068d4 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 13:07:38 -0400 Subject: [PATCH 17/21] Don't store abbreviations for state. --- cms/djangoapps/course_creators/admin.py | 3 +- .../migrations/0001_initial.py | 4 +-- cms/djangoapps/course_creators/models.py | 31 ++++++++++++------- .../course_creators/tests/test_admin.py | 18 ++++------- .../course_creators/tests/test_views.py | 8 ++--- cms/djangoapps/course_creators/views.py | 12 +++---- 6 files changed, 38 insertions(+), 38 deletions(-) diff --git a/cms/djangoapps/course_creators/admin.py b/cms/djangoapps/course_creators/admin.py index 097e4f95a2..7518946270 100644 --- a/cms/djangoapps/course_creators/admin.py +++ b/cms/djangoapps/course_creators/admin.py @@ -32,8 +32,7 @@ class CourseCreatorAdmin(admin.ModelAdmin): ) # Fields that filtering support list_filter = ['state', 'state_changed'] - # Fields that search supports. Note that the search term for state has to be - # its key (ie, 'g' instead of 'granted'). + # Fields that search supports. search_fields = ['user__username', 'user__email', 'state', 'note'] # Turn off the action bar (we have no bulk actions) actions = None diff --git a/cms/djangoapps/course_creators/migrations/0001_initial.py b/cms/djangoapps/course_creators/migrations/0001_initial.py index dd36b79fb3..ec7fd9ad9d 100644 --- a/cms/djangoapps/course_creators/migrations/0001_initial.py +++ b/cms/djangoapps/course_creators/migrations/0001_initial.py @@ -13,7 +13,7 @@ class Migration(SchemaMigration): ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), ('user', self.gf('django.db.models.fields.related.ForeignKey')(to=orm['auth.User'], unique=True)), ('state_changed', self.gf('django.db.models.fields.DateTimeField')(auto_now_add=True, blank=True)), - ('state', self.gf('django.db.models.fields.CharField')(default='u', max_length=1)), + ('state', self.gf('django.db.models.fields.CharField')(default='unrequested', max_length=24)), ('note', self.gf('django.db.models.fields.CharField')(max_length=512, blank=True)), )) db.send_create_signal('course_creators', ['CourseCreator']) @@ -65,7 +65,7 @@ class Migration(SchemaMigration): 'Meta': {'object_name': 'CourseCreator'}, 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), 'note': ('django.db.models.fields.CharField', [], {'max_length': '512', 'blank': 'True'}), - 'state': ('django.db.models.fields.CharField', [], {'default': "'u'", 'max_length': '1'}), + 'state': ('django.db.models.fields.CharField', [], {'default': "'unrequested'", 'max_length': '24'}), 'state_changed': ('django.db.models.fields.DateTimeField', [], {'auto_now_add': 'True', 'blank': 'True'}), 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']", 'unique': 'True'}) } diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index d0aaa4bec1..aedf761cee 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -7,6 +7,7 @@ from django.dispatch import receiver, Signal from django.contrib.auth.models import User from django.utils import timezone +from django.utils.translation import ugettext as _ # A signal that will be sent when users should be added or removed from the creator group update_creator_state = Signal(providing_args=["caller", "user", "add"]) @@ -15,23 +16,29 @@ class CourseCreator(models.Model): """ Creates the database table model. """ + UNREQUESTED = 'unrequested' + PENDING = 'pending' + GRANTED = 'granted' + DENIED = 'denied' + + # Second value is the "human-readable" version. STATES = ( - (u'u', u'unrequested'), - (u'p', u'pending'), - (u'g', u'granted'), - (u'd', u'denied'), + (UNREQUESTED, _(u'unrequested')), + (PENDING, _(u'pending')), + (GRANTED, _(u'granted')), + (DENIED, _(u'denied')), ) - user = models.ForeignKey(User, help_text="Studio user", unique=True) + user = models.ForeignKey(User, help_text=_("Studio user"), unique=True) state_changed = models.DateTimeField('state last updated', auto_now_add=True, - help_text='The date when state was last updated') - state = models.CharField(max_length=1, blank=False, choices=STATES, default='u', - help_text='Current course creator state') - note = models.CharField(max_length=512, blank=True, help_text='Optional notes about this user (for example, ' - 'why course creation access was denied)') + help_text=_("The date when state was last updated")) + state = models.CharField(max_length=24, blank=False, choices=STATES, default=UNREQUESTED, + help_text=_("Current course creator state")) + 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 = "%str | %str [%str] | %str" % (self.user, self.state, self.state_changed, self.note) + s = u'%str | %str [%str] | %str' % (self.user, self.state, self.state_changed, self.note) return s @@ -57,7 +64,7 @@ def post_save_callback(sender, **kwargs): sender=sender, caller=instance.admin, user=instance.user, - add=instance.state == 'g' + add=instance.state == CourseCreator.GRANTED ) instance.state_changed = timezone.now() instance.orig_state = instance.state diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py index 8a7c6cbb79..0d1ec33b6f 100644 --- a/cms/djangoapps/course_creators/tests/test_admin.py +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -46,23 +46,17 @@ class CourseCreatorAdminTest(TestCase): # User is initially unrequested. self.assertFalse(is_user_in_creator_group(self.user)) - # change state to 'g' (granted) - change_state('g', True) + change_state(CourseCreator.GRANTED, True) - # change state to 'd' (denied) - change_state('d', False) + change_state(CourseCreator.DENIED, False) - # and change state back to 'g' (granted) - change_state('g', True) + change_state(CourseCreator.GRANTED, True) - # change state to 'p' (pending) - change_state('p', False) + change_state(CourseCreator.PENDING, False) - # and change state back to 'g' (granted) - change_state('g', True) + change_state(CourseCreator.GRANTED, True) - # and change state back to 'u' (unrequested) - change_state('u', False) + change_state(CourseCreator.UNREQUESTED, False) def test_add_permission(self): diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index ad37208d23..bd91208b9c 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -42,11 +42,11 @@ class CourseCreatorView(TestCase): def test_add_unrequested(self): add_user_with_status_unrequested(self.admin, self.user) - self.assertEqual('u', get_course_creator_status(self.user)) + self.assertEqual('unrequested', 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)) + self.assertEqual('unrequested', get_course_creator_status(self.user)) def test_add_granted(self): with mock.patch.dict('django.conf.settings.MITX_FEATURES', {"ENABLE_CREATOR_GROUP": True}): @@ -54,11 +54,11 @@ class CourseCreatorView(TestCase): 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)) + 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) - self.assertEqual('g', get_course_creator_status(self.user)) + self.assertEqual('granted', get_course_creator_status(self.user)) self.assertTrue(is_user_in_creator_group(self.user)) diff --git a/cms/djangoapps/course_creators/views.py b/cms/djangoapps/course_creators/views.py index 4fc273cf33..902406e620 100644 --- a/cms/djangoapps/course_creators/views.py +++ b/cms/djangoapps/course_creators/views.py @@ -14,7 +14,7 @@ def add_user_with_status_unrequested(caller, user): If the user is already in the table, this method is a no-op (state will not be changed). Caller must have staff permissions. """ - _add_user(caller, user, 'u') + _add_user(caller, user, CourseCreator.UNREQUESTED) def add_user_with_status_granted(caller, user): @@ -26,7 +26,7 @@ def add_user_with_status_granted(caller, user): This method also adds the user to the course creator group maintained by authz.py. """ - _add_user(caller, user, 'g') + _add_user(caller, user, CourseCreator.GRANTED) update_course_creator_group(caller, user, True) @@ -47,10 +47,10 @@ def get_course_creator_status(user): Returns the status for a particular user, or None if user is not in the table. Possible return values are: - 'g' = 'granted' - 'u' = 'unrequested' - 'p' = 'pending' - 'd' = 'denied' + 'unrequested' = user has not requested course creation rights + 'pending' = user has requested course creation rights + 'granted' = user has been granted course creation rights + 'denied' = user has been denied course creation rights None = user does not exist in the table """ user = CourseCreator.objects.filter(user=user) From ee2d874fdce3b3df38d779701997c77a823df4ef Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 13:12:15 -0400 Subject: [PATCH 18/21] Remove extra blank lines. --- cms/djangoapps/course_creators/tests/test_admin.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py index 0d1ec33b6f..6ef48746e7 100644 --- a/cms/djangoapps/course_creators/tests/test_admin.py +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -58,21 +58,18 @@ class CourseCreatorAdminTest(TestCase): change_state(CourseCreator.UNREQUESTED, False) - def test_add_permission(self): """ Tests that staff cannot add entries """ self.assertFalse(self.creator_admin.has_add_permission(self.request)) - def test_delete_permission(self): """ Tests that staff cannot delete entries """ self.assertFalse(self.creator_admin.has_delete_permission(self.request)) - def test_change_permission(self): """ Tests that only staff can change entries From d7946efcaf530b334938067fdf283413d913adbb Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 13:15:18 -0400 Subject: [PATCH 19/21] Note about course creator table and rake command change. --- CHANGELOG.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3011821caf..8f45e680d1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studio: Add table for tracking course creator permissions (not yet used). +Update rake django-admin[syncdb] and rake django-admin[migrate] so they +run for both LMS and CMS. + Common: Student information is now passed to the tracking log via POST instead of GET. Common: Add tests for documentation generation to test suite From f73aadbee7df2ce01f52951e97d4fd512e20b1cd Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 14:32:15 -0400 Subject: [PATCH 20/21] Simplify boolean check. --- rakelib/django.rake | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rakelib/django.rake b/rakelib/django.rake index 41265beeac..eeb8135d4d 100644 --- a/rakelib/django.rake +++ b/rakelib/django.rake @@ -62,10 +62,7 @@ task :runserver => :lms desc "Run django-admin against the specified system and environment" task "django-admin", [:action, :system, :env, :options] do |t, args| # If no system was explicitly set, we want to run both CMS and LMS for migrate and syncdb. - no_system_set = true - if args.system - no_system_set = false - end + no_system_set = !args.system args.with_defaults(:env => 'dev', :system => 'lms', :options => '') sh(django_admin(args.system, args.env, args.action, args.options)) if no_system_set and (args.action == 'migrate' or args.action == 'syncdb') From 1fd0451124f92a905b9f3d316aadaa80a4ebca40 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 9 Jul 2013 14:33:27 -0400 Subject: [PATCH 21/21] pylint cleanup --- cms/djangoapps/course_creators/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/course_creators/models.py b/cms/djangoapps/course_creators/models.py index aedf761cee..607dae4af2 100644 --- a/cms/djangoapps/course_creators/models.py +++ b/cms/djangoapps/course_creators/models.py @@ -12,6 +12,7 @@ from django.utils.translation import ugettext as _ # A signal that will be sent when users should be added or removed from the creator group update_creator_state = Signal(providing_args=["caller", "user", "add"]) + class CourseCreator(models.Model): """ Creates the database table model. @@ -38,8 +39,7 @@ class CourseCreator(models.Model): "why course creation access was denied)")) def __unicode__(self): - s = u'%str | %str [%str] | %str' % (self.user, self.state, self.state_changed, self.note) - return s + return u'%str | %str [%str] | %str' % (self.user, self.state, self.state_changed, self.note) @receiver(post_init, sender=CourseCreator)