From 04e9b8e61432f613296359e6bbd70f1a05b13c03 Mon Sep 17 00:00:00 2001 From: Eric Merrill Date: Thu, 29 May 2014 17:19:37 -0400 Subject: [PATCH 1/2] MDL-29905 question: Try to delete unused hidden/random questions Before going to display the warning about moving in use questions, try to delete unused Random qtype questions, as well as 'hidden' questions (these are questions that were deleted from the category), but were in use at the time. --- lang/en/question.php | 2 +- question/category.php | 52 +++++++++++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/lang/en/question.php b/lang/en/question.php index 2ce0f2c175d..78b328584ce 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -65,7 +65,7 @@ $string['categorycurrent'] = 'Current category'; $string['categorycurrentuse'] = 'Use this category'; $string['categorydoesnotexist'] = 'This category does not exist'; $string['categoryinfo'] = 'Category info'; -$string['categorymove'] = 'The category \'{$a->name}\' contains {$a->count} questions (some of them may be old, hidden, questions that are still in use in some existing quizzes). Please choose another category to move them to.'; +$string['categorymove'] = 'The category \'{$a->name}\' contains {$a->count} questions (some of them may be old, hidden, questions, or Random questions that are still in use in some existing quizzes). Please choose another category to move them to.'; $string['categorymoveto'] = 'Save in category'; $string['categorynamecantbeblank'] = 'The category name cannot be blank.'; $string['clickflag'] = 'Flag question'; diff --git a/question/category.php b/question/category.php index 0d6288710e8..39d5de99332 100644 --- a/question/category.php +++ b/question/category.php @@ -81,21 +81,45 @@ if ($param->moveupcontext || $param->movedowncontext) { // The previous line does a redirect(). } -if ($param->delete && ($questionstomove = $DB->count_records("question", array("category" => $param->delete)))) { - if (!$category = $DB->get_record("question_categories", array("id" => $param->delete))) { // security - print_error('nocate', 'question', $thispageurl->out(), $param->delete); +if ($param->delete) { + $questionstomove = $DB->count_records("question", array("category" => $param->delete)); + + // First pass, try and remove unused random or hidden questions in the category. + if ($questionstomove) { + if (!$category = $DB->get_record("question_categories", array("id" => $param->delete))) { // security + print_error('nocate', 'question', $thispageurl->out(), $param->delete); + } + + $select = "category = ? AND (qtype = 'random' OR hidden = 1)"; + $questions = $DB->get_recordset_select("question", $select, array("category" => $param->delete), '', 'id'); + if ($questions->valid()) { + $question = $questions->current(); + if (question_has_capability_on($question->id, 'edit')) { + foreach ($questions as $question) { + question_delete_question($question->id); + } + $questionstomove = $DB->count_records("question", array("category" => $param->delete)); + } + } + $questions->close(); } - $categorycontext = context::instance_by_id($category->contextid); - $qcobject->moveform = new question_move_form($thispageurl, - array('contexts'=>array($categorycontext), 'currentcat'=>$param->delete)); - if ($qcobject->moveform->is_cancelled()){ - redirect($thispageurl); - } elseif ($formdata = $qcobject->moveform->get_data()) { - /// 'confirm' is the category to move existing questions to - list($tocategoryid, $tocontextid) = explode(',', $formdata->category); - $qcobject->move_questions_and_delete_category($formdata->delete, $tocategoryid); - $thispageurl->remove_params('cat', 'category'); - redirect($thispageurl); + + // Second pass, if we still have questions to move, setup the form . + if ($questionstomove) { + $categorycontext = context::instance_by_id($category->contextid); + $qcobject->moveform = new question_move_form($thispageurl, + array('contexts'=>array($categorycontext), 'currentcat'=>$param->delete)); + if ($qcobject->moveform->is_cancelled()){ + redirect($thispageurl); + } elseif ($formdata = $qcobject->moveform->get_data()) { + /// 'confirm' is the category to move existing questions to + list($tocategoryid, $tocontextid) = explode(',', $formdata->category); + $qcobject->move_questions_and_delete_category($formdata->delete, $tocategoryid); + $thispageurl->remove_params('cat', 'category'); + redirect($thispageurl); + } + } else { + $questionstomove = 0; } } else { $questionstomove = 0; From 5bc25db29cc7ac6421a896920fb1d842f6ace8e7 Mon Sep 17 00:00:00 2001 From: Frederic Massart Date: Wed, 25 May 2016 17:42:43 +0800 Subject: [PATCH 2/2] MDL-29905 question: Auto remove stale questions upon category deletion --- lib/questionlib.php | 31 +++++++++++++++++++++ lib/tests/questionlib_test.php | 50 ++++++++++++++++++++++++++++++++++ question/category.php | 36 ++++++------------------ 3 files changed, 90 insertions(+), 27 deletions(-) diff --git a/lib/questionlib.php b/lib/questionlib.php index 2a2d106f86e..5d766ee21c7 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -220,6 +220,37 @@ function match_grade_options($gradeoptionsfull, $grade, $matchgrades = 'error') } } +/** + * Remove stale questions from a category. + * + * While questions should not be left behind when they are not used any more, + * it does happen, maybe via restore, or old logic, or uncovered scenarios. When + * this happens, the users are unable to delete the question category unless + * they move those stale questions to another one category, but to them the + * category is empty as it does not contain anything. The purpose of this function + * is to detect the questions that may have gone stale and remove them. + * + * You will typically use this prior to checking if the category contains questions. + * + * The stale questions (unused and hidden to the user) handled are: + * - hidden questions + * - random questions + * + * @param int $categoryid The category ID. + */ +function question_remove_stale_questions_from_category($categoryid) { + global $DB; + + $select = 'category = :categoryid AND (qtype = :qtype OR hidden = :hidden)'; + $params = ['categoryid' => $categoryid, 'qtype' => 'random', 'hidden' => 1]; + $questions = $DB->get_recordset_select("question", $select, $params, '', 'id'); + foreach ($questions as $question) { + // The function question_delete_question does not delete questions in use. + question_delete_question($question->id); + } + $questions->close(); +} + /** * Category is about to be deleted, * 1/ All questions are deleted for this question category. diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index 6455616711f..132a111be67 100644 --- a/lib/tests/questionlib_test.php +++ b/lib/tests/questionlib_test.php @@ -397,4 +397,54 @@ class core_questionlib_testcase extends advanced_testcase { $criteria = array('category' => $qcat->id); $this->assertEquals(0, $DB->count_records('question', $criteria)); } + + public function test_question_remove_stale_questions_from_category() { + global $DB; + $this->resetAfterTest(true); + $dg = $this->getDataGenerator(); + $course = $dg->create_course(); + $quiz = $dg->create_module('quiz', ['course' => $course->id]); + + $qgen = $dg->get_plugin_generator('core_question'); + $context = context_system::instance(); + + $qcat1 = $qgen->create_question_category(['contextid' => $context->id]); + $q1a = $qgen->create_question('shortanswer', null, ['category' => $qcat1->id]); // Will be hidden. + $q1b = $qgen->create_question('random', null, ['category' => $qcat1->id]); // Will not be used. + $DB->set_field('question', 'hidden', 1, ['id' => $q1a->id]); + + $qcat2 = $qgen->create_question_category(['contextid' => $context->id]); + $q2a = $qgen->create_question('shortanswer', null, ['category' => $qcat2->id]); // Will be hidden. + $q2b = $qgen->create_question('shortanswer', null, ['category' => $qcat2->id]); // Will be hidden but used. + $q2c = $qgen->create_question('random', null, ['category' => $qcat2->id]); // Will not be used. + $q2d = $qgen->create_question('random', null, ['category' => $qcat2->id]); // Will be used. + $DB->set_field('question', 'hidden', 1, ['id' => $q2a->id]); + $DB->set_field('question', 'hidden', 1, ['id' => $q2b->id]); + quiz_add_quiz_question($q2b->id, $quiz); + quiz_add_quiz_question($q2d->id, $quiz); + + $this->assertEquals(2, $DB->count_records('question', ['category' => $qcat1->id])); + $this->assertEquals(4, $DB->count_records('question', ['category' => $qcat2->id])); + + // Non-existing category, nothing will happen. + question_remove_stale_questions_from_category(0); + $this->assertEquals(2, $DB->count_records('question', ['category' => $qcat1->id])); + $this->assertEquals(4, $DB->count_records('question', ['category' => $qcat2->id])); + + // First category, should be empty afterwards. + question_remove_stale_questions_from_category($qcat1->id); + $this->assertEquals(0, $DB->count_records('question', ['category' => $qcat1->id])); + $this->assertEquals(4, $DB->count_records('question', ['category' => $qcat2->id])); + $this->assertFalse($DB->record_exists('question', ['id' => $q1a->id])); + $this->assertFalse($DB->record_exists('question', ['id' => $q1b->id])); + + // Second category, used questions should be left untouched. + question_remove_stale_questions_from_category($qcat2->id); + $this->assertEquals(0, $DB->count_records('question', ['category' => $qcat1->id])); + $this->assertEquals(2, $DB->count_records('question', ['category' => $qcat2->id])); + $this->assertFalse($DB->record_exists('question', ['id' => $q2a->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $q2b->id])); + $this->assertFalse($DB->record_exists('question', ['id' => $q2c->id])); + $this->assertTrue($DB->record_exists('question', ['id' => $q2d->id])); + } } diff --git a/question/category.php b/question/category.php index 39d5de99332..4e5d02f5db4 100644 --- a/question/category.php +++ b/question/category.php @@ -82,44 +82,26 @@ if ($param->moveupcontext || $param->movedowncontext) { } if ($param->delete) { - $questionstomove = $DB->count_records("question", array("category" => $param->delete)); - - // First pass, try and remove unused random or hidden questions in the category. - if ($questionstomove) { - if (!$category = $DB->get_record("question_categories", array("id" => $param->delete))) { // security - print_error('nocate', 'question', $thispageurl->out(), $param->delete); - } - - $select = "category = ? AND (qtype = 'random' OR hidden = 1)"; - $questions = $DB->get_recordset_select("question", $select, array("category" => $param->delete), '', 'id'); - if ($questions->valid()) { - $question = $questions->current(); - if (question_has_capability_on($question->id, 'edit')) { - foreach ($questions as $question) { - question_delete_question($question->id); - } - $questionstomove = $DB->count_records("question", array("category" => $param->delete)); - } - } - $questions->close(); + if (!$category = $DB->get_record("question_categories", array("id" => $param->delete))) { + print_error('nocate', 'question', $thispageurl->out(), $param->delete); } - // Second pass, if we still have questions to move, setup the form . + question_remove_stale_questions_from_category($param->delete); + $questionstomove = $DB->count_records("question", array("category" => $param->delete)); + + // Second pass, if we still have questions to move, setup the form. if ($questionstomove) { $categorycontext = context::instance_by_id($category->contextid); $qcobject->moveform = new question_move_form($thispageurl, - array('contexts'=>array($categorycontext), 'currentcat'=>$param->delete)); - if ($qcobject->moveform->is_cancelled()){ + array('contexts' => array($categorycontext), 'currentcat' => $param->delete)); + if ($qcobject->moveform->is_cancelled()) { redirect($thispageurl); - } elseif ($formdata = $qcobject->moveform->get_data()) { - /// 'confirm' is the category to move existing questions to + } else if ($formdata = $qcobject->moveform->get_data()) { list($tocategoryid, $tocontextid) = explode(',', $formdata->category); $qcobject->move_questions_and_delete_category($formdata->delete, $tocategoryid); $thispageurl->remove_params('cat', 'category'); redirect($thispageurl); } - } else { - $questionstomove = 0; } } else { $questionstomove = 0;