Address code review feedback:

- improve docstrings
- only pass in the state for a particular input and
not the whole dictionary
- refactor some common code
- minor syntax cleanup

common/lib/capa/capa/capa_problem.py

@@ -91,8 +91,12 @@ class LoncapaProblem(object):
 
          - problem_text (string): xml defining the problem
          - id           (string): identifier for this problem; often a filename (no spaces)
-         - state        (dict): student state
-         - seed         (int): random number generator seed (int)
+         - state        (dict): containing the following keys:
+                                - 'seed' - (int) random number generator seed
+                                - 'student_answers' - (dict) maps input id to the stored answer for that input
+                                - 'correct_map' (CorrectMap) a map of each input to their 'correctness'
+                                - 'done' - (bool) indicates whether or not this problem is considered done
+                                - 'input_state' - (dict) maps input_id to a dictionary that holds the state for that input
          - system       (ModuleSystem): ModuleSystem instance which provides OS,
                                         rendering, and user context
 
@@ -104,27 +108,16 @@ class LoncapaProblem(object):
         self.system = system
         if self.system is None:
             raise Exception()
-        self.seed = seed
-        self.input_state = None
-
-        if state:
-            if 'seed' in state:
-                self.seed = state['seed']
-            if 'student_answers' in state:
-                self.student_answers = state['student_answers']
-            if 'correct_map' in state:
-                self.correct_map.set_dict(state['correct_map'])
-            if 'done' in state:
-                self.done = state['done']
-            if 'input_state' in state:
-                self.input_state = state['input_state']
-
-        # TODO: Does this deplete the Linux entropy pool? Is this fast enough?
-        if not self.seed:
-            self.seed = struct.unpack('i', os.urandom(4))[0]
-
-        if not self.input_state:
-            self.input_state = {}
+
+        state = state if state else {}
+        self.seed = seed if seed else state.get('seed', struct.unpack('i', os.urandom(4))[0]) 
+        self.student_answers = state.get('student_answers', {})
+        if 'correct_map' in state:
+            self.correct_map.set_dict(state['correct_map'])
+        self.done = state.get('done', False)
+        self.input_state = state.get('input_state', {})
+
+
 
         # Convert startouttext and endouttext to proper <text></text>
         problem_text = re.sub("startouttext\s*/", "text", problem_text)
@@ -240,13 +233,14 @@ class LoncapaProblem(object):
 
     def ungraded_response(self, xqueue_msg, queuekey):
         '''
-        Handle any responses from the xqueue that are not related to grading
+        Handle any responses from the xqueue that do not contain grades
+        Will try to pass the queue message to all inputtypes that can handle ungraded responses 
 
         Does not return any value
         '''
         # check against each inputtype
         for the_input in self.inputs.values():
-            # if the input type has an xqueue_response function, pass in the values
+            # if the input type has an ungraded function, pass in the values
             if hasattr(the_input, 'ungraded_response'):
                 the_input.ungraded_response(xqueue_msg, queuekey)
 
@@ -542,11 +536,14 @@ class LoncapaProblem(object):
             if self.student_answers and problemid in self.student_answers:
                 value = self.student_answers[problemid]
             
+            if input_id not in self.input_state:
+                self.input_state[input_id] = {}
+                
             # do the rendering
             state = {'value': value,
                    'status': status,
                    'id': input_id,
-                   'input_state': self.input_state,
+                   'input_state': self.input_state[input_id],
                    'feedback': {'message': msg,
                                 'hint': hint,
                                 'hintmode': hintmode, }}

common/lib/capa/capa/inputtypes.py

@@ -161,7 +161,7 @@ class InputTypeBase(object):
         self.msg = feedback.get('message', '')
         self.hint = feedback.get('hint', '')
         self.hintmode = feedback.get('hintmode', None)
-        self.input_state_dict = state.get('input_state', {})
+        self.input_state = state.get('input_state', {})
 
         # put hint above msg if it should be displayed
         if self.hintmode == 'always':
@@ -591,14 +591,14 @@ class CodeInput(InputTypeBase):
                 Attribute('tabsize', 4, transform=int),
                 ]
 
-    def setup(self):
+    def setup_code_response_rendering(self):
         """
         Implement special logic: handle queueing state, and default input.
         """
         # if no student input yet, then use the default input given by the
         # problem
-        if not self.value:
-            self.value = self.xml.text
+        if not self.value and self.xml.text:
+            self.value = self.xml.text.strip()
 
         # Check if problem has been queued
         self.queue_len = 0
@@ -609,6 +609,11 @@ class CodeInput(InputTypeBase):
             self.queue_len = self.msg
             self.msg = self.submitted_msg
 
+
+    def setup(self):
+        ''' setup this input type '''
+        self.setup_code_response_rendering()
+
     def _extra_context(self):
         """Defined queue_len, add it """
         return {'queue_len': self.queue_len, }
@@ -623,8 +628,10 @@ class MatlabInput(CodeInput):
     '''
     InputType for handling Matlab code input
 
+    TODO: API_KEY will go away once we have a way to specify it per-course
     Example:
      <matlabinput rows="10" cols="80" tabsize="4">
+        Initial Text
         <plot_payload>
           %api_key=API_KEY
         </plot_payload>
@@ -633,51 +640,56 @@ class MatlabInput(CodeInput):
     template = "matlabinput.html"
     tags = ['matlabinput']
 
-    # pulled out for testing
-    submitted_msg = ("Submitted.  As soon as your submission is"
-                     " graded, this message will be replaced with the grader's feedback.")
+    plot_submitted_msg = ("Submitted. As soon as a response is returned, "
+                    "this message will be replaced by that feedback.")
 
     def setup(self):
         '''
         Handle matlab-specific parsing
         '''
-        # if we don't have state for this input type yet, make one
-        if self.id not in self.input_state_dict:
-            self.input_state_dict[self.id] = {}
+        self.setup_code_response_rendering()
 
-        self.input_state = self.input_state_dict[self.id]
         xml = self.xml
         self.plot_payload = xml.findtext('./plot_payload')
-        # if no student input yet, then use the default input given by the
-        # problem
-        if not self.value:
-            self.value = self.xml.text
 
         # Check if problem has been queued
-        self.queue_len = 0
         self.queuename = 'matlab'
-        # Flag indicating that the problem has been queued, 'msg' is length of
         self.queue_msg = ''
         if 'queue_msg' in self.input_state and self.status in ['incomplete', 'unsubmitted']:
             self.queue_msg = self.input_state['queue_msg']
         if 'queued' in self.input_state and self.input_state['queuestate'] is not None:
             self.status = 'queued'
             self.queue_len = 1
-        # queue
-        if self.status == 'incomplete':
-            self.status = 'queued'
-            self.queue_len = self.msg
-            self.msg = self.submitted_msg
-
+            self.msg = self.plot_submitted_msg
 
 
     def handle_ajax(self, dispatch, get):
-        ''' Handle AJAX calls directed to this input'''
+        ''' 
+        Handle AJAX calls directed to this input
+
+        Args:
+            - dispatch (str) - indicates how we want this ajax call to be handled
+            - get (dict) - dictionary of key-value pairs that contain useful data
+        Returns:
+            
+        '''
+
         if dispatch == 'plot':
             return self._plot_data(get)
+        return {}
 
     def ungraded_response(self, queue_msg, queuekey):
-        ''' Handle any XQueue responses that have to be saved and rendered '''
+        ''' 
+        Handle the response from the XQueue
+        Stores the response in the input_state so it can be rendered later
+
+        Args:
+            - queue_msg (str) - message returned from the queue. The message to be rendered
+            - queuekey (str) - a key passed to the queue. Will be matched up to verify that this is the response we're waiting for
+
+        Returns:
+            nothing
+        '''
         # check the queuekey against the saved queuekey
         if('queuestate' in self.input_state and self.input_state['queuestate'] == 'queued' 
                 and self.input_state['queuekey'] == queuekey):
@@ -697,9 +709,11 @@ class MatlabInput(CodeInput):
 
     def _parse_data(self, queue_msg):
         '''
-        takes a queue_msg returned from the queue and parses it and returns
-        whatever is stored in msg
-        returns string msg
+        Parses the message out of the queue message
+        Args:
+            queue_msg (str) - a JSON encoded string
+        Returns:
+            returns the value for the the key 'msg' in queue_msg
         '''
         try:
             result = json.loads(queue_msg)
@@ -712,42 +726,50 @@ class MatlabInput(CodeInput):
 
 
     def _plot_data(self, get):
-        ''' send data via xqueue to the mathworks backend'''
+        ''' 
+        AJAX handler for the plot button
+        Args:
+            get (dict) - should have key 'submission' which contains the student submission
+        Returns:
+            dict - 'success' - whether or not we successfully queued this submission
+                 - 'message' - message to be rendered in case of error 
+        '''
         # only send data if xqueue exists
-        if self.system.xqueue is not None:
-            # pull relevant info out of get
-            response = get['submission']
-
-            # construct xqueue headers
-            qinterface = self.system.xqueue['interface']
-            qtime = datetime.strftime(datetime.now(), xqueue_interface.dateformat)
-            callback_url = self.system.xqueue['construct_callback']('ungraded_response')
-            anonymous_student_id = self.system.anonymous_student_id
-            queuekey = xqueue_interface.make_hashkey(str(self.system.seed) + qtime +
-                                                     anonymous_student_id +
-                                                     self.id)
-            xheader = xqueue_interface.make_xheader(
-                    lms_callback_url = callback_url,
-                    lms_key = queuekey,
-                    queue_name = self.queuename)
-
-            # save the input state
-            self.input_state['queuekey'] = queuekey
-            self.input_state['queuestate'] = 'queued'
-
-
-            # construct xqueue body
-            student_info = {'anonymous_student_id': anonymous_student_id,
-                    'submission_time': qtime}
-            contents = {'grader_payload': self.plot_payload,
-                        'student_info': json.dumps(student_info),
-                        'student_response': response}
-
-            (error, msg) = qinterface.send_to_queue(header=xheader,
-                                                    body = json.dumps(contents))
-
-            return {'success': error == 0, 'message': msg}
-        return {'success': False, 'message': 'Cannot connect to the queue'}
+        if self.system.xqueue is None:
+            return {'success': False, 'message': 'Cannot connect to the queue'}
+
+        # pull relevant info out of get
+        response = get['submission']
+
+        # construct xqueue headers
+        qinterface = self.system.xqueue['interface']
+        qtime = datetime.strftime(datetime.utcnow(), xqueue_interface.dateformat)
+        callback_url = self.system.xqueue['construct_callback']('ungraded_response')
+        anonymous_student_id = self.system.anonymous_student_id
+        queuekey = xqueue_interface.make_hashkey(str(self.system.seed) + qtime +
+                                                 anonymous_student_id +
+                                                 self.id)
+        xheader = xqueue_interface.make_xheader(
+                lms_callback_url = callback_url,
+                lms_key = queuekey,
+                queue_name = self.queuename)
+
+        # save the input state
+        self.input_state['queuekey'] = queuekey
+        self.input_state['queuestate'] = 'queued'
+
+
+        # construct xqueue body
+        student_info = {'anonymous_student_id': anonymous_student_id,
+                'submission_time': qtime}
+        contents = {'grader_payload': self.plot_payload,
+                    'student_info': json.dumps(student_info),
+                    'student_response': response}
+
+        (error, msg) = qinterface.send_to_queue(header=xheader,
+                                                body = json.dumps(contents))
+
+        return {'success': error == 0, 'message': msg}
 
 
 registry.register(MatlabInput)

common/lib/capa/capa/responsetypes.py

@@ -1147,10 +1147,10 @@ def sympy_check2():
                     correct = []
                     messages = []
                     for input_dict in input_list:
-                        correct.append('correct' if input_dict[
-                                       'ok'] else 'incorrect')
-                        msg = self.clean_message_html(input_dict[
-                                                      'msg']) if 'msg' in input_dict else None
+                        correct.append('correct' 
+                                if input_dict['ok'] else 'incorrect')
+                        msg = (self.clean_message_html(input_dict['msg']) 
+                                if 'msg' in input_dict else None)
                         messages.append(msg)
 
                 # Otherwise, we do not recognize the dictionary
@@ -1164,8 +1164,8 @@ def sympy_check2():
             # indicating whether all inputs should be marked
             # correct or incorrect
             else:
-                correct = ['correct'] * len(
-                    idset) if ret else ['incorrect'] * len(idset)
+                n = len(idset)
+                correct = ['correct'] * n if ret else ['incorrect'] * n
 
         # build map giving "correct"ness of the answer(s)
         correct_map = CorrectMap()
@@ -1174,8 +1174,8 @@ def sympy_check2():
         correct_map.set_overall_message(overall_message)
 
         for k in range(len(idset)):
-            npoints = self.maxpoints[idset[
-                k]] if correct[k] == 'correct' else 0
+            npoints = (self.maxpoints[idset[k]] 
+                    if correct[k] == 'correct' else 0)
             correct_map.set(idset[k], correct[k], msg=messages[k],
                             npoints=npoints)
         return correct_map

common/lib/capa/capa/tests/test_inputtypes.py

@@ -357,7 +357,7 @@ class MatlabTest(unittest.TestCase):
     def test_rendering_with_state(self):
         state = {'value': 'print "good evening"',
                  'status': 'incomplete',
-                 'input_state': {'prob_1_2': {'queue_msg': 'message'}},
+                 'input_state': {'queue_msg': 'message'},
                  'feedback': {'message': '3'}, }
         elt = etree.fromstring(self.xml)
 

common/lib/xmodule/xmodule/capa_module.py

@@ -543,8 +543,16 @@ class CapaModule(CapaFields, XModule):
 
     def handle_ungraded_response(self, get):
         '''
-        Delivers a response to the capa problem where the expectation where this response does
-        not have relevant grading information
+        Delivers a response from the XQueue to the capa problem
+
+        The score of the problem will not be updated
+
+        Args:
+            - get (dict) must contain keys:
+                            queuekey - a key specific to this response
+                            xqueue_body - the body of the response
+        Returns:
+            empty dictionary
 
         No ajax return is needed, so an empty dict is returned
         '''
@@ -557,8 +565,12 @@ class CapaModule(CapaFields, XModule):
 
     def handle_input_ajax(self, get):
         '''
-        Passes information down to the capa problem so that it can handle its own ajax calls
-        Returns the response from the capa problem
+        Handle ajax calls meant for a particular input in the problem
+
+        Args:
+            - get (dict) - data that should be passed to the input
+        Returns:
+            - dict containing the response from the input
         '''
         response = self.lcp.handle_input_ajax(get)
         # save any state changes that may occur

common/lib/xmodule/xmodule/tests/test_combined_open_ended.py

@@ -183,7 +183,7 @@ class OpenEndedModuleTest(unittest.TestCase):
         self.test_system.location = self.location
         self.mock_xqueue = MagicMock()
         self.mock_xqueue.send_to_queue.return_value = (None, "Message")
-        def constructed_callback(dispatch = "score_update"):
+        def constructed_callback(dispatch="score_update"):
             return dispatch
         
         self.test_system.xqueue = {'interface': self.mock_xqueue, 'construct_callback': constructed_callback, 'default_queuename': 'testqueue',

lms/djangoapps/courseware/module_render.py

@@ -182,7 +182,7 @@ def get_module_for_descriptor(user, request, descriptor, model_data_cache, cours
         proto=request.META.get('HTTP_X_FORWARDED_PROTO', 'https' if request.is_secure() else 'http')
     )
 
-    def make_xqueue_callback(dispatch = 'score_update'):
+    def make_xqueue_callback(dispatch='score_update'):
         # Fully qualified callback URL for external queueing system
         xqueue_callback_url = '{proto}://{host}'.format(
             host=request.get_host(),