From 804b2c1c3f4fa6e5cbcab821c6ae31ca56edbf9a Mon Sep 17 00:00:00 2001 From: Mark Johnson Date: Tue, 11 Feb 2025 08:40:07 +0000 Subject: [PATCH] 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; + } }