From 6200b2903fadc0c73bb1cd2aba8aca7f3ca2ac4b Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Tue, 30 Jul 2013 14:58:27 -0400 Subject: [PATCH 01/12] have the Files and Upload pages surface a 'portable_url' which uses the /static/ shorthand which is more portable across course runs --- cms/djangoapps/contentstore/views/assets.py | 4 +++- cms/templates/asset_index.html | 4 ++-- common/lib/xmodule/xmodule/contentstore/content.py | 7 +++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index e4201cddd7..11bea55833 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -105,6 +105,7 @@ def asset_index(request, org, course, name): asset_location = StaticContent.compute_location(asset_id['org'], asset_id['course'], asset_id['name']) display_info['url'] = StaticContent.get_url_path_from_location(asset_location) + display_info['portable_url'] = StaticContent.get_static_path_from_location(asset_location) # note, due to the schema change we may not have a 'thumbnail_location' in the result set _thumbnail_location = asset.get('thumbnail_location', None) @@ -187,12 +188,13 @@ def upload_asset(request, org, course, coursename): response_payload = {'displayname': content.name, 'uploadDate': get_default_time_display(readback.last_modified_at), 'url': StaticContent.get_url_path_from_location(content.location), + 'portable_url': StaticContent.get_static_path_from_location(content.location), 'thumb_url': StaticContent.get_url_path_from_location(thumbnail_location) if thumbnail_content is not None else None, 'msg': 'Upload completed' } response = JsonResponse(response_payload) - response['asset_url'] = StaticContent.get_url_path_from_location(content.location) + response['asset_url'] = StaticContent.get_static_path_from_location(content.location) return response diff --git a/cms/templates/asset_index.html b/cms/templates/asset_index.html index 6c92994a6f..c681cf5058 100644 --- a/cms/templates/asset_index.html +++ b/cms/templates/asset_index.html @@ -29,7 +29,7 @@ {{uploadDate}} - + @@ -89,7 +89,7 @@ ${asset['uploadDate']} - + diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 28a78ea8c1..b270d1dbcb 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -58,6 +58,13 @@ class StaticContent(object): else: return None + @staticmethod + def get_static_path_from_location(location): + if location is not None: + return "/static/{name}".format(**location.dict()) + else: + return None + @staticmethod def get_base_url_path_for_course_assets(loc): if loc is not None: From d329e3c833de9071da0c9251b79c8e28f48f28a1 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 31 Jul 2013 17:23:09 -0400 Subject: [PATCH 02/12] don't attempt to do URL rewriting in the import step, let the LMS/CMS runtimes do it on the fly on reference --- .../xmodule/modulestore/xml_importer.py | 89 ------------------- 1 file changed, 89 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 8df768f442..698310da87 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -1,7 +1,6 @@ import logging import os import mimetypes -from lxml.html import rewrite_links as lxml_rewrite_links from path import path from xblock.core import Scope @@ -61,45 +60,6 @@ def import_static_content(modules, course_loc, course_data_path, static_content_ return remap_dict -def verify_content_links(module, base_dir, static_content_store, link, remap_dict=None): - if link.startswith('/static/'): - # yes, then parse out the name - path = link[len('/static/'):] - - static_pathname = base_dir / path - - if os.path.exists(static_pathname): - try: - content_loc = StaticContent.compute_location(module.location.org, module.location.course, path) - filename = os.path.basename(path) - mime_type = mimetypes.guess_type(filename)[0] - - with open(static_pathname, 'rb') as f: - data = f.read() - - content = StaticContent(content_loc, filename, mime_type, data, import_path=path) - - # first let's save a thumbnail so we can get back a thumbnail location - (thumbnail_content, thumbnail_location) = static_content_store.generate_thumbnail(content) - - if thumbnail_content is not None: - content.thumbnail_location = thumbnail_location - - #then commit the content - static_content_store.save(content) - - new_link = StaticContent.get_url_path_from_location(content_loc) - - if remap_dict is not None: - remap_dict[link] = new_link - - return new_link - except Exception, e: - logging.exception('Skipping failed content load from {0}. Exception: {1}'.format(path, e)) - - return link - - 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: @@ -126,27 +86,6 @@ def import_module_from_xml(modulestore, static_content_store, course_data_path, module.children = new_locs if hasattr(module, 'data'): - # cdodge: now go through any link references to '/static/' and make sure we've imported - # it as a StaticContent asset - try: - remap_dict = {} - - # use the rewrite_links as a utility means to enumerate through all links - # in the module data. We use that to load that reference into our asset store - # IMPORTANT: There appears to be a bug in lxml.rewrite_link which makes us not be able to - # do the rewrites natively in that code. - # For example, what I'm seeing is -> - # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's - # no good, so we have to do this kludge - if isinstance(module.data, str) or isinstance(module.data, unicode): # some module 'data' fields are non strings which blows up the link traversal code - lxml_rewrite_links(module.data, lambda link: verify_content_links(module, course_data_path, static_content_store, link, remap_dict)) - - for key in remap_dict.keys(): - module.data = module.data.replace(key, remap_dict[key]) - - except Exception: - logging.exception("failed to rewrite links on {0}. Continuing...".format(module.location)) - modulestore.update_item(module.location, module.data) if module.has_children: @@ -166,9 +105,6 @@ def import_course_from_xml(modulestore, static_content_store, course_data_path, {"type": "discussion", "name": "Discussion"}, {"type": "wiki", "name": "Wiki"}] # note, add 'progress' when we can support it on Edge - # a bit of a hack, but typically the "course image" which is shown on marketing pages is hard coded to /images/course_image.jpg - # so let's make sure we import in case there are no other references to it in the modules - verify_content_links(module, course_data_path, static_content_store, '/static/images/course_image.jpg') import_module_from_xml(modulestore, static_content_store, course_data_path, module, target_location_namespace, verbose=verbose) @@ -241,10 +177,6 @@ def import_from_xml(store, data_dir, course_dirs=None, import_module(module, store, course_data_path, static_content_store) - # a bit of a hack, but typically the "course image" which is shown on marketing pages is hard coded to /images/course_image.jpg - # so let's make sure we import in case there are no other references to it in the modules - verify_content_links(module, course_data_path, static_content_store, '/static/images/course_image.jpg') - course_items.append(module) # then import all the static content @@ -302,27 +234,6 @@ def import_module(module, store, course_data_path, static_content_store, allow_n module_data = {} if 'data' in content: module_data = content['data'] - - # cdodge: now go through any link references to '/static/' and make sure we've imported - # it as a StaticContent asset - try: - remap_dict = {} - - # use the rewrite_links as a utility means to enumerate through all links - # in the module data. We use that to load that reference into our asset store - # IMPORTANT: There appears to be a bug in lxml.rewrite_link which makes us not be able to - # do the rewrites natively in that code. - # For example, what I'm seeing is -> - # Note the dropped element closing tag. This causes the LMS to fail when rendering modules - that's - # no good, so we have to do this kludge - if isinstance(module_data, str) or isinstance(module_data, unicode): # some module 'data' fields are non strings which blows up the link traversal code - lxml_rewrite_links(module_data, lambda link: verify_content_links(module, course_data_path, static_content_store, link, remap_dict)) - - for key in remap_dict.keys(): - module_data = module_data.replace(key, remap_dict[key]) - - except Exception: - logging.exception("failed to rewrite links on {0}. Continuing...".format(module.location)) else: module_data = content From 3dbe4c189f714a9e4930ef975109ce12da3b89c1 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 31 Jul 2013 17:47:05 -0400 Subject: [PATCH 03/12] add unit tests around asserting that /static/ links are not re-written on import --- cms/djangoapps/contentstore/tests/test_contentstore.py | 10 ++++++++++ common/test/data/toy/course/2012_Fall.xml | 1 + common/test/data/toy/html/toyhtml.html | 1 + common/test/data/toy/html/toyhtml.xml | 1 + 4 files changed, 13 insertions(+) create mode 100644 common/test/data/toy/html/toyhtml.html create mode 100644 common/test/data/toy/html/toyhtml.xml diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 4c9fcf7f81..d01cd64a34 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -303,6 +303,16 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): num_drafts = self._get_draft_counts(course) self.assertEqual(num_drafts, 1) + def test_no_static_link_rewrites_on_import(self): + module_store = modulestore('direct') + import_from_xml(module_store, 'common/test/data/', ['toy']) + + handouts = module_store.get_item(Location(['i4x', 'edX', 'toy', 'course_info', 'handouts', None])) + self.assertIn('/static/', handouts.data) + + handouts = module_store.get_item(Location(['i4x', 'edX', 'toy', 'html', 'toyhtml', None])) + self.assertIn('/static/', handouts.data) + def test_import_textbook_as_content_element(self): module_store = modulestore('direct') import_from_xml(module_store, 'common/test/data/', ['toy']) diff --git a/common/test/data/toy/course/2012_Fall.xml b/common/test/data/toy/course/2012_Fall.xml index 679f7bbfdb..2fd5401c24 100644 --- a/common/test/data/toy/course/2012_Fall.xml +++ b/common/test/data/toy/course/2012_Fall.xml @@ -4,6 +4,7 @@ +