Merge pull request #945 from cpennington/use-wrap-child

Use wrap_child instead of replacing get_html

cms/djangoapps/contentstore/views/preview.py

@@ -8,7 +8,7 @@ from django.core.urlresolvers import reverse
 from django.contrib.auth.decorators import login_required
 from mitxmako.shortcuts import render_to_response
 
-from xmodule_modifiers import replace_static_urls, wrap_xmodule, save_module  # pylint: disable=F0401
+from xmodule_modifiers import replace_static_urls, wrap_xmodule
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import exc_info_to_str
 from xmodule.exceptions import NotFoundError, ProcessingError
@@ -75,12 +75,10 @@ def preview_component(request, location):
         return HttpResponseForbidden()
 
     component = modulestore().get_item(location)
+    # Wrap the generated fragment in the xmodule_editor div so that the javascript
+    # can bind to it correctly
+    component.runtime.wrappers.append(partial(wrap_xmodule, 'xmodule_edit.html'))
 
-    component.get_html = wrap_xmodule(
-        component.get_html,
-        component,
-        'xmodule_edit.html'
-    )
     return render_to_response('component.html', {
         'preview': get_preview_html(request, component, 0),
         'editor': component.runtime.render(component, None, 'studio_view').content,
@@ -103,6 +101,12 @@ def preview_module_system(request, preview_id, descriptor):
         return lms_field_data(descriptor._field_data, student_data)
 
     course_id = get_course_for_item(descriptor.location).location.course_id
+
+    if descriptor.location.category == 'static_tab':
+        wrapper_template = 'xmodule_tab_display.html'
+    else:
+        wrapper_template = 'xmodule_display.html'
+
     return ModuleSystem(
         ajax_url=reverse('preview_dispatch', args=[preview_id, descriptor.location.url(), '']).rstrip('/'),
         # TODO (cpennington): Do we want to track how instructors are using the preview problems?
@@ -117,7 +121,21 @@ def preview_module_system(request, preview_id, descriptor):
         can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)),
         mixins=settings.XBLOCK_MIXINS,
         course_id=course_id,
-        anonymous_student_id='student'
+        anonymous_student_id='student',
+
+        # Set up functions to modify the fragment produced by student_view
+        wrappers=(
+            # This wrapper wraps the module in the template specified above
+            partial(wrap_xmodule, wrapper_template),
+
+            # This wrapper replaces urls in the output that start with /static
+            # with the correct course-specific url for the static content
+            partial(
+                replace_static_urls,
+                getattr(descriptor, 'data_dir', descriptor.location.course),
+                course_id=descriptor.location.org + '/' + descriptor.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE',
+            ),
+        )
     )
 
 
@@ -139,33 +157,6 @@ def load_preview_module(request, preview_id, descriptor):
             error_msg=exc_info_to_str(sys.exc_info())
         ).xmodule(system)
 
-    # cdodge: Special case
-    if module.location.category == 'static_tab':
-        module.get_html = wrap_xmodule(
-            module.get_html,
-            module,
-            "xmodule_tab_display.html",
-        )
-    else:
-        module.get_html = wrap_xmodule(
-            module.get_html,
-            module,
-            "xmodule_display.html",
-        )
-
-    # we pass a partially bogus course_id as we don't have the RUN information passed yet
-    # through the CMS. Also the contentstore is also not RUN-aware at this point in time.
-    module.get_html = replace_static_urls(
-        module.get_html,
-        getattr(module, 'data_dir', module.location.course),
-        course_id=module.location.org + '/' + module.location.course + '/BOGUS_RUN_REPLACE_WHEN_AVAILABLE'
-    )
-
-    module.get_html = save_module(
-        module.get_html,
-        module
-    )
-
     return module
 
 

common/djangoapps/tests.py

-'''
-Created on Jun 6, 2013
-
-@author: dmitchell
-'''
-from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
-from student.tests.factories import AdminFactory
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-import xmodule_modifiers
-import datetime
-from pytz import UTC
-from xmodule.modulestore.tests import factories
-
-class TestXmoduleModfiers(ModuleStoreTestCase):
-
-    # FIXME disabled b/c start date inheritance is not occuring and render_... in get_html is failing due
-    # to middleware.lookup['main'] not being defined
-    def _test_add_histogram(self):
-        instructor = AdminFactory.create()
-        self.client.login(username=instructor.username, password='test')
-
-        course = CourseFactory.create(org='test',
-            number='313', display_name='histogram test')
-        section = ItemFactory.create(
-            parent_location=course.location, display_name='chapter hist',
-            category='chapter')
-        problem = ItemFactory.create(
-            parent_location=section.location, display_name='problem hist 1',
-            category='problem')
-        problem.has_score = False  # don't trip trying to retrieve db data
-
-        late_problem = ItemFactory.create(
-            parent_location=section.location, display_name='problem hist 2',
-            category='problem')
-        late_problem.start = datetime.datetime.now(UTC) + datetime.timedelta(days=32)
-        late_problem.has_score = False
-
-
-        problem_module = factories.get_test_xmodule_for_descriptor(problem)
-        problem_module.get_html = xmodule_modifiers.add_histogram(lambda:'', problem_module, instructor)
-
-        self.assertRegexpMatches(
-            problem_module.get_html(), r'.*<font color=\'green\'>Not yet</font>.*')
-
-        problem_module = factories.get_test_xmodule_for_descriptor(late_problem)
-        problem_module.get_html = xmodule_modifiers.add_histogram(lambda: '', problem_module, instructor)
-
-        self.assertRegexpMatches(
-            problem_module.get_html(), r'.*<font color=\'red\'>Yes!</font>.*')

common/djangoapps/xmodule_modifiers.py

+"""
+Functions that can are used to modify XBlock fragments for use in the LMS and Studio
+"""
+
+import datetime
 import json
 import logging
 import static_replace
 
 from django.conf import settings
-from functools import wraps
+from django.utils.timezone import UTC
 from mitxmako.shortcuts import render_to_string
+from xblock.fragment import Fragment
+
 from xmodule.seq_module import SequenceModule
 from xmodule.vertical_module import VerticalModule
-import datetime
-from django.utils.timezone import UTC
 
-log = logging.getLogger("mitx.xmodule_modifiers")
+log = logging.getLogger(__name__)
+
+
+def wrap_fragment(fragment, new_content):
+    """
+    Returns a new Fragment that has `new_content` and all
+    as its content, and all of the resources from fragment
+    """
+    wrapper_frag = Fragment(content=new_content)
+    wrapper_frag.add_frag_resources(fragment)
+    return wrapper_frag
 
 
-def wrap_xmodule(get_html, module, template, context=None):
+def wrap_xmodule(template, block, view, frag, context):  # pylint: disable=unused-argument
     """
     Wraps the results of get_html in a standard <section> with identifying
     data so that the appropriate javascript module can be loaded onto it.
@@ -26,27 +41,22 @@ def wrap_xmodule(get_html, module, template, context=None):
         class_: the module class name
         module_name: the js_module_name of the module
     """
-    if context is None:
-        context = {}
 
     # If XBlock generated this class, then use the first baseclass
     # as the name (since that's the original, unmixed class)
-    class_name = getattr(module, 'unmixed_class', module.__class__).__name__
+    class_name = getattr(block, 'unmixed_class', block.__class__).__name__
 
-    @wraps(get_html)
-    def _get_html():
-        context.update({
-            'content': get_html(),
-            'display_name': module.display_name,
-            'class_': class_name,
-            'module_name': module.js_module_name
-        })
+    template_context = {
+        'content': frag.content,
+        'display_name': block.display_name,
+        'class_': class_name,
+        'module_name': block.js_module_name,
+    }
 
-        return render_to_string(template, context)
-    return _get_html
+    return wrap_fragment(frag, render_to_string(template, template_context))
 
 
-def replace_jump_to_id_urls(get_html, course_id, jump_to_id_base_url):
+def replace_jump_to_id_urls(course_id, jump_to_id_base_url, block, view, frag, context):  # pylint: disable=unused-argument
     """
     This will replace a link between courseware in the format
     /jump_to/<id> with a URL for a page that will correctly redirect
@@ -59,38 +69,33 @@ def replace_jump_to_id_urls(get_html, course_id, jump_to_id_base_url):
         redirect. e.g. /courses/<org>/<course>/<run>/jump_to_id. NOTE the <id> will be appended to
         the end of this URL at re-write time
 
-    output: a wrapped get_html() function pointer, which, when called, will apply the
-        rewrite rules
+    output: a new :class:`~xblock.fragment.Fragment` that modifies `frag` with
+        content that has been update with /jump_to links replaced
     """
-    @wraps(get_html)
-    def _get_html():
-        return static_replace.replace_jump_to_id_urls(get_html(), course_id, jump_to_id_base_url)
-    return _get_html
+    return wrap_fragment(frag, static_replace.replace_jump_to_id_urls(frag.content, course_id, jump_to_id_base_url))
 
 
-def replace_course_urls(get_html, course_id):
+def replace_course_urls(course_id, block, view, frag, context):  # pylint: disable=unused-argument
     """
     Updates the supplied module with a new get_html function that wraps
     the old get_html function and substitutes urls of the form /course/...
     with urls that are /courses/<course_id>/...
     """
-    @wraps(get_html)
-    def _get_html():
-        return static_replace.replace_course_urls(get_html(), course_id)
-    return _get_html
+    return wrap_fragment(frag, static_replace.replace_course_urls(frag.content, course_id))
 
 
-def replace_static_urls(get_html, data_dir, course_id=None, static_asset_path=''):
+def replace_static_urls(data_dir, block, view, frag, context, course_id=None, static_asset_path=''):  # pylint: disable=unused-argument
     """
     Updates the supplied module with a new get_html function that wraps
     the old get_html function and substitutes urls of the form /static/...
     with urls that are /static/<prefix>/...
     """
-
-    @wraps(get_html)
-    def _get_html():
-        return static_replace.replace_static_urls(get_html(), data_dir, course_id, static_asset_path=static_asset_path)
-    return _get_html
+    return wrap_fragment(frag, static_replace.replace_static_urls(
+        frag.content,
+        data_dir,
+        course_id,
+        static_asset_path=static_asset_path
+    ))
 
 
 def grade_histogram(module_id):
@@ -115,22 +120,7 @@ def grade_histogram(module_id):
     return grades
 
 
-def save_module(get_html, module):
-    """
-    Updates the given get_html function for the given module to save the fields
-    after rendering.
-    """
-    @wraps(get_html)
-    def _get_html():
-        """Cache the rendered output, save, then return the output."""
-        rendered_html = get_html()
-        module.save()
-        return rendered_html
-
-    return _get_html
-
-
-def add_histogram(get_html, module, user):
+def add_histogram(user, block, view, frag, context):  # pylint: disable=unused-argument
     """
     Updates the supplied module with a new get_html function that wraps
     the output of the old get_html function with additional information
@@ -139,64 +129,60 @@ def add_histogram(get_html, module, user):
 
     Does nothing if module is a SequenceModule or a VerticalModule.
     """
-    @wraps(get_html)
-    def _get_html():
-
-        if type(module) in [SequenceModule, VerticalModule]:  # TODO: make this more general, eg use an XModule attribute instead
-            return get_html()
-
-        module_id = module.id
-        if module.descriptor.has_score:
-            histogram = grade_histogram(module_id)
-            render_histogram = len(histogram) > 0
-        else:
-            histogram = None
-            render_histogram = False
-
-        if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'):
-            [filepath, filename] = getattr(module.descriptor, 'xml_attributes', {}).get('filename', ['', None])
-            osfs = module.system.filestore
-            if filename is not None and osfs.exists(filename):
-                # if original, unmangled filename exists then use it (github
-                # doesn't like symlinks)
-                filepath = filename
-            data_dir = osfs.root_path.rsplit('/')[-1]
-            giturl = module.giturl or 'https://github.com/MITx'
-            edit_link = "%s/%s/tree/master/%s" % (giturl, data_dir, filepath)
-        else:
-            edit_link = False
-            # Need to define all the variables that are about to be used
-            giturl = ""
-            data_dir = ""
-
-        source_file = module.source_file  # source used to generate the problem XML, eg latex or word
-
-        # useful to indicate to staff if problem has been released or not
-        # TODO (ichuang): use _has_access_descriptor.can_load in lms.courseware.access, instead of now>mstart comparison here
-        now = datetime.datetime.now(UTC())
-        is_released = "unknown"
-        mstart = module.descriptor.start
-
-        if mstart is not None:
-            is_released = "<font color='red'>Yes!</font>" if (now > mstart) else "<font color='green'>Not yet</font>"
-
-        staff_context = {'fields': [(name, field.read_from(module)) for name, field in module.fields.items()],
-                         'xml_attributes': getattr(module.descriptor, 'xml_attributes', {}),
-                         'location': module.location,
-                         'xqa_key': module.xqa_key,
-                         'source_file': source_file,
-                         'source_url': '%s/%s/tree/master/%s' % (giturl, data_dir, source_file),
-                         'category': str(module.__class__.__name__),
-                         # Template uses element_id in js function names, so can't allow dashes
-                         'element_id': module.location.html_id().replace('-', '_'),
-                         'edit_link': edit_link,
-                         'user': user,
-                         'xqa_server': settings.MITX_FEATURES.get('USE_XQA_SERVER', 'http://xqa:server@content-qa.mitx.mit.edu/xqa'),
-                         'histogram': json.dumps(histogram),
-                         'render_histogram': render_histogram,
-                         'module_content': get_html(),
-                         'is_released': is_released,
-                         }
-        return render_to_string("staff_problem_info.html", staff_context)
-
-    return _get_html
+    # TODO: make this more general, eg use an XModule attribute instead
+    if isinstance(block, (SequenceModule, VerticalModule)):
+        return frag
+
+    block_id = block.id
+    if block.descriptor.has_score:
+        histogram = grade_histogram(block_id)
+        render_histogram = len(histogram) > 0
+    else:
+        histogram = None
+        render_histogram = False
+
+    if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'):
+        [filepath, filename] = getattr(block.descriptor, 'xml_attributes', {}).get('filename', ['', None])
+        osfs = block.system.filestore
+        if filename is not None and osfs.exists(filename):
+            # if original, unmangled filename exists then use it (github
+            # doesn't like symlinks)
+            filepath = filename
+        data_dir = osfs.root_path.rsplit('/')[-1]
+        giturl = block.giturl or 'https://github.com/MITx'
+        edit_link = "%s/%s/tree/master/%s" % (giturl, data_dir, filepath)
+    else:
+        edit_link = False
+        # Need to define all the variables that are about to be used
+        giturl = ""
+        data_dir = ""
+
+    source_file = block.source_file  # source used to generate the problem XML, eg latex or word
+
+    # useful to indicate to staff if problem has been released or not
+    # TODO (ichuang): use _has_access_descriptor.can_load in lms.courseware.access, instead of now>mstart comparison here
+    now = datetime.datetime.now(UTC())
+    is_released = "unknown"
+    mstart = block.descriptor.start
+
+    if mstart is not None:
+        is_released = "<font color='red'>Yes!</font>" if (now > mstart) else "<font color='green'>Not yet</font>"
+
+    staff_context = {'fields': [(name, field.read_from(block)) for name, field in block.fields.items()],
+                     'xml_attributes': getattr(block.descriptor, 'xml_attributes', {}),
+                     'location': block.location,
+                     'xqa_key': block.xqa_key,
+                     'source_file': source_file,
+                     'source_url': '%s/%s/tree/master/%s' % (giturl, data_dir, source_file),
+                     'category': str(block.__class__.__name__),
+                     # Template uses element_id in js function names, so can't allow dashes
+                     'element_id': block.location.html_id().replace('-', '_'),
+                     'edit_link': edit_link,
+                     'user': user,
+                     'xqa_server': settings.MITX_FEATURES.get('USE_XQA_SERVER', 'http://xqa:server@content-qa.mitx.mit.edu/xqa'),
+                     'histogram': json.dumps(histogram),
+                     'render_histogram': render_histogram,
+                     'block_content': frag.content,
+                     'is_released': is_released,
+                     }
+    return wrap_fragment(frag, render_to_string("staff_problem_info.html", staff_context))

common/lib/xmodule/xmodule/conditional_module.py

@@ -148,7 +148,7 @@ class ConditionalModule(ConditionalFields, XModule):
                                                context)
             return json.dumps({'html': [html], 'message': bool(message)})
 
-        html = [child.get_html() for child in self.get_display_items()]
+        html = [self.runtime.render_child(child, None, 'student_view').content for child in self.get_display_items()]
 
         return json.dumps({'html': html})
 

common/lib/xmodule/xmodule/crowdsource_hinter.py

@@ -113,16 +113,18 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
 
         try:
             child = self.get_display_items()[0]
-            out = child.get_html()
+            out = self.runtime.render_child(child, None, 'student_view').content
             # The event listener uses the ajax url to find the child.
-            child_url = child.system.ajax_url
+            child_url = child.runtime.ajax_url
         except IndexError:
-            out = 'Error in loading crowdsourced hinter - can\'t find child problem.'
+            out = u"Error in loading crowdsourced hinter - can't find child problem."
             child_url = ''
 
         # Wrap the module in a <section>.  This lets us pass data attributes to the javascript.
-        out += '<section class="crowdsource-wrapper" data-url="' + self.system.ajax_url +\
-            '" data-child-url = "' + child_url + '"> </section>'
+        out += u'<section class="crowdsource-wrapper" data-url="{ajax_url}" data-child-url="{child_url}"> </section>'.format(
+            ajax_url=self.runtime.ajax_url,
+            child_url=child_url
+        )
 
         return out
 
@@ -172,7 +174,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
             out.update({'op': 'error'})
         else:
             out.update({'op': dispatch})
-        return json.dumps({'contents': self.system.render_template('hinter_display.html', out)})
+        return json.dumps({'contents': self.runtime.render_template('hinter_display.html', out)})
 
     def get_hint(self, data):
         """

common/lib/xmodule/xmodule/modulestore/tests/factories.py

 import datetime
 
-from factory import Factory, LazyAttributeSequence
+from factory import Factory, lazy_attribute_sequence, lazy_attribute
+from factory.containers import CyclicDefinitionError
 from uuid import uuid4
 from pytz import UTC
 
 from xmodule.modulestore import Location
-from xmodule.modulestore.django import editable_modulestore
 from xmodule.x_module import XModuleDescriptor
+from xmodule.course_module import CourseDescriptor
 
 
-class XModuleCourseFactory(Factory):
+class Dummy(object):
+    pass
+
+
+class XModuleFactory(Factory):
     """
-    Factory for XModule courses.
+    Factory for XModules
     """
 
-    ABSTRACT_FACTORY = True
+    # We have to give a Factory a FACTORY_FOR.
+    # However, the class that we create is actually determined by the category
+    # specified in the factory
+    FACTORY_FOR = Dummy
+
+    @lazy_attribute
+    def modulestore(self):
+        # Delayed import so that we only depend on django if the caller
+        # hasn't provided their own modulestore
+        from xmodule.modulestore.django import editable_modulestore
+        return editable_modulestore('direct')
+
+
+class CourseFactory(XModuleFactory):
+    """
+    Factory for XModule courses.
+    """
+    org = 'MITx'
+    number = '999'
+    display_name = 'Robot Super Course'
 
     @classmethod
     def _create(cls, target_class, **kwargs):
 
+        # All class attributes (from this class and base classes) are
+        # passed in via **kwargs. However, some of those aren't actual field values,
+        # so pop those off for use separately
         org = kwargs.pop('org', None)
         number = kwargs.pop('number', kwargs.pop('course', None))
-        display_name = kwargs.pop('display_name', None)
-        location = Location('i4x', org, number, 'course', Location.clean(display_name))
+        store = kwargs.pop('modulestore')
 
-        store = editable_modulestore('direct')
+        location = Location('i4x', org, number, 'course', Location.clean(kwargs.get('display_name')))
 
         # Write the data to the mongo datastore
         new_course = store.create_xmodule(location, metadata=kwargs.get('metadata', None))
 
-        # This metadata code was copied from cms/djangoapps/contentstore/views.py
-        if display_name is not None:
-            new_course.display_name = display_name
-
         new_course.start = datetime.datetime.now(UTC).replace(microsecond=0)
 
         # The rest of kwargs become attributes on the course:
@@ -44,33 +66,41 @@ class XModuleCourseFactory(Factory):
         return new_course
 
 
-class Course:
-    pass
-
+class ItemFactory(XModuleFactory):
+    """
+    Factory for XModule items.
+    """
 
-class CourseFactory(XModuleCourseFactory):
-    FACTORY_FOR = Course
+    category = 'chapter'
+    parent = None
 
-    org = 'MITx'
-    number = '999'
-    display_name = 'Robot Super Course'
+    @lazy_attribute_sequence
+    def display_name(self, n):
+        return "{} {}".format(self.category, n)
 
+    @lazy_attribute
+    def location(self):
+        if self.display_name is None:
+            dest_name = uuid4().hex
+        else:
+            dest_name = self.display_name.replace(" ", "_")
 
-class XModuleItemFactory(Factory):
-    """
-    Factory for XModule items.
-    """
+        return self.parent_location.replace(category=self.category, name=dest_name)
 
-    ABSTRACT_FACTORY = True
+    @lazy_attribute
+    def parent_location(self):
+        default_location = Location('i4x://MITx/999/course/Robot_Super_Course')
+        try:
+            parent = self.parent
+        # This error is raised if the caller hasn't provided either parent or parent_location
+        # In this case, we'll just return the default parent_location
+        except CyclicDefinitionError:
+            return default_location
 
-    parent_location = 'i4x://MITx/999/course/Robot_Super_Course'
-    category = 'problem'
-    display_name = LazyAttributeSequence(lambda o, n: "{} {}".format(o.category, n))
+        if parent is None:
+            return default_location
 
-    @staticmethod
-    def location(parent, category, display_name):
-        dest_name = display_name.replace(" ", "_") if display_name is not None else uuid4().hex
-        return Location(parent).replace(category=category, name=dest_name)
+        return parent.location
 
     @classmethod
     def _create(cls, target_class, **kwargs):
@@ -97,13 +127,11 @@ class XModuleItemFactory(Factory):
         DETACHED_CATEGORIES = ['about', 'static_tab', 'course_info']
         # catch any old style users before they get into trouble
         assert not 'template' in kwargs
-        parent_location = Location(kwargs.get('parent_location'))
         data = kwargs.get('data')
         category = kwargs.get('category')
         display_name = kwargs.get('display_name')
         metadata = kwargs.get('metadata', {})
-        location = kwargs.get('location', XModuleItemFactory.location(parent_location, category, display_name))
-        assert location != parent_location
+        location = kwargs.get('location')
         if kwargs.get('boilerplate') is not None:
             template_id = kwargs.get('boilerplate')
             clz = XModuleDescriptor.load_class(category)
@@ -113,10 +141,7 @@ class XModuleItemFactory(Factory):
             if not isinstance(data, basestring):
                 data.update(template.get('data'))
 
-        store = editable_modulestore('direct')
-
-        # This code was based off that in cms/djangoapps/contentstore/views.py
-        parent = store.get_item(parent_location)
+        store = kwargs.get('modulestore')
 
         # replace the display name with an optional parameter passed in from the caller
         if display_name is not None:
@@ -124,16 +149,14 @@ class XModuleItemFactory(Factory):
         store.create_and_save_xmodule(location, metadata=metadata, definition_data=data)
 
         if location.category not in DETACHED_CATEGORIES:
-            parent.children.append(location.url())
-            store.update_children(parent_location, parent.children)
 
-        return store.get_item(location)
+            parent_location = Location(kwargs.get('parent_location'))
+            assert location != parent_location
 
+            # This code was based off that in cms/djangoapps/contentstore/views.py
+            parent = kwargs.get('parent') or store.get_item(parent_location)
 
-class Item:
-    pass
-
+            parent.children.append(location.url())
+            store.update_children(parent_location, parent.children)
 
-class ItemFactory(XModuleItemFactory):
-    FACTORY_FOR = Item
-    category = 'chapter'
+        return store.get_item(location)

common/lib/xmodule/xmodule/randomize_module.py

@@ -80,9 +80,9 @@ class RandomizeModule(RandomizeFields, XModule):
     def get_html(self):
         if self.child is None:
             # raise error instead?  In fact, could complain on descriptor load...
-            return "<div>Nothing to randomize between</div>"
+            return u"<div>Nothing to randomize between</div>"
 
-        return self.child.get_html()
+        return self.runtime.render_child(self.child, None, 'student_view').content
 
     def get_icon_class(self):
         return self.child.get_icon_class() if self.child else 'other'

common/lib/xmodule/xmodule/seq_module.py

@@ -82,7 +82,7 @@ class SequenceModule(SequenceFields, XModule):
         for child in self.get_display_items():
             progress = child.get_progress()
             childinfo = {
-                'content': child.get_html(),
+                'content': self.runtime.render_child(child, None, 'student_view').content,
                 'title': "\n".join(
                     grand_child.display_name
                     for grand_child in child.get_children()

common/lib/xmodule/xmodule/tests/rendering/__init__.py

+import core
+import xmodule_asserts
\ No newline at end of file

common/lib/xmodule/xmodule/tests/rendering/core.py

+"""
+This module is indended to provide a pluggable way to add assertions about
+the rendered content of XBlocks.
+
+For each view on the XBlock, this module defines a @singledispatch function
+that can be used to test the contents of the rendered html.
+
+The functions are of the form:
+
+    @singledispatch
+    def assert_student_view_valid_html(block, html):
+        '''
+        block: The block that rendered the HTML
+        html: An lxml.html parse of the HTML for this block
+        '''
+        ...
+        assert foo
+        ...
+        for child in children:
+            assert_xblock_html(child, child_html)
+
+    @singledispatch
+    def assert_student_view_invalid_html(block, html):
+        '''
+        block: The block that rendered the HTML
+        html: A string of unparsable html
+        '''
+        ...
+        assert foo
+        ...
+        for child in children:
+            assert_xblock_html(child, child_html)
+        ...
+
+A further extension would be to provide a companion set of functions that
+resources that are provided to the Fragment
+"""
+import lxml.html
+import lxml.etree
+
+from singledispatch import singledispatch
+
+@singledispatch
+def assert_student_view_valid_html(block, html):
+    """
+    Asserts that the html generated by the `student_view` view is correct for
+    the supplied block
+
+    :param block: The :class:`XBlock` that generated the html
+    :param html: The generated html as parsed by lxml.html
+    """
+    pass
+
+
+@singledispatch
+def assert_studio_view_valid_html(block, html):
+    """
+    Asserts that the html generated by the `studio_view` view is correct for
+    the supplied block
+
+    :param block: The :class:`XBlock` that generated the html
+    :param html: The generated html as parsed by lxml.html
+    """
+    pass
+
+
+@singledispatch
+def assert_student_view_invalid_html(block, html):
+    """
+    Asserts that the html generated by the `student_view` view is correct for
+    the supplied block, given that html wasn't parsable
+
+    :param block: The :class:`XBlock` that generated the html
+    :param html: A string, not parseable as html
+    """
+    assert False, "student_view should produce valid html"
+
+
+@singledispatch
+def assert_studio_view_invalid_html(block, html):
+    """
+    Asserts that the html generated by the `studio_view` view is correct for
+    the supplied block
+
+    :param block: The :class:`XBlock` that generated the html
+    :param html: A string, not parseable as html
+    """
+    assert False, "studio_view should produce valid html"
+
+
+def assert_student_view(block, fragment):
+    """
+    Helper function to assert that the `fragment` is valid output
+    the specified `block`s `student_view`
+    """
+    try:
+        html = lxml.html.fragment_fromstring(fragment.content)
+    except lxml.etree.ParserError:
+        assert_student_view_invalid_html(block, fragment.content)
+    else:
+        assert_student_view_valid_html(block, html)
+
+
+def assert_studio_view(block, fragment):
+    """
+    Helper function to assert that the `fragment` is valid output
+    the specified `block`s `studio_view`
+    """
+    try:
+        html = lxml.html.fragment_fromstring(fragment.content)
+    except lxml.etree.ParserError:
+        assert_studio_view_invalid_html(block, fragment.content)
+    else:
+        assert_studio_view_valid_html(block, html)

common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py

+"""
+View assertion functions for XModules
+"""
+
+from __future__ import absolute_import
+
+from nose.tools import assert_equals, assert_not_equals  # pylint: disable=no-name-in-module
+
+from xmodule.timelimit_module import TimeLimitModule, TimeLimitDescriptor
+
+from xmodule.tests.rendering.core import assert_student_view_valid_html, assert_student_view_invalid_html
+
+
+@assert_student_view_valid_html.register(TimeLimitModule)
+@assert_student_view_valid_html.register(TimeLimitDescriptor)
+def _(block, html):
+    """
+    Assert that a TimeLimitModule renders student_view html correctly
+    """
+    assert_not_equals(0, block.get_display_items())
+    assert_student_view_valid_html(block.get_children()[0], html)
+
+
+@assert_student_view_invalid_html.register(TimeLimitModule)
+@assert_student_view_invalid_html.register(TimeLimitDescriptor)
+def _(block, html):
+    """
+    Assert that a TimeLimitModule renders student_view html correctly
+    """
+    assert_equals(0, len(block.get_display_items()))
+    assert_equals(u"", html)

common/lib/xmodule/xmodule/tests/test_conditional.py

@@ -8,6 +8,7 @@ from mock import Mock, patch
 
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
+from xblock.fragment import Fragment
 from xmodule.error_module import NonStaffErrorDescriptor
 from xmodule.modulestore import Location
 from xmodule.modulestore.xml import ImportSystem, XMLModuleStore
@@ -76,13 +77,16 @@ class ConditionalFactory(object):
         # construct other descriptors:
         child_descriptor = Mock()
         cond_descriptor = Mock()
+        cond_descriptor.runtime = system
         cond_descriptor.get_required_module_descriptors = lambda: [source_descriptor, ]
         cond_descriptor.get_children = lambda: [child_descriptor, ]
         cond_descriptor.xml_attributes = {"attempted": "true"}
 
         # create child module:
         child_module = Mock()
-        child_module.get_html = lambda: '<p>This is a secret</p>'
+        child_module.runtime = system
+        child_module.get_html.return_value = u'<p>This is a secret</p>'
+        child_module.student_view.return_value = Fragment(child_module.get_html.return_value)
         child_module.displayable_items = lambda: [child_module]
         module_map = {source_descriptor: source_module, child_descriptor: child_module}
         system.get_module = lambda descriptor: module_map[descriptor]

common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py

@@ -9,6 +9,7 @@ import copy
 from xmodule.crowdsource_hinter import CrowdsourceHinterModule
 from xmodule.vertical_module import VerticalModule, VerticalDescriptor
 from xblock.field_data import DictFieldData
+from xblock.fragment import Fragment
 
 from . import get_test_system
 
@@ -209,14 +210,16 @@ class FakeChild(object):
     A fake Xmodule.
     """
     def __init__(self):
-        self.system = Mock()
-        self.system.ajax_url = 'this/is/a/fake/ajax/url'
+        self.runtime = get_test_system()
+        self.runtime.ajax_url = 'this/is/a/fake/ajax/url'
+        self.student_view = Mock(return_value=Fragment(self.get_html()))
+        self.save = Mock()
 
     def get_html(self):
         """
         Return a fake html string.
         """
-        return 'This is supposed to be test html.'
+        return u'This is supposed to be test html.'
 
 
 class CrowdsourceHinterTest(unittest.TestCase):
@@ -238,7 +241,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
             """
             return [FakeChild()]
         mock_module.get_display_items = fake_get_display_items
-        out_html = mock_module.get_html()
+        out_html = mock_module.runtime.render(mock_module, None, 'student_view').content
         self.assertTrue('This is supposed to be test html.' in out_html)
         self.assertTrue('this/is/a/fake/ajax/url' in out_html)
 
@@ -255,7 +258,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
             """
             return []
         mock_module.get_display_items = fake_get_display_items
-        out_html = mock_module.get_html()
+        out_html = mock_module.runtime.render(mock_module, None, 'student_view').content
         self.assertTrue('Error in loading crowdsourced hinter' in out_html)
 
     @unittest.skip("Needs to be finished.")
@@ -266,8 +269,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         NOT WORKING RIGHT NOW
         """
         mock_module = VerticalWithModulesFactory.create()
-        out_html = mock_module.get_html()
-        print out_html
+        out_html = mock_module.runtime.render(mock_module, None, 'student_view').content
         self.assertTrue('Test numerical problem.' in out_html)
         self.assertTrue('Another test numerical problem.' in out_html)
 

common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py

@@ -131,7 +131,7 @@ class TestStudentView(TestXBlockWrapper):
     # it generates the same thing from student_view that it does from get_html
     def check_student_view_leaf_node(self, descriptor_cls):
         xmodule = self.leaf_module(descriptor_cls)
-        assert_equal(xmodule.get_html(), xmodule.student_view(None).content)
+        assert_equal(xmodule.get_html(), xmodule.runtime.render(xmodule, None, 'student_view').content)
 
 
     # Test that for all container XModule Descriptors,
@@ -152,7 +152,7 @@ class TestStudentView(TestXBlockWrapper):
     # as it does using get_html
     def check_student_view_container_node_xmodules_only(self, descriptor_cls):
         xmodule = self.container_module(descriptor_cls, 2)
-        assert_equal(xmodule.get_html(), xmodule.student_view(None).content)
+        assert_equal(xmodule.get_html(), xmodule.runtime.render(xmodule, None, 'student_view').content)
 
     # Check that when an xmodule is generated from descriptor_cls
     # with mixed xmodule and xblock children, it generates the same html from student_view
@@ -183,7 +183,7 @@ class TestStudioView(TestXBlockWrapper):
             raise SkipTest(descriptor_cls.__name__ + "is not editable in studio")
 
         descriptor = self.leaf_descriptor(descriptor_cls)
-        assert_equal(descriptor.get_html(), descriptor.studio_view(None).content)
+        assert_equal(descriptor.get_html(), descriptor.runtime.render(descriptor, None, 'studio_view').content)
 
 
     # Test that for all of the Descriptors listed in CONTAINER_XMODULES
@@ -206,7 +206,7 @@ class TestStudioView(TestXBlockWrapper):
             raise SkipTest(descriptor_cls.__name__ + "is not editable in studio")
 
         descriptor = self.container_descriptor(descriptor_cls)
-        assert_equal(descriptor.get_html(), descriptor.studio_view(None).content)
+        assert_equal(descriptor.get_html(), descriptor.runtime.render(descriptor, None, 'studio_view').content)
 
     # Check that when a descriptor is generated from descriptor_cls
     # with mixed xmodule and xblock children, it generates the same html from studio_view

common/lib/xmodule/xmodule/timelimit_module.py

@@ -15,6 +15,8 @@ log = logging.getLogger(__name__)
 
 
 class TimeLimitFields(object):
+    has_children = True
+
     beginning_at = Float(help="The time this timer was started", scope=Scope.user_state)
     ending_at = Float(help="The time this timer will end", scope=Scope.user_state)
     accomodation_code = String(help="A code indicating accommodations to be given the student", scope=Scope.user_state)
@@ -31,8 +33,6 @@ class TimeLimitModule(TimeLimitFields, XModule):
     def __init__(self, *args, **kwargs):
         XModule.__init__(self, *args, **kwargs)
 
-        self.rendered = False
-
     # For a timed activity, we are only interested here
     # in time-related accommodations, and these should be disjoint.
     # (For proctored exams, it is possible to have multiple accommodations
@@ -85,8 +85,13 @@ class TimeLimitModule(TimeLimitFields, XModule):
         return int((self.ending_at - time()) * 1000)
 
     def get_html(self):
-        self.render()
-        return self.content
+        # assumes there is one and only one child, so it only renders the first child
+        children = self.get_display_items()
+        if children:
+            child = children[0]
+            return self.runtime.render_child(child, None, 'student_view').content
+        else:
+            return u""
 
     def get_progress(self):
         ''' Return the total progress, adding total done and total available.
@@ -101,16 +106,6 @@ class TimeLimitModule(TimeLimitFields, XModule):
     def handle_ajax(self, _dispatch, _data):
         raise NotFoundError('Unexpected dispatch type')
 
-    def render(self):
-        if self.rendered:
-            return
-        # assumes there is one and only one child, so it only renders the first child
-        children = self.get_display_items()
-        if children:
-            child = children[0]
-            self.content = child.get_html()
-        self.rendered = True
-
     def get_icon_class(self):
         children = self.get_children()
         if children:

common/lib/xmodule/xmodule/vertical_module.py

@@ -23,7 +23,7 @@ class VerticalModule(VerticalFields, XModule):
         if self.contents is None:
             self.contents = [{
                 'id': child.id,
-                'content': child.get_html()
+                'content': self.runtime.render_child(child, None, 'student_view').content
             } for child in self.get_display_items()]
 
         return self.system.render_template('vert_module.html', {

common/lib/xmodule/xmodule/x_module.py

@@ -657,7 +657,38 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         return Fragment(self.get_html())
 
 
-class DescriptorSystem(Runtime):
+class ConfigurableFragmentWrapper(object):  # pylint: disable=abstract-method
+    """
+    Runtime mixin that allows for composition of many `wrap_child` wrappers
+    """
+    def __init__(self, wrappers=None, **kwargs):
+        """
+        :param wrappers: A list of wrappers, where each wrapper is:
+
+            def wrapper(block, view, frag, context):
+                ...
+                return wrapped_frag
+        """
+        super(ConfigurableFragmentWrapper, self).__init__(**kwargs)
+        if wrappers is not None:
+            self.wrappers = wrappers
+        else:
+            self.wrappers = []
+
+    def wrap_child(self, block, view, frag, context):
+        """
+        See :func:`Runtime.wrap_child`
+        """
+        for wrapper in self.wrappers:
+            frag = wrapper(block, view, frag, context)
+
+        return frag
+
+
+class DescriptorSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
+    """
+    Base class for :class:`Runtime`s to be used with :class:`XModuleDescriptor`s
+    """
 
     def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
         """
@@ -750,7 +781,7 @@ class XMLParsingSystem(DescriptorSystem):
         self.policy = policy
 
 
-class ModuleSystem(Runtime):
+class ModuleSystem(ConfigurableFragmentWrapper, Runtime):  # pylint: disable=abstract-method
     """
     This is an abstraction such that x_modules can function independent
     of the courseware (e.g. import into other types of courseware, LMS,

lms/djangoapps/courseware/module_render.py

@@ -25,7 +25,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.x_module import ModuleSystem
-from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_histogram, wrap_xmodule, save_module  # pylint: disable=F0401
+from xmodule_modifiers import replace_course_urls, replace_jump_to_id_urls, replace_static_urls, add_histogram, wrap_xmodule
 
 import static_replace
 from psychometrics.psychoanalyze import make_psychometrics_data_update_handler
@@ -334,10 +334,46 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
 
         dog_stats_api.increment("lms.courseware.question_answered", tags=tags)
 
+    # Build a list of wrapping functions that will be applied in order
+    # to the Fragment content coming out of the xblocks that are about to be rendered.
+    block_wrappers = []
+
+    # Wrap the output display in a single div to allow for the XModule
+    # javascript to be bound correctly
+    if wrap_xmodule_display is True:
+        block_wrappers.append(partial(wrap_xmodule, 'xmodule_display.html'))
+
     # TODO (cpennington): When modules are shared between courses, the static
     # prefix is going to have to be specific to the module, not the directory
     # that the xml was loaded from
 
+    # Rewrite urls beginning in /static to point to course-specific content
+    block_wrappers.append(partial(
+        replace_static_urls,
+        getattr(descriptor, 'data_dir', None),
+        course_id=course_id,
+        static_asset_path=static_asset_path or descriptor.static_asset_path
+    ))
+
+    # Allow URLs of the form '/course/' refer to the root of multicourse directory
+    #   hierarchy of this course
+    block_wrappers.append(partial(replace_course_urls, course_id))
+
+    # this will rewrite intra-courseware links (/jump_to_id/<id>). This format
+    # is an improvement over the /course/... format for studio authored courses,
+    # because it is agnostic to course-hierarchy.
+    # NOTE: module_id is empty string here. The 'module_id' will get assigned in the replacement
+    # function, we just need to specify something to get the reverse() to work.
+    block_wrappers.append(partial(
+        replace_jump_to_id_urls,
+        course_id,
+        reverse('jump_to_id', kwargs={'course_id': course_id, 'module_id': ''}),
+    ))
+
+    if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'):
+        if has_access(user, descriptor, 'staff', course_id):
+            block_wrappers.append(partial(add_histogram, user))
+
     system = ModuleSystem(
         track_function=track_function,
         render_template=render_to_string,
@@ -377,7 +413,8 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
         cache=cache,
         can_execute_unsafe_code=(lambda: can_execute_unsafe_code(course_id)),
         # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington)
-        mixins=descriptor.system.mixologist._mixins,
+        mixins=descriptor.runtime.mixologist._mixins,  # pylint: disable=protected-access
+        wrappers=block_wrappers,
     )
 
     # pass position specified in URL to module through ModuleSystem
@@ -408,41 +445,6 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours
         return err_descriptor.xmodule(system)
 
     system.set('user_is_staff', has_access(user, descriptor.location, 'staff', course_id))
-    _get_html = module.get_html
-
-    if wrap_xmodule_display is True:
-        _get_html = wrap_xmodule(module.get_html, module, 'xmodule_display.html')
-
-    module.get_html = replace_static_urls(
-        _get_html,
-        getattr(descriptor, 'data_dir', None),
-        course_id=course_id,
-        static_asset_path=static_asset_path or descriptor.static_asset_path
-    )
-
-    # Allow URLs of the form '/course/' refer to the root of multicourse directory
-    #   hierarchy of this course
-    module.get_html = replace_course_urls(module.get_html, course_id)
-
-    # this will rewrite intra-courseware links
-    # that use the shorthand /jump_to_id/<id>. This is very helpful
-    # for studio authored courses (compared to the /course/... format) since it is
-    # is durable with respect to moves and the author doesn't need to
-    # know the hierarchy
-    # NOTE: module_id is empty string here. The 'module_id' will get assigned in the replacement
-    # function, we just need to specify something to get the reverse() to work
-    module.get_html = replace_jump_to_id_urls(
-        module.get_html,
-        course_id,
-        reverse('jump_to_id', kwargs={'course_id': course_id, 'module_id': ''})
-    )
-
-    if settings.MITX_FEATURES.get('DISPLAY_HISTOGRAMS_TO_STAFF'):
-        if has_access(user, module, 'staff', course_id):
-            module.get_html = add_histogram(module.get_html, module, user)
-
-    # force the module to save after rendering
-    module.get_html = save_module(module.get_html, module)
     return module
 
 

lms/djangoapps/courseware/tests/__init__.py

@@ -11,6 +11,7 @@ from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
 from django.test.client import Client
 
+from mitxmako.shortcuts import render_to_string
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from xblock.field_data import DictFieldData
@@ -73,7 +74,7 @@ class BaseTestXmodule(ModuleStoreTestCase):
 
         # username = robot{0}, password = 'test'
         self.users = [
-            UserFactory.create(username='robot%d' % i, email='robot+test+%d@edx.org' % i)
+            UserFactory.create()
             for i in range(self.USER_COUNT)
         ]
 
@@ -93,6 +94,8 @@ class BaseTestXmodule(ModuleStoreTestCase):
 
         self.runtime.xmodule_field_data = self.xmodule_field_data
 
+        self.runtime.get_module = lambda descr: descr.xmodule(self.runtime)
+
         self.item_module = self.item_descriptor.xmodule(self.runtime)
 
         self.item_url = Location(self.item_module.location).url()
@@ -114,6 +117,9 @@ class BaseTestXmodule(ModuleStoreTestCase):
             args=(self.course.id, self.item_url, dispatch)
         )
 
-    def tearDown(self):
-        for user in self.users:
-            user.delete()
+
+class XModuleRenderingTestBase(BaseTestXmodule):
+    def setUp(self):
+        super(XModuleRenderingTestBase, self).setUp()
+
+        self.runtime.render_template = render_to_string

lms/djangoapps/courseware/tests/test_module_render.py

@@ -74,7 +74,7 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase):
         )
 
         # get the rendered HTML output which should have the rewritten link
-        html = module.get_html()
+        html = module.system.render(module, None, 'student_view').content
 
         # See if the url got rewritten to the target link
         # note if the URL mapping changes then this assertion will break

lms/djangoapps/courseware/tests/test_timelimit_module.py

+"""
+Tests of the TimeLimitModule
+
+TODO: This should be a test in common/lib/xmodule. However,
+actually rendering HTML templates for XModules at this point requires
+Django (which is storing the templates), so the test can't run in isolation
+"""
+from xmodule.modulestore.tests.factories import ItemFactory
+from xmodule.tests.rendering.core import assert_student_view
+
+from . import XModuleRenderingTestBase
+
+
+class TestTimeLimitModuleRendering(XModuleRenderingTestBase):
+    """
+    Tests of TimeLimitModule html rendering
+    """
+    def test_with_children(self):
+        block = ItemFactory.create(category='timelimit')
+        ItemFactory.create(category='html', data='<html>This is just text</html>', parent=block)
+
+        assert_student_view(block, self.runtime.render(block.xmodule(self.runtime), None, 'student_view'))
+
+    def test_without_children(self):
+        block = ItemFactory.create(category='timelimit')
+        assert_student_view(block, self.runtime.render(block.xmodule(self.runtime), None, 'student_view'))

lms/djangoapps/instructor/views/legacy.py

@@ -814,7 +814,9 @@ def instructor_dashboard(request, course_id):
     # HTML editor for email
     if idash_mode == 'Email' and is_studio_course:
         html_module = HtmlDescriptor(course.system, DictFieldData({'data': html_message}), ScopeIds(None, None, None, None))
-        email_editor = wrap_xmodule(html_module.get_html, html_module, 'xmodule_edit.html')()
+        fragment = course.system.render(html_module, None, 'studio_view')
+        fragment = wrap_xmodule('xmodule_edit.html', html_module, 'studio_view', fragment, None)
+        email_editor = fragment.content
 
     studio_url = None
     if is_studio_course:

lms/templates/staff_problem_info.html

 <%! from django.utils.translation import ugettext as _ %>
 
 ## The JS for this is defined in xqa_interface.html
-${module_content}
+${block_content}
 %if location.category in ['problem','video','html','combinedopenended','graphical_slider_tool']:
 %  if edit_link:
 <div>

requirements/edx/base.txt

@@ -58,6 +58,7 @@ PyYAML==3.10
 requests==1.2.3
 scipy==0.11.0
 Shapely==1.2.16
+singledispatch==3.4.0.2
 sorl-thumbnail==11.12
 South==0.7.6
 sympy==0.7.1