diff --git a/cms/djangoapps/contentstore/tests/test_import_draft_order.py b/cms/djangoapps/contentstore/tests/test_import_draft_order.py new file mode 100644 index 0000000000..26095bed3e --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_import_draft_order.py @@ -0,0 +1,53 @@ +from django.test.utils import override_settings + +from xmodule.modulestore.xml_importer import import_from_xml + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.django import modulestore +from xmodule.modulestore import Location + +from contentstore.tests.modulestore_config import TEST_MODULESTORE + + +# This test is in the CMS module because the test configuration to use a draft +# modulestore is dependent on django. +@override_settings(MODULESTORE=TEST_MODULESTORE) +class DraftReorderTestCase(ModuleStoreTestCase): + + def test_order(self): + store = modulestore('direct') + draft_store = modulestore('default') + import_from_xml(store, 'common/test/data/', ['import_draft_order'], draft_store=draft_store) + sequential = draft_store.get_item( + Location('i4x', 'test_org', 'import_draft_order', 'sequential', '0f4f7649b10141b0bdc9922dcf94515a', None) + ) + verticals = sequential.children + + # The order that files are read in from the file system is not guaranteed (cannot rely on + # alphabetical ordering, for example). Therefore, I have added a lot of variation in filename and desired + # ordering so that the test reliably failed with the bug, at least on Linux. + # + # 'a', 'b', 'c', 'd', and 'z' are all drafts, with 'index_in_children_list' of + # 2 , 4 , 6 , 5 , and 0 respectively. + # + # '5a05be9d59fc4bb79282c94c9e6b88c7' and 'second' are public verticals. + self.assertEqual(7, len(verticals)) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/z', verticals[0]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/5a05be9d59fc4bb79282c94c9e6b88c7', verticals[1]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/a', verticals[2]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/second', verticals[3]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/b', verticals[4]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/d', verticals[5]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/c', verticals[6]) + + # Now also test that the verticals in a second sequential are correct. + sequential = draft_store.get_item( + Location('i4x', 'test_org', 'import_draft_order', 'sequential', 'secondseq', None) + ) + verticals = sequential.children + # 'asecond' and 'zsecond' are drafts with 'index_in_children_list' 0 and 2, respectively. + # 'secondsubsection' is a public vertical. + self.assertEqual(3, len(verticals)) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/asecond', verticals[0]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/secondsubsection', verticals[1]) + self.assertEqual(u'i4x://test_org/import_draft_order/vertical/zsecond', verticals[2]) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 176710bca2..bcb75ca282 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -381,10 +381,18 @@ def import_course_draft( # create a new 'System' object which will manage the importing errorlog = make_error_tracker() + + # The course_dir as passed to ImportSystem is expected to just be relative, not + # the complete path including data_dir. ImportSystem will concatenate the two together. + data_dir = xml_module_store.data_dir + # Whether or not data_dir ends with a "/" differs in production vs. test. + if not data_dir.endswith("/"): + data_dir += "/" + draft_course_dir = draft_dir.replace(data_dir, '', 1) system = ImportSystem( xmlstore=xml_module_store, course_id=target_location_namespace.course_id, - course_dir=draft_dir, + course_dir=draft_course_dir, policy={}, error_tracker=errorlog.tracker, parent_tracker=ParentTracker(), @@ -393,6 +401,10 @@ def import_course_draft( # now walk the /vertical directory where each file in there # will be a draft copy of the Vertical + + # First it is necessary to order the draft items by their desired index in the child list + # (order os.walk returns them in is not guaranteed). + drafts = dict() for dirname, dirnames, filenames in os.walk(draft_dir + "/vertical"): for filename in filenames: module_path = os.path.join(dirname, filename) @@ -434,6 +446,29 @@ def import_course_draft( descriptor = system.process_xml(xml) + # HACK: since we are doing partial imports of drafts + # the vertical doesn't have the 'url-name' set in the + # attributes (they are normally in the parent object, + # aka sequential), so we have to replace the location.name + # with the XML filename that is part of the pack + fn, fileExtension = os.path.splitext(filename) + descriptor.location = descriptor.location._replace(name=fn) + + index = int(descriptor.xml_attributes['index_in_children_list']) + if index in drafts: + drafts[index].append(descriptor) + else: + drafts[index] = [descriptor] + + except Exception, e: + logging.exception('There was an error. {err}'.format( + err=unicode(e) + )) + + # For each index_in_children_list key, there is a list of vertical descriptors. + for key in sorted(drafts.iterkeys()): + for descriptor in drafts[key]: + try: def _import_module(module): module.location = module.location._replace(revision='draft') # make sure our parent has us in its list of children @@ -467,14 +502,6 @@ def import_course_draft( for child in module.get_children(): _import_module(child) - # HACK: since we are doing partial imports of drafts - # the vertical doesn't have the 'url-name' set in the - # attributes (they are normally in the parent object, - # aka sequential), so we have to replace the location.name - # with the XML filename that is part of the pack - fn, fileExtension = os.path.splitext(filename) - descriptor.location = descriptor.location._replace(name=fn) - _import_module(descriptor) except Exception, e: diff --git a/common/test/data/import_draft_order/chapter/3247df3732ea492380e45a4ea1918ffa.xml b/common/test/data/import_draft_order/chapter/3247df3732ea492380e45a4ea1918ffa.xml new file mode 100644 index 0000000000..5174712732 --- /dev/null +++ b/common/test/data/import_draft_order/chapter/3247df3732ea492380e45a4ea1918ffa.xml @@ -0,0 +1,4 @@ + + + + diff --git a/common/test/data/import_draft_order/course.xml b/common/test/data/import_draft_order/course.xml new file mode 100644 index 0000000000..5ef1d94a49 --- /dev/null +++ b/common/test/data/import_draft_order/course.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/course/import_draft_order.xml b/common/test/data/import_draft_order/course/import_draft_order.xml new file mode 100644 index 0000000000..951051827f --- /dev/null +++ b/common/test/data/import_draft_order/course/import_draft_order.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/import_draft_order/drafts/vertical/a.xml b/common/test/data/import_draft_order/drafts/vertical/a.xml new file mode 100644 index 0000000000..593a7c7da4 --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/a.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/asecond.xml b/common/test/data/import_draft_order/drafts/vertical/asecond.xml new file mode 100644 index 0000000000..989277e61c --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/asecond.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/b.xml b/common/test/data/import_draft_order/drafts/vertical/b.xml new file mode 100644 index 0000000000..e7402fb174 --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/b.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/c.xml b/common/test/data/import_draft_order/drafts/vertical/c.xml new file mode 100644 index 0000000000..7aaff64056 --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/c.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/d.xml b/common/test/data/import_draft_order/drafts/vertical/d.xml new file mode 100644 index 0000000000..4ef000d835 --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/d.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/z.xml b/common/test/data/import_draft_order/drafts/vertical/z.xml new file mode 100644 index 0000000000..92063711b7 --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/z.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/drafts/vertical/zsecond.xml b/common/test/data/import_draft_order/drafts/vertical/zsecond.xml new file mode 100644 index 0000000000..d217c70e8b --- /dev/null +++ b/common/test/data/import_draft_order/drafts/vertical/zsecond.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/policies/assets.json b/common/test/data/import_draft_order/policies/assets.json new file mode 100644 index 0000000000..0967ef424b --- /dev/null +++ b/common/test/data/import_draft_order/policies/assets.json @@ -0,0 +1 @@ +{} diff --git a/common/test/data/import_draft_order/policies/import_draft_order/grading_policy.json b/common/test/data/import_draft_order/policies/import_draft_order/grading_policy.json new file mode 100644 index 0000000000..2095bb70f7 --- /dev/null +++ b/common/test/data/import_draft_order/policies/import_draft_order/grading_policy.json @@ -0,0 +1 @@ +{"GRADER": [{"short_label": "HW", "min_count": 12, "type": "Homework", "drop_count": 2, "weight": 0.15}, {"min_count": 12, "type": "Lab", "drop_count": 2, "weight": 0.15}, {"short_label": "Midterm", "min_count": 1, "type": "Midterm Exam", "drop_count": 0, "weight": 0.3}, {"short_label": "Final", "min_count": 1, "type": "Final Exam", "drop_count": 0, "weight": 0.4}], "GRADE_CUTOFFS": {"Pass": 0.5}} diff --git a/common/test/data/import_draft_order/policies/import_draft_order/policy.json b/common/test/data/import_draft_order/policies/import_draft_order/policy.json new file mode 100644 index 0000000000..1f144546f0 --- /dev/null +++ b/common/test/data/import_draft_order/policies/import_draft_order/policy.json @@ -0,0 +1 @@ +{"course/import_draft_order": {"tabs": [{"type": "courseware", "name": "Courseware"}, {"type": "course_info", "name": "Course Info"}, {"type": "discussion", "name": "Discussion"}, {"type": "wiki", "name": "Wiki"}, {"type": "progress", "name": "Progress"}], "display_name": "reorder privates", "discussion_topics": {"General": {"id": "i4x-test_org-test_course-course-import_draft_order"}}}} diff --git a/common/test/data/import_draft_order/sequential/0f4f7649b10141b0bdc9922dcf94515a.xml b/common/test/data/import_draft_order/sequential/0f4f7649b10141b0bdc9922dcf94515a.xml new file mode 100644 index 0000000000..9c19b34ca7 --- /dev/null +++ b/common/test/data/import_draft_order/sequential/0f4f7649b10141b0bdc9922dcf94515a.xml @@ -0,0 +1,4 @@ + + + + diff --git a/common/test/data/import_draft_order/sequential/secondseq.xml b/common/test/data/import_draft_order/sequential/secondseq.xml new file mode 100644 index 0000000000..9176fe0677 --- /dev/null +++ b/common/test/data/import_draft_order/sequential/secondseq.xml @@ -0,0 +1,3 @@ + + + diff --git a/common/test/data/import_draft_order/vertical/5a05be9d59fc4bb79282c94c9e6b88c7.xml b/common/test/data/import_draft_order/vertical/5a05be9d59fc4bb79282c94c9e6b88c7.xml new file mode 100644 index 0000000000..065805bcd2 --- /dev/null +++ b/common/test/data/import_draft_order/vertical/5a05be9d59fc4bb79282c94c9e6b88c7.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/vertical/second.xml b/common/test/data/import_draft_order/vertical/second.xml new file mode 100644 index 0000000000..09a3852150 --- /dev/null +++ b/common/test/data/import_draft_order/vertical/second.xml @@ -0,0 +1 @@ + diff --git a/common/test/data/import_draft_order/vertical/secondsubsection.xml b/common/test/data/import_draft_order/vertical/secondsubsection.xml new file mode 100644 index 0000000000..010e72901d --- /dev/null +++ b/common/test/data/import_draft_order/vertical/secondsubsection.xml @@ -0,0 +1 @@ +