Merge pull request #620 from edx/feature/cdodge/convert-to-portable-links-on-import-and-clone

Feature/cdodge/convert to portable links on import and clone

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -644,6 +644,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         content_store = contentstore()
 
+        # now do the actual cloning
         clone_course(module_store, content_store, source_location, dest_location)
 
         # first assert that all draft content got cloned as well
@@ -693,6 +694,56 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
                     expected_children.append(child_loc.url())
                 self.assertEqual(expected_children, lookup_item.children)
 
+    def test_portable_link_rewrites_during_clone_course(self):
+        course_data = {
+            'org': 'MITx',
+            'number': '999',
+            'display_name': 'Robot Super Course',
+            'run': '2013_Spring'
+        }
+
+        module_store = modulestore('direct')
+        draft_store = modulestore('draft')
+        content_store = contentstore()
+
+        import_from_xml(module_store, 'common/test/data/', ['toy'])
+
+        source_course_id = 'edX/toy/2012_Fall'
+        dest_course_id = 'MITx/999/2013_Spring'
+        source_location = CourseDescriptor.id_to_location(source_course_id)
+        dest_location = CourseDescriptor.id_to_location(dest_course_id)
+
+        # let's force a non-portable link in the clone source
+        # as a final check, make sure that any non-portable links are rewritten during cloning
+        html_module_location = Location([
+            source_location.tag, source_location.org, source_location.course, 'html', 'nonportable'])
+        html_module = module_store.get_instance(source_location.course_id, html_module_location)
+
+        self.assertTrue(isinstance(html_module.data, basestring))
+        new_data = html_module.data.replace('/static/', '/c4x/{0}/{1}/asset/'.format(
+            source_location.org, source_location.course))
+        module_store.update_item(html_module_location, new_data)
+
+        html_module = module_store.get_instance(source_location.course_id, html_module_location)
+        self.assertEqual(new_data, html_module.data)
+
+        # create the destination course
+
+        resp = self.client.post(reverse('create_new_course'), course_data)
+        self.assertEqual(resp.status_code, 200)
+        data = parse_json(resp)
+        self.assertEqual(data['id'], 'i4x://MITx/999/course/2013_Spring')
+
+        # do the actual cloning
+        clone_course(module_store, content_store, source_location, dest_location)
+
+        # make sure that any non-portable links are rewritten during cloning
+        html_module_location = Location([
+            dest_location.tag, dest_location.org, dest_location.course, 'html', 'nonportable'])
+        html_module = module_store.get_instance(dest_location.course_id, html_module_location)
+
+        self.assertIn('/static/foo.jpg', html_module.data)
+
     def test_illegal_draft_crud_ops(self):
         draft_store = modulestore('draft')
         direct_store = modulestore('direct')
@@ -720,6 +771,22 @@ 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)
+
+        # first check a static asset link
+        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)
+
+        # then check a intra courseware link
+        html_module_location = Location(['i4x', 'edX', 'toy', 'html', 'nonportable_link'])
+        html_module = module_store.get_instance('edX/toy/2012_Fall', html_module_location)
+        self.assertIn('/jump_to_id/nonportable_link', 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
@@ -1384,6 +1451,31 @@ class ContentStoreTest(ModuleStoreTestCase):
                                 json.dumps({'id': del_loc.url()}), "application/json")
         self.assertEqual(200, resp.status_code)
 
+    def test_import_into_new_course_id(self):
+        module_store = modulestore('direct')
+        target_location = Location(['i4x', 'MITx', '999', 'course', '2013_Spring'])
+
+        course_data = {
+            'org': target_location.org,
+            'number': target_location.course,
+            'display_name': 'Robot Super Course',
+            'run': target_location.name
+        }
+
+        resp = self.client.post(reverse('create_new_course'), course_data)
+        self.assertEqual(resp.status_code, 200)
+        data = parse_json(resp)
+        self.assertEqual(data['id'], target_location.url())
+
+        import_from_xml(module_store, 'common/test/data/', ['simple'], target_location_namespace=target_location)
+
+        modules = module_store.get_items(Location([
+            target_location.tag, target_location.org, target_location.course, None, None, None]))
+
+        # we should have a number of modules in there
+        # we can't specify an exact number since it'll always be changing
+        self.assertGreater(len(modules), 10)
+
     def test_import_metadata_with_attempts_empty_string(self):
         module_store = modulestore('direct')
         import_from_xml(module_store, 'common/test/data/', ['simple'])

cms/djangoapps/contentstore/views/assets.py

@@ -315,6 +315,8 @@ def import_course(request, org, course, name):
 
         create_all_course_groups(request.user, course_items[0].location)
 
+        logging.debug('created all course groups at {0}'.format(course_items[0].location))
+
         return HttpResponse(json.dumps({'Status': 'OK'}))
     else:
         course_module = modulestore().get_item(location)

cms/envs/dev.py

@@ -150,14 +150,13 @@ DEBUG_TOOLBAR_PANELS = (
     'debug_toolbar.panels.sql.SQLDebugPanel',
     'debug_toolbar.panels.signals.SignalDebugPanel',
     'debug_toolbar.panels.logger.LoggingPanel',
-    'debug_toolbar_mongo.panel.MongoDebugPanel',
 
     #  Enabling the profiler has a weird bug as of django-debug-toolbar==0.9.4 and
     #  Django=1.3.1/1.4 where requests to views get duplicated (your method gets
     #  hit twice). So you can uncomment when you need to diagnose performance
     #  problems, but you shouldn't leave it on.
     #  'debug_toolbar.panels.profiling.ProfilingDebugPanel',
-    )
+)
 
 DEBUG_TOOLBAR_CONFIG = {
     'INTERCEPT_REDIRECTS': False
@@ -165,7 +164,7 @@ DEBUG_TOOLBAR_CONFIG = {
 
 # To see stacktraces for MongoDB queries, set this to True.
 # Stacktraces slow down page loads drastically (for pages with lots of queries).
-DEBUG_TOOLBAR_MONGO_STACKTRACES = True
+DEBUG_TOOLBAR_MONGO_STACKTRACES = False
 
 # disable NPS survey in dev mode
 MITX_FEATURES['STUDIO_NPS_SURVEY'] = False

cms/envs/dev_dbperf.py

+"""
+This configuration is to turn on the Django Toolbar stats for DB access stats, for performance analysis
+"""
+
+# We intentionally define lots of variables that aren't used, and
+# want to import all variables from base settings files
+# pylint: disable=W0401, W0614
+
+from .dev import *
+
+DEBUG_TOOLBAR_PANELS = (
+    'debug_toolbar.panels.version.VersionDebugPanel',
+    'debug_toolbar.panels.timer.TimerDebugPanel',
+    'debug_toolbar.panels.settings_vars.SettingsVarsDebugPanel',
+    'debug_toolbar.panels.headers.HeaderDebugPanel',
+    'debug_toolbar.panels.request_vars.RequestVarsDebugPanel',
+    'debug_toolbar.panels.sql.SQLDebugPanel',
+    'debug_toolbar.panels.signals.SignalDebugPanel',
+    'debug_toolbar.panels.logger.LoggingPanel',
+    'debug_toolbar_mongo.panel.MongoDebugPanel'
+
+    #  Enabling the profiler has a weird bug as of django-debug-toolbar==0.9.4 and
+    #  Django=1.3.1/1.4 where requests to views get duplicated (your method gets
+    #  hit twice). So you can uncomment when you need to diagnose performance
+    #  problems, but you shouldn't leave it on.
+    #  'debug_toolbar.panels.profiling.ProfilingDebugPanel',
+)
+
+# To see stacktraces for MongoDB queries, set this to True.
+# Stacktraces slow down page loads drastically (for pages with lots of queries).
+DEBUG_TOOLBAR_MONGO_STACKTRACES = True

common/lib/xmodule/xmodule/modulestore/store_utilities.py

+import re
 from xmodule.contentstore.content import StaticContent
 from xmodule.modulestore import Location
 from xmodule.modulestore.mongo import MongoModuleStore
@@ -6,7 +7,105 @@ from xmodule.modulestore.inheritance import own_metadata
 import logging
 
 
-def _clone_modules(modulestore, modules, dest_location):
+def _prefix_only_url_replace_regex(prefix):
+    """
+    Match static urls in quotes that don't end in '?raw'.
+
+    To anyone contemplating making this more complicated:
+    http://xkcd.com/1171/
+    """
+    return r"""
+        (?x)                      # flags=re.VERBOSE
+        (?P<quote>\\?['"])        # the opening quotes
+        (?P<prefix>{prefix})      # the prefix
+        (?P<rest>.*?)             # everything else in the url
+        (?P=quote)                # the first matching closing quote
+        """.format(prefix=re.escape(prefix))
+
+
+def _prefix_and_category_url_replace_regex(prefix):
+    """
+    Match static urls in quotes that don't end in '?raw'.
+
+    To anyone contemplating making this more complicated:
+    http://xkcd.com/1171/
+    """
+    return r"""
+        (?x)                      # flags=re.VERBOSE
+        (?P<quote>\\?['"])        # the opening quotes
+        (?P<prefix>{prefix})      # the prefix
+        (?P<category>[^/]+)/
+        (?P<rest>.*?)             # everything else in the url
+        (?P=quote)                # the first matching closing quote
+        """.format(prefix=re.escape(prefix))
+
+
+def rewrite_nonportable_content_links(source_course_id, dest_course_id, text):
+    """
+    Does a regex replace on non-portable links:
+         /c4x/<org>/<course>/asset/<name> -> /static/<name>
+         /jump_to/i4x://<org>/<course>/<category>/<name> -> /jump_to_id/<id>
+
+    """
+
+    org, course, run = source_course_id.split("/")
+    dest_org, dest_course, dest_run = dest_course_id.split("/")
+
+    def portable_asset_link_subtitution(match):
+        quote = match.group('quote')
+        rest = match.group('rest')
+        return quote + '/static/' + rest + quote
+
+    def portable_jump_to_link_substitution(match):
+        quote = match.group('quote')
+        rest = match.group('rest')
+        return quote + '/jump_to_id/' + rest + quote
+
+    def generic_courseware_link_substitution(match):
+        quote = match.group('quote')
+        rest = match.group('rest')
+        dest_generic_courseware_lik_base = '/courses/{org}/{course}/{run}/'.format(
+            org=dest_org, course=dest_course, run=dest_run
+        )
+        return quote + dest_generic_courseware_lik_base + rest + quote
+
+    course_location = Location(['i4x', org, course, 'course', run])
+
+    # NOTE: ultimately link updating is not a hard requirement, so if something blows up with
+    # the regex subsitution, log the error and continue
+    try:
+        c4x_link_base = '{0}/'.format(StaticContent.get_base_url_path_for_course_assets(course_location))
+        text = re.sub(_prefix_only_url_replace_regex(c4x_link_base), portable_asset_link_subtitution, text)
+    except Exception, e:
+        logging.warning("Error going regex subtituion %r on text = %r.\n\nError msg = %s", c4x_link_base, text, str(e))
+
+    try:
+        jump_to_link_base = '/courses/{org}/{course}/{run}/jump_to/i4x://{org}/{course}/'.format(
+            org=org, course=course, run=run
+        )
+        text = re.sub(_prefix_and_category_url_replace_regex(jump_to_link_base), portable_jump_to_link_substitution, text)
+    except Exception, e:
+        logging.warning("Error going regex subtituion %r on text = %r.\n\nError msg = %s", jump_to_link_base, text, str(e))
+
+    # Also, there commonly is a set of link URL's used in the format:
+    # /courses/<org>/<course>/<run> which will be broken if migrated to a different course_id
+    # so let's rewrite those, but the target will also be non-portable,
+    #
+    # Note: we only need to do this if we are changing course-id's
+    #
+    if source_course_id != dest_course_id:
+        try:
+            generic_courseware_link_base = '/courses/{org}/{course}/{run}/'.format(
+                org=org, course=course, run=run
+            )
+            text = re.sub(_prefix_only_url_replace_regex(generic_courseware_link_base), portable_asset_link_subtitution, text)
+        except Exception, e:
+            logging.warning("Error going regex subtituion %r on text = %r.\n\nError msg = %s", generic_courseware_link_base, text, str(e))
+
+    return text
+
+
+def _clone_modules(modulestore, modules, source_location, dest_location):
     for module in modules:
         original_loc = Location(module.location)
 
@@ -21,7 +120,12 @@ def _clone_modules(modulestore, modules, dest_location):
         print "Cloning module {0} to {1}....".format(original_loc, module.location)
 
         # NOTE: usage of the the internal module.xblock_kvs._data does not include any 'default' values for the fields
-        modulestore.update_item(module.location, module.xblock_kvs._data)
+        data = module.xblock_kvs._data
+        if isinstance(data, basestring):
+            data = rewrite_nonportable_content_links(
+                source_location.course_id, dest_location.course_id, data)
+
+        modulestore.update_item(module.location, data)
 
         # repoint children
         if module.has_children:
@@ -73,10 +177,10 @@ def clone_course(modulestore, contentstore, source_location, dest_location, dele
     # Get all modules under this namespace which is (tag, org, course) tuple
 
     modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, None])
-    _clone_modules(modulestore, modules, dest_location)
+    _clone_modules(modulestore, modules, source_location, dest_location)
 
     modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])
-    _clone_modules(modulestore, modules, dest_location)
+    _clone_modules(modulestore, modules, source_location, dest_location)
 
     # now iterate through all of the assets and clone them
     # first the thumbnails

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

@@ -5,6 +5,8 @@
       <html url_name="secret:toylab"/>
       <html url_name="toyjumpto"/>
       <html url_name="toyhtml"/>
+      <html url_name="nonportable"/>
+      <html url_name="nonportable_link"/>
       <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/nonportable.html

+<a href="/c4x/edX/toy/asset/foo.jpg">link</a>

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

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

common/test/data/toy/html/nonportable_link.html

+<a href="/courses/edX/toy/2012_Fall/jump_to/i4x://edX/toy/html/nonportable_link">link</a>
+

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

+<html filename="nonportable_link.html"/>