diff --git a/xmodule/capa_block.py b/xmodule/capa_block.py index 958ca1354142..0406de536f13 100644 --- a/xmodule/capa_block.py +++ b/xmodule/capa_block.py @@ -1752,7 +1752,6 @@ def submit_problem( # pylint: disable=too-many-statements,too-many-branches,too self.lcp.has_saved_answers = False answers = self.make_dict_of_responses(data) answers_without_files = convert_files_to_filenames(answers) - self.student_answers_history.append(answers_without_files) event_info["answers"] = answers_without_files # Can override current time @@ -1812,6 +1811,9 @@ def submit_problem( # pylint: disable=too-many-statements,too-many-branches,too # self.lcp.context['attempt'] refers to the attempt number (1-based) self.lcp.context["attempt"] = self.attempts + 1 correct_map = self.lcp.grade_answers(answers) + # recorded only once grading has succeeded, to stay aligned with the + # correct map history that grade_answers appends to + self.student_answers_history.append(answers_without_files) # self.attempts refers to the number of attempts that did not # raise an error (0-based) self.attempts = self.attempts + 1 diff --git a/xmodule/tests/test_capa_block.py b/xmodule/tests/test_capa_block.py index 67e656fcdd1f..32a467de7578 100644 --- a/xmodule/tests/test_capa_block.py +++ b/xmodule/tests/test_capa_block.py @@ -1367,6 +1367,48 @@ def test_submit_problem_error(self): # but that this was considered attempt number 2 for grading purposes assert block.lcp.context["attempt"] == 2 + def test_submit_problem_error_does_not_record_answer_history(self): + """ + Verify that a submission that fails to grade is not recorded in `student_answers_history`, which keeps it + aligned with `correct_map_history`. + """ + exception_classes = [StudentInputError, LoncapaProblemError, ResponseError] + for exception_class in exception_classes: + block = CapaFactory.create(attempts=1, user_is_staff=False) + + with patch('xblocks_contrib.problem.capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade: + mock_grade.side_effect = exception_class('test error') + + get_request_dict = {CapaFactory.input_key(): '3.14'} + block.submit_problem(get_request_dict) + + assert not block.student_answers_history, \ + f"student_answers_history must stay empty when grading raises {exception_class.__name__}" + assert len(block.student_answers_history) == len(block.correct_map_history), ( + "student_answers_history and correct_map_history must stay the same length " + f"when grading raises {exception_class.__name__}" + ) + + def test_answer_and_correct_map_histories_stay_aligned(self): + """ + Verify that `student_answers_history` and `correct_map_history` hold one entry per graded attempt, so + an attempt that fails to grade does not misalign the two lists for the attempts that follow it. + """ + block = CapaFactory.create(attempts=0, max_attempts=3, rerandomize=RANDOMIZATION.NEVER) + + block.submit_problem({CapaFactory.input_key(): '3.14'}) + + with patch('xblocks_contrib.problem.capa.capa_problem.LoncapaProblem.grade_answers') as mock_grade: + mock_grade.side_effect = StudentInputError('test error') + block.submit_problem({CapaFactory.input_key(): 'not a number'}) + + block.submit_problem({CapaFactory.input_key(): '3.21'}) + + assert len(block.student_answers_history) == len(block.correct_map_history) == 2, \ + "both histories must hold one entry per graded attempt, excluding the attempt that failed to grade" + assert block.student_answers_history[-1] == {CapaFactory.answer_key(): '3.21'}, \ + "the last entry of student_answers_history must be the answers of the last graded attempt" + def test_submit_problem_error_with_codejail_exception(self): """Verify codejail execution errors are sanitized and handled correctly.""" diff --git a/xmodule/tests/test_delay_between_attempts.py b/xmodule/tests/test_delay_between_attempts.py index bc2d074df91b..61d6a86d32b0 100644 --- a/xmodule/tests/test_delay_between_attempts.py +++ b/xmodule/tests/test_delay_between_attempts.py @@ -198,6 +198,26 @@ def test_submit_quiz_too_soon(self): # Also, the number of attempts should not be incremented self.assertRegex(result['success'], r"You must wait at least 3 minutes between submissions. 2 minutes remaining\..*") # pylint: disable=line-too-long # noqa: PT009 assert block.attempts == num_attempts + assert not block.student_answers_history, \ + "student_answers_history must stay empty when a submission arrives before the wait period elapsed" + + def test_no_answer_history_when_submitted_too_soon(self): + """ + Verify that a submission rejected for arriving before the wait period + elapsed is not recorded in `student_answers_history`. + """ + num_attempts = 1 + (block, result) = self.create_and_check( + num_attempts=num_attempts, + last_submission_time=datetime.datetime(2013, 12, 6, 0, 17, 36, tzinfo=UTC), + submission_wait_seconds=180, + considered_now=datetime.datetime(2013, 12, 6, 0, 18, 36, tzinfo=UTC) + ) + self.assertRegex(result['success'], r"You must wait at least.*") + assert block.attempts == num_attempts, \ + "attempts must not change when a submission arrives before the wait period elapsed" + assert block.student_answers_history == [], \ + "student_answers_history must stay empty when a submission arrives before the wait period elapsed" def test_submit_quiz_1_second_too_soon(self): # Already attempted once (just now)