diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 54c4d9f8390..1be7df264b0 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1213,6 +1213,15 @@ class question_attempt { $qa->behaviour = question_engine::make_behaviour( $record->behaviour, $qa, $preferredbehaviour); + // If attemptstepid is null (which should not happen, but has happened + // due to corrupt data, see MDL-34251) then the current pointer in $records + // will not be advanced in the while loop below, and we get stuck in an + // infinite loop, since this method is supposed to always consume at + // least one record. Therefore, in this case, advance the record here. + if (is_null($record->attemptstepid)) { + $records->next(); + } + $i = 0; while ($record && $record->questionattemptid == $questionattemptid && !is_null($record->attemptstepid)) { $qa->steps[$i] = question_attempt_step::load_from_records($records, $record->attemptstepid); diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index 8d7ffa92096..f9470c0060e 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -714,6 +714,14 @@ class question_usage_by_activity { $quba->observer = new question_engine_unit_of_work($quba); + // If slot is null then the current pointer in $records will not be + // advanced in the while loop below, and we get stuck in an infinite loop, + // since this method is supposed to always consume at least one record. + // Therefore, in this case, advance the record here. + if (is_null($record->slot)) { + $records->next(); + } + while ($record && $record->qubaid == $qubaid && !is_null($record->slot)) { $quba->questionattempts[$record->slot] = question_attempt::load_from_records($records, diff --git a/question/engine/tests/questionusagebyactivity_test.php b/question/engine/tests/questionusagebyactivity_test.php index 73ff8b30de8..71434ee5270 100644 --- a/question/engine/tests/questionusagebyactivity_test.php +++ b/question/engine/tests/questionusagebyactivity_test.php @@ -169,16 +169,17 @@ class question_usage_by_activity_test extends advanced_testcase { */ class question_usage_db_test extends data_loading_method_test_base { public function test_load() { + $scid = context_system::instance()->id; $records = new question_test_recordset(array( array('qubaid', 'contextid', 'component', 'preferredbehaviour', - 'questionattemptid', 'contextid', 'questionusageid', 'slot', + 'questionattemptid', 'questionusageid', 'slot', 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged', 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified', 'attemptstepid', 'sequencenumber', 'state', 'fraction', 'timecreated', 'userid', 'name', 'value'), - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null), - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'), - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'), + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null), + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'), + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'), )); $question = test_question_maker::make_question('truefalse', 'true'); @@ -221,4 +222,63 @@ class question_usage_db_test extends data_loading_method_test_base { $this->assertEquals(1, $step->get_user_id()); $this->assertEquals(array('-finish' => '1'), $step->get_all_data()); } + + public function test_load_data_no_steps() { + // The code had a bug where if one question_attempt had no steps, + // load_from_records got stuck in an infinite loop. This test is to + // verify that no longer happens. + $scid = context_system::instance()->id; + $records = new question_test_recordset(array( + array('qubaid', 'contextid', 'component', 'preferredbehaviour', + 'questionattemptid', 'questionusageid', 'slot', + 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged', + 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified', + 'attemptstepid', 'sequencenumber', 'state', 'fraction', + 'timecreated', 'userid', 'name', 'value'), + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null), + array(1, $scid, 'unit_test', 'interactive', 2, 1, 2, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null), + array(1, $scid, 'unit_test', 'interactive', 3, 1, 3, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null), + )); + + question_bank::start_unit_test(); + $quba = question_usage_by_activity::load_from_records($records, 1); + question_bank::end_unit_test(); + + $this->assertEquals('unit_test', $quba->get_owning_component()); + $this->assertEquals(1, $quba->get_id()); + $this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer()); + $this->assertEquals('interactive', $quba->get_preferred_behaviour()); + + $this->assertEquals(array(1, 2, 3), $quba->get_slots()); + + $qa = $quba->get_question_attempt(1); + $this->assertEquals(0, $qa->get_num_steps()); + } + + public function test_load_data_no_qas() { + // The code had a bug where if a question_usage had no question_attempts, + // load_from_records got stuck in an infinite loop. This test is to + // verify that no longer happens. + $scid = context_system::instance()->id; + $records = new question_test_recordset(array( + array('qubaid', 'contextid', 'component', 'preferredbehaviour', + 'questionattemptid', 'questionusageid', 'slot', + 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged', + 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified', + 'attemptstepid', 'sequencenumber', 'state', 'fraction', + 'timecreated', 'userid', 'name', 'value'), + array(1, $scid, 'unit_test', 'interactive', null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null), + )); + + question_bank::start_unit_test(); + $quba = question_usage_by_activity::load_from_records($records, 1); + question_bank::end_unit_test(); + + $this->assertEquals('unit_test', $quba->get_owning_component()); + $this->assertEquals(1, $quba->get_id()); + $this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer()); + $this->assertEquals('interactive', $quba->get_preferred_behaviour()); + + $this->assertEquals(array(), $quba->get_slots()); + } }