From d36616757cadc594c3e3220c8a6aec4e5f5b4e64 Mon Sep 17 00:00:00 2001 From: James C <5689414+james-cnz@users.noreply.github.com> Date: Mon, 1 Dec 2025 18:01:57 +1300 Subject: [PATCH] MDL-87356 qtype_random: orphaned random questions should be deleted This patch reinstates the task from MDL-63260 and MDL-66273. --- backup/moodle2/restore_stepslib.php | 9 ++ .../classes/task/remove_unused_questions.php | 98 ++++++++++++ question/type/random/db/tasks.php | 38 +++++ .../type/random/tests/cleanup_task_test.php | 144 ++++++++++++++++++ .../tests/fixtures/broken_question_course.mbz | Bin 0 -> 4802 bytes question/type/random/version.php | 2 +- 6 files changed, 290 insertions(+), 1 deletion(-) create mode 100644 question/type/random/classes/task/remove_unused_questions.php create mode 100644 question/type/random/db/tasks.php create mode 100644 question/type/random/tests/cleanup_task_test.php create mode 100644 question/type/random/tests/fixtures/broken_question_course.mbz diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index c34d3490413..22fc462d393 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -5289,6 +5289,15 @@ class restore_create_categories_and_questions extends restore_structure_step { $this->set_mapping('question_bank_entry', $this->latestqbe->oldid, $this->latestqbe->newid); } + if ( + ($data->qtype === 'random') + && ($this->latestversion->status == \core_question\local\bank\question_version_status::QUESTION_STATUS_HIDDEN) + ) { + // Ensure that this newly created question is considered by + // \qtype_random\task\remove_unused_questions. + $this->latestversion->status = \core_question\local\bank\question_version_status::QUESTION_STATUS_DRAFT; + } + // Now store the question. $newitemid = $DB->insert_record('question', $data); $this->set_mapping('question', $oldid, $newitemid); diff --git a/question/type/random/classes/task/remove_unused_questions.php b/question/type/random/classes/task/remove_unused_questions.php new file mode 100644 index 00000000000..eb059b4ecc6 --- /dev/null +++ b/question/type/random/classes/task/remove_unused_questions.php @@ -0,0 +1,98 @@ +. + +/** + * A scheduled task to remove unneeded random questions. + * + * @package qtype_random + * @category task + * @copyright 2018 Bo Pierce + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace qtype_random\task; + +use core\task\manager; + + +/** + * A scheduled task to remove unneeded random questions. + * + * @copyright 2018 Bo Pierce + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class remove_unused_questions extends \core\task\scheduled_task { + /** + * Get a descriptive name for this task (shown to admins). + * + * @return string + */ + public function get_name(): string { + return get_string('taskunusedrandomscleanup', 'qtype_random'); + } + + /** + * Do the job. + * + * @return void + */ + public function execute() { + global $DB, $CFG; + require_once($CFG->libdir . '/questionlib.php'); + + // Confirm, that there is no restore in progress to make sure we do not + // clean up questions that have their quiz slots not restored yet. + $restoretasks = [ + '\core\task\asynchronous_copy_task', + '\core\task\asynchronous_restore_task', + ]; + + $running = manager::get_running_tasks(); + foreach ($running as $task) { + if (in_array($task->classname, $restoretasks)) { + mtrace('Detected running async restore. Aborting the task.'); + return; + } + } + + // Find potentially unused random questions (up to 5000). + // Note, because we call question_delete_question below, + // the question will not actually be deleted if something else + // is using them, but nothing else in Moodle core uses qtype_random, + // and not many third-party plugins do. + $unusedrandomids = $DB->get_records_sql( + " SELECT DISTINCT q.id, 1 + FROM {question} q + JOIN {question_versions} qv on qv.questionid = q.id + JOIN {question_bank_entries} qbe on qbe.id = qv.questionbankentryid + LEFT JOIN {question_references} qr on qr.questionbankentryid = qbe.id + WHERE qr.questionbankentryid IS NULL + AND q.qtype = ? AND qv.status <> ?", + ['random', 'hidden'], + 0, + 5000 + ); + + $count = 0; + foreach ($unusedrandomids as $unusedrandomid => $notused) { + question_delete_question($unusedrandomid); + // In case the question was not actually deleted (because it was in use somehow), + // it will be marked as hidden, so the query above will not return it again. + $count += 1; + } + mtrace('Cleaned up ' . $count . ' unused random questions.'); + } +} diff --git a/question/type/random/db/tasks.php b/question/type/random/db/tasks.php new file mode 100644 index 00000000000..198dcba16c9 --- /dev/null +++ b/question/type/random/db/tasks.php @@ -0,0 +1,38 @@ +. + +/** + * Definition of question/type/random scheduled tasks. + * + * @package qtype_random + * @category task + * @copyright 2018 Bo Pierce + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +$tasks = [ + [ + 'classname' => 'qtype_random\task\remove_unused_questions', + 'blocking' => 0, + 'minute' => 'R', + 'hour' => '*', + 'day' => '*', + 'month' => '*', + 'dayofweek' => '*', + ], +]; diff --git a/question/type/random/tests/cleanup_task_test.php b/question/type/random/tests/cleanup_task_test.php new file mode 100644 index 00000000000..4448aee648d --- /dev/null +++ b/question/type/random/tests/cleanup_task_test.php @@ -0,0 +1,144 @@ +. + +/** + * Tests of the scheduled task for cleaning up random questions. + * + * @package qtype_random + * @copyright 2018 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + + +namespace qtype_random; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + + +/** + * Tests of the scheduled task for cleaning up random questions. + * + * @copyright 2018 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \qtype_random\task\remove_unused_questions + */ +final class cleanup_task_test extends \advanced_testcase { + /** + * Test that remove_unused_questions deletes questions as appropriate. + * + * @covers ::execute + */ + public function test_cleanup_task_removes_unused_question(): void { + global $DB, $USER; + $this->resetAfterTest(); + $this->setAdminUser(); + + // To do the test, we will be restoring a backup that contains 3 questions: + // A non-hidden broken question, a hidden broken question, and a non-broken question. + // Only the non-hidden broken question should be deleted, + // because questions are hidden when a delete was attempted but failed, + // and this is used to indicate we should skip over them in future deletes, + // to avoid continuingly attempting to delete undeletable questions. + + // Extract backup file. + $backupid = 'test_cleanup_task_removes_unused_question'; + $backuppath = make_backup_temp_directory($backupid); + get_file_packer('application/vnd.moodle.backup')->extract_to_pathname( + __DIR__ . '/fixtures/broken_question_course.mbz', + $backuppath + ); + + // Do restore to new course with default settings. + $categoryid = $DB->get_field_sql("SELECT MIN(id) FROM {course_categories}"); + $newcourseid = \restore_dbops::create_new_course('Broken Question Course', 'BQC', $categoryid); + $rc = new \restore_controller( + $backupid, + $newcourseid, + \backup::INTERACTIVE_NO, + \backup::MODE_GENERAL, + $USER->id, + \backup::TARGET_NEW_COURSE + ); + $rc->execute_precheck(); + $rc->execute_plan(); + $rc->destroy(); + + // Check the hidden question was unhidden during the restore, + // to make it eligible for deletion. + $hiddenquestionid = $DB->get_field('question', 'id', ['name' => 'Random (BQC hidden broken question)']); + $this->assertNotEquals( + 'hidden', + $DB->get_field('question_versions', 'status', ['questionid' => $hiddenquestionid]) + ); + + // Revert the hidden question back to hidden, so we can check it isn't deleted. + $DB->set_field('question_versions', 'status', 'hidden', ['questionid' => $hiddenquestionid]); + + // Run the scheduled task. + $task = new \qtype_random\task\remove_unused_questions(); + $this->expectOutputString("Cleaned up 1 unused random questions.\n"); + $task->execute(); + + // Verify. + $this->assertFalse( + $DB->record_exists('question', ['name' => 'Random (BQC non-hidden broken question)']) + ); + $this->assertTrue( + $DB->record_exists('question', ['name' => 'Random (BQC hidden broken question)']) + ); + $this->assertTrue( + $DB->record_exists('question', ['name' => 'BQC non-broken question']) + ); + } + + /** + * Test that remove_unused_questions aborts when there is a course restore in progress. + * + * @covers ::execute + */ + public function test_cleanup_task_checks_for_active_restores(): void { + $this->resetAfterTest(); + + // Get ready the tasks. + $cleanuptask = new \qtype_random\task\remove_unused_questions(); + $restoretask = new \core\task\asynchronous_restore_task(); + \core\task\manager::queue_adhoc_task($restoretask); + $copytask = new \core\task\asynchronous_copy_task(); + \core\task\manager::queue_adhoc_task($copytask); + + // Start the first adhoc task. This might be either restore or copy adhoc task. + $task1 = \core\task\manager::get_next_adhoc_task(time()); + \core\task\manager::adhoc_task_starting($task1); + $cleanuptask->execute(); + + // Complete the first task and start the second one. + \core\task\manager::adhoc_task_complete($task1); + $task2 = \core\task\manager::get_next_adhoc_task(time()); + \core\task\manager::adhoc_task_starting($task2); + $cleanuptask->execute(); + + // Complete the second adhoc task. + \core\task\manager::adhoc_task_complete($task2); + $cleanuptask->execute(); + + $aborted = 'Detected running async restore. Aborting the task.'; + $completed = 'Cleaned up 0 unused random questions.'; + $this->expectOutputRegex("/.*$aborted.*\s.*$aborted.*\s.*$completed.*/"); + } +} diff --git a/question/type/random/tests/fixtures/broken_question_course.mbz b/question/type/random/tests/fixtures/broken_question_course.mbz new file mode 100644 index 0000000000000000000000000000000000000000..204a62ddcaaa1727c00a4ca58ac698fe3cc9b744 GIT binary patch literal 4802 zcmV;z5dp)zW zG2u{9BqVW*BGn{SRr1sP(f-qZ$>P39mQ>vmtJ_UHQ~{ZZ#F+^onSgG-`tj|*efRfo z7vKH$n{WT|;v&IDA@Dr%9|iM2a9#TaaO}|b9M{8efS}F`%f3u?^suj+xI$GKqO6ZF z{G}|@99nUeY_dCOtumCziWENFSZ~XH(R^V!;8jv?cR6gbvbcHJ<}X(-!{{Xl18?d$ zksTuRzN+EmW%|;7`G%+HKSmFH$Co(*PLma@vVto{c+O;zTw=4{-sgF@CWg{yd< zH=I1`qJ{--!t4zicIysSHJSonUV$6?%7R5wrdhH6^6GCt{rP%& z^=AHRMz!ZcP8S55w1LmvoH0BxuvPJ*WndW(eZN=!aa`N~*`X79FD(C}wPN8jm;cso z?E+;S(f>{u_=^5_kp2fQA|Lht0O4u+-^$W2uYC8)O3I>v4-HA$(bfEwWzFhMSv5tx zh4Zg}csrZOBu1|Gc}{Uvc?%2c2ewzmrwqh+HW37vI!lZFb_uKbY$Bqhi2ZgOR|f>< zF$Ib?jN7=G1A$}!Qap_|CzRnuBt>^x1A`>JZnl?=u zH*gL@*Y`p^lTm&F#Wb);NgN?~6oXOSZc&2}l*K6UVR>j!xdhDO7?HzuoE*rh1mn~s zuYU!1sILiHO=9$2R%go`;)oXA}CLK(Z}NDu}u#odYNEBOe4Is;~;@bf_BHD#L`K?}8u-fJ+Gl z0#jCyoP{8%mQ3*eiqS#iE^7{WL`1LYKu#+6HfYMH89Ax&iPDL~8F%RlKHxD}Au*Z% z=1+h9+xOr9={K{94yQ9DMnj@(pvY7)V3<%Qi?Yd92Ta(45kge`H9DUXQ3XUY@kGXw z^_HVh{kf=-v8D!P{eE6)QN~CNRyUZGz59dP%Q{mB2@Uwo#?Ml z3`IO9x&+G(jdTLa1V@u*Zn89m1wUu1ZN~x<=X)~CX@pj1(pR6$R+KVNvwD}uhYzS{ z7rd4mc6FEDw#1x&jX8Gy!)HEY{r7@!T>pm%r>_5Of>7;R=8(l%O3YF;qkX()^ZCh< zG{zZlL)C)o6v?fHQdzq;P?`789^>JhfropPM+ca+a>ixt<2WmuR@O_DaZLUl08IG@ zzCFtS5aCq$Clf?S|Dr@Ws*G=wCmk>Xit9RC7sT4G5Nc2gt3M8D`2517-%!p}#!>sP zXUe}DI^+63NO*7i?}AU1PD8N7K6eAh@M!4#DthKJj>v!LJE0-}j^mEse+CJs%DJ<1J4$;5-b`>a>sd&8Oj7l z@6hJxzN}K!YQ#aDCMj#xszj0kx34NReg_rLL7WD+lpfSrOlHOgo2+O6HgPGb7J;Mj z7r+*s+ePQL>AWsFuTAH7(fMt4`` zCsL?RnZl%9I03fk+C>wfPKWLLZH5%urfjpM&^BeeH5A&WY)=Xb<&>J;7`uLeFal1? zZY*dO+p$ip+KvhJzJ}F8S9t_J=|pJCP1A3;$r|7sc z!B#MZ?75HiCkNr}e!()1*?;Z8@c*^ZaE;%81_`Iyf0)D3Y(gW?UF|WPDYWjiT%e4j z-+z3^@c##HIQoAM5>AzWZJv;Yk3tk@$%zi(X`bh~TRMiWpj}wIP#H($-wgs_-v9B! zV3hwM!jt45&l4i*ann@Ua?c%QCq|kIK687@Am24>VCtaiy3#VRi_wu(bC;z6j!)sP z+2HdXjmT^$F>&1q&Y-}BL-`xsxtt*5^}2%Vm^e&g4`MaVX>K9hZL;+SpE*TbRHiC- zd3jIVo>|o74a*`wkT$2d)?MIs7IwcA^KApW&q2fwI&%G!&OthH>l{Q~WpU{&#?CwX z(5dQPA7wVi!cAU-kP)9oa>O~mOf_vNKu(7<8%7`jQfZpQjyus(3Ib3mP=oim$%g?| zx~xEIp-AA|bt#PiXmz4D6O3Ak>k10DRcTNWkf2H1oKk~N66eW2j}0$v70M||hm)2A zHb`1G5Xd%um^=23O>Zp-kWQ1KiJ+5YvJ`S-SQ|JF;S_AzYBUn26@|@NMp*>JiMIQ^ zS;Yz3N6Z1IQeZ+A33HJwu(riJoCmsSlHE8|Ss=w9i0~%QHgcWpYceN3fWRz^Z|xNL&~yAx!K2 zj$V7gH)3S+47!QhXr^J0ZZbEVs%BE59EQbA)l~BC4o&BIK>m7Y*L+Rf|nrZ{`q$(1mCipbwXRlzITB zfuEr5tq1%iSSoBg^BZL__jLSy+f}kka5ueip|c0wllRzt_udTM{EW->{pg?u_EOfVpo+?aUHwPL4y3Hth-?s3UJ8U$J9R6KBd1h^8$jc*GyYg?0oI z2*bp;ec-L8Ug)Kv=cjf8r>Qha;UkJ_Qc%y)mbmcCRGoR_mxv+~AabvqZ2Q%uBqA5@ zLoD=Vs>0y|O!h>(I@oiBCprlDaZLw^6B6wJzRA+MXT~I3_7=Y-t(3z+5@M;hnel5~ z9T%Eu=VeV!YDOa}D$OdGknaMW?|O@+s76mbjGtil@{Ond#?G2~Y~j;T_E9ShwQW zA#hycNb|93#}WNFDh{OYw_+=2SlW$Rwce+iWToq=qkV}2(~%Q#&R+p*ZBT%<_P-woYXqK)K*#vzN& z%k`j5Rlm~@tQ+_n3Cp;+?I{;rrpefbG_btxAX?dzfqPgnrVZO%vW>yx%A!M-{t)jn z>_k53%C5Z3b@pP}F11}U-_Pos=hl{;)=(u{u!Dc!tayL1yFgvBGRF3%_v=@YWY%QO zawQBy%(V5~2DyQQ`k?fs$~fx(=eVZ-j~n=-{db6Ps{NPG6a274R%Bduy1>ufq%k}p zXm7?}sEi}ue?r?a{eNwLeE)5jaH{;9kC}0Quht`H)UD6fU!MGiz6N8Vb=T@Mh%44> z!(41ycv+fmFML}NDEke;g1K!8>zxUu)WjR>w|Vo&-EY^;AAj0Fi@jfCiDu!d3t_S9 z4LXyiR#xzraIN2Ux9fK|nC*n{r8^I$Ncm!|l45qhCf@FtY5Aty-NeaF@yl!A*Z}wr z01oh8fA!A(?+1EYa=T;Mn9C%2piXt4RNRcBC~7$x9rr+!1C6)z3&dD)?40x>`yR*Z zn>LO%ZCYK*h(ChKa)z-C><~pWTS1KJoJFwjNq|;_(i3`aU zjMIa8DcLB^fGe23;?*I>Xw%(Up(2z0$GAw#t@Zn_e|T#ZWpPdY`K%=s5nkFK%o9?p zK6pyw1Cy41oSaQ&Ba`{HOJrR`G+=SQf-uErDkLf<+ z2O0~_$h}e2$A6uFDnaNm=PFszHS?IgkT32z6 z|3X;2QSCZUU#`I5%M}=CxdNvzS537?EOTuA``{Hzcptx}4r{gxJl}r(eN%gX|IZWG zDslsDYDf0d)+!_0Gp$v)0~ex4A72dOqP`byodo;p}vLFF-UmQ?|*5|Ulx5dnEoWNrrR~e zE^j8*%|0c5z~*H|9L)-L4N8CKS%4{}`lF<+pD5LItKZhGag}W7Q#hg~5=3+){k1~& zi)5dNOB;E#=b7LC03bB|zmQTG<$s89s{GS=Vt9YHfzREXF+4Gd+qV}i1Iu{m`~B}f zqd3}2hqmXq9)X8}gVullf^`z%GnfC)+t+8F3{M_^v>@R3U&isjeCQs3mN?V4;Mn@_ zx8DEqfH$uHgM^dUe>x-RlXqx}kPZKgStjtWmIxa~-_Nzo>iu>bSL&~jaEOAH1{fVc zNZ`9Yew=2GFLbdehTmm%w#@03H5SF-_%6=!c$wu{bNHWHdp5I$dPQxXyRy)qpp`I8 zK_2c*&6hlhMXBSRRWGvr!2Bd6@4G$s$kWg*|LiBq2al>Irym*tx@~pVA{4ZSZ|NiyA zyY0``Ke4d?Y|*bLO%{`9Z|P(qH=n99&r!}}FKWFyR!j<)`}NNje^^&xP5p+frnG=< zpUNjMZvA6jkC`0hKu15> zz~ojm18o%c71}qQr5XMS&G?{7JYQzs{P@mjJ!*EkJT<#`;9ciI>V$U4mg_D3$X+4Q z3!t4YZQlV)Kd_gn?XDu<10NtUtE$|dQqysK9vL5K$5Ap(LExkyisEGyISGKlDw#qr zoCeM~KA-0J_#Zqz{)hC^c`~1(p~HR+-Qx4Kp)#`{8!9{T!)OWosqK16yh?50ETb?% zdkvV{-gJneIva=nHMv(~KWWsaAJ3?{UOHVaQ$NNIGhQ4=FGS8_ cLyut$V;IAy82%ps0RR630JZv|W&o-H0Q}&Jl>h($ literal 0 HcmV?d00001 diff --git a/question/type/random/version.php b/question/type/random/version.php index 1d00a5e47f1..b6285245068 100644 --- a/question/type/random/version.php +++ b/question/type/random/version.php @@ -26,7 +26,7 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'qtype_random'; -$plugin->version = 2025041400; +$plugin->version = 2025041401; $plugin->requires = 2025040800;