Fixed tests for removing hash access to hints. Fixed instructor view for same.

common/lib/xmodule/xmodule/crowdsource_hinter.py

@@ -188,6 +188,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         if not self.validate_answer(answer):
             # Answer is not in the right form.
             log.exception('Answer not valid: ' + str(answer))
+            return
         if answer not in self.user_submissions:
             self.user_submissions += [answer]
         # Look for a hint to give.
@@ -219,12 +220,12 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule):
         self.previous_answers += [[best_hint_answer, [best_hint_index]]]
         for i in xrange(min(2, n_hints-1)):
             # Keep making random hints until we hit a target, or run out.
-            (hint_index, (rand_hint, votes, hint_answer)) =\
-                random.choice(matching_hints.items())
-            if rand_hint in hints:
-                # Don't show the same hint twice.  Try again.
-                i -= 1
-                continue
+            go_on = False
+            while not go_on:
+                (hint_index, (rand_hint, votes, hint_answer)) =\
+                    random.choice(matching_hints.items())
+                if not rand_hint in hints:
+                    go_on = True
             hints.append(rand_hint)
             self.previous_answers += [[hint_index, [hint_answer]]]
         return {'hints': hints,

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

@@ -55,6 +55,7 @@ class CHModuleFactory(object):
     @staticmethod
     def create(hints=None,
                previous_answers=None,
+               user_submissions=None,
                user_voted=None,
                moderate=None,
                mod_queue=None):
@@ -87,9 +88,14 @@ class CHModuleFactory(object):
         else:
             model_data['previous_answers'] = [
                 ['24.0', [0, 3, 4]],
-                ['29.0', [None, None, None]]
+                ['29.0', []]
             ]
 
+        if user_submissions is not None:
+            model_data['user_submissions'] = user_submissions
+        else:
+            model_data['user_submissions'] = ['24.0', '29.0']
+
         if user_voted is not None:
             model_data['user_voted'] = user_voted
 
@@ -104,7 +110,24 @@ class CHModuleFactory(object):
 
         # Make a fake capa module.
         capa_module = MagicMock()
-        capa_module.responders = {'responder0': MagicMock()}
+        capa_module.lcp = MagicMock()
+        responder = MagicMock()
+
+        def validate_answer(answer):
+            """ A mock answer validator - simulates a numerical response"""
+            try:
+                float(answer)
+                return True
+            except ValueError:
+                return False
+        responder.validate_answer = validate_answer
+
+        def answer_compare(a, b):
+            """ A fake answer comparer """
+            return a == b
+        responder.answer_compare = answer_compare
+
+        capa_module.lcp.responders = {'responder0': responder}
         capa_module.displayable_items = lambda: [capa_module]
 
         system = get_test_system()
@@ -122,28 +145,6 @@ class CHModuleFactory(object):
 
         return module
 
-    @staticmethod
-    def setup_formula_response(module):
-        """
-        Adds additional mock methods to the module, so that we can test
-        formula responses.
-        """
-        # Do a bunch of monkey patching, to mock the lon-capa problem.
-        responder = MagicMock()
-        responder.randomize_variables = MagicMock(return_value=4)
-
-        def fake_hash_answers(answer, test_values):
-            """ A fake answer hasher """
-            if test_values == 4 and answer == 'x*y^2':
-                return 'good'
-            raise StudentInputError
-
-        responder.hash_answers = fake_hash_answers
-        lcp = MagicMock()
-        lcp.responders = {'responder0': responder}
-        module.get_display_items()[0].lcp = lcp
-        return module
-
 
 class VerticalWithModulesFactory(object):
     """
@@ -288,46 +289,6 @@ 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.
-        """
-        mock_module = CHModuleFactory.create()
-        mock_module = CHModuleFactory.setup_formula_response(mock_module)
-        answer = 'x*y^2'
-        out = mock_module.formula_answer_signature(answer)
-        self.assertTrue(out == 'good')
-
-    def test_formula_answer_signature_failure(self):
-        """
-        Makes sure that bad answer strings return None as a signature.
-        """
-        mock_module = CHModuleFactory.create()
-        mock_module = CHModuleFactory.setup_formula_response(mock_module)
-        answer = 'fish'
-        out = mock_module.formula_answer_signature(answer)
-        self.assertTrue(out is None)
-
     def test_gethint_0hint(self):
         """
         Someone asks for a hint, when there's no hint to give.
@@ -337,8 +298,9 @@ class CrowdsourceHinterTest(unittest.TestCase):
         mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '26.0'}
         out = mock_module.get_hint(json_in)
+        print mock_module.previous_answers
         self.assertTrue(out is None)
-        self.assertTrue(['26.0', [None, None, None]] in mock_module.previous_answers)
+        self.assertTrue('26.0' in mock_module.user_submissions)
 
     def test_gethint_unparsable(self):
         """
@@ -359,10 +321,13 @@ class CrowdsourceHinterTest(unittest.TestCase):
         """
         mock_module = CHModuleFactory.create()
         old_answers = copy.deepcopy(mock_module.previous_answers)
+        old_user_submissions = copy.deepcopy(mock_module.user_submissions)
         json_in = {'problem1': 'fish'}
         out = mock_module.get_hint(json_in)
         self.assertTrue(out is None)
         self.assertTrue(mock_module.previous_answers == old_answers)
+        self.assertTrue(mock_module.user_submissions == old_user_submissions)
+
 
     def test_gethint_1hint(self):
         """
@@ -372,7 +337,11 @@ class CrowdsourceHinterTest(unittest.TestCase):
         mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '25.0'}
         out = mock_module.get_hint(json_in)
-        self.assertTrue(out['best_hint'] == 'Really popular hint')
+        self.assertTrue('Really popular hint' in out['hints'])
+        # Also make sure that the input gets added to user_submissions,
+        # and that the hint is logged in previous_answers.
+        self.assertTrue('25.0' in mock_module.user_submissions)
+        self.assertTrue(['25.0', ['1']] in mock_module.previous_answers)
 
     def test_gethint_manyhints(self):
         """
@@ -385,9 +354,8 @@ class CrowdsourceHinterTest(unittest.TestCase):
         mock_module = CHModuleFactory.create()
         json_in = {'problem_name': '24.0'}
         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)
+        self.assertTrue('Best hint' in out['hints'])
+        self.assertTrue(len(out['hints']) == 3)
 
     def test_getfeedback_0wronganswers(self):
         """
@@ -527,8 +495,7 @@ class CrowdsourceHinterTest(unittest.TestCase):
         # Make a hint request.
         json_in = {'problem name': '25.0'}
         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.'))
+        self.assertTrue('This is a new hint.' in out['hints'])
 
     def test_submithint_moderate(self):
         """

lms/djangoapps/instructor/hint_manager.py

@@ -10,7 +10,6 @@ import re
 
 from django.http import HttpResponse, Http404
 from django_future.csrf import ensure_csrf_cookie
-from django.core.exceptions import ObjectDoesNotExist
 
 from mitxmako.shortcuts import render_to_response, render_to_string
 
@@ -114,27 +113,8 @@ def get_hints(request, course_id, field):
                 # Put all non-numerical answers first.
                 return float('-inf')
 
-        # Find the signature to answer converter for this problem.  Sometimes,
-        # it doesn't exist; just assume that the signatures are the answers.
-        try:
-            signature_to_ans = XModuleContentField.objects.get(
-                field_name='signature_to_ans',
-                definition_id__regex=chopped_id
-            )
-            signature_to_ans = json.loads(signature_to_ans.value)
-        except ObjectDoesNotExist:
-            signature_to_ans = {}
-
-        signatures_dict = json.loads(hints_by_problem.value)
-        unsorted = []
-        for signature, dict_of_hints in signatures_dict.items():
-            if signature in signature_to_ans:
-                ans_txt = signature_to_ans[signature]
-            else:   
-                ans_txt = signature
-            unsorted.append([signature, ans_txt, dict_of_hints])
-        # Answer list contains [signature, answer, dict_of_hints] sub-lists.
-        answer_list = sorted(unsorted, key=answer_sorter)
+        # Answer list contains [answer, dict_of_hints] pairs.
+        answer_list = sorted(json.loads(hints_by_problem.value).items(), key=answer_sorter)
         big_out_dict[hints_by_problem.definition_id] = answer_list
 
     render_dict = {'field': field,
@@ -165,7 +145,7 @@ def delete_hints(request, course_id, field):
     Deletes the hints specified.
 
     `request.POST` contains some fields keyed by integers.  Each such field contains a
-    [problem_defn_id, signature, pk] tuple.  These tuples specify the hints to be deleted.
+    [problem_defn_id, answer, pk] tuple.  These tuples specify the hints to be deleted.
 
     Example `request.POST`:
     {'op': 'delete_hints',
@@ -177,12 +157,12 @@ def delete_hints(request, course_id, field):
     for key in request.POST:
         if key == 'op' or key == 'field':
             continue
-        problem_id, signature, pk = request.POST.getlist(key)
+        problem_id, answer, pk = request.POST.getlist(key)
         # Can be optimized - sort the delete list by problem_id, and load each problem
         # from the database only once.
         this_problem = XModuleContentField.objects.get(field_name=field, definition_id=problem_id)
         problem_dict = json.loads(this_problem.value)
-        del problem_dict[signature][pk]
+        del problem_dict[answer][pk]
         this_problem.value = json.dumps(problem_dict)
         this_problem.save()
 
@@ -191,18 +171,18 @@ def change_votes(request, course_id, field):
     """
     Updates the number of votes.
 
-    The numbered fields of `request.POST` contain [problem_id, signature, pk, new_votes] tuples.
+    The numbered fields of `request.POST` contain [problem_id, answer, pk, new_votes] tuples.
     - Very similar to `delete_hints`.  Is there a way to merge them?  Nah, too complicated.
     """
 
     for key in request.POST:
         if key == 'op' or key == 'field':
             continue
-        problem_id, signature, pk, new_votes = request.POST.getlist(key)
+        problem_id, answer, pk, new_votes = request.POST.getlist(key)
         this_problem = XModuleContentField.objects.get(field_name=field, definition_id=problem_id)
         problem_dict = json.loads(this_problem.value)
-        # problem_dict[signature][pk] points to a [hint_text, #votes] pair.
-        problem_dict[signature][pk][1] = int(new_votes)
+        # problem_dict[answer][pk] points to a [hint_text, #votes] pair.
+        problem_dict[answer][pk][1] = int(new_votes)
         this_problem.value = json.dumps(problem_dict)
         this_problem.save()
 
@@ -214,13 +194,24 @@ def add_hint(request, course_id, field):
     field
     problem - The problem id
     answer - The answer to which a hint will be added
-           - Needs to be converted into signature first.
     hint - The text of the hint
     """
 
     problem_id = request.POST['problem']
     answer = request.POST['answer']
     hint_text = request.POST['hint']
+
+    # Validate the answer.  This requires initializing the xmodules, which
+    # is annoying.
+    loc = Location(problem_id)
+    descriptors = modulestore().get_items(loc)
+    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)
+    if not hinter_module.validate_answer(answer):
+        # Invalid answer.  Don't add it to the database, or else the
+        # hinter will crash when we encounter it.
+        return 'Error - the answer you specified is not properly formatted: ' + str(answer)
+
     this_problem = XModuleContentField.objects.get(field_name=field, definition_id=problem_id)
 
     hint_pk_entry = XModuleContentField.objects.get(field_name='hint_pk', definition_id=problem_id)
@@ -228,23 +219,10 @@ def add_hint(request, course_id, field):
     hint_pk_entry.value = this_pk + 1
     hint_pk_entry.save()
 
-    # Make signature.  This is really annoying, but I don't see
-    # any alternative :(
-    loc = Location(problem_id)
-    descriptors = modulestore().get_items(loc)
-    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.
-        # We should probably return an error message, too... working on that.
-        return 'Error - your answer could not be parsed as a formula expression.'
-    hinter_module.add_signature(signature, answer)
-
     problem_dict = json.loads(this_problem.value)
-    if signature not in problem_dict:
-        problem_dict[signature] = {}
-    problem_dict[signature][this_pk] = [hint_text, 1]
+    if answer not in problem_dict:
+        problem_dict[answer] = {}
+    problem_dict[answer][this_pk] = [hint_text, 1]
     this_problem.value = json.dumps(problem_dict)
     this_problem.save()
 
@@ -254,26 +232,26 @@ def approve(request, course_id, field):
     Approve a list of hints, moving them from the mod_queue to the real
     hint list.  POST:
     op, field
-    (some number) -> [problem, signature, pk]
+    (some number) -> [problem, answer, pk]
     """
 
     for key in request.POST:
         if key == 'op' or key == 'field':
             continue
-        problem_id, signature, pk = request.POST.getlist(key)
+        problem_id, answer, pk = request.POST.getlist(key)
         # Can be optimized - sort the delete list by problem_id, and load each problem
         # from the database only once.
         problem_in_mod = XModuleContentField.objects.get(field_name=field, definition_id=problem_id)
         problem_dict = json.loads(problem_in_mod.value)
-        hint_to_move = problem_dict[signature][pk]
-        del problem_dict[signature][pk]
+        hint_to_move = problem_dict[answer][pk]
+        del problem_dict[answer][pk]
         problem_in_mod.value = json.dumps(problem_dict)
         problem_in_mod.save()
 
         problem_in_hints = XModuleContentField.objects.get(field_name='hints', definition_id=problem_id)
         problem_dict = json.loads(problem_in_hints.value)
-        if signature not in problem_dict:
-            problem_dict[signature] = {}
-        problem_dict[signature][pk] = hint_to_move
+        if answer not in problem_dict:
+            problem_dict[answer] = {}
+        problem_dict[answer][pk] = hint_to_move
         problem_in_hints.value = json.dumps(problem_dict)
         problem_in_hints.save()

lms/djangoapps/instructor/tests/test_hint_manager.py

@@ -2,7 +2,7 @@ import json
 
 from django.test.client import Client, RequestFactory
 from django.test.utils import override_settings
-from mock import MagicMock, patch
+from mock import patch, MagicMock
 
 from courseware.models import XModuleContentField
 from courseware.tests.factories import ContentFactory
@@ -12,8 +12,6 @@ from student.tests.factories import UserFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
-import unittest
-
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class HintManagerTest(ModuleStoreTestCase):
@@ -92,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.0', {u'2': [u'Hint 2', 1]}]]}
+        expected = {self.problem_id: [(u'2.0', {u'2': [u'Hint 2', 1]})]}
         self.assertTrue(out['all_hints'] == expected)
 
     def test_gethints_other(self):
@@ -104,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.0', {'1': ['Hint 1', 2],
-                                                      '3': ['Hint 3', 12]}],
-                                      ['2.0', '2.0', {'4': ['Hint 4', 3]}]
+        expected = {self.problem_id: [('1.0', {'1': ['Hint 1', 2],
+                                               '3': ['Hint 3', 12]}),
+                                      ('2.0', {'4': ['Hint 4', 3]})
                                       ]}
         self.assertTrue(out['all_hints'] == expected)
 
@@ -143,7 +141,7 @@ class HintManagerTest(ModuleStoreTestCase):
         # Because add_hint accesses the xmodule, this test requires a bunch
         # of monkey patching.
         hinter = MagicMock()
-        hinter.answer_signature = lambda string: string
+        hinter.validate_answer = lambda string: True
 
         request = RequestFactory()
         post = request.post(self.url, {'field': 'mod_queue',
@@ -158,6 +156,27 @@ class HintManagerTest(ModuleStoreTestCase):
         problem_hints = XModuleContentField.objects.get(field_name='mod_queue', definition_id=self.problem_id).value
         self.assertTrue('3.14' in json.loads(problem_hints))
 
+    def test_addbadhint(self):
+        """
+        Check that instructors cannot add hints with unparsable answers.
+        """
+        # Patching.
+        hinter = MagicMock()
+        hinter.validate_answer = lambda string: False
+
+        request = RequestFactory()
+        post = request.post(self.url, {'field': 'mod_queue',
+                                       'op': 'add hint',
+                                       'problem': self.problem_id,
+                                       'answer': 'fish',
+                                       'hint': 'This is a new hint.'})
+        post.user = 'fake user'
+        with patch('courseware.module_render.get_module', MagicMock(return_value=hinter)):
+            with patch('courseware.model_data.ModelDataCache', MagicMock(return_value=None)):
+                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('fish' not in json.loads(problem_hints))
+
     def test_approve(self):
         """
         Check that instructors can approve hints.  (Move them

lms/templates/instructor/hint_manager_inner.html

@@ -8,12 +8,12 @@ Switch to <a id="switch-fields" other-field="${other_field}">${other_field_label
 
 % for definition_id in all_hints:
     <h2> Problem: ${id_to_name[definition_id]} </h2>
-    % for signature, answer, hint_dict in all_hints[definition_id]:
+    % for answer, hint_dict in all_hints[definition_id]:
         % if len(hint_dict) > 0:
             <h4> Answer: ${answer} </h4><div style="background-color:#EEEEEE">
         % endif
         % for pk, hint in hint_dict.items():
-            <p data-problem="${definition_id}" data-pk="${pk}" data-answer="${signature}">
+            <p data-problem="${definition_id}" data-pk="${pk}" data-answer="${answer}">
                 <input class="hint-select" type="checkbox"/> ${hint[0]}
                 <br /> 
                 Votes: <input type="text" class="votes" value="${str(hint[1])}" style="font-size:12px; height:20px; width:50px"></input>