From d9517ea13e2cbb63f4c2203186bb108a3074e78f Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 18 Jul 2013 15:02:19 -0400 Subject: [PATCH] Fixed tests for removing hash access to hints. Fixed instructor view for same. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 13 ++- .../xmodule/tests/test_crowdsource_hinter.py | 109 ++++++------------ lms/djangoapps/instructor/hint_manager.py | 84 +++++--------- .../instructor/tests/test_hint_manager.py | 35 ++++-- .../instructor/hint_manager_inner.html | 4 +- 5 files changed, 105 insertions(+), 140 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 37f244cdba..ebb07506cb 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/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, diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index cc0634cb7d..15939f2679 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/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): """ diff --git a/lms/djangoapps/instructor/hint_manager.py b/lms/djangoapps/instructor/hint_manager.py index 1f0d17ab03..3616445543 100644 --- a/lms/djangoapps/instructor/hint_manager.py +++ b/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() diff --git a/lms/djangoapps/instructor/tests/test_hint_manager.py b/lms/djangoapps/instructor/tests/test_hint_manager.py index e9387a8eef..fae2e48bb4 100644 --- a/lms/djangoapps/instructor/tests/test_hint_manager.py +++ b/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 diff --git a/lms/templates/instructor/hint_manager_inner.html b/lms/templates/instructor/hint_manager_inner.html index 3acf8102ac..45101be2f6 100644 --- a/lms/templates/instructor/hint_manager_inner.html +++ b/lms/templates/instructor/hint_manager_inner.html @@ -8,12 +8,12 @@ Switch to ${other_field_label % for definition_id in all_hints:

Problem: ${id_to_name[definition_id]}

- % for signature, answer, hint_dict in all_hints[definition_id]: + % for answer, hint_dict in all_hints[definition_id]: % if len(hint_dict) > 0:

Answer: ${answer}

% endif % for pk, hint in hint_dict.items(): -

+

${hint[0]}
Votes: