Commit 4a45b711 authored by Tim Hunt's avatar Tim Hunt
Browse files

MDL-68757 questions: don't do output in low-level functions

parent d8511836
......@@ -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');
}
......
......@@ -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)) {
......
......@@ -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;
......
......@@ -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');
}
......
......@@ -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;
}
......
......@@ -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.
......
......@@ -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 ===
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment