Merge pull request #328 from edx/felix/hinter2

Felix/hinter2

common/lib/xmodule/xmodule/crowdsource_hinter.py

@@ -107,18 +107,18 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         """
         return str(float(answer.values()[0]))
 
-    def handle_ajax(self, dispatch, get):
+    def handle_ajax(self, dispatch, data):
         """
         This is the landing method for AJAX calls.
         """
         if dispatch == 'get_hint':
-            out = self.get_hint(get)
+            out = self.get_hint(data)
         elif dispatch == 'get_feedback':
-            out = self.get_feedback(get)
+            out = self.get_feedback(data)
         elif dispatch == 'vote':
-            out = self.tally_vote(get)
+            out = self.tally_vote(data)
         elif dispatch == 'submit_hint':
-            out = self.submit_hint(get)
+            out = self.submit_hint(data)
         else:
             return json.dumps({'contents': 'Error - invalid operation.'})
 
@@ -128,19 +128,24 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
             out.update({'op': dispatch})
         return json.dumps({'contents': self.system.render_template('hinter_display.html', out)})
 
-    def get_hint(self, get):
+    def get_hint(self, data):
         """
-        The student got the incorrect answer found in get.  Give him a hint.
+        The student got the incorrect answer found in data.  Give him a hint.
 
         Called by hinter javascript after a problem is graded as incorrect.
         Args:
-        `get` -- must be interpretable by capa_answer_to_str.
+        `data` -- must be interpretable by capa_answer_to_str.
         Output keys:
             - 'best_hint' is the hint text with the most votes.
-            - 'rand_hint_1' and 'rand_hint_2' are two random hints to the answer in `get`.
+            - 'rand_hint_1' and 'rand_hint_2' are two random hints to the answer in `data`.
             - 'answer' is the parsed answer that was submitted.
         """
-        answer = self.capa_answer_to_str(get)
+        try:
+            answer = self.capa_answer_to_str(data)
+        except ValueError:
+            # Sometimes, we get an answer that's just not parsable.  Do nothing.
+            log.exception('Answer not parsable: ' + str(data))
+            return
         # Look for a hint to give.
         # Make a local copy of self.hints - this means we only need to do one json unpacking.
         # (This is because xblocks storage makes the following command a deep copy.)
@@ -176,12 +181,12 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
                 'rand_hint_2': rand_hint_2,
                 'answer': answer}
 
-    def get_feedback(self, get):
+    def get_feedback(self, data):
         """
         The student got it correct.  Ask him to vote on hints, or submit a hint.
 
         Args:
-        `get` -- not actually used.  (It is assumed that the answer is correct.)
+        `data` -- not actually used.  (It is assumed that the answer is correct.)
         Output keys:
             - 'index_to_hints' maps previous answer indices to hints that the user saw earlier.
             - 'index_to_answer' maps previous answer indices to the actual answer submitted.
@@ -216,20 +221,20 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
 
         return {'index_to_hints': index_to_hints, 'index_to_answer': index_to_answer}
 
-    def tally_vote(self, get):
+    def tally_vote(self, data):
         """
         Tally a user's vote on his favorite hint.
 
         Args:
-        `get` -- expected to have the following keys:
+        `data` -- expected to have the following keys:
             'answer': ans_no (index in previous_answers)
             'hint': hint_pk
         Returns key 'hint_and_votes', a list of (hint_text, #votes) pairs.
         """
         if self.user_voted:
             return {}
-        ans_no = int(get['answer'])
-        hint_no = str(get['hint'])
+        ans_no = int(data['answer'])
+        hint_no = str(data['hint'])
         answer = self.previous_answers[ans_no][0]
         # We use temp_dict because we need to do a direct write for the database to update.
         temp_dict = self.hints
@@ -249,19 +254,19 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         self.previous_answers = []
         return {'hint_and_votes': hint_and_votes}
 
-    def submit_hint(self, get):
+    def submit_hint(self, data):
         """
         Take a hint submission and add it to the database.
 
         Args:
-        `get` -- expected to have the following keys:
+        `data` -- expected to have the following keys:
             'answer': answer index in previous_answers
             'hint': text of the new hint that the user is adding
         Returns a thank-you message.
         """
         # Do html escaping.  Perhaps in the future do profanity filtering, etc. as well.
-        hint = escape(get['hint'])
-        answer = self.previous_answers[int(get['answer'])][0]
+        hint = escape(data['hint'])
+        answer = self.previous_answers[int(data['answer'])][0]
         # Only allow a student to vote or submit a hint once.
         if self.user_voted:
             return {'message': 'Sorry, but you have already voted!'}

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

@@ -185,15 +185,15 @@ class CrowdsourceHinterTest(unittest.TestCase):
         A simple test of get_html - make sure it returns the html of the inner
         problem.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
 
         def fake_get_display_items():
             """
             A mock of get_display_items
             """
             return [FakeChild()]
-        m.get_display_items = fake_get_display_items
-        out_html = m.get_html()
+        mock_module.get_display_items = fake_get_display_items
+        out_html = mock_module.get_html()
         self.assertTrue('This is supposed to be test html.' in out_html)
         self.assertTrue('this/is/a/fake/ajax/url' in out_html)
 
@@ -202,15 +202,15 @@ class CrowdsourceHinterTest(unittest.TestCase):
         get_html, except the module has no child :(  Should return a polite
         error message.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
 
         def fake_get_display_items():
             """
             Returns no children.
             """
             return []
-        m.get_display_items = fake_get_display_items
-        out_html = m.get_html()
+        mock_module.get_display_items = fake_get_display_items
+        out_html = mock_module.get_html()
         self.assertTrue('Error in loading crowdsourced hinter' in out_html)
 
     @unittest.skip("Needs to be finished.")
@@ -220,8 +220,8 @@ class CrowdsourceHinterTest(unittest.TestCase):
         is called.
         NOT WORKING RIGHT NOW
         """
-        m = VerticalWithModulesFactory.create()
-        out_html = m.get_html()
+        mock_module = VerticalWithModulesFactory.create()
+        out_html = mock_module.get_html()
         print out_html
         self.assertTrue('Test numerical problem.' in out_html)
         self.assertTrue('Another test numerical problem.' in out_html)
@@ -232,20 +232,32 @@ class CrowdsourceHinterTest(unittest.TestCase):
         - Output should be blank.
         - New entry should be added to previous_answers
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '26.0'}
-        out = m.get_hint(json_in)
+        out = mock_module.get_hint(json_in)
         self.assertTrue(out is None)
-        self.assertTrue(['26.0', [None, None, None]] in m.previous_answers)
+        self.assertTrue(['26.0', [None, None, None]] in mock_module.previous_answers)
+
+    def test_gethint_unparsable(self):
+        """
+        Someone submits a hint that cannot be parsed into a float.
+        - The answer should not be added to previous_answers.
+        """
+        mock_module = CHModuleFactory.create()
+        old_answers = copy.deepcopy(mock_module.previous_answers)
+        json_in = {'problem_name': 'fish'}
+        out = mock_module.get_hint(json_in)
+        self.assertTrue(out is None)
+        self.assertTrue(mock_module.previous_answers == old_answers)
 
     def test_gethint_1hint(self):
         """
         Someone asks for a hint, with exactly one hint in the database.
         Output should contain that hint.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '25.0'}
-        out = m.get_hint(json_in)
+        out = mock_module.get_hint(json_in)
         self.assertTrue(out['best_hint'] == 'Really popular hint')
 
     def test_gethint_manyhints(self):
@@ -256,9 +268,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
         Currently, the best hint could be returned twice - need to fix this
         in implementation.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '24.0'}
-        out = m.get_hint(json_in)
+        out = mock_module.get_hint(json_in)
         self.assertTrue(out['best_hint'] == 'Best hint')
         self.assertTrue('rand_hint_1' in out)
         self.assertTrue('rand_hint_2' in out)
@@ -268,9 +280,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
         Someone has gotten the problem correct on the first try.
         Output should be empty.
         """
-        m = CHModuleFactory.create(previous_answers=[])
+        mock_module = CHModuleFactory.create(previous_answers=[])
         json_in = {'problem_name': '42.5'}
-        out = m.get_feedback(json_in)
+        out = mock_module.get_feedback(json_in)
         self.assertTrue(out is None)
 
     def test_getfeedback_1wronganswer_nohints(self):
@@ -279,9 +291,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
         answer.  However, we don't actually have hints for this problem.
         There should be a dialog to submit a new hint.
         """
-        m = CHModuleFactory.create(previous_answers=[['26.0', [None, None, None]]])
+        mock_module = CHModuleFactory.create(previous_answers=[['26.0', [None, None, None]]])
         json_in = {'problem_name': '42.5'}
-        out = m.get_feedback(json_in)
+        out = mock_module.get_feedback(json_in)
         print out['index_to_answer']
         self.assertTrue(out['index_to_hints'][0] == [])
         self.assertTrue(out['index_to_answer'][0] == '26.0')
@@ -292,9 +304,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
         a voting dialog, with the correct choices, plus a hint submission
         dialog.
         """
-        m = CHModuleFactory.create(previous_answers=[['24.0', [0, 3, None]]])
+        mock_module = CHModuleFactory.create(previous_answers=[['24.0', [0, 3, None]]])
         json_in = {'problem_name': '42.5'}
-        out = m.get_feedback(json_in)
+        out = mock_module.get_feedback(json_in)
         print out['index_to_hints']
         self.assertTrue(len(out['index_to_hints'][0]) == 2)
 
@@ -303,10 +315,10 @@ class CrowdsourceHinterTest(unittest.TestCase):
         Someone gets a problem correct, but one of the hints that he saw
         earlier (pk=100) has been deleted.  Should just skip that hint.
         """
-        m = CHModuleFactory.create(
+        mock_module = CHModuleFactory.create(
             previous_answers=[['24.0', [0, 100, None]]])
         json_in = {'problem_name': '42.5'}
-        out = m.get_feedback(json_in)
+        out = mock_module.get_feedback(json_in)
         self.assertTrue(len(out['index_to_hints'][0]) == 1)
 
     def test_vote_nopermission(self):
@@ -314,23 +326,23 @@ class CrowdsourceHinterTest(unittest.TestCase):
         A user tries to vote for a hint, but he has already voted!
         Should not change any vote tallies.
         """
-        m = CHModuleFactory.create(user_voted=True)
+        mock_module = CHModuleFactory.create(user_voted=True)
         json_in = {'answer': 0, 'hint': 1}
-        old_hints = copy.deepcopy(m.hints)
-        m.tally_vote(json_in)
-        self.assertTrue(m.hints == old_hints)
+        old_hints = copy.deepcopy(mock_module.hints)
+        mock_module.tally_vote(json_in)
+        self.assertTrue(mock_module.hints == old_hints)
 
     def test_vote_withpermission(self):
         """
         A user votes for a hint.
         Also tests vote result rendering.
         """
-        m = CHModuleFactory.create(
+        mock_module = CHModuleFactory.create(
             previous_answers=[['24.0', [0, 3, None]]])
         json_in = {'answer': 0, 'hint': 3}
-        dict_out = m.tally_vote(json_in)
-        self.assertTrue(m.hints['24.0']['0'][1] == 40)
-        self.assertTrue(m.hints['24.0']['3'][1] == 31)
+        dict_out = mock_module.tally_vote(json_in)
+        self.assertTrue(mock_module.hints['24.0']['0'][1] == 40)
+        self.assertTrue(mock_module.hints['24.0']['3'][1] == 31)
         self.assertTrue(['Best hint', 40] in dict_out['hint_and_votes'])
         self.assertTrue(['Another hint', 31] in dict_out['hint_and_votes'])
 
@@ -338,34 +350,34 @@ class CrowdsourceHinterTest(unittest.TestCase):
         """
         A user tries to submit a hint, but he has already voted.
         """
-        m = CHModuleFactory.create(user_voted=True)
+        mock_module = CHModuleFactory.create(user_voted=True)
         json_in = {'answer': 1, 'hint': 'This is a new hint.'}
-        print m.user_voted
-        m.submit_hint(json_in)
-        print m.hints
-        self.assertTrue('29.0' not in m.hints)
+        print mock_module.user_voted
+        mock_module.submit_hint(json_in)
+        print mock_module.hints
+        self.assertTrue('29.0' not in mock_module.hints)
 
     def test_submithint_withpermission_new(self):
         """
         A user submits a hint to an answer for which no hints
         exist yet.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
         json_in = {'answer': 1, 'hint': 'This is a new hint.'}
-        m.submit_hint(json_in)
-        self.assertTrue('29.0' in m.hints)
+        mock_module.submit_hint(json_in)
+        self.assertTrue('29.0' in mock_module.hints)
 
     def test_submithint_withpermission_existing(self):
         """
         A user submits a hint to an answer that has other hints
         already.
         """
-        m = CHModuleFactory.create(previous_answers=[['25.0', [1, None, None]]])
+        mock_module = CHModuleFactory.create(previous_answers=[['25.0', [1, None, None]]])
         json_in = {'answer': 0, 'hint': 'This is a new hint.'}
-        m.submit_hint(json_in)
+        mock_module.submit_hint(json_in)
         # Make a hint request.
         json_in = {'problem name': '25.0'}
-        out = m.get_hint(json_in)
+        out = mock_module.get_hint(json_in)
         self.assertTrue((out['best_hint'] == 'This is a new hint.')
                         or (out['rand_hint_1'] == 'This is a new hint.'))
 
@@ -375,27 +387,27 @@ class CrowdsourceHinterTest(unittest.TestCase):
         show up in the mod_queue, not the public-facing hints
         dict.
         """
-        m = CHModuleFactory.create(moderate='True')
+        mock_module = CHModuleFactory.create(moderate='True')
         json_in = {'answer': 1, 'hint': 'This is a new hint.'}
-        m.submit_hint(json_in)
-        self.assertTrue('29.0' not in m.hints)
-        self.assertTrue('29.0' in m.mod_queue)
+        mock_module.submit_hint(json_in)
+        self.assertTrue('29.0' not in mock_module.hints)
+        self.assertTrue('29.0' in mock_module.mod_queue)
 
     def test_submithint_escape(self):
         """
         Make sure that hints are being html-escaped.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
         json_in = {'answer': 1, 'hint': '<script> alert("Trololo"); </script>'}
-        m.submit_hint(json_in)
-        print m.hints
-        self.assertTrue(m.hints['29.0'][0][0] == u'&lt;script&gt; alert(&quot;Trololo&quot;); &lt;/script&gt;')
+        mock_module.submit_hint(json_in)
+        print mock_module.hints
+        self.assertTrue(mock_module.hints['29.0'][0][0] == u'&lt;script&gt; alert(&quot;Trololo&quot;); &lt;/script&gt;')
 
     def test_template_gethint(self):
         """
         Test the templates for get_hint.
         """
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
 
         def fake_get_hint(get):
             """
@@ -407,9 +419,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
                     'rand_hint_2': 'Another random hint',
                     'answer': '42.5'}
 
-        m.get_hint = fake_get_hint
+        mock_module.get_hint = fake_get_hint
         json_in = {'problem_name': '42.5'}
-        out = json.loads(m.handle_ajax('get_hint', json_in))['contents']
+        out = json.loads(mock_module.handle_ajax('get_hint', json_in))['contents']
         self.assertTrue('This is the best hint.' in out)
         self.assertTrue('A random hint' in out)
         self.assertTrue('Another random hint' in out)
@@ -420,7 +432,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         NOT FINISHED
 
         from lxml import etree
-        m = CHModuleFactory.create()
+        mock_module = CHModuleFactory.create()
 
         def fake_get_feedback(get):
             index_to_answer = {'0': '42.0', '1': '9000.01'}
@@ -429,9 +441,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
                               '1': [('A hint for 9000.01', 32)]}
             return {'index_to_hints': index_to_hints, 'index_to_answer': index_to_answer}
 
-        m.get_feedback = fake_get_feedback
+        mock_module.get_feedback = fake_get_feedback
         json_in = {'problem_name': '42.5'}
-        out = json.loads(m.handle_ajax('get_feedback', json_in))['contents']
+        out = json.loads(mock_module.handle_ajax('get_feedback', json_in))['contents']
         html_tree = etree.XML(out)
         # To be continued...
 

common/templates/hinter_display.html

@@ -95,13 +95,17 @@ What would you say to help someone who got this wrong answer?
 </%def>
 
 <%def name="show_votes()">
-    Thank you for voting!
-    <br />
-    % for hint, votes in hint_and_votes:
-        <span style="color:green"> ${votes} votes. </span>
-        ${hint}
-        <br />
-    % endfor
+    % if hint_and_votes is UNDEFINED:
+      Sorry, but you've already voted!
+    % else:
+      Thank you for voting!
+      <br />
+      % for hint, votes in hint_and_votes:
+          <span style="color:green"> ${votes} votes. </span>
+          ${hint}
+          <br />
+      % endfor
+    % endif
 </%def>
 
 <%def name="simple_message()">
@@ -123,3 +127,4 @@ What would you say to help someone who got this wrong answer?
 % if op == "vote":
     ${show_votes()}
 % endif
+