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

@@ -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;
     }
 

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'), '/') . '/');
+    }
 }
 
 /**

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)) {

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);

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 "<strong>{$a}</strong>" does not contain any question';
 $string['pleaseananswerallparts'] = 'Please answer all parts of the question.';

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

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

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 = '<span class="nolink">' . $token . '</span>';
                 $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);
         }

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);
+    }
 }

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(),
+        );
+    }
 }

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(