diff --git a/cms/djangoapps/contentstore/tests/test_assets.py b/cms/djangoapps/contentstore/tests/test_assets.py index cde40d502e..b627237729 100644 --- a/cms/djangoapps/contentstore/tests/test_assets.py +++ b/cms/djangoapps/contentstore/tests/test_assets.py @@ -10,6 +10,8 @@ from unittest import TestCase, skip from .utils import CourseTestCase from django.core.urlresolvers import reverse from contentstore.views import assets +from xmodule.contentstore.content import StaticContent +from xmodule.modulestore import Location class AssetsTestCase(CourseTestCase): @@ -35,6 +37,11 @@ class AssetsTestCase(CourseTestCase): content = json.loads(resp.content) self.assertIsInstance(content, list) + def test_static_url_generation(self): + location = Location(['i4x', 'foo', 'bar', 'asset', 'my_file_name.jpg']) + path = StaticContent.get_static_path_from_location(location) + self.assertEquals(path, '/static/my_file_name.jpg') + class UploadTestCase(CourseTestCase): """ diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 96ea9556b2..838af2cafa 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/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index e4201cddd7..1c22114d76 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,12 @@ 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) return response diff --git a/cms/static/js/views/assets.js b/cms/static/js/views/assets.js index 282aeab69c..4b0b97180a 100644 --- a/cms/static/js/views/assets.js +++ b/cms/static/js/views/assets.js @@ -96,7 +96,7 @@ function displayFinishedUpload(xhr) { } var resp = JSON.parse(xhr.responseText); - $('.upload-modal .embeddable-xml-input').val(xhr.getResponseHeader('asset_url')); + $('.upload-modal .embeddable-xml-input').val(resp.portable_url); $('.upload-modal .embeddable').show(); $('.upload-modal .file-name').hide(); $('.upload-modal .progress-fill').html(resp.msg); 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/cms/templates/widgets/html-edit.html b/cms/templates/widgets/html-edit.html index 879ae43e07..34866321c4 100644 --- a/cms/templates/widgets/html-edit.html +++ b/cms/templates/widgets/html-edit.html @@ -1,6 +1,6 @@ <%! from django.utils.translation import ugettext as _ %> -
+
  • ${_("Visual")}
    • diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 28a78ea8c1..eaf3f48dea 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -58,6 +58,20 @@ class StaticContent(object): else: return None + @staticmethod + def get_static_path_from_location(location): + """ + This utility static method will take a location identifier and create a 'durable' /static/.. URL representation of it. + This link is 'durable' as it can maintain integrity across cloning of courseware across course-ids, e.g. reruns of + courses. + In the LMS/CMS, we have runtime link-rewriting, so at render time, this /static/... format will get translated into + the actual /c4x/... path which the client needs to reference static content + """ + 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: diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 1cc7bed948..567f5c7eef 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -14,6 +14,7 @@ from xmodule.stringify import stringify_children from xmodule.x_module import XModule from xmodule.xml_module import XmlDescriptor, name_to_pathname import textwrap +from xmodule.contentstore.content import StaticContent log = logging.getLogger("mitx.courseware") @@ -79,6 +80,17 @@ class HtmlDescriptor(HtmlFields, XmlDescriptor, EditingDescriptor): nc.append(candidate[:-4] + '.html') return candidates + nc + def get_context(self): + """ + an override to add in specific rendering context, in this case we need to + add in a base path to our c4x content addressing scheme + """ + _context = EditingDescriptor.get_context(self) + # Add some specific HTML rendering context when editing HTML modules where we pass + # the root /c4x/ url for assets. This allows client-side substitutions to occur. + _context.update({'base_asset_url': StaticContent.get_base_url_path_for_course_assets(self.location) + '/'}) + return _context + # NOTE: html descriptors are special. We do not want to parse and # export them ourselves, because that can break things (e.g. lxml # adds body tags when it exports, but they should just be html diff --git a/common/lib/xmodule/xmodule/js/fixtures/html-edit-with-links.html b/common/lib/xmodule/xmodule/js/fixtures/html-edit-with-links.html new file mode 100644 index 0000000000..46d6313699 --- /dev/null +++ b/common/lib/xmodule/xmodule/js/fixtures/html-edit-with-links.html @@ -0,0 +1,10 @@ +
    + +
    + + +
    +
    \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/js/spec/html/edit_spec.coffee b/common/lib/xmodule/xmodule/js/spec/html/edit_spec.coffee index 554235ef9c..11aaf8e526 100644 --- a/common/lib/xmodule/xmodule/js/spec/html/edit_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/html/edit_spec.coffee @@ -48,6 +48,16 @@ describe 'HTMLEditingDescriptor', -> expect(@descriptor.showingVisualEditor).toEqual(true) data = @descriptor.save().data expect(data).toEqual('from visual editor') + it 'Performs link rewriting for static assets when saving', -> + visualEditorStub = + isDirty: () -> true + getContent: () -> 'from visual editor with /c4x/foo/bar/asset/image.jpg' + spyOn(@descriptor, 'getVisualEditor').andCallFake () -> + visualEditorStub + expect(@descriptor.showingVisualEditor).toEqual(true) + @descriptor.base_asset_url = '/c4x/foo/bar/asset/' + data = @descriptor.save().data + expect(data).toEqual('from visual editor with /static/image.jpg') describe 'Can switch to Advanced Editor', -> beforeEach -> loadFixtures 'html-edit.html' @@ -88,3 +98,23 @@ describe 'HTMLEditingDescriptor', -> expect(visualEditorStub.isDirty()).toEqual(false) expect(visualEditorStub.getContent()).toEqual('Advanced Editor Text') expect(visualEditorStub.startContent).toEqual('Advanced Editor Text') + it 'When switching to visual editor links are rewritten to c4x format', -> + loadFixtures 'html-edit-with-links.html' + @descriptor = new HTMLEditingDescriptor($('.html-edit')) + @descriptor.base_asset_url = '/c4x/foo/bar/asset/' + @descriptor.showingVisualEditor = false + + visualEditorStub = + isNotDirty: false + content: 'not set' + startContent: 'not set', + focus: () -> true + isDirty: () -> not @isNotDirty + setContent: (x) -> @content = x + getContent: -> @content + + @descriptor.showVisualEditor(visualEditorStub) + expect(@descriptor.showingVisualEditor).toEqual(true) + expect(visualEditorStub.isDirty()).toEqual(false) + expect(visualEditorStub.getContent()).toEqual('Advanced Editor Text with link /c4x/foo/bar/asset/dummy.jpg') + expect(visualEditorStub.startContent).toEqual('Advanced Editor Text with link /c4x/foo/bar/asset/dummy.jpg') \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/js/src/html/edit.coffee b/common/lib/xmodule/xmodule/js/src/html/edit.coffee index eae9df0f20..f4510ff89c 100644 --- a/common/lib/xmodule/xmodule/js/src/html/edit.coffee +++ b/common/lib/xmodule/xmodule/js/src/html/edit.coffee @@ -3,6 +3,9 @@ class @HTMLEditingDescriptor constructor: (element) -> @element = element; + @base_asset_url = @element.find("#editor-tab").data('base-asset-url') + if @base_asset_url == undefined + @base_asset_url = null @advanced_editor = CodeMirror.fromTextArea($(".edit-box", @element)[0], { mode: "text/html" @@ -47,7 +50,7 @@ class @HTMLEditingDescriptor setup : @setupTinyMCE, # Cannot get access to tinyMCE Editor instance (for focusing) until after it is rendered. # The tinyMCE callback passes in the editor as a paramter. - init_instance_callback: @focusVisualEditor + init_instance_callback: @initInstanceCallback }) @showingVisualEditor = true @@ -95,21 +98,34 @@ class @HTMLEditingDescriptor # Show the Advanced (codemirror) Editor. Pulled out as a helper method for unit testing. showAdvancedEditor: (visualEditor) -> if visualEditor.isDirty() - @advanced_editor.setValue(visualEditor.getContent({no_events: 1})) + content = @rewriteStaticLinks(visualEditor.getContent({no_events: 1}), @base_asset_url, '/static/') + @advanced_editor.setValue(content) @advanced_editor.setCursor(0) @advanced_editor.refresh() @advanced_editor.focus() @showingVisualEditor = false + rewriteStaticLinks: (content, from, to) -> + if from == null || to == null + return content + + regex = new RegExp(from, 'g') + return content.replace(regex, to) + # Show the Visual (tinyMCE) Editor. Pulled out as a helper method for unit testing. showVisualEditor: (visualEditor) -> - visualEditor.setContent(@advanced_editor.getValue()) # In order for isDirty() to return true ONLY if edits have been made after setting the text, # both the startContent must be sync'ed up and the dirty flag set to false. - visualEditor.startContent = visualEditor.getContent({format: "raw", no_events: 1}); + content = @rewriteStaticLinks(@advanced_editor.getValue(), '/static/', @base_asset_url) + visualEditor.setContent(content) + visualEditor.startContent = content @focusVisualEditor(visualEditor) @showingVisualEditor = true + initInstanceCallback: (visualEditor) => + visualEditor.setContent(@rewriteStaticLinks(@advanced_editor.getValue(), '/static/', @base_asset_url)) + @focusVisualEditor(visualEditor) + focusVisualEditor: (visualEditor) => visualEditor.focus() # Need to mark editor as not dirty both when it is initially created and when we switch back to it. @@ -131,5 +147,5 @@ class @HTMLEditingDescriptor text = @advanced_editor.getValue() visualEditor = @getVisualEditor() if @showingVisualEditor and visualEditor.isDirty() - text = visualEditor.getContent({no_events: 1}) + text = @rewriteStaticLinks(visualEditor.getContent({no_events: 1}), @base_asset_url, '/static/') data: text 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 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 @@ +