diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f541a43e8d..8fa3403864 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studo: Fix import/export bug with conditional modules. STUD-149 + Blades: Persist student progress in video. BLD-385. Blades: Fix for the list metadata editor that gets into a bad state where "Add" diff --git a/cms/djangoapps/contentstore/tests/test_import_nostatic.py b/cms/djangoapps/contentstore/tests/test_import.py similarity index 71% rename from cms/djangoapps/contentstore/tests/test_import_nostatic.py rename to cms/djangoapps/contentstore/tests/test_import.py index 513e30f0eb..41b26b5501 100644 --- a/cms/djangoapps/contentstore/tests/test_import_nostatic.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -1,7 +1,7 @@ #pylint: disable=E1101 -''' -Tests for importing with no static -''' +""" +Tests for import_from_xml using the mongo modulestore. +""" from django.test.client import Client from django.test.utils import override_settings @@ -32,7 +32,7 @@ TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4(). @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE) -class ContentStoreImportNoStaticTest(ModuleStoreTestCase): +class ContentStoreImportTest(ModuleStoreTestCase): """ Tests that rely on the toy and test_import_course courses. NOTE: refactor using CourseFactory so they do not. @@ -130,7 +130,52 @@ class ContentStoreImportNoStaticTest(ModuleStoreTestCase): self.assertIn('/static/', handouts.data) def test_tab_name_imports_correctly(self): - module_store, content_store, course, course_location = self.load_test_import_course() + _module_store, _content_store, course, _course_location = self.load_test_import_course() print "course tabs = {0}".format(course.tabs) - self.assertEqual(course.tabs[2]['name'],'Syllabus') - + self.assertEqual(course.tabs[2]['name'], 'Syllabus') + + def test_rewrite_reference_list(self): + module_store = modulestore('direct') + target_location = Location(['i4x', 'testX', 'conditional_copy', 'course', 'copy_run']) + import_from_xml( + module_store, + 'common/test/data/', + ['conditional'], + target_location_namespace=target_location + ) + conditional_module = module_store.get_item( + Location(['i4x', 'testX', 'conditional_copy', 'conditional', 'condone']) + ) + self.assertIsNotNone(conditional_module) + self.assertListEqual( + [ + u'i4x://testX/conditional_copy/problem/choiceprob', + u'i4x://edX/different_course/html/for_testing_import_rewrites' + ], + conditional_module.sources_list + ) + self.assertListEqual( + [ + u'i4x://testX/conditional_copy/html/congrats', + u'i4x://testX/conditional_copy/html/secret_page' + ], + conditional_module.show_tag_list + ) + + def test_rewrite_reference(self): + module_store = modulestore('direct') + target_location = Location(['i4x', 'testX', 'peergrading_copy', 'course', 'copy_run']) + import_from_xml( + module_store, + 'common/test/data/', + ['open_ended'], + target_location_namespace=target_location + ) + peergrading_module = module_store.get_item( + Location(['i4x', 'testX', 'peergrading_copy', 'peergrading', 'PeerGradingLinked']) + ) + self.assertIsNotNone(peergrading_module) + self.assertEqual( + u'i4x://testX/peergrading_copy/combinedopenended/SampleQuestion', + peergrading_module.link_to_location + ) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 9dc9b1ec81..ead07df520 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -6,6 +6,7 @@ import json from .xml import XMLModuleStore, ImportSystem, ParentTracker from xmodule.modulestore import Location +from xblock.fields import Scope, Reference, ReferenceList from xmodule.contentstore.content import StaticContent from .inheritance import own_metadata from xmodule.errortracker import make_error_tracker @@ -424,7 +425,7 @@ def import_course_draft( # 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) + descriptor.location = descriptor.location.replace(name=fn) index = int(descriptor.xml_attributes['index_in_children_list']) if index in drafts: @@ -442,13 +443,13 @@ def import_course_draft( for descriptor in drafts[key]: try: def _import_module(module): - module.location = module.location._replace(revision='draft') + module.location = module.location.replace(revision='draft') # make sure our parent has us in its list of children # 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': - non_draft_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']) @@ -456,7 +457,7 @@ def import_course_draft( # IMPORTANT: Be sure to update the sequential # in the NEW namespace - seq_location = seq_location._replace( + seq_location = seq_location.replace( org=target_location_namespace.org, course=target_location_namespace.course ) @@ -486,20 +487,21 @@ def remap_namespace(module, target_location_namespace): if target_location_namespace is None: return module + original_location = module.location + # This looks a bit wonky as we need to also change the 'name' of the # imported course to be what the caller passed in if module.location.category != 'course': - module.location = module.location._replace( + module.location = module.location.replace( tag=target_location_namespace.tag, org=target_location_namespace.org, course=target_location_namespace.course ) else: - original_location = module.location # # module is a course module # - module.location = module.location._replace( + module.location = module.location.replace( tag=target_location_namespace.tag, org=target_location_namespace.org, course=target_location_namespace.course, @@ -524,22 +526,41 @@ def remap_namespace(module, target_location_namespace): module.save() - # then remap children pointers since they too will be re-namespaced + all_fields = module.get_explicitly_set_fields_by_scope(Scope.content) + all_fields.update(module.get_explicitly_set_fields_by_scope(Scope.settings)) if hasattr(module, 'children'): - children_locs = module.children - if children_locs is not None and children_locs != []: - new_locs = [] - for child in children_locs: - child_loc = Location(child) - new_child_loc = child_loc._replace( - tag=target_location_namespace.tag, - org=target_location_namespace.org, - course=target_location_namespace.course - ) + all_fields['children'] = module.children - new_locs.append(new_child_loc.url()) + def convert_ref(reference): + """ + Convert a reference to the new namespace, but only + if the original namespace matched the original course. - module.children = new_locs + Otherwise, returns the input value. + """ + new_ref = reference + ref = Location(reference) + in_original_namespace = (original_location.tag == ref.tag and + original_location.org == ref.org and + original_location.course == ref.course) + if in_original_namespace: + new_ref = ref.replace( + tag=target_location_namespace.tag, + org=target_location_namespace.org, + course=target_location_namespace.course + ).url() + return new_ref + + for field in all_fields: + if isinstance(module.fields.get(field), Reference): + new_ref = convert_ref(getattr(module, field)) + setattr(module, field, new_ref) + module.save() + elif isinstance(module.fields.get(field), ReferenceList): + references = getattr(module, field) + new_references = [convert_ref(reference) for reference in references] + setattr(module, field, new_references) + module.save() return module diff --git a/common/test/data/conditional/README.md b/common/test/data/conditional/README.md index 84b9cba58e..f44ef84fe8 100644 --- a/common/test/data/conditional/README.md +++ b/common/test/data/conditional/README.md @@ -1,3 +1,9 @@ -course for testing conditional module +Course for testing conditional module. + +Note that 'i4x://edX/different_course/html/for_testing_import_rewrites' in sources +is only present for testing that import does NOT rewrite references to +courses that differ from the original course being imported. + +It is not expected that i4x://edX/different_course/html/for_testing_import_rewrites will render. diff --git a/common/test/data/conditional/conditional/condone.xml b/common/test/data/conditional/conditional/condone.xml index 8a2f6b9df5..586ca00312 100644 --- a/common/test/data/conditional/conditional/condone.xml +++ b/common/test/data/conditional/conditional/condone.xml @@ -1,4 +1,4 @@ - + Conditionally shown page