Merge pull request #533 from edx/fix/cdodge/use-static-shorthand-for-asset-urls

To increase portability of courses, have the Asset Index page display the classic /static/... URL shorthand.

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):
     """

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'])

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
 
 

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);

cms/templates/asset_index.html

@@ -29,7 +29,7 @@
         {{uploadDate}}
       </td>
       <td class="embed-col">
-        <input type="text" class="embeddable-xml-input" value='{{url}}' readonly>
+        <input type="text" class="embeddable-xml-input" value='{{portable_url}}' readonly>
       </td>
       <td class="delete-col">
         <a href="#" data-tooltip="${_('Delete this asset')}" class="remove-asset-button"><span class="delete-icon"></span></a>
@@ -89,7 +89,7 @@
                 ${asset['uploadDate']}
               </td>
               <td class="embed-col">
-                <input type="text" class="embeddable-xml-input" value="${asset['url']}" readonly>
+                <input type="text" class="embeddable-xml-input" value="${asset['portable_url']}" readonly>
               </td>
               <td class="delete-col">
                 <a href="#" data-tooltip="${_('Delete this asset')}" class="remove-asset-button"><span class="delete-icon"></span></a>

cms/templates/widgets/html-edit.html

 <%! from django.utils.translation import ugettext as _ %>
 
-<div class="wrapper-comp-editor" id="editor-tab">
+<div class="wrapper-comp-editor" id="editor-tab" data-base-asset-url="${base_asset_url}">
     <section class="html-editor editor">
       <ul class="editor-tabs">
         <li><a href="#" class="visual-tab tab current" data-tab="visual">${_("Visual")}</a></li>

common/lib/xmodule/xmodule/contentstore/content.py

@@ -59,6 +59,20 @@ class StaticContent(object):
             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:
             return "/c4x/{org}/{course}/asset".format(**loc.dict())

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

common/lib/xmodule/xmodule/js/fixtures/html-edit-with-links.html

+<section class="html-edit">
+    <ul class="editor-tabs">
+        <li><a href="#" class="visual-tab tab current" data-tab="visual">Visual</a></li>
+        <li><a href="#" class="html-tab tab" data-tab="advanced">HTML</a></li>
+    </ul>
+    <div class="row">
+        <textarea class="tiny-mce">dummy text</textarea>
+        <textarea name="" class="edit-box">Advanced Editor Text with link /static/dummy.jpg</textarea>
+    </div>
+</section>
\ No newline at end of file

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

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

common/lib/xmodule/xmodule/modulestore/xml_importer.py

 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 <img src='foo.jpg' />   ->   <img src='bar.jpg'>
-            # 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 <img src='foo.jpg' />   ->   <img src='bar.jpg'>
-            # 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
 

common/test/data/toy/course/2012_Fall.xml

@@ -4,6 +4,7 @@
     <videosequence url_name="Toy_Videos">
       <html url_name="secret:toylab"/>
       <html url_name="toyjumpto"/>
+      <html url_name="toyhtml"/>
       <video url_name="Video_Resources" youtube_id_1_0="1bK-WdDi6Qw" display_name="Video Resources"/>
     </videosequence>
     <video url_name="Welcome" youtube_id_1_0="p2Q6BrNhdh8" display_name="Welcome"/>

common/test/data/toy/html/toyhtml.html

+<a href='/static/handouts/sample_handout.txt'>Sample</a>
\ No newline at end of file

common/test/data/toy/html/toyhtml.xml

+<html filename="toyhtml.html"/>
\ No newline at end of file