lots of tweeks to better support importing of existing courseware

cms/djangoapps/contentstore/views.py

@@ -287,9 +287,13 @@ def edit_unit(request, location):
     # TODO (cpennington): If we share units between courses,
     # this will need to change to check permissions correctly so as
     # to pick the correct parent subsection
+
+    logging.debug('looking for parent of {0}'.format(location))
+
     containing_subsection_locs = modulestore().get_parent_locations(location)
     containing_subsection = modulestore().get_item(containing_subsection_locs[0])
 
+    logging.debug('looking for parent of {0}'.format(containing_subsection.location))
     containing_section_locs = modulestore().get_parent_locations(containing_subsection.location)
     containing_section = modulestore().get_item(containing_section_locs[0])
 
@@ -997,7 +1001,8 @@ def import_course(request, org, course, name):
 
         data_root = path(settings.GITHUB_REPO_ROOT)
 
-        course_dir = data_root / "{0}-{1}-{2}".format(org, course, name)
+        course_subdir = "{0}-{1}-{2}".format(org, course, name)
+        course_dir = data_root / course_subdir
         if not course_dir.isdir():
             os.mkdir(course_dir)
 
@@ -1033,7 +1038,7 @@ def import_course(request, org, course, name):
                 shutil.move(r/fname, course_dir)
 
         module_store, course_items = import_from_xml(modulestore('direct'), settings.GITHUB_REPO_ROOT,
-            [course_dir], load_error_modules=False, static_content_store=contentstore(), target_location_namespace = Location(location))
+            [course_subdir], load_error_modules=False, static_content_store=contentstore(), target_location_namespace = Location(location))
 
         # we can blow this away when we're done importing.
         shutil.rmtree(course_dir)

common/djangoapps/mitxmako/shortcuts.py

@@ -42,7 +42,7 @@ def render_to_string(template_name, dictionary, context=None, namespace='main'):
         context_dictionary.update(context)
     # fetch and render template
     template = middleware.lookup[namespace].get_template(template_name)
-    return template.render(**context_dictionary)
+    return template.render_unicode(**context_dictionary)
 
 
 def render_to_response(template_name, dictionary, context_instance=None, namespace='main', **kwargs):

common/djangoapps/static_replace.py

@@ -5,6 +5,10 @@ from staticfiles.storage import staticfiles_storage
 from staticfiles import finders
 from django.conf import settings
 
+from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.xml import XMLModuleStore
+from xmodule.contentstore.content import StaticContent
+
 log = logging.getLogger(__name__)
 
 def try_staticfiles_lookup(path):
@@ -22,7 +26,7 @@ def try_staticfiles_lookup(path):
     return url
 
 
-def replace(static_url, prefix=None):
+def replace(static_url, prefix=None, course_namespace=None):
     if prefix is None:
         prefix = ''
     else:
@@ -41,13 +45,23 @@ def replace(static_url, prefix=None):
         return static_url.group(0)
     else:
         # don't error if file can't be found
-        url = try_staticfiles_lookup(prefix + static_url.group('rest'))
-        return "".join([quote, url, quote])
+        # cdodge: to support the change over to Mongo backed content stores, lets
+        # use the utility functions in StaticContent.py
+        if static_url.group('prefix') == '/static/' and not isinstance(modulestore(), XMLModuleStore):
+            if course_namespace is None:
+                raise BaseException('You must pass in course_namespace when remapping static content urls with MongoDB stores')
+            url = StaticContent.convert_legacy_static_url(static_url.group('rest'), course_namespace)
+        else:
+            url = try_staticfiles_lookup(prefix + static_url.group('rest'))
+
+        new_link = "".join([quote, url, quote])
+        return new_link
 
 
-def replace_urls(text, staticfiles_prefix=None, replace_prefix='/static/'):
+
+def replace_urls(text, staticfiles_prefix=None, replace_prefix='/static/', course_namespace=None):
     def replace_url(static_url):
-        return replace(static_url, staticfiles_prefix)
+        return replace(static_url, staticfiles_prefix, course_namespace = course_namespace)
 
     return re.sub(r"""
         (?x)                 # flags=re.VERBOSE

common/djangoapps/xmodule_modifiers.py

@@ -50,7 +50,7 @@ def replace_course_urls(get_html, course_id):
         return replace_urls(get_html(), staticfiles_prefix='/courses/'+course_id, replace_prefix='/course/')
     return _get_html
 
-def replace_static_urls(get_html, prefix):
+def replace_static_urls(get_html, prefix, course_namespace=None):
     """
     Updates the supplied module with a new get_html function that wraps
     the old get_html function and substitutes urls of the form /static/...
@@ -59,7 +59,7 @@ def replace_static_urls(get_html, prefix):
 
     @wraps(get_html)
     def _get_html():
-        return replace_urls(get_html(), staticfiles_prefix=prefix)
+        return replace_urls(get_html(), staticfiles_prefix=prefix, course_namespace = course_namespace)
     return _get_html
 
 

common/lib/xmodule/setup.py

@@ -37,6 +37,8 @@ setup(
             "discussion = xmodule.discussion_module:DiscussionDescriptor",
             "course_info = xmodule.html_module:HtmlDescriptor",
             "static_tab = xmodule.html_module:HtmlDescriptor",
+            "custom_tag_template = xmodule.raw_module:RawDescriptor",
+            "about = xmodule.html_module:HtmlDescriptor"
         ]
     }
 )

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

@@ -62,6 +62,13 @@ class StaticContent(object):
     @staticmethod
     def get_id_from_path(path):
         return get_id_from_location(get_location_from_path(path))
+
+    @staticmethod
+    def convert_legacy_static_url(path, course_namespace):
+        loc = StaticContent.compute_location(course_namespace.org, course_namespace.course, path)
+        return StaticContent.get_url_path_from_location(loc)
+
+
     
 
 class ContentStore(object):

common/lib/xmodule/xmodule/course_module.py

@@ -202,7 +202,7 @@ class CourseDescriptor(SequenceDescriptor):
         # Try to load grading policy
         paths = ['grading_policy.json']
         if policy_dir:
-            paths = [policy_dir + 'grading_policy.json'] + paths
+            paths = [policy_dir + '/grading_policy.json'] + paths
 
         policy = json.loads(cls.read_grading_policy(paths, system))
         
@@ -394,3 +394,4 @@ class CourseDescriptor(SequenceDescriptor):
         return self.location.org
 
 
+

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -1,5 +1,6 @@
 import pymongo
 import sys
+import logging
 
 from bson.son import SON
 from fs.osfs import OSFS
@@ -49,6 +50,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 self.load_item, resources_fs, error_tracker, render_template)
         self.modulestore = modulestore
         self.module_data = module_data
+
         self.default_class = default_class
 
     def load_item(self, location):

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -427,42 +427,38 @@ class XMLModuleStore(ModuleStoreBase):
 
             # now import all pieces of course_info which is expected to be stored
             # in <content_dir>/info or <content_dir>/info/<url_name>
-            if url_name:
-                info_path = self.data_dir / course_dir / 'info' / url_name
-
-            if not os.path.exists(info_path):
-                info_path = self.data_dir / course_dir / 'info'
-
-            # we have a fixed number of .html info files that we expect there
-            for info_filename in ['handouts', 'guest_handouts', 'updates', 'guest_updates']:
-                filepath = info_path / info_filename + '.html'
-                if os.path.exists(filepath):
-                    with open(filepath) as info_file:
-                        html = info_file.read().decode('utf-8')
-                        loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, 'course_info', info_filename)
-                        info_module = HtmlDescriptor(system, definition={'data' : html}, **{'location' : loc})
-                        self.modules[course_id][info_module.location] = info_module
+            self.load_extra_content(system, course_descriptor, 'course_info', self.data_dir / course_dir / 'info', course_dir, url_name)
 
             # now import all static tabs which are expected to be stored in
-            # in <content_dir>/tabs or <content_dir>/tabs/<url_name>
-            if url_name:
-                tabs_path = self.data_dir / course_dir / 'tabs' / url_name
+            # in <content_dir>/tabs or <content_dir>/tabs/<url_name>           
+            self.load_extra_content(system, course_descriptor, 'static_tab', self.data_dir / course_dir / 'tabs', course_dir, url_name)
 
-            if not os.path.exists(tabs_path):
-                tabs_path = self.data_dir / course_dir / 'tabs'
+            self.load_extra_content(system, course_descriptor, 'custom_tag_template', self.data_dir / course_dir / 'custom_tags', course_dir, url_name)
 
-            for tab_filepath in glob.glob(tabs_path / '*.html'):
-                with open(tab_filepath) as tab_file:
-                    html = tab_file.read().decode('utf-8')
-                    # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix
-                    slug = os.path.splitext(os.path.basename(tab_filepath))[0]
-                    loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, 'static_tab', slug)
-                    tab_module = HtmlDescriptor(system, definition={'data' : html}, **{'location' : loc})
-                    self.modules[course_id][tab_module.location] = tab_module               
+            self.load_extra_content(system, course_descriptor, 'about', self.data_dir / course_dir / 'about', course_dir, url_name)
 
             log.debug('========> Done with course import from {0}'.format(course_dir))
             return course_descriptor
 
+    def load_extra_content(self, system, course_descriptor, category, base_dir, course_dir, url_name):
+        if url_name:
+            path =  base_dir / url_name
+
+        if not os.path.exists(path):
+            path = base_dir
+
+        for filepath in glob.glob(path/ '*'):
+            with open(filepath) as f:
+                try:
+                    html = f.read().decode('utf-8')
+                    # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix
+                    slug = os.path.splitext(os.path.basename(filepath))[0]
+                    loc = Location('i4x', course_descriptor.location.org, course_descriptor.location.course, category, slug)
+                    module = HtmlDescriptor(system, definition={'data' : html}, **{'location' : loc})
+                    module.metadata['data_dir'] = course_dir
+                    self.modules[course_descriptor.id][module.location] = module   
+                except Exception, e:
+                    logging.exception("Failed to load {0}. Skipping... Exception: {1}".format(filepath, str(e)))          
 
     def get_instance(self, course_id, location, depth=0):
         """

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

@@ -1,6 +1,7 @@
 import logging
 import os
 import mimetypes
+from lxml.html import rewrite_links as lxml_rewrite_links
 
 from .xml import XMLModuleStore
 from .exceptions import DuplicateItemError
@@ -9,7 +10,7 @@ from xmodule.contentstore.content import StaticContent, XASSET_SRCREF_PREFIX
 
 log = logging.getLogger(__name__)
 
-def import_static_content(modules, data_dir, static_content_store):
+def import_static_content(modules, data_dir, static_content_store, target_location_namespace):
     
     remap_dict = {}
 
@@ -31,15 +32,13 @@ def import_static_content(modules, data_dir, static_content_store):
     # now import all static assets
     static_dir = '{0}/static/'.format(course_data_dir)
 
-    logging.debug("Importing static assets in {0}".format(static_dir))
-
     for dirname, dirnames, filenames in os.walk(static_dir):
         for filename in filenames:
 
             try:
                 content_path = os.path.join(dirname, filename)
                 fullname_with_subpath = content_path.replace(static_dir, '')  # strip away leading path from the name
-                content_loc = StaticContent.compute_location(course_loc.org, course_loc.course, fullname_with_subpath)
+                content_loc = StaticContent.compute_location(target_location_namespace.org, target_location_namespace.course, fullname_with_subpath)
                 mime_type = mimetypes.guess_type(filename)[0]
 
                 f = open(content_path, 'rb')
@@ -59,12 +58,50 @@ def import_static_content(modules, data_dir, static_content_store):
 
                 #store the remapping information which will be needed to subsitute in the module data
                 remap_dict[fullname_with_subpath] = content_loc.name
-
             except:
-                raise    
+                raise 
 
     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]
+
+                f = open(static_pathname, 'rb')
+                data = f.read()
+                f.close()
+
+                content = StaticContent(content_loc, filename, mime_type, data) 
+
+                # first let's save a thumbnail so we can get back a thumbnail location
+                thumbnail_content = static_content_store.generate_thumbnail(content)
+
+                if thumbnail_content is not None:
+                    content.thumbnail_location = thumbnail_content.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_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):
@@ -94,15 +131,20 @@ def import_from_xml(store, data_dir, course_dirs=None,
     # method on XmlModuleStore.
     course_items = []
     for course_id in module_store.modules.keys():
-        remap_dict = {}
+
+        course_data_dir = None
+        for module in module_store.modules[course_id].itervalues():
+            if module.category == 'course':
+                course_data_dir = module.metadata['data_dir']
+
         if static_content_store is not None:
-            remap_dict = import_static_content(module_store.modules[course_id], data_dir, static_content_store)
+            import_static_content(module_store.modules[course_id], data_dir, static_content_store, 
+                target_location_namespace if target_location_namespace is not None else  module_store.modules[course_id].location)
 
         for module in module_store.modules[course_id].itervalues():
 
             # 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':
@@ -120,8 +162,8 @@ def import_from_xml(store, data_dir, course_dirs=None,
                         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)
+
+                        new_locs.append(new_child_loc.url())
 
                     module.definition['children'] = new_locs
 
@@ -129,22 +171,37 @@ def import_from_xml(store, data_dir, course_dirs=None,
             if module.category == 'course':
                 # HACK: for now we don't support progress tabs. There's a special metadata configuration setting for this.
                 module.metadata['hide_progress_tab'] = True
+
+                # 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, data_dir / course_data_dir, static_content_store, '/static/images/course_image.jpg')
+
                 course_items.append(module)
 
             if 'data' in module.definition:
                 module_data = module.definition['data']
 
-                # cdodge: update any references to the static content paths
-                # This is a bit brute force - simple search/replace - but it's unlikely that such references to '/static/....'
-                # would occur naturally (in the wild)
-                # @TODO, sorry a bit of technical debt here. There are some helper methods in xmodule_modifiers.py and static_replace.py which could
-                # better do the url replace on the html rendering side rather than on the ingest side
-                try:
-                    if '/static/' in module_data:
-                        for subkey in remap_dict.keys():
-                            module_data = module_data.replace('/static/' + subkey, 'xasset:' + remap_dict[subkey])
-                except:
-                    pass    # part of the techincal debt is that module_data might not be a string (e.g. ABTest)
+                # 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, data_dir / course_data_dir, 
+                            static_content_store, link, remap_dict))                     
+
+                        for key in remap_dict.keys():
+                            module_data = module_data.replace(key, remap_dict[key])
+
+                except Exception, e:
+                    logging.exception("failed to rewrite links on {0}. Continuing...".format(module.location))
 
                 store.update_item(module.location, module_data)
 
@@ -156,4 +213,6 @@ def import_from_xml(store, data_dir, course_dirs=None,
             # inherited metadata everywhere.
             store.update_metadata(module.location, dict(module.own_metadata))
 
+
+
     return module_store, course_items

common/lib/xmodule/xmodule/template_module.py

@@ -2,6 +2,7 @@ from xmodule.x_module import XModule
 from xmodule.raw_module import RawDescriptor
 from lxml import etree
 from mako.template import Template
+from xmodule.modulestore.django import modulestore
 
 
 class CustomTagModule(XModule):
@@ -40,8 +41,7 @@ class CustomTagDescriptor(RawDescriptor):
     module_class = CustomTagModule
     template_dir_name = 'customtag'
 
-    @staticmethod
-    def render_template(system, xml_data):
+    def render_template(self, system, xml_data):
         '''Render the template, given the definition xml_data'''
         xmltree = etree.fromstring(xml_data)
         if 'impl' in xmltree.attrib:
@@ -57,15 +57,23 @@ class CustomTagDescriptor(RawDescriptor):
                                 .format(location))
 
         params = dict(xmltree.items())
-        with system.resources_fs.open('custom_tags/{name}'
-                                   .format(name=template_name)) as template:
-            return Template(template.read()).render(**params)
+
+        # cdodge: look up the template as a module
+        template_loc = self.location._replace(category='custom_tag_template', name=template_name)
+
+        template_module = modulestore().get_item(template_loc)
+        template_module_data = template_module.definition['data']
+        template = Template(template_module_data)
+        return template.render(**params)
 
 
     def __init__(self, system, definition, **kwargs):
         '''Render and save the template for this descriptor instance'''
         super(CustomTagDescriptor, self).__init__(system, definition, **kwargs)
-        self.rendered_html = self.render_template(system, definition['data'])
+
+    @property
+    def rendered_html(self):
+        return self.render_template(self.system, self.definition['data'])
 
     def export_to_file(self):
         """

lms/djangoapps/courseware/courses.py

@@ -14,6 +14,7 @@ from module_render import get_module
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
+from xmodule.contentstore.content import StaticContent
 from xmodule.modulestore.xml import XMLModuleStore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.x_module import XModule
@@ -68,8 +69,13 @@ def get_opt_course_with_access(user, course_id, action):
 def course_image_url(course):
     """Try to look up the image url for the course.  If it's not found,
     log an error and return the dead link"""
-    path = course.metadata['data_dir'] + "/images/course_image.jpg"
-    return try_staticfiles_lookup(path)
+    if isinstance(modulestore(), XMLModuleStore):
+        path = course.metadata['data_dir'] + "/images/course_image.jpg"
+        return try_staticfiles_lookup(path)
+    else:
+        loc = course.location._replace(tag='c4x', category='asset', name='images_course_image.jpg')
+        path = StaticContent.get_url_path_from_location(loc)
+        return path
 
 def find_file(fs, dirs, filename):
     """
@@ -123,14 +129,12 @@ def get_course_about_section(course, section_key):
                        'effort', 'end_date', 'prerequisites', 'ocw_links']:
 
         try:
-            fs = course.system.resources_fs
-            # first look for a run-specific version
-            dirs = [path("about") / course.url_name, path("about")]
-            filepath = find_file(fs, dirs, section_key + ".html")
-            with fs.open(filepath) as htmlFile:
-                return replace_urls(htmlFile.read().decode('utf-8'),
-                                    course.metadata['data_dir'])
-        except ResourceNotFoundError:
+            loc = course.location._replace(category='about', name=section_key)
+            item = modulestore().get_instance(course.id, loc)
+
+            return item.definition['data']
+
+        except ItemNotFoundError:
             log.warning("Missing about section {key} in course {url}".format(
                 key=section_key, url=course.location.url()))
             return None
@@ -160,8 +164,6 @@ def get_course_info_section(request, cache, course, section_key):
     loc = Location(course.location.tag, course.location.org, course.location.course, 'course_info', section_key)
     course_module = get_module(request.user, request, loc, cache, course.id)
 
-    logging.debug('course_module = {0}'.format(course_module))
-
     html = ''
 
     if course_module is not None:

lms/djangoapps/courseware/module_render.py

@@ -256,7 +256,8 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
 
     module.get_html = replace_static_urls(
         wrap_xmodule(module.get_html, module, 'xmodule_display.html'),
-        module.metadata['data_dir'] if 'data_dir' in module.metadata else '')
+        module.metadata['data_dir'] if 'data_dir' in module.metadata else '', 
+        course_namespace = module.location._replace(category=None, name=None))
 
     # Allow URLs of the form '/course/' refer to the root of multicourse directory
     #   hierarchy of this course

lms/djangoapps/courseware/tests/tests.py

@@ -247,6 +247,7 @@ class PageLoader(ActivateLoginTestCase):
         all_ok = True
         for descriptor in modstore.get_items(
                 Location(None, None, None, None, None)):
+
             n += 1
             print "Checking ", descriptor.location.url()
             #print descriptor.__class__, descriptor.location
@@ -256,9 +257,13 @@ class PageLoader(ActivateLoginTestCase):
             msg = str(resp.status_code)
 
             if resp.status_code != 302:
-                msg = "ERROR " + msg
-                all_ok = False
-                num_bad += 1
+                # cdodge: we're adding new module category as part of the Studio work
+                # such as 'course_info', etc, which are not supposed to be jump_to'able
+                # so a successful return value here should be a 404
+                if descriptor.location.category not in ['about', 'static_tab', 'course_info', 'custom_tag_template'] or resp.status_code != 404:
+                    msg = "ERROR " + msg
+                    all_ok = False
+                    num_bad += 1
             print msg
             self.assertTrue(all_ok)  # fail fast
 

lms/djangoapps/courseware/views.py

@@ -421,7 +421,7 @@ def course_about(request, course_id):
                             settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'))
 
     return render_to_response('portal/course_about.html',
-                              {'course': course,
+                              { 'course': course,
                                'registered': registered,
                                'course_target': course_target,
                                'show_courseware_link' : show_courseware_link})