From 21f4b058133934465e1d5d983a622eac23d67b25 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 21 Aug 2013 14:46:38 -0400 Subject: [PATCH 1/2] fix some gaps which would allow the temporary xml attributes 'parent_sequential_url' and 'index_in_children_list' to get persisted in the database, whereas they are meant to be only scoped during export/import --- .../contentstore/tests/test_contentstore.py | 15 ++++++++++++++ .../xmodule/modulestore/xml_importer.py | 20 ++++++++++++------- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 96b0b84e36..1cadcd69bf 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -945,8 +945,23 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): 'vertical', 'vertical_test', None]), depth=1) self.assertTrue(getattr(vertical, 'is_draft', False)) + self.assertNotIn('index_in_children_list', child.xml_attributes) + if hasattr(vertical, 'data'): + self.assertNotIn('index_in_children_list', vertical.data) + self.assertNotIn('parent_sequential_url', vertical.xml_attributes) + if hasattr(vertical, 'data'): + self.assertNotIn('parent_sequential_url', vertical.data) + for child in vertical.get_children(): self.assertTrue(getattr(child, 'is_draft', False)) + self.assertNotIn('index_in_children_list', child.xml_attributes) + if hasattr(child, 'data'): + self.assertNotIn('index_in_children_list', child.data) + self.assertNotIn('parent_sequential_url', child.xml_attributes) + if hasattr(child, 'data'): + self.assertNotIn('parent_sequential_url', child.data) + + # make sure that we don't have a sequential that is in draft mode sequential = draft_store.get_item(Location(['i4x', 'edX', 'toy', diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 7bea0fdcac..109d759693 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -212,6 +212,15 @@ def import_module(module, store, course_data_path, static_content_store, # NOTE: It's important to use own_metadata here to avoid writing # inherited metadata everywhere. + + # remove any export/import only xml_attributes which are used to wire together draft imports + if 'parent_sequential_url' in module.xml_attributes: + del module.xml_attributes['parent_sequential_url'] + + if 'index_in_children_list' in module.xml_attributes: + del module.xml_attributes['index_in_children_list'] + module.save() + store.update_metadata(module.location, dict(own_metadata(module))) @@ -281,7 +290,7 @@ def import_course_draft(xml_module_store, store, draft_store, course_data_path, # this is to make sure private only verticals show up in the list of children since # they would have been filtered out from the non-draft store export if module.location.category == 'vertical': - module.location = module.location._replace(revision=None) + non_draft_location = module.location._replace(revision=None) sequential_url = module.xml_attributes['parent_sequential_url'] index = int(module.xml_attributes['index_in_children_list']) @@ -291,15 +300,12 @@ def import_course_draft(xml_module_store, store, draft_store, course_data_path, seq_location = seq_location._replace(org=target_location_namespace.org, course=target_location_namespace.course ) - sequential = store.get_item(seq_location) + sequential = store.get_item(seq_location, depth=0) - if module.location.url() not in sequential.children: - sequential.children.insert(index, module.location.url()) + if non_draft_location.url() not in sequential.children: + sequential.children.insert(index, non_draft_location.url()) store.update_children(sequential.location, sequential.children) - del module.xml_attributes['parent_sequential_url'] - del module.xml_attributes['index_in_children_list'] - import_module(module, draft_store, course_data_path, static_content_store, source_location_namespace, target_location_namespace, allow_not_found=True) for child in module.get_children(): From bd412182c77f50af647e3e3393ff9f381d38138c Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 22 Aug 2013 12:01:17 -0400 Subject: [PATCH 2/2] verticals shouldn't have 'data' --- cms/djangoapps/contentstore/tests/test_contentstore.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 1cadcd69bf..e909fa7c5c 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -946,11 +946,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): self.assertTrue(getattr(vertical, 'is_draft', False)) self.assertNotIn('index_in_children_list', child.xml_attributes) - if hasattr(vertical, 'data'): - self.assertNotIn('index_in_children_list', vertical.data) self.assertNotIn('parent_sequential_url', vertical.xml_attributes) - if hasattr(vertical, 'data'): - self.assertNotIn('parent_sequential_url', vertical.data) for child in vertical.get_children(): self.assertTrue(getattr(child, 'is_draft', False)) @@ -961,8 +957,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): if hasattr(child, 'data'): self.assertNotIn('parent_sequential_url', child.data) - - # make sure that we don't have a sequential that is in draft mode sequential = draft_store.get_item(Location(['i4x', 'edX', 'toy', 'sequential', 'vertical_sequential', None]))