From 3c3a316f088e886b48adfab64c4ef60afe717768 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 14 Aug 2014 10:27:10 -0400 Subject: [PATCH] Make it branch agnostic LMS-11210 Conflicts: common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py --- .../contentstore/tests/test_clone_course.py | 4 +- cms/djangoapps/contentstore/tests/utils.py | 19 +++--- .../lib/xmodule/xmodule/modulestore/mixed.py | 2 +- .../lib/xmodule/xmodule/modulestore/search.py | 4 +- .../xmodule/modulestore/split_migrator.py | 7 +-- .../modulestore/split_mongo/split_draft.py | 10 +++ .../tests/test_mixed_modulestore.py | 25 ++++---- .../xmodule/modulestore/xml_exporter.py | 63 ++++++++++--------- common/lib/xmodule/xmodule/tests/__init__.py | 47 +++++++------- 9 files changed, 98 insertions(+), 83 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index 71f148820a..acd2d93714 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -28,14 +28,14 @@ class CloneCourseTest(CourseTestCase): # 3. clone course (mongo -> split) with self.store.default_store(ModuleStoreEnum.Type.split): split_course3_id = CourseLocator( - org="edx3", course="split3", run="2013_Fall", branch=ModuleStoreEnum.BranchName.draft + org="edx3", course="split3", run="2013_Fall" ) self.store.clone_course(mongo_course2_id, split_course3_id, self.user.id) self.assertCoursesEqual(mongo_course2_id, split_course3_id) # 4. clone course (split -> split) split_course4_id = CourseLocator( - org="edx4", course="split4", run="2013_Fall", branch=ModuleStoreEnum.BranchName.draft + org="edx4", course="split4", run="2013_Fall" ) self.store.clone_course(split_course3_id, split_course4_id, self.user.id) self.assertCoursesEqual(split_course3_id, split_course4_id) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index cdd0a4b512..fca7d8b2a2 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -257,13 +257,14 @@ class CourseTestCase(ModuleStoreTestCase): ) for course1_item in course1_items: - course2_item_location = course1_item.location.map_into_course(course2_id) - if course1_item.location.category == 'course': + course1_item_loc = course1_item.location + course2_item_loc = course2_id.make_usage_key(course1_item_loc.block_type, course1_item_loc.block_id) + if course1_item_loc.block_type == 'course': # mongo uses the run as the name, split uses 'course' store = self.store._get_modulestore_for_courseid(course2_id) # pylint: disable=protected-access - new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_location.run - course2_item_location = course2_item_location.replace(name=new_name) - course2_item = self.store.get_item(course2_item_location) + new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_loc.run + course2_item_loc = course2_item_loc.replace(name=new_name) + course2_item = self.store.get_item(course2_item_loc) try: # compare published state @@ -278,7 +279,7 @@ class CourseTestCase(ModuleStoreTestCase): c1_state, c2_state, "Publish states not equal: course item {} in state {} != course item {} in state {}".format( - course1_item.location, c1_state, course2_item.location, c2_state + course1_item_loc, c1_state, course2_item.location, c2_state ) ) @@ -296,11 +297,9 @@ class CourseTestCase(ModuleStoreTestCase): expected_children = [] for course1_item_child in course1_item.children: expected_children.append( - course1_item_child.map_into_course(course2_id) + course2_id.make_usage_key(course1_item_child.block_type, course1_item_child.block_id) ) - # also process course2_children just in case they have version guids - course2_children = [child.version_agnostic() for child in course2_item.children] - self.assertEqual(expected_children, course2_children) + self.assertEqual(expected_children, course2_item.children) # compare assets content_store = self.store.contentstore diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 3521abd140..26fbf769e7 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -46,7 +46,7 @@ def strip_key(func): # remove version and branch, by default rem_vers = kwargs.pop('remove_version', True) - rem_branch = kwargs.pop('remove_branch', False) + rem_branch = kwargs.pop('remove_branch', True) # helper function for stripping individual values def strip_key_func(val): diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index a0ea03a66d..47fd799d79 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -95,10 +95,10 @@ def path_to_location(modulestore, usage_key): category = path[path_index].block_type if category == 'sequential' or category == 'videosequence': section_desc = modulestore.get_item(path[path_index]) - child_locs = [c.location.version_agnostic() for c in section_desc.get_children()] + child_locs = [c.location.block_id for c in section_desc.get_children()] # positions are 1-indexed, and should be strings to be consistent with # url parsing. - position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) + position_list.append(str(child_locs.index(path[path_index + 1].block_id) + 1)) position = "_".join(position_list) return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 9124195636..5f8c987f47 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -168,17 +168,16 @@ class SplitMigrator(object): ) new_parent = self.split_modulestore.get_item(split_parent_loc, **kwargs) # this only occurs if the parent was also awaiting adoption: skip this one, go to next - if any(new_locator == child.version_agnostic() for child in new_parent.children): + if any(new_locator.block_id == child.block_id for child in new_parent.children): continue # find index for module: new_parent may be missing quite a few of old_parent's children new_parent_cursor = 0 for old_child_loc in old_parent.children: - if old_child_loc == draft_location: + if old_child_loc.block_id == draft_location.block_id: break # moved cursor enough, insert it here - sibling_loc = new_draft_course_loc.make_usage_key(old_child_loc.category, old_child_loc.block_id) # sibling may move cursor for idx in range(new_parent_cursor, len(new_parent.children)): - if new_parent.children[idx].version_agnostic() == sibling_loc: + if new_parent.children[idx].block_id == old_child_loc.block_id: new_parent_cursor = idx + 1 break # skipped sibs enough, pick back up scan new_parent.children.insert(new_parent_cursor, new_locator) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index a57da5c16c..26714f1e1f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -52,6 +52,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS course_id = self._map_revision_to_branch(course_id) return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs) + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, revision=None, **kwargs): + """ + See :py:meth: xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.clone_course + """ + dest_course_id = self._map_revision_to_branch(dest_course_id, revision=revision) + return super(DraftVersioningModuleStore, self).clone_course( + source_course_id, dest_course_id, user_id, fields=fields, **kwargs + ) + def get_courses(self, **kwargs): """ Returns all the courses on the Draft or Published branch depending on the branch setting. @@ -76,6 +85,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): + descriptor.location = self._map_revision_to_branch(descriptor.location) item = super(DraftVersioningModuleStore, self).update_item( descriptor, user_id, diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index f1abe0426e..f4c574d23f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -472,7 +472,6 @@ class TestMixedModuleStore(unittest.TestCase): ) # verify pre delete state (just to verify that the test is valid) - self.assertTrue(hasattr(private_vert, 'is_draft') or private_vert.location.branch == ModuleStoreEnum.BranchName.draft) if hasattr(private_vert.location, 'version_guid'): # change to the HEAD version vert_loc = private_vert.location.for_version(private_leaf.location.version_guid) @@ -699,20 +698,22 @@ class TestMixedModuleStore(unittest.TestCase): Make sure that path_to_location works """ self.initdb(default_ms) - self._create_block_hierarchy() course_key = self.course_locations[self.MONGO_COURSEID].course_key - should_work = ( - (self.problem_x1a_2, - (course_key, u"Chapter_x", u"Sequential_x1", '1')), - (self.chapter_x, - (course_key, "Chapter_x", None, None)), - ) + with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): + self._create_block_hierarchy() - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) - for location, expected in should_work: - with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends): - self.assertEqual(path_to_location(self.store, location), expected) + should_work = ( + (self.problem_x1a_2, + (course_key, u"Chapter_x", u"Sequential_x1", '1')), + (self.chapter_x, + (course_key, "Chapter_x", None, None)), + ) + + mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) + for location, expected in should_work: + with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends): + self.assertEqual(path_to_location(self.store, location), expected) not_found = ( course_key.make_usage_key('video', 'WelcomeX'), diff --git a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py index 8c001695a5..bc31250ef9 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_exporter.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_exporter.py @@ -104,40 +104,44 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir): policy = {'course/' + course.location.name: own_metadata(course)} course_policy.write(dumps(policy, cls=EdxJSONEncoder)) - # NOTE: this code assumes that verticals are the top most draftable container - # should we change the application, then this assumption will no longer be valid - # NOTE: we need to explicitly implement the logic for setting the vertical's parent - # and index here since the XML modulestore cannot load draft modules - draft_verticals = modulestore.get_items( - course_key, - qualifiers={'category': 'vertical'}, - revision=ModuleStoreEnum.RevisionOption.draft_only - ) - if len(draft_verticals) > 0: - draft_course_dir = export_fs.makeopendir(DRAFT_DIR) - for draft_vertical in draft_verticals: - parent_loc = modulestore.get_parent_location( - draft_vertical.location, - revision=ModuleStoreEnum.RevisionOption.draft_preferred + #### DRAFTS #### + # xml backed courses don't support drafts! + if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml: + # NOTE: this code assumes that verticals are the top most draftable container + # should we change the application, then this assumption will no longer be valid + # NOTE: we need to explicitly implement the logic for setting the vertical's parent + # and index here since the XML modulestore cannot load draft modules + with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key): + draft_verticals = modulestore.get_items( + course_key, + qualifiers={'category': 'vertical'}, + revision=ModuleStoreEnum.RevisionOption.draft_only ) - # Don't try to export orphaned items. - if parent_loc is not None: - logging.debug('parent_loc = {0}'.format(parent_loc)) - if parent_loc.category in DIRECT_ONLY_CATEGORIES: - draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string() - sequential = modulestore.get_item(parent_loc) - index = sequential.children.index(draft_vertical.location) - draft_vertical.xml_attributes['index_in_children_list'] = str(index) - draft_vertical.runtime.export_fs = draft_course_dir - adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir) - node = lxml.etree.Element('unknown') - draft_vertical.add_xml_to_node(node) + + if len(draft_verticals) > 0: + draft_course_dir = export_fs.makeopendir(DRAFT_DIR) + for draft_vertical in draft_verticals: + parent_loc = modulestore.get_parent_location( + draft_vertical.location, + revision=ModuleStoreEnum.RevisionOption.draft_preferred + ) + # Don't try to export orphaned items. + if parent_loc is not None: + logging.debug('parent_loc = {0}'.format(parent_loc)) + if parent_loc.category in DIRECT_ONLY_CATEGORIES: + draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string() + sequential = modulestore.get_item(parent_loc) + index = sequential.children.index(draft_vertical.location) + draft_vertical.xml_attributes['index_in_children_list'] = str(index) + draft_vertical.runtime.export_fs = draft_course_dir + adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir) + node = lxml.etree.Element('unknown') + draft_vertical.add_xml_to_node(node) def adapt_references(subtree, destination_course_key, export_fs): """ - Map every reference in the subtree into destination_course_key and set it back into the xblock fields. - Make sure every runtime knows where the export_fs is. + Map every reference in the subtree into destination_course_key and set it back into the xblock fields """ subtree.runtime.export_fs = export_fs # ensure everything knows where it's going! for field_name, field in subtree.fields.iteritems(): @@ -162,6 +166,7 @@ def adapt_references(subtree, destination_course_key, export_fs): ) + def _export_field_content(xblock_item, item_dir): """ Export all fields related to 'xblock_item' other than 'metadata' and 'data' to json file in provided directory diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index e1f07c0266..1939294d6a 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -212,23 +212,27 @@ class CourseComparisonTest(unittest.TestCase): will be ignored for the purpose of equality checking. """ # compare published - expected_items = expected_store.get_items(expected_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) - actual_items = actual_store.get_items(actual_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) - self.assertGreater(len(expected_items), 0) - self._assertCoursesEqual(expected_items, actual_items, actual_course_key) + with expected_store.branch_setting(ModuleStoreEnum.Branch.published_only, expected_course_key): + with actual_store.branch_setting(ModuleStoreEnum.Branch.published_only, actual_course_key): + expected_items = expected_store.get_items(expected_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) + actual_items = actual_store.get_items(actual_course_key, revision=ModuleStoreEnum.RevisionOption.published_only) + self.assertGreater(len(expected_items), 0) + self._assertCoursesEqual(expected_items, actual_items, actual_course_key) - # compare draft - if expected_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: - revision = ModuleStoreEnum.RevisionOption.draft_only - else: - revision = None - expected_items = expected_store.get_items(expected_course_key, revision=revision) - if actual_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: - revision = ModuleStoreEnum.RevisionOption.draft_only - else: - revision = None - actual_items = actual_store.get_items(actual_course_key, revision=revision) - self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True) + with expected_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, expected_course_key): + with actual_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, actual_course_key): + # compare draft + if expected_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: + revision = ModuleStoreEnum.RevisionOption.draft_only + else: + revision = None + expected_items = expected_store.get_items(expected_course_key, revision=revision) + if actual_store.get_modulestore_type(None) == ModuleStoreEnum.Type.split: + revision = ModuleStoreEnum.RevisionOption.draft_only + else: + revision = None + actual_items = actual_store.get_items(actual_course_key, revision=revision) + self._assertCoursesEqual(expected_items, actual_items, actual_course_key, expect_drafts=True) def _assertCoursesEqual(self, expected_items, actual_items, actual_course_key, expect_drafts=False): self.assertEqual(len(expected_items), len(actual_items)) @@ -282,18 +286,15 @@ class CourseComparisonTest(unittest.TestCase): # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) if expected_item.has_children: - actual_course_key = actual_item.location.course_key.version_agnostic() expected_children = [ - course1_item_child.location.map_into_course(actual_course_key) + (course1_item_child.location.block_type, course1_item_child.location.block_id) + # get_children() rather than children to strip privates from public parents for course1_item_child in expected_item.get_children() - # get_children was returning drafts for published parents :-( - if expect_drafts or not getattr(course1_item_child, 'is_draft', False) ] actual_children = [ - item_child.location.version_agnostic() + (item_child.location.block_type, item_child.location.block_id) + # get_children() rather than children to strip privates from public parents for item_child in actual_item.get_children() - # get_children was returning drafts for published parents :-( - if expect_drafts or not getattr(item_child, 'is_draft', False) ] self.assertEqual(expected_children, actual_children)