From 21cca9f28a7d40baf9a858710281693d51b82716 Mon Sep 17 00:00:00 2001
From: Benjamin Abraham <benjamin.abraham@unige.ch>
Date: Fri, 16 Feb 2024 16:18:21 +0100
Subject: [PATCH] MDL-79316 Deleting Draft questions can break quiz

On the missing question placeholder using time() for the prepend part of questionid so it doesn't convert to a string.
Removed the exclusion of draft status from the question with references query to take them into account.
Additional check on the question capability to take into account missing questions.
Unit test
---
 lib/questionlib.php                           |  7 ++++
 lib/tests/questionlib_test.php                | 33 +++++++++++++++++++
 .../classes/question/bank/qbank_helper.php    |  2 +-
 .../classes/question_reference_manager.php    |  3 +-
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/lib/questionlib.php b/lib/questionlib.php
index b292ee0639256..d07c1de2144cc 100644
--- a/lib/questionlib.php
+++ b/lib/questionlib.php
@@ -1466,6 +1466,13 @@ function question_has_capability_on($questionorid, $cap, $notused = -1): bool {
 
             // Well, at least we tried. Seems that we really have to read from DB.
             $question = $DB->get_record_sql($sql, ['id' => $questionid]);
+
+            // The question does not exist, we return false here to exit
+            // this function and be able to treat it as a missing question
+            // further down the line.
+            if ($question === false) {
+                return false;
+            }
         }
     }
 
diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php
index 7c6cc9c98fcbc..8ce28f10f44b9 100644
--- a/lib/tests/questionlib_test.php
+++ b/lib/tests/questionlib_test.php
@@ -298,6 +298,39 @@ public function test_question_delete_question() {
         $this->assertTrue($DB->record_exists('question', ['id' => $q2->id]));
     }
 
+    /**
+     * Test that a question with a 'draft' status used in a quiz
+     * is hidden instead of deleted.
+     *
+     * @covers ::question_delete_question
+     */
+    public function test_question_delete_question_used_draft() {
+        global $DB;
+        $this->resetAfterTest(true);
+
+        list($category, $course, $quiz, $qcat, $questions) = $this->setup_quiz_and_questions();
+
+        $q1 = $questions[0];
+
+        // Set the status of the question to 'draft'.
+        $DB->set_field(
+            'question_versions',
+            'status',
+            \core_question\local\bank\question_version_status::QUESTION_STATUS_DRAFT,
+            ['questionid' => $q1->id]
+        );
+
+        // Do.
+        question_delete_question($q1->id);
+
+        // Verify that the question is still existing and that the version status is 'hidden'.
+        $this->assertTrue($DB->record_exists('question', ['id' => $q1->id]));
+        $this->assertEquals(
+            $DB->get_record('question_versions', ['questionid' => $q1->id])->status,
+            \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN
+        );
+    }
+
     /**
      * Test that deleting a broken question from the question bank does not cause fatal errors.
      */
diff --git a/mod/quiz/classes/question/bank/qbank_helper.php b/mod/quiz/classes/question/bank/qbank_helper.php
index 2f9c05e18470b..ec8014bb57712 100644
--- a/mod/quiz/classes/question/bank/qbank_helper.php
+++ b/mod/quiz/classes/question/bank/qbank_helper.php
@@ -186,7 +186,7 @@ public static function get_question_structure(int $quizid, \context_module $quiz
                 $slot->length = 1;
             } else if ($slot->qtype === null) {
                 // This question must have gone missing. Put in a placeholder.
-                $slot->questionid = 's' . $slot->id; // Sometimes this is used as an array key, so needs to be unique.
+                $slot->questionid = time() . $slot->id; // Sometimes this is used as an array key, so needs to be unique.
                 $slot->category = 0;
                 $slot->qtype = 'missingtype';
                 $slot->name = get_string('missingquestion', 'quiz');
diff --git a/question/classes/question_reference_manager.php b/question/classes/question_reference_manager.php
index f3bd6c5c1ed37..4fd662a493869 100644
--- a/question/classes/question_reference_manager.php
+++ b/question/classes/question_reference_manager.php
@@ -64,7 +64,6 @@ public static function questions_with_references(array $questionids): array {
                          FROM {question_versions} lqv
                          JOIN {question_versions} lv ON lv.questionbankentryid = lqv.questionbankentryid
                         WHERE lqv.questionid $lqidtest
-                          AND lv.status <> :draft
                      GROUP BY lqv.questionbankentryid
                    ) latestversions ON latestversions.questionbankentryid = qv.questionbankentryid
 
@@ -72,7 +71,7 @@ public static function questions_with_references(array $questionids): array {
                        AND (qr.version = qv.version OR qr.version IS NULL AND qv.version = latestversions.latestusableversion)
 
              WHERE qv.questionid $qidtest
-            ", array_merge($params, $lparams, ['draft' => question_version_status::QUESTION_STATUS_DRAFT]));
+            ", array_merge($params, $lparams));
     }
 
     /**