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/question_type_test.php b/question/type/multianswer/tests/question_type_test.php index a25b5ccce2b..d36a13e85a4 100644 --- a/question/type/multianswer/tests/question_type_test.php +++ b/question/type/multianswer/tests/question_type_test.php @@ -20,6 +20,7 @@ use qtype_multianswer; use qtype_multianswer_edit_form; use qtype_multichoice_base; use question_bank; +use test_question_maker; defined('MOODLE_INTERNAL') || die(); @@ -360,4 +361,43 @@ class question_type_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 02ba5df4068..aaf525f7f18 100644 --- a/question/type/multianswer/tests/walkthrough_test.php +++ b/question/type/multianswer/tests/walkthrough_test.php @@ -16,14 +16,18 @@ namespace qtype_multianswer; +use qtype_multianswer; +use question_bank; use question_display_options; use question_hint_with_parts; use question_state; +use test_question_maker; defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->dirroot . '/question/engine/tests/helpers.php'); +require_once($CFG->dirroot . '/question/type/multianswer/questiontype.php'); /** @@ -530,4 +534,43 @@ class walkthrough_test extends \qbehaviour_walkthrough_test_base { $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(