Changed hint voting ui to show all hints on one page.

Fixed broken tests for hint manager.

common/lib/xmodule/xmodule/crowdsource_hinter.py

@@ -18,6 +18,9 @@ from xblock.core import Scope, String, Integer, Boolean, Dict, List
 
 from capa.responsetypes import FormulaResponse, StudentInputError
 
+from calc import evaluator, UndefinedVariable
+from pyparsing import ParseException
+
 from django.utils.html import escape
 
 log = logging.getLogger(__name__)
@@ -85,8 +88,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
             self.answer_signature = self.formula_answer_signature
         else:
             self.answer_to_str = self.numerical_answer_to_str
-            # Right now, numerical problems don't need special answer signature treatment.
-            self.answer_signature = lambda x: x
+            self.answer_signature = self.numerical_answer_signature
 
     def get_html(self):
         """
@@ -134,6 +136,17 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         """
         return str(answer.values()[0])
 
+    def numerical_answer_signature(self, answer):
+        """
+        Runs the answer string through the evaluator.  (This is because
+        symbolic expressions like sin(pi/12)*3 are allowed.)
+        """
+        try:
+            out = str(evaluator(dict(), dict(), answer))
+        except (UndefinedVariable, ParseException):
+            out = None
+        return out
+
     def formula_answer_signature(self, answer):
         """
         Converts a capa answer string (output of formula_answer_to_str)
@@ -200,7 +213,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
             return
         # Make a signature of the answer, for formula responses.
         signature = self.answer_signature(answer)
-        if signature == None:
+        if signature is None:
             # Sometimes, signature conversion may fail.
             log.exception('Signature conversion failed: ' + str(answer))
             return
@@ -258,13 +271,21 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         # be allowed to make one vote / submission, but he can choose which wrong answer
         # he wants to look at.
         answer_to_hints = {}    # answer_to_hints[answer text][hint pk] -> hint text
+        signature_to_ans = {}   # Lets us combine equivalent answers
+                                # Same mapping as the field, but local.
 
         # Go through each previous answer, and populate index_to_hints and index_to_answer.
         for i in xrange(len(self.previous_answers)):
             answer, hints_offered = self.previous_answers[i]
+            # Does this answer equal one of the ones offered already?
+            signature = self.answer_signature(answer)
+            if signature in signature_to_ans:
+                # Re-assign this answer text to the one we've seen already.
+                answer = signature_to_ans[signature]
+            else:
+                signature_to_ans[signature] = answer
             if answer not in answer_to_hints:
                 answer_to_hints[answer] = {}
-            signature = self.answer_signature(answer)
             if signature in self.hints:
                 # Go through each hint, and add to index_to_hints
                 for hint_id in hints_offered:
@@ -285,9 +306,10 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         `data` -- expected to have the following keys:
             'answer': text of answer we're voting on
             'hint': hint_pk
-            'pk_list': We will return a list of how many votes each hint has so far.
+            'pk_list': A list of [answer, pk] pairs, each of which representing a hint.
+                       We will return a list of how many votes each hint in the list has so far.
                        It's up to the browser to specify which hints to return vote counts for.
-                       Every pk listed here will have a hint count returned.
+                       
         Returns key 'hint_and_votes', a list of (hint_text, #votes) pairs.
         """
         if self.user_voted:
@@ -299,7 +321,6 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
             log.exception('Failure in hinter tally_vote: Unable to parse answer: ' + ans)
             return {'error': 'Failure in voting!'}
         hint_pk = str(data['hint'])
-        pk_list = json.loads(data['pk_list'])
         # We use temp_dict because we need to do a direct write for the database to update.
         temp_dict = self.hints
         try:
@@ -313,12 +334,18 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         self.user_voted = True
 
         # Return a list of how many votes each hint got.
+        pk_list = json.loads(data['pk_list'])
         hint_and_votes = []
-        for vote_pk in pk_list:
+        for answer, vote_pk in pk_list:
+            signature = self.answer_signature(answer)
+            if signature is None:
+                log.exception('In hinter tally_vote, couldn\'t parse ' + answer)
+                continue
             try:
                 hint_and_votes.append(temp_dict[signature][str(vote_pk)])
             except KeyError:
-                log.exception('In hinter tally_vote: pk_list contains non-existant pk: ' + str(vote_pk))
+                log.exception('In hinter tally_vote, couldn\'t find: '
+                              + answer + ', ' + str(vote_pk))
 
         hint_and_votes.sort(key=lambda pair: pair[1], reverse=True)
         # Reset self.previous_answers.

common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee

@@ -48,9 +48,7 @@ class @Hinter
 
   vote: (eventObj) =>
     target = @$(eventObj.currentTarget)
-    parent_div = $('.previous-answer[data-answer="'+target.attr('data-answer')+'"]')
-    all_pks = parent_div.attr('data-all-pks')
-    console.debug(all_pks)
+    all_pks = @$('#pk-list').attr('data-pk-list')
     post_json = {'answer': target.attr('data-answer'), 'hint': target.data('hintno'), 'pk_list': all_pks}
     $.postWithPrefix "#{@url}/vote", post_json, (response) =>
       @render(response.contents)
@@ -58,7 +56,6 @@ class @Hinter
   submit_hint: (eventObj) =>
     target = @$(eventObj.currentTarget)
     textarea = $('.custom-hint[data-answer="'+target.attr('data-answer')+'"]')
-    console.debug(textarea)
     post_json = {'answer': target.attr('data-answer'), 'hint': @$(textarea).val()}
     $.postWithPrefix "#{@url}/submit_hint",post_json, (response) =>
       @render(response.contents)

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

@@ -288,6 +288,26 @@ class CrowdsourceHinterTest(unittest.TestCase):
         parsed = mock_module.formula_answer_to_str(get)
         self.assertTrue(parsed == 'x*y^2')
 
+    def test_numerical_answer_signature(self):
+        """
+        Tests answer signature generator for numerical responses.
+        """
+        mock_module = CHModuleFactory.create()
+        answer = '4*5+3'
+        signature = mock_module.numerical_answer_signature(answer)
+        print signature
+        self.assertTrue(signature == '23.0')
+
+    def test_numerical_answer_signature_failure(self):
+        """
+        Makes sure that unparsable numerical answers return None.
+        """
+        mock_module = CHModuleFactory.create()
+        answer = 'fish'
+        signature = mock_module.numerical_answer_signature(answer)
+        print signature
+        self.assertTrue(signature is None)
+
     def test_formula_answer_signature(self):
         """
         Tests the answer signature generator for formula responses.
@@ -406,7 +426,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         Should not change any vote tallies.
         """
         mock_module = CHModuleFactory.create(user_voted=True)
-        json_in = {'answer': '24.0', 'hint': 1, 'pk_list': json.dumps([1, 3])}
+        json_in = {'answer': '24.0', 'hint': 1, 'pk_list': json.dumps([['24.0', 1], ['24.0', 3]])}
         old_hints = copy.deepcopy(mock_module.hints)
         mock_module.tally_vote(json_in)
         self.assertTrue(mock_module.hints == old_hints)
@@ -418,7 +438,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         """
         mock_module = CHModuleFactory.create(
             previous_answers=[['24.0', [0, 3, None]]])
-        json_in = {'answer': '24.0', 'hint': 3, 'pk_list': json.dumps([0, 3])}
+        json_in = {'answer': '24.0', 'hint': 3, 'pk_list': json.dumps([['24.0', 0], ['24.0', 3]])}
         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)
@@ -457,7 +477,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         Should just skip those.
         """
         mock_module = CHModuleFactory.create()
-        json_in = {'answer': '24.0', 'hint': '0', 'pk_list': json.dumps([0, 12])}
+        json_in = {'answer': '24.0', 'hint': '0', 'pk_list': json.dumps([['24.0', 0], ['24.0', 12]])}
         hint_and_votes = mock_module.tally_vote(json_in)['hint_and_votes']
         self.assertTrue(['Best hint', 41] in hint_and_votes)
         self.assertTrue(len(hint_and_votes) == 1)

common/templates/hinter_display.html

@@ -18,11 +18,6 @@
 </%def>
 
 <%def name="get_feedback()">
-    <p><em> Participation in the hinting system is strictly optional, and will not influence your grade. </em></p>
-    <p>
-    Help your classmates by writing hints for this problem. Start by picking one of your previous incorrect answers from below:
-    </p>
-
     <%
     def unspace(string):
         """
@@ -30,7 +25,44 @@
         removes spaces.
         """
         return ''.join(string.split())
+
+    # Make a list of all hints shown.  (This is fed back to the site as pk_list.)
+    # At the same time, determine whether any hints were shown at all.
+    # If the user never saw hints, don't ask him to vote.
+    import json
+    hints_exist = False
+    pk_list = []
+    for answer, pk_dict in answer_to_hints.items():
+        if len(pk_dict) > 0:
+            hints_exist = True
+        for pk, hint_text in pk_dict.items():
+            pk_list.append([answer, pk])
+    json_pk_list = json.dumps(pk_list)
     %>
+    % if hints_exist:
+        <p>
+        <em> Optional. </em> Help us improve our hints!  Which hint was most helpful to you?
+        </p>
+
+        <div id="pk-list" data-pk-list='${json_pk_list}' style="display:none"> </div>
+
+        % for answer, pk_dict in answer_to_hints.items():
+            % for hint_pk, hint_text in pk_dict.items():
+                <p>
+                <input class="vote" data-answer="${answer}" data-hintno="${hint_pk}" type="button" value="Vote">
+                ${answer}: ${hint_text}
+                </p>
+            % endfor
+        % endfor
+
+        <p>
+        Don't like any of the hints above?  Please write your own, for one of your previous incorrect answers below:
+        </p>
+    % else:
+        <p>
+        <em> Optional. </em> Help us write hints for this problem!  Select one of your incorrect answers below:
+        </p>
+    % endif
 
     <div id="answer-tabs">
         <ul>
@@ -40,36 +72,22 @@
         </ul>
 
         % for answer, pk_dict in answer_to_hints.items():
-        <%
-        import json
-        all_pks = json.dumps(pk_dict.keys())
-        %>
-        <div class = "previous-answer" id="previous-answer-${unspace(answer)}" data-answer="${answer}" data-all-pks='${all_pks}'>
-        <div class = "hint-inner-container">
-            % if len(pk_dict) > 0:
-                <p>
-                Which hint would be most effective to show a student who also got ${answer}?
-                </p>
-                % for hint_pk, hint_text in pk_dict.items():
-                    <p>
-                    <input class="vote" data-answer="${answer}" data-hintno="${hint_pk}" type="button" value="Vote">
-                    ${hint_text}
-                    </p>
-                % endfor
+            <%
+                import json
+                all_pks = json.dumps(pk_dict.keys())
+            %>
+            <div class = "previous-answer" id="previous-answer-${unspace(answer)}" data-answer="${answer}" data-all-pks='${all_pks}'>
+            <div class = "hint-inner-container">
                 <p>
-                Don't like any of the hints above?  You can also submit your own.
+                What hint would you give a student who made the same mistake you did?  Please don't give away the answer.
                 </p>
-            % endif
-            <p>
-            What hint would you give a student who made the same mistake you did?  Please don't give away the answer.
-            </p>
-            <textarea cols="50" class="custom-hint" data-answer="${answer}">
+                <textarea cols="50" class="custom-hint" data-answer="${answer}">
 What would you say to help someone who got this wrong answer?
 (Don't give away the answer, please.)
-            </textarea>
-            <br/><br/>
-            <input class="submit-hint" data-answer="${answer}" type="button" value="submit">
-        </div></div>
+                </textarea>
+                <br/><br/>
+                <input class="submit-hint" data-answer="${answer}" type="button" value="submit">
+            </div></div>
         % endfor
     </div>
 

lms/djangoapps/instructor/hint_manager.py

@@ -16,8 +16,8 @@ from mitxmako.shortcuts import render_to_response, render_to_string
 
 from courseware.courses import get_course_with_access
 from courseware.models import XModuleContentField
-from courseware.module_render import get_module
-from courseware.model_data import ModelDataCache
+import courseware.module_render as module_render
+import courseware.model_data as model_data
 from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 
@@ -232,8 +232,8 @@ def add_hint(request, course_id, field):
     # any alternative :(
     loc = Location(problem_id)
     descriptors = modulestore().get_items(loc)
-    m_d_c = ModelDataCache(descriptors, course_id, request.user)
-    hinter_module = get_module(request.user, request, loc, m_d_c, course_id)
+    m_d_c = model_data.ModelDataCache(descriptors, course_id, request.user)
+    hinter_module = module_render.get_module(request.user, request, loc, m_d_c, course_id)
     signature = hinter_module.answer_signature(answer)
     if signature is None:
         # Signature generation failed.

lms/djangoapps/instructor/tests/test_hint_manager.py

@@ -2,6 +2,7 @@ import json
 
 from django.test.client import Client, RequestFactory
 from django.test.utils import override_settings
+from mock import MagicMock
 
 from courseware.models import XModuleContentField
 from courseware.tests.factories import ContentFactory
@@ -89,7 +90,7 @@ class HintManagerTest(ModuleStoreTestCase):
         out = view.get_hints(post, self.course_id, 'mod_queue')
         print out
         self.assertTrue(out['other_field'] == 'hints')
-        expected = {self.problem_id: [(u'2.0', {u'2': [u'Hint 2', 1]})]}
+        expected = {self.problem_id: [[u'2.0', u'2.0', {u'2': [u'Hint 2', 1]}]]}
         self.assertTrue(out['all_hints'] == expected)
 
     def test_gethints_other(self):
@@ -101,9 +102,9 @@ class HintManagerTest(ModuleStoreTestCase):
         out = view.get_hints(post, self.course_id, 'hints')
         print out
         self.assertTrue(out['other_field'] == 'mod_queue')
-        expected = {self.problem_id: [('1.0', {'1': ['Hint 1', 2],
-                                               '3': ['Hint 3', 12]}),
-                                      ('2.0', {'4': ['Hint 4', 3]})
+        expected = {self.problem_id: [['1.0', '1.0', {'1': ['Hint 1', 2],
+                                                      '3': ['Hint 3', 12]}],
+                                      ['2.0', '2.0', {'4': ['Hint 4', 3]}]
                                       ]}
         self.assertTrue(out['all_hints'] == expected)
 
@@ -137,16 +138,30 @@ class HintManagerTest(ModuleStoreTestCase):
         """
         Check that instructors can add new hints.
         """
+        # Because add_hint accesses the xmodule, this test requires a bunch
+        # of monkey patching.
+        import courseware.module_render as module_render
+        import courseware.model_data as model_data
+        hinter = MagicMock()
+        hinter.answer_signature = lambda string: string
+        module_render.get_module = MagicMock(return_value=hinter)
+        model_data.ModelDataCache = MagicMock(return_value=None)
+
         request = RequestFactory()
         post = request.post(self.url, {'field': 'mod_queue',
                                        'op': 'add hint',
                                        'problem': self.problem_id,
                                        'answer': '3.14',
                                        'hint': 'This is a new hint.'})
+        post.user = 'fake user'
         view.add_hint(post, self.course_id, 'mod_queue')
         problem_hints = XModuleContentField.objects.get(field_name='mod_queue', definition_id=self.problem_id).value
         self.assertTrue('3.14' in json.loads(problem_hints))
 
+        # Reload the things we monkey-patched.
+        reload(module_render)
+        reload(model_data)
+
     def test_approve(self):
         """
         Check that instructors can approve hints.  (Move them