From 53d3843955d681443d20a27a1ebaca53cdc5355c Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Fri, 11 Feb 2022 11:08:29 +0800 Subject: [PATCH] MDL-54724 qtype_multianswer: Better handle corrupted questions When questions are deleted, we now check if the parent is in use before deletion. Prior to this, it would be possible for multianswer questions to reference questions that have been deleted. This results in fatal errors when the quiz is viewed. This patch uses a dummy 'subquestion_replacement' to handle this case and display some information to the end user about what has happened so that they may take action to repair the corrupted question. As a result of the bug described above, the sequence column of mdl_question_multianswer can contiain references to questions that no longer exist, and these IDs can make their way in to backups. When this happens, the backups cannot be restored. To avoid this, this patch skips trying to restore those questions that reference question IDs that no longer exist (as there is no way to recover them). --- lib/questionlib.php | 8 +- question/engine/tests/helpers.php | 18 ++++ ...restore_qtype_multianswer_plugin.class.php | 2 +- .../multianswer/edit_multianswer_form.php | 5 +- .../multianswer/lang/en/qtype_multianswer.php | 2 + question/type/multianswer/question.php | 4 +- question/type/multianswer/questiontype.php | 92 +++++++++++++++++-- question/type/multianswer/renderer.php | 28 +++++- .../multianswer/tests/questiontype_test.php | 39 ++++++++ .../multianswer/tests/walkthrough_test.php | 39 ++++++++ question/type/multianswer/version.php | 2 +- 11 files changed, 225 insertions(+), 14 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index 8cb54e258b6..e578d7adbf1 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -354,8 +354,14 @@ function question_delete_question($questionid): void { WHERE q.id = ?'; $questiondata = $DB->get_record_sql($sql, [$question->id]); + $questionstocheck = [$question->id]; + + if ($question->parent !== 0) { + $questionstocheck[] = $question->parent; + } + // Do not delete a question if it is used by an activity module - if (questions_in_use([$question->id])) { + if (questions_in_use($questionstocheck)) { return; } diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 59a8b50f846..b3865a4dec8 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -1301,6 +1301,24 @@ abstract class qbehaviour_walkthrough_test_base extends question_testcase { // Does not currently verify hint text. return new question_contains_tag_with_attribute('div', 'class', 'hint'); } + + /** + * Returns an expectation that a string contains a corrupted question notification. + * + * @return question_pattern_expectation an expectation for use with check_current_output. + */ + protected function get_contains_corruption_notification() { + return new question_pattern_expectation('/' . preg_quote(get_string('corruptedquestion', 'qtype_multianswer'), '/') . '/'); + } + + /** + * Returns an expectation that a string contains a corrupted subquestion message. + * + * @return question_pattern_expectation an expectation for use with check_current_output. + */ + protected function get_contains_corrupted_subquestion_message() { + return new question_pattern_expectation('/' . preg_quote(get_string('missingsubquestion', 'qtype_multianswer'), '/') . '/'); + } } /** diff --git a/question/type/multianswer/backup/moodle2/restore_qtype_multianswer_plugin.class.php b/question/type/multianswer/backup/moodle2/restore_qtype_multianswer_plugin.class.php index 80865b11523..f51ec0272f8 100644 --- a/question/type/multianswer/backup/moodle2/restore_qtype_multianswer_plugin.class.php +++ b/question/type/multianswer/backup/moodle2/restore_qtype_multianswer_plugin.class.php @@ -110,7 +110,7 @@ class restore_qtype_multianswer_plugin extends restore_qtype_plugin { foreach ($sequencearr as $key => $question) { $sequencearr[$key] = $this->get_mappingid('question', $question); } - $sequence = implode(',', $sequencearr); + $sequence = implode(',', array_filter($sequencearr)); $DB->set_field('question_multianswer', 'sequence', $sequence, array('id' => $rec->id)); if (!empty($sequence)) { diff --git a/question/type/multianswer/edit_multianswer_form.php b/question/type/multianswer/edit_multianswer_form.php index 58a8f44349d..02a625cea4d 100644 --- a/question/type/multianswer/edit_multianswer_form.php +++ b/question/type/multianswer/edit_multianswer_form.php @@ -150,7 +150,8 @@ class qtype_multianswer_edit_form extends question_edit_form { $storemess = ''; if (isset($this->savedquestiondisplay->options->questions[$sub]->qtype) && $this->savedquestiondisplay->options->questions[$sub]->qtype != - $this->questiondisplay->options->questions[$sub]->qtype) { + $this->questiondisplay->options->questions[$sub]->qtype && + $this->savedquestiondisplay->options->questions[$sub]->qtype != 'subquestion_replacement') { $this->qtypechange = true; $storemess = ' ' . html_writer::tag('span', get_string( 'storedqtype', 'qtype_multianswer', question_bank::get_qtype_name( @@ -276,6 +277,8 @@ class qtype_multianswer_edit_form extends question_edit_form { case 'numerical': $parsableanswerdef .= 'NUMERICAL:'; break; + case 'subquestion_replacement': + continue 2; default: print_error('unknownquestiontype', 'question', '', $wrapped->qtype); diff --git a/question/type/multianswer/lang/en/qtype_multianswer.php b/question/type/multianswer/lang/en/qtype_multianswer.php index b51660ff1f3..ba4f575d18c 100644 --- a/question/type/multianswer/lang/en/qtype_multianswer.php +++ b/question/type/multianswer/lang/en/qtype_multianswer.php @@ -27,6 +27,7 @@ $string['confirmquestionsaveasedited'] = 'I confirm that I want the question to $string['confirmsave'] = 'Confirm then save {$a}'; $string['correctanswer'] = 'Correct answer'; $string['correctanswerandfeedback'] = 'Correct answer and feedback'; +$string['corruptedquestion'] = 'This question is corrupted and contains subquestions that are not present in your system.'; $string['decodeverifyquestiontext'] = 'Decode and verify the question text'; $string['invalidmultianswerquestion'] = 'Invalid embedded answers (Cloze) question ({$a}).'; $string['layout'] = 'Layout'; @@ -36,6 +37,7 @@ $string['layoutmultiple_vertical'] = 'Vertical column of checkboxes'; $string['layoutselectinline'] = 'Drop-down menu in-line in the text'; $string['layoutundefined'] = 'Undefined layout'; $string['layoutvertical'] = 'Vertical column of radio buttons'; +$string['missingsubquestion'] = 'This subquestion is missing from your system and cannot be displayed.'; $string['nooptionsforsubquestion'] = 'Unable to get options for question part # {$a->sub} (question->id={$a->id})'; $string['noquestions'] = 'The Cloze(multianswer) question "{$a}" does not contain any question'; $string['pleaseananswerallparts'] = 'Please answer all parts of the question.'; diff --git a/question/type/multianswer/question.php b/question/type/multianswer/question.php index de5522455a6..2910fca66d9 100644 --- a/question/type/multianswer/question.php +++ b/question/type/multianswer/question.php @@ -110,7 +110,7 @@ class qtype_multianswer_question extends question_graded_automatically_with_coun $fractionmax += $subq->defaultmark; $fractionsum += $subq->defaultmark * $subq->get_min_fraction(); } - return $fractionsum / $fractionmax; + return $fractionsum / (!empty($this->subquestions) ? $fractionmax : 1); } public function get_max_fraction() { @@ -120,7 +120,7 @@ class qtype_multianswer_question extends question_graded_automatically_with_coun $fractionmax += $subq->defaultmark; $fractionsum += $subq->defaultmark * $subq->get_max_fraction(); } - return $fractionsum / $fractionmax; + return $fractionsum / (!empty($this->subquestions) ? $fractionmax : 1); } public function get_expected_data() { diff --git a/question/type/multianswer/questiontype.php b/question/type/multianswer/questiontype.php index 07462f07c24..8e511ed474b 100644 --- a/question/type/multianswer/questiontype.php +++ b/question/type/multianswer/questiontype.php @@ -38,18 +38,86 @@ require_once($CFG->dirroot . '/question/type/numerical/questiontype.php'); */ class qtype_multianswer extends question_type { + /** + * Generate a subquestion replacement question class. + * + * Due to a bug, subquestions can be lost (see MDL-54724). This class exists to take + * the place of those lost questions so that the system can keep working and inform + * the user of the corrupted data. + * + * @return question_automatically_gradable The replacement question class. + */ + public static function deleted_subquestion_replacement(): question_automatically_gradable { + return new class implements question_automatically_gradable { + public $qtype; + + public function __construct() { + $this->qtype = new class() { + public function name() { + return 'subquestion_replacement'; + } + }; + } + + public function is_gradable_response(array $response) { + return false; + } + + public function is_complete_response(array $response) { + return false; + } + + public function is_same_response(array $prevresponse, array $newresponse) { + return false; + } + + public function summarise_response(array $response) { + return ''; + } + + public function un_summarise_response(string $summary) { + return []; + } + + public function classify_response(array $response) { + return []; + } + + public function get_validation_error(array $response) { + return ''; + } + + public function grade_response(array $response) { + return []; + } + + public function get_hint($hintnumber, question_attempt $qa) { + return; + } + + public function get_right_answer_summary() { + return null; + } + }; + } + public function can_analyse_responses() { return false; } public function get_question_options($question) { - global $DB, $OUTPUT; + global $DB; parent::get_question_options($question); // Get relevant data indexed by positionkey from the multianswers table. $sequence = $DB->get_field('question_multianswer', 'sequence', array('question' => $question->id), MUST_EXIST); + if (empty($sequence)) { + $question->options->questions = []; + return true; + } + $wrappedquestions = $DB->get_records_list('question', 'id', explode(',', $sequence), 'id ASC'); @@ -59,13 +127,19 @@ class qtype_multianswer extends question_type { $val++; }); - // If a question is lost, the corresponding index is null - // so this null convention is used to test $question->options->questions - // before using the values. - // First all possible questions from sequence are nulled - // then filled with the data if available in $wrappedquestions. + // Due to a bug, questions can be lost (see MDL-54724). So we first fill the question + // options with this dummy "replacement" type. These are overridden in the loop below + // leaving behind only those questions which no longer exist. The renderer then looks + // for this deleted type to display information to the user about the corrupted question + // data. foreach ($sequence as $seq) { - $question->options->questions[$seq] = ''; + $question->options->questions[$seq] = (object)[ + 'qtype' => 'subquestion_replacement', + 'defaultmark' => 1, + 'options' => (object)[ + 'answers' => [] + ] + ]; } foreach ($wrappedquestions as $wrapped) { @@ -216,6 +290,10 @@ class qtype_multianswer extends question_type { $i += 1; } foreach ($questiondata->options->questions as $key => $subqdata) { + if ($subqdata->qtype == 'subquestion_replacement') { + continue; + } + $subqdata->contextid = $questiondata->contextid; if ($subqdata->qtype == 'multichoice') { $answerregs = array(); diff --git a/question/type/multianswer/renderer.php b/question/type/multianswer/renderer.php index 5993da13f02..27c31e9a6fd 100644 --- a/question/type/multianswer/renderer.php +++ b/question/type/multianswer/renderer.php @@ -46,17 +46,38 @@ class qtype_multianswer_renderer extends qtype_renderer { $output = ''; $subquestions = array(); + + $missingsubquestions = false; foreach ($question->textfragments as $i => $fragment) { if ($i > 0) { $index = $question->places[$i]; + $questionisvalid = !empty($question->subquestions[$index]) && + $question->subquestions[$index]->qtype->name() !== 'subquestion_replacement'; + + if (!$questionisvalid) { + $missingsubquestions = true; + $questionreplacement = qtype_multianswer::deleted_subquestion_replacement(); + + // It is possible that the subquestion index does not exist. When corrupted quizzes (see MDL-54724) are + // restored, the sequence column of mdl_quiz_multianswer can be empty, in this case + // qtype_multianswer::get_question_options cannot fill in deleted questions, so we need to do it here. + $question->subquestions[$index] = $question->subquestions[$index] ?? $questionreplacement; + } + $token = 'qtypemultianswer' . $i . 'marker'; $token = '' . $token . ''; $output .= $token; $subquestions[$token] = $this->subquestion($qa, $options, $index, $question->subquestions[$index]); } + $output .= $fragment; } + + if ($missingsubquestions) { + $output = $this->notification(get_string('corruptedquestion', 'qtype_multianswer'), 'error') . $output; + } + $output = $question->format_text($output, $question->questiontextformat, $qa, 'question', 'questiontext', $question->id); $output = str_replace(array_keys($subquestions), array_values($subquestions), $output); @@ -78,7 +99,7 @@ class qtype_multianswer_renderer extends qtype_renderer { } public function subquestion(question_attempt $qa, - question_display_options $options, $index, question_graded_automatically $subq) { + question_display_options $options, $index, question_automatically_gradable $subq) { $subtype = $subq->qtype->name(); if ($subtype == 'numerical' || $subtype == 'shortanswer') { @@ -99,6 +120,11 @@ class qtype_multianswer_renderer extends qtype_renderer { $subrenderer = 'multichoice_vertical'; } } + } else if ($subtype == 'subquestion_replacement') { + return html_writer::div( + get_string('missingsubquestion', 'qtype_multianswer'), + 'notifyproblem' + ); } else { throw new coding_exception('Unexpected subquestion type.', $subq); } diff --git a/question/type/multianswer/tests/questiontype_test.php b/question/type/multianswer/tests/questiontype_test.php index 77c96878481..a87f66ad1c9 100644 --- a/question/type/multianswer/tests/questiontype_test.php +++ b/question/type/multianswer/tests/questiontype_test.php @@ -362,4 +362,43 @@ class qtype_multianswer_test extends advanced_testcase { } } } + + /** + * Test get_question_options. + * + * @covers \qtype_multianswer::get_question_options + */ + public function test_get_question_options() { + global $DB; + + $this->resetAfterTest(true); + + $syscontext = context_system::instance(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(['contextid' => $syscontext->id]); + + $fromform = test_question_maker::get_question_form_data('multianswer', 'twosubq'); + $fromform->category = $category->id . ',' . $syscontext->id; + + $question = new stdClass(); + $question->category = $category->id; + $question->qtype = 'multianswer'; + $question->createdby = 0; + + $question = $this->qtype->save_question($question, $fromform); + $questiondata = question_bank::load_question_data($question->id); + + $questiontodeletekey = array_keys($questiondata->options->questions)[0]; + $questiontodelete = $questiondata->options->questions[$questiontodeletekey]; + + $this->assertCount(2, $questiondata->options->questions); + $this->assertEquals('shortanswer', $questiondata->options->questions[$questiontodeletekey]->qtype); + + // Forcibly delete a subquestion to ensure get_question_options replaces it. + $DB->delete_records('question', ['id' => $questiontodelete->id]); + $this->qtype->get_question_options($questiondata); + + $this->assertCount(2, $questiondata->options->questions); + $this->assertEquals('subquestion_replacement', $questiondata->options->questions[$questiontodeletekey]->qtype); + } } diff --git a/question/type/multianswer/tests/walkthrough_test.php b/question/type/multianswer/tests/walkthrough_test.php index 57e22329b1e..dcdfa02d05a 100644 --- a/question/type/multianswer/tests/walkthrough_test.php +++ b/question/type/multianswer/tests/walkthrough_test.php @@ -533,4 +533,43 @@ class qtype_multianswer_walkthrough_test extends qbehaviour_walkthrough_test_bas $this->get_contains_partcorrect_expectation(), $this->get_does_not_contain_validation_error_expectation()); } + + /** + * Test corrupted question display. + * + * @covers \qtype_multianswer_renderer::subquestion + */ + public function test_corrupted_question() { + global $DB; + + $syscontext = context_system::instance(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $category = $generator->create_question_category(['contextid' => $syscontext->id]); + + $fromform = test_question_maker::get_question_form_data('multianswer', 'twosubq'); + $fromform->category = $category->id . ',' . $syscontext->id; + + $question = new stdClass(); + $question->category = $category->id; + $question->qtype = 'multianswer'; + $question->createdby = 0; + + $question = (new qtype_multianswer)->save_question($question, $fromform); + $questiondata = question_bank::load_question_data($question->id); + $questiontodeletekey = array_keys($questiondata->options->questions)[0]; + $questiontodelete = $questiondata->options->questions[$questiontodeletekey]; + $DB->delete_records('question', ['id' => $questiontodelete->id]); + + question_bank::notify_question_edited($question->id); + $question = question_bank::load_question($question->id); + + $this->start_attempt_at_question($question, 'deferredfeedback', 2); + $this->check_current_output( + $this->get_contains_marked_out_of_summary(), + $this->get_does_not_contain_feedback_expectation(), + $this->get_does_not_contain_validation_error_expectation(), + $this->get_contains_corruption_notification(), + $this->get_contains_corrupted_subquestion_message(), + ); + } } diff --git a/question/type/multianswer/version.php b/question/type/multianswer/version.php index 9cdfc75d372..e15bd438a9e 100644 --- a/question/type/multianswer/version.php +++ b/question/type/multianswer/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'qtype_multianswer'; -$plugin->version = 2021052500; +$plugin->version = 2021052500.01; $plugin->requires = 2021052500; $plugin->dependencies = array(