diff --git a/course/classes/category.php b/course/classes/category.php index aea6ed0df83..df21e273627 100644 --- a/course/classes/category.php +++ b/course/classes/category.php @@ -1993,7 +1993,7 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr // Now delete anything that may depend on course category context. grade_course_category_delete($this->id, 0, $showfeedback); - if (!question_delete_course_category($this, 0, $showfeedback)) { + if (!question_delete_course_category($this, null)) { throw new moodle_exception('cannotdeletecategoryquestions', '', '', $this->get_formatted_name()); } @@ -2153,7 +2153,7 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr // Now delete anything that may depend on course category context. grade_course_category_delete($this->id, $newparentid, $showfeedback); - if (!question_delete_course_category($this, $newparentcat, $showfeedback)) { + if (!question_delete_course_category($this, $newparentcat)) { if ($showfeedback) { echo $OUTPUT->notification(get_string('errordeletingquestionsfromcategory', 'question', $catname), 'notifysuccess'); } diff --git a/course/lib.php b/course/lib.php index 7268824761a..13b466733e5 100644 --- a/course/lib.php +++ b/course/lib.php @@ -1054,10 +1054,7 @@ function course_delete_module($cmid, $async = false) { } } - // Delete activity context questions and question categories. - $showinfo = !defined('AJAX_SCRIPT') || AJAX_SCRIPT == '0'; - - question_delete_activity($cm, $showinfo); + question_delete_activity($cm); // Call the delete_instance function, if it returns false throw an exception. if (!$deleteinstancefunction($cm->instance)) { diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php index fef3933a62f..8f00f155602 100644 --- a/course/tests/courselib_test.php +++ b/course/tests/courselib_test.php @@ -1653,11 +1653,8 @@ class core_course_courselib_testcase extends advanced_testcase { case 'quiz': $qgen = $this->getDataGenerator()->get_plugin_generator('core_question'); $qcat = $qgen->create_question_category(array('contextid' => $modcontext->id)); - $questions = array( - $qgen->create_question('shortanswer', null, array('category' => $qcat->id)), - $qgen->create_question('shortanswer', null, array('category' => $qcat->id)), - ); - $this->expectOutputRegex('/'.get_string('unusedcategorydeleted', 'question').'/'); + $qgen->create_question('shortanswer', null, array('category' => $qcat->id)); + $qgen->create_question('shortanswer', null, array('category' => $qcat->id)); break; default: break; diff --git a/lib/moodlelib.php b/lib/moodlelib.php index f1c9a30e560..4cf5faf808c 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -5270,7 +5270,7 @@ function remove_course_contents($courseid, $showfeedback = true, array $options foreach ($instances as $cm) { if ($cm->id) { // Delete activity context questions and question categories. - question_delete_activity($cm, $showfeedback); + question_delete_activity($cm); // Notify the competency subsystem. \core_competency\api::hook_course_module_deleted($cm); } @@ -5330,7 +5330,7 @@ function remove_course_contents($courseid, $showfeedback = true, array $options } // Delete questions and question categories. - question_delete_course($course, $showfeedback); + question_delete_course($course); if ($showfeedback) { echo $OUTPUT->notification($strdeleted.get_string('questions', 'question'), 'notifysuccess'); } diff --git a/lib/questionlib.php b/lib/questionlib.php index 8f2c8344469..993aee48d28 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -402,17 +402,12 @@ function question_delete_question($questionid) { /** * All question categories and their questions are deleted for this context id. * - * @param object $contextid The contextid to delete question categories from - * @return array Feedback from deletes (if any) + * @param int $contextid The contextid to delete question categories from + * @return array only returns an empty array for backwards compatibility. */ function question_delete_context($contextid) { global $DB; - //To store feedback to be showed at the end of the process - $feedbackdata = array(); - - //Cache some strings - $strcatdeleted = get_string('unusedcategorydeleted', 'question'); $fields = 'id, parent, name, contextid'; if ($categories = $DB->get_records('question_categories', array('contextid' => $contextid), 'parent', $fields)) { //Sort categories following their tree (parent-child) relationships @@ -421,32 +416,21 @@ function question_delete_context($contextid) { foreach ($categories as $category) { question_category_delete_safe($category); - - //Fill feedback - $feedbackdata[] = array($category->name, $strcatdeleted); } } - return $feedbackdata; + return []; } /** * All question categories and their questions are deleted for this course. * * @param stdClass $course an object representing the activity - * @param boolean $feedback to specify if the process must output a summary of its work - * @return boolean + * @param bool $notused this argument is not used any more. Kept for backwards compatibility. + * @return bool always true. */ -function question_delete_course($course, $feedback=true) { +function question_delete_course($course, $notused = false) { $coursecontext = context_course::instance($course->id); - $feedbackdata = question_delete_context($coursecontext->id, $feedback); - - // Inform about changes performed if feedback is enabled. - if ($feedback && $feedbackdata) { - $table = new html_table(); - $table->head = array(get_string('category', 'question'), get_string('action')); - $table->data = $feedbackdata; - echo html_writer::table($table); - } + question_delete_context($coursecontext->id); return true; } @@ -455,26 +439,18 @@ function question_delete_course($course, $feedback=true) { * 1/ All question categories and their questions are deleted for this course category. * 2/ All questions are moved to new category * - * @param object|core_course_category $category course category object - * @param object|core_course_category $newcategory empty means everything deleted, otherwise id of + * @param stdClass|core_course_category $category course category object + * @param stdClass|core_course_category $newcategory empty means everything deleted, otherwise id of * category where content moved - * @param boolean $feedback to specify if the process must output a summary of its work + * @param bool $notused this argument is no longer used. Kept for backwards compatibility. * @return boolean */ -function question_delete_course_category($category, $newcategory, $feedback=true) { - global $DB, $OUTPUT; +function question_delete_course_category($category, $newcategory, $notused=false) { + global $DB; $context = context_coursecat::instance($category->id); if (empty($newcategory)) { - $feedbackdata = question_delete_context($context->id, $feedback); - - // Output feedback if requested. - if ($feedback && $feedbackdata) { - $table = new html_table(); - $table->head = array(get_string('questioncategory', 'question'), get_string('action')); - $table->data = $feedbackdata; - echo html_writer::table($table); - } + question_delete_context($context->id); } else { // Move question categories to the new context. @@ -491,14 +467,6 @@ function question_delete_course_category($category, $newcategory, $feedback=true // Now delete the top category. $DB->delete_records('question_categories', array('id' => $topcategory->id)); } - - if ($feedback) { - $a = new stdClass(); - $a->oldplace = $context->get_context_name(); - $a->newplace = $newcontext->get_context_name(); - echo $OUTPUT->notification( - get_string('movedquestionsandcategories', 'question', $a), 'notifysuccess'); - } } return true; @@ -542,21 +510,14 @@ function question_save_from_deletion($questionids, $newcontextid, $oldplace, * All question categories and their questions are deleted for this activity. * * @param object $cm the course module object representing the activity - * @param boolean $feedback to specify if the process must output a summary of its work + * @param bool $notused the argument is not used any more. Kept for backwards compatibility. * @return boolean */ -function question_delete_activity($cm, $feedback=true) { +function question_delete_activity($cm, $notused = false) { global $DB; $modcontext = context_module::instance($cm->id); - $feedbackdata = question_delete_context($modcontext->id, $feedback); - // Inform about changes performed if feedback is enabled. - if ($feedback && $feedbackdata) { - $table = new html_table(); - $table->head = array(get_string('category', 'question'), get_string('action')); - $table->data = $feedbackdata; - echo html_writer::table($table); - } + question_delete_context($modcontext->id); return true; } diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index 59a2bcbfc72..3b79c46dac7 100644 --- a/lib/tests/questionlib_test.php +++ b/lib/tests/questionlib_test.php @@ -53,18 +53,6 @@ class core_questionlib_testcase extends advanced_testcase { $this->resetAfterTest(); } - /** - * Return true and false to test functions with feedback on and off. - * - * @return array Test data - */ - public function provider_feedback() { - return array( - 'Feedback test' => array(true), - 'No feedback test' => array(false) - ); - } - /** * Setup a course, a quiz, a question category and a question for testing. * @@ -223,7 +211,7 @@ class core_questionlib_testcase extends advanced_testcase { 'contextid' => $questioncat2->contextid))); // Now we want to test deleting the course category and moving the questions to another category. - question_delete_course_category($coursecat1, $coursecat2, false); + question_delete_course_category($coursecat1, $coursecat2); // Test that all tag_instances belong to one context. $this->assertEquals(8, $DB->count_records('tag_instance', array('component' => 'core_question', @@ -349,11 +337,8 @@ class core_questionlib_testcase extends advanced_testcase { /** * This function tests the question_delete_activity function. - * - * @param bool $feedback Whether to return feedback - * @dataProvider provider_feedback */ - public function test_question_delete_activity($feedback) { + public function test_question_delete_activity() { global $DB; $this->resetAfterTest(true); $this->setAdminUser(); @@ -361,11 +346,9 @@ class core_questionlib_testcase extends advanced_testcase { list($category, $course, $quiz, $qcat, $questions) = $this->setup_quiz_and_questions(); $cm = get_coursemodule_from_instance('quiz', $quiz->id); - // Test that the feedback works. - if ($feedback) { - $this->expectOutputRegex('|'.get_string('unusedcategorydeleted', 'question').'|'); - } - question_delete_activity($cm, $feedback); + + // Test the deletion. + question_delete_activity($cm); // Verify category deleted. $criteria = array('id' => $qcat->id); @@ -396,31 +379,20 @@ class core_questionlib_testcase extends advanced_testcase { // Verify questions deleted or moved. $criteria = array('category' => $qcat->id); $this->assertEquals(0, $DB->count_records('question', $criteria)); - - // Test that the feedback works. - $expected[] = array('top', get_string('unusedcategorydeleted', 'question')); - $expected[] = array($qcat->name, get_string('unusedcategorydeleted', 'question')); - $this->assertEquals($expected, $result); } /** * This function tests the question_delete_course function. - * - * @param bool $feedback Whether to return feedback - * @dataProvider provider_feedback */ - public function test_question_delete_course($feedback) { + public function test_question_delete_course() { global $DB; $this->resetAfterTest(true); $this->setAdminUser(); list($category, $course, $quiz, $qcat, $questions) = $this->setup_quiz_and_questions('course'); - // Test that the feedback works. - if ($feedback) { - $this->expectOutputRegex('|'.get_string('unusedcategorydeleted', 'question').'|'); - } - question_delete_course($course, $feedback); + // Test the deletion. + question_delete_course($course); // Verify category deleted. $criteria = array('id' => $qcat->id); @@ -433,11 +405,8 @@ class core_questionlib_testcase extends advanced_testcase { /** * This function tests the question_delete_course_category function. - * - * @param bool $feedback Whether to return feedback - * @dataProvider provider_feedback */ - public function test_question_delete_course_category($feedback) { + public function test_question_delete_course_category() { global $DB; $this->resetAfterTest(true); $this->setAdminUser(); @@ -445,10 +414,7 @@ class core_questionlib_testcase extends advanced_testcase { list($category, $course, $quiz, $qcat, $questions) = $this->setup_quiz_and_questions('category'); // Test that the feedback works. - if ($feedback) { - $this->expectOutputRegex('|'.get_string('unusedcategorydeleted', 'question').'|'); - } - question_delete_course_category($category, 0, $feedback); + question_delete_course_category($category, null); // Verify category deleted. $criteria = array('id' => $qcat->id); @@ -461,11 +427,8 @@ class core_questionlib_testcase extends advanced_testcase { /** * This function tests the question_delete_course_category function when it is supposed to move question categories. - * - * @param bool $feedback Whether to return feedback - * @dataProvider provider_feedback */ - public function test_question_delete_course_category_move_qcats($feedback) { + public function test_question_delete_course_category_move_qcats() { global $DB; $this->resetAfterTest(true); $this->setAdminUser(); @@ -476,26 +439,19 @@ class core_questionlib_testcase extends advanced_testcase { $questionsinqcat1 = count($questions1); $questionsinqcat2 = count($questions2); - // Test that the feedback works. - if ($feedback) { - $a = new stdClass(); - $a->oldplace = context::instance_by_id($qcat1->contextid)->get_context_name(); - $a->newplace = context::instance_by_id($qcat2->contextid)->get_context_name(); - $this->expectOutputRegex('|'.get_string('movedquestionsandcategories', 'question', $a).'|'); - } - question_delete_course_category($category1, $category2, $feedback); + // Test the delete. + question_delete_course_category($category1, $category2); // Verify category not deleted. $criteria = array('id' => $qcat1->id); $this->assertEquals(1, $DB->count_records('question_categories', $criteria)); // Verify questions are moved. - $criteria = array('category' => $qcat1->id); $params = array($qcat2->contextid); $actualquestionscount = $DB->count_records_sql("SELECT COUNT(*) FROM {question} q JOIN {question_categories} qc ON q.category = qc.id - WHERE qc.contextid = ?", $params, $criteria); + WHERE qc.contextid = ?", $params); $this->assertEquals($questionsinqcat1 + $questionsinqcat2, $actualquestionscount); // Verify there is just a single top-level category. diff --git a/question/upgrade.txt b/question/upgrade.txt index 00251c72a1f..6354f1ff602 100644 --- a/question/upgrade.txt +++ b/question/upgrade.txt @@ -2,12 +2,22 @@ This files describes API changes for code that uses the question API. === 3.9 == -For years, the ..._questions_in_use callback has been the right way for plugins to -tell the core question system if questions are required. Previously this callback -only worked in mods. Now it works in all plugins. +1) For years, the ..._questions_in_use callback has been the right way for plugins to + tell the core question system if questions are required. Previously this callback + only worked in mods. Now it works in all plugins. + + At the same time, if you are still relying on the legacy ..._question_list_instances + callback for this, you will now get a debugging warning telling you to upgrade. + +2) Previously, the functions question_delete_activity, question_delete_course and + question_delete_course_category would echo output. This was not correct behaviour for + a low-level API function. Now, they no longer output. Related to this, the helper + function they use, question_delete_context, now always returns an empty array. + + This probably won't acutally cause you any problems. However, you may previously + have had to add expectOutputRegex calls to your unit tests to avoid warnings about + risky tests. If you have done that, those tests will now fail until you delete that expectation. -At the same time, if you are still relying on the legacy ..._question_list_instances -callback for this, you will now get a debugging warning telling you to upgrade. === 3.8 ===