diff --git a/question/bank/managecategories/category.php b/question/bank/managecategories/category.php index 183fff8f942..ade905f02ec 100644 --- a/question/bank/managecategories/category.php +++ b/question/bank/managecategories/category.php @@ -107,7 +107,7 @@ if ($param->delete) { helper::question_remove_stale_questions_from_category($param->delete); - $questionstomove = $DB->count_records('question_bank_entries', ['questioncategoryid' => $param->delete]); + $questionstomove = count($qcobject->get_real_question_ids_in_category($param->delete)); // Second pass, if we still have questions to move, setup the form. if ($questionstomove) { diff --git a/question/bank/managecategories/classes/question_category_object.php b/question/bank/managecategories/classes/question_category_object.php index 39e6d561cc5..b84dfcc288d 100644 --- a/question/bank/managecategories/classes/question_category_object.php +++ b/question/bank/managecategories/classes/question_category_object.php @@ -324,17 +324,8 @@ class question_category_object { * @throws \dml_exception */ public function move_questions(int $oldcat, int $newcat): void { - global $DB; - - $sql = "SELECT 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 - WHERE qbe.questioncategoryid = ? - AND (q.parent = 0 OR q.parent = q.id)"; - - $questionids = $DB->get_records_sql_menu($sql, [$oldcat]); - question_move_questions_to_category(array_keys($questionids), $newcat); + $questionids = $this->get_real_question_ids_in_category($oldcat); + question_move_questions_to_category($questionids, $newcat); } /** @@ -484,13 +475,7 @@ class question_category_object { // If the category name has changed, rename any random questions in that category. if ($oldcat->name != $cat->name) { // Get the question ids for each question category. - $sql = "SELECT q.id - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.questioncategoryid = ?"; - - $questionids = $DB->get_records_sql($sql, [$cat->id]); + $questionids = $this->get_real_question_ids_in_category($cat->id); foreach ($questionids as $question) { $where = "qtype = 'random' AND id = ? AND " . $DB->sql_compare_text('questiontext') . " = ?"; @@ -516,4 +501,27 @@ class question_category_object { redirect($this->pageurl); } } + + /** + * Returns ids of the question in the given question category. + * + * This method only returns the real question. It does not include + * subquestions of question types like multianswer. + * + * @param int $categoryid id of the category. + * @return int[] array of question ids. + */ + public function get_real_question_ids_in_category(int $categoryid): array { + global $DB; + + $sql = "SELECT q.id + FROM {question} q + JOIN {question_versions} qv ON qv.questionid = q.id + JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid + WHERE qbe.questioncategoryid = :categoryid + AND (q.parent = 0 OR q.parent = q.id)"; + + $questionids = $DB->get_records_sql($sql, ['categoryid' => $categoryid]); + return array_keys($questionids); + } } diff --git a/question/bank/managecategories/tests/behat/question_categories.feature b/question/bank/managecategories/tests/behat/question_categories.feature index e2b8e7c7bd5..9c32b807fdd 100644 --- a/question/bank/managecategories/tests/behat/question_categories.feature +++ b/question/bank/managecategories/tests/behat/question_categories.feature @@ -71,3 +71,20 @@ Feature: A teacher can put questions in categories in the question bank Then I should not see "Used category" And I follow "Add category" And I should see "Default for C1 (1)" + + @_file_upload + Scenario: Multi answer questions with their child questions can be moved to another category when the current category is deleted + When I navigate to "Question bank" in current page administration + And I select "Import" from the "Question bank tertiary navigation" singleselect + And I set the field "id_format_xml" to "1" + And I upload "question/format/xml/tests/fixtures/multianswer.xml" file to "Import" filemanager + And I press "id_submitbutton" + And I press "Continue" + And I select "Categories" from the "Question bank tertiary navigation" singleselect + And I click on "Delete" "link" in the "Default for Test images in backup" "list_item" + And I should see "The category 'Default for Test images in backup' contains 1 questions" + And I select "Used category" from the "Category" singleselect + And I press "Save in category" + Then I should not see "Default for Test images in backup" + And I follow "Add category" + And I should see "Used category (2)" diff --git a/question/bank/managecategories/tests/helper_test.php b/question/bank/managecategories/tests/helper_test.php index f59865247f0..12c9cf113f7 100644 --- a/question/bank/managecategories/tests/helper_test.php +++ b/question/bank/managecategories/tests/helper_test.php @@ -16,6 +16,9 @@ namespace qbank_managecategories; +use moodle_url; +use core_question\local\bank\question_edit_contexts; + /** * Unit tests for helper class. * @@ -47,6 +50,11 @@ class helper_test extends \advanced_testcase { */ protected $quiz; + /** + * @var question_category_object used in the tests. + */ + protected $qcobject; + /** * Tests initial setup. */ @@ -60,6 +68,12 @@ class helper_test extends \advanced_testcase { $this->quiz = $datagenerator->create_module('quiz', ['course' => $this->course->id]); $this->qgenerator = $datagenerator->get_plugin_generator('core_question'); $this->context = \context_module::instance($this->quiz->cmid); + + $contexts = new question_edit_contexts($this->context); + $this->qcobject = new question_category_object(null, + new moodle_url('/question/bank/managecategories/category.php', ['courseid' => SITEID]), + $contexts->having_one_edit_tab_cap('categories'), 0, null, 0, + $contexts->having_cap('moodle/question:add')); } /** @@ -98,45 +112,25 @@ class helper_test extends \advanced_testcase { $q1b = $this->qgenerator->create_question('random', null, ['category' => $qcat1->id]); // Will not be used. $q2c = $this->qgenerator->create_question('random', null, ['category' => $qcat2->id]); // Will not be used. - $sql = "SELECT count(q.id) - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.questioncategoryid = ?"; - $this->assertEquals(2, $DB->count_records_sql($sql, [$qcat1->id])); - $this->assertEquals(3, $DB->count_records_sql($sql, [$qcat2->id])); + $this->assertEquals(2, count($this->qcobject->get_real_question_ids_in_category($qcat1->id))); + $this->assertEquals(3, count($this->qcobject->get_real_question_ids_in_category($qcat2->id))); // Non-existing category, nothing will happen. helper::question_remove_stale_questions_from_category(0); - $sql = "SELECT count(q.id) - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.questioncategoryid = ?"; - $this->assertEquals(2, $DB->count_records_sql($sql, [$qcat1->id])); - $this->assertEquals(3, $DB->count_records_sql($sql, [$qcat2->id])); + $this->assertEquals(2, count($this->qcobject->get_real_question_ids_in_category($qcat1->id))); + $this->assertEquals(3, count($this->qcobject->get_real_question_ids_in_category($qcat2->id))); // First category, should be empty afterwards. helper::question_remove_stale_questions_from_category($qcat1->id); - $sql = "SELECT count(q.id) - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.questioncategoryid = ?"; - $this->assertEquals(0, $DB->count_records_sql($sql, [$qcat1->id])); - $this->assertEquals(3, $DB->count_records_sql($sql, [$qcat2->id])); + $this->assertEquals(0, count($this->qcobject->get_real_question_ids_in_category($qcat1->id))); + $this->assertEquals(3, count($this->qcobject->get_real_question_ids_in_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. helper::question_remove_stale_questions_from_category($qcat2->id); - $sql = "SELECT count(q.id) - FROM {question} q - JOIN {question_versions} qv ON qv.questionid = q.id - JOIN {question_bank_entries} qbe ON qbe.id = qv.questionbankentryid - WHERE qbe.questioncategoryid = ?"; - $this->assertEquals(0, $DB->count_records_sql($sql, [$qcat1->id])); - $this->assertEquals(1, $DB->count_records_sql($sql, [$qcat2->id])); + $this->assertEquals(0, count($this->qcobject->get_real_question_ids_in_category($qcat1->id))); + $this->assertEquals(1, count($this->qcobject->get_real_question_ids_in_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])); diff --git a/question/bank/managecategories/tests/question_category_object_test.php b/question/bank/managecategories/tests/question_category_object_test.php index ed889290982..8b6efbaacd7 100644 --- a/question/bank/managecategories/tests/question_category_object_test.php +++ b/question/bank/managecategories/tests/question_category_object_test.php @@ -340,4 +340,100 @@ class question_category_object_test extends \advanced_testcase { $this->assertDebuggingNotCalled(); } + + /** + * Test that get_real_question_ids_in_category() returns question id + * of a shortanswer question in a category. + * + * @covers ::get_real_question_ids_in_category + */ + public function test_get_real_question_ids_in_category_shortanswer() { + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $categoryid = $this->defaultcategoryobj->id; + + // Short answer question is made of one question. + $shortanswer = $generator->create_question('shortanswer', null, ['category' => $categoryid]); + $questionids = $this->qcobject->get_real_question_ids_in_category($categoryid); + $this->assertCount(1, $questionids); + $this->assertContains($shortanswer->id, $questionids); + } + + /** + * Test that get_real_question_ids_in_category() returns question id + * of a multianswer question in a category. + * + * @covers ::get_real_question_ids_in_category + */ + public function test_get_real_question_ids_in_category_multianswer() { + global $DB; + $countq = $DB->count_records('question'); + $countqbe = $DB->count_records('question_bank_entries'); + + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $categoryid = $this->defaultcategoryobj->id; + + // Multi answer question is made of one parent and two child questions. + $multianswer = $generator->create_question('multianswer', null, ['category' => $categoryid]); + $questionids = $this->qcobject->get_real_question_ids_in_category($categoryid); + $this->assertCount(1, $questionids); + $this->assertContains($multianswer->id, $questionids); + $this->assertEquals(3, $DB->count_records('question') - $countq); + $this->assertEquals(3, $DB->count_records('question_bank_entries') - $countqbe); + } + + /** + * Test that get_real_question_ids_in_category() returns question ids + * of two versions of a multianswer question in a category. + * + * @covers ::get_real_question_ids_in_category + */ + public function test_get_real_question_ids_in_category_multianswer_two_versions() { + global $DB; + $countq = $DB->count_records('question'); + $countqv = $DB->count_records('question_versions'); + $countqbe = $DB->count_records('question_bank_entries'); + + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $categoryid = $this->defaultcategoryobj->id; + + // Create two versions of a multianswer question which will lead to + // 2 parents and 4 child questions in the question bank. + $multianswer = $generator->create_question('multianswer', null, ['category' => $categoryid]); + $multianswernew = $generator->update_question($multianswer, null, ['name' => 'This is a new version']); + $questionids = $this->qcobject->get_real_question_ids_in_category($categoryid); + $this->assertCount(2, $questionids); + $this->assertContains($multianswer->id, $questionids); + $this->assertContains($multianswernew->id, $questionids); + $this->assertEquals(6, $DB->count_records('question') - $countq); + $this->assertEquals(6, $DB->count_records('question_versions') - $countqv); + $this->assertEquals(3, $DB->count_records('question_bank_entries') - $countqbe); + } + + /** + * Test that get_real_question_ids_in_category() returns question id + * of a multianswer question in a category even if their child questions are + * linked to a category that doesn't exist. + * + * @covers ::get_real_question_ids_in_category + */ + public function test_get_real_question_ids_in_category_multianswer_bad_data() { + global $DB; + $countqbe = $DB->count_records('question_bank_entries'); + + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $categoryid = $this->defaultcategoryobj->id; + + // Multi answer question is made of one parent and two child questions. + $multianswer = $generator->create_question('multianswer', null, ['category' => $categoryid]); + $qversion = $DB->get_record('question_versions', ['questionid' => $multianswer->id]); + + // Update category id for child questions to a category that doesn't exist. + $DB->set_field_select('question_bank_entries', 'questioncategoryid', + 123456, 'id <> :id', ['id' => $qversion->questionbankentryid]); + + $questionids = $this->qcobject->get_real_question_ids_in_category($categoryid); + $this->assertCount(1, $questionids); + $this->assertContains($multianswer->id, $questionids); + $this->assertEquals(3, $DB->count_records('question_bank_entries') - $countqbe); + } }