From 9775be13e9351a4f02a458a93dc2e300f73c7a5c Mon Sep 17 00:00:00 2001
From: Tim Hunt <T.J.Hunt@open.ac.uk>
Date: Tue, 25 Apr 2023 17:58:58 +0100
Subject: [PATCH] MDL-78025 question generator: make the behaviour less
 surprising

* The object returned by update_question is alwasy a new clone
  and the $question passed in will not be modified.

* The returned object has the fields like questionbankentryid and
  the ones related to versionning, so it is more like the data
  returned by question_bank::load_question_data.
---
 question/tests/generator/lib.php |  8 +++++++-
 question/tests/version_test.php  | 29 +++++++++++++++++------------
 question/upgrade.txt             | 11 +++++++++++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/question/tests/generator/lib.php b/question/tests/generator/lib.php
index 05427126e45..a1d3deb6be8 100644
--- a/question/tests/generator/lib.php
+++ b/question/tests/generator/lib.php
@@ -118,11 +118,12 @@ class core_question_generator extends component_generator_base {
      * @param string $which as for the corresponding argument of
      *      {@link question_test_helper::get_question_form_data}. null for the default one.
      * @param array|stdClass $overrides any fields that should be different from the base example.
-     * @return stdClass the question data.
+     * @return stdClass the question data, including version info and questionbankentryid
      */
     public function update_question($question, $which = null, $overrides = null) {
         global $CFG, $DB;
         require_once($CFG->dirroot . '/question/engine/tests/helpers.php');
+        $question = clone($question);
 
         $qtype = $question->qtype;
 
@@ -144,6 +145,11 @@ class core_question_generator extends component_generator_base {
             }
             $DB->update_record('question', $question);
         }
+        $questionversion = $DB->get_record('question_versions', ['questionid' => $question->id], '*', MUST_EXIST);
+        $question->versionid = $questionversion->id;
+        $question->questionbankentryid = $questionversion->questionbankentryid;
+        $question->version = $questionversion->version;
+        $question->status = $questionversion->status;
 
         return $question;
     }
diff --git a/question/tests/version_test.php b/question/tests/version_test.php
index 43c6c2e2ed8..2a431637a95 100644
--- a/question/tests/version_test.php
+++ b/question/tests/version_test.php
@@ -16,6 +16,7 @@
 
 namespace core_question;
 
+use core_question\local\bank\question_version_status;
 use question_bank;
 
 /**
@@ -89,7 +90,7 @@ class version_test extends \advanced_testcase {
         $this->assertEquals($questionversion->questionbankentryid, $questiondefinition->questionbankentryid);
 
         // If a question is updated, a new version should be created.
-        $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
+        $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
         $newquestiondefinition = question_bank::load_question($question->id);
         // The version should be 2.
         $this->assertEquals('2', $newquestiondefinition->version);
@@ -112,7 +113,7 @@ class version_test extends \advanced_testcase {
         $questionfirstversionid = $question->id;
 
         // Create a new version and try to remove it.
-        $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
+        $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
 
         // The new version and bank entry record should exist.
         $sql = "SELECT q.id, qv.id AS versionid, qv.questionbankentryid
@@ -158,12 +159,14 @@ class version_test extends \advanced_testcase {
      * @covers ::question_delete_question
      */
     public function test_delete_question_in_use() {
+        global $DB;
+
         $qcategory = $this->qgenerator->create_question_category(['contextid' => $this->context->id]);
         $question = $this->qgenerator->create_question('shortanswer', null, ['category' => $qcategory->id]);
         $questionfirstversionid = $question->id;
 
         // Create a new version and try to remove it after adding it to a quiz.
-        $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
+        $question = $this->qgenerator->update_question($question, null, ['name' => 'This is a new version']);
 
         // Add it to the quiz.
         quiz_add_quiz_question($question->id, $this->quiz);
@@ -173,11 +176,13 @@ class version_test extends \advanced_testcase {
         // Try to delete old version.
         question_delete_question($questionfirstversionid);
 
-        // The questions should exist even after trying to remove it.
-        $questionversion1 = question_bank::load_question($question->id);
-        $questionversion2 = question_bank::load_question($questionfirstversionid);
-        $this->assertEquals($questionversion1->id, $question->id);
-        $this->assertEquals($questionversion2->id, $questionfirstversionid);
+        // The used question version should exist even after trying to remove it, but now hidden.
+        $questionversion2 = question_bank::load_question($question->id);
+        $this->assertEquals($question->id, $questionversion2->id);
+        $this->assertEquals(question_version_status::QUESTION_STATUS_HIDDEN, $questionversion2->status);
+
+        // The unused version should be completely gone.
+        $this->assertFalse($DB->record_exists('question', ['id' => $questionfirstversionid]));
     }
 
     /**
@@ -233,10 +238,10 @@ class version_test extends \advanced_testcase {
         $questionid1 = $question->id;
 
         // Create a new version and try to remove it after adding it to a quiz.
-        $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']);
+        $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']);
         $questionid2 = $question->id;
         // Change the id number and get the question object.
-        $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']);
+        $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']);
         $questionid3 = $question->id;
 
         // The new version and bank entry record should exist.
@@ -270,10 +275,10 @@ class version_test extends \advanced_testcase {
         $questionid1 = $question->id;
 
         // Create a new version.
-        $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']);
+        $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id2']);
         $questionid2 = $question->id;
         // Change the id number and get the question object.
-        $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']);
+        $question = $this->qgenerator->update_question($question, null, ['idnumber' => 'id3']);
         $questionid3 = $question->id;
 
         $questiondefinition = question_bank::get_all_versions_of_question($question->id);
diff --git a/question/upgrade.txt b/question/upgrade.txt
index 808662bcfdd..3f2b533e51e 100644
--- a/question/upgrade.txt
+++ b/question/upgrade.txt
@@ -1,5 +1,16 @@
 This files describes API changes for code that uses the question API.
 
+=== 4.3 ===
+
+1) The core_question_generator::update_question has been changed so that it no longer modifies the $question
+   object that was passed in. Instead, the update question is returned (which was already the case).
+   If you were relying on the old behavioru in your tests, you will need a change like
+       $questiongenerator->update_question($question, ...);
+   to
+       $question = $questiongenerator->update_question($question, ...);
+   Also, the $question object returned now has fields questionbankentryid, versionid, version and status.
+
+
 === 4.2 ===
 
 1) The question/qengine.js has been deprecated. We create core_question/question_engine