From 9e8be1bbc3a5e1b7e5ac69a5ec23777f31a075c5 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 24 Jan 2014 15:56:02 -0500 Subject: [PATCH 01/15] make SplitMigrator take a User object, rather than a user ID --- .../xmodule/modulestore/split_migrator.py | 22 +++++++++---------- .../modulestore/tests/test_split_migrator.py | 3 ++- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index c3c7ed6a2f..e377e7355c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -23,7 +23,7 @@ class SplitMigrator(object): self.draft_modulestore = draft_modulestore self.loc_mapper = loc_mapper - def migrate_mongo_course(self, course_location, user_id, new_package_id=None): + def migrate_mongo_course(self, course_location, user, new_package_id=None): """ Create a new course in split_mongo representing the published and draft versions of the course from the original mongo store. And return the new_package_id (which the caller can also get by calling @@ -32,7 +32,7 @@ class SplitMigrator(object): If the new course already exists, this raises DuplicateItemError :param course_location: a Location whose category is 'course' and points to the course - :param user_id: the user whose action is causing this migration + :param user: the user whose action is causing this migration :param new_package_id: (optional) the Locator.package_id for the new course. Defaults to whatever translate_location_to_locator returns """ @@ -48,18 +48,18 @@ class SplitMigrator(object): new_course_root_locator = self.loc_mapper.translate_location(old_course_id, course_location) new_course = self.split_modulestore.create_course( course_location.org, original_course.display_name, - user_id, id_root=new_package_id, + user.id, id_root=new_package_id, fields=self._get_json_fields_translate_children(original_course, old_course_id, True), root_block_id=new_course_root_locator.block_id, master_branch=new_course_root_locator.branch ) - self._copy_published_modules_to_course(new_course, course_location, old_course_id, user_id) - self._add_draft_modules_to_course(new_package_id, old_course_id, course_location, user_id) + self._copy_published_modules_to_course(new_course, course_location, old_course_id, user) + self._add_draft_modules_to_course(new_package_id, old_course_id, course_location, user) return new_package_id - def _copy_published_modules_to_course(self, new_course, old_course_loc, old_course_id, user_id): + def _copy_published_modules_to_course(self, new_course, old_course_loc, old_course_id, user): """ Copy all of the modules from the 'direct' version of the course to the new split course. """ @@ -79,7 +79,7 @@ class SplitMigrator(object): old_course_id, module.location, True, add_entry_if_missing=True ) _new_module = self.split_modulestore.create_item( - course_version_locator, module.category, user_id, + course_version_locator, module.category, user.id, block_id=new_locator.block_id, fields=self._get_json_fields_translate_children(module, old_course_id, True), continue_version=True @@ -94,7 +94,7 @@ class SplitMigrator(object): # children which meant some pointers were to non-existent locations in 'direct' self.split_modulestore.internal_clean_children(course_version_locator) - def _add_draft_modules_to_course(self, new_package_id, old_course_id, old_course_loc, user_id): + def _add_draft_modules_to_course(self, new_package_id, old_course_id, old_course_loc, user): """ update each draft. Create any which don't exist in published and attach to their parents. """ @@ -124,12 +124,12 @@ class SplitMigrator(object): if name != 'children' and field.is_set_on(module): field.write_to(split_module, field.read_from(module)) - _new_module = self.split_modulestore.update_item(split_module, user_id) + _new_module = self.split_modulestore.update_item(split_module, user.id) else: # only a draft version (aka, 'private'). parent needs updated too. # create a new course version just in case the current head is also the prod head _new_module = self.split_modulestore.create_item( - new_draft_course_loc, module.category, user_id, + new_draft_course_loc, module.category, user.id, block_id=new_locator.block_id, fields=self._get_json_fields_translate_children(module, old_course_id, True) ) @@ -156,7 +156,7 @@ class SplitMigrator(object): new_parent_cursor = idx + 1 break new_parent.children.insert(new_parent_cursor, new_block_id) - new_parent = self.split_modulestore.update_item(new_parent, user_id) + new_parent = self.split_modulestore.update_item(new_parent, user.id) def _get_json_fields_translate_children(self, xblock, old_course_id, published): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index 98c64dcac5..0341d43436 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py @@ -271,7 +271,8 @@ class TestMigration(unittest.TestCase): self.compare_dags(presplit, pre_child, split_child, published) def test_migrator(self): - self.migrator.migrate_mongo_course(self.course_location, random.getrandbits(32)) + user = mock.Mock(id=1) + self.migrator.migrate_mongo_course(self.course_location, user) # now compare the migrated to the original course self.compare_courses(self.old_mongo, True) self.compare_courses(self.draft_mongo, False) From bbec626651818d038ba8d433a322a8487d1987bc Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 24 Jan 2014 09:58:27 -0500 Subject: [PATCH 02/15] first pass on a migrate_to_split Django command --- .../management/commands/migrate_to_split.py | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 cms/djangoapps/contentstore/management/commands/migrate_to_split.py diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py new file mode 100644 index 0000000000..01c6be25a0 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -0,0 +1,74 @@ +""" +Django management command to migrate a course from the old Mongo modulestore +to the new split-Mongo modulestore. +""" +from django.core.management.base import BaseCommand, CommandError +from django.contrib.auth.models import User +from xmodule.modulestore import Location +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.split_migrator import SplitMigrator +from xmodule.modulestore import InvalidLocationError + + +class Command(BaseCommand): + help = "Migrate a course from old-Mongo to split-Mongo" + args = "location email " + + def parse_args(self, *args): + """ + Return a three-tuple of (location, user, locator_string). + If the user didn't specify a locator string, the third return value + will be None. + """ + if len(args) < 2: + raise CommandError( + "migrate_to_split requires at least two arguments: " + "a location and a user identifier (email or ID)" + ) + + try: + location = Location(args[0]) + except InvalidLocationError: + raise CommandError("Invalid location string {}".format(args[0])) + + user_id = None + email = None + try: + user_id = int(args[1]) + except ValueError: + email = args[1] + if user_id: + try: + user = User.objects.get(pk=user_id) + except User.DoesNotExist: + raise CommandError("No user exists with ID {}".format(user_id)) + else: + try: + user = User.objects.get(email=email) + except User.DoesNotExist: + raise CommandError("No user exists with email {}".format(email)) + + assert user, "User doesn't exist! That shouldn't happen..." + + try: + locator_string = args[2] + except IndexError: + locator_string = None + + return location, user, locator_string + + def handle(self, *args, **options): + location, user, locator_string = self.parse_args(*args) + + draft = modulestore('default') + direct = modulestore('direct') + split = modulestore('split') + + migrator = SplitMigrator( + draft_modulestore=draft, + direct_modulestore=direct, + split_modulestore=split, + loc_mapper=split.loc_mapper, + ) + + migrator.migrate_mongo_course(location, user, locator_string) From f08dc42802ce0442744f250c4f329db5cc7bae03 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 24 Jan 2014 13:23:47 -0500 Subject: [PATCH 03/15] Add some dummy arg parsing tests --- .../commands/tests/test_migrate_to_split.py | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py new file mode 100644 index 0000000000..f800b2f62a --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -0,0 +1,33 @@ +""" +Unittests for importing a course via management command +""" + +import unittest + +from django.core.management import CommandError +from contentstore.management.commands.migrate_to_split import Command + + +class TestArgParsing(unittest.TestCase): + def setUp(self): + self.command = Command() + + def test_no_args(self): + errstring = "migrate_to_split requires at least two arguments" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle() + + def test_invalid_location(self): + errstring = "Invalid location string" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("foo", "bar") + + def test_nonexistant_user_id(self): + errstring = "No user exists with ID 99" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("i4x://org/course/category/name", "99") + + def test_nonexistant_user_email(self): + errstring = "No user exists with email fake@example.com" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("i4x://org/course/category/name", "fake@example.com") From de96a47eac35a7cb340848c6d14a3512f86a1210 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Fri, 24 Jan 2014 13:40:05 -0500 Subject: [PATCH 04/15] happy path unit test --- .../management/commands/migrate_to_split.py | 13 +++----- .../commands/tests/test_migrate_to_split.py | 32 +++++++++++++++++-- .../xmodule/modulestore/tests/django_utils.py | 5 ++- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py index 01c6be25a0..87197886e2 100644 --- a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -8,6 +8,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.split_migrator import SplitMigrator from xmodule.modulestore import InvalidLocationError +from xmodule.modulestore.django import loc_mapper class Command(BaseCommand): @@ -60,15 +61,11 @@ class Command(BaseCommand): def handle(self, *args, **options): location, user, locator_string = self.parse_args(*args) - draft = modulestore('default') - direct = modulestore('direct') - split = modulestore('split') - migrator = SplitMigrator( - draft_modulestore=draft, - direct_modulestore=direct, - split_modulestore=split, - loc_mapper=split.loc_mapper, + draft_modulestore=modulestore('default'), + direct_modulestore=modulestore('direct'), + split_modulestore=modulestore('split'), + loc_mapper=loc_mapper(), ) migrator.migrate_mongo_course(location, user, locator_string) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index f800b2f62a..55879e4f43 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -1,11 +1,17 @@ """ Unittests for importing a course via management command """ - import unittest -from django.core.management import CommandError +from django.core.management import CommandError, call_command +from django.test.utils import override_settings from contentstore.management.commands.migrate_to_split import Command +from contentstore.tests.modulestore_config import TEST_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.django import loc_mapper class TestArgParsing(unittest.TestCase): @@ -31,3 +37,25 @@ class TestArgParsing(unittest.TestCase): errstring = "No user exists with email fake@example.com" with self.assertRaisesRegexp(CommandError, errstring): self.command.handle("i4x://org/course/category/name", "fake@example.com") + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestMigrateToSplit(ModuleStoreTestCase): + """ + Unit tests for importing a course from command line + """ + + def setUp(self): + super(TestMigrateToSplit, self).setUp() + self.course = CourseFactory() + self.user = UserFactory() + + def test_happy_path(self): + call_command( + "migrate_to_split", + str(self.course.location), + self.user.email, + ) + locator = loc_mapper().translate_location(self.course.id, self.course.location) + course_from_split = modulestore('split').get_course(locator) + self.assertIsNotNone(course_from_split) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 684664890e..12994c9498 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -5,7 +5,7 @@ Modulestore configuration for test cases. from uuid import uuid4 from django.test import TestCase from xmodule.modulestore.django import editable_modulestore, \ - clear_existing_modulestores + clear_existing_modulestores, loc_mapper from xmodule.contentstore.django import contentstore @@ -225,6 +225,9 @@ class ModuleStoreTestCase(TestCase): if contentstore().fs_files: db = contentstore().fs_files.database db.connection.drop_database(db) + lm = loc_mapper() + if lm.db: + lm.location_map.drop() @classmethod def setUpClass(cls): From dcc1d201cedee043375e7858018950c6df7e8d95 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 27 Jan 2014 09:53:29 -0500 Subject: [PATCH 05/15] Can't use UserFactory in CMS tests :( --- .../management/commands/tests/test_migrate_to_split.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 55879e4f43..7839d8c2e9 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -3,13 +3,13 @@ Unittests for importing a course via management command """ import unittest +from django.contrib.auth.models import User from django.core.management import CommandError, call_command from django.test.utils import override_settings from contentstore.management.commands.migrate_to_split import Command from contentstore.tests.modulestore_config import TEST_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from student.tests.factories import UserFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import loc_mapper @@ -47,8 +47,11 @@ class TestMigrateToSplit(ModuleStoreTestCase): def setUp(self): super(TestMigrateToSplit, self).setUp() + uname = 'testuser' + email = 'test+courses@edx.org' + password = 'foo' + self.user = User.objects.create_user(uname, email, password) self.course = CourseFactory() - self.user = UserFactory() def test_happy_path(self): call_command( From 096088cc755e73b4d9eaa02ddb9e4c68352c0930 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Mon, 27 Jan 2014 15:05:00 -0500 Subject: [PATCH 06/15] fix test failures: clear loc_mapper cache --- .../contentstore/tests/test_contentstore.py | 42 +++++++++---------- .../lib/xmodule/xmodule/modulestore/django.py | 3 ++ .../xmodule/modulestore/loc_mapper_store.py | 17 ++++---- .../xmodule/modulestore/tests/django_utils.py | 4 +- 4 files changed, 36 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 3cd0ba90b1..d25ffe0376 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -133,7 +133,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # just pick one vertical descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0] - locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, False, True) + locator = loc_mapper().translate_location(course.location.course_id, descriptor.location, True, True) resp = self.client.get_html(locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) _test_no_locations(self, resp) @@ -144,12 +144,11 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): def test_advanced_components_in_edit_unit(self): # This could be made better, but for now let's just assert that we see the advanced modules mentioned in the page # response HTML - self.check_components_on_page(ADVANCED_COMPONENT_TYPES, ['Word cloud', - 'Annotation', - 'Text Annotation', - 'Video Annotation', - 'Open Response Assessment', - 'Peer Grading Interface']) + self.check_components_on_page( + ADVANCED_COMPONENT_TYPES, + ['Word cloud', 'Annotation', 'Text Annotation', 'Video Annotation', + 'Open Response Assessment', 'Peer Grading Interface'], + ) def test_advanced_components_require_two_clicks(self): self.check_components_on_page(['word_cloud'], ['Word cloud']) @@ -161,7 +160,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # just pick one vertical descriptor = store.get_items(Location('i4x', 'edX', 'simple', 'vertical', None, None))[0] location = descriptor.location.replace(name='.' + descriptor.location.name) - locator = loc_mapper().translate_location(course_items[0].location.course_id, location, False, True) + locator = loc_mapper().translate_location( + course_items[0].location.course_id, location, add_entry_if_missing=True) resp = self.client.get_html(locator.url_reverse('unit')) self.assertEqual(resp.status_code, 400) @@ -449,7 +449,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): """ Returns the locator for a given tab. """ tab_location = 'i4x://edX/999/static_tab/{0}'.format(tab['url_slug']) return loc_mapper().translate_location( - course.location.course_id, Location(tab_location), False, True + course.location.course_id, Location(tab_location), True, True ) def _create_static_tabs(self): @@ -457,7 +457,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): module_store = modulestore('direct') CourseFactory.create(org='edX', course='999', display_name='Robot Super Course') course_location = Location('i4x', 'edX', '999', 'course', 'Robot_Super_Course', None) - new_location = loc_mapper().translate_location(course_location.course_id, course_location, False, True) + new_location = loc_mapper().translate_location(course_location.course_id, course_location, True, True) ItemFactory.create( parent_location=course_location, @@ -512,7 +512,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # also try a custom response which will trigger the 'is this course in whitelist' logic locator = loc_mapper().translate_location( - course_items[0].location.course_id, location, False, True + course_items[0].location.course_id, location, True, True ) resp = self.client.get_html(locator.url_reverse('xblock')) self.assertEqual(resp.status_code, 200) @@ -534,7 +534,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent points to the child object which is to be deleted self.assertTrue(sequential.location.url() in chapter.children) - location = loc_mapper().translate_location(course_location.course_id, sequential.location, False, True) + location = loc_mapper().translate_location(course_location.course_id, sequential.location, True, True) self.client.delete(location.url_reverse('xblock'), {'recurse': True, 'all_versions': True}) found = False @@ -685,7 +685,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # go through the website to do the delete, since the soft-delete logic is in the view course = course_items[0] - location = loc_mapper().translate_location(course.location.course_id, course.location, False, True) + location = loc_mapper().translate_location(course.location.course_id, course.location, True, True) url = location.url_reverse('assets/', '/c4x/edX/toy/asset/sample_static.txt') resp = self.client.delete(url) self.assertEqual(resp.status_code, 204) @@ -1062,7 +1062,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): ) # Unit test fails in Jenkins without this. - loc_mapper().translate_location(course_location.course_id, course_location, False, True) + loc_mapper().translate_location(course_location.course_id, course_location, True, True) items = module_store.get_items(stub_location.replace(category='vertical', name=None)) self._check_verticals(items, course_location.course_id) @@ -1353,7 +1353,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # Assert is here to make sure that the course being tested actually has verticals (units) to check. self.assertGreater(len(items), 0) for descriptor in items: - unit_locator = loc_mapper().translate_location(course_id, descriptor.location, False, True) + unit_locator = loc_mapper().translate_location(course_id, descriptor.location, True, True) resp = self.client.get_html(unit_locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) _test_no_locations(self, resp) @@ -1645,7 +1645,7 @@ class ContentStoreTest(ModuleStoreTestCase): import_from_xml(modulestore('direct'), 'common/test/data/', ['simple']) loc = Location(['i4x', 'edX', 'simple', 'course', '2012_Fall', None]) - new_location = loc_mapper().translate_location(loc.course_id, loc, False, True) + new_location = loc_mapper().translate_location(loc.course_id, loc, True, True) resp = self._show_course_overview(loc) self.assertEqual(resp.status_code, 200) @@ -1666,14 +1666,14 @@ class ContentStoreTest(ModuleStoreTestCase): # go look at a subsection page subsection_location = loc.replace(category='sequential', name='test_sequence') - subsection_locator = loc_mapper().translate_location(loc.course_id, subsection_location, False, True) + subsection_locator = loc_mapper().translate_location(loc.course_id, subsection_location, True, True) resp = self.client.get_html(subsection_locator.url_reverse('subsection')) self.assertEqual(resp.status_code, 200) _test_no_locations(self, resp) # go look at the Edit page unit_location = loc.replace(category='vertical', name='test_vertical') - unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, False, True) + unit_locator = loc_mapper().translate_location(loc.course_id, unit_location, True, True) resp = self.client.get_html(unit_locator.url_reverse('unit')) self.assertEqual(resp.status_code, 200) _test_no_locations(self, resp) @@ -1681,7 +1681,7 @@ class ContentStoreTest(ModuleStoreTestCase): def delete_item(category, name): """ Helper method for testing the deletion of an xblock item. """ del_loc = loc.replace(category=category, name=name) - del_location = loc_mapper().translate_location(loc.course_id, del_loc, False, True) + del_location = loc_mapper().translate_location(loc.course_id, del_loc, True, True) resp = self.client.delete(del_location.url_reverse('xblock')) self.assertEqual(resp.status_code, 204) _test_no_locations(self, resp, status_code=204, html=False) @@ -1883,7 +1883,7 @@ class ContentStoreTest(ModuleStoreTestCase): """ Show the course overview page. """ - new_location = loc_mapper().translate_location(location.course_id, location, False, True) + new_location = loc_mapper().translate_location(location.course_id, location, True, True) resp = self.client.get_html(new_location.url_reverse('course/', '')) _test_no_locations(self, resp) return resp @@ -1998,7 +1998,7 @@ def _course_factory_create_course(): Creates a course via the CourseFactory and returns the locator for it. """ course = CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course') - return loc_mapper().translate_location(course.location.course_id, course.location, False, True) + return loc_mapper().translate_location(course.location.course_id, course.location, True, True) def _get_course_id(test_course_data): diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 13e0a2878f..62ed8de035 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/common/lib/xmodule/xmodule/modulestore/django.py @@ -152,6 +152,9 @@ def clear_existing_modulestores(): _MODULESTORES.clear() # pylint: disable=W0603 global _loc_singleton + cache = getattr(_loc_singleton, "cache", None) + if cache: + cache.clear() _loc_singleton = None diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 9d5c29e6ae..ce22793651 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -93,7 +93,7 @@ class LocMapperStore(object): package_id = "{0.org}.{0.course}".format(course_location) # very like _interpret_location_id but w/o the _id location_id = self._construct_location_son( - course_location.org, course_location.course, + course_location.org, course_location.course, course_location.name if course_location.category == 'course' else None ) @@ -219,6 +219,11 @@ class LocMapperStore(object): return None result = None for candidate in maps: + if get_course and 'name' in candidate['_id']: + candidate_id = candidate['_id'] + return Location( + 'i4x', candidate_id['org'], candidate_id['course'], 'course', candidate_id['name'] + ) old_course_id = self._generate_location_course_id(candidate['_id']) for old_name, cat_to_usage in candidate['block_map'].iteritems(): for category, block_id in cat_to_usage.iteritems(): @@ -240,7 +245,7 @@ class LocMapperStore(object): candidate['course_id'], branch=candidate['draft_branch'], block_id=block_id ) self._cache_location_map_entry(old_course_id, location, published_locator, draft_locator) - + if get_course and category == 'course': result = location elif not get_course and block_id == locator.block_id: @@ -261,8 +266,6 @@ class LocMapperStore(object): return cached location_id = self._interpret_location_course_id(old_style_course_id, location) - if old_style_course_id is None: - old_style_course_id = self._generate_location_course_id(location_id) maps = self.location_map.find(location_id) maps = list(maps) @@ -320,10 +323,10 @@ class LocMapperStore(object): return {'_id': self._construct_location_son(location.org, location.course, location.name)} else: return bson.son.SON([('_id.org', location.org), ('_id.course', location.course)]) - + def _generate_location_course_id(self, entry_id): """ - Generate a Location course_id for the given entry's id + Generate a Location course_id for the given entry's id. """ # strip id envelope if any entry_id = entry_id.get('_id', entry_id) @@ -334,7 +337,7 @@ class LocMapperStore(object): return '{0[_id.org]}/{0[_id.course]}'.format(entry_id) else: return '{0[org]}/{0[course]}'.format(entry_id) - + def _construct_location_son(self, org, course, name=None): """ Construct the SON needed to repr the location for either a query or an insertion diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 12994c9498..b0cd3df228 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -4,8 +4,8 @@ Modulestore configuration for test cases. from uuid import uuid4 from django.test import TestCase -from xmodule.modulestore.django import editable_modulestore, \ - clear_existing_modulestores, loc_mapper +from xmodule.modulestore.django import ( + editable_modulestore, clear_existing_modulestores, loc_mapper) from xmodule.contentstore.django import contentstore From c3b8d52dcd7a5883864662a15543e4a9ada94d2e Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 28 Jan 2014 10:13:07 -0500 Subject: [PATCH 07/15] move user-finding logic out of command --- .../management/commands/migrate_to_split.py | 35 ++++++++++--------- .../commands/tests/test_migrate_to_split.py | 18 +++++++--- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py index 87197886e2..283193dd0f 100644 --- a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -11,6 +11,21 @@ from xmodule.modulestore import InvalidLocationError from xmodule.modulestore.django import loc_mapper +def user_from_str(s): + """ + Return a user identified by the given string. The string could be an email + address, or a stringified integer corresponding to the ID of the user in + the database. If no user could be found, a User.DoesNotExist exception + will be raised. + """ + try: + user_id = int(s) + except ValueError: + return User.objects.get(email=s) + else: + return User.objects.get(id=user_id) + + class Command(BaseCommand): help = "Migrate a course from old-Mongo to split-Mongo" args = "location email " @@ -32,24 +47,10 @@ class Command(BaseCommand): except InvalidLocationError: raise CommandError("Invalid location string {}".format(args[0])) - user_id = None - email = None try: - user_id = int(args[1]) - except ValueError: - email = args[1] - if user_id: - try: - user = User.objects.get(pk=user_id) - except User.DoesNotExist: - raise CommandError("No user exists with ID {}".format(user_id)) - else: - try: - user = User.objects.get(email=email) - except User.DoesNotExist: - raise CommandError("No user exists with email {}".format(email)) - - assert user, "User doesn't exist! That shouldn't happen..." + user = user_from_str(args[1]) + except User.DoesNotExist: + raise CommandError("No user found identified by {}".format(args[1])) try: locator_string = args[2] diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 7839d8c2e9..2389120616 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -29,12 +29,12 @@ class TestArgParsing(unittest.TestCase): self.command.handle("foo", "bar") def test_nonexistant_user_id(self): - errstring = "No user exists with ID 99" + errstring = "No user found identified by 99" with self.assertRaisesRegexp(CommandError, errstring): self.command.handle("i4x://org/course/category/name", "99") def test_nonexistant_user_email(self): - errstring = "No user exists with email fake@example.com" + errstring = "No user found identified by fake@example.com" with self.assertRaisesRegexp(CommandError, errstring): self.command.handle("i4x://org/course/category/name", "fake@example.com") @@ -53,11 +53,21 @@ class TestMigrateToSplit(ModuleStoreTestCase): self.user = User.objects.create_user(uname, email, password) self.course = CourseFactory() - def test_happy_path(self): + def test_happy_path_email(self): call_command( "migrate_to_split", str(self.course.location), - self.user.email, + str(self.user.email), + ) + locator = loc_mapper().translate_location(self.course.id, self.course.location) + course_from_split = modulestore('split').get_course(locator) + self.assertIsNotNone(course_from_split) + + def test_happy_path_user_id(self): + call_command( + "migrate_to_split", + str(self.course.location), + str(self.user.id), ) locator = loc_mapper().translate_location(self.course.id, self.course.location) course_from_split = modulestore('split').get_course(locator) From 3eee65892b6eafc5a7b0b9164f4395a2a8f0faac Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 28 Jan 2014 13:40:56 -0500 Subject: [PATCH 08/15] Added command to delete split-mongo course --- .../commands/delete_split_course.py | 31 +++++++++++ .../tests/test_delete_split_course.py | 53 +++++++++++++++++++ .../commands/tests/test_migrate_to_split.py | 2 +- 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 cms/djangoapps/contentstore/management/commands/delete_split_course.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py diff --git a/cms/djangoapps/contentstore/management/commands/delete_split_course.py b/cms/djangoapps/contentstore/management/commands/delete_split_course.py new file mode 100644 index 0000000000..68230bc1b7 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/delete_split_course.py @@ -0,0 +1,31 @@ +""" +Django management command to rollback a migration to split. The way to do this +is to delete the course from the split mongo datastore. +""" +from django.core.management.base import BaseCommand, CommandError +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.locator import CourseLocator + + +class Command(BaseCommand): + "Delete a course from the split Mongo datastore" + + help = "Delete a course from the split Mongo datastore" + args = "locator" + + def handle(self, *args, **options): + if len(args) < 1: + raise CommandError( + "delete_split_course requires at least one argument (locator)" + ) + + try: + locator = CourseLocator(url=args[0]) + except ValueError: + raise CommandError("Invalid locator string {}".format(args[0])) + + try: + modulestore('split').delete_course(locator.package_id) + except ItemNotFoundError: + raise CommandError("No course found with locator {}".format(locator)) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py new file mode 100644 index 0000000000..02db42abc5 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py @@ -0,0 +1,53 @@ +""" +Unittests for deleting a split mongo course +""" +import unittest + +from django.core.management import CommandError, call_command +from django.test.utils import override_settings +from contentstore.management.commands.delete_split_course import Command +from contentstore.tests.modulestore_config import TEST_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.persistent_factories import PersistentCourseFactory +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + + +class TestArgParsing(unittest.TestCase): + def setUp(self): + self.command = Command() + + def test_no_args(self): + errstring = "delete_split_course requires at least one argument" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle() + + def test_invalid_locator(self): + errstring = "Invalid locator string !?!" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("!?!") + + def test_nonexistant_locator(self): + errstring = "No course found with locator course/branch/name" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("course/branch/name") + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestDeleteSplitCourse(ModuleStoreTestCase): + """ + Unit tests for deleting a split-mongo course from command line + """ + + def setUp(self): + super(TestDeleteSplitCourse, self).setUp() + self.course = PersistentCourseFactory() + + def test_happy_path(self): + locator = self.course.location + call_command( + "delete_split_course", + str(locator), + ) + with self.assertRaises(ItemNotFoundError): + modulestore('split').get_course(locator) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 2389120616..d8e90bb9c2 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -1,5 +1,5 @@ """ -Unittests for importing a course via management command +Unittests for migrating a course to split mongo """ import unittest From 692502c0fb8f6ef1eacd1388a0ca772993e8d794 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 28 Jan 2014 15:54:00 -0500 Subject: [PATCH 09/15] Don't cache falsy values --- .../lib/xmodule/xmodule/modulestore/loc_mapper_store.py | 4 ++++ .../xmodule/modulestore/tests/test_location_mapper.py | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index ce22793651..7e00abfdac 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -404,6 +404,8 @@ class LocMapperStore(object): """ Get the course Locator for this old course id """ + if not old_course_id: + return None entry = self.cache.get(old_course_id) if entry is not None: if published: @@ -428,6 +430,8 @@ class LocMapperStore(object): """ For quick lookup of courses """ + if not old_course_id: + return self.cache.set(old_course_id, (published_course_locator, draft_course_locator)) def _cache_location_map_entry(self, old_course_id, location, published_usage, draft_usage): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 90da230e76..a0e869d9fe 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -80,8 +80,8 @@ class TestLocationMapper(unittest.TestCase): Request translation, check package_id, block_id, and branch """ prob_locator = loc_mapper().translate_location( - old_style_course_id, - location, + old_style_course_id, + location, published= (branch=='published'), add_entry_if_missing=add_entry ) @@ -114,7 +114,7 @@ class TestLocationMapper(unittest.TestCase): new_style_package_id = '{}.geek_dept.{}.baz_run'.format(org, course) block_map = { - 'abc123': {'problem': 'problem2'}, + 'abc123': {'problem': 'problem2'}, 'def456': {'problem': 'problem4'}, 'ghi789': {'problem': 'problem7'}, } @@ -139,7 +139,7 @@ class TestLocationMapper(unittest.TestCase): # add a distractor course (note that abc123 has a different translation in this one) distractor_block_map = { - 'abc123': {'problem': 'problem3'}, + 'abc123': {'problem': 'problem3'}, 'def456': {'problem': 'problem4'}, 'ghi789': {'problem': 'problem7'}, } From e18507f1886eabee458f4a60269a2e228c7af46f Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Tue, 28 Jan 2014 16:08:08 -0500 Subject: [PATCH 10/15] fix pep8/pylint issues --- .../contentstore/management/commands/migrate_to_split.py | 8 +++++--- .../management/commands/tests/test_delete_split_course.py | 4 ++++ .../management/commands/tests/test_migrate_to_split.py | 4 ++++ .../lib/xmodule/xmodule/modulestore/tests/django_utils.py | 6 +++--- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py index 283193dd0f..dbba474d49 100644 --- a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -11,7 +11,7 @@ from xmodule.modulestore import InvalidLocationError from xmodule.modulestore.django import loc_mapper -def user_from_str(s): +def user_from_str(identifier): """ Return a user identified by the given string. The string could be an email address, or a stringified integer corresponding to the ID of the user in @@ -19,14 +19,16 @@ def user_from_str(s): will be raised. """ try: - user_id = int(s) + user_id = int(identifier) except ValueError: - return User.objects.get(email=s) + return User.objects.get(email=identifier) else: return User.objects.get(id=user_id) class Command(BaseCommand): + "Migrate a course from old-Mongo to split-Mongo" + help = "Migrate a course from old-Mongo to split-Mongo" args = "location email " diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py index 02db42abc5..f8411d0967 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py @@ -11,9 +11,13 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.persistent_factories import PersistentCourseFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError +# pylint: disable=E1101 class TestArgParsing(unittest.TestCase): + """ + Tests for parsing arguments for the `delete_split_course` management command + """ def setUp(self): self.command = Command() diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index d8e90bb9c2..2dd652b422 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -12,9 +12,13 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import loc_mapper +# pylint: disable=E1101 class TestArgParsing(unittest.TestCase): + """ + Tests for parsing arguments for the `migrate_to_split` management command + """ def setUp(self): self.command = Command() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index b0cd3df228..88bc6c87d9 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -225,9 +225,9 @@ class ModuleStoreTestCase(TestCase): if contentstore().fs_files: db = contentstore().fs_files.database db.connection.drop_database(db) - lm = loc_mapper() - if lm.db: - lm.location_map.drop() + location_mapper = loc_mapper() + if location_mapper.db: + location_mapper.location_map.drop() @classmethod def setUpClass(cls): From e0c1abc11f16acf93fa56f7433e3428401c42dae Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 29 Jan 2014 10:20:03 -0500 Subject: [PATCH 11/15] check for old mongo course --- .gitignore | 1 + .../commands/delete_split_course.py | 31 ----- .../commands/rollback_split_course.py | 51 +++++++++ .../tests/test_delete_split_course.py | 57 --------- .../tests/test_rollback_split_course.py | 108 ++++++++++++++++++ 5 files changed, 160 insertions(+), 88 deletions(-) delete mode 100644 cms/djangoapps/contentstore/management/commands/delete_split_course.py create mode 100644 cms/djangoapps/contentstore/management/commands/rollback_split_course.py delete mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py create mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py diff --git a/.gitignore b/.gitignore index eb1c8904f8..10cc4812a8 100644 --- a/.gitignore +++ b/.gitignore @@ -49,6 +49,7 @@ coverage.xml cover/ cover_html/ reports/ +jscover.log jscover.log.* ### Installation artifacts diff --git a/cms/djangoapps/contentstore/management/commands/delete_split_course.py b/cms/djangoapps/contentstore/management/commands/delete_split_course.py deleted file mode 100644 index 68230bc1b7..0000000000 --- a/cms/djangoapps/contentstore/management/commands/delete_split_course.py +++ /dev/null @@ -1,31 +0,0 @@ -""" -Django management command to rollback a migration to split. The way to do this -is to delete the course from the split mongo datastore. -""" -from django.core.management.base import BaseCommand, CommandError -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.locator import CourseLocator - - -class Command(BaseCommand): - "Delete a course from the split Mongo datastore" - - help = "Delete a course from the split Mongo datastore" - args = "locator" - - def handle(self, *args, **options): - if len(args) < 1: - raise CommandError( - "delete_split_course requires at least one argument (locator)" - ) - - try: - locator = CourseLocator(url=args[0]) - except ValueError: - raise CommandError("Invalid locator string {}".format(args[0])) - - try: - modulestore('split').delete_course(locator.package_id) - except ItemNotFoundError: - raise CommandError("No course found with locator {}".format(locator)) diff --git a/cms/djangoapps/contentstore/management/commands/rollback_split_course.py b/cms/djangoapps/contentstore/management/commands/rollback_split_course.py new file mode 100644 index 0000000000..3681ebf282 --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/rollback_split_course.py @@ -0,0 +1,51 @@ +""" +Django management command to rollback a migration to split. The way to do this +is to delete the course from the split mongo datastore. +""" +from django.core.management.base import BaseCommand, CommandError +from xmodule.modulestore.django import modulestore, loc_mapper +from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError +from xmodule.modulestore.locator import CourseLocator + + +class Command(BaseCommand): + "Rollback a course that was migrated to the split Mongo datastore" + + help = "Rollback a course that was migrated to the split Mongo datastore" + args = "locator" + + def handle(self, *args, **options): + if len(args) < 1: + raise CommandError( + "rollback_split_course requires at least one argument (locator)" + ) + + try: + locator = CourseLocator(url=args[0]) + except ValueError: + raise CommandError("Invalid locator string {}".format(args[0])) + + location = loc_mapper().translate_locator_to_location(locator, get_course=True) + if not location: + raise CommandError( + "This course does not exist in the old Mongo store. " + "This command is designed to rollback a course, not delete " + "it entirely." + ) + old_mongo_course = modulestore('direct').get_item(location) + if not old_mongo_course: + raise CommandError( + "This course does not exist in the old Mongo store. " + "This command is designed to rollback a course, not delete " + "it entirely." + ) + + try: + modulestore('split').delete_course(locator.package_id) + except ItemNotFoundError: + raise CommandError("No course found with locator {}".format(locator)) + + print( + 'Course rolled back successfully. To delete this course entirely, ' + 'call the "delete_course" management command.' + ) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py deleted file mode 100644 index f8411d0967..0000000000 --- a/cms/djangoapps/contentstore/management/commands/tests/test_delete_split_course.py +++ /dev/null @@ -1,57 +0,0 @@ -""" -Unittests for deleting a split mongo course -""" -import unittest - -from django.core.management import CommandError, call_command -from django.test.utils import override_settings -from contentstore.management.commands.delete_split_course import Command -from contentstore.tests.modulestore_config import TEST_MODULESTORE -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.persistent_factories import PersistentCourseFactory -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.exceptions import ItemNotFoundError -# pylint: disable=E1101 - - -class TestArgParsing(unittest.TestCase): - """ - Tests for parsing arguments for the `delete_split_course` management command - """ - def setUp(self): - self.command = Command() - - def test_no_args(self): - errstring = "delete_split_course requires at least one argument" - with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle() - - def test_invalid_locator(self): - errstring = "Invalid locator string !?!" - with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle("!?!") - - def test_nonexistant_locator(self): - errstring = "No course found with locator course/branch/name" - with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle("course/branch/name") - - -@override_settings(MODULESTORE=TEST_MODULESTORE) -class TestDeleteSplitCourse(ModuleStoreTestCase): - """ - Unit tests for deleting a split-mongo course from command line - """ - - def setUp(self): - super(TestDeleteSplitCourse, self).setUp() - self.course = PersistentCourseFactory() - - def test_happy_path(self): - locator = self.course.location - call_command( - "delete_split_course", - str(locator), - ) - with self.assertRaises(ItemNotFoundError): - modulestore('split').get_course(locator) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py new file mode 100644 index 0000000000..e30f9324fe --- /dev/null +++ b/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py @@ -0,0 +1,108 @@ +""" +Unittests for deleting a split mongo course +""" +import unittest +from StringIO import StringIO +from mock import patch + +from django.contrib.auth.models import User +from django.core.management import CommandError, call_command +from django.test.utils import override_settings +from contentstore.management.commands.rollback_split_course import Command +from contentstore.tests.modulestore_config import TEST_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.persistent_factories import PersistentCourseFactory +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.django import modulestore, loc_mapper +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.split_migrator import SplitMigrator +# pylint: disable=E1101 + + +class TestArgParsing(unittest.TestCase): + """ + Tests for parsing arguments for the `rollback_split_course` management command + """ + def setUp(self): + self.command = Command() + + def test_no_args(self): + errstring = "rollback_split_course requires at least one argument" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle() + + def test_invalid_locator(self): + errstring = "Invalid locator string !?!" + with self.assertRaisesRegexp(CommandError, errstring): + self.command.handle("!?!") + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestRollbackSplitCourseNoOldMongo(ModuleStoreTestCase): + """ + Unit tests for rolling back a split-mongo course from command line + """ + + def setUp(self): + super(TestRollbackSplitCourseNoOldMongo, self).setUp() + self.course = PersistentCourseFactory() + + def test_no_old_course(self): + locator = self.course.location + errstring = "course does not exist in the old Mongo store" + with self.assertRaisesRegexp(CommandError, errstring): + Command().handle(str(locator)) + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestRollbackSplitCourseNoSplitMongo(ModuleStoreTestCase): + """ + Unit tests for rolling back a split-mongo course from command line + """ + + def setUp(self): + super(TestRollbackSplitCourseNoSplitMongo, self).setUp() + self.old_course = CourseFactory() + + def test_nonexistent_locator(self): + locator = loc_mapper().translate_location(self.old_course.id, self.old_course.location) + errstring = "No course found with locator" + with self.assertRaisesRegexp(CommandError, errstring): + Command().handle(str(locator)) + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestRollbackSplitCourse(ModuleStoreTestCase): + """ + Unit tests for rolling back a split-mongo course from command line + """ + def setUp(self): + super(TestRollbackSplitCourse, self).setUp() + self.old_course = CourseFactory() + uname = 'testuser' + email = 'test+courses@edx.org' + password = 'foo' + self.user = User.objects.create_user(uname, email, password) + + # migrate old course to split + migrator = SplitMigrator( + draft_modulestore=modulestore('default'), + direct_modulestore=modulestore('direct'), + split_modulestore=modulestore('split'), + loc_mapper=loc_mapper(), + ) + migrator.migrate_mongo_course(self.old_course.location, self.user) + locator = loc_mapper().translate_location(self.old_course.id, self.old_course.location) + self.course = modulestore('split').get_course(locator) + + @patch("sys.stdout", new_callable=StringIO) + def test_happy_path(self, mock_stdout): + locator = self.course.location + call_command( + "rollback_split_course", + str(locator), + ) + with self.assertRaises(ItemNotFoundError): + modulestore('split').get_course(locator) + + self.assertIn("Course rolled back successfully", mock_stdout.getvalue()) + From cfdc5b5e96df594ca1a0668e07fd70c62e8e735f Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 29 Jan 2014 11:42:38 -0500 Subject: [PATCH 12/15] fix docstrings --- .../management/commands/tests/test_migrate_to_split.py | 2 +- .../management/commands/tests/test_rollback_split_course.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 2dd652b422..87b7872454 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -46,7 +46,7 @@ class TestArgParsing(unittest.TestCase): @override_settings(MODULESTORE=TEST_MODULESTORE) class TestMigrateToSplit(ModuleStoreTestCase): """ - Unit tests for importing a course from command line + Unit tests for migrating a course from old mongo to split mongo """ def setUp(self): diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py index e30f9324fe..98b1ea807e 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py @@ -40,7 +40,8 @@ class TestArgParsing(unittest.TestCase): @override_settings(MODULESTORE=TEST_MODULESTORE) class TestRollbackSplitCourseNoOldMongo(ModuleStoreTestCase): """ - Unit tests for rolling back a split-mongo course from command line + Unit tests for rolling back a split-mongo course from command line, + where the course doesn't exist in the old mongo store """ def setUp(self): @@ -56,7 +57,8 @@ class TestRollbackSplitCourseNoOldMongo(ModuleStoreTestCase): @override_settings(MODULESTORE=TEST_MODULESTORE) class TestRollbackSplitCourseNoSplitMongo(ModuleStoreTestCase): """ - Unit tests for rolling back a split-mongo course from command line + Unit tests for rolling back a split-mongo course from command line, + where the course doesn't exist in the split mongo store """ def setUp(self): From e10e47d8abb6d033bad9e0e2a60ad30240cd6e37 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 29 Jan 2014 15:10:52 -0500 Subject: [PATCH 13/15] Add test for specifying locator to migrate_to_split command --- .../commands/tests/test_migrate_to_split.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 87b7872454..215bb9d9aa 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -10,8 +10,8 @@ from contentstore.management.commands.migrate_to_split import Command from contentstore.tests.modulestore_config import TEST_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.django import modulestore, loc_mapper +from xmodule.modulestore.locator import CourseLocator # pylint: disable=E1101 @@ -57,7 +57,7 @@ class TestMigrateToSplit(ModuleStoreTestCase): self.user = User.objects.create_user(uname, email, password) self.course = CourseFactory() - def test_happy_path_email(self): + def test_user_email(self): call_command( "migrate_to_split", str(self.course.location), @@ -67,7 +67,7 @@ class TestMigrateToSplit(ModuleStoreTestCase): course_from_split = modulestore('split').get_course(locator) self.assertIsNotNone(course_from_split) - def test_happy_path_user_id(self): + def test_user_id(self): call_command( "migrate_to_split", str(self.course.location), @@ -76,3 +76,14 @@ class TestMigrateToSplit(ModuleStoreTestCase): locator = loc_mapper().translate_location(self.course.id, self.course.location) course_from_split = modulestore('split').get_course(locator) self.assertIsNotNone(course_from_split) + + def test_locator_string(self): + call_command( + "migrate_to_split", + str(self.course.location), + str(self.user.id), + "org.dept.name.run", + ) + locator = CourseLocator(package_id="org.dept.name.run", branch="published") + course_from_split = modulestore('split').get_course(locator) + self.assertIsNotNone(course_from_split) From 7cbee5e817b6d2bbf4fbcbf8cf1cf327bdbabc9c Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 29 Jan 2014 15:55:57 -0500 Subject: [PATCH 14/15] rename locator_string to package_id --- .../management/commands/migrate_to_split.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py index dbba474d49..3ef8dd79fb 100644 --- a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -55,14 +55,14 @@ class Command(BaseCommand): raise CommandError("No user found identified by {}".format(args[1])) try: - locator_string = args[2] + package_id = args[2] except IndexError: - locator_string = None + package_id = None - return location, user, locator_string + return location, user, package_id def handle(self, *args, **options): - location, user, locator_string = self.parse_args(*args) + location, user, package_id = self.parse_args(*args) migrator = SplitMigrator( draft_modulestore=modulestore('default'), @@ -71,4 +71,4 @@ class Command(BaseCommand): loc_mapper=loc_mapper(), ) - migrator.migrate_mongo_course(location, user, locator_string) + migrator.migrate_mongo_course(location, user, package_id) From dd627edae4c4273ecb5dd616e23bdc846eabd09b Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Wed, 29 Jan 2014 16:57:23 -0500 Subject: [PATCH 15/15] add logging for auditing when split-mongo courses are deleted --- common/lib/xmodule/xmodule/modulestore/split_mongo/split.py | 1 + 1 file changed, 1 insertion(+) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 5f5f047f1b..279002e473 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1284,6 +1284,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if index is None: raise ItemNotFoundError(package_id) # this is the only real delete in the system. should it do something else? + log.info("deleting course from split-mongo: %s", package_id) self.db_connection.delete_course_index(index['_id']) def get_errored_courses(self):