From 36fda350408e1d2719b4e2322af654c56e8d26c6 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 8 Aug 2013 23:27:48 -0400 Subject: [PATCH 01/12] do portable link rewriting on import and add test to confirm it --- .../contentstore/tests/test_contentstore.py | 11 ++ .../xmodule/modulestore/store_utilities.py | 23 ++++ .../xmodule/modulestore/xml_importer.py | 121 ++++++++++-------- common/test/data/toy/course/2012_Fall.xml | 1 + common/test/data/toy/html/nonportable.html | 1 + common/test/data/toy/html/nonportable.xml | 1 + 6 files changed, 104 insertions(+), 54 deletions(-) create mode 100644 common/test/data/toy/html/nonportable.html create mode 100644 common/test/data/toy/html/nonportable.xml diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 23135964a9..f6dd5a24b7 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -720,6 +720,17 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): resp = self.client.get('http://localhost:8001/c4x/CDX/123123/asset/&images_circuits_Lab7Solution2.png') self.assertEqual(resp.status_code, 400) + def test_rewrite_nonportable_links_on_import(self): + module_store = modulestore('direct') + content_store = contentstore() + + import_from_xml(module_store, 'common/test/data/', ['toy'], static_content_store=content_store) + + html_module_location = Location(['i4x', 'edX', 'toy', 'html', 'nonportable']) + html_module = module_store.get_instance('edX/toy/2012_Fall', html_module_location) + + self.assertIn('/static/foo.jpg', html_module.data) + def test_delete_course(self): """ This test will import a course, make a draft item, and delete it. This will also assert that the diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index cfe0a0a6c5..bd871ad9d0 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -1,11 +1,34 @@ +import re from xmodule.contentstore.content import StaticContent from xmodule.modulestore import Location from xmodule.modulestore.mongo import MongoModuleStore from xmodule.modulestore.inheritance import own_metadata +from static_replace import _url_replace_regex import logging +def convert_to_portable_links(source_course_id, text): + """ + Does a regex replace on non-portable links: + /c4x///asset/ -> /static/ + /jump_to/i4x:///// -> /jump_to_id/ + """ + + def portable_asset_link_subtitution(match): + quote = match.group('quote') + rest = match.group('rest') + return "".join([quote, '/static/'+rest, quote]) + + org, course, run = source_course_id.split("/") + course_location = Location(['i4x', org, course, 'course', run]) + + c4x_link_base = '{0}/'.format(StaticContent.get_base_url_path_for_course_assets(course_location)) + text = re.sub(_url_replace_regex(c4x_link_base), portable_asset_link_subtitution, text) + + return text + + def _clone_modules(modulestore, modules, dest_location): for module in modules: original_loc = Location(module.location) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 698310da87..c4edb2f6d6 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -10,6 +10,7 @@ from xmodule.modulestore import Location from xmodule.contentstore.content import StaticContent from .inheritance import own_metadata from xmodule.errortracker import make_error_tracker +from .store_utilities import convert_to_portable_links log = logging.getLogger(__name__) @@ -60,54 +61,6 @@ def import_static_content(modules, course_loc, course_data_path, static_content_ return remap_dict -def import_module_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace=None, verbose=False): - # remap module to the new namespace - if target_location_namespace is not None: - # 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(tag=target_location_namespace.tag, org=target_location_namespace.org, - course=target_location_namespace.course) - else: - module.location = module.location._replace(tag=target_location_namespace.tag, org=target_location_namespace.org, - course=target_location_namespace.course, name=target_location_namespace.name) - - # then remap children pointers since they too will be re-namespaced - if module.has_children: - children_locs = module.children - 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) - - new_locs.append(new_child_loc.url()) - - module.children = new_locs - - if hasattr(module, 'data'): - modulestore.update_item(module.location, module.data) - - if module.has_children: - modulestore.update_children(module.location, module.children) - - modulestore.update_metadata(module.location, own_metadata(module)) - - -def import_course_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace=None, verbose=False): - # cdodge: more hacks (what else). Seems like we have a problem when importing a course (like 6.002) which - # does not have any tabs defined in the policy file. The import goes fine and then displays fine in LMS, - # but if someone tries to add a new tab in the CMS, then the LMS barfs because it expects that - - # if there is *any* tabs - then there at least needs to be some predefined ones - if module.tabs is None or len(module.tabs) == 0: - module.tabs = [{"type": "courseware"}, - {"type": "course_info", "name": "Course Info"}, - {"type": "discussion", "name": "Discussion"}, - {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge - - import_module_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace, verbose=verbose) - - def import_from_xml(store, data_dir, course_dirs=None, default_class='xmodule.raw_module.RawDescriptor', load_error_modules=True, static_content_store=None, target_location_namespace=None, @@ -175,7 +128,7 @@ def import_from_xml(store, data_dir, course_dirs=None, {"type": "discussion", "name": "Discussion"}, {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge - import_module(module, store, course_data_path, static_content_store) + import_module(module, store, course_data_path, static_content_store, course_location) course_items.append(module) @@ -202,12 +155,12 @@ def import_from_xml(store, data_dir, course_dirs=None, if verbose: log.debug('importing module location {0}'.format(module.location)) - import_module(module, store, course_data_path, static_content_store) + import_module(module, store, course_data_path, static_content_store, course_location) # now import any 'draft' items if draft_store is not None: import_course_draft(xml_module_store, store, draft_store, course_data_path, - static_content_store, target_location_namespace if target_location_namespace is not None + static_content_store, course_location, target_location_namespace if target_location_namespace is not None else course_location) finally: @@ -220,7 +173,7 @@ def import_from_xml(store, data_dir, course_dirs=None, return xml_module_store, course_items -def import_module(module, store, course_data_path, static_content_store, allow_not_found=False): +def import_module(module, store, course_data_path, static_content_store, source_course_location, allow_not_found=False): content = {} for field in module.fields: if field.scope != Scope.content: @@ -237,6 +190,11 @@ def import_module(module, store, course_data_path, static_content_store, allow_n else: module_data = content + if isinstance(module_data, basestring): + # we want to convert all 'non-portable' links in the module_data (if it is a string) to + # portable strings (e.g. /static/) + module_data = convert_to_portable_links(source_course_location.course_id, module_data) + if allow_not_found: store.update_item(module.location, module_data, allow_not_found=allow_not_found) else: @@ -250,7 +208,7 @@ def import_module(module, store, course_data_path, static_content_store, allow_n store.update_metadata(module.location, dict(own_metadata(module))) -def import_course_draft(xml_module_store, store, draft_store, course_data_path, static_content_store, target_location_namespace): +def import_course_draft(xml_module_store, store, draft_store, course_data_path, static_content_store, source_location_namespace, target_location_namespace): ''' This will import all the content inside of the 'drafts' folder, if it exists NOTE: This is not a full course import, basically in our current application only verticals (and downwards) @@ -307,7 +265,7 @@ def import_course_draft(xml_module_store, store, draft_store, course_data_path, 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, allow_not_found=True) + import_module(module, draft_store, course_data_path, static_content_store, source_location_namespace, allow_not_found=True) for child in module.get_children(): _import_module(child) @@ -524,3 +482,58 @@ def perform_xlint(data_dir, course_dirs, print "This course can be imported successfully." return err_cnt + + +# +# UNSURE IF THIS IS UNUSED CODE - IF SO NEEDS TO BE PRUNED. TO BE INVESTIGATED. +# +def import_module_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace=None, verbose=False): + # remap module to the new namespace + if target_location_namespace is not None: + # 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(tag=target_location_namespace.tag, org=target_location_namespace.org, + course=target_location_namespace.course) + else: + module.location = module.location._replace(tag=target_location_namespace.tag, org=target_location_namespace.org, + course=target_location_namespace.course, name=target_location_namespace.name) + + # then remap children pointers since they too will be re-namespaced + if module.has_children: + children_locs = module.children + 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) + + new_locs.append(new_child_loc.url()) + + module.children = new_locs + + if hasattr(module, 'data'): + modulestore.update_item(module.location, module.data) + + if module.has_children: + modulestore.update_children(module.location, module.children) + + modulestore.update_metadata(module.location, own_metadata(module)) + + +def import_course_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace=None, verbose=False): + # CDODGE: Is this unused code (along with import_module_from_xml)? I can't find any references to it. If so, then + # we need to delete this apparently duplicate code. + + # cdodge: more hacks (what else). Seems like we have a problem when importing a course (like 6.002) which + # does not have any tabs defined in the policy file. The import goes fine and then displays fine in LMS, + # but if someone tries to add a new tab in the CMS, then the LMS barfs because it expects that - + # if there is *any* tabs - then there at least needs to be some predefined ones + if module.tabs is None or len(module.tabs) == 0: + module.tabs = [{"type": "courseware"}, + {"type": "course_info", "name": "Course Info"}, + {"type": "discussion", "name": "Discussion"}, + {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge + + import_module_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace, verbose=verbose) + diff --git a/common/test/data/toy/course/2012_Fall.xml b/common/test/data/toy/course/2012_Fall.xml index 2fd5401c24..c2faad5727 100644 --- a/common/test/data/toy/course/2012_Fall.xml +++ b/common/test/data/toy/course/2012_Fall.xml @@ -5,6 +5,7 @@ +