From 28d9827468beeec9490b6e2c6556a9f91eed77bc Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 27 Dec 2012 10:20:28 -0500 Subject: [PATCH 1/5] fix up some thumbnail issues. Also we need to manage the user permissions when we clone and delete course --- cms/djangoapps/auth/authz.py | 17 +++++++++++++++++ .../contentstore/management/commands/clone.py | 11 +++++++++-- .../management/commands/delete_course.py | 8 ++++++-- .../lib/xmodule/xmodule/contentstore/content.py | 2 +- .../lib/xmodule/xmodule/contentstore/mongo.py | 2 +- .../xmodule/modulestore/store_utilities.py | 5 +++-- rakefile | 1 + 7 files changed, 38 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/auth/authz.py b/cms/djangoapps/auth/authz.py index e3d2e352a1..22bbc4bc1c 100644 --- a/cms/djangoapps/auth/authz.py +++ b/cms/djangoapps/auth/authz.py @@ -71,6 +71,23 @@ def _delete_course_group(location): user.groups.remove(staff) user.save() +''' +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 +''' +def _copy_course_group(source, dest): + instructors = Group.objects.get(name=get_course_groupname_for_role(source, INSTRUCTOR_ROLE_NAME)) + new_instructors_group = Group.objects.get(name=get_course_groupname_for_role(dest, INSTRUCTOR_ROLE_NAME)) + for user in instructors.user_set.all(): + user.groups.add(new_instructors_group) + user.save() + + staff = Group.objects.get(name=get_course_groupname_for_role(source, STAFF_ROLE_NAME)) + new_staff_group = Group.objects.get(name=get_course_groupname_for_role(dest, STAFF_ROLE_NAME)) + for user in staff.user_set.all(): + user.groups.add(new_staff_group) + user.save() + def add_user_to_course_group(caller, user, location, role): # only admins can add/remove other users diff --git a/cms/djangoapps/contentstore/management/commands/clone.py b/cms/djangoapps/contentstore/management/commands/clone.py index 64cf4d4263..0f0f020599 100644 --- a/cms/djangoapps/contentstore/management/commands/clone.py +++ b/cms/djangoapps/contentstore/management/commands/clone.py @@ -8,6 +8,8 @@ from xmodule.contentstore.django import contentstore from xmodule.modulestore import Location from xmodule.course_module import CourseDescriptor +from auth.authz import _copy_course_group + # # To run from command line: rake cms:clone SOURCE_LOC=MITx/111/Foo1 DEST_LOC=MITx/135/Foo3 # @@ -18,14 +20,19 @@ class Command(BaseCommand): def handle(self, *args, **options): if len(args) != 2: - raise CommandError("clone requires two arguments: ") + raise CommandError("clone requires either two or six arguments: ") source_location_str = args[0] dest_location_str = args[1] + ms = modulestore('direct') + cs = contentstore() + print "Cloning course {0} to {1}".format(source_location_str, dest_location_str) source_location = CourseDescriptor.id_to_location(source_location_str) dest_location = CourseDescriptor.id_to_location(dest_location_str) - clone_course(modulestore('direct'), contentstore(), source_location, dest_location) + if clone_course(ms, cs, source_location, dest_location): + print "copying User permissions..." + _copy_course_group(source_location, dest_location) diff --git a/cms/djangoapps/contentstore/management/commands/delete_course.py b/cms/djangoapps/contentstore/management/commands/delete_course.py index 55c04bf5ea..0313f7faed 100644 --- a/cms/djangoapps/contentstore/management/commands/delete_course.py +++ b/cms/djangoapps/contentstore/management/commands/delete_course.py @@ -21,14 +21,18 @@ class Command(BaseCommand): def handle(self, *args, **options): if len(args) != 1: - raise CommandError("delete_course requires one arguments: ") + raise CommandError("delete_course requires one argument: ") loc_str = args[0] + ms = modulestore('direct') + cs = contentstore() + if query_yes_no("Deleting course {0}. Confirm?".format(loc_str), default="no"): if query_yes_no("Are you sure. This action cannot be undone!", default="no"): loc = CourseDescriptor.id_to_location(loc_str) - if delete_course(modulestore('direct'), contentstore(), loc) == True: + if delete_course(ms, cs, loc) == True: + print 'removing User permissions from course....' # in the django layer, we need to remove all the user permissions groups associated with this course _delete_course_group(loc) diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 9badd4b892..b70ab5058f 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -18,7 +18,7 @@ class StaticContent(object): self.content_type = content_type self.data = data self.last_modified_at = last_modified_at - self.thumbnail_location = thumbnail_location + self.thumbnail_location = Location(thumbnail_location) @property def is_thumbnail(self): diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 213beb140a..aebb6dfd32 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -47,7 +47,7 @@ class MongoContentStore(ContentStore): try: with self.fs.get(id) as fp: return StaticContent(location, fp.displayname, fp.content_type, fp.read(), - fp.uploadDate, thumbnail_location = fp.thumbnail_location if 'thumbnail_location' in fp else None) + fp.uploadDate, thumbnail_location = fp.thumbnail_location if hasattr(fp, 'thumbnail_location') else None) except NoFile: raise NotFoundError() diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index df89cbf41c..af346dbb7e 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -81,14 +81,15 @@ def clone_course(modulestore, contentstore, source_location, dest_location, dele # be sure to update the pointer to the thumbnail if content.thumbnail_location is not None: - content.thumbnail_location._replace(tag = dest_location.tag, org = dest_location.org, + content.thumbnail_location = content.thumbnail_location._replace(org = dest_location.org, course = dest_location.course) - print "Cloning asset {0} to {1}".format(asset_loc, content.location) contentstore.save(content) + return True + def delete_course(modulestore, contentstore, source_location): # first check to see if the modulestore is Mongo backed if not isinstance(modulestore, MongoModuleStore): diff --git a/rakefile b/rakefile index 9d3540ee8a..0d5ff5cf82 100644 --- a/rakefile +++ b/rakefile @@ -399,6 +399,7 @@ end namespace :cms do desc "Clone existing MongoDB based course" task :clone do + if ENV['SOURCE_LOC'] and ENV['DEST_LOC'] sh(django_admin(:cms, :dev, :clone, ENV['SOURCE_LOC'], ENV['DEST_LOC'])) else From 708882123310a920aafeacd575ab490a0f642757 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 27 Dec 2012 10:30:26 -0500 Subject: [PATCH 2/5] fix command error message. There are really only two arguments supported (the optional 6 had been removed, but I forgot to change the message --- cms/djangoapps/contentstore/management/commands/clone.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/management/commands/clone.py b/cms/djangoapps/contentstore/management/commands/clone.py index 0f0f020599..7aa5469bd3 100644 --- a/cms/djangoapps/contentstore/management/commands/clone.py +++ b/cms/djangoapps/contentstore/management/commands/clone.py @@ -20,7 +20,7 @@ class Command(BaseCommand): def handle(self, *args, **options): if len(args) != 2: - raise CommandError("clone requires either two or six arguments: ") + raise CommandError("clone requires either two arguments: ") source_location_str = args[0] dest_location_str = args[1] From fe408be1cc9450f1ad171a63f7abbb93fbdb096b Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 27 Dec 2012 10:32:07 -0500 Subject: [PATCH 3/5] message edit --- cms/djangoapps/contentstore/management/commands/clone.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/management/commands/clone.py b/cms/djangoapps/contentstore/management/commands/clone.py index 7aa5469bd3..2357cd1dbd 100644 --- a/cms/djangoapps/contentstore/management/commands/clone.py +++ b/cms/djangoapps/contentstore/management/commands/clone.py @@ -20,7 +20,7 @@ class Command(BaseCommand): def handle(self, *args, **options): if len(args) != 2: - raise CommandError("clone requires either two arguments: ") + raise CommandError("clone requires two arguments: ") source_location_str = args[0] dest_location_str = args[1] From 9dff39dd6dd76133dc34b0ceb47cb926a2df2c58 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 27 Dec 2012 11:25:04 -0500 Subject: [PATCH 4/5] start adding clone unit test --- cms/djangoapps/contentstore/tests/tests.py | 29 ++++++++++++++++++++++ cms/envs/test.py | 8 ++++++ 2 files changed, 37 insertions(+) diff --git a/cms/djangoapps/contentstore/tests/tests.py b/cms/djangoapps/contentstore/tests/tests.py index f5ce0b3692..53868b1ad6 100644 --- a/cms/djangoapps/contentstore/tests/tests.py +++ b/cms/djangoapps/contentstore/tests/tests.py @@ -13,6 +13,10 @@ from xmodule.modulestore.xml_importer import import_from_xml import copy from factories import * +from xmodule.modulestore.store_utilities import clone_course +from xmodule.modulestore.store_utilities import delete_course +from xmodule.modulestore.django import modulestore +from xmodule.contentstore.django import contentstore def parse_json(response): """Parse response, which is assumed to be json""" @@ -339,4 +343,29 @@ class ContentStoreTest(TestCase): def test_edit_unit_full(self): self.check_edit_unit('full') + def test_clone_course(self): + import_from_xml(modulestore(), 'common/test/data/', ['full']) + + CourseFactory.create(org='edX', course='1001', display_name='Clone target') + + ms = modulestore('direct') + cs = contentstore() + + source_location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + dest_location = CourseDescriptor.id_to_location('edX/1001/Clone_target') + + clone_course(ms, cs, source_location, dest_location) + + # now loop through all the units in the course and verify that the clone can render them, which + # means the objects are at least present + items = ms.get_items(Location('edX', 'full', 'vertical', None, None)) + self.assertGreater(len(items), 0) + clone_items = ms.get_items(Location('edX','1001','Clone_target')) + self.assertGreater(len(clone_items), 0) + for descriptor in items: + new_loc = descriptor.location._update({'course':'1001'}) + print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) + resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) + self.assertEqual(resp.status_code, 200) + diff --git a/cms/envs/test.py b/cms/envs/test.py index cb84f32ff1..2971528604 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -59,6 +59,14 @@ MODULESTORE = { } } +CONTENTSTORE = { + 'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore', + 'OPTIONS': { + 'host': 'localhost', + 'db' : 'xcontent', + } +} + DATABASES = { 'default': { 'ENGINE': 'django.db.backends.sqlite3', From 6951865229a7e072109626fdf6cb45a5a659dc2c Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 27 Dec 2012 12:20:45 -0500 Subject: [PATCH 5/5] add unit tests for course cloning and deleting --- cms/djangoapps/contentstore/tests/tests.py | 27 ++++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/tests.py b/cms/djangoapps/contentstore/tests/tests.py index 53868b1ad6..ad4ec8790f 100644 --- a/cms/djangoapps/contentstore/tests/tests.py +++ b/cms/djangoapps/contentstore/tests/tests.py @@ -17,6 +17,7 @@ from xmodule.modulestore.store_utilities import clone_course from xmodule.modulestore.store_utilities import delete_course from xmodule.modulestore.django import modulestore from xmodule.contentstore.django import contentstore +from xmodule.course_module import CourseDescriptor def parse_json(response): """Parse response, which is assumed to be json""" @@ -346,26 +347,42 @@ class ContentStoreTest(TestCase): def test_clone_course(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) - CourseFactory.create(org='edX', course='1001', display_name='Clone target') + resp = self.client.post(reverse('create_new_course'), self.course_data) + self.assertEqual(resp.status_code, 200) + data = parse_json(resp) + self.assertEqual(data['id'], 'i4x://MITx/999/course/Robot_Super_Course') ms = modulestore('direct') cs = contentstore() source_location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') - dest_location = CourseDescriptor.id_to_location('edX/1001/Clone_target') + dest_location = CourseDescriptor.id_to_location('MITx/999/Robot_Super_Course') clone_course(ms, cs, source_location, dest_location) # now loop through all the units in the course and verify that the clone can render them, which # means the objects are at least present - items = ms.get_items(Location('edX', 'full', 'vertical', None, None)) + items = ms.get_items(Location(['i4x','edX', 'full', 'vertical', None])) self.assertGreater(len(items), 0) - clone_items = ms.get_items(Location('edX','1001','Clone_target')) + clone_items = ms.get_items(Location(['i4x', 'MITx','999','vertical', None])) self.assertGreater(len(clone_items), 0) for descriptor in items: - new_loc = descriptor.location._update({'course':'1001'}) + new_loc = descriptor.location._replace(org = 'MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) self.assertEqual(resp.status_code, 200) + def test_delete_course(self): + import_from_xml(modulestore(), 'common/test/data/', ['full']) + + ms = modulestore('direct') + cs = contentstore() + + location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + + delete_course(ms, cs, location) + + items = ms.get_items(Location(['i4x','edX', 'full', 'vertical', None])) + self.assertEqual(len(items), 0) +