From 804b2c1c3f4fa6e5cbcab821c6ae31ca56edbf9a Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 11 Feb 2025 08:40:07 +0000 Subject: [PATCH 1/2] MDL-84466 qbank: Transfer question files and tags in separate tasks On a large site, the task to transfer question categories to the new mod_qbanks contexts could take a very long time, as of any transferred categories contain questions using files, each file has to be updated individually. This change splits up the transfer into 2 stages. The first stage (transfer_question_categories) updates the context IDs on each question category and moves them to their new top category, then spawns an instance of transfer_questions for each category that was moved. transfer_questions then handles moving the files and tags for questions in a single category at a time. This allows the transfer process to be parallelised by using multi cron or ad-hoc task runners, so should speed things up on larger sites. --- lib/questionlib.php | 15 +- .../task/transfer_question_categories.php | 50 +++++- mod/qbank/classes/task/transfer_questions.php | 76 +++++++++ .../transfer_question_categories_test.php | 156 +++++++++++++++++- question/type/essay/tests/fixtures/1.png | Bin 0 -> 670 bytes question/type/essay/tests/fixtures/2.png | Bin 0 -> 1033 bytes question/type/essay/tests/fixtures/3.png | Bin 0 -> 1133 bytes question/type/essay/tests/helper.php | 84 +++++++++- 8 files changed, 365 insertions(+), 16 deletions(-) create mode 100644 mod/qbank/classes/task/transfer_questions.php create mode 100644 question/type/essay/tests/fixtures/1.png create mode 100644 question/type/essay/tests/fixtures/2.png create mode 100644 question/type/essay/tests/fixtures/3.png diff --git a/lib/questionlib.php b/lib/questionlib.php index 94e234e5eb2..65023e9d712 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -671,14 +671,15 @@ function move_question_set_references(int $oldcategoryid, int $newcatgoryid, if (isset($filter['questioncategoryid'])) { $filter = question_reference_manager::convert_legacy_set_reference_filter_condition($filter); } - if ((int)$filter['filter']['category']['values'][0] === $oldcategoryid) { - $setreference->questionscontextid = $newcontextid; - if ($oldcategoryid !== $newcatgoryid) { - $filter['filter']['category']['values'][0] = $newcatgoryid; - $setreference->filtercondition = json_encode($filter); - } - $DB->update_record('question_set_references', $setreference); + $setreference->questionscontextid = $newcontextid; + if ( + (int)$filter['filter']['category']['values'][0] === $oldcategoryid + && $oldcategoryid !== $newcatgoryid + ) { + $filter['filter']['category']['values'][0] = $newcatgoryid; + $setreference->filtercondition = json_encode($filter); } + $DB->update_record('question_set_references', $setreference); } $setreferences->close(); } diff --git a/mod/qbank/classes/task/transfer_question_categories.php b/mod/qbank/classes/task/transfer_question_categories.php index ec20fc5cad7..e7253764d87 100644 --- a/mod/qbank/classes/task/transfer_question_categories.php +++ b/mod/qbank/classes/task/transfer_question_categories.php @@ -19,6 +19,7 @@ namespace mod_qbank\task; use context_system; use core\context; use core\task\adhoc_task; +use core\task\manager; use core_course_category; use core_question\local\bank\question_bank_helper; use stdClass; @@ -65,6 +66,8 @@ class transfer_question_categories extends adhoc_task { $recordset = $DB->get_recordset('question_categories', ['parent' => 0]); + $movedcategorycontexts = []; + foreach ($recordset as $oldtopcategory) { if (!$oldcontext = context::instance_by_id($oldtopcategory->contextid, IGNORE_MISSING)) { @@ -132,7 +135,9 @@ class transfer_question_categories extends adhoc_task { } // We have our new mod instance, now move all the subcategories of the old 'top' category to this new context. - $this->move_question_category($oldtopcategory, $newmod->context); + $movedcategories = $this->move_question_category($oldtopcategory, $newmod->context); + + $movedcategorycontexts += array_fill_keys($movedcategories, $oldtopcategory->contextid); // Job done, lets delete the old 'top' category. $DB->delete_records('question_categories', ['id' => $oldtopcategory->id]); @@ -140,6 +145,15 @@ class transfer_question_categories extends adhoc_task { } $recordset->close(); + + // Create a set of new tasks to update the questions in each category to the new contexts. + // The category itself is already in the new context. We record the old context so we know where to move + // files and tags from. + foreach ($movedcategorycontexts as $categoryid => $oldcontextid) { + $task = new transfer_questions(); + $task->set_custom_data(['categoryid' => $categoryid, 'contextid' => $oldcontextid]); + manager::queue_adhoc_task($task); + } } /** @@ -163,10 +177,10 @@ class transfer_question_categories extends adhoc_task { * Create a new 'Top' category in our new context and move the old categories descendents beneath it. * * @param stdClass $oldtopcategory The old 'Top' category that we are moving. - * @param \context $newcontext The context we are moving our category to. - * @return void + * @param context\module $newcontext The context we are moving our category to. + * @return int[] The IDs of all categories moved to the new context. */ - protected function move_question_category(stdClass $oldtopcategory, \context $newcontext): void { + protected function move_question_category(stdClass $oldtopcategory, context\module $newcontext): array { global $DB; $newtopcategory = question_get_top_category($newcontext->id, true); @@ -174,10 +188,36 @@ class transfer_question_categories extends adhoc_task { move_question_set_references($oldtopcategory->id, $newtopcategory->id, $oldtopcategory->contextid, $newcontext->id, true); // This function moves subcategories, so we have to start at the top. - question_move_category_to_context($oldtopcategory->id, $oldtopcategory->contextid, $newcontext->id); + $movedcategories = $this->move_subcategories_to_context($oldtopcategory->id, $newcontext); // Move the parent from the old top category to the new one. $DB->set_field('question_categories', 'parent', $newtopcategory->id, ['parent' => $oldtopcategory->id]); + + return $movedcategories; + } + + /** + * Recursively update the contextid for all subcategories of the given category. + * + * @param int $categoryid The ID of the category to update subcategories for. When calling directly, + * this should be a top category. + * @param context\module $newcontext The new context for the subcategories. + * @return int[] The IDs of all categories moved to the new context. + */ + protected function move_subcategories_to_context(int $categoryid, context\module $newcontext): array { + global $DB; + $movedcategories = []; + + $subcatids = $DB->get_fieldset('question_categories', 'id', ['parent' => $categoryid]); + foreach ($subcatids as $subcatid) { + $DB->set_field('question_categories', 'contextid', $newcontext->id, ['id' => $subcatid]); + $movedcategories[] = $subcatid; + $movedcategories = array_merge( + $this->move_subcategories_to_context($subcatid, $newcontext), + $movedcategories, + ); + } + return $movedcategories; } /** diff --git a/mod/qbank/classes/task/transfer_questions.php b/mod/qbank/classes/task/transfer_questions.php new file mode 100644 index 00000000000..bb6bfc7e93d --- /dev/null +++ b/mod/qbank/classes/task/transfer_questions.php @@ -0,0 +1,76 @@ +. + +namespace mod_qbank\task; + +use core\context; +use core\task\adhoc_task; + +/** + * Move all the question files and tags under a given question category to a new context. + * + * An instance of this task will be created for each category moved to a new context in + * {@see transfer_question_categories}, allowing the heavy lifting of moving the data for each + * question to be parallelised. + * + * @package mod_qbank + * @copyright 2025 onwards Catalyst IT EU {@link https://catalyst-eu.net} + * @author Mark Johnson + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class transfer_questions extends adhoc_task { + + /** + * Find the questions in the category, move their files and tags to the new context. + * + * @return void + */ + public function execute() { + global $DB, $CFG; + + require_once($CFG->dirroot . '/question/engine/lib.php'); + + $data = $this->get_custom_data(); + + $newcontextid = $DB->get_field('question_categories', 'contextid', ['id' => $data->categoryid]); + + if (!$newcontextid) { + mtrace("Could not find a category record for id {$data->categoryid}. Terminating task."); + return; + } + + $newcontext = context::instance_by_id($newcontextid); + + $sql = "SELECT q.id, q.qtype + FROM {question} q + JOIN {question_versions} qv ON qv.questionid = q.id + JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid + WHERE qbe.questioncategoryid = ?"; + + $questions = $DB->get_records_sql($sql, [$data->categoryid]); + $questioncount = count($questions); + mtrace("Moving files and tags for {$questioncount} questions in category {$data->categoryid}."); + $transaction = $DB->start_delegated_transaction(); + foreach ($questions as $question) { + \question_bank::get_qtype($question->qtype, false)->move_files($question->id, $data->contextid, $newcontext->id); + // Purge this question from the cache. + \question_bank::notify_question_edited($question->id); + } + + question_move_question_tags_to_new_context($questions, $newcontext); + $transaction->allow_commit(); + } +} diff --git a/mod/qbank/tests/task/transfer_question_categories_test.php b/mod/qbank/tests/task/transfer_question_categories_test.php index 9418282e91d..8cd90237bbd 100644 --- a/mod/qbank/tests/task/transfer_question_categories_test.php +++ b/mod/qbank/tests/task/transfer_question_categories_test.php @@ -21,7 +21,9 @@ use context_course; use context_coursecat; use context_module; use context_system; +use core\task\manager; use core_question\local\bank\random_question_loader; +use core_question\local\bank\question_bank_helper; use mod_quiz\quiz_settings; use stdClass; use core_question\local\bank\question_version_status; @@ -191,13 +193,17 @@ final class transfer_question_categories_test extends \advanced_testcase { $coursecatcat = $this->create_question_category('Course Cat Parent Cat', $this->coursecatcontext->id); // Add a question to the category just made. - $question3 = $questiongenerator->create_question('shortanswer', null, ['category' => $coursecatcat->id]); + $question3 = $questiongenerator->create_question('essay', 'files', ['category' => $coursecatcat->id]); // Add a quiz to the course category and put those questions into it. $course = self::getDataGenerator()->create_course(['category' => $coursecategory->id]); $quiz = $quizgenerator->create_instance(['course' => $course->id, 'grade' => 100.0, 'sumgrades' => 2, 'layout' => '1,0']); quiz_add_quiz_question($question3->id, $quiz, 1); + // Create an additional question with a missing type, to catch edge cases. + $question4 = $questiongenerator->create_question('missingtype', 'invalid', ['category' => $coursecatcat->id]); + $DB->set_field('question', 'qtype', 'invalid', ['id' => $question4->id]); + // Create 2 nested categories with questions in them at course context level. $course = self::getDataGenerator()->create_course(); $this->coursecontext = context_course::instance($course->id); @@ -370,11 +376,18 @@ final class transfer_question_categories_test extends \advanced_testcase { $parentcat = end($allcoursecatcats); $this->assertEquals($topcat->id, $parentcat->parent); - // Make sure we have 1 question in the above course category level question category. + // Make sure we have 2 questions in the above course category level question category. $questions = $this->get_question_data(array_map(static fn($cat) => $cat->id, $allcoursecatcats)); - $this->assertCount(1, $questions); + $this->assertCount(2, $questions); $question = reset($questions); $this->assertEquals($parentcat->id, $question->categoryid); + // Make sure there are files in the expected fileareas for this question. + $fs = get_file_storage(); + $this->assertTrue($fs->file_exists($this->coursecatcontext->id, 'question', 'questiontext', $question->id, '/', '1.png')); + $this->assertTrue( + $fs->file_exists($this->coursecatcontext->id, 'question', 'generalfeedback', $question->id, '/', '2.png'), + ); + $this->assertTrue($fs->file_exists($this->coursecatcontext->id, 'qtype_essay', 'graderinfo', $question->id, '/', '3.png')); // Make sure we have 4 question categories at course level (including 'top') with some questions in them. $allcoursecats = $DB->get_records('question_categories', ['contextid' => $this->coursecontext->id], 'id ASC'); @@ -556,6 +569,7 @@ final class transfer_question_categories_test extends \advanced_testcase { $grandparentcat = reset($courseqcats); $parentcat = next($courseqcats); $childcat = next($courseqcats); + $this->assertEquals($topcat->id, $grandparentcat->parent); $this->assertEquals($grandparentcat->id, $parentcat->parent); $this->assertEquals($parentcat->id, $childcat->parent); @@ -715,4 +729,140 @@ final class transfer_question_categories_test extends \advanced_testcase { $this->assertEquals($expectedcontext->id, $actualcategory->contextid, "Checking context of category $actualcategory->name."); } + + public function test_transfer_questions(): void { + global $DB; + $this->resetAfterTest(); + $this->setup_pre_install_data(); + + $task = new \mod_qbank\task\transfer_question_categories(); + $task->execute(); + + // Assert that files are still in their original context. + $courses = $DB->get_records('course', ['category' => $this->coursecatcontext->instanceid], 'id ASC'); + $newcourse = end($courses); + $coursemodinfo = get_fast_modinfo($newcourse); + $coursecatqbanks = $coursemodinfo->get_instances_of('qbank'); + $coursecatqbank = reset($coursecatqbanks); + $coursecatqcats = $DB->get_records('question_categories', ['contextid' => $coursecatqbank->context->id], 'parent ASC'); + $parentcat = end($coursecatqcats); + $questions = get_questions_category($parentcat, true); + $question = reset($questions); + $fs = get_file_storage(); + $this->assertTrue($fs->file_exists( + $this->coursecatcontext->id, + 'question', + 'questiontext', + $question->id, + '/', + '1.png' + )); + $this->assertTrue($fs->file_exists( + $this->coursecatcontext->id, + 'question', + 'generalfeedback', + $question->id, + '/', + '2.png' + )); + $this->assertTrue($fs->file_exists( + $this->coursecatcontext->id, + 'qtype_essay', + 'graderinfo', + $question->id, + '/', + '3.png' + )); + $this->assertFalse($fs->file_exists( + $coursecatqbank->context->id, + 'question', + 'questiontext', + $question->id, + '/', + '1.png' + )); + $this->assertFalse($fs->file_exists( + $coursecatqbank->context->id, + 'question', + 'generalfeedback', + $question->id, + '/', + '2.png' + )); + $this->assertFalse($fs->file_exists( + $coursecatqbank->context->id, + 'qtype_essay', + 'graderinfo', + $question->id, + '/', + '3.png' + )); + + $questiontasks = manager::get_adhoc_tasks(transfer_questions::class); + + // We should have a transfer_questions task for each category that was moved. + // 2 site categories, + // 1 coursecat category, + // 3 regular course categories, + // 2 used/unused course categories. + $this->assertCount(8, $questiontasks); + + $this->expectOutputRegex('~Moving files and tags~'); + // Delete one of the categories before running the tasks, to ensure missing categories are handled gracefully. + $unusedcat = $DB->get_record('question_categories', ['name' => 'Unused Question Cat']); + question_category_delete_safe($unusedcat); + $this->expectOutputRegex("~Could not find a category record for id {$unusedcat->id}. Terminating task.~"); + + $this->runAdhocTasks(); + + // The files have now been moved to the new context. + $this->assertFalse($fs->file_exists( + $this->coursecatcontext->id, + 'question', + 'questiontext', + $question->id, + '/', + '1.png' + )); + $this->assertFalse($fs->file_exists( + $this->coursecatcontext->id, + 'question', + 'generalfeedback', + $question->id, + '/', + '2.png' + )); + $this->assertFalse($fs->file_exists( + $this->coursecatcontext->id, + 'qtype_essay', + 'graderinfo', + $question->id, + '/', + '3.png' + )); + $this->assertTrue($fs->file_exists( + $coursecatqbank->context->id, + 'question', + 'questiontext', + $question->id, + '/', + '1.png' + )); + $this->assertTrue($fs->file_exists( + $coursecatqbank->context->id, + 'question', + 'generalfeedback', + $question->id, + '/', + '2.png' + )); + $this->assertTrue($fs->file_exists( + $coursecatqbank->context->id, + 'qtype_essay', + 'graderinfo', + $question->id, + '/', + '3.png' + )); + } } diff --git a/question/type/essay/tests/fixtures/1.png b/question/type/essay/tests/fixtures/1.png new file mode 100644 index 0000000000000000000000000000000000000000..9ea556145a873b7fdd50edd01025b0b7e35c1b4a GIT binary patch literal 670 zcmeAS@N?(olHy`uVBq!ia0vp^xggBJ1|(PicpnC&I14-?iy0U+D?yksl&8}OC@5Lt z8c`CQpH@mmtT}V`<;yx0|S$`r;B4q#hkY{H+G3Qinu*2 zE)^HM7*f;BC|oPT%VKD3booTNfq|hAOR}*r^Zf%3JU-$k7it^@Qh2{eSv=6`;X7n< zRP!G{+pa3L9Y+^-w`nfmoLan?z2krRhQeAdnSLpivM5IP0<&9%#NgL=e^gzx@)R=wgXNZkA9%3XW6bDQ;p?J~dKW`1D&q44ico3d4ZXTI}X zy~_JQ@q=qGBBvRJZdnz@sNcAHaeCw>ljF&uciHZJRKFqp;Q5ZuX^Z>ALfb>XvQ#j} z#N4lAy_3-NH`n;^vUyD3&9C)0&UWaPOs_O{sb;BR+{awL`}f0NSN|2fi8y_6e@Dsp z_Eo!>=ahWE-Mju=|E$c+Ul|N{|5lxTE9p0J?XtT!&JgSAWj~(((%;8)-t5|aH{MP8 zm)%~!6Mk^-h2=D0*ikXqa2Un);_Fw<0-J(qKeme&u%s_;b?I|o7@xV-Mv-4c<0CL_ OF?hQAxvXmmtT}V`<;yx0|WCkPZ!6KiaBrZ`gVktN*w?A zezCCT496DznNdl8-EA>jOC$HW>@InFEcfA}s4W>2qn(#5WodGF()zu}PX_(g) z`NGXnU{kpW?|iZ6`tv3(oB48{b$$Kwe|7~A=h(NOE`EO6vOJEjoXNxI)S_F=H~#c) z-S(b2cq`k+$X;gegsLqo&)kVgmz(YD;=`2RtlvLL#&^PE&LR`XKD84USuAHMxC=kY zXrfT?Sem_yy9>(($qL~JP8y77USV|lyO3*LRRk|0|@w3hKn@@Mg(F5NPOuP1p`GHLV zP>hrpSC1wZg^NNam6#`rsGy=mj$ ztOPIu%!!8rC@{&lDG1e6vodzPXom&FN2~mDj$$Qr0y3e0e8} zcHg#b@vLDyiAxqv?lKNZfju{MojQUfSPCl^i%%9!&Lhind-O?e!evV0h>Qe0k?K6Ka0E)c|-6Hgc zX~Qa=Shj6x&*Hx>dli=tv|eqNeZx9E@97P>Gk?18UD>u+9B2XWEaNv*H& z=H}-S0h=xweHJ=fee1->r-9T@gjmz?t^f9K?F>Q-#eU~MJ|Hwh?ySp?PZ}@eg{AiJ Uhir2$0%l_dPgg&ebxsLQ07EsmmtT}V`<;yx0|WCfPZ!6KiaBrZobMJdl{x{6w3+H_-7d)#us?ZuG04q$%0-@36KBb@0aRt? ze~qp2xW#gZJBHhbIrxC(1MLSPE`Ndw_H5kPua+tMLF9wZhVH5NFW;Z!$E5FiM11nB z^}N@XyLqZ>@c&v(l z$IjM`QR%X|9H;wNeRW+X5qN!3?EW`jzpJ)MgkE1{dpF|0nbJA2*(oe@JH!5)RWIB4 zGGJw-+tz1`J#T5f6n&68?cV$71KCfPN%j5?|M%bH7O&0C8z#JWnBFYd#m0G+|6TKe z)yy@I_dHoMGvmP91I5~Z-pUlPSjY(O_sd&Sw&v&MhTW3lvo5dQC#^WN_gv78 z`;EJm9{FrM$aZ0;`f9hQ$`2SHY_IwpdZ4i=n0LMKMX{^)%lb{t-fEhj*02&?Yj!(g zzuTo_Nt1b2c3->_!T(@>P&3c!?u%t6*Xlbu?7Mx9miW1@_HXq!F+00%%loBg%w{c# zowQCm;lEtLlvm$&x#suOZs6R|>-P2R`)p6+N%N+ctX{J6skP@VjZFOynj7|iT<+gc zu5R)=VrP8y945Jy;ti~x_XA_IS#B`pJpC_!E;3>MYuDuG8S{fmake_attachments($attachments), 'question', 'response_attachments'); } + /** + * Create a question with images embedded in the questiontext, generalfeedback and graderinfo. + * + * @return stdClass + */ + public function get_essay_question_form_data_files() { + global $CFG, $USER; + $fromform = new stdClass(); + $usercontext = context_user::instance($USER->id); + $questiontextdraftid = 1; + file_prepare_draft_area($questiontextdraftid, $usercontext->id, null, null, null); + $fs = get_file_storage(); + $filerecord = new stdClass(); + $filerecord->contextid = $usercontext->id; + $filerecord->component = 'user'; + $filerecord->filearea = 'draft'; + $filerecord->itemid = $questiontextdraftid; + $filerecord->filepath = '/'; + $filerecord->filename = '1.png'; + $fs->create_file_from_pathname($filerecord, $CFG->dirroot . + '/question/type/essay/tests/fixtures/1.png'); + + $feedbackdraftid = 2; + file_prepare_draft_area($feedbackdraftid, $usercontext->id, null, null, null); + $fs = get_file_storage(); + $filerecord = new stdClass(); + $filerecord->contextid = $usercontext->id; + $filerecord->component = 'user'; + $filerecord->filearea = 'draft'; + $filerecord->itemid = $feedbackdraftid; + $filerecord->filepath = '/'; + $filerecord->filename = '2.png'; + $fs->create_file_from_pathname($filerecord, $CFG->dirroot . + '/question/type/essay/tests/fixtures/2.png'); + + $graderinfodraftid = 3; + file_prepare_draft_area($graderinfodraftid, $usercontext->id, null, null, null); + $fs = get_file_storage(); + $filerecord = new stdClass(); + $filerecord->contextid = $usercontext->id; + $filerecord->component = 'user'; + $filerecord->filearea = 'draft'; + $filerecord->itemid = $graderinfodraftid; + $filerecord->filepath = '/'; + $filerecord->filename = '3.png'; + $fs->create_file_from_pathname($filerecord, $CFG->dirroot . + '/question/type/essay/tests/fixtures/3.png'); + + $fromform->name = 'Essay question (HTML editor)'; + $questionfileurl = $CFG->wwwroot . '/draftfile.php/' . $usercontext->id . '/user/draft/' . $questiontextdraftid . '/1.png'; + $fromform->questiontext = [ + 'text' => 'Please write a story about a frog. ', + 'format' => FORMAT_HTML, + 'itemid' => $questiontextdraftid, + ]; + $fromform->defaultmark = 1.0; + + $feedbackfileurl = $CFG->wwwroot . '/draftfile.php/' . $usercontext->id . '/user/draft/' . $feedbackdraftid . '/2.png'; + $fromform->generalfeedback = [ + 'text' => 'I hope your story had a beginning, a middle and an end. ', + 'format' => FORMAT_HTML, + 'itemid' => $feedbackdraftid, + ]; + $fromform->responseformat = 'editor'; + $fromform->responserequired = 1; + $fromform->responsefieldlines = 10; + $fromform->attachments = 0; + $fromform->attachmentsrequired = 0; + $fromform->maxbytes = 0; + $fromform->filetypeslist = ''; // Although once saved in the DB, this becomes null, the form returns '' here. + + $graderinfourl = $CFG->wwwroot . '/draftfile.php/' . $usercontext->id . '/user/draft/' . $graderinfodraftid . '/3.png'; + $fromform->graderinfo = [ + 'text' => '', + 'format' => FORMAT_HTML, + 'itemid' => $graderinfodraftid, + ]; + $fromform->responsetemplate = ['text' => '', 'format' => FORMAT_HTML]; + $fromform->status = \core_question\local\bank\question_version_status::QUESTION_STATUS_READY; + + return $fromform; + } } From 939e57219c87d3c378e1f13502a75b3c2fe302b4 Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Thu, 20 Mar 2025 11:01:45 +0000 Subject: [PATCH 2/2] MDL-84466 question/qbank: Update pending transfer notification This updates the logic and text of the notification that is shown while the question transfer task is still pending, to include instances of the new transfer_questions task. It also ensures the notification is shown if a user tries to access the question editing screen from outside the question bank. --- mod/qbank/lang/en/qbank.php | 2 +- .../tests/task/transfer_question_categories_test.php | 4 ++++ question/bank/editquestion/question.php | 11 +++++++++++ question/classes/local/bank/question_bank_helper.php | 3 ++- question/edit.php | 8 ++++++-- 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/mod/qbank/lang/en/qbank.php b/mod/qbank/lang/en/qbank.php index 8fd24af0450..395079a397a 100644 --- a/mod/qbank/lang/en/qbank.php +++ b/mod/qbank/lang/en/qbank.php @@ -48,5 +48,5 @@ $string['saveandreturn'] = 'Save and return to question bank list'; $string['sharedbank'] = '{$a} shared question bank'; $string['showdescription'] = 'Display description on manage question banks page'; $string['showdescription_help'] = 'If enabled, the description above will be displayed on the manage question bank page just below the link to the bank.'; -$string['transfernotfinished'] = 'Adhoc task \\mod_qbank\\task\\transfer_question_categories has not yet completed or has failed. Some of your pre-installed question banks may not be transferred to mod_qbank instances yet. Any question categories they contained can\'t be shared or managed until the task has completed.'; +$string['transfernotfinished'] = 'Adhoc tasks \\mod_qbank\\task\\transfer_question_categories and \\mod_qbank\\task\\transfer_questions have not yet completed or have failed. Some of your pre-installed question banks may not be transferred to mod_qbank instances yet. Any question categories they contained can\'t be shared or managed until these tasks have completed.'; $string['unknownbanktype'] = 'Unknown question bank type {$a}'; diff --git a/mod/qbank/tests/task/transfer_question_categories_test.php b/mod/qbank/tests/task/transfer_question_categories_test.php index 8cd90237bbd..b6cc82f274f 100644 --- a/mod/qbank/tests/task/transfer_question_categories_test.php +++ b/mod/qbank/tests/task/transfer_question_categories_test.php @@ -798,6 +798,8 @@ final class transfer_question_categories_test extends \advanced_testcase { '3.png' )); + $this->assertFalse(question_bank_helper::has_bank_migration_task_completed_successfully()); + $questiontasks = manager::get_adhoc_tasks(transfer_questions::class); // We should have a transfer_questions task for each category that was moved. @@ -864,5 +866,7 @@ final class transfer_question_categories_test extends \advanced_testcase { '/', '3.png' )); + + $this->assertTrue(question_bank_helper::has_bank_migration_task_completed_successfully()); } } diff --git a/question/bank/editquestion/question.php b/question/bank/editquestion/question.php index 0d5cb179670..bc074161c03 100644 --- a/question/bank/editquestion/question.php +++ b/question/bank/editquestion/question.php @@ -128,6 +128,17 @@ if ($id) { throw new moodle_exception('notenoughdatatoeditaquestion', 'question', $returnurl); } +// If we get to the editing page from somewhere outside the question bank (such as the quiz) while migration is still +// pending, redirect back with a warning. +if (!\core_question\local\bank\question_bank_helper::has_bank_migration_task_completed_successfully()) { + $defaultactivityname = \core_question\local\bank\question_bank_helper::get_default_question_bank_activity_name(); + redirect( + $returnurl, + get_string('transfernotfinished', 'mod_' . $defaultactivityname), + messagetype: \core\output\notification::NOTIFY_WARNING, + ); +} + $qtypeobj = question_bank::get_qtype($question->qtype); if (isset($question->categoryobject)) { diff --git a/question/classes/local/bank/question_bank_helper.php b/question/classes/local/bank/question_bank_helper.php index fdf44c28dac..5df7609b3b6 100644 --- a/question/classes/local/bank/question_bank_helper.php +++ b/question/classes/local/bank/question_bank_helper.php @@ -643,7 +643,8 @@ class question_bank_helper { public static function has_bank_migration_task_completed_successfully(): bool { $defaultbank = self::get_default_question_bank_activity_name(); $task = manager::get_adhoc_tasks("\\mod_{$defaultbank}\\task\\transfer_question_categories"); - return empty($task); + $subtasks = manager::get_adhoc_tasks("\\mod_{$defaultbank}\\task\\transfer_questions"); + return empty($task) && empty($subtasks); } /** diff --git a/question/edit.php b/question/edit.php index 5ea2f91ac0e..e0b567d8523 100644 --- a/question/edit.php +++ b/question/edit.php @@ -71,9 +71,13 @@ $PAGE->activityheader->disable(); echo $OUTPUT->header(); if (!\core_question\local\bank\question_bank_helper::has_bank_migration_task_completed_successfully()) { $defaultactivityname = \core_question\local\bank\question_bank_helper::get_default_question_bank_activity_name(); - echo $OUTPUT->notification(get_string('transfernotfinished', 'mod_' . $defaultactivityname), - \core\output\notification::NOTIFY_WARNING + echo $OUTPUT->notification( + get_string('transfernotfinished', 'mod_' . $defaultactivityname), + \core\output\notification::NOTIFY_WARNING, + false, ); + echo $OUTPUT->footer(); + exit(); } // Print horizontal nav if needed.