diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 918d19b473..5c7fff16f7 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/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 diff --git a/common/djangoapps/tests.py b/common/djangoapps/tests.py deleted file mode 100644 index 17e9c07f6a..0000000000 --- a/common/djangoapps/tests.py +++ /dev/null @@ -1,49 +0,0 @@ -''' -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'.*Not yet.*') - - 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'.*Yes!.*') diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 14f80c0a70..7683768e12 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -1,19 +1,34 @@ +""" +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_xmodule(get_html, module, template, context=None): +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(template, block, view, frag, context): # pylint: disable=unused-argument """ Wraps the results of get_html in a standard
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/ 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////jump_to_id. NOTE the 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//... """ - @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//... """ - - @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(): + # TODO: make this more general, eg use an XModule attribute instead + if isinstance(block, (SequenceModule, VerticalModule)): + return frag - if type(module) in [SequenceModule, VerticalModule]: # TODO: make this more general, eg use an XModule attribute instead - return get_html() + 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 - 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(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 = "" - 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 = block.source_file # source used to generate the problem XML, eg latex or word - 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 = block.descriptor.start - # 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 = "Yes!" if (now > mstart) else "Not yet" - if mstart is not None: - is_released = "Yes!" if (now > mstart) else "Not yet" - - 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 + 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)) diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index 82416d48d1..a489b8a19f 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/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}) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index fcc74d5cb9..b302982b05 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/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
. This lets us pass data attributes to the javascript. - out += '
' + out += u'
'.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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index c8228c5e3e..5b7f9ec3d7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -1,38 +1,60 @@ 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 XModules + """ + + # 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. """ - - ABSTRACT_FACTORY = True + 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 CourseFactory(XModuleCourseFactory): - FACTORY_FOR = Course - - org = 'MITx' - number = '999' - display_name = 'Robot Super Course' - - -class XModuleItemFactory(Factory): +class ItemFactory(XModuleFactory): """ Factory for XModule items. """ - ABSTRACT_FACTORY = True + category = 'chapter' + parent = None - parent_location = 'i4x://MITx/999/course/Robot_Super_Course' - category = 'problem' - display_name = LazyAttributeSequence(lambda o, n: "{} {}".format(o.category, n)) + @lazy_attribute_sequence + def display_name(self, n): + return "{} {}".format(self.category, n) - @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) + @lazy_attribute + def location(self): + if self.display_name is None: + dest_name = uuid4().hex + else: + dest_name = self.display_name.replace(" ", "_") + + return self.parent_location.replace(category=self.category, name=dest_name) + + @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 + + if parent is None: + return default_location + + 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_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) + parent.children.append(location.url()) store.update_children(parent_location, parent.children) return store.get_item(location) - - -class Item: - pass - - -class ItemFactory(XModuleItemFactory): - FACTORY_FOR = Item - category = 'chapter' diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py index 38535d38bb..0cc853fd90 100644 --- a/common/lib/xmodule/xmodule/randomize_module.py +++ b/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 "
Nothing to randomize between
" + return u"
Nothing to randomize between
" - 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' diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 867eae19ce..7890edd586 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/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() diff --git a/common/lib/xmodule/xmodule/tests/rendering/__init__.py b/common/lib/xmodule/xmodule/tests/rendering/__init__.py new file mode 100644 index 0000000000..9a3a52262e --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/rendering/__init__.py @@ -0,0 +1,2 @@ +import core +import xmodule_asserts \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/tests/rendering/core.py b/common/lib/xmodule/xmodule/tests/rendering/core.py new file mode 100644 index 0000000000..1324af853c --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/rendering/core.py @@ -0,0 +1,114 @@ +""" +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) diff --git a/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py b/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py new file mode 100644 index 0000000000..fa4dd66b06 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/rendering/xmodule_asserts.py @@ -0,0 +1,31 @@ +""" +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) diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index 040af7a230..b665ef02f7 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/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: '

This is a secret

' + child_module.runtime = system + child_module.get_html.return_value = u'

This is a secret

' + 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] diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 0369fffe8e..8ddb074d11 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py b/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py index ef81a9a26b..948b4f6e18 100644 --- a/common/lib/xmodule/xmodule/tests/test_xblock_wrappers.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/timelimit_module.py b/common/lib/xmodule/xmodule/timelimit_module.py index a5de35cfc2..70d3d9dba9 100644 --- a/common/lib/xmodule/xmodule/timelimit_module.py +++ b/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: diff --git a/common/lib/xmodule/xmodule/vertical_module.py b/common/lib/xmodule/xmodule/vertical_module.py index 610d180c11..2d6bfaa633 100644 --- a/common/lib/xmodule/xmodule/vertical_module.py +++ b/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', { diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index ef37843371..6b3e3b67c2 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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, diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index f86bef29ad..3c1fe4e0e7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/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/). 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/. 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 diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index 6988094221..afd5fae2cc 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 26f27bd90a..f287c9acc4 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_timelimit_module.py b/lms/djangoapps/courseware/tests/test_timelimit_module.py new file mode 100644 index 0000000000..41b2a00304 --- /dev/null +++ b/lms/djangoapps/courseware/tests/test_timelimit_module.py @@ -0,0 +1,26 @@ +""" +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='This is just text', 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')) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index fdf77e5909..61b43fa9cc 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/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: diff --git a/lms/templates/staff_problem_info.html b/lms/templates/staff_problem_info.html index 97730bca41..7f87674047 100644 --- a/lms/templates/staff_problem_info.html +++ b/lms/templates/staff_problem_info.html @@ -1,7 +1,7 @@ <%! 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:
diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 278c1f8aa7..72d763339d 100644 --- a/requirements/edx/base.txt +++ b/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