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 00000000000..9ea556145a8 Binary files /dev/null and b/question/type/essay/tests/fixtures/1.png differ diff --git a/question/type/essay/tests/fixtures/2.png b/question/type/essay/tests/fixtures/2.png new file mode 100644 index 00000000000..85ec921f9de Binary files /dev/null and b/question/type/essay/tests/fixtures/2.png differ diff --git a/question/type/essay/tests/fixtures/3.png b/question/type/essay/tests/fixtures/3.png new file mode 100644 index 00000000000..5e66fa0fff6 Binary files /dev/null and b/question/type/essay/tests/fixtures/3.png differ diff --git a/question/type/essay/tests/helper.php b/question/type/essay/tests/helper.php index 26f283ed57b..928dc8468ee 100644 --- a/question/type/essay/tests/helper.php +++ b/question/type/essay/tests/helper.php @@ -34,7 +34,7 @@ defined('MOODLE_INTERNAL') || die(); */ class qtype_essay_test_helper extends question_test_helper { public function get_test_questions() { - return array('editor', 'editorfilepicker', 'plain', 'monospaced', 'responsetemplate', 'noinline'); + return ['editor', 'editorfilepicker', 'plain', 'monospaced', 'responsetemplate', 'noinline', 'files']; } /** @@ -291,5 +291,87 @@ class qtype_essay_test_helper extends question_test_helper { return new question_file_saver($this->make_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; + } }