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

@@ -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,
+
+            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()
+                                '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:

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.
 
-class SimpleInput():#  XModule
-    '''
-    Type for simple inputs -- plain HTML with a form element
-    '''
+    If an already-registered input type has claimed one of those tags, will raise ValueError.
 
-    # Maps tags to functions
-    xml_tags = {}
+    If there are no tags in cls.tags, will also raise ValueError.
+    """
 
-    def __init__(self, system, xml, item_id=None, track_url=None, state=None, use='capa_input'):
-        '''
-        Instantiate a SimpleInput class.  Arguments:
+    # 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()
+
+
+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]
+
+
+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:
+        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
+        # put hint above msg if it should be displayed
         if self.hintmode == 'always':
-                # TODO: is the '.' in <br/.> below a bug?
-                self.msg = self.hint + ('<br/.>' if self.msg else '') + self.msg
+            self.msg = self.hint + ('<br/>' 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']
+        (Separate from get_html to faciliate testing of logic separately from the rendering)
+        """
+        raise NotImplementedError
 
     def get_html(self):
-        return self.xml_tags[self.tag](self.xml, self.value,
-                                       self.status, self.system.render_template, self.msg)
+        """
+        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)
 
-def register_render_function(fn, names=None, cls=SimpleInput):
-    if names is None:
-        SimpleInput.xml_tags[fn.__name__] = fn
-    else:
-        raise NotImplementedError
 
-    def wrapped(*args, **kwargs):
-        return fn(*args, **kwargs)
-    return wrapped
+## 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:
+
+    <optioninput options="('Up','Down')" correct="Up"/><text>The location of the sky</text>
+    """
+
+    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 <codeinput>
-@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 <span>...</span> 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)

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 = """<optioninput options="('Up','Down')" id="sky_input" correct="Up"/>"""
         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 = """<optioninput options="('Up','Down')" id="sky_input" correct="Up"/>"""
+        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)