From ed3f609757b5023d8aa92e448e3e5bfac0cafab0 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 12 Jan 2015 13:58:13 -0500 Subject: [PATCH] Change split to handle dags and fix children references on copy and delete w/ more than one parent --- .../xmodule/modulestore/split_mongo/split.py | 31 ++++++++------ .../tests/test_mixed_modulestore.py | 40 ++++++++++++++++++ common/test/data/xml_dag/README.md | 2 + common/test/data/xml_dag/course.xml | 1 + common/test/data/xml_dag/course/2012_Fall.xml | 13 ++++++ common/test/data/xml_dag/html/toyhtml.html | 1 + common/test/data/xml_dag/html/toyhtml.xml | 1 + .../test/data/xml_dag/policies/2012_Fall.json | 8 ++++ .../test/data/xml_dag/static/just_a_test.jpg | Bin 0 -> 547 bytes .../data/xml_dag/vertical/vertical_test.xml | 4 ++ 10 files changed, 87 insertions(+), 14 deletions(-) create mode 100644 common/test/data/xml_dag/README.md create mode 100644 common/test/data/xml_dag/course.xml create mode 100644 common/test/data/xml_dag/course/2012_Fall.xml create mode 100644 common/test/data/xml_dag/html/toyhtml.html create mode 100644 common/test/data/xml_dag/html/toyhtml.xml create mode 100644 common/test/data/xml_dag/policies/2012_Fall.json create mode 100644 common/test/data/xml_dag/static/just_a_test.jpg create mode 100644 common/test/data/xml_dag/vertical/vertical_test.xml diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 3874d8d872..155a12d223 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -1061,13 +1061,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError(locator) course = self._lookup_course(locator.course_key) - parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course.structure) - if parent_id is None: + parent_ids = self._get_parents_from_structure(BlockKey.from_usage_key(locator), course.structure) + if len(parent_ids) == 0: return None + # find alphabetically least + parent_ids.sort(key=lambda parent: (parent.type, parent.id)) return BlockUsageLocator.make_relative( locator, - block_type=parent_id.type, - block_id=parent_id.id, + block_type=parent_ids[0].type, + block_id=parent_ids[0].id, ) def get_orphans(self, course_key, **kwargs): @@ -2041,8 +2043,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): for subtree_root in subtree_list: if BlockKey.from_usage_key(subtree_root) != source_structure['root']: # find the parents and put root in the right sequence - parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure) - if parent is not None: # may be a detached category xblock + parents = self._get_parents_from_structure(BlockKey.from_usage_key(subtree_root), source_structure) + for parent in parents: if parent not in destination_blocks: raise ItemNotFoundError(parent) orphans.update( @@ -2101,8 +2103,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): new_structure = self.version_structure(usage_locator.course_key, original_structure, user_id) new_blocks = new_structure['blocks'] new_id = new_structure['_id'] - parent_block_key = self._get_parent_from_structure(block_key, original_structure) - if parent_block_key: + parent_block_keys = self._get_parents_from_structure(block_key, original_structure) + for parent_block_key in parent_block_keys: parent_block = new_blocks[parent_block_key] parent_block['fields']['children'].remove(block_key) parent_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) @@ -2626,15 +2628,16 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): } @contract(block_key=BlockKey) - def _get_parent_from_structure(self, block_key, structure): + def _get_parents_from_structure(self, block_key, structure): """ Given a structure, find block_key's parent in that structure. Note returns the encoded format for parent """ - for parent_block_key, value in structure['blocks'].iteritems(): - if block_key in value['fields'].get('children', []): - return parent_block_key - return None + return [ + parent_block_key + for parent_block_key, value in structure['blocks'].iteritems() + if block_key in value['fields'].get('children', []) + ] def _sync_children(self, source_parent, destination_parent, new_child): """ @@ -2733,7 +2736,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ Delete the orphan and any of its descendants which no longer have parents. """ - if self._get_parent_from_structure(orphan, structure) is None: + if len(self._get_parents_from_structure(orphan, structure)) == 0: for child in structure['blocks'][orphan]['fields'].get('children', []): self._delete_if_true_orphan(BlockKey(*child), structure) del structure['blocks'][orphan] 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 d055c8a602..a8712f4d66 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1962,6 +1962,46 @@ class TestMixedModuleStore(CourseComparisonTest): with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id): self.assertTrue(self.store.has_item(vertical_loc)) + @ddt.data(ModuleStoreEnum.Type.split) # Need to fix and add ModuleStoreEnum.Type.mongo, + def test_delete_dag(self, default): + """ + Test that deleting an element with more than one parent fully removes it from the course. + """ + # set the default modulestore + with MongoContentstoreBuilder().build() as contentstore: + self.store = MixedModuleStore( + contentstore=contentstore, + create_modulestore_instance=create_modulestore_instance, + mappings={}, + **self.OPTIONS + ) + self.addCleanup(self.store.close_all_connections) + with self.store.default_store(default): + dest_course_key = self.store.make_course_key('a', 'course', 'course') + courses = import_from_xml( + self.store, self.user_id, DATA_DIR, ['xml_dag'], load_error_modules=False, + static_content_store=contentstore, + target_course_id=dest_course_key, + create_course_if_not_present=True, + ) + course_id = courses[0].id + # ensure both parents point to the dag item + dag_item = course_id.make_usage_key('html', 'toyhtml') + one_parent = course_id.make_usage_key('vertical', 'vertical_test') + other_parent = course_id.make_usage_key('vertical', 'zeta') + with self.store.bulk_operations(course_id): + # actually should test get_parent but it's not alphabetized yet + self.assertEqual(self.store.get_parent_location(dag_item), one_parent) + for parent_loc in [one_parent, other_parent]: + parent = self.store.get_item(parent_loc) + self.assertIn(dag_item, parent.children) + # just testing draft branch assuming it doesn't matter which branch + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_id): + self.store.delete_item(dag_item, self.user_id) + for parent_loc in [one_parent, other_parent]: + parent = self.store.get_item(parent_loc) + self.assertNotIn(dag_item, parent.children) + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_import_edit_import(self, default): """ diff --git a/common/test/data/xml_dag/README.md b/common/test/data/xml_dag/README.md new file mode 100644 index 0000000000..1c9f2d3b27 --- /dev/null +++ b/common/test/data/xml_dag/README.md @@ -0,0 +1,2 @@ +Minimal course for testing the handling of non-tree courses. The same html component occurs under more than one +vertical. \ No newline at end of file diff --git a/common/test/data/xml_dag/course.xml b/common/test/data/xml_dag/course.xml new file mode 100644 index 0000000000..ebe7e5f887 --- /dev/null +++ b/common/test/data/xml_dag/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/xml_dag/course/2012_Fall.xml b/common/test/data/xml_dag/course/2012_Fall.xml new file mode 100644 index 0000000000..40193e8b87 --- /dev/null +++ b/common/test/data/xml_dag/course/2012_Fall.xml @@ -0,0 +1,13 @@ + + + + + + + + + + + + + diff --git a/common/test/data/xml_dag/html/toyhtml.html b/common/test/data/xml_dag/html/toyhtml.html new file mode 100644 index 0000000000..08f0504f36 --- /dev/null +++ b/common/test/data/xml_dag/html/toyhtml.html @@ -0,0 +1 @@ +Sample diff --git a/common/test/data/xml_dag/html/toyhtml.xml b/common/test/data/xml_dag/html/toyhtml.xml new file mode 100644 index 0000000000..4e84f53549 --- /dev/null +++ b/common/test/data/xml_dag/html/toyhtml.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/xml_dag/policies/2012_Fall.json b/common/test/data/xml_dag/policies/2012_Fall.json new file mode 100644 index 0000000000..82bfa20787 --- /dev/null +++ b/common/test/data/xml_dag/policies/2012_Fall.json @@ -0,0 +1,8 @@ +{ + "course/2012_Fall": { + "graceperiod": "2 days 5 hours 59 minutes 59 seconds", + "start": "2015-07-17T12:00", + "display_name": "Toy Course", + "graded": "true", + } +} diff --git a/common/test/data/xml_dag/static/just_a_test.jpg b/common/test/data/xml_dag/static/just_a_test.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6bb7f377a03e406c6a448c7adfc4994f03294874 GIT binary patch literal 547 zcmbu4y$ZrW5QJy$P%JF&z*6vhk`#f&2mxOqXh0hU30UUc#E0;w@kv}S1O>&IBD3A{ z?aqvx + + +