From 2b6e935985c650039dd0ae4394031c8f0b950eff Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 11 Oct 2012 11:26:49 -0400 Subject: [PATCH] Initial refactor of inputtypes into classes. - for now, wraps existing render functions as separate classes - a bit of cleanup in how it's called from capa_problem - initial tests to make sure things are testable. --- common/lib/capa/capa/capa_problem.py | 29 +- common/lib/capa/capa/inputtypes.py | 252 ++++++++++++------ common/lib/capa/capa/tests/test_inputtypes.py | 53 +++- 3 files changed, 222 insertions(+), 112 deletions(-) diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index 29e9b7eb97..252b536927 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -481,8 +481,8 @@ class LoncapaProblem(object): problemid = problemtree.get('id') # my ID - if problemtree.tag in inputtypes.get_input_xml_tags(): - + if problemtree.tag in inputtypes.registered_input_tags(): + # If this is an inputtype subtree, let it render itself. status = "unsubmitted" msg = '' hint = '' @@ -499,20 +499,17 @@ class LoncapaProblem(object): value = self.student_answers[problemid] # do the rendering - render_object = inputtypes.SimpleInput(system=self.system, - xml=problemtree, - state={'value': value, - 'status': status, - 'id': problemtree.get('id'), - 'feedback': {'message': msg, - 'hint': hint, - 'hintmode': hintmode, - } - }, - use='capa_input') - # function(problemtree, value, status, msg) - # render the special response (textline, schematic,...) - return render_object.get_html() + + state = {'value': value, + 'status': status, + 'id': problemtree.get('id'), + 'feedback': {'message': msg, + 'hint': hint, + 'hintmode': hintmode,}} + + input_type_cls = inputtypes.get_class_for_tag(problemtree.tag) + the_input = input_type_cls(self.system, problemtree, state) + return the_input.get_html() # let each Response render itself if problemtree in self.responders: diff --git a/common/lib/capa/capa/inputtypes.py b/common/lib/capa/capa/inputtypes.py index 2858b2171f..49cc91f343 100644 --- a/common/lib/capa/capa/inputtypes.py +++ b/common/lib/capa/capa/inputtypes.py @@ -37,102 +37,174 @@ import xml.sax.saxutils as saxutils log = logging.getLogger('mitx.' + __name__) +######################################################################### -def get_input_xml_tags(): - ''' Eventually, this will be for all registered input types ''' - return SimpleInput.get_xml_tags() +_TAGS_TO_CLASSES = {} + +def register_input_class(cls): + """ + Register cls as a supported input type. It is expected to have the same constructor as + InputTypeBase, and to define cls.tags as a list of tags that it implements. + + If an already-registered input type has claimed one of those tags, will raise ValueError. + + If there are no tags in cls.tags, will also raise ValueError. + """ + + # Do all checks and complain before changing any state. + if len(cls.tags) == 0: + raise ValueError("No supported tags for class {0}".format(cls.__name__)) + + for t in cls.tags: + if t in _TAGS_TO_CLASSES: + other_cls = _TAGS_TO_CLASSES[t] + if cls == other_cls: + # registering the same class multiple times seems silly, but ok + continue + raise ValueError("Tag {0} already registered by class {1}. Can't register for class {2}" + .format(t, other_cls.__name__, cls.__name__)) + + # Ok, should be good to change state now. + for t in cls.tags: + _TAGS_TO_CLASSES[t] = cls + +def registered_input_tags(): + """ + Get a list of all the xml tags that map to known input types. + """ + return _TAGS_TO_CLASSES.keys() -class SimpleInput():# XModule - ''' - Type for simple inputs -- plain HTML with a form element - ''' +def get_class_for_tag(tag): + """ + For any tag in registered_input_tags(), return the corresponding class. Otherwise, will raise KeyError. + """ + return _TAGS_TO_CLASSES[tag] - # Maps tags to functions - xml_tags = {} - def __init__(self, system, xml, item_id=None, track_url=None, state=None, use='capa_input'): - ''' - Instantiate a SimpleInput class. Arguments: +class InputTypeBase(object): + """ + Abstract base class for input types. + """ - - system : ModuleSystem instance which provides OS, rendering, and user context + template = None + + def __init__(self, system, xml, state): + """ + Instantiate an InputType class. Arguments: + + - system : ModuleSystem instance which provides OS, rendering, and user context. Specifically, must + have a render_template function. - xml : Element tree of this Input element - - item_id : id for this input element (assigned by capa_problem.LoncapProblem) - string - - track_url : URL used for tracking - string - state : a dictionary with optional keys: - * Value - * ID - * Status (answered, unanswered, unsubmitted) - * Feedback (dictionary containing keys for hints, errors, or other - feedback from previous attempt) - - use : - ''' + * 'value' + * 'id' + * 'status' (answered, unanswered, unsubmitted) + * 'feedback' (dictionary containing keys for hints, errors, or other + feedback from previous attempt. Specifically 'message', 'hint', 'hintmode'. If 'hintmode' + is 'always', the hint is always displayed.) + """ self.xml = xml self.tag = xml.tag self.system = system - if not state: - state = {} - ## NOTE: ID should only come from one place. - ## If it comes from multiple, we use state first, XML second, and parameter - ## third. Since we don't make this guarantee, we can swap this around in - ## the future if there's a more logical order. - if item_id: - self.id = item_id + ## NOTE: ID should only come from one place. If it comes from multiple, + ## we use state first, XML second (in case the xml changed, but we have + ## existing state with an old id). Since we don't make this guarantee, + ## we can swap this around in the future if there's a more logical + ## order. - if xml.get('id'): - self.id = xml.get('id') - - if 'id' in state: - self.id = state['id'] + self.id = state.get('id', xml.get('id')) + if self.id is None: + raise ValueError("input id state is None. xml is {0}".format(etree.tostring(xml))) self.value = state.get('value', '') - self.msg = '' - feedback = state.get('feedback') - if feedback is not None: - self.msg = feedback.get('message', '') - self.hint = feedback.get('hint', '') - self.hintmode = feedback.get('hintmode', None) + feedback = state.get('feedback', {}) + self.msg = feedback.get('message', '') + self.hint = feedback.get('hint', '') + self.hintmode = feedback.get('hintmode', None) - # put hint above msg if to be displayed - if self.hintmode == 'always': - # TODO: is the '.' in
below a bug? - self.msg = self.hint + ('
' if self.msg else '') + self.msg + # put hint above msg if it should be displayed + if self.hintmode == 'always': + self.msg = self.hint + ('
' if self.msg else '') + self.msg - self.status = 'unanswered' - if 'status' in state: - self.status = state['status'] + self.status = state.get('status', 'unanswered') - @classmethod - def get_xml_tags(c): - return c.xml_tags.keys() + def _get_render_context(self): + """ + Abstract method. Subclasses should implement to return the dictionary + of keys needed to render their template. - @classmethod - def get_uses(c): - return ['capa_input', 'capa_transform'] - - def get_html(self): - return self.xml_tags[self.tag](self.xml, self.value, - self.status, self.system.render_template, self.msg) - - -def register_render_function(fn, names=None, cls=SimpleInput): - if names is None: - SimpleInput.xml_tags[fn.__name__] = fn - else: + (Separate from get_html to faciliate testing of logic separately from the rendering) + """ raise NotImplementedError - def wrapped(*args, **kwargs): - return fn(*args, **kwargs) - return wrapped + def get_html(self): + """ + Return a the html for this input, as an etree element. + """ + if self.template is None: + raise NotImplementedError("no rendering template specified for class {0}".format(self.__class__)) + + html = self.system.render_template(self.template, self._get_render_context()) + return etree.XML(html) + + +## TODO: Remove once refactor is complete +def make_class_for_render_function(fn): + """ + Take an old-style render function, return a new-style input class. + """ + + class Impl(InputTypeBase): + """ + Inherit all the constructor logic from InputTypeBase... + """ + tags = [fn.__name__] + def get_html(self): + """...delegate to the render function to do the work""" + return fn(self.xml, self.value, self.status, self.system.render_template, self.msg) + + # don't want all the classes to be called Impl (confuses register_input_class). + Impl.__name__ = fn.__name__.capitalize() + return Impl + + +def _reg(fn): + """ + Register an old-style inputtype render function as a new-style subclass of InputTypeBase. + This will go away once converting all input types to the new format is complete. (TODO) + """ + register_input_class(make_class_for_render_function(fn)) + #----------------------------------------------------------------------------- -@register_render_function +class OptionInput(InputTypeBase): + """ + Input type for selecting and Select option input type. + + Example: + + The location of the sky + """ + + template = "optioninput.html" + tags = ['optioninput'] + + def _get_render_context(self): + return _optioninput(self.xml, self.value, self.status, self.system.render_template, self.msg) + + def optioninput(element, value, status, render_template, msg=''): + context = _optioninput(element, value, status, render_template, msg) + html = render_template("optioninput.html", context) + return etree.XML(html) + +def _optioninput(element, value, status, render_template, msg=''): """ Select option input type. @@ -164,16 +236,16 @@ def optioninput(element, value, status, render_template, msg=''): 'options': osetdict, 'inline': element.get('inline',''), } + return context - html = render_template("optioninput.html", context) - return etree.XML(html) +register_input_class(OptionInput) #----------------------------------------------------------------------------- # TODO: consolidate choicegroup, radiogroup, checkboxgroup after discussion of # desired semantics. -@register_render_function +# @register_render_function def choicegroup(element, value, status, render_template, msg=''): ''' Radio button inputs: multiple choice or true/false @@ -210,6 +282,7 @@ def choicegroup(element, value, status, render_template, msg=''): html = render_template("choicegroup.html", context) return etree.XML(html) +_reg(choicegroup) #----------------------------------------------------------------------------- def extract_choices(element): @@ -237,7 +310,6 @@ def extract_choices(element): # TODO: consolidate choicegroup, radiogroup, checkboxgroup after discussion of # desired semantics. -@register_render_function def radiogroup(element, value, status, render_template, msg=''): ''' Radio button inputs: (multiple choice) @@ -258,9 +330,10 @@ def radiogroup(element, value, status, render_template, msg=''): return etree.XML(html) +_reg(radiogroup) + # TODO: consolidate choicegroup, radiogroup, checkboxgroup after discussion of # desired semantics. -@register_render_function def checkboxgroup(element, value, status, render_template, msg=''): ''' Checkbox inputs: (select one or more choices) @@ -280,7 +353,8 @@ def checkboxgroup(element, value, status, render_template, msg=''): html = render_template("choicegroup.html", context) return etree.XML(html) -@register_render_function +_reg(checkboxgroup) + def javascriptinput(element, value, status, render_template, msg='null'): ''' Hidden field for javascript to communicate via; also loads the required @@ -311,16 +385,16 @@ def javascriptinput(element, value, status, render_template, msg='null'): html = render_template("javascriptinput.html", context) return etree.XML(html) +_reg(javascriptinput) + -@register_render_function def textline(element, value, status, render_template, msg=""): ''' Simple text line input, with optional size specification. ''' # TODO: 'dojs' flag is temporary, for backwards compatibility with 8.02x if element.get('math') or element.get('dojs'): - return SimpleInput.xml_tags['textline_dynamath'](element, value, status, - render_template, msg) + return textline_dynamath(element, value, status, render_template, msg) eid = element.get('id') if eid is None: msg = 'textline has no id: it probably appears outside of a known response type' @@ -356,10 +430,11 @@ def textline(element, value, status, render_template, msg=""): raise return xhtml +_reg(textline) + #----------------------------------------------------------------------------- -@register_render_function def textline_dynamath(element, value, status, render_template, msg=''): ''' Text line input with dynamic math display (equation rendered on client in real time @@ -399,9 +474,10 @@ def textline_dynamath(element, value, status, render_template, msg=''): html = render_template("textinput_dynamath.html", context) return etree.XML(html) +_reg(textline_dynamath) + #----------------------------------------------------------------------------- -@register_render_function def filesubmission(element, value, status, render_template, msg=''): ''' Upload a single file (e.g. for programming assignments) @@ -431,10 +507,11 @@ def filesubmission(element, value, status, render_template, msg=''): html = render_template("filesubmission.html", context) return etree.XML(html) +_reg(filesubmission) + #----------------------------------------------------------------------------- ## TODO: Make a wrapper for -@register_render_function def textbox(element, value, status, render_template, msg=''): ''' The textbox is used for code input. The message is the return HTML string from @@ -493,8 +570,9 @@ def textbox(element, value, status, render_template, msg=''): return xhtml +_reg(textbox) + #----------------------------------------------------------------------------- -@register_render_function def schematic(element, value, status, render_template, msg=''): eid = element.get('id') height = element.get('height') @@ -517,10 +595,10 @@ def schematic(element, value, status, render_template, msg=''): html = render_template("schematicinput.html", context) return etree.XML(html) +_reg(schematic) #----------------------------------------------------------------------------- ### TODO: Move out of inputtypes -@register_render_function def math(element, value, status, render_template, msg=''): ''' This is not really an input type. It is a convention from Lon-CAPA, used for @@ -565,16 +643,17 @@ def math(element, value, status, render_template, msg=''): # xhtml.tail = element.tail # don't forget to include the tail! return xhtml +_reg(math) + #----------------------------------------------------------------------------- -@register_render_function def solution(element, value, status, render_template, msg=''): ''' This is not really an input type. It is just a ... which is given an ID, that is used for displaying an extended answer (a problem "solution") after "show answers" is pressed. Note that the solution content is NOT sent with the HTML. It is obtained - by a JSON call. + by an ajax call. ''' eid = element.get('id') size = element.get('size') @@ -587,10 +666,11 @@ def solution(element, value, status, render_template, msg=''): html = render_template("solutionspan.html", context) return etree.XML(html) +_reg(solution) + #----------------------------------------------------------------------------- -@register_render_function def imageinput(element, value, status, render_template, msg=''): ''' Clickable image as an input field. Element should specify the image source, height, @@ -626,3 +706,5 @@ def imageinput(element, value, status, render_template, msg=''): } html = render_template("imageinput.html", context) return etree.XML(html) + +_reg(imageinput) diff --git a/common/lib/capa/capa/tests/test_inputtypes.py b/common/lib/capa/capa/tests/test_inputtypes.py index 8451f963d5..9ef642d468 100644 --- a/common/lib/capa/capa/tests/test_inputtypes.py +++ b/common/lib/capa/capa/tests/test_inputtypes.py @@ -2,9 +2,9 @@ Tests of input types (and actually responsetypes too) """ - from datetime import datetime import json +from mock import Mock from nose.plugins.skip import SkipTest import os import unittest @@ -14,24 +14,55 @@ from capa import inputtypes from lxml import etree +def tst_render_template(template, context): + """ + A test version of render to template. Renders to the repr of the context, completely ignoring the template name. + """ + return repr(context) + + +system = Mock(render_template=tst_render_template) + class OptionInputTest(unittest.TestCase): ''' Make sure option inputs work ''' - def test_rendering(self): + def test_rendering_new(self): xml = """""" element = etree.fromstring(xml) value = 'Down' - status = 'incorrect' - rendered_element = inputtypes.optioninput(element, value, status, test_system.render_template) - rendered_str = etree.tostring(rendered_element) - print rendered_str - self.assertTrue(False) + status = 'answered' + context = inputtypes._optioninput(element, value, status, test_system.render_template) + print 'context: ', context + + expected = {'value': 'Down', + 'options': [('Up', 'Up'), ('Down', 'Down')], + 'state': 'answered', + 'msg': '', + 'inline': '', + 'id': 'sky_input'} + + self.assertEqual(context, expected) + def test_rendering(self): + xml_str = """""" + element = etree.fromstring(xml_str) + + state = {'value': 'Down', + 'id': 'sky_input', + 'status': 'answered'} + option_input = inputtypes.OptionInput(system, element, state) + + context = option_input._get_render_context() + + expected = {'value': 'Down', + 'options': [('Up', 'Up'), ('Down', 'Down')], + 'state': 'answered', + 'msg': '', + 'inline': '', + 'id': 'sky_input'} + + self.assertEqual(context, expected) - # TODO: split each inputtype into a get_render_context function and a - # template property, and have the rendering done in one place. (and be - # able to test the logic without dealing with xml at least on the output - # end)