diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 5c5c869088..b82241bd08 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee index 2e4d0e2c3b..db7ccfaba0 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/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) diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index b8a205e90e..844c73a990 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/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) diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 3ca1ce5330..b13dc83ea2 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -18,11 +18,6 @@ <%def name="get_feedback()"> -

Participation in the hinting system is strictly optional, and will not influence your grade.

-

- Help your classmates by writing hints for this problem. Start by picking one of your previous incorrect answers from below: -

- <% 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: +

+ Optional. Help us improve our hints! Which hint was most helpful to you? +

+ + + + % for answer, pk_dict in answer_to_hints.items(): + % for hint_pk, hint_text in pk_dict.items(): +

+ + ${answer}: ${hint_text} +

+ % endfor + % endfor + +

+ Don't like any of the hints above? Please write your own, for one of your previous incorrect answers below: +

+ % else: +

+ Optional. Help us write hints for this problem! Select one of your incorrect answers below: +

+ % endif
% for answer, pk_dict in answer_to_hints.items(): - <% - import json - all_pks = json.dumps(pk_dict.keys()) - %> -
-
- % if len(pk_dict) > 0: + <% + import json + all_pks = json.dumps(pk_dict.keys()) + %> +
+

- Which hint would be most effective to show a student who also got ${answer}? + What hint would you give a student who made the same mistake you did? Please don't give away the answer.

- % for hint_pk, hint_text in pk_dict.items(): -

- - ${hint_text} -

- % endfor -

- Don't like any of the hints above? You can also submit your own. -

- % endif -

- What hint would you give a student who made the same mistake you did? Please don't give away the answer. -

- -

- -
+ +

+ +
% endfor
diff --git a/lms/djangoapps/instructor/hint_manager.py b/lms/djangoapps/instructor/hint_manager.py index b20cdc81e6..1f0d17ab03 100644 --- a/lms/djangoapps/instructor/hint_manager.py +++ b/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. diff --git a/lms/djangoapps/instructor/tests/test_hint_manager.py b/lms/djangoapps/instructor/tests/test_hint_manager.py index 456f8e0ed8..9e579ab997 100644 --- a/lms/djangoapps/instructor/tests/test_hint_manager.py +++ b/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