From 9e6249ddd6777141d82f26a852d65d7abf34fd0e Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 12:47:10 -0500 Subject: [PATCH 01/22] Enable split for new courses. --- cms/djangoapps/contentstore/views/course.py | 5 +-- cms/envs/common.py | 40 +++++++++++++++++---- lms/envs/common.py | 22 ++++++------ 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 1b62db5842..ef2c648f59 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -664,10 +664,7 @@ def _create_new_course(request, org, number, run, fields): Returns the URL for the course overview page. Raises DuplicateCourseError if the course already exists """ - store_for_new_course = ( - settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE') or - modulestore().default_modulestore.get_modulestore_type() - ) + store_for_new_course = modulestore().default_modulestore.get_modulestore_type() new_course = create_new_course_in_store(store_for_new_course, request.user, org, number, run, fields) return JsonResponse({ 'url': reverse_course_url('course_handler', new_course.id), diff --git a/cms/envs/common.py b/cms/envs/common.py index 9404427b0f..fbc8eb311a 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -35,7 +35,7 @@ import sys import lms.envs.common # Although this module itself may not use these imported variables, other dependent modules may. from lms.envs.common import ( - USE_TZ, TECH_SUPPORT_EMAIL, PLATFORM_NAME, BUGS_EMAIL, DOC_STORE_CONFIG, ALL_LANGUAGES, WIKI_ENABLED, MODULESTORE, + USE_TZ, TECH_SUPPORT_EMAIL, PLATFORM_NAME, BUGS_EMAIL, DOC_STORE_CONFIG, DATA_DIR, ALL_LANGUAGES, WIKI_ENABLED, update_module_store_settings, ASSET_IGNORE_REGEX, COPYRIGHT_YEAR ) from path import path @@ -112,9 +112,6 @@ FEATURES = { # Turn off Advanced Security by default 'ADVANCED_SECURITY': False, - # Modulestore to use for new courses - 'DEFAULT_STORE_FOR_NEW_COURSE': None, - # Turn off Video Upload Pipeline through Studio, by default 'ENABLE_VIDEO_UPLOAD_PIPELINE': False, @@ -130,9 +127,7 @@ FEATURES = { 'ENABLE_EDXNOTES': False, # Enable support for content libraries. Note that content libraries are - # only supported in courses using split mongo. Change the setting - # DEFAULT_STORE_FOR_NEW_COURSE to be 'split' to have future courses - # and libraries created with split. + # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': False, # Milestones application flag @@ -314,6 +309,37 @@ XBLOCK_SELECT_FUNCTION = prefer_xmodules ############################ Modulestore Configuration ################################ MODULESTORE_BRANCH = 'draft-preferred' +MODULESTORE = { + 'default': { + 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', + 'OPTIONS': { + 'mappings': {}, + 'stores': [ + { + 'NAME': 'split', + 'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': DATA_DIR, + 'render_template': 'edxmako.shortcuts.render_to_string', + } + }, + { + 'NAME': 'draft', + 'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': DATA_DIR, + 'render_template': 'edxmako.shortcuts.render_to_string', + } + } + ] + } + } +} + ############################ DJANGO_BUILTINS ################################ # Change DEBUG/TEMPLATE_DEBUG in your environment settings files, not here DEBUG = False diff --git a/lms/envs/common.py b/lms/envs/common.py index 4146828573..3be88b5932 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -626,6 +626,16 @@ MODULESTORE = { 'OPTIONS': { 'mappings': {}, 'stores': [ + { + 'NAME': 'split', + 'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': { + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + 'fs_root': DATA_DIR, + 'render_template': 'edxmako.shortcuts.render_to_string', + } + }, { 'NAME': 'draft', 'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore', @@ -643,17 +653,7 @@ MODULESTORE = { 'data_dir': DATA_DIR, 'default_class': 'xmodule.hidden_module.HiddenDescriptor', } - }, - { - 'NAME': 'split', - 'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore', - 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, - 'OPTIONS': { - 'default_class': 'xmodule.hidden_module.HiddenDescriptor', - 'fs_root': DATA_DIR, - 'render_template': 'edxmako.shortcuts.render_to_string', - } - }, + } ] } } From 4e6c53271719b8f9cbd56ff9d213f347dbc837ec Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 14:16:39 -0500 Subject: [PATCH 02/22] Force usage of the old mongo modulestore. The way that assets are accessed (the usage of _id) seems specific to old mongo and fails with split. --- .../management/commands/tests/test_cleanup_assets.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py b/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py index ca3288307d..e51a34ea2d 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py @@ -8,6 +8,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.contentstore.content import XASSET_LOCATION_TAG from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import modulestore +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.mongo.base import location_to_query from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.xml_importer import import_from_xml @@ -25,7 +26,8 @@ class ExportAllCourses(ModuleStoreTestCase): super(ExportAllCourses, self).setUp() self.content_store = contentstore() - self.module_store = modulestore() + # pylint: disable=protected-access + self.module_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) def test_export_all_courses(self): """ From e66e2eda209d2275c3a046177490abc665316383 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 14:28:45 -0500 Subject: [PATCH 03/22] Use old mongo store to create courses. export_courses_to_output_path uses old mongo locator to create directory structure. --- .../management/commands/tests/test_export_all_courses.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py b/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py index 96def7711b..79282ac32a 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_export_all_courses.py @@ -21,8 +21,12 @@ class ExportAllCourses(ModuleStoreTestCase): super(ExportAllCourses, self).setUp() self.store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) self.temp_dir = mkdtemp() - self.first_course = CourseFactory.create(org="test", course="course1", display_name="run1") - self.second_course = CourseFactory.create(org="test", course="course2", display_name="run2") + self.first_course = CourseFactory.create( + org="test", course="course1", display_name="run1", default_store=ModuleStoreEnum.Type.mongo + ) + self.second_course = CourseFactory.create( + org="test", course="course2", display_name="run2", default_store=ModuleStoreEnum.Type.mongo + ) def test_export_all_courses(self): """ From 29e235213c915b7cc0217fbf8156a9b28c9db6a0 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 14:43:56 -0500 Subject: [PATCH 04/22] Update for default store being split. Note that now 2 courses can be imported where the only difference between them is run. --- .../management/commands/tests/test_import.py | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_import.py b/cms/djangoapps/contentstore/management/commands/tests/test_import.py index da9e875719..f613c27875 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_import.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_import.py @@ -12,7 +12,6 @@ from django.core.management import call_command from django_comment_common.utils import are_permissions_roles_seeded from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from opaque_keys.edx.locations import SlashSeparatedCourseKey class TestImport(ModuleStoreTestCase): @@ -20,10 +19,6 @@ class TestImport(ModuleStoreTestCase): Unit tests for importing a course from command line """ - BASE_COURSE_KEY = SlashSeparatedCourseKey(u'edX', u'test_import_course', u'2013_Spring') - DIFF_KEY = SlashSeparatedCourseKey(u'edX', u'test_import_course', u'2014_Spring') - TRUNCATED_KEY = SlashSeparatedCourseKey(u'edX', u'test_import', u'2014_Spring') - def create_course_xml(self, content_dir, course_id): directory = tempfile.mkdtemp(dir=content_dir) os.makedirs(os.path.join(directory, "course")) @@ -44,38 +39,23 @@ class TestImport(ModuleStoreTestCase): self.content_dir = path(tempfile.mkdtemp()) self.addCleanup(shutil.rmtree, self.content_dir) - # Create good course xml - self.good_dir = self.create_course_xml(self.content_dir, self.BASE_COURSE_KEY) + self.base_course_key = self.store.make_course_key(u'edX', u'test_import_course', u'2013_Spring') + self.truncated_key = self.store.make_course_key(u'edX', u'test_import', u'2014_Spring') - # Create run changed course xml - self.dupe_dir = self.create_course_xml(self.content_dir, self.DIFF_KEY) + # Create good course xml + self.good_dir = self.create_course_xml(self.content_dir, self.base_course_key) # Create course XML where TRUNCATED_COURSE.org == BASE_COURSE_ID.org # and BASE_COURSE_ID.startswith(TRUNCATED_COURSE.course) - self.course_dir = self.create_course_xml(self.content_dir, self.TRUNCATED_KEY) + self.course_dir = self.create_course_xml(self.content_dir, self.truncated_key) def test_forum_seed(self): """ Tests that forum roles were created with import. """ - self.assertFalse(are_permissions_roles_seeded(self.BASE_COURSE_KEY)) + self.assertFalse(are_permissions_roles_seeded(self.base_course_key)) call_command('import', self.content_dir, self.good_dir) - self.assertTrue(are_permissions_roles_seeded(self.BASE_COURSE_KEY)) - - def test_duplicate_with_url(self): - """ - Check to make sure an import doesn't import courses that have the - same org and course, but they have different runs in order to - prevent modulestore "findone" exceptions on deletion - """ - # Load up base course and verify it is available - call_command('import', self.content_dir, self.good_dir) - store = modulestore() - self.assertIsNotNone(store.get_course(self.BASE_COURSE_KEY)) - - # Now load up duped course and verify it doesn't load - call_command('import', self.content_dir, self.dupe_dir) - self.assertIsNone(store.get_course(self.DIFF_KEY)) + self.assertTrue(are_permissions_roles_seeded(self.base_course_key)) def test_truncated_course_with_url(self): """ @@ -87,8 +67,8 @@ class TestImport(ModuleStoreTestCase): # Load up base course and verify it is available call_command('import', self.content_dir, self.good_dir) store = modulestore() - self.assertIsNotNone(store.get_course(self.BASE_COURSE_KEY)) + self.assertIsNotNone(store.get_course(self.base_course_key)) # Now load up the course with a similar course_id and verify it loads call_command('import', self.content_dir, self.course_dir) - self.assertIsNotNone(store.get_course(self.TRUNCATED_KEY)) + self.assertIsNotNone(store.get_course(self.truncated_key)) From 5fb30f333a7c3d9e06c348b8d822978fe9c12192 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 16:39:49 -0500 Subject: [PATCH 05/22] Clean up test to work with split modulestore. Get updated xblocks from modulestore before checking properties. --- .../courseware/tests/test_group_access.py | 67 ++++++++++++------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_group_access.py b/lms/djangoapps/courseware/tests/test_group_access.py index 26a92ca0db..28793b1929 100644 --- a/lms/djangoapps/courseware/tests/test_group_access.py +++ b/lms/djangoapps/courseware/tests/test_group_access.py @@ -64,13 +64,22 @@ class GroupAccessTestCase(ModuleStoreTestCase): """ partition.scheme.set_group_for_user(user, partition, group) - def set_group_access(self, block, access_dict): + def set_group_access(self, block_location, access_dict): """ - DRY helper. + Set group_access on block specified by location. """ + block = modulestore().get_item(block_location) block.group_access = access_dict modulestore().update_item(block, 1) + def set_user_partitions(self, block_location, partitions): + """ + Sets the user_partitions on block specified by location. + """ + block = modulestore().get_item(block_location) + block.user_partitions = partitions + modulestore().update_item(block, 1) + def setUp(self): super(GroupAccessTestCase, self).setUp() @@ -117,10 +126,15 @@ class GroupAccessTestCase(ModuleStoreTestCase): self.course = CourseFactory.create( user_partitions=[self.animal_partition, self.color_partition], ) - self.chapter = ItemFactory.create(category='chapter', parent=self.course) - self.section = ItemFactory.create(category='sequential', parent=self.chapter) - self.vertical = ItemFactory.create(category='vertical', parent=self.section) - self.component = ItemFactory.create(category='problem', parent=self.vertical) + chapter = ItemFactory.create(category='chapter', parent=self.course) + section = ItemFactory.create(category='sequential', parent=chapter) + vertical = ItemFactory.create(category='vertical', parent=section) + component = ItemFactory.create(category='problem', parent=vertical) + + self.chapter_location = chapter.location + self.section_location = section.location + self.vertical_location = vertical.location + self.component_location = component.location self.red_cat = UserFactory() # student in red and cat groups self.set_user_group(self.red_cat, self.animal_partition, self.cat_group) @@ -145,15 +159,15 @@ class GroupAccessTestCase(ModuleStoreTestCase): # avoid repeatedly declaring the same sequence for ddt in all the test cases. PARENT_CHILD_PAIRS = ( - ('chapter', 'chapter'), - ('chapter', 'section'), - ('chapter', 'vertical'), - ('chapter', 'component'), - ('section', 'section'), - ('section', 'vertical'), - ('section', 'component'), - ('vertical', 'vertical'), - ('vertical', 'component'), + ('chapter_location', 'chapter_location'), + ('chapter_location', 'section_location'), + ('chapter_location', 'vertical_location'), + ('chapter_location', 'component_location'), + ('section_location', 'section_location'), + ('section_location', 'vertical_location'), + ('section_location', 'component_location'), + ('vertical_location', 'vertical_location'), + ('vertical_location', 'component_location'), ) def tearDown(self): @@ -163,19 +177,20 @@ class GroupAccessTestCase(ModuleStoreTestCase): """ UserPartition.scheme_extensions = None - def check_access(self, user, block, is_accessible): + def check_access(self, user, block_location, is_accessible): """ DRY helper. """ self.assertIs( - access.has_access(user, 'load', block, self.course.id), + access.has_access(user, 'load', modulestore().get_item(block_location), self.course.id), is_accessible ) - def ensure_staff_access(self, block): + def ensure_staff_access(self, block_location): """ Another DRY helper. """ + block = modulestore().get_item(block_location) self.assertTrue(access.has_access(self.staff, 'load', block, self.course.id)) # NOTE: in all the tests that follow, `block_specified` and @@ -396,12 +411,12 @@ class GroupAccessTestCase(ModuleStoreTestCase): except user_partitions in use by the split_test module. """ # Initially, "red_cat" user can't view the vertical. - self.set_group_access(self.chapter, {self.animal_partition.id: [self.dog_group.id]}) - self.check_access(self.red_cat, self.vertical, False) + self.set_group_access(self.chapter_location, {self.animal_partition.id: [self.dog_group.id]}) + self.check_access(self.red_cat, self.vertical_location, False) # Change the vertical's user_partitions value to the empty list. Now red_cat can view the vertical. - self.vertical.user_partitions = [] - self.check_access(self.red_cat, self.vertical, True) + self.set_user_partitions(self.vertical_location, []) + self.check_access(self.red_cat, self.vertical_location, True) # Change the vertical's user_partitions value to include only "split_test" partitions. split_test_partition = UserPartition( @@ -411,9 +426,9 @@ class GroupAccessTestCase(ModuleStoreTestCase): [Group(2, 'random group')], scheme=UserPartition.get_scheme("random"), ) - self.vertical.user_partitions = [split_test_partition] - self.check_access(self.red_cat, self.vertical, True) + self.set_user_partitions(self.vertical_location, [split_test_partition]) + self.check_access(self.red_cat, self.vertical_location, True) # Finally, add back in a cohort user_partition - self.vertical.user_partitions = [split_test_partition, self.animal_partition] - self.check_access(self.red_cat, self.vertical, False) + self.set_user_partitions(self.vertical_location, [split_test_partition, self.animal_partition]) + self.check_access(self.red_cat, self.vertical_location, False) From 6272817c070b08b484bbb5efec15a89d822dd3cd Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 20:43:18 -0500 Subject: [PATCH 06/22] Tests fail with split modulestore. Have not been able to work out why. --- common/djangoapps/contentserver/tests/test.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/contentserver/tests/test.py b/common/djangoapps/contentserver/tests/test.py index 9aa5204920..a666909402 100644 --- a/common/djangoapps/contentserver/tests/test.py +++ b/common/djangoapps/contentserver/tests/test.py @@ -13,8 +13,8 @@ from django.test.utils import override_settings from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import modulestore -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.xml_importer import import_from_xml from contentserver.middleware import parse_range_header @@ -45,22 +45,23 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.client = Client() self.contentstore = contentstore() + store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) # pylint: disable=protected-access - self.course_key = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + self.course_key = store.make_course_key('edX', 'toy', '2012_Fall') import_from_xml( - modulestore(), self.user.id, TEST_DATA_DIR, ['toy'], + store, self.user.id, TEST_DATA_DIR, ['toy'], static_content_store=self.contentstore, verbose=True ) # A locked asset self.locked_asset = self.course_key.make_asset_key('asset', 'sample_static.txt') - self.url_locked = self.locked_asset.to_deprecated_string() + self.url_locked = unicode(self.locked_asset) self.contentstore.set_attr(self.locked_asset, 'locked', True) # An unlocked asset self.unlocked_asset = self.course_key.make_asset_key('asset', 'another_static.txt') - self.url_unlocked = self.unlocked_asset.to_deprecated_string() + self.url_unlocked = unicode(self.unlocked_asset) self.length_unlocked = self.contentstore.get_attr(self.unlocked_asset, 'length') def test_unlocked_asset(self): From 5b0d150fe94112b7849b2273ce0e415e9b40b37a Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 21:20:05 -0500 Subject: [PATCH 07/22] Update to work with split_modulestore. Cannot hold on to xblock references. --- lms/lib/xblock/test/test_mixin.py | 115 ++++++++++++++---------------- 1 file changed, 54 insertions(+), 61 deletions(-) diff --git a/lms/lib/xblock/test/test_mixin.py b/lms/lib/xblock/test/test_mixin.py index ebc713e7e7..014da754b9 100644 --- a/lms/lib/xblock/test/test_mixin.py +++ b/lms/lib/xblock/test/test_mixin.py @@ -34,10 +34,22 @@ class LmsXBlockMixinTestCase(ModuleStoreTestCase): self.group1 = self.user_partition.groups[0] # pylint: disable=no-member self.group2 = self.user_partition.groups[1] # pylint: disable=no-member self.course = CourseFactory.create(user_partitions=[self.user_partition]) - self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section') - self.subsection = ItemFactory.create(parent=self.section, category='sequential', display_name='Test Subsection') - self.vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='Test Unit') - self.video = ItemFactory.create(parent=self.vertical, category='video', display_name='Test Video 1') + section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section') + subsection = ItemFactory.create(parent=section, category='sequential', display_name='Test Subsection') + vertical = ItemFactory.create(parent=subsection, category='vertical', display_name='Test Unit') + video = ItemFactory.create(parent=vertical, category='video', display_name='Test Video 1') + self.section_location = section.location + self.subsection_location = subsection.location + self.vertical_location = vertical.location + self.video_location = video.location + + def set_group_access(self, block_location, access_dict): + """ + Sets the group_access dict on the block referenced by block_location. + """ + block = self.store.get_item(block_location) + block.group_access = access_dict + self.store.update_item(block, 1) class XBlockValidationTest(LmsXBlockMixinTestCase): @@ -59,23 +71,23 @@ class XBlockValidationTest(LmsXBlockMixinTestCase): """ Test the validation messages produced for an xblock with full group access. """ - validation = self.video.validate() + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 0) def test_validate_restricted_group_access(self): """ Test the validation messages produced for an xblock with a valid group access restriction """ - self.video.group_access[self.user_partition.id] = [self.group1.id, self.group2.id] # pylint: disable=no-member - validation = self.video.validate() + self.set_group_access(self.video_location, {self.user_partition.id: [self.group1.id, self.group2.id]}) + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 0) def test_validate_invalid_user_partitions(self): """ Test the validation messages produced for an xblock referring to non-existent user partitions. """ - self.video.group_access[999] = [self.group1.id] - validation = self.video.validate() + self.set_group_access(self.video_location, {999: [self.group1.id]}) + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 1) self.verify_validation_message( validation.messages[0], @@ -86,8 +98,8 @@ class XBlockValidationTest(LmsXBlockMixinTestCase): # Now add a second invalid user partition and validate again. # Note that even though there are two invalid configurations, # only a single error message will be returned. - self.video.group_access[998] = [self.group2.id] - validation = self.video.validate() + self.set_group_access(self.video_location, {998: [self.group2.id]}) + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 1) self.verify_validation_message( validation.messages[0], @@ -99,8 +111,8 @@ class XBlockValidationTest(LmsXBlockMixinTestCase): """ Test the validation messages produced for an xblock referring to non-existent groups. """ - self.video.group_access[self.user_partition.id] = [self.group1.id, 999] # pylint: disable=no-member - validation = self.video.validate() + self.set_group_access(self.video_location, {self.user_partition.id: [self.group1.id, 999]}) + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 1) self.verify_validation_message( validation.messages[0], @@ -109,8 +121,8 @@ class XBlockValidationTest(LmsXBlockMixinTestCase): ) # Now try again with two invalid group ids - self.video.group_access[self.user_partition.id] = [self.group1.id, 998, 999] # pylint: disable=no-member - validation = self.video.validate() + self.set_group_access(self.video_location, {self.user_partition.id: [self.group1.id, 998, 999]}) + validation = self.store.get_item(self.video_location).validate() self.assertEqual(len(validation.messages), 1) self.verify_validation_message( validation.messages[0], @@ -184,10 +196,11 @@ class XBlockGetParentTest(LmsXBlockMixinTestCase): # move the video to the new vertical with self.store.default_store(modulestore_type): self.build_course() - new_vertical = ItemFactory.create(parent=self.subsection, category='vertical', display_name='New Test Unit') - child_to_move_location = self.video.location.for_branch(None) + subsection = self.store.get_item(self.subsection_location) + new_vertical = ItemFactory.create(parent=subsection, category='vertical', display_name='New Test Unit') + child_to_move_location = self.video_location.for_branch(None) new_parent_location = new_vertical.location.for_branch(None) - old_parent_location = self.vertical.location.for_branch(None) + old_parent_location = self.vertical_location.for_branch(None) with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): self.assertIsNone(self.course.get_parent()) @@ -252,23 +265,23 @@ class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): PARTITION_2_GROUP_2 = 22 PARENT_CHILD_PAIRS = ( - ddt_named('section', 'subsection'), - ddt_named('section', 'vertical'), - ddt_named('section', 'video'), - ddt_named('subsection', 'vertical'), - ddt_named('subsection', 'video'), + ddt_named('section_location', 'subsection_location'), + ddt_named('section_location', 'vertical_location'), + ddt_named('section_location', 'video_location'), + ddt_named('subsection_location', 'vertical_location'), + ddt_named('subsection_location', 'video_location'), ) def setUp(self): super(XBlockMergedGroupAccessTest, self).setUp() self.build_course() - def set_group_access(self, block, access_dict): + def verify_group_access(self, block_location, expected_dict): """ - DRY helper. + Verify the expected value for the block's group_access. """ - block.group_access = access_dict - block.runtime.modulestore.update_item(block, 1) + block = self.store.get_item(block_location) + self.assertEqual(block.merged_group_access, expected_dict) @ddt.data(*PARENT_CHILD_PAIRS) @ddt.unpack @@ -284,14 +297,8 @@ class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}) self.set_group_access(child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) - self.assertEqual( - parent_block.merged_group_access, - {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}, - ) - self.assertEqual( - child_block.merged_group_access, - {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}, - ) + self.verify_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}) + self.verify_group_access(child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) @ddt.data(*PARENT_CHILD_PAIRS) @ddt.unpack @@ -306,14 +313,8 @@ class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) self.set_group_access(child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) - self.assertEqual( - parent_block.merged_group_access, - {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}, - ) - self.assertEqual( - child_block.merged_group_access, - {self.PARTITION_1: False}, - ) + self.verify_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.verify_group_access(child_block, {self.PARTITION_1: False}) def test_disjoint_groups_no_override(self): """ @@ -321,18 +322,14 @@ class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): to the block being queried even if blocks further down in the hierarchy try to override it. """ - self.set_group_access(self.section, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) - self.set_group_access(self.subsection, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) - self.set_group_access(self.vertical, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]}) + self.set_group_access(self.section_location, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.set_group_access(self.subsection_location, {self.PARTITION_1: [self.PARTITION_1_GROUP_2]}) + self.set_group_access( + self.vertical_location, {self.PARTITION_1: [self.PARTITION_1_GROUP_1, self.PARTITION_1_GROUP_2]} + ) - self.assertEqual( - self.vertical.merged_group_access, - {self.PARTITION_1: False}, - ) - self.assertEqual( - self.video.merged_group_access, - {self.PARTITION_1: False}, - ) + self.verify_group_access(self.vertical_location, {self.PARTITION_1: False}) + self.verify_group_access(self.video_location, {self.PARTITION_1: False}) @ddt.data(*PARENT_CHILD_PAIRS) @ddt.unpack @@ -348,11 +345,7 @@ class XBlockMergedGroupAccessTest(LmsXBlockMixinTestCase): self.set_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) self.set_group_access(child_block, {self.PARTITION_2: [self.PARTITION_1_GROUP_2]}) - self.assertEqual( - parent_block.merged_group_access, - {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}, - ) - self.assertEqual( - child_block.merged_group_access, - {self.PARTITION_1: [self.PARTITION_1_GROUP_1], self.PARTITION_2: [self.PARTITION_1_GROUP_2]}, + self.verify_group_access(parent_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1]}) + self.verify_group_access( + child_block, {self.PARTITION_1: [self.PARTITION_1_GROUP_1], self.PARTITION_2: [self.PARTITION_1_GROUP_2]} ) From 7a3a48b15b8af98d9ed6a80e4da02d7ad7929e72 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 21:34:02 -0500 Subject: [PATCH 08/22] Update to work with split_modulestore. "run" should not be passed in kwargs to create. --- common/lib/xmodule/xmodule/modulestore/tests/factories.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 2e0965fd91..4e7b1063c3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -102,7 +102,7 @@ class CourseFactory(XModuleFactory): number = kwargs.pop('course', kwargs.pop('number', None)) store = kwargs.pop('modulestore') name = kwargs.get('name', kwargs.get('run', Location.clean(kwargs.get('display_name')))) - run = kwargs.get('run', name) + run = kwargs.pop('run', name) user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test) # Pass the metadata just as field=value pairs From e9f2f626ca64f55d00a32ea0d8d4a0848c2c70db Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 26 Jan 2015 22:02:49 -0500 Subject: [PATCH 09/22] Change to work with split modulestore. make_asset_key does not start with "/" for split locators. --- .../contentstore/tests/test_utils.py | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 37c68a88d5..1b1022122b 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -168,28 +168,38 @@ class ExtraPanelTabTestCase(TestCase): class CourseImageTestCase(ModuleStoreTestCase): """Tests for course image URLs.""" + def verify_url(self, expected_url, actual_url): + """ + Helper method for verifying the URL is as expected. + """ + if not expected_url.startswith("/"): + expected_url = "/" + expected_url + self.assertEquals(expected_url, actual_url) + def test_get_image_url(self): """Test image URL formatting.""" course = CourseFactory.create() - url = utils.course_image_url(course) - self.assertEquals(url, unicode(course.id.make_asset_key('asset', course.course_image))) + self.verify_url( + unicode(course.id.make_asset_key('asset', course.course_image)), + utils.course_image_url(course) + ) def test_non_ascii_image_name(self): """ Verify that non-ascii image names are cleaned """ course_image = u'before_\N{SNOWMAN}_after.jpg' course = CourseFactory.create(course_image=course_image) - self.assertEquals( - utils.course_image_url(course), - unicode(course.id.make_asset_key('asset', course_image.replace(u'\N{SNOWMAN}', '_'))) + self.verify_url( + unicode(course.id.make_asset_key('asset', course_image.replace(u'\N{SNOWMAN}', '_'))), + utils.course_image_url(course) ) def test_spaces_in_image_name(self): """ Verify that image names with spaces in them are cleaned """ course_image = u'before after.jpg' course = CourseFactory.create(course_image=u'before after.jpg') - self.assertEquals( - utils.course_image_url(course), - unicode(course.id.make_asset_key('asset', course_image.replace(" ", "_"))) + self.verify_url( + unicode(course.id.make_asset_key('asset', course_image.replace(" ", "_"))), + utils.course_image_url(course) ) From 818a9508153f6972db23c0a46f6b448da4f65976 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 08:09:59 -0500 Subject: [PATCH 10/22] Change to work with split modulestore. Cannot create an xblock in a course that doesn't exist. --- cms/djangoapps/contentstore/tests/test_utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 1b1022122b..f7faea20ee 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -212,6 +212,7 @@ class XBlockVisibilityTestCase(TestCase): self.dummy_user = ModuleStoreEnum.UserID.test self.past = datetime(1970, 1, 1) self.future = datetime.now(UTC) + timedelta(days=1) + self.course = CourseFactory.create() def test_private_unreleased_xblock(self): """Verifies that a private unreleased xblock is not visible""" @@ -263,10 +264,9 @@ class XBlockVisibilityTestCase(TestCase): def _create_xblock_with_start_date(self, name, start_date, publish=False, visible_to_staff_only=False): """Helper to create an xblock with a start date, optionally publishing it""" - course_key = CourseLocator('edX', 'visibility', '2012_Fall') vertical = modulestore().create_item( - self.dummy_user, course_key, 'vertical', name, + self.dummy_user, self.course.location.course_key, 'vertical', name, fields={'start': start_date, 'visible_to_staff_only': visible_to_staff_only} ) From b5e798062bdf8d4aebe74f01bae778616d3c6871 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 08:15:35 -0500 Subject: [PATCH 11/22] Specify old mongo modulestore. Tests verify operation counts for old mongo. Should be extended to split in the future. --- cms/djangoapps/contentstore/tests/test_course_listing.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 9fc19fe155..fe489f98a6 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -53,7 +53,8 @@ class TestCourseListing(ModuleStoreTestCase): course = CourseFactory.create( org=course_location.org, number=course_location.course, - run=course_location.run + run=course_location.run, + default_store=ModuleStoreEnum.Type.mongo ) if user is not None: From 840b42a9f1658d82071e02aee495770248aa21e0 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 08:17:38 -0500 Subject: [PATCH 12/22] Specify old mongo modulestore. Testing migration from old mongo to split. --- .../management/commands/tests/test_migrate_to_split.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 def308c08b..49805090f4 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 @@ -60,7 +60,7 @@ class TestMigrateToSplit(ModuleStoreTestCase): def setUp(self): super(TestMigrateToSplit, self).setUp(create_user=True) - self.course = CourseFactory() + self.course = CourseFactory(default_store=ModuleStoreEnum.Type.mongo) def test_user_email(self): """ From f5d3d6050d873d16faec658ff7c7cc04e3af811b Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 09:42:34 -0500 Subject: [PATCH 13/22] Test of draft content depends on old mongo. --- .../contentstore/tests/test_import_pure_xblock.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py index e2b5fd73cc..af821fcff2 100644 --- a/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py +++ b/cms/djangoapps/contentstore/tests/test_import_pure_xblock.py @@ -5,6 +5,8 @@ Integration tests for importing courses containing pure XBlocks. from xblock.core import XBlock from xblock.fields import String +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.xml_importer import import_from_xml from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.mongo.draft import as_draft @@ -60,8 +62,11 @@ class XBlockImportTest(ModuleStoreTestCase): the expected field value set. """ + # It is necessary to use the "old mongo" modulestore because split doesn't work + # with the "has_draft" logic below. + store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) # pylint: disable=protected-access courses = import_from_xml( - self.store, self.user.id, TEST_DATA_DIR, [course_dir] + store, self.user.id, TEST_DATA_DIR, [course_dir], create_course_if_not_present=True ) xblock_location = courses[0].id.make_usage_key('stubxblock', 'xblock_test') @@ -69,12 +74,12 @@ class XBlockImportTest(ModuleStoreTestCase): if has_draft: xblock_location = as_draft(xblock_location) - xblock = self.store.get_item(xblock_location) + xblock = store.get_item(xblock_location) self.assertTrue(isinstance(xblock, StubXBlock)) self.assertEqual(xblock.test_field, expected_field_val) if has_draft: - draft_xblock = self.store.get_item(xblock_location) + draft_xblock = store.get_item(xblock_location) self.assertTrue(getattr(draft_xblock, 'is_draft', False)) self.assertTrue(isinstance(draft_xblock, StubXBlock)) self.assertEqual(draft_xblock.test_field, expected_field_val) From 040f96223cbf8a7ebc746c2327fb3344cb9a5923 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 09:15:21 -0500 Subject: [PATCH 14/22] Update for split modulestore as default. --- .../contentstore/tests/test_import.py | 52 +++++++++++-------- .../tests/test_import_draft_order.py | 4 +- .../contentstore/tests/test_permissions.py | 5 +- .../tests/test_users_default_role.py | 3 +- cms/lib/xblock/test/test_authoring_mixin.py | 46 +++++++++------- .../student/tests/test_course_listing.py | 35 +++++++------ .../courseware/tests/test_courses.py | 4 +- .../instructor_analytics/tests/test_basic.py | 5 +- .../mobile_api/course_info/tests.py | 5 ++ lms/djangoapps/mobile_api/testutils.py | 2 +- 10 files changed, 91 insertions(+), 70 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 4297df6d2c..bf75b96f2b 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -16,7 +16,6 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.contentstore.django import contentstore from xmodule.modulestore.tests.factories import check_exact_number_of_calls, check_number_of_calls -from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation from xmodule.modulestore.xml_importer import import_from_xml from xmodule.exceptions import NotFoundError from uuid import uuid4 @@ -40,13 +39,14 @@ class ContentStoreImportTest(ModuleStoreTestCase): self.client = Client() self.client.login(username=self.user.username, password=password) - def load_test_import_course(self, target_course_id=None, create_new_course_if_not_present=False): + def load_test_import_course(self, target_course_id=None, create_course_if_not_present=True, module_store=None): ''' Load the standard course used to test imports (for do_import_static=False behavior). ''' content_store = contentstore() - module_store = modulestore() + if module_store is None: + module_store = modulestore() import_from_xml( module_store, self.user.id, @@ -56,7 +56,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): do_import_static=False, verbose=True, target_course_id=target_course_id, - create_course_if_not_present=create_new_course_if_not_present, + create_course_if_not_present=create_course_if_not_present, ) course_id = module_store.make_course_key('edX', 'test_import_course', '2012_Fall') course = module_store.get_course(course_id) @@ -84,13 +84,14 @@ class ContentStoreImportTest(ModuleStoreTestCase): # Test that importing course with unicode 'id' and 'display name' doesn't give UnicodeEncodeError """ module_store = modulestore() - course_id = SlashSeparatedCourseKey(u'Юникода', u'unicode_course', u'échantillon') + course_id = module_store.make_course_key(u'Юникода', u'unicode_course', u'échantillon') import_from_xml( module_store, self.user.id, TEST_DATA_DIR, ['2014_Uni'], - target_course_id=course_id + target_course_id=course_id, + create_course_if_not_present=True ) course = module_store.get_course(course_id) @@ -113,9 +114,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): content = None try: - location = AssetLocation.from_deprecated_string( - '/c4x/edX/test_import_course/asset/should_be_imported.html' - ) + location = course.id.make_asset_key('asset', 'should_be_imported.html') content = content_store.find(location) except NotFoundError: pass @@ -133,9 +132,13 @@ class ContentStoreImportTest(ModuleStoreTestCase): content_store = contentstore() module_store = modulestore() - import_from_xml(module_store, self.user.id, TEST_DATA_DIR, ['toy'], static_content_store=content_store, do_import_static=False, verbose=True) + import_from_xml( + module_store, self.user.id, TEST_DATA_DIR, ['toy'], + static_content_store=content_store, do_import_static=False, + create_course_if_not_present=True, verbose=True + ) - course = module_store.get_course(SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')) + course = module_store.get_course(module_store.make_course_key('edX', 'toy', '2012_Fall')) # make sure we have NO assets in our contentstore all_assets, count = content_store.get_all_content_for_course(course.id) @@ -144,7 +147,10 @@ class ContentStoreImportTest(ModuleStoreTestCase): def test_no_static_link_rewrites_on_import(self): module_store = modulestore() - courses = import_from_xml(module_store, self.user.id, TEST_DATA_DIR, ['toy'], do_import_static=False, verbose=True) + courses = import_from_xml( + module_store, self.user.id, TEST_DATA_DIR, ['toy'], do_import_static=False, verbose=True, + create_course_if_not_present=True + ) course_key = courses[0].id handouts = module_store.get_item(course_key.make_usage_key('course_info', 'handouts')) @@ -171,17 +177,19 @@ class ContentStoreImportTest(ModuleStoreTestCase): # NOTE: On Jenkins, with memcache enabled, the number of calls here is only 1. # Locally, without memcache, the number of calls is actually 2 (once more during the publish step) with check_number_of_calls(store, '_compute_metadata_inheritance_tree', 2): - self.load_test_import_course() + self.load_test_import_course(create_course_if_not_present=False, module_store=store) @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_reimport(self, default_ms_type): with modulestore().default_store(default_ms_type): - __, __, course = self.load_test_import_course(create_new_course_if_not_present=True) + __, __, course = self.load_test_import_course(create_course_if_not_present=True) self.load_test_import_course(target_course_id=course.id) def test_rewrite_reference_list(self): - module_store = modulestore() - target_course_id = SlashSeparatedCourseKey('testX', 'conditional_copy', 'copy_run') + # This test fails with split modulestore (the HTML component is not in "different_course_id" namespace). + # More investigation needs to be done. + module_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) + target_course_id = module_store.make_course_key('testX', 'conditional_copy', 'copy_run') import_from_xml( module_store, self.user.id, @@ -193,7 +201,7 @@ class ContentStoreImportTest(ModuleStoreTestCase): target_course_id.make_usage_key('conditional', 'condone') ) self.assertIsNotNone(conditional_module) - different_course_id = SlashSeparatedCourseKey('edX', 'different_course', None) + different_course_id = module_store.make_course_key('edX', 'different_course', None) self.assertListEqual( [ target_course_id.make_usage_key('problem', 'choiceprob'), @@ -211,13 +219,14 @@ class ContentStoreImportTest(ModuleStoreTestCase): def test_rewrite_reference(self): module_store = modulestore() - target_course_id = SlashSeparatedCourseKey('testX', 'peergrading_copy', 'copy_run') + target_course_id = module_store.make_course_key('testX', 'peergrading_copy', 'copy_run') import_from_xml( module_store, self.user.id, TEST_DATA_DIR, ['open_ended'], - target_course_id=target_course_id + target_course_id=target_course_id, + create_course_if_not_present=True ) peergrading_module = module_store.get_item( target_course_id.make_usage_key('peergrading', 'PeerGradingLinked') @@ -252,13 +261,14 @@ class ContentStoreImportTest(ModuleStoreTestCase): def _verify_split_test_import(self, target_course_name, source_course_name, split_test_name, groups_to_verticals): module_store = modulestore() - target_course_id = SlashSeparatedCourseKey('testX', target_course_name, 'copy_run') + target_course_id = module_store.make_course_key('testX', target_course_name, 'copy_run') import_from_xml( module_store, self.user.id, TEST_DATA_DIR, [source_course_name], - target_course_id=target_course_id + target_course_id=target_course_id, + create_course_if_not_present=True ) split_test_module = module_store.get_item( target_course_id.make_usage_key('split_test', split_test_name) diff --git a/cms/djangoapps/contentstore/tests/test_import_draft_order.py b/cms/djangoapps/contentstore/tests/test_import_draft_order.py index 4f3e2c1d3a..29484ea6a5 100644 --- a/cms/djangoapps/contentstore/tests/test_import_draft_order.py +++ b/cms/djangoapps/contentstore/tests/test_import_draft_order.py @@ -13,7 +13,9 @@ class DraftReorderTestCase(ModuleStoreTestCase): def test_order(self): store = modulestore() - course_items = import_from_xml(store, self.user.id, TEST_DATA_DIR, ['import_draft_order']) + course_items = import_from_xml( + store, self.user.id, TEST_DATA_DIR, ['import_draft_order'], create_course_if_not_present=True + ) course_key = course_items[0].id sequential = store.get_item(course_key.make_usage_key('sequential', '0f4f7649b10141b0bdc9922dcf94515a')) verticals = sequential.children diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py index 5a2274cf1c..f509e807f2 100644 --- a/cms/djangoapps/contentstore/tests/test_permissions.py +++ b/cms/djangoapps/contentstore/tests/test_permissions.py @@ -7,7 +7,6 @@ from django.contrib.auth.models import User from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from contentstore.tests.utils import AjaxEnabledTestClient -from opaque_keys.edx.locations import SlashSeparatedCourseKey from contentstore.utils import reverse_url, reverse_course_url from student.roles import CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole from student import auth @@ -29,7 +28,7 @@ class TestCourseAccess(ModuleStoreTestCase): self.client.login(username=self.user.username, password=user_password) # create a course via the view handler which has a different strategy for permissions than the factory - self.course_key = SlashSeparatedCourseKey('myu', 'mydept.mycourse', 'myrun') + self.course_key = self.store.make_course_key('myu', 'mydept.mycourse', 'myrun') course_url = reverse_url('course_handler') self.client.ajax_post( course_url, @@ -101,7 +100,7 @@ class TestCourseAccess(ModuleStoreTestCase): self.assertContains(response, user.email) # test copying course permissions - copy_course_key = SlashSeparatedCourseKey('copyu', 'copydept.mycourse', 'myrun') + copy_course_key = self.store.make_course_key('copyu', 'copydept.mycourse', 'myrun') for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]: if (role is OrgStaffRole) or (role is OrgInstructorRole): auth.add_users( diff --git a/cms/djangoapps/contentstore/tests/test_users_default_role.py b/cms/djangoapps/contentstore/tests/test_users_default_role.py index c02b6574f1..c4a1bed05f 100644 --- a/cms/djangoapps/contentstore/tests/test_users_default_role.py +++ b/cms/djangoapps/contentstore/tests/test_users_default_role.py @@ -6,7 +6,6 @@ from contentstore.tests.utils import AjaxEnabledTestClient from contentstore.utils import delete_course_and_groups, reverse_url from courseware.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from opaque_keys.edx.locations import SlashSeparatedCourseKey from student.models import CourseEnrollment @@ -26,7 +25,7 @@ class TestUsersDefaultRole(ModuleStoreTestCase): self.client.login(username=self.user.username, password='test') # create a course via the view handler to create course - self.course_key = SlashSeparatedCourseKey('Org_1', 'Course_1', 'Run_1') + self.course_key = self.store.make_course_key('Org_1', 'Course_1', 'Run_1') self._create_course_with_given_location(self.course_key) def _create_course_with_given_location(self, course_key): diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py index 326b8cc1b8..3103eebc7c 100644 --- a/cms/lib/xblock/test/test_authoring_mixin.py +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -27,16 +27,18 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): parent_location=chapter.location, display_name='Test Sequential' ) - self.vertical = ItemFactory.create( + vertical = ItemFactory.create( category='vertical', parent_location=sequential.location, display_name='Test Vertical' ) - self.video = ItemFactory.create( + video = ItemFactory.create( category='video', - parent_location=self.vertical.location, + parent_location=vertical.location, display_name='Test Vertical' ) + self.vertical_location = vertical.location + self.video_location = video.location self.pet_groups = [Group(1, 'Cat Lovers'), Group(2, 'Dog Lovers')] def create_content_groups(self, content_groups): @@ -54,68 +56,72 @@ class AuthoringMixinTestCase(ModuleStoreTestCase): self.course.user_partitions = [self.content_partition] self.store.update_item(self.course, self.user.id) - def set_staff_only(self, item): + def set_staff_only(self, item_location): """Make an item visible to staff only.""" + item = self.store.get_item(item_location) item.visible_to_staff_only = True self.store.update_item(item, self.user.id) - def set_group_access(self, item, group_ids): + def set_group_access(self, item_location, group_ids): """ Set group_access for the specified item to the specified group ids within the content partition. """ + item = self.store.get_item(item_location) item.group_access[self.content_partition.id] = group_ids # pylint: disable=no-member self.store.update_item(item, self.user.id) - def verify_visibility_view_contains(self, item, substrings): + def verify_visibility_view_contains(self, item_location, substrings): """ Verify that an item's visibility view returns an html string containing all the expected substrings. """ + item = self.store.get_item(item_location) html = item.visibility_view().body_html() for string in substrings: self.assertIn(string, html) def test_html_no_partition(self): - self.verify_visibility_view_contains(self.video, 'No content groups exist') + self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_empty_partition(self): self.create_content_groups([]) - self.verify_visibility_view_contains(self.video, 'No content groups exist') + self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_populated_partition(self): self.create_content_groups(self.pet_groups) - self.verify_visibility_view_contains(self.video, ['Cat Lovers', 'Dog Lovers']) + self.verify_visibility_view_contains(self.video_location, ['Cat Lovers', 'Dog Lovers']) def test_html_no_partition_staff_locked(self): - self.set_staff_only(self.vertical) - self.verify_visibility_view_contains(self.video, ['No content groups exist']) + self.set_staff_only(self.vertical_location) + self.verify_visibility_view_contains(self.video_location, ['No content groups exist']) def test_html_empty_partition_staff_locked(self): self.create_content_groups([]) - self.set_staff_only(self.vertical) - self.verify_visibility_view_contains(self.video, 'No content groups exist') + self.set_staff_only(self.vertical_location) + self.verify_visibility_view_contains(self.video_location, 'No content groups exist') def test_html_populated_partition_staff_locked(self): self.create_content_groups(self.pet_groups) - self.set_staff_only(self.vertical) + self.set_staff_only(self.vertical_location) self.verify_visibility_view_contains( - self.video, ['The Unit this component is contained in is hidden from students.', 'Cat Lovers', 'Dog Lovers'] + self.video_location, + ['The Unit this component is contained in is hidden from students.', 'Cat Lovers', 'Dog Lovers'] ) def test_html_false_content_group(self): self.create_content_groups(self.pet_groups) - self.set_group_access(self.video, ['false_group_id']) + self.set_group_access(self.video_location, ['false_group_id']) self.verify_visibility_view_contains( - self.video, ['Cat Lovers', 'Dog Lovers', 'Content group no longer exists.'] + self.video_location, ['Cat Lovers', 'Dog Lovers', 'Content group no longer exists.'] ) def test_html_false_content_group_staff_locked(self): self.create_content_groups(self.pet_groups) - self.set_staff_only(self.vertical) - self.set_group_access(self.video, ['false_group_id']) + self.set_staff_only(self.vertical_location) + self.set_group_access(self.video_location, ['false_group_id']) self.verify_visibility_view_contains( - self.video, + self.video_location, [ 'Cat Lovers', 'Dog Lovers', diff --git a/common/djangoapps/student/tests/test_course_listing.py b/common/djangoapps/student/tests/test_course_listing.py index 35dd4b1b80..87160c6307 100644 --- a/common/djangoapps/student/tests/test_course_listing.py +++ b/common/djangoapps/student/tests/test_course_listing.py @@ -10,13 +10,11 @@ from student.roles import GlobalStaff from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.django import modulestore from xmodule.error_module import ErrorDescriptor from django.test.client import Client from student.models import CourseEnrollment from student.views import get_course_enrollment_pairs -from opaque_keys.edx.keys import CourseKey from util.milestones_helpers import ( get_pre_requisite_courses_not_completed, set_prerequisite_courses, @@ -42,7 +40,7 @@ class TestCourseListing(ModuleStoreTestCase): self.client = Client() self.client.login(username=self.teacher.username, password='test') - def _create_course_with_access_groups(self, course_location, metadata=None): + def _create_course_with_access_groups(self, course_location, metadata=None, default_store=None): """ Create dummy course with 'CourseFactory' and enroll the student """ @@ -51,7 +49,8 @@ class TestCourseListing(ModuleStoreTestCase): org=course_location.org, number=course_location.course, run=course_location.run, - metadata=metadata + metadata=metadata, + default_store=default_store ) CourseEnrollment.enroll(self.student, course.id) @@ -70,7 +69,7 @@ class TestCourseListing(ModuleStoreTestCase): """ Test getting courses """ - course_location = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + course_location = self.store.make_course_key('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_location) # get dashboard @@ -87,8 +86,10 @@ class TestCourseListing(ModuleStoreTestCase): """ Test the course list for regular staff when get_course returns an ErrorDescriptor """ - course_key = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') - self._create_course_with_access_groups(course_key) + # pylint: disable=protected-access + mongo_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) + course_key = mongo_store.make_course_key('Org1', 'Course1', 'Run1') + self._create_course_with_access_groups(course_key, default_store=ModuleStoreEnum.Type.mongo) with patch('xmodule.modulestore.mongo.base.MongoKeyValueStore', Mock(side_effect=Exception)): self.assertIsInstance(modulestore().get_course(course_key), ErrorDescriptor) @@ -104,15 +105,15 @@ class TestCourseListing(ModuleStoreTestCase): """ mongo_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) - good_location = SlashSeparatedCourseKey('testOrg', 'testCourse', 'RunBabyRun') - self._create_course_with_access_groups(good_location) + good_location = mongo_store.make_course_key('testOrg', 'testCourse', 'RunBabyRun') + self._create_course_with_access_groups(good_location, default_store=ModuleStoreEnum.Type.mongo) - course_location = SlashSeparatedCourseKey('testOrg', 'doomedCourse', 'RunBabyRun') - self._create_course_with_access_groups(course_location) + course_location = mongo_store.make_course_key('testOrg', 'doomedCourse', 'RunBabyRun') + self._create_course_with_access_groups(course_location, default_store=ModuleStoreEnum.Type.mongo) mongo_store.delete_course(course_location, ModuleStoreEnum.UserID.test) - course_location = SlashSeparatedCourseKey('testOrg', 'erroredCourse', 'RunBabyRun') - course = self._create_course_with_access_groups(course_location) + course_location = mongo_store.make_course_key('testOrg', 'erroredCourse', 'RunBabyRun') + course = self._create_course_with_access_groups(course_location, default_store=ModuleStoreEnum.Type.mongo) course_db_record = mongo_store._find_one(course.location) course_db_record.setdefault('metadata', {}).get('tabs', []).append({ "type": "wiko", @@ -137,18 +138,18 @@ class TestCourseListing(ModuleStoreTestCase): Checks course where pre-requisite course is set has appropriate info. """ seed_milestone_relationship_types() - course_location2 = CourseKey.from_string('Org1/Course2/Run2') + course_location2 = self.store.make_course_key('Org1', 'Course2', 'Run2') self._create_course_with_access_groups(course_location2) - pre_requisite_course_location = CourseKey.from_string('Org1/Course3/Run3') + pre_requisite_course_location = self.store.make_course_key('Org1', 'Course3', 'Run3') self._create_course_with_access_groups(pre_requisite_course_location) - pre_requisite_course_location2 = CourseKey.from_string('Org1/Course4/Run4') + pre_requisite_course_location2 = self.store.make_course_key('Org1', 'Course4', 'Run4') self._create_course_with_access_groups(pre_requisite_course_location2) # create a course with pre_requisite_courses pre_requisite_courses = [ unicode(pre_requisite_course_location), unicode(pre_requisite_course_location2), ] - course_location = CourseKey.from_string('Org1/Course1/Run1') + course_location = self.store.make_course_key('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_location, { 'pre_requisite_courses': pre_requisite_courses }) diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index 541a0d61d3..ec738c44ab 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/lms/djangoapps/courseware/tests/test_courses.py @@ -43,9 +43,9 @@ class CoursesTest(ModuleStoreTestCase): org='org', number='num', display_name='name' ) - cms_url = u"//{}/course/org/num/name".format(CMS_BASE_TEST) + cms_url = u"//{}/course/{}".format(CMS_BASE_TEST, unicode(self.course.id)) self.assertEqual(cms_url, get_cms_course_link(self.course)) - cms_url = u"//{}/course/i4x://org/num/course/name".format(CMS_BASE_TEST) + cms_url = u"//{}/course/{}".format(CMS_BASE_TEST, unicode(self.course.location)) self.assertEqual(cms_url, get_cms_block_link(self.course, 'course')) diff --git a/lms/djangoapps/instructor_analytics/tests/test_basic.py b/lms/djangoapps/instructor_analytics/tests/test_basic.py index dc43788600..e40144b06b 100644 --- a/lms/djangoapps/instructor_analytics/tests/test_basic.py +++ b/lms/djangoapps/instructor_analytics/tests/test_basic.py @@ -8,7 +8,6 @@ from django.core.urlresolvers import reverse from mock import patch from student.roles import CourseSalesAdminRole from student.tests.factories import UserFactory, CourseModeFactory -from opaque_keys.edx.locations import SlashSeparatedCourseKey from shoppingcart.models import ( CourseRegistrationCode, RegistrationCodeRedemption, Order, Invoice, Coupon, CourseRegCodeItem, CouponRedemption, CourseRegistrationCodeInvoiceItem @@ -33,7 +32,7 @@ class TestAnalyticsBasic(ModuleStoreTestCase): def setUp(self): super(TestAnalyticsBasic, self).setUp() - self.course_key = SlashSeparatedCourseKey('robot', 'course', 'id') + self.course_key = self.store.make_course_key('robot', 'course', 'id') self.users = tuple(UserFactory() for _ in xrange(30)) self.ces = tuple(CourseEnrollment.enroll(user, self.course_key) for user in self.users) @@ -80,7 +79,7 @@ class TestAnalyticsBasic(ModuleStoreTestCase): self.assertIn(userreport['meta.company'], ["Open edX Inc {}".format(user.id) for user in self.users]) def test_enrolled_students_features_keys_cohorted(self): - course = CourseFactory.create(course_key=self.course_key) + course = CourseFactory.create(org="test", course="course1", display_name="run1") course.cohort_config = {'cohorted': True, 'auto_cohort': True, 'auto_cohort_groups': ['cohort']} self.store.update_item(course, self.instructor.id) cohort = CohortFactory.create(name='cohort', course_id=course.id) diff --git a/lms/djangoapps/mobile_api/course_info/tests.py b/lms/djangoapps/mobile_api/course_info/tests.py index 3dfb44b757..bc6dcbef7f 100644 --- a/lms/djangoapps/mobile_api/course_info/tests.py +++ b/lms/djangoapps/mobile_api/course_info/tests.py @@ -120,6 +120,11 @@ class TestHandouts(MobileAPITestCase, MobileAuthTestMixin, MobileEnrolledCourseA def setUp(self): super(TestHandouts, self).setUp() + # Deleting handouts fails with split modulestore because the handout has no parent. + # This needs further investigation to determine if it is a bug in the split modulestore. + # pylint: disable=protected-access + self.store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) + # use toy course with handouts, and make it mobile_available course_items = import_from_xml(self.store, self.user.id, settings.COMMON_TEST_DATA_ROOT, ['toy']) self.course = course_items[0] diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index 61e6f58faa..c5bc3f2872 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -35,7 +35,7 @@ class MobileAPITestCase(ModuleStoreTestCase, APITestCase): """ def setUp(self): super(MobileAPITestCase, self).setUp() - self.course = CourseFactory.create(mobile_available=True) + self.course = CourseFactory.create(mobile_available=True, static_asset_path="needed_for_split") self.user = UserFactory.create() self.password = 'test' self.username = self.user.username From 974eb8a28dc981b98204f2d4fb8232dd4edd86aa Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 15:22:31 -0500 Subject: [PATCH 15/22] Enforce old mongo for test cases doing locator comparisons. --- lms/djangoapps/edxnotes/tests.py | 63 +++++++++++++++++++------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index a93468ea58..1b41e26641 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -19,6 +19,7 @@ from provider.oauth2.models import Client from xmodule.tabs import EdxNotesTab from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor @@ -65,7 +66,9 @@ class EdxNotesDecoratorTest(ModuleStoreTestCase): super(EdxNotesDecoratorTest, self).setUp() ClientFactory(name="edx-notes") - self.course = CourseFactory.create(edxnotes=True) + # Using old mongo because of locator comparison issues (see longer + # note below in EdxNotesHelpersTest setUp. + self.course = CourseFactory.create(edxnotes=True, default_store=ModuleStoreEnum.Type.mongo) self.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx") self.client.login(username=self.user.username, password="edx") self.problem = TestProblem(self.course) @@ -144,34 +147,42 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): Setup a dummy course content. """ super(EdxNotesHelpersTest, self).setUp() - ClientFactory(name="edx-notes") - self.course = CourseFactory.create() - self.chapter = ItemFactory.create(category="chapter", parent_location=self.course.location) - self.chapter_2 = ItemFactory.create(category="chapter", parent_location=self.course.location) - self.sequential = ItemFactory.create(category="sequential", parent_location=self.chapter.location) - self.vertical = ItemFactory.create(category="vertical", parent_location=self.sequential.location) - self.html_module_1 = ItemFactory.create(category="html", parent_location=self.vertical.location) - self.html_module_2 = ItemFactory.create(category="html", parent_location=self.vertical.location) - self.vertical_with_container = ItemFactory.create(category='vertical', parent_location=self.sequential.location) - self.child_container = ItemFactory.create( - category='split_test', parent_location=self.vertical_with_container.location) - self.child_vertical = ItemFactory.create(category='vertical', parent_location=self.child_container.location) - self.child_html_module = ItemFactory.create(category="html", parent_location=self.child_vertical.location) - # Read again so that children lists are accurate - self.course = self.store.get_item(self.course.location) - self.chapter = self.store.get_item(self.chapter.location) - self.chapter_2 = self.store.get_item(self.chapter_2.location) - self.sequential = self.store.get_item(self.sequential.location) - self.vertical = self.store.get_item(self.vertical.location) + # There are many tests that are comparing locators as returned from helper methods. When using + # the split modulestore, some of those locators have version and branch information, but the + # comparison values do not. This needs further investigation in order to enable these tests + # with the split modulestore. + with self.store.default_store(ModuleStoreEnum.Type.mongo): + ClientFactory(name="edx-notes") + self.course = CourseFactory.create() + self.chapter = ItemFactory.create(category="chapter", parent_location=self.course.location) + self.chapter_2 = ItemFactory.create(category="chapter", parent_location=self.course.location) + self.sequential = ItemFactory.create(category="sequential", parent_location=self.chapter.location) + self.vertical = ItemFactory.create(category="vertical", parent_location=self.sequential.location) + self.html_module_1 = ItemFactory.create(category="html", parent_location=self.vertical.location) + self.html_module_2 = ItemFactory.create(category="html", parent_location=self.vertical.location) + self.vertical_with_container = ItemFactory.create( + category='vertical', parent_location=self.sequential.location + ) + self.child_container = ItemFactory.create( + category='split_test', parent_location=self.vertical_with_container.location) + self.child_vertical = ItemFactory.create(category='vertical', parent_location=self.child_container.location) + self.child_html_module = ItemFactory.create(category="html", parent_location=self.child_vertical.location) - self.vertical_with_container = self.store.get_item(self.vertical_with_container.location) - self.child_container = self.store.get_item(self.child_container.location) - self.child_vertical = self.store.get_item(self.child_vertical.location) - self.child_html_module = self.store.get_item(self.child_html_module.location) + # Read again so that children lists are accurate + self.course = self.store.get_item(self.course.location) + self.chapter = self.store.get_item(self.chapter.location) + self.chapter_2 = self.store.get_item(self.chapter_2.location) + self.sequential = self.store.get_item(self.sequential.location) + self.vertical = self.store.get_item(self.vertical.location) - self.user = UserFactory.create(username="Joe", email="joe@example.com", password="edx") - self.client.login(username=self.user.username, password="edx") + self.vertical_with_container = self.store.get_item(self.vertical_with_container.location) + self.child_container = self.store.get_item(self.child_container.location) + self.child_vertical = self.store.get_item(self.child_vertical.location) + self.child_html_module = self.store.get_item(self.child_html_module.location) + + self.user = UserFactory.create(username="Joe", email="joe@example.com", password="edx") + self.client.login(username=self.user.username, password="edx") def _get_unit_url(self, course, chapter, section, position=1): """ From 7a64e6f69fd5c3bb4e0468ce76c5f419ed237455 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 27 Jan 2015 16:05:21 -0500 Subject: [PATCH 16/22] Support running test with split modulestore as the default. With split, "name" returns "course" instead of the run name. --- .../xmodule/modulestore/xml_exporter.py | 2 +- .../commands/tests/test_dump_course.py | 34 +++++++++++-------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index a0a97a47ca..3c62f44c32 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -113,7 +113,7 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'about', 'about', '.html') # export the grading policy - course_run_policy_dir = policies_dir.makeopendir(course.location.name) + course_run_policy_dir = policies_dir.makeopendir(course.location.run) with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy: grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder, sort_keys=True, indent=4)) diff --git a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py index 587b5bff15..4ffda63514 100644 --- a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py +++ b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py @@ -23,7 +23,6 @@ from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.xml_importer import import_from_xml DATA_DIR = settings.COMMON_TEST_DATA_ROOT -TEST_COURSE_ID = 'edX/simple/2012_Fall' XML_COURSE_DIRS = ['toy', 'simple', 'open_ended'] MAPPINGS = { 'edX/toy/2012_Fall': 'xml', @@ -47,6 +46,7 @@ class CommandsTestBase(TestCase): def setUp(self): super(CommandsTestBase, self).setUp() + self.test_course_key = modulestore().make_course_key("edX", "simple", "2012_Fall") self.loaded_courses = self.load_courses() def load_courses(self): @@ -56,15 +56,19 @@ class CommandsTestBase(TestCase): # Add a course with a unicode name, if the modulestore # supports adding modules. if hasattr(store, 'create_xmodule'): - CourseFactory.create(org=u'ëḋẌ', - course=u'śíḿṕĺé', - display_name=u'2012_Fáĺĺ', - modulestore=store) + CourseFactory.create( + org=u'ëḋẌ', + course=u'śíḿṕĺé', + display_name=u'2012_Fáĺĺ', + modulestore=store + ) courses = store.get_courses() # NOTE: if xml store owns these, it won't import them into mongo - if SlashSeparatedCourseKey.from_deprecated_string(TEST_COURSE_ID) not in [c.id for c in courses]: - import_from_xml(store, ModuleStoreEnum.UserID.mgmt_command, DATA_DIR, XML_COURSE_DIRS) + if self.test_course_key not in [c.id for c in courses]: + import_from_xml( + store, ModuleStoreEnum.UserID.mgmt_command, DATA_DIR, XML_COURSE_DIRS, create_course_if_not_present=True + ) return [course.id for course in store.get_courses()] @@ -80,12 +84,12 @@ class CommandsTestBase(TestCase): output = self.call_command('dump_course_ids', **kwargs) dumped_courses = output.decode('utf-8').strip().split('\n') - course_ids = {course_id.to_deprecated_string() for course_id in self.loaded_courses} + course_ids = {unicode(course_id) for course_id in self.loaded_courses} dumped_ids = set(dumped_courses) self.assertEqual(course_ids, dumped_ids) def test_correct_course_structure_metadata(self): - course_id = 'edX/open_ended/2012_Fall' + course_id = unicode(modulestore().make_course_key('edX', 'open_ended', '2012_Fall')) args = [course_id] kwargs = {'modulestore': 'default'} @@ -98,7 +102,7 @@ class CommandsTestBase(TestCase): self.assertGreater(len(dump.values()), 0) def test_dump_course_structure(self): - args = [TEST_COURSE_ID] + args = [unicode(self.test_course_key)] kwargs = {'modulestore': 'default'} output = self.call_command('dump_course_structure', *args, **kwargs) @@ -113,8 +117,8 @@ class CommandsTestBase(TestCase): self.assertNotIn('inherited_metadata', element) # Check a few elements in the course dump - test_course_key = SlashSeparatedCourseKey.from_deprecated_string(TEST_COURSE_ID) - parent_id = test_course_key.make_usage_key('chapter', 'Overview').to_deprecated_string() + test_course_key = self.test_course_key + parent_id = unicode(test_course_key.make_usage_key('chapter', 'Overview')) self.assertEqual(dump[parent_id]['category'], 'chapter') self.assertEqual(len(dump[parent_id]['children']), 3) @@ -132,7 +136,7 @@ class CommandsTestBase(TestCase): self.assertEqual(len(dump), 16) def test_dump_inherited_course_structure(self): - args = [TEST_COURSE_ID] + args = [unicode(self.test_course_key)] kwargs = {'modulestore': 'default', 'inherited': True} output = self.call_command('dump_course_structure', *args, **kwargs) dump = json.loads(output) @@ -148,7 +152,7 @@ class CommandsTestBase(TestCase): self.assertNotIn('due', element['inherited_metadata']) def test_dump_inherited_course_structure_with_defaults(self): - args = [TEST_COURSE_ID] + args = [unicode(self.test_course_key)] kwargs = {'modulestore': 'default', 'inherited': True, 'inherited_defaults': True} output = self.call_command('dump_course_structure', *args, **kwargs) dump = json.loads(output) @@ -180,7 +184,7 @@ class CommandsTestBase(TestCase): self.check_export_file(tar_file) def run_export_course(self, filename): # pylint: disable=missing-docstring - args = [TEST_COURSE_ID, filename] + args = [unicode(self.test_course_key), filename] kwargs = {'modulestore': 'default'} return self.call_command('export_course', *args, **kwargs) From c56c88be4a19ede83016834fa6b13d1ad553c5fd Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 30 Jan 2015 14:51:59 -0500 Subject: [PATCH 17/22] create_xmodule was renamed to create_xblock, and mixed modulestore has create_xblock method. Split modulestore throws duplicate errors if course locator is not unique. --- .../commands/tests/test_dump_course.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py index 4ffda63514..708bb80dcc 100644 --- a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py +++ b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py @@ -8,6 +8,7 @@ import shutil from StringIO import StringIO import tarfile from tempfile import mkdtemp +import factory from django.conf import settings from django.core.management import call_command @@ -53,15 +54,14 @@ class CommandsTestBase(TestCase): """Load test courses and return list of ids""" store = modulestore() - # Add a course with a unicode name, if the modulestore - # supports adding modules. - if hasattr(store, 'create_xmodule'): - CourseFactory.create( - org=u'ëḋẌ', - course=u'śíḿṕĺé', - display_name=u'2012_Fáĺĺ', - modulestore=store - ) + # Add a course with a unicode name. + unique_org = factory.Sequence(lambda n: u'ëḋẌ.%d' % n) + CourseFactory.create( + org=unique_org, + course=u'śíḿṕĺé', + display_name=u'2012_Fáĺĺ', + modulestore=store + ) courses = store.get_courses() # NOTE: if xml store owns these, it won't import them into mongo From a89304b9911dc401cc3f33cc38ce34be672c9987 Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 30 Jan 2015 16:22:59 -0500 Subject: [PATCH 18/22] Cleanup to make tests agnostic to the modulestore being used. --- .../contentstore/features/course-export.py | 1 - .../contentstore/features/grading.py | 1 - .../contentstore/tests/test_contentstore.py | 102 ++++++++++-------- .../contentstore/tests/test_course_listing.py | 24 ++--- .../views/tests/test_course_updates.py | 1 - 5 files changed, 71 insertions(+), 58 deletions(-) diff --git a/cms/djangoapps/contentstore/features/course-export.py b/cms/djangoapps/contentstore/features/course-export.py index a3312cc09e..38f2dc5c32 100644 --- a/cms/djangoapps/contentstore/features/course-export.py +++ b/cms/djangoapps/contentstore/features/course-export.py @@ -5,7 +5,6 @@ from lettuce import world, step from component_settings_editor_helpers import enter_xml_in_advanced_problem from nose.tools import assert_true, assert_equal -from opaque_keys.edx.locations import SlashSeparatedCourseKey from contentstore.utils import reverse_usage_url diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py index 10ea72b794..d243d10681 100644 --- a/cms/djangoapps/contentstore/features/grading.py +++ b/cms/djangoapps/contentstore/features/grading.py @@ -5,7 +5,6 @@ from lettuce import world, step from common import * from terrain.steps import reload_the_page from selenium.common.exceptions import InvalidElementStateException -from opaque_keys.edx.locations import SlashSeparatedCourseKey from contentstore.utils import reverse_course_url from nose.tools import assert_in, assert_not_in, assert_equal, assert_not_equal # pylint: disable=no-name-in-module diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 06e027816c..b804ed07cb 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -31,7 +31,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata from opaque_keys.edx.keys import UsageKey, CourseKey -from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation, CourseLocator +from opaque_keys.edx.locations import AssetLocation, CourseLocator from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from xmodule.modulestore.xml_exporter import export_to_xml from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint @@ -92,7 +92,9 @@ class ImportRequiredTestCases(ContentStoreTestCase): Tests which legitimately need to import a course """ def test_no_static_link_rewrites_on_import(self): - course_items = import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) + course_items = import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True + ) course = course_items[0] handouts_usage_key = course.id.make_usage_key('course_info', 'handouts') @@ -113,7 +115,9 @@ class ImportRequiredTestCases(ContentStoreTestCase): e.g. /about/Fall_2012/effort.html while there is a base definition in /about/effort.html ''' - course_items = import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) + course_items = import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True + ) course_key = course_items[0].id effort = self.store.get_item(course_key.make_usage_key('about', 'effort')) self.assertEqual(effort.data, '6 hours') @@ -129,9 +133,12 @@ class ImportRequiredTestCases(ContentStoreTestCase): ''' content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], static_content_store=content_store, verbose=True) + import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], static_content_store=content_store, verbose=True, + create_course_if_not_present=True + ) - course = self.store.get_course(SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')) + course = self.store.get_course(self.store.make_course_key('edX', 'toy', '2012_Fall')) self.assertIsNotNone(course) @@ -159,7 +166,7 @@ class ImportRequiredTestCases(ContentStoreTestCase): data_dir = TEST_DATA_DIR courses = import_from_xml( self.store, self.user.id, data_dir, ['course_info_updates'], - static_content_store=content_store, verbose=True, + static_content_store=content_store, verbose=True, create_course_if_not_present=True ) course = courses[0] @@ -207,10 +214,13 @@ class ImportRequiredTestCases(ContentStoreTestCase): def test_rewrite_nonportable_links_on_import(self): content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], static_content_store=content_store) + import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], + static_content_store=content_store, create_course_if_not_present=True + ) # first check a static asset link - course_key = SlashSeparatedCourseKey('edX', 'toy', 'run') + course_key = self.store.make_course_key('edX', 'toy', 'run') html_module_location = course_key.make_usage_key('html', 'nonportable') html_module = self.store.get_item(html_module_location) self.assertIn('/static/foo.jpg', html_module.data) @@ -289,7 +299,7 @@ class ImportRequiredTestCases(ContentStoreTestCase): self.check_import(root_dir, content_store, course_id) # import to different course id - new_course_id = SlashSeparatedCourseKey('anotherX', 'anotherToy', 'Someday') + new_course_id = self.store.make_course_key('anotherX', 'anotherToy', 'Someday') self.check_import(root_dir, content_store, new_course_id) self.assertCoursesEqual(course_id, new_course_id) @@ -328,8 +338,8 @@ class ImportRequiredTestCases(ContentStoreTestCase): def test_export_course_with_metadata_only_video(self): content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) - course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True) + course_id = self.store.make_course_key('edX', 'toy', '2012_Fall') # create a new video module and add it as a child to a vertical # this re-creates a bug whereby since the video template doesn't have @@ -357,8 +367,8 @@ class ImportRequiredTestCases(ContentStoreTestCase): """ content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['word_cloud']) - course_id = SlashSeparatedCourseKey('HarvardX', 'ER22x', '2013_Spring') + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['word_cloud'], create_course_if_not_present=True) + course_id = self.store.make_course_key('HarvardX', 'ER22x', '2013_Spring') verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) @@ -384,8 +394,8 @@ class ImportRequiredTestCases(ContentStoreTestCase): """ content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) - course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True) + course_id = self.store.make_course_key('edX', 'toy', '2012_Fall') verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) @@ -415,16 +425,16 @@ class ImportRequiredTestCases(ContentStoreTestCase): """ content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True) - course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + course_id = self.store.make_course_key('edX', 'toy', '2012_Fall') # Export the course root_dir = path(mkdtemp_clean()) export_to_xml(self.store, content_store, course_id, root_dir, 'test_roundtrip') # Reimport and get the video back - import_from_xml(self.store, self.user.id, root_dir) + import_from_xml(self.store, self.user.id, root_dir, create_course_if_not_present=True) # get the sample HTML with styling information html_module = self.store.get_item(course_id.make_usage_key('html', 'with_styling')) @@ -437,7 +447,9 @@ class ImportRequiredTestCases(ContentStoreTestCase): def test_export_course_without_content_store(self): # Create toy course - course_items = import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) + course_items = import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True + ) course_id = course_items[0].id root_dir = path(mkdtemp_clean()) @@ -472,8 +484,8 @@ class ImportRequiredTestCases(ContentStoreTestCase): exported successfully """ content_store = contentstore() - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) - course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True) + course_id = self.store.make_course_key('edX', 'toy', '2012_Fall') verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'}) vertical = verticals[0] @@ -545,7 +557,6 @@ class MiscCourseTests(ContentStoreTestCase): self.user.id, self.vert_loc, 'poll_question', fields={ "name": "T1_changemind_poll_foo_2", "display_name": "Change your answer", - "reset": False, "question": "Have you changed your mind?", "answers": [{"id": "yes", "text": "Yes"}, {"id": "no", "text": "No"}], } @@ -954,7 +965,7 @@ class ContentStoreTest(ContentStoreTestCase): test_course_data.update(self.course_data) if number_suffix: test_course_data['number'] = '{0}_{1}'.format(test_course_data['number'], number_suffix) - course_key = _get_course_id(test_course_data) + course_key = _get_course_id(self.store, test_course_data) _create_course(self, course_key, test_course_data) # Verify that the creator is now registered in the course. self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_key)) @@ -983,12 +994,12 @@ class ContentStoreTest(ContentStoreTestCase): def test_create_course_check_forum_seeding(self): """Test new course creation and verify forum seeding """ test_course_data = self.assert_created_course(number_suffix=uuid4().hex) - self.assertTrue(are_permissions_roles_seeded(_get_course_id(test_course_data))) + self.assertTrue(are_permissions_roles_seeded(_get_course_id(self.store, test_course_data))) def test_forum_unseeding_on_delete(self): """Test new course creation and verify forum unseeding """ test_course_data = self.assert_created_course(number_suffix=uuid4().hex) - course_id = _get_course_id(test_course_data) + course_id = _get_course_id(self.store, test_course_data) self.assertTrue(are_permissions_roles_seeded(course_id)) delete_course_and_groups(course_id, self.user.id) # should raise an exception for checking permissions on deleted course @@ -1001,13 +1012,13 @@ class ContentStoreTest(ContentStoreTestCase): second_course_data = self.assert_created_course(number_suffix=uuid4().hex) # unseed the forums for the first course - course_id = _get_course_id(test_course_data) + course_id = _get_course_id(self.store, test_course_data) delete_course_and_groups(course_id, self.user.id) # should raise an exception for checking permissions on deleted course with self.assertRaises(ItemNotFoundError): are_permissions_roles_seeded(course_id) - second_course_id = _get_course_id(second_course_data) + second_course_id = _get_course_id(self.store, second_course_data) # permissions should still be there for the other course self.assertTrue(are_permissions_roles_seeded(second_course_id)) @@ -1016,7 +1027,7 @@ class ContentStoreTest(ContentStoreTestCase): Test that course deletion doesn't remove course enrollments or user's roles """ test_course_data = self.assert_created_course(number_suffix=uuid4().hex) - course_id = _get_course_id(test_course_data) + course_id = _get_course_id(self.store, test_course_data) # test that a user gets his enrollment and its 'student' role as default on creating a course self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) @@ -1034,7 +1045,7 @@ class ContentStoreTest(ContentStoreTestCase): of all format e.g, 'instructor_edX/Course/Run', 'instructor_edX.Course.Run', 'instructor_Course' """ test_course_data = self.assert_created_course(number_suffix=uuid4().hex) - course_id = _get_course_id(test_course_data) + course_id = _get_course_id(self.store, test_course_data) # Add user in possible groups and check that user in instructor groups of this course instructor_role = CourseInstructorRole(course_id) @@ -1063,7 +1074,7 @@ class ContentStoreTest(ContentStoreTestCase): """ test_enrollment = False try: - course_id = _get_course_id(self.course_data) + course_id = _get_course_id(self.store, self.course_data) initially_enrolled = CourseEnrollment.is_enrolled(self.user, course_id) test_enrollment = True except InvalidKeyError: @@ -1267,7 +1278,9 @@ class ContentStoreTest(ContentStoreTestCase): ) self.assertEqual(resp.status_code, 200) - course_items = import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['simple']) + course_items = import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['simple'], create_course_if_not_present=True + ) course_key = course_items[0].id resp = self._show_course_overview(course_key) @@ -1311,7 +1324,7 @@ class ContentStoreTest(ContentStoreTestCase): delete_item(category='chapter', name='chapter_2') def test_import_into_new_course_id(self): - target_course_id = _get_course_id(self.course_data) + target_course_id = _get_course_id(self.store, self.course_data) _create_course(self, target_course_id, self.course_data) import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy'], target_course_id=target_course_id) @@ -1336,7 +1349,7 @@ class ContentStoreTest(ContentStoreTestCase): def test_import_into_new_course_id_wiki_slug_renamespacing(self): # If reimporting into the same course do not change the wiki_slug. - target_course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall') + target_course_id = self.store.make_course_key('edX', 'toy', '2012_Fall') course_data = { 'org': target_course_id.org, 'number': target_course_id.course, @@ -1354,7 +1367,7 @@ class ContentStoreTest(ContentStoreTestCase): self.assertEquals(course_module.wiki_slug, 'toy') # But change the wiki_slug if it is a different course. - target_course_id = SlashSeparatedCourseKey('MITx', '111', '2013_Spring') + target_course_id = self.store.make_course_key('MITx', '111', '2013_Spring') course_data = { 'org': target_course_id.org, 'number': target_course_id.course, @@ -1374,10 +1387,10 @@ class ContentStoreTest(ContentStoreTestCase): self.assertEquals(course_module.wiki_slug, 'MITx.111.2013_Spring') def test_import_metadata_with_attempts_empty_string(self): - import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['simple']) + import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['simple'], create_course_if_not_present=True) did_load_item = False try: - course_key = SlashSeparatedCourseKey('edX', 'simple', 'problem') + course_key = self.store.make_course_key('edX', 'simple', 'problem') usage_key = course_key.make_usage_key('problem', 'ps01-simple') self.store.get_item(usage_key) did_load_item = True @@ -1396,7 +1409,9 @@ class ContentStoreTest(ContentStoreTestCase): self.assertNotEquals(new_discussion_item.discussion_id, '$$GUID$$') def test_metadata_inheritance(self): - course_items = import_from_xml(self.store, self.user.id, TEST_DATA_DIR, ['toy']) + course_items = import_from_xml( + self.store, self.user.id, TEST_DATA_DIR, ['toy'], create_course_if_not_present=True + ) course = course_items[0] verticals = self.store.get_items(course.id, qualifiers={'category': 'vertical'}) @@ -1467,7 +1482,8 @@ class ContentStoreTest(ContentStoreTestCase): self.user.id, TEST_DATA_DIR, ['conditional_and_poll'], - static_content_store=content_store + static_content_store=content_store, + create_course_if_not_present=True ) course = courses[0] @@ -1489,7 +1505,7 @@ class ContentStoreTest(ContentStoreTestCase): def test_wiki_slug(self): """When creating a course a unique wiki_slug should be set.""" - course_key = _get_course_id(self.course_data) + course_key = _get_course_id(self.store, self.course_data) _create_course(self, course_key, self.course_data) course_module = self.store.get_course(course_key) self.assertEquals(course_module.wiki_slug, 'MITx.111.2013_Spring') @@ -1583,7 +1599,7 @@ class RerunCourseTest(ContentStoreTestCase): if not destination_course_data: destination_course_data = self.destination_course_data rerun_course_data.update(destination_course_data) - destination_course_key = _get_course_id(destination_course_data) + destination_course_key = _get_course_id(self.store, destination_course_data) # post the request course_url = get_url('course_handler', destination_course_key, 'course_key_string') @@ -1809,6 +1825,6 @@ def _create_course(test, course_key, course_data): test.assertEqual(data['url'], course_url) -def _get_course_id(course_data, key_class=SlashSeparatedCourseKey): - """Returns the course ID (org/number/run).""" - return key_class(course_data['org'], course_data['number'], course_data['run']) +def _get_course_id(store, course_data): + """Returns the course ID.""" + return store.make_course_key(course_data['org'], course_data['number'], course_data['run']) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index fe489f98a6..43f4b6782f 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -18,7 +18,7 @@ from student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff, Or from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls from xmodule.modulestore import ModuleStoreEnum -from opaque_keys.edx.locations import SlashSeparatedCourseKey, CourseLocator +from opaque_keys.edx.locations import CourseLocator from xmodule.modulestore.django import modulestore from xmodule.error_module import ErrorDescriptor from course_action_state.models import CourseRerunState @@ -74,7 +74,7 @@ class TestCourseListing(ModuleStoreTestCase): """ Test getting courses with new access group format e.g. 'instructor_edx.course.run' """ - course_location = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + course_location = self.store.make_course_key('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_location, self.user) # get courses through iterating all courses @@ -93,7 +93,7 @@ class TestCourseListing(ModuleStoreTestCase): """ GlobalStaff().add_users(self.user) - course_key = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + course_key = self.store.make_course_key('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_key, self.user) with patch('xmodule.modulestore.mongo.base.MongoKeyValueStore', Mock(side_effect=Exception)): @@ -112,9 +112,9 @@ class TestCourseListing(ModuleStoreTestCase): Test the course list for regular staff when get_course returns an ErrorDescriptor """ GlobalStaff().remove_users(self.user) - CourseStaffRole(SlashSeparatedCourseKey('Non', 'Existent', 'Course')).add_users(self.user) + CourseStaffRole(self.store.make_course_key('Non', 'Existent', 'Course')).add_users(self.user) - course_key = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + course_key = self.store.make_course_key('Org1', 'Course1', 'Run1') self._create_course_with_access_groups(course_key, self.user) with patch('xmodule.modulestore.mongo.base.MongoKeyValueStore', Mock(side_effect=Exception)): @@ -133,7 +133,7 @@ class TestCourseListing(ModuleStoreTestCase): """ Test getting courses with invalid course location (course deleted from modulestore). """ - course_key = SlashSeparatedCourseKey('Org', 'Course', 'Run') + course_key = self.store.make_course_key('Org', 'Course', 'Run') self._create_course_with_access_groups(course_key, self.user) # get courses through iterating all courses @@ -169,7 +169,7 @@ class TestCourseListing(ModuleStoreTestCase): org = 'Org{0}'.format(number) course = 'Course{0}'.format(number) run = 'Run{0}'.format(number) - course_location = SlashSeparatedCourseKey(org, course, run) + course_location = self.store.make_course_key(org, course, run) if number in user_course_ids: self._create_course_with_access_groups(course_location, self.user) else: @@ -218,14 +218,14 @@ class TestCourseListing(ModuleStoreTestCase): """ store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo) - course_location = SlashSeparatedCourseKey('testOrg', 'testCourse', 'RunBabyRun') + course_location = self.store.make_course_key('testOrg', 'testCourse', 'RunBabyRun') self._create_course_with_access_groups(course_location, self.user) - course_location = SlashSeparatedCourseKey('testOrg', 'doomedCourse', 'RunBabyRun') + course_location = self.store.make_course_key('testOrg', 'doomedCourse', 'RunBabyRun') self._create_course_with_access_groups(course_location, self.user) store.delete_course(course_location, self.user.id) - course_location = SlashSeparatedCourseKey('testOrg', 'erroredCourse', 'RunBabyRun') + course_location = self.store.make_course_key('testOrg', 'erroredCourse', 'RunBabyRun') course = self._create_course_with_access_groups(course_location, self.user) course_db_record = store._find_one(course.location) course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki"}) @@ -245,14 +245,14 @@ class TestCourseListing(ModuleStoreTestCase): Create multiple courses within the same org. Verify that someone with org-wide permissions can access all of them. """ - org_course_one = SlashSeparatedCourseKey('AwesomeOrg', 'Course1', 'RunBabyRun') + org_course_one = self.store.make_course_key('AwesomeOrg', 'Course1', 'RunBabyRun') CourseFactory.create( org=org_course_one.org, number=org_course_one.course, run=org_course_one.run ) - org_course_two = SlashSeparatedCourseKey('AwesomeOrg', 'Course2', 'RunRunRun') + org_course_two = self.store.make_course_key('AwesomeOrg', 'Course2', 'RunRunRun') CourseFactory.create( org=org_course_two.org, number=org_course_two.course, diff --git a/cms/djangoapps/contentstore/views/tests/test_course_updates.py b/cms/djangoapps/contentstore/views/tests/test_course_updates.py index 0875bdbb61..a45c7b1db5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_updates.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_updates.py @@ -5,7 +5,6 @@ import json from contentstore.tests.test_course_settings import CourseTestCase from contentstore.utils import reverse_course_url, reverse_usage_url -from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.keys import UsageKey from xmodule.modulestore.django import modulestore From 149c4e78c118117af3cb5fcabd3fa6a2ae36e7a3 Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 5 Feb 2015 12:30:15 -0500 Subject: [PATCH 19/22] Update tests in reaction to ModuleStoreTestCase cleanup. --- .../contentstore/tests/test_import.py | 3 +- .../contentstore/tests/test_utils.py | 2 +- .../commands/tests/test_dump_course.py | 28 +++++++++++++------ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index bf75b96f2b..8a4006943b 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -91,7 +91,8 @@ class ContentStoreImportTest(ModuleStoreTestCase): TEST_DATA_DIR, ['2014_Uni'], target_course_id=course_id, - create_course_if_not_present=True + # When this test is run with split modulestore as the default, the create course option must be specified. + # create_course_if_not_present=True ) course = module_store.get_course(course_id) diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index f7faea20ee..4c44f999b8 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -203,7 +203,7 @@ class CourseImageTestCase(ModuleStoreTestCase): ) -class XBlockVisibilityTestCase(TestCase): +class XBlockVisibilityTestCase(ModuleStoreTestCase): """Tests for xblock visibility for students.""" def setUp(self): diff --git a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py index 708bb80dcc..fc13685384 100644 --- a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py +++ b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py @@ -13,13 +13,13 @@ import factory from django.conf import settings from django.core.management import call_command from django.test.utils import override_settings -from django.test.testcases import TestCase -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config -from xmodule.modulestore.tests.django_utils import TEST_DATA_MONGO_MODULESTORE +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_MONGO_MODULESTORE, TEST_DATA_MONGO_MODULESTORE_SPLIT_DEFAULT +) from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.xml_importer import import_from_xml @@ -36,7 +36,7 @@ TEST_DATA_MIXED_XML_MODULESTORE = mixed_store_config( ) -class CommandsTestBase(TestCase): +class CommandsTestBase(ModuleStoreTestCase): """ Base class for testing different django commands. @@ -44,6 +44,7 @@ class CommandsTestBase(TestCase): to be tested. """ + __test__ = False def setUp(self): super(CommandsTestBase, self).setUp() @@ -204,17 +205,28 @@ class CommandsTestBase(TestCase): assert_in('edX-simple-2012_Fall/sequential/Lecture_2.xml', names) -class CommandsXMLTestCase(CommandsTestBase, ModuleStoreTestCase): +class CommandsXMLTestCase(CommandsTestBase): """ - Test case for management commands using the xml modulestore. + Test case for management commands with the xml modulestore present. """ MODULESTORE = TEST_DATA_MIXED_XML_MODULESTORE + __test__ = True -class CommandsMongoTestCase(CommandsTestBase, ModuleStoreTestCase): +class CommandsMongoTestCase(CommandsTestBase): """ - Test case for management commands using the mixed mongo modulestore. + Test case for management commands using the mixed mongo modulestore with old mongo as the default. """ MODULESTORE = TEST_DATA_MONGO_MODULESTORE + __test__ = True + + +class CommandSplitMongoTestCase(CommandsTestBase): + """ + Test case for management commands using the mixed mongo modulestore with split as the default. + + """ + MODULESTORE = TEST_DATA_MONGO_MODULESTORE_SPLIT_DEFAULT + __test__ = True From 78a8fd807a35a31280770c19f17c12416d7f32bf Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 6 Feb 2015 16:42:04 -0500 Subject: [PATCH 20/22] If existing course is found on import, make sure dest_course_id is correct. TNL-1362 --- .../management/commands/tests/test_import.py | 23 ++++++++++++++++++- .../xmodule/modulestore/xml_importer.py | 9 +++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_import.py b/cms/djangoapps/contentstore/management/commands/tests/test_import.py index f613c27875..7a56893607 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_import.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_import.py @@ -11,6 +11,7 @@ from django.core.management import call_command from django_comment_common.utils import are_permissions_roles_seeded from xmodule.modulestore.django import modulestore +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -27,7 +28,7 @@ class TestImport(ModuleStoreTestCase): 'course="{0.course}"/>'.format(course_id)) with open(os.path.join(directory, "course", "{0.run}.xml".format(course_id)), "w+") as f: - f.write('') + f.write('') return directory @@ -72,3 +73,23 @@ class TestImport(ModuleStoreTestCase): # Now load up the course with a similar course_id and verify it loads call_command('import', self.content_dir, self.course_dir) self.assertIsNotNone(store.get_course(self.truncated_key)) + + def test_existing_course_with_different_modulestore(self): + """ + Checks that a course that originally existed in old mongo can be re-imported when + split is the default modulestore. + """ + with modulestore().default_store(ModuleStoreEnum.Type.mongo): + call_command('import', self.content_dir, self.good_dir) + + # Clear out the modulestore mappings, else when the next import command goes to create a destination + # course_key, it will find the existing course and return the mongo course_key. To reproduce TNL-1362, + # the destination course_key needs to be the one for split modulestore. + modulestore().mappings = {} + + with modulestore().default_store(ModuleStoreEnum.Type.split): + call_command('import', self.content_dir, self.good_dir) + course = modulestore().get_course(self.base_course_key) + # With the bug, this fails because the chapter's course_key is the split mongo form, + # while the course's course_key is the old mongo form. + self.assertEqual(unicode(course.location.course_key), unicode(course.children[0].course_key)) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index e697c61011..1054520a7e 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -195,11 +195,18 @@ def import_from_xml( if target_course_id is not None: dest_course_id = target_course_id else: + # Note that dest_course_id will be in the format for the default modulestore. dest_course_id = store.make_course_key(course_key.org, course_key.course, course_key.run) + existing_course_id = store.has_course(dest_course_id, ignore_case=True) + # store.has_course will return the course_key in the format for the modulestore in which it was found. + # This may be different from dest_course_id, so correct to the format found. + if existing_course_id: + dest_course_id = existing_course_id + runtime = None # Creates a new course if it doesn't already exist - if create_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True): + if create_course_if_not_present and not existing_course_id: try: new_course = store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id) runtime = new_course.runtime From d88b0c488e7f9d54d9e0d5d277773d6738fcaddf Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 6 Feb 2015 17:22:47 -0500 Subject: [PATCH 21/22] store.has_course fails on old mongo with unicode chars. xml_importer now calls store.has_course even if create_course_if_not_present is False. --- .../contentstore/tests/test_import.py | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 8a4006943b..01efcca80c 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -83,23 +83,24 @@ class ContentStoreImportTest(ModuleStoreTestCase): """ # Test that importing course with unicode 'id' and 'display name' doesn't give UnicodeEncodeError """ - module_store = modulestore() - course_id = module_store.make_course_key(u'Юникода', u'unicode_course', u'échantillon') - import_from_xml( - module_store, - self.user.id, - TEST_DATA_DIR, - ['2014_Uni'], - target_course_id=course_id, - # When this test is run with split modulestore as the default, the create course option must be specified. - # create_course_if_not_present=True - ) + # Test with the split modulestore because store.has_course fails in old mongo with unicode characters. + with modulestore().default_store(ModuleStoreEnum.Type.split): + module_store = modulestore() + course_id = module_store.make_course_key(u'Юникода', u'unicode_course', u'échantillon') + import_from_xml( + module_store, + self.user.id, + TEST_DATA_DIR, + ['2014_Uni'], + target_course_id=course_id, + create_course_if_not_present=True + ) - course = module_store.get_course(course_id) - self.assertIsNotNone(course) + course = module_store.get_course(course_id) + self.assertIsNotNone(course) - # test that course 'display_name' same as imported course 'display_name' - self.assertEqual(course.display_name, u"Φυσικά το όνομα Unicode") + # test that course 'display_name' same as imported course 'display_name' + self.assertEqual(course.display_name, u"Φυσικά το όνομα Unicode") def test_static_import(self): ''' From d86c7bec3ce9628381463dccedbafc4af04181db Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 9 Feb 2015 09:30:52 -0500 Subject: [PATCH 22/22] Update for renamed test split modulestore. --- .../courseware/management/commands/tests/test_dump_course.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py index fc13685384..bda05fea4b 100644 --- a/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py +++ b/lms/djangoapps/courseware/management/commands/tests/test_dump_course.py @@ -18,7 +18,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config from xmodule.modulestore.tests.django_utils import ( - TEST_DATA_MONGO_MODULESTORE, TEST_DATA_MONGO_MODULESTORE_SPLIT_DEFAULT + TEST_DATA_MONGO_MODULESTORE, TEST_DATA_SPLIT_MODULESTORE ) from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.xml_importer import import_from_xml @@ -228,5 +228,5 @@ class CommandSplitMongoTestCase(CommandsTestBase): Test case for management commands using the mixed mongo modulestore with split as the default. """ - MODULESTORE = TEST_DATA_MONGO_MODULESTORE_SPLIT_DEFAULT + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE __test__ = True