From fc8acb61a33974ca022349fac8f682bb355392dc Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Fri, 28 Jun 2013 12:02:52 -0400 Subject: [PATCH 01/28] Changed voting dialog to only display one tab per answer. No tests for this, yet. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py common/templates/hinter_display.html --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 41 +++++++++---------- .../js/src/crowdsource_hinter/display.coffee | 13 ++++-- common/templates/hinter_display.html | 22 ++++++---- 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 7c7f94a26b..734708ec04 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -188,8 +188,8 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Args: `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. + - 'answer_to_hints': a nested dictionary. + answer_to_hints[answer][hint_pk] returns the text of the hint. """ # The student got it right. # Did he submit at least one wrong answer? @@ -199,27 +199,24 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Make a hint-voting interface for each wrong answer. The student will only # be allowed to make one vote / submission, but he can choose which wrong answer # he wants to look at. - # index_to_hints[previous answer #] = [(hint text, hint pk), + ] - index_to_hints = {} - # index_to_answer[previous answer #] = answer text - index_to_answer = {} + answer_to_hints = {} #answer_to_hints[answer text][hint pk] -> hint text # 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] - index_to_hints[i] = [] - index_to_answer[i] = answer + if answer not in answer_to_hints: + answer_to_hints[answer] = {} if answer in self.hints: # Go through each hint, and add to index_to_hints for hint_id in hints_offered: - if hint_id is not None: + if (hint_id is not None) and (hint_id not in answer_to_hints[answer]): try: - index_to_hints[i].append((self.hints[answer][str(hint_id)][0], hint_id)) + answer_to_hints[answer][hint_id] = self.hints[answer][str(hint_id)][0] except KeyError: # Sometimes, the hint that a user saw will have been deleted by the instructor. continue - return {'index_to_hints': index_to_hints, 'index_to_answer': index_to_answer} + return {'answer_to_hints': answer_to_hints} def tally_vote(self, data): """ @@ -229,27 +226,29 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): `data` -- expected to have the following keys: 'answer': ans_no (index in previous_answers) 'hint': hint_pk + 'pk_list': We will return a list of how many votes each hint 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: - return {} - ans_no = int(data['answer']) - hint_no = str(data['hint']) - answer = self.previous_answers[ans_no][0] + return json.dumps({'contents': 'Sorry, but you have already voted!'}) + ans = data['answer'] + 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 - temp_dict[answer][hint_no][1] += 1 + temp_dict[ans][hint_pk][1] += 1 self.hints = temp_dict # Don't let the user vote again! self.user_voted = True # Return a list of how many votes each hint got. hint_and_votes = [] - for hint_no in self.previous_answers[ans_no][1]: - if hint_no is None: - continue - hint_and_votes.append(temp_dict[answer][str(hint_no)]) + for vote_pk in pk_list: + hint_and_votes.append(temp_dict[ans][vote_pk]) + hint_and_votes.sort(key=lambda pair: pair[1], reverse=True) # Reset self.previous_answers. self.previous_answers = [] return {'hint_and_votes': hint_and_votes} @@ -266,7 +265,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): """ # Do html escaping. Perhaps in the future do profanity filtering, etc. as well. hint = escape(data['hint']) - answer = self.previous_answers[int(data['answer'])][0] + answer = data['answer'] # Only allow a student to vote or submit a hint once. if self.user_voted: return {'message': 'Sorry, but you have already voted!'} 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 72522f5b03..3df970a618 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -28,6 +28,10 @@ class @Hinter $: (selector) -> $(selector, @el) + jq_escape: (string) => + # Escape a string for jquery selector use. + return string.replace(/[!"#$%&'()*+,.\/:;<=>?@\[\\\]^`{|}~]/g, '\\$&') + bind: => window.update_schematics() @$('input.vote').click @vote @@ -44,14 +48,17 @@ class @Hinter vote: (eventObj) => target = @$(eventObj.currentTarget) - post_json = {'answer': target.data('answer'), 'hint': target.data('hintno')} + parent_div_selector = '#previous-answer-' + @jq_escape(target.attr('data-answer')) + all_pks = @$(parent_div_selector).attr('data-all-pks') + console.debug(all_pks) + post_json = {'answer': target.attr('data-answer'), 'hint': target.data('hintno'), 'pk_list': all_pks} $.postWithPrefix "#{@url}/vote", post_json, (response) => @render(response.contents) submit_hint: (eventObj) => target = @$(eventObj.currentTarget) - textarea_id = '#custom-hint-' + target.data('answer') - post_json = {'answer': target.data('answer'), 'hint': @$(textarea_id).val()} + textarea_id = '#custom-hint-' + @jq_escape(target.attr('data-answer')) + post_json = {'answer': target.attr('data-answer'), 'hint': @$(textarea_id).val()} $.postWithPrefix "#{@url}/submit_hint",post_json, (response) => @render(response.contents) diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 6f5d6f37fb..d349a17fc5 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -25,21 +25,25 @@
- % for index, answer in index_to_answer.items(): -
+ % for answer, pk_dict in answer_to_hints.items(): + <% + import json + all_pks = json.dumps(pk_dict.keys()) + %> +
- % if index in index_to_hints and len(index_to_hints[index]) > 0: + % if len(pk_dict) > 0:

Which hint would be most effective to show a student who also got ${answer}?

- % for hint_text, hint_pk in index_to_hints[index]: + % for hint_pk, hint_text in pk_dict.items():

- + ${hint_text}

% endfor @@ -50,12 +54,12 @@

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

-

- +
% endfor
From 69380756d2a45e1147bba360377b9ac6f5f0395c Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Fri, 28 Jun 2013 13:49:25 -0400 Subject: [PATCH 02/28] Fixed tests to work with new vote-tabbing method. Fixed some string-int incompatibility errors. Conflicts: common/lib/xmodule/xmodule/crowdsource_hinter.py common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 12 ++++---- .../xmodule/tests/test_crowdsource_hinter.py | 28 ++++++++----------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 734708ec04..f0f36e7ef8 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -199,7 +199,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Make a hint-voting interface for each wrong answer. The student will only # 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 + answer_to_hints = {} # answer_to_hints[answer text][hint pk] -> hint text # Go through each previous answer, and populate index_to_hints and index_to_answer. for i in xrange(len(self.previous_answers)): @@ -224,7 +224,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Args: `data` -- expected to have the following keys: - 'answer': ans_no (index in previous_answers) + '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. It's up to the browser to specify which hints to return vote counts for. @@ -246,7 +246,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Return a list of how many votes each hint got. hint_and_votes = [] for vote_pk in pk_list: - hint_and_votes.append(temp_dict[ans][vote_pk]) + hint_and_votes.append(temp_dict[ans][str(vote_pk)]) hint_and_votes.sort(key=lambda pair: pair[1], reverse=True) # Reset self.previous_answers. @@ -259,7 +259,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Args: `data` -- expected to have the following keys: - 'answer': answer index in previous_answers + 'answer': text of answer 'hint': text of the new hint that the user is adding Returns a thank-you message. """ @@ -276,9 +276,9 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): else: temp_dict = self.hints if answer in temp_dict: - temp_dict[answer][self.hint_pk] = [hint, 1] # With one vote (the user himself). + temp_dict[answer][str(self.hint_pk)] = [hint, 1] # With one vote (the user himself). else: - temp_dict[answer] = {self.hint_pk: [hint, 1]} + temp_dict[answer] = {str(self.hint_pk): [hint, 1]} self.hint_pk += 1 if self.moderate == 'True': self.mod_queue = temp_dict diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 19e156a0f3..989b264694 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -294,9 +294,7 @@ class CrowdsourceHinterTest(unittest.TestCase): mock_module = CHModuleFactory.create(previous_answers=[['26.0', [None, None, None]]]) json_in = {'problem_name': '42.5'} 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') + self.assertTrue(out['answer_to_hints'] == {'26.0': {}}) def test_getfeedback_1wronganswer_withhints(self): """ @@ -307,8 +305,7 @@ class CrowdsourceHinterTest(unittest.TestCase): mock_module = CHModuleFactory.create(previous_answers=[['24.0', [0, 3, None]]]) json_in = {'problem_name': '42.5'} out = mock_module.get_feedback(json_in) - print out['index_to_hints'] - self.assertTrue(len(out['index_to_hints'][0]) == 2) + self.assertTrue(len(out['answer_to_hints']['24.0']) == 2) def test_getfeedback_missingkey(self): """ @@ -319,7 +316,7 @@ class CrowdsourceHinterTest(unittest.TestCase): previous_answers=[['24.0', [0, 100, None]]]) json_in = {'problem_name': '42.5'} out = mock_module.get_feedback(json_in) - self.assertTrue(len(out['index_to_hints'][0]) == 1) + self.assertTrue(len(out['answer_to_hints']['24.0']) == 1) def test_vote_nopermission(self): """ @@ -327,7 +324,7 @@ class CrowdsourceHinterTest(unittest.TestCase): Should not change any vote tallies. """ mock_module = CHModuleFactory.create(user_voted=True) - json_in = {'answer': 0, 'hint': 1} + json_in = {'answer': '24.0', 'hint': 1, 'pk_list': json.dumps([1, 3])} old_hints = copy.deepcopy(mock_module.hints) mock_module.tally_vote(json_in) self.assertTrue(mock_module.hints == old_hints) @@ -339,7 +336,7 @@ class CrowdsourceHinterTest(unittest.TestCase): """ mock_module = CHModuleFactory.create( previous_answers=[['24.0', [0, 3, None]]]) - json_in = {'answer': 0, 'hint': 3} + json_in = {'answer': '24.0', 'hint': 3, 'pk_list': json.dumps([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) @@ -351,7 +348,7 @@ class CrowdsourceHinterTest(unittest.TestCase): A user tries to submit a hint, but he has already voted. """ mock_module = CHModuleFactory.create(user_voted=True) - json_in = {'answer': 1, 'hint': 'This is a new hint.'} + json_in = {'answer': '29.0', 'hint': 'This is a new hint.'} print mock_module.user_voted mock_module.submit_hint(json_in) print mock_module.hints @@ -363,7 +360,7 @@ class CrowdsourceHinterTest(unittest.TestCase): exist yet. """ mock_module = CHModuleFactory.create() - json_in = {'answer': 1, 'hint': 'This is a new hint.'} + json_in = {'answer': '29.0', 'hint': 'This is a new hint.'} mock_module.submit_hint(json_in) self.assertTrue('29.0' in mock_module.hints) @@ -372,8 +369,8 @@ class CrowdsourceHinterTest(unittest.TestCase): A user submits a hint to an answer that has other hints already. """ - mock_module = CHModuleFactory.create(previous_answers=[['25.0', [1, None, None]]]) - json_in = {'answer': 0, 'hint': 'This is a new hint.'} + mock_module = CHModuleFactory.create(previous_answers = [['25.0', [1, None, None]]]) + json_in = {'answer': '25.0', 'hint': 'This is a new hint.'} mock_module.submit_hint(json_in) # Make a hint request. json_in = {'problem name': '25.0'} @@ -388,7 +385,7 @@ class CrowdsourceHinterTest(unittest.TestCase): dict. """ mock_module = CHModuleFactory.create(moderate='True') - json_in = {'answer': 1, 'hint': 'This is a new hint.'} + json_in = {'answer': '29.0', 'hint': 'This is a new hint.'} mock_module.submit_hint(json_in) self.assertTrue('29.0' not in mock_module.hints) self.assertTrue('29.0' in mock_module.mod_queue) @@ -398,10 +395,9 @@ class CrowdsourceHinterTest(unittest.TestCase): Make sure that hints are being html-escaped. """ mock_module = CHModuleFactory.create() - json_in = {'answer': 1, 'hint': ''} + json_in = {'answer': '29.0', 'hint': ''} mock_module.submit_hint(json_in) - print mock_module.hints - self.assertTrue(mock_module.hints['29.0'][0][0] == u'<script> alert("Trololo"); </script>') + self.assertTrue(mock_module.hints['29.0']['0'][0] == u'<script> alert("Trololo"); </script>') def test_template_gethint(self): """ From c34a81a845b72b7c7e2f714ac9300682c4a81534 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Wed, 10 Jul 2013 13:11:50 -0400 Subject: [PATCH 03/28] Refactored formula response grader to expose formula-evaluating to outside world. In preparation for adding hints to formula response. --- common/lib/capa/capa/responsetypes.py | 88 ++++++++++++++------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 731230ecc1..c3d680ee20 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1778,46 +1778,24 @@ class FormulaResponse(LoncapaResponse): self.correct_answer, given, self.samples) return CorrectMap(self.answer_id, correctness) - def check_formula(self, expected, given, samples): - variables = samples.split('@')[0].split(',') - numsamples = int(samples.split('@')[1].split('#')[1]) - sranges = zip(*map(lambda x: map(float, x.split(",")), - samples.split('@')[1].split('#')[0].split(':'))) - - ranges = dict(zip(variables, sranges)) - for _ in range(numsamples): - instructor_variables = self.strip_dict(dict(self.context)) - student_variables = {} - # ranges give numerical ranges for testing - for var in ranges: - # TODO: allow specified ranges (i.e. integers and complex numbers) for random variables - value = random.uniform(*ranges[var]) - instructor_variables[str(var)] = value - student_variables[str(var)] = value - # log.debug('formula: instructor_vars=%s, expected=%s' % - # (instructor_variables,expected)) - - # Call `evaluator` on the instructor's answer and get a number - instructor_result = evaluator( - instructor_variables, {}, - expected, case_sensitive=self.case_sensitive - ) + def hash_answers(self, answer, var_dict_list): + """ + Takes in an answer and a list of dictionaries mapping variables to values. + Each dictionary represents a test case for the answer. + Returns a tuple of formula evaluation results. + """ + out = [] + for var_dict in var_dict_list: try: - # log.debug('formula: student_vars=%s, given=%s' % - # (student_variables,given)) - - # Call `evaluator` on the student's answer; look for exceptions - student_result = evaluator( - student_variables, - {}, - given, - case_sensitive=self.case_sensitive - ) + out.append(evaluator( + var_dict, + dict(), + answer, + cs=self.case_sensitive, + )) except UndefinedVariable as uv: log.debug( - 'formularesponse: undefined variable in given=%s', - given - ) + 'formularesponse: undefined variable in formula=%s' % answer) raise StudentInputError( "Invalid input: " + uv.message + " not permitted in answer" ) @@ -1840,15 +1818,43 @@ class FormulaResponse(LoncapaResponse): # If non-factorial related ValueError thrown, handle it the same as any other Exception log.debug('formularesponse: error {0} in formula'.format(ve)) raise StudentInputError("Invalid input: Could not parse '%s' as a formula" % - cgi.escape(given)) + cgi.escape(answer)) except Exception as err: # traceback.print_exc() log.debug('formularesponse: error %s in formula', err) raise StudentInputError("Invalid input: Could not parse '%s' as a formula" % - cgi.escape(given)) + cgi.escape(answer)) + return tuple(out) - # No errors in student's response--actually test for correctness - if not compare_with_tolerance(student_result, instructor_result, self.tolerance): + def randomize_variables(self, samples): + """ + Returns a list of dictionaries mapping variables to random values in range, + as expected by hash_answers. + """ + variables = samples.split('@')[0].split(',') + numsamples = int(samples.split('@')[1].split('#')[1]) + sranges = zip(*map(lambda x: map(float, x.split(",")), + samples.split('@')[1].split('#')[0].split(':'))) + ranges = dict(zip(variables, sranges)) + + out = [] + for i in range(numsamples): + var_dict = {} + # ranges give numerical ranges for testing + for var in ranges: + # TODO: allow specified ranges (i.e. integers and complex numbers) for random variables + value = random.uniform(*ranges[var]) + var_dict[str(var)] = value + out.append(var_dict) + return out + + def check_formula(self, expected, given, samples): + var_dict_list = self.randomize_variables(samples) + student_result = self.hash_answers(given, var_dict_list) + instructor_result = self.hash_answers(expected, var_dict_list) + + for i in xrange(len(instructor_result)): + if not compare_with_tolerance(student_result[i], instructor_result[i], self.tolerance): return "incorrect" return "correct" From b88c6b8d68fc99207ab640304ec816a022e0bbe5 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 11 Jul 2013 11:12:12 -0400 Subject: [PATCH 04/28] Hinter now works with formula responses. Tests broken. --- common/lib/capa/capa/responsetypes.py | 2 +- .../lib/xmodule/xmodule/crowdsource_hinter.py | 102 ++++++++++++++---- 2 files changed, 82 insertions(+), 22 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index c3d680ee20..47289aad51 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1824,7 +1824,7 @@ class FormulaResponse(LoncapaResponse): log.debug('formularesponse: error %s in formula', err) raise StudentInputError("Invalid input: Could not parse '%s' as a formula" % cgi.escape(answer)) - return tuple(out) + return out def randomize_variables(self, samples): """ diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index f0f36e7ef8..6c868cb621 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -16,6 +16,8 @@ from xmodule.x_module import XModule from xmodule.xml_module import XmlDescriptor from xblock.core import Scope, String, Integer, Boolean, Dict, List +from capa.responsetypes import FormulaResponse, StudentInputError + from django.utils.html import escape log = logging.getLogger(__name__) @@ -37,6 +39,14 @@ class CrowdsourceHinterFields(object): mod_queue = Dict(help='A dictionary containing hints still awaiting approval', scope=Scope.content, default={}) hint_pk = Integer(help='Used to index hints.', scope=Scope.content, default=0) + # signature_to_ans maps an answer signature to an answer string that shows that answer in a + # human-readable form. + signature_to_ans = Dict(help='Maps a signature to a representative formula.', scope=Scope.content, + default={}) + # A list of dictionaries, each of which represents an n-dimenstional point that we plug into + # formulas. Each dictionary maps variables to values, eg {'x': 5.1}. + formula_test_values = List(help='The values that we plug into formula responses', scope=Scope.content, + default=[]) # A list of previous answers this student made to this problem. # Of the form [answer, [hint_pk_1, hint_pk_2, hint_pk_3]] for each problem. hint_pk's are # None if the hint was not given. @@ -68,6 +78,15 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): def __init__(self, *args, **kwargs): XModule.__init__(self, *args, **kwargs) + # We need to know whether we are working with a FormulaResponse problem. + self.is_formula = (type(self.get_display_items()[0].lcp.responders.values()[0]) == FormulaResponse) + if self.is_formula: + self.answer_to_str = self.formula_answer_to_str + 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 def get_html(self): """ @@ -98,15 +117,45 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): return out - def capa_answer_to_str(self, answer): + def numerical_answer_to_str(self, answer): """ - Converts capa answer format to a string representation + Converts capa numerical answer format to a string representation of the answer. -Lon-capa dependent. -Assumes that the problem only has one part. """ return str(float(answer.values()[0])) + def formula_answer_to_str(self, answer): + """ + Converts capa formula answer into a string. + -Lon-capa dependent. + -Assumes that the problem only has one part. + """ + return str(answer.values()[0]) + + def formula_answer_signature(self, answer): + """ + Converts a capa answer string (output of formula_answer_to_str) + to a string unique to each formula equality class. + So, x^2 and x*x would have the same signature, which would differ + from the signature of 2*x^2. + """ + responder = self.get_display_items()[0].lcp.responders.values()[0] + if self.formula_test_values == []: + # Make a set of test values, and save them. + self.formula_test_values = responder.randomize_variables(responder.samples) + try: + # TODO, maybe: add some rounding to signature generation, so that floating point + # errors don't make a difference. + out = str(responder.hash_answers(answer, self.formula_test_values)) + except StudentInputError: + # I'm not sure what's the best thing to do here. + # I'll return the empty string, for now. + # That way, all invalid hints are clustered together. + return '' + return out + def handle_ajax(self, dispatch, data): """ This is the landing method for AJAX calls. @@ -134,44 +183,46 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Called by hinter javascript after a problem is graded as incorrect. Args: - `data` -- must be interpretable by capa_answer_to_str. + `data` -- must be interpretable by 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 `data`. - 'answer' is the parsed answer that was submitted. """ try: - answer = self.capa_answer_to_str(data) + answer = self.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 + # Make a signature of the answer, for formula responses. + signature = self.answer_signature(answer) # 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.) local_hints = self.hints - if (answer not in local_hints) or (len(local_hints[answer]) == 0): + if (signature not in local_hints) or (len(local_hints[signature]) == 0): # No hints to give. Return. self.previous_answers += [[answer, [None, None, None]]] return # Get the top hint, plus two random hints. - n_hints = len(local_hints[answer]) - best_hint_index = max(local_hints[answer], key=lambda key: local_hints[answer][key][1]) - best_hint = local_hints[answer][best_hint_index][0] - if len(local_hints[answer]) == 1: + n_hints = len(local_hints[signature]) + best_hint_index = max(local_hints[signature], key=lambda key: local_hints[signature][key][1]) + best_hint = local_hints[signature][best_hint_index][0] + if len(local_hints[signature]) == 1: rand_hint_1 = '' rand_hint_2 = '' self.previous_answers += [[answer, [best_hint_index, None, None]]] elif n_hints == 2: - best_hint = local_hints[answer].values()[0][0] - best_hint_index = local_hints[answer].keys()[0] - rand_hint_1 = local_hints[answer].values()[1][0] - hint_index_1 = local_hints[answer].keys()[1] + best_hint = local_hints[signature].values()[0][0] + best_hint_index = local_hints[signature].keys()[0] + rand_hint_1 = local_hints[signature].values()[1][0] + hint_index_1 = local_hints[signature].keys()[1] rand_hint_2 = '' self.previous_answers += [[answer, [best_hint_index, hint_index_1, None]]] else: (hint_index_1, rand_hint_1), (hint_index_2, rand_hint_2) =\ - random.sample(local_hints[answer].items(), 2) + random.sample(local_hints[signature].items(), 2) rand_hint_1 = rand_hint_1[0] rand_hint_2 = rand_hint_2[0] self.previous_answers += [[answer, [best_hint_index, hint_index_1, hint_index_2]]] @@ -206,12 +257,13 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): answer, hints_offered = self.previous_answers[i] if answer not in answer_to_hints: answer_to_hints[answer] = {} - if answer in self.hints: + 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: - if (hint_id is not None) and (hint_id not in answer_to_hints[answer]): + if (hint_id is not None) and (hint_id not in answer_to_hints[signature]): try: - answer_to_hints[answer][hint_id] = self.hints[answer][str(hint_id)][0] + answer_to_hints[answer][hint_id] = self.hints[signature][str(hint_id)][0] except KeyError: # Sometimes, the hint that a user saw will have been deleted by the instructor. continue @@ -234,11 +286,12 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if self.user_voted: return json.dumps({'contents': 'Sorry, but you have already voted!'}) ans = data['answer'] + signature = self.answer_signature(ans) 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 - temp_dict[ans][hint_pk][1] += 1 + temp_dict[signature][hint_pk][1] += 1 self.hints = temp_dict # Don't let the user vote again! self.user_voted = True @@ -246,7 +299,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Return a list of how many votes each hint got. hint_and_votes = [] for vote_pk in pk_list: - hint_and_votes.append(temp_dict[ans][str(vote_pk)]) + hint_and_votes.append(temp_dict[signature][str(vote_pk)]) hint_and_votes.sort(key=lambda pair: pair[1], reverse=True) # Reset self.previous_answers. @@ -266,6 +319,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Do html escaping. Perhaps in the future do profanity filtering, etc. as well. hint = escape(data['hint']) answer = data['answer'] + signature = self.answer_signature(answer) # Only allow a student to vote or submit a hint once. if self.user_voted: return {'message': 'Sorry, but you have already voted!'} @@ -276,9 +330,15 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): else: temp_dict = self.hints if answer in temp_dict: - temp_dict[answer][str(self.hint_pk)] = [hint, 1] # With one vote (the user himself). + temp_dict[signature][str(self.hint_pk)] = [hint, 1] # With one vote (the user himself). else: - temp_dict[answer] = {str(self.hint_pk): [hint, 1]} + temp_dict[signature] = {str(self.hint_pk): [hint, 1]} + # Add the signature to signature_to_ans, if it's not there yet. + # This allows instructors to see a human-readable answer that corresponds to each signature. + if answer not in self.signature_to_ans: + local_sta = self.signature_to_ans + local_sta[signature] = answer + self.signature_to_ans = local_sta self.hint_pk += 1 if self.moderate == 'True': self.mod_queue = temp_dict From 199b6325137e7cef6f36e1be3c9113680b5d0eb1 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 11 Jul 2013 17:15:01 -0400 Subject: [PATCH 05/28] Crowdsourced hinter now supports formula responses. Tests still broken. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 33 ++++-- .../js/src/crowdsource_hinter/display.coffee | 9 +- common/templates/hinter_display.html | 15 ++- lms/djangoapps/instructor/hint_manager.py | 103 ++++++++++++------ .../hint_manager.html | 2 +- .../hint_manager_inner.html | 6 +- 6 files changed, 116 insertions(+), 52 deletions(-) rename lms/templates/{courseware => instructor}/hint_manager.html (98%) rename lms/templates/{courseware => instructor}/hint_manager_inner.html (90%) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 6c868cb621..bac6982085 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -13,7 +13,7 @@ from pkg_resources import resource_string from lxml import etree from xmodule.x_module import XModule -from xmodule.xml_module import XmlDescriptor +from xmodule.raw_module import RawDescriptor from xblock.core import Scope, String, Integer, Boolean, Dict, List from capa.responsetypes import FormulaResponse, StudentInputError @@ -150,10 +150,10 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # errors don't make a difference. out = str(responder.hash_answers(answer, self.formula_test_values)) except StudentInputError: - # I'm not sure what's the best thing to do here. - # I'll return the empty string, for now. - # That way, all invalid hints are clustered together. - return '' + # I'm not sure what's the best thing to do here. I'm returning + # None, for now, so that the calling function has a chance to catch + # the error without having to import StudentInputError. + return None return out def handle_ajax(self, dispatch, data): @@ -197,6 +197,10 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): return # Make a signature of the answer, for formula responses. signature = self.answer_signature(answer) + if signature == None: + # Sometimes, signature conversion may fail. + log.exception('Signature conversion failed: ' + str(answer)) + 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.) @@ -261,7 +265,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if signature in self.hints: # Go through each hint, and add to index_to_hints for hint_id in hints_offered: - if (hint_id is not None) and (hint_id not in answer_to_hints[signature]): + if (hint_id is not None) and (hint_id not in answer_to_hints[answer]): try: answer_to_hints[answer][hint_id] = self.hints[signature][str(hint_id)][0] except KeyError: @@ -335,10 +339,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): temp_dict[signature] = {str(self.hint_pk): [hint, 1]} # Add the signature to signature_to_ans, if it's not there yet. # This allows instructors to see a human-readable answer that corresponds to each signature. - if answer not in self.signature_to_ans: - local_sta = self.signature_to_ans - local_sta[signature] = answer - self.signature_to_ans = local_sta + self.add_signature(signature, answer) self.hint_pk += 1 if self.moderate == 'True': self.mod_queue = temp_dict @@ -349,8 +350,18 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): self.previous_answers = [] return {'message': 'Thank you for your hint!'} + def add_signature(self, signature, answer): + """ + Add a signature to self.signature_to_ans. If the signature already + exists, do nothing. + """ + if signature not in self.signature_to_ans: + local_sta = self.signature_to_ans + local_sta[signature] = answer + self.signature_to_ans = local_sta -class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, XmlDescriptor): + +class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, RawDescriptor): module_class = CrowdsourceHinterModule stores_state = True 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 3df970a618..2e4d0e2c3b 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -48,8 +48,8 @@ class @Hinter vote: (eventObj) => target = @$(eventObj.currentTarget) - parent_div_selector = '#previous-answer-' + @jq_escape(target.attr('data-answer')) - all_pks = @$(parent_div_selector).attr('data-all-pks') + parent_div = $('.previous-answer[data-answer="'+target.attr('data-answer')+'"]') + all_pks = parent_div.attr('data-all-pks') console.debug(all_pks) post_json = {'answer': target.attr('data-answer'), 'hint': target.data('hintno'), 'pk_list': all_pks} $.postWithPrefix "#{@url}/vote", post_json, (response) => @@ -57,8 +57,9 @@ class @Hinter submit_hint: (eventObj) => target = @$(eventObj.currentTarget) - textarea_id = '#custom-hint-' + @jq_escape(target.attr('data-answer')) - post_json = {'answer': target.attr('data-answer'), 'hint': @$(textarea_id).val()} + 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/templates/hinter_display.html b/common/templates/hinter_display.html index d349a17fc5..4f40113265 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -23,10 +23,19 @@ Help your classmates by writing hints for this problem. Start by picking one of your previous incorrect answers from below:

+ <% + def unspace(string): + """ + HTML id's can't have spaces in them. This little function + removes spaces. + """ + return ''.join(string.split()) + %> +
@@ -35,7 +44,7 @@ import json all_pks = json.dumps(pk_dict.keys()) %> -
+
% if len(pk_dict) > 0:

@@ -54,7 +63,7 @@

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

- diff --git a/lms/djangoapps/instructor/hint_manager.py b/lms/djangoapps/instructor/hint_manager.py index 899e386f70..b20cdc81e6 100644 --- a/lms/djangoapps/instructor/hint_manager.py +++ b/lms/djangoapps/instructor/hint_manager.py @@ -10,11 +10,14 @@ 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 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 from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore @@ -28,24 +31,29 @@ def hint_manager(request, course_id): return HttpResponse(out) if request.method == 'GET': out = get_hints(request, course_id, 'mod_queue') - return render_to_response('courseware/hint_manager.html', out) + out.update({'error': ''}) + return render_to_response('instructor/hint_manager.html', out) field = request.POST['field'] if not (field == 'mod_queue' or field == 'hints'): # Invalid field. (Don't let users continue - they may overwrite other db's) out = 'Error in hint manager - an invalid field was accessed.' return HttpResponse(out) - if request.POST['op'] == 'delete hints': - delete_hints(request, course_id, field) - if request.POST['op'] == 'switch fields': - pass - if request.POST['op'] == 'change votes': - change_votes(request, course_id, field) - if request.POST['op'] == 'add hint': - add_hint(request, course_id, field) - if request.POST['op'] == 'approve': - approve(request, course_id, field) - rendered_html = render_to_string('courseware/hint_manager_inner.html', get_hints(request, course_id, field)) + switch_dict = { + 'delete hints': delete_hints, + 'switch fields': lambda *args: None, # Takes any number of arguments, returns None. + 'change votes': change_votes, + 'add hint': add_hint, + 'approve': approve, + } + + # Do the operation requested, and collect any error messages. + error_text = switch_dict[request.POST['op']](request, course_id, field) + if error_text is None: + error_text = '' + render_dict = get_hints(request, course_id, field) + render_dict.update({'error': error_text}) + rendered_html = render_to_string('instructor/hint_manager_inner.html', render_dict) return HttpResponse(json.dumps({'success': True, 'contents': rendered_html})) @@ -106,8 +114,27 @@ def get_hints(request, course_id, field): # Put all non-numerical answers first. return float('-inf') - # Answer list contains [answer, dict_of_hints] pairs. - answer_list = sorted(json.loads(hints_by_problem.value).items(), key=answer_sorter) + # 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) big_out_dict[hints_by_problem.definition_id] = answer_list render_dict = {'field': field, @@ -138,7 +165,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, answer, pk] tuple. These tuples specify the hints to be deleted. + [problem_defn_id, signature, pk] tuple. These tuples specify the hints to be deleted. Example `request.POST`: {'op': 'delete_hints', @@ -150,12 +177,12 @@ def delete_hints(request, course_id, field): for key in request.POST: if key == 'op' or key == 'field': continue - problem_id, answer, pk = request.POST.getlist(key) + problem_id, signature, 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[answer][pk] + del problem_dict[signature][pk] this_problem.value = json.dumps(problem_dict) this_problem.save() @@ -164,18 +191,18 @@ def change_votes(request, course_id, field): """ Updates the number of votes. - The numbered fields of `request.POST` contain [problem_id, answer, pk, new_votes] tuples. + The numbered fields of `request.POST` contain [problem_id, signature, 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, answer, pk, new_votes = request.POST.getlist(key) + problem_id, signature, 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[answer][pk] points to a [hint_text, #votes] pair. - problem_dict[answer][pk][1] = int(new_votes) + # problem_dict[signature][pk] points to a [hint_text, #votes] pair. + problem_dict[signature][pk][1] = int(new_votes) this_problem.value = json.dumps(problem_dict) this_problem.save() @@ -187,6 +214,7 @@ 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 """ @@ -200,10 +228,23 @@ 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 = ModelDataCache(descriptors, course_id, request.user) + hinter_module = 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 answer not in problem_dict: - problem_dict[answer] = {} - problem_dict[answer][this_pk] = [hint_text, 1] + if signature not in problem_dict: + problem_dict[signature] = {} + problem_dict[signature][this_pk] = [hint_text, 1] this_problem.value = json.dumps(problem_dict) this_problem.save() @@ -213,26 +254,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, answer, pk] + (some number) -> [problem, signature, pk] """ for key in request.POST: if key == 'op' or key == 'field': continue - problem_id, answer, pk = request.POST.getlist(key) + problem_id, signature, 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[answer][pk] - del problem_dict[answer][pk] + hint_to_move = problem_dict[signature][pk] + del problem_dict[signature][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 answer not in problem_dict: - problem_dict[answer] = {} - problem_dict[answer][pk] = hint_to_move + if signature not in problem_dict: + problem_dict[signature] = {} + problem_dict[signature][pk] = hint_to_move problem_in_hints.value = json.dumps(problem_dict) problem_in_hints.save() diff --git a/lms/templates/courseware/hint_manager.html b/lms/templates/instructor/hint_manager.html similarity index 98% rename from lms/templates/courseware/hint_manager.html rename to lms/templates/instructor/hint_manager.html index ebd7091a09..4a6e219560 100644 --- a/lms/templates/courseware/hint_manager.html +++ b/lms/templates/instructor/hint_manager.html @@ -1,6 +1,6 @@ <%inherit file="/main.html" /> <%namespace name='static' file='/static_content.html'/> -<%namespace name="content" file="/courseware/hint_manager_inner.html"/> +<%namespace name="content" file="/instructor/hint_manager_inner.html"/> <%block name="headextra"> diff --git a/lms/templates/courseware/hint_manager_inner.html b/lms/templates/instructor/hint_manager_inner.html similarity index 90% rename from lms/templates/courseware/hint_manager_inner.html rename to lms/templates/instructor/hint_manager_inner.html index c69539522f..3acf8102ac 100644 --- a/lms/templates/courseware/hint_manager_inner.html +++ b/lms/templates/instructor/hint_manager_inner.html @@ -4,15 +4,16 @@

${field_label}

Switch to ${other_field_label} +

${error}

% for definition_id in all_hints:

Problem: ${id_to_name[definition_id]}

- % for answer, hint_dict in all_hints[definition_id]: + % for signature, 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: @@ -36,6 +37,7 @@ Switch to ${other_field_label
% endfor +

${error}

% if field == 'mod_queue': From a730f9189bc32558995937ef72aae687a3f9c8be Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Mon, 15 Jul 2013 11:25:59 -0400 Subject: [PATCH 06/28] Fixed numerous 500 errors that result from mal-formatted post requests. Fixed tests to work with symbolic answer changes. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 24 +++- .../xmodule/tests/test_crowdsource_hinter.py | 135 +++++++++++++++++- common/templates/hinter_display.html | 4 + 3 files changed, 158 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index bac6982085..5c5c869088 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -173,6 +173,9 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if out is None: out = {'op': 'empty'} + elif 'error' in out: + # Error in processing. + out.update({'op': 'error'}) else: out.update({'op': dispatch}) return json.dumps({'contents': self.system.render_template('hinter_display.html', out)}) @@ -288,14 +291,23 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Returns key 'hint_and_votes', a list of (hint_text, #votes) pairs. """ if self.user_voted: - return json.dumps({'contents': 'Sorry, but you have already voted!'}) + return {'error': 'Sorry, but you have already voted!'} ans = data['answer'] signature = self.answer_signature(ans) + if signature is None: + # Uh oh. Invalid answer. + 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 - temp_dict[signature][hint_pk][1] += 1 + try: + temp_dict[signature][hint_pk][1] += 1 + except KeyError: + log.exception('Failure in hinter tally_vote: User voted for non-existant hint: Answer=' + + ans + ' pk=' + hint_pk) + return {'error': 'Failure in voting!'} self.hints = temp_dict # Don't let the user vote again! self.user_voted = True @@ -303,7 +315,10 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Return a list of how many votes each hint got. hint_and_votes = [] for vote_pk in pk_list: - hint_and_votes.append(temp_dict[signature][str(vote_pk)]) + 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)) hint_and_votes.sort(key=lambda pair: pair[1], reverse=True) # Reset self.previous_answers. @@ -324,6 +339,9 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): hint = escape(data['hint']) answer = data['answer'] signature = self.answer_signature(answer) + if signature is None: + log.exception('Failure in hinter submit_hint: Unable to parse answer: ' + answer) + return {'error': 'Could not submit answer'} # Only allow a student to vote or submit a hint once. if self.user_voted: return {'message': 'Sorry, but you have already voted!'} diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 989b264694..b8a205e90e 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -2,13 +2,15 @@ Tests the crowdsourced hinter xmodule. """ -from mock import Mock +from mock import Mock, MagicMock import unittest import copy from xmodule.crowdsource_hinter import CrowdsourceHinterModule from xmodule.vertical_module import VerticalModule, VerticalDescriptor +from capa.responsetypes import StudentInputError + from . import get_test_system import json @@ -94,12 +96,54 @@ class CHModuleFactory(object): if moderate is not None: model_data['moderate'] = moderate - descriptor = Mock(weight="1") + descriptor = Mock(weight='1') + # Make the descriptor have a capa problem child. + capa_descriptor = MagicMock() + capa_descriptor.name = 'capa' + descriptor.get_children = lambda: [capa_descriptor] + + # Make a fake capa module. + capa_module = MagicMock() + capa_module.responders = {'responder0': MagicMock()} + capa_module.displayable_items = lambda: [capa_module] + system = get_test_system() + # Make the system have a marginally-functional get_module + + def fake_get_module(descriptor): + """ + A fake module-maker. + """ + if descriptor.name == 'capa': + return capa_module + system.get_module = fake_get_module + module = CrowdsourceHinterModule(system, descriptor, model_data) 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): """ @@ -226,6 +270,44 @@ class CrowdsourceHinterTest(unittest.TestCase): self.assertTrue('Test numerical problem.' in out_html) self.assertTrue('Another test numerical problem.' in out_html) + def test_numerical_answer_to_str(self): + """ + Tests the get request to string converter for numerical responses. + """ + mock_module = CHModuleFactory.create() + get = {'response1': '4'} + parsed = mock_module.numerical_answer_to_str(get) + self.assertTrue(parsed == '4.0') + + def test_formula_answer_to_str(self): + """ + Tests the get request to string converter for formula responses. + """ + mock_module = CHModuleFactory.create() + get = {'response1': 'x*y^2'} + parsed = mock_module.formula_answer_to_str(get) + self.assertTrue(parsed == 'x*y^2') + + 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. @@ -343,6 +425,44 @@ class CrowdsourceHinterTest(unittest.TestCase): self.assertTrue(['Best hint', 40] in dict_out['hint_and_votes']) self.assertTrue(['Another hint', 31] in dict_out['hint_and_votes']) + def test_vote_unparsable(self): + """ + A user somehow votes for an unparsable answer. + Should return a friendly error. + (This is an unusual exception path - I don't know how it occurs, + except if you manually make a post request. But, it seems to happen + occasionally.) + """ + mock_module = CHModuleFactory.create() + # None means that the answer couldn't be parsed. + mock_module.answer_signature = lambda text: None + json_in = {'answer': 'fish', 'hint': 3, 'pk_list': '[]'} + dict_out = mock_module.tally_vote(json_in) + print dict_out + self.assertTrue(dict_out == {'error': 'Failure in voting!'}) + + def test_vote_nohint(self): + """ + A user somehow votes for a hint that doesn't exist. + Should return a friendly error. + """ + mock_module = CHModuleFactory.create() + json_in = {'answer': '24.0', 'hint': '25', 'pk_list': '[]'} + dict_out = mock_module.tally_vote(json_in) + self.assertTrue(dict_out == {'error': 'Failure in voting!'}) + + def test_vote_badpklist(self): + """ + Some of the pk's specified in pk_list are invalid. + Should just skip those. + """ + mock_module = CHModuleFactory.create() + json_in = {'answer': '24.0', 'hint': '0', 'pk_list': json.dumps([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) + + def test_submithint_nopermission(self): """ A user tries to submit a hint, but he has already voted. @@ -399,6 +519,17 @@ class CrowdsourceHinterTest(unittest.TestCase): mock_module.submit_hint(json_in) self.assertTrue(mock_module.hints['29.0']['0'][0] == u'<script> alert("Trololo"); </script>') + def test_submithint_unparsable(self): + mock_module = CHModuleFactory.create() + mock_module.answer_signature = lambda text: None + json_in = {'answer': 'fish', 'hint': 'A hint'} + dict_out = mock_module.submit_hint(json_in) + print dict_out + print mock_module.hints + self.assertTrue('error' in dict_out) + self.assertTrue(None not in mock_module.hints) + self.assertTrue('fish' not in mock_module.hints) + def test_template_gethint(self): """ Test the templates for get_hint. diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 4f40113265..3ca1ce5330 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -137,6 +137,10 @@ What would you say to help someone who got this wrong answer? ${simple_message()} % endif +% if op == "error": + ${error} +% endif + % if op == "vote": ${show_votes()} % endif From 6b40c5cf132164d9615e353f04034285af9eece4 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Mon, 15 Jul 2013 15:59:58 -0400 Subject: [PATCH 07/28] Changed hint voting ui to show all hints on one page. Fixed broken tests for hint manager. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 45 ++++++++--- .../js/src/crowdsource_hinter/display.coffee | 5 +- .../xmodule/tests/test_crowdsource_hinter.py | 26 +++++- common/templates/hinter_display.html | 80 ++++++++++++------- lms/djangoapps/instructor/hint_manager.py | 8 +- .../instructor/tests/test_hint_manager.py | 23 +++++- 6 files changed, 132 insertions(+), 55 deletions(-) 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
    @@ -40,36 +72,22 @@
% 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 From 4ee8111cf4a77229404716ad85eeb00995a0fbbc Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Tue, 16 Jul 2013 09:12:35 -0400 Subject: [PATCH 08/28] Fixed monkey-patching persistent state bug. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 4 ++-- .../xmodule/tests/test_crowdsource_hinter.py | 18 +++++++++++++++--- .../instructor/tests/test_hint_manager.py | 16 ++++++---------- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index b82241bd08..1d7cf954a4 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -126,7 +126,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): -Lon-capa dependent. -Assumes that the problem only has one part. """ - return str(float(answer.values()[0])) + return str(answer.values()[0]) def formula_answer_to_str(self, answer): """ @@ -207,7 +207,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): """ try: answer = self.answer_to_str(data) - except ValueError: + except (ValueError, AttributeError): # Sometimes, we get an answer that's just not parsable. Do nothing. log.exception('Answer not parsable: ' + str(data)) return diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index 844c73a990..cc0634cb7d 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -277,7 +277,7 @@ class CrowdsourceHinterTest(unittest.TestCase): mock_module = CHModuleFactory.create() get = {'response1': '4'} parsed = mock_module.numerical_answer_to_str(get) - self.assertTrue(parsed == '4.0') + self.assertTrue(parsed == '4') def test_formula_answer_to_str(self): """ @@ -342,12 +342,24 @@ class CrowdsourceHinterTest(unittest.TestCase): def test_gethint_unparsable(self): """ - Someone submits a hint that cannot be parsed into a float. + Someone submits an answer that is in the wrong format. - 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'} + json_in = 'blah' + out = mock_module.get_hint(json_in) + self.assertTrue(out is None) + self.assertTrue(mock_module.previous_answers == old_answers) + + def test_gethint_signature_error(self): + """ + Someone submits an answer that cannot be calculated as a float. + Nothing should change. + """ + mock_module = CHModuleFactory.create() + old_answers = copy.deepcopy(mock_module.previous_answers) + json_in = {'problem1': 'fish'} out = mock_module.get_hint(json_in) self.assertTrue(out is None) self.assertTrue(mock_module.previous_answers == old_answers) diff --git a/lms/djangoapps/instructor/tests/test_hint_manager.py b/lms/djangoapps/instructor/tests/test_hint_manager.py index 9e579ab997..e9387a8eef 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 +from mock import MagicMock, patch from courseware.models import XModuleContentField from courseware.tests.factories import ContentFactory @@ -12,6 +12,8 @@ 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): @@ -140,12 +142,8 @@ class HintManagerTest(ModuleStoreTestCase): """ # 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', @@ -154,14 +152,12 @@ class HintManagerTest(ModuleStoreTestCase): 'answer': '3.14', 'hint': 'This is a new hint.'}) post.user = 'fake user' - view.add_hint(post, self.course_id, 'mod_queue') + 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('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 From 8ce53ed8eb865f651d755245d554926a3583a7c2 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Wed, 17 Jul 2013 17:00:51 -0400 Subject: [PATCH 09/28] Got rid of answer signatures - these don't work with approximate answers at all. Instead, implemented comparison-based hint matching. Tests are broken, hint manager is probably broken. --- common/lib/capa/capa/responsetypes.py | 39 ++++ .../lib/xmodule/xmodule/crowdsource_hinter.py | 196 ++++++++---------- common/templates/hinter_display.html | 24 +-- 3 files changed, 131 insertions(+), 128 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 47289aad51..5f2408a09e 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -917,6 +917,27 @@ class NumericalResponse(LoncapaResponse): # TODO: add check_hint_condition(self, hxml_set, student_answers) + def answer_compare(self, a, b): + """ + Outside-facing function that lets us compare two numerical answers, + with this problem's tolerance. + """ + return compare_with_tolerance( + evaluator(dict(), dict(), a), + evaluator(dict(), dict(), b), + self.tolerance + ) + + def validate_answer(self, answer): + """ + Returns whether this answer is in a valid form. + """ + try: + evaluator(dict(), dict(), answer) + return True + except StudentInputError: + return False + def get_answers(self): return {self.answer_id: self.correct_answer} @@ -1858,6 +1879,24 @@ class FormulaResponse(LoncapaResponse): return "incorrect" return "correct" + def answer_compare(self, a, b): + """ + An external interface for comparing whether a and b are equal. + """ + internal_result = self.check_formula(a, b, self.samples) + return internal_result == "correct" + + def validate_answer(self, answer): + """ + Returns whether this answer is in a valid form. + """ + var_dict_list = self.randomize_variables(self.samples) + try: + self.hash_answers(answer, var_dict_list) + return True + except StudentInputError: + return False + def strip_dict(self, d): ''' Takes a dict. Returns an identical dict, with all non-word keys and all non-numeric values stripped out. All values also diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 1d7cf954a4..37f244cdba 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -42,18 +42,11 @@ class CrowdsourceHinterFields(object): mod_queue = Dict(help='A dictionary containing hints still awaiting approval', scope=Scope.content, default={}) hint_pk = Integer(help='Used to index hints.', scope=Scope.content, default=0) - # signature_to_ans maps an answer signature to an answer string that shows that answer in a - # human-readable form. - signature_to_ans = Dict(help='Maps a signature to a representative formula.', scope=Scope.content, - default={}) - # A list of dictionaries, each of which represents an n-dimenstional point that we plug into - # formulas. Each dictionary maps variables to values, eg {'x': 5.1}. - formula_test_values = List(help='The values that we plug into formula responses', scope=Scope.content, - default=[]) + # A list of previous answers this student made to this problem. - # Of the form [answer, [hint_pk_1, hint_pk_2, hint_pk_3]] for each problem. hint_pk's are - # None if the hint was not given. - previous_answers = List(help='A list of previous submissions.', scope=Scope.user_state, default=[]) + # Of the form [answer, [hint_pk_1, ...]] for each problem. + previous_answers = List(help='A list of hints viewed.', scope=Scope.user_state, default=[]) + user_submissions = List(help='A list of previous submissions', scope=Scope.user_state, default=[]) user_voted = Boolean(help='Specifies if the user has voted on this problem or not.', scope=Scope.user_state, default=False) @@ -82,13 +75,20 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): def __init__(self, *args, **kwargs): XModule.__init__(self, *args, **kwargs) # We need to know whether we are working with a FormulaResponse problem. - self.is_formula = (type(self.get_display_items()[0].lcp.responders.values()[0]) == FormulaResponse) + responder = self.get_display_items()[0].lcp.responders.values()[0] + self.is_formula = (type(responder) == FormulaResponse) if self.is_formula: self.answer_to_str = self.formula_answer_to_str - self.answer_signature = self.formula_answer_signature else: self.answer_to_str = self.numerical_answer_to_str - self.answer_signature = self.numerical_answer_signature + # answer_compare is expected to return whether its two inputs are close enough + # to be equal, or raise a StudentInputError if one of the inputs is malformatted. + try: + self.answer_compare = responder.answer_compare + self.validate_answer = responder.validate_answer + except AttributeError: + # This response type is not supported! + log.exception('Response type not supported for hinting: ' + str(responder)) def get_html(self): """ @@ -136,38 +136,12 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): """ return str(answer.values()[0]) - def numerical_answer_signature(self, answer): + def get_matching_answers(self, answer): """ - Runs the answer string through the evaluator. (This is because - symbolic expressions like sin(pi/12)*3 are allowed.) + Look in self.hints, and find all answer keys that are "equal with tolerance" + to the input answer. """ - 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) - to a string unique to each formula equality class. - So, x^2 and x*x would have the same signature, which would differ - from the signature of 2*x^2. - """ - responder = self.get_display_items()[0].lcp.responders.values()[0] - if self.formula_test_values == []: - # Make a set of test values, and save them. - self.formula_test_values = responder.randomize_variables(responder.samples) - try: - # TODO, maybe: add some rounding to signature generation, so that floating point - # errors don't make a difference. - out = str(responder.hash_answers(answer, self.formula_test_values)) - except StudentInputError: - # I'm not sure what's the best thing to do here. I'm returning - # None, for now, so that the calling function has a chance to catch - # the error without having to import StudentInputError. - return None - return out + return [key for key in self.hints if self.answer_compare(key, answer)] def handle_ajax(self, dispatch, data): """ @@ -211,47 +185,67 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Sometimes, we get an answer that's just not parsable. Do nothing. log.exception('Answer not parsable: ' + str(data)) return - # Make a signature of the answer, for formula responses. - signature = self.answer_signature(answer) - if signature is None: - # Sometimes, signature conversion may fail. - log.exception('Signature conversion failed: ' + str(answer)) - return + if not self.validate_answer(answer): + # Answer is not in the right form. + log.exception('Answer not valid: ' + str(answer)) + if answer not in self.user_submissions: + self.user_submissions += [answer] # 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.) local_hints = self.hints - if (signature not in local_hints) or (len(local_hints[signature]) == 0): + # For all answers similar enough to our own, accumulate all hints together. + # Also track the original answer of each hint. + matching_answers = self.get_matching_answers(answer) + matching_hints = {} + for matching_answer in matching_answers: + temp_dict = local_hints[matching_answer] + for key, value in temp_dict.items(): + # Each value now has hint, votes, matching_answer. + temp_dict[key] = value + [matching_answer] + matching_hints.update(local_hints[matching_answer]) + # matching_hints now maps pk's to lists of [hint, votes, matching_answer] + if len(matching_hints) == 0: # No hints to give. Return. - self.previous_answers += [[answer, [None, None, None]]] return # Get the top hint, plus two random hints. - n_hints = len(local_hints[signature]) - best_hint_index = max(local_hints[signature], key=lambda key: local_hints[signature][key][1]) - best_hint = local_hints[signature][best_hint_index][0] - if len(local_hints[signature]) == 1: - rand_hint_1 = '' - rand_hint_2 = '' - self.previous_answers += [[answer, [best_hint_index, None, None]]] - elif n_hints == 2: - best_hint = local_hints[signature].values()[0][0] - best_hint_index = local_hints[signature].keys()[0] - rand_hint_1 = local_hints[signature].values()[1][0] - hint_index_1 = local_hints[signature].keys()[1] - rand_hint_2 = '' - self.previous_answers += [[answer, [best_hint_index, hint_index_1, None]]] - else: - (hint_index_1, rand_hint_1), (hint_index_2, rand_hint_2) =\ - random.sample(local_hints[signature].items(), 2) - rand_hint_1 = rand_hint_1[0] - rand_hint_2 = rand_hint_2[0] - self.previous_answers += [[answer, [best_hint_index, hint_index_1, hint_index_2]]] - - return {'best_hint': best_hint, - 'rand_hint_1': rand_hint_1, - 'rand_hint_2': rand_hint_2, + n_hints = len(matching_hints) + hints = [] + best_hint_index = max(matching_hints, key=lambda key: matching_hints[key][1]) + hints.append(matching_hints[best_hint_index][0]) + best_hint_answer = matching_hints[best_hint_index][2] + # The brackets surrounding the index are for backwards compatability purposes. + # (It used to be that each answer was paired with multiple hints in a list.) + 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 + hints.append(rand_hint) + self.previous_answers += [[hint_index, [hint_answer]]] + return {'hints': hints, 'answer': answer} + # rand_hint_1 = '' + # rand_hint_2 = '' + # if n_hints == 2: + # best_hint = matching_hints.values()[0][0] + # best_hint_index = matching_hints.keys()[0] + # rand_hint_1 = matching_hints.values()[1][0] + # hint_index_1 = matching_hints.keys()[1] + # rand_hint_2 = '' + # self.previous_answers += [[answer, [best_hint_index, hint_index_1]]] + # else: + # (hint_index_1, rand_hint_1), (hint_index_2, rand_hint_2) =\ + # random.sample(matching_hints.items(), 2) + # rand_hint_1 = rand_hint_1[0] + # rand_hint_2 = rand_hint_2[0] + # self.previous_answers += [[answer, [best_hint_index, hint_index_1, hint_index_2]]] + def get_feedback(self, data): """ The student got it correct. Ask him to vote on hints, or submit a hint. @@ -271,32 +265,24 @@ 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] = {} - if signature in self.hints: + if answer in self.hints: # Go through each hint, and add to index_to_hints for hint_id in hints_offered: if (hint_id is not None) and (hint_id not in answer_to_hints[answer]): try: - answer_to_hints[answer][hint_id] = self.hints[signature][str(hint_id)][0] + answer_to_hints[answer][hint_id] = self.hints[answer][str(hint_id)][0] except KeyError: # Sometimes, the hint that a user saw will have been deleted by the instructor. continue - return {'answer_to_hints': answer_to_hints} + return {'answer_to_hints': answer_to_hints, + 'user_submissions': self.user_submissions} def tally_vote(self, data): """ @@ -315,8 +301,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if self.user_voted: return {'error': 'Sorry, but you have already voted!'} ans = data['answer'] - signature = self.answer_signature(ans) - if signature is None: + if not self.validate_answer(ans): # Uh oh. Invalid answer. log.exception('Failure in hinter tally_vote: Unable to parse answer: ' + ans) return {'error': 'Failure in voting!'} @@ -324,7 +309,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # We use temp_dict because we need to do a direct write for the database to update. temp_dict = self.hints try: - temp_dict[signature][hint_pk][1] += 1 + temp_dict[ans][hint_pk][1] += 1 except KeyError: log.exception('Failure in hinter tally_vote: User voted for non-existant hint: Answer=' + ans + ' pk=' + hint_pk) @@ -337,19 +322,19 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): pk_list = json.loads(data['pk_list']) hint_and_votes = [] for answer, vote_pk in pk_list: - signature = self.answer_signature(answer) - if signature is None: + if not self.validate_answer(answer): log.exception('In hinter tally_vote, couldn\'t parse ' + answer) continue try: - hint_and_votes.append(temp_dict[signature][str(vote_pk)]) + hint_and_votes.append(temp_dict[answer][str(vote_pk)]) except KeyError: 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. + # Reset self.previous_answers and user_submissions. self.previous_answers = [] + self.user_submissions = [] return {'hint_and_votes': hint_and_votes} def submit_hint(self, data): @@ -365,8 +350,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Do html escaping. Perhaps in the future do profanity filtering, etc. as well. hint = escape(data['hint']) answer = data['answer'] - signature = self.answer_signature(answer) - if signature is None: + if not self.validate_answer(answer): log.exception('Failure in hinter submit_hint: Unable to parse answer: ' + answer) return {'error': 'Could not submit answer'} # Only allow a student to vote or submit a hint once. @@ -379,12 +363,9 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): else: temp_dict = self.hints if answer in temp_dict: - temp_dict[signature][str(self.hint_pk)] = [hint, 1] # With one vote (the user himself). + temp_dict[answer][str(self.hint_pk)] = [hint, 1] # With one vote (the user himself). else: - temp_dict[signature] = {str(self.hint_pk): [hint, 1]} - # Add the signature to signature_to_ans, if it's not there yet. - # This allows instructors to see a human-readable answer that corresponds to each signature. - self.add_signature(signature, answer) + temp_dict[answer] = {str(self.hint_pk): [hint, 1]} self.hint_pk += 1 if self.moderate == 'True': self.mod_queue = temp_dict @@ -393,18 +374,9 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # Mark the user has having voted; reset previous_answers self.user_voted = True self.previous_answers = [] + self.user_submissions = [] return {'message': 'Thank you for your hint!'} - def add_signature(self, signature, answer): - """ - Add a signature to self.signature_to_ans. If the signature already - exists, do nothing. - """ - if signature not in self.signature_to_ans: - local_sta = self.signature_to_ans - local_sta[signature] = answer - self.signature_to_ans = local_sta - class CrowdsourceHinterDescriptor(CrowdsourceHinterFields, RawDescriptor): module_class = CrowdsourceHinterModule diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index b13dc83ea2..ebd98e09bd 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -3,18 +3,14 @@ <%def name="get_hint()"> - % if best_hint != '': + % if len(hints) > 0:

Hints from students who made similar mistakes:

    -
  • ${best_hint}
  • + % for hint in hints: +
  • ${hint}
  • + % endfor +
% endif - % if rand_hint_1 != '': -
  • ${rand_hint_1}
  • - % endif - % if rand_hint_2 != '': -
  • ${rand_hint_2}
  • - % endif - <%def name="get_feedback()"> @@ -66,17 +62,13 @@
    - % for answer, pk_dict in answer_to_hints.items(): - <% - import json - all_pks = json.dumps(pk_dict.keys()) - %> -
    + % for answer in user_submissions: +

    What hint would you give a student who made the same mistake you did? Please don't give away the answer. From d9517ea13e2cbb63f4c2203186bb108a3074e78f Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 18 Jul 2013 15:02:19 -0400 Subject: [PATCH 10/28] 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: From e2aea75f1626b7407053cbd44905ba07bb4087c8 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 18 Jul 2013 15:39:01 -0400 Subject: [PATCH 11/28] Fixed a bug in recording hints shown. Removed the answer display next to voting. (This was deemed distracting.) --- common/lib/xmodule/xmodule/crowdsource_hinter.py | 3 +-- common/templates/hinter_display.html | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index ebb07506cb..147de956af 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -227,7 +227,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if not rand_hint in hints: go_on = True hints.append(rand_hint) - self.previous_answers += [[hint_index, [hint_answer]]] + self.previous_answers += [[hint_answer, [hint_index]]] return {'hints': hints, 'answer': answer} @@ -281,7 +281,6 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): except KeyError: # Sometimes, the hint that a user saw will have been deleted by the instructor. continue - return {'answer_to_hints': answer_to_hints, 'user_submissions': self.user_submissions} diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index ebd98e09bd..4050824c5b 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -46,7 +46,7 @@ % for hint_pk, hint_text in pk_dict.items():

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

    % endfor % endfor @@ -71,7 +71,7 @@

    - What hint would you give a student who made the same mistake you did? Please don't give away the answer. + What hint would you give a student who also arrived at an answer of ${answer}? Please don't give away the correct answer.

    -

    - -
    - % endfor -
    + +

    + -

    Read about what makes a good hint.

    - + % endif + From 7c93b45d0c0a7c2a0bc1d131593fe11258c028d2 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Wed, 31 Jul 2013 17:13:32 -0400 Subject: [PATCH 18/28] Added wizard / slideshow style hint collection script. Still requires a little polishing, I think. --- .../js/src/crowdsource_hinter/display.coffee | 32 +++++- common/templates/hinter_display.html | 101 +++++++++++------- 2 files changed, 87 insertions(+), 46 deletions(-) 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 a1dc35a604..34956603c6 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -36,8 +36,9 @@ class @Hinter @$('input.vote').click @vote @$('input.submit-hint').click @submit_hint @$('.custom-hint').click @clear_default_text - @$('#answer-tabs').tabs({active: 0}) @$('.expand').click @expand + @$('.wizard-link').click @wizard_link_handle + @$('.answer-choice').click @answer_choice_handle expand: (eventObj) => target = @$('#' + @$(eventObj.currentTarget).data('target')) @@ -55,11 +56,10 @@ class @Hinter submit_hint: (eventObj) => textarea = $('.custom-hint') - answer = $('input:radio[name=answer-select]:checked').val() - if answer == undefined - # The user didn't choose an answer. Do nothing. + if @answer == '' + # The user didn't choose an answer, somehow. Do nothing. return - post_json = {'answer': answer, 'hint': textarea.val()} + post_json = {'answer': @answer, 'hint': textarea.val()} $.postWithPrefix "#{@url}/submit_hint",post_json, (response) => @render(response.contents) @@ -69,6 +69,15 @@ class @Hinter target.val('') target.data('cleared', true) + wizard_link_handle: (eventObj) => + target = @$(eventObj.currentTarget) + @go_to(target.attr('dest')) + + answer_choice_handle: (eventObj) => + @answer = @$(eventObj.target).attr('value') + @$('#blank-answer').html(@answer) + @go_to('p3') + render: (content) -> if content # Trim leading and trailing whitespace @@ -82,3 +91,16 @@ class @Hinter @$('#previous-answer-0').css('display', 'inline') else @el.hide() + # Initialize the answer choice - remembers which answer the user picked on + # p2 when he submits a hint on p3. + @answer = '' + # Make the correct wizard view show up. + hints_exist = @$('#hints-exist').html() == 'True' + if hints_exist + @go_to('p1') + else + @go_to('p2') + + go_to: (view_id) -> + $('.wizard-view').css('display', 'none') + $('#' + view_id).css('display', 'block') \ No newline at end of file diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 8fd219e847..0f350083de 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -34,50 +34,71 @@ 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(): -

    - - ${hint_text} -

    - % endfor - % endfor - -

    - Don't like any of the hints above? - - Write your own! -

    - - +
    + From 35a8e9be326dab1ca27cc80160e0a29319f62653 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 1 Aug 2013 09:40:49 -0400 Subject: [PATCH 19/28] Some fixes on hint ui. --- common/templates/hinter_display.html | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 0f350083de..a7f2eafd1c 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -78,9 +78,7 @@ % endfor % if hints_exist:
    -

    - Back to voting. -

    + Back % endif
    @@ -89,17 +87,7 @@

    Write a hint for other students who get the wrong answer of . -
    - - Choose a different answer. -

    - -

    - -

    Read about what makes a good hint.

    + + +

    + +

    + Back
    From 2d1c9158d885fee77e35146f5588cb931b6d3704 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 1 Aug 2013 14:52:37 -0400 Subject: [PATCH 20/28] Added fancy sliding transitions into hint collection page. --- .../css/crowdsource_hinter/display.scss | 71 +++----- .../js/src/crowdsource_hinter/display.coffee | 36 +++- common/templates/hinter_display.html | 170 +++++++++--------- 3 files changed, 143 insertions(+), 134 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss index c1d7b1f048..ce40d10b53 100644 --- a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss +++ b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss @@ -7,52 +7,6 @@ background: rgb(253, 248, 235); } -#answer-tabs { - background: #FFFFFF; - border: none; - margin-bottom: 20px; - padding-bottom: 20px; -} - -#answer-tabs .ui-widget-header { - border-bottom: 1px solid #DCDCDC; - background: #FDF8EB; -} - -#answer-tabs .ui-tabs-nav .ui-state-default { - border: 1px solid #DCDCDC; - background: #E6E6E3; - margin-bottom: 0px; -} - -#answer-tabs .ui-tabs-nav .ui-state-default:hover { - background: #FFFFFF; -} - -#answer-tabs .ui-tabs-nav .ui-state-active:hover { - background: #FFFFFF; -} - -#answer-tabs .ui-tabs-nav .ui-state-active { - border: 1px solid #DCDCDC; - background: #FFFFFF; -} - -#answer-tabs .ui-tabs-nav .ui-state-active a { - color: #222222; - background: #FFFFFF; -} - -#answer-tabs .ui-tabs-nav .ui-state-default a:hover { - color: #222222; - background: #FFFFFF; -} - -#answer-tabs .custom-hint { - height: 100px; - width: 100%; -} - .hint-inner-container { padding-left: 15px; padding-right: 15px; @@ -63,3 +17,28 @@ padding-top: 0px !important; padding-bottom: 0px !important; } + +.wizard-view { + float: left; + width: 800px; +} + +.wizard-container { + width: 3000px; + + -webkit-transition:all 1.0s ease-in-out; + -moz-transition:all 1.0s ease-in-out; + -o-transition:all 1.0s ease-in-out; + transition:all 1.0s ease-in-out; +} + +.wizard-viewbox { + width: 790px; + overflow: hidden; + position: relative; +} + +.bottom { + position: absolute; + bottom: 0; +} \ No newline at end of file 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 34956603c6..3ecf73a0dc 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -41,13 +41,17 @@ class @Hinter @$('.answer-choice').click @answer_choice_handle expand: (eventObj) => + # Expand a hidden div. target = @$('#' + @$(eventObj.currentTarget).data('target')) if @$(target).css('display') == 'none' @$(target).css('display', 'block') else @$(target).css('display', 'none') + # Fix positioning errors with the bottom class. + @$('.bottom').removeClass('bottom').addClass('bottom') vote: (eventObj) => + # Make an ajax request with the user's vote. target = @$(eventObj.currentTarget) all_pks = @$('#pk-list').attr('data-pk-list') post_json = {'answer': target.attr('data-answer'), 'hint': target.data('hintno'), 'pk_list': all_pks} @@ -55,6 +59,7 @@ class @Hinter @render(response.contents) submit_hint: (eventObj) => + # Make an ajax request with the user's new hint. textarea = $('.custom-hint') if @answer == '' # The user didn't choose an answer, somehow. Do nothing. @@ -64,21 +69,25 @@ class @Hinter @render(response.contents) clear_default_text: (eventObj) => + # Remove placeholder text in the hint submission textbox. target = @$(eventObj.currentTarget) if target.data('cleared') == undefined target.val('') target.data('cleared', true) wizard_link_handle: (eventObj) => + # Move to another wizard view, based on the link that the user clicked. target = @$(eventObj.currentTarget) @go_to(target.attr('dest')) answer_choice_handle: (eventObj) => + # A special case of wizard_link_handle - we need to track a state variable, + # the answer that the user chose. @answer = @$(eventObj.target).attr('value') @$('#blank-answer').html(@answer) @go_to('p3') - render: (content) -> + render: (content) => if content # Trim leading and trailing whitespace content = content.replace /^\s+|\s+$/g, "" @@ -94,6 +103,13 @@ class @Hinter # Initialize the answer choice - remembers which answer the user picked on # p2 when he submits a hint on p3. @answer = '' + # Determine whether the browser supports CSS3 transforms. + styles = document.body.style + if styles.WebkitTransform == '' or styles.transform == '' + @go_to = @transform_go_to + else + @go_to = @legacy_go_to + # Make the correct wizard view show up. hints_exist = @$('#hints-exist').html() == 'True' if hints_exist @@ -101,6 +117,18 @@ class @Hinter else @go_to('p2') - go_to: (view_id) -> - $('.wizard-view').css('display', 'none') - $('#' + view_id).css('display', 'block') \ No newline at end of file + transform_go_to: (view_id) -> + # Switch wizard views using sliding transitions. + id_to_index = { + 'p1': 0, + 'p2': 1, + 'p3': 2, + } + translate_string = 'translateX(' +id_to_index[view_id] * -1 * parseInt($('#' + view_id).css('width'), 10) + 'px)' + @$('.wizard-container').css('transform', translate_string) + @$('.wizard-container').css('-webkit-transform', translate_string) + + legacy_go_to: (view_id) -> + # For older browsers - switch wizard views by changing the screen. + @$('.wizard-view').css('display', 'none') + @$('#' + view_id).css('display', 'block') \ No newline at end of file diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index a7f2eafd1c..6cd5040072 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -38,98 +38,100 @@ - -
    -

    - 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(): -

    - - ${hint_text} -

    - % endfor - % endfor - -

    - Don't like any of the hints above? - - Write your own! -

    -
    - -
    - % if hints_exist: +
    +

    - Choose the incorrect answer for which you want to write a hint: -

    - % else: -

    - Optional. Help other students by submitting a hint! Pick one of your previous - answers for which you would like to write a hint: -

    - % endif - % for answer in user_submissions: - ${answer}
    - % endfor - % if hints_exist: -
    - Back - % endif - -
    - -
    - -

    - Write a hint for other students who get the wrong answer of . -

    -

    Read about what makes a good hint.

    - - -

    - -

    - Back -
    +
    + % if hints_exist: +

    + Choose the incorrect answer for which you want to write a hint: +

    + % else: +

    + Optional. Help other students by submitting a hint! Pick one of your previous + answers for which you would like to write a hint: +

    + % endif + % for answer in user_submissions: + ${answer}
    + % endfor + % if hints_exist: + +

     

    + Back + % endif +
    + +
    + +

    + Write a hint for other students who get the wrong answer of . +

    +

    Read about what makes a good hint.

    + + +

    + + +
    + Back +
    + +
    From 42202520551cfb9988723599565cc8a37b9fe961 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Fri, 2 Aug 2013 09:22:22 -0400 Subject: [PATCH 21/28] Fixed terribly annoying issues with back button. Back button is now on the bottom of the hint div, no matter what. --- .../xmodule/css/crowdsource_hinter/display.scss | 5 ----- .../js/src/crowdsource_hinter/display.coffee | 15 +++++++++++++-- common/templates/hinter_display.html | 11 ++++++----- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss index ce40d10b53..0c479ab5cb 100644 --- a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss +++ b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss @@ -37,8 +37,3 @@ overflow: hidden; position: relative; } - -.bottom { - position: absolute; - bottom: 0; -} \ No newline at end of file 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 3ecf73a0dc..2e99b107f9 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -48,7 +48,7 @@ class @Hinter else @$(target).css('display', 'none') # Fix positioning errors with the bottom class. - @$('.bottom').removeClass('bottom').addClass('bottom') + @set_bottom_links() vote: (eventObj) => # Make an ajax request with the user's vote. @@ -87,6 +87,15 @@ class @Hinter @$('#blank-answer').html(@answer) @go_to('p3') + set_bottom_links: => + # Makes each .bottom class stick to the bottom of .wizard-viewbox + @$('.bottom').css('margin-top', '0px') + viewbox_height = parseInt(@$('.wizard-viewbox').css('height'), 10) + @$('.bottom').each((index, obj) -> + view_height = parseInt($(obj).parent().css('height'), 10) + $(obj).css('margin-top', (viewbox_height - view_height) + 'px') + ) + render: (content) => if content # Trim leading and trailing whitespace @@ -127,8 +136,10 @@ class @Hinter translate_string = 'translateX(' +id_to_index[view_id] * -1 * parseInt($('#' + view_id).css('width'), 10) + 'px)' @$('.wizard-container').css('transform', translate_string) @$('.wizard-container').css('-webkit-transform', translate_string) + @set_bottom_links() legacy_go_to: (view_id) -> # For older browsers - switch wizard views by changing the screen. @$('.wizard-view').css('display', 'none') - @$('#' + view_id).css('display', 'block') \ No newline at end of file + @$('#' + view_id).css('display', 'block') + @set_bottom_links() \ No newline at end of file diff --git a/common/templates/hinter_display.html b/common/templates/hinter_display.html index 6cd5040072..12f6b9f2f4 100644 --- a/common/templates/hinter_display.html +++ b/common/templates/hinter_display.html @@ -77,9 +77,9 @@ ${answer}
    % endfor % if hints_exist: - -

     

    - Back +

    + Back +

    % endif
    @@ -127,8 +127,9 @@ Write your hint here. Please don't give away the correct answer. Learn even more

    -
    - Back +

    + Back +

    From 69fbe77dcb82840cc8fea78fac1b103fe69858cd Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Thu, 15 Aug 2013 10:57:06 -0400 Subject: [PATCH 22/28] Fixed a rebase error in responsetypes. Fixed a css bug in the hinter. --- common/lib/capa/capa/responsetypes.py | 2 +- .../lib/xmodule/xmodule/css/crowdsource_hinter/display.scss | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 182266c21a..192d6c0663 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -1812,7 +1812,7 @@ class FormulaResponse(LoncapaResponse): var_dict, dict(), answer, - cs=self.case_sensitive, + case_sensitive=self.case_sensitive, )) except UndefinedVariable as uv: log.debug( diff --git a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss index 0c479ab5cb..dab94ed2f7 100644 --- a/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss +++ b/common/lib/xmodule/xmodule/css/crowdsource_hinter/display.scss @@ -20,7 +20,8 @@ .wizard-view { float: left; - width: 800px; + width: 790px; + margin-right: 10px; } .wizard-container { @@ -33,7 +34,7 @@ } .wizard-viewbox { - width: 790px; + width: 800px; overflow: hidden; position: relative; } From e6067d88145992e9b6ebc02ff78bf34e088081b7 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Mon, 19 Aug 2013 13:58:59 -0400 Subject: [PATCH 23/28] Addressed PR comments. Fixed coffeescript event logging error. Fixed crowdsourced hinter dependence on self.field.append() not working. --- common/lib/capa/capa/responsetypes.py | 4 +- .../lib/xmodule/xmodule/crowdsource_hinter.py | 48 ++++++++++--------- .../xmodule/js/src/capa/display.coffee | 1 + 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 192d6c0663..550042d1df 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -923,8 +923,8 @@ class NumericalResponse(LoncapaResponse): with this problem's tolerance. """ return compare_with_tolerance( - evaluator(dict(), dict(), a), - evaluator(dict(), dict(), b), + evaluator({}, {}, a), + evaluator({}, {}, b), self.tolerance ) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 74b5f7b36e..90699beda6 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -7,6 +7,7 @@ Currently experimental - not for instructor use, yet. import logging import json import random +import copy from pkg_resources import resource_string @@ -82,17 +83,17 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): log.exception('Unable to find a capa problem child.') return - self.is_formula = (type(responder) == FormulaResponse) + self.is_formula = isinstance(self, FormulaResponse) if self.is_formula: self.answer_to_str = self.formula_answer_to_str else: self.answer_to_str = self.numerical_answer_to_str # compare_answer is expected to return whether its two inputs are close enough # to be equal, or raise a StudentInputError if one of the inputs is malformatted. - try: + if hasattr(responder, 'compare_answer') and hasattr(responder, 'validate_answer'): self.compare_answer = responder.compare_answer self.validate_answer = responder.validate_answer - except AttributeError: + else: # This response type is not supported! log.exception('Response type not supported for hinting: ' + str(responder)) @@ -199,43 +200,43 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): if answer not in self.user_submissions: self.user_submissions += [answer] - # Next, find all of the hints that could possibly go with this answer. - # 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.) - local_hints = self.hints # For all answers similar enough to our own, accumulate all hints together. # Also track the original answer of each hint. matching_answers = self.get_matching_answers(answer) matching_hints = {} for matching_answer in matching_answers: - temp_dict = local_hints[matching_answer] + temp_dict = copy.deepcopy(self.hints[matching_answer]) for key, value in temp_dict.items(): # Each value now has hint, votes, matching_answer. temp_dict[key] = value + [matching_answer] - matching_hints.update(local_hints[matching_answer]) + matching_hints.update(temp_dict) # matching_hints now maps pk's to lists of [hint, votes, matching_answer] # Finally, randomly choose a subset of matching_hints to actually show. - if len(matching_hints) == 0: + if not matching_hints: # No hints to give. Return. return # Get the top hint, plus two random hints. n_hints = len(matching_hints) hints = [] - best_hint_index = max(matching_hints, key=lambda key: matching_hints[key][1]) + # max(dict) returns the maximum key in dict. + # The key function takes each pk, and returns the number of votes for the + # hint with that pk. + best_hint_index = max(matching_hints, key=lambda pk: matching_hints[pk][1]) hints.append(matching_hints[best_hint_index][0]) best_hint_answer = matching_hints[best_hint_index][2] # The brackets surrounding the index are for backwards compatability purposes. # (It used to be that each answer was paired with multiple hints in a list.) self.previous_answers += [[best_hint_answer, [best_hint_index]]] - for i in xrange(min(2, n_hints-1)): + for i in xrange(min(2, n_hints - 1)): # Keep making random hints until we hit a target, or run out. - go_on = False - while not go_on: + while True: + # random.choice randomly chooses an element from its input list. + # (We then unpack the item, in this case data for a hint.) (hint_index, (rand_hint, votes, hint_answer)) =\ random.choice(matching_hints.items()) - if not rand_hint in hints: - go_on = True + if rand_hint not in hints: + break hints.append(rand_hint) self.previous_answers += [[hint_answer, [hint_index]]] return {'hints': hints, @@ -297,7 +298,7 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): ans = data['answer'] if not self.validate_answer(ans): # Uh oh. Invalid answer. - log.exception('Failure in hinter tally_vote: Unable to parse answer: ' + ans) + log.exception('Failure in hinter tally_vote: Unable to parse answer: {ans}'.format(ans=ans)) return {'error': 'Failure in voting!'} hint_pk = str(data['hint']) # We use temp_dict because we need to do a direct write for the database to update. @@ -305,8 +306,8 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): try: temp_dict[ans][hint_pk][1] += 1 except KeyError: - log.exception('Failure in hinter tally_vote: User voted for non-existant hint: Answer=' + - ans + ' pk=' + hint_pk) + log.exception('''Failure in hinter tally_vote: User voted for non-existant hint: + Answer={ans} pk={hint_pk}'''.format(ans=ans, hint_pk=hint_pk)) return {'error': 'Failure in voting!'} self.hints = temp_dict # Don't let the user vote again! @@ -317,13 +318,13 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): hint_and_votes = [] for answer, vote_pk in pk_list: if not self.validate_answer(answer): - log.exception('In hinter tally_vote, couldn\'t parse ' + answer) + log.exception('In hinter tally_vote, couldn\'t parse {ans}'.format(ans=answer)) continue try: hint_and_votes.append(temp_dict[answer][str(vote_pk)]) except KeyError: - log.exception('In hinter tally_vote, couldn\'t find: ' - + answer + ', ' + str(vote_pk)) + log.exception('In hinter tally_vote, couldn\'t find: {ans}, {vote_pk}'.format( + ans=answer, vote_pk=str(vote_pk))) hint_and_votes.sort(key=lambda pair: pair[1], reverse=True) # Reset self.previous_answers and user_submissions. @@ -345,7 +346,8 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): hint = escape(data['hint']) answer = data['answer'] if not self.validate_answer(answer): - log.exception('Failure in hinter submit_hint: Unable to parse answer: ' + answer) + log.exception('Failure in hinter submit_hint: Unable to parse answer: {ans}'.format( + ans=answer)) return {'error': 'Could not submit answer'} # Only allow a student to vote or submit a hint once. if self.user_voted: diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index 7263dde1f0..ab64617644 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -247,6 +247,7 @@ class @Problem @updateProgress response else @gentle_alert response.success + Logger.log 'problem_graded', [@answers, response.contents], @url if not abort_submission $.ajaxWithPrefix("#{@url}/problem_check", settings) From 68bb45ab67dc3dee4215e2c166ff898544099cd9 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Tue, 20 Aug 2013 14:44:12 -0400 Subject: [PATCH 24/28] Added jasmine tests to crowdsource_hinter. UI is not covered; only event-inteception is covered. --- .../js/fixtures/crowdsource_hinter.html | 52 ++++++++++++++++++ .../xmodule/js/spec/capa/display_spec.coffee | 12 ++++- .../crowdsource_hinter/display_spec.coffee | 54 +++++++++++++++++++ .../xmodule/js/src/capa/display.coffee | 1 - .../js/src/crowdsource_hinter/display.coffee | 2 +- 5 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 common/lib/xmodule/xmodule/js/fixtures/crowdsource_hinter.html create mode 100644 common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee diff --git a/common/lib/xmodule/xmodule/js/fixtures/crowdsource_hinter.html b/common/lib/xmodule/xmodule/js/fixtures/crowdsource_hinter.html new file mode 100644 index 0000000000..02122017bd --- /dev/null +++ b/common/lib/xmodule/xmodule/js/fixtures/crowdsource_hinter.html @@ -0,0 +1,52 @@ +
  • + + +
    + + +
    +
    + + +

    + Numerical Input +

    + +
    (1/1 points)
    + +
    +

    The answer is 2*x^2*y + 5 +


    Answer = +
    +
    + + +
    + + + +
    + +
    + + + + +
    +
    +
    + +
    + + + +
    + + + + +
    + + + +
  • \ No newline at end of file diff --git a/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee index 33d74e2335..111174a0b4 100644 --- a/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/capa/display_spec.coffee @@ -125,9 +125,10 @@ describe 'Problem', -> expect(@problem.bind).toHaveBeenCalled() describe 'check_fd', -> - xit 'should have specs written for this functionality', -> + xit 'should have more specs written for this functionality', -> expect(false) + describe 'check', -> beforeEach -> @problem = new Problem($('.xmodule_display')) @@ -137,6 +138,15 @@ describe 'Problem', -> @problem.check() expect(Logger.log).toHaveBeenCalledWith 'problem_check', 'foo=1&bar=2' + it 'log the problem_graded event, after the problem is done grading.', -> + spyOn($, 'postWithPrefix').andCallFake (url, answers, callback) -> + response = + success: 'correct' + contents: 'mock grader response' + callback(response) + @problem.check() + expect(Logger.log).toHaveBeenCalledWith 'problem_graded', ['foo=1&bar=2', 'mock grader response'], @problem.url + it 'submit the answer for check', -> spyOn $, 'postWithPrefix' @problem.check() diff --git a/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee new file mode 100644 index 0000000000..917741f8af --- /dev/null +++ b/common/lib/xmodule/xmodule/js/spec/crowdsource_hinter/display_spec.coffee @@ -0,0 +1,54 @@ +describe 'Crowdsourced hinter', -> + beforeEach -> + window.update_schematics = -> + jasmine.stubRequests() + # note that the fixturesPath is set in spec/helper.coffee + loadFixtures 'crowdsource_hinter.html' + @hinter = new Hinter($('#hinter-root')) + + describe 'high-level integration tests', -> + # High-level, happy-path tests for integration with capa problems. + beforeEach -> + # Make a more thorough $.postWithPrefix mock. + spyOn($, 'postWithPrefix').andCallFake( -> + last_argument = arguments[arguments.length - 1] + if typeof last_argument == 'function' + response = + success: 'incorrect' + contents: 'mock grader response' + last_argument(response) + ) + @problem = new Problem($('#problem')) + @problem.bind() + + it 'knows when a capa problem is graded, using check.', -> + @problem.answers = 'test answer' + @problem.check() + expect($.postWithPrefix).toHaveBeenCalledWith("#{@hinter.url}/get_hint", 'test answer', jasmine.any(Function)) + + it 'knows when a capa problem is graded usig check_fd.', -> + spyOn($, 'ajaxWithPrefix').andCallFake((url, settings) -> + response = + success: 'incorrect' + contents: 'mock grader response' + settings.success(response) + ) + @problem.answers = 'test answer' + @problem.check_fd() + expect($.postWithPrefix).toHaveBeenCalledWith("#{@hinter.url}/get_hint", 'test answer', jasmine.any(Function)) + + describe 'capture_problem', -> + beforeEach -> + spyOn($, 'postWithPrefix').andReturn(null) + + it 'gets hints for an incorrect answer', -> + data = ['some answers', ''] + @hinter.capture_problem('problem_graded', data, 'fake element') + expect($.postWithPrefix).toHaveBeenCalledWith("#{@hinter.url}/get_hint", 'some answers', jasmine.any(Function)) + + it 'gets feedback for a correct answer', -> + data = ['some answers', ''] + @hinter.capture_problem('problem_graded', data, 'fake element') + expect($.postWithPrefix).toHaveBeenCalledWith("#{@hinter.url}/get_feedback", 'some answers', jasmine.any(Function)) + + diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.coffee b/common/lib/xmodule/xmodule/js/src/capa/display.coffee index ab64617644..61df101d08 100644 --- a/common/lib/xmodule/xmodule/js/src/capa/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/capa/display.coffee @@ -19,7 +19,6 @@ class @Problem problem_prefix = @element_id.replace(/problem_/,'') @inputs = @$("[id^=input_#{problem_prefix}_]") - @$('section.action input:button').click @refreshAnswers @$('section.action input.check').click @check_fd @$('section.action input.reset').click @reset 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 2e99b107f9..c53a2e2066 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -96,7 +96,7 @@ class @Hinter $(obj).css('margin-top', (viewbox_height - view_height) + 'px') ) - render: (content) => + render: (content) -> if content # Trim leading and trailing whitespace content = content.replace /^\s+|\s+$/g, "" From 1f9eafeedecb164b87963c3ee2b6592220e64d18 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Wed, 21 Aug 2013 15:10:34 -0400 Subject: [PATCH 25/28] Addressed some minor PR comments. --- .../xmodule/js/src/crowdsource_hinter/display.coffee | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) 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 c53a2e2066..f9f3e43826 100644 --- a/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/crowdsource_hinter/display.coffee @@ -28,10 +28,6 @@ class @Hinter $: (selector) -> $(selector, @el) - jq_escape: (string) => - # Escape a string for jquery selector use. - return string.replace(/[!"#$%&'()*+,.\/:;<=>?@\[\\\]^`{|}~]/g, '\\$&') - bind: => @$('input.vote').click @vote @$('input.submit-hint').click @submit_hint @@ -99,7 +95,7 @@ class @Hinter render: (content) -> if content # Trim leading and trailing whitespace - content = content.replace /^\s+|\s+$/g, "" + content = content.trim() if content @el.html(content) From 74ed2dafb3e07ed085e920eb4eba2d3024846b22 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Fri, 23 Aug 2013 10:19:36 -0400 Subject: [PATCH 26/28] Improved docstrings. --- .../lib/xmodule/xmodule/crowdsource_hinter.py | 7 +++++-- lms/djangoapps/instructor/hint_manager.py | 19 ++++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index 90699beda6..f2d21c459b 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -182,9 +182,10 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Args: `data` -- must be interpretable by 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 `data`. + - 'hints' is a list of hint strings to show to the user. - 'answer' is the parsed answer that was submitted. + Will record the user's wrong answer in user_submissions, and the hints shown + in previous_answers. """ # First, validate our inputs. try: @@ -251,6 +252,8 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): Output keys: - 'answer_to_hints': a nested dictionary. answer_to_hints[answer][hint_pk] returns the text of the hint. + - 'user_submissions': the same thing as self.user_submissions. A list of + the answers that the user previously submitted. """ # The student got it right. # Did he submit at least one wrong answer? diff --git a/lms/djangoapps/instructor/hint_manager.py b/lms/djangoapps/instructor/hint_manager.py index 3616445543..71c2beb7d5 100644 --- a/lms/djangoapps/instructor/hint_manager.py +++ b/lms/djangoapps/instructor/hint_manager.py @@ -1,8 +1,10 @@ """ Views for hint management. -Along with the crowdsource_hinter xmodule, this code is still -experimental, and should not be used in new courses, yet. +Get to these views through courseurl/hint_manager. +For example: https://courses.edx.org/courses/MITx/2.01x/2013_Spring/hint_manager + +These views will only be visible if MITX_FEATURES['ENABLE_HINTER_INSTRUCTOR_VIEW'] = True """ import json @@ -23,6 +25,9 @@ from xmodule.modulestore.django import modulestore @ensure_csrf_cookie def hint_manager(request, course_id): + """ + The URL landing function for all calls to the hint manager, both POST and GET. + """ try: get_course_with_access(request.user, course_id, 'staff', depth=None) except Http404: @@ -172,7 +177,13 @@ def change_votes(request, course_id, field): Updates the number of votes. 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. + See `delete_hints`. + + Example `request.POST`: + {'op': 'delete_hints', + 'field': 'mod_queue', + 1: ['problem_whatever', '42.0', '3', 42], + 2: ['problem_whatever', '32.5', '12', 9001]} """ for key in request.POST: @@ -233,6 +244,8 @@ def approve(request, course_id, field): hint list. POST: op, field (some number) -> [problem, answer, pk] + + The numbered fields are analogous to those in `delete_hints` and `change_votes`. """ for key in request.POST: From 6e64e994f66138533b844c4def9755140b95ef74 Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Mon, 26 Aug 2013 09:46:29 -0400 Subject: [PATCH 27/28] Fixed a test broken when the mixed modulestore was introduced. --- lms/djangoapps/instructor/hint_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/hint_manager.py b/lms/djangoapps/instructor/hint_manager.py index 71c2beb7d5..f97115f19b 100644 --- a/lms/djangoapps/instructor/hint_manager.py +++ b/lms/djangoapps/instructor/hint_manager.py @@ -215,7 +215,7 @@ def add_hint(request, course_id, field): # Validate the answer. This requires initializing the xmodules, which # is annoying. loc = Location(problem_id) - descriptors = modulestore().get_items(loc) + descriptors = modulestore().get_items(loc, course_id=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) if not hinter_module.validate_answer(answer): From 444f51d6de684d7f40c388d16e02b00a69e5391c Mon Sep 17 00:00:00 2001 From: Felix Sun Date: Tue, 27 Aug 2013 09:41:29 -0400 Subject: [PATCH 28/28] Fixed some pep/pylint violations. --- common/lib/capa/capa/responsetypes.py | 12 +++++------- common/lib/xmodule/xmodule/crowdsource_hinter.py | 4 ++-- .../xmodule/xmodule/tests/test_crowdsource_hinter.py | 8 +++++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 550042d1df..b53f38fd90 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -915,16 +915,14 @@ class NumericalResponse(LoncapaResponse): else: return CorrectMap(self.answer_id, 'incorrect') - # TODO: add check_hint_condition(self, hxml_set, student_answers) - - def compare_answer(self, a, b): + def compare_answer(self, ans1, ans2): """ Outside-facing function that lets us compare two numerical answers, with this problem's tolerance. """ return compare_with_tolerance( - evaluator({}, {}, a), - evaluator({}, {}, b), + evaluator({}, {}, ans1), + evaluator({}, {}, ans2), self.tolerance ) @@ -1886,11 +1884,11 @@ class FormulaResponse(LoncapaResponse): else: return "incorrect" - def compare_answer(self, a, b): + def compare_answer(self, ans1, ans2): """ An external interface for comparing whether a and b are equal. """ - internal_result = self.check_formula(a, b, self.samples) + internal_result = self.check_formula(ans1, ans2, self.samples) return internal_result == "correct" def validate_answer(self, answer): diff --git a/common/lib/xmodule/xmodule/crowdsource_hinter.py b/common/lib/xmodule/xmodule/crowdsource_hinter.py index f2d21c459b..7e538efa24 100644 --- a/common/lib/xmodule/xmodule/crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/crowdsource_hinter.py @@ -229,12 +229,12 @@ class CrowdsourceHinterModule(CrowdsourceHinterFields, XModule): # The brackets surrounding the index are for backwards compatability purposes. # (It used to be that each answer was paired with multiple hints in a list.) self.previous_answers += [[best_hint_answer, [best_hint_index]]] - for i in xrange(min(2, n_hints - 1)): + for _ in xrange(min(2, n_hints - 1)): # Keep making random hints until we hit a target, or run out. while True: # random.choice randomly chooses an element from its input list. # (We then unpack the item, in this case data for a hint.) - (hint_index, (rand_hint, votes, hint_answer)) =\ + (hint_index, (rand_hint, _, hint_answer)) =\ random.choice(matching_hints.items()) if rand_hint not in hints: break diff --git a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py index d28d2ee06b..8347b71076 100644 --- a/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py +++ b/common/lib/xmodule/xmodule/tests/test_crowdsource_hinter.py @@ -120,9 +120,9 @@ class CHModuleFactory(object): return False responder.validate_answer = validate_answer - def compare_answer(a, b): + def compare_answer(ans1, ans2): """ A fake answer comparer """ - return a == b + return ans1 == ans2 responder.compare_answer = compare_answer capa_module.lcp.responders = {'responder0': responder} @@ -189,11 +189,13 @@ class VerticalWithModulesFactory(object): @staticmethod def next_num(): + """Increments a global counter for naming.""" CHModuleFactory.num += 1 return CHModuleFactory.num @staticmethod def create(): + """Make a vertical.""" model_data = {'data': VerticalWithModulesFactory.sample_problem_xml} system = get_test_system() descriptor = VerticalDescriptor.from_xml(VerticalWithModulesFactory.sample_problem_xml, system) @@ -532,7 +534,7 @@ class CrowdsourceHinterTest(unittest.TestCase): """ mock_module = CHModuleFactory.create() - def fake_get_hint(get): + def fake_get_hint(_): """ Creates a rendering dictionary, with which we can test the templates.