Skip to content

Commit

Permalink
MDL-68757 questions: don't do output in low-level functions
Browse files Browse the repository at this point in the history
  • Loading branch information
@timhunt
timhunt committed May 18, 2020
1 parent d851183 commit 4a45b71
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 131 deletions.
4 changes: 2 additions & 2 deletions course/classes/category.php
View file
Expand Up @@ -1993,7 +1993,7 @@ public function delete_full($showfeedback = true) {

// 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());
}

Expand Down Expand Up @@ -2153,7 +2153,7 @@ public function delete_move($newparentid, $showfeedback = false) {

// 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');
}
Expand Down
5 changes: 1 addition & 4 deletions course/lib.php
View file
Expand Up @@ -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)) {
Expand Down
7 changes: 2 additions & 5 deletions course/tests/courselib_test.php
View file
Expand Up @@ -1653,11 +1653,8 @@ public function test_course_delete_module($type, $options) {
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;
Expand Down
4 changes: 2 additions & 2 deletions lib/moodlelib.php
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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');
}
Expand Down
71 changes: 16 additions & 55 deletions lib/questionlib.php
View file
Expand Up @@ -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
Expand All @@ -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;
}

Expand All @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down
72 changes: 14 additions & 58 deletions lib/tests/questionlib_test.php
View file
Expand Up @@ -53,18 +53,6 @@ public function setUp() {
$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.
*
Expand Down Expand Up @@ -223,7 +211,7 @@ public function test_altering_tag_instance_context() {
'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',
Expand Down Expand Up @@ -349,23 +337,18 @@ public function test_question_category_delete_safe() {

/**
* 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();

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);
Expand Down Expand Up @@ -396,31 +379,20 @@ public function test_question_delete_context() {
// 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);
Expand All @@ -433,22 +405,16 @@ public function test_question_delete_course($feedback) {

/**
* 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();

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);
Expand All @@ -461,11 +427,8 @@ public function test_question_delete_course_category($feedback) {

/**
* 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();
Expand All @@ -476,26 +439,19 @@ public function test_question_delete_course_category_move_qcats($feedback) {
$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.
Expand Down
20 changes: 15 additions & 5 deletions question/upgrade.txt
View file
Expand Up @@ -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 ===

Expand Down

0 comments on commit 4a45b71

Please sign in to comment.