From 9c4f0fa7467f51581486bc3707a3be3863959a85 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 30 Sep 2019 13:13:06 +0100 Subject: [PATCH 1/2] MDL-66796 question bank: 0 is a valid question category idnumber --- question/category_class.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/question/category_class.php b/question/category_class.php index 856da40420c..6711a833b8d 100644 --- a/question/category_class.php +++ b/question/category_class.php @@ -469,9 +469,7 @@ class question_category_object { $cat->infoformat = $newinfoformat; $cat->sortorder = 999; $cat->stamp = make_unique_id_code(); - if ($idnumber) { - $cat->idnumber = $idnumber; - } + $cat->idnumber = $idnumber; $categoryid = $DB->insert_record("question_categories", $cat); // Log the creation of this category. @@ -552,9 +550,7 @@ class question_category_object { $cat->infoformat = $newinfoformat; $cat->parent = $parentid; $cat->contextid = $tocontextid; - if ($idnumber) { - $cat->idnumber = $idnumber; - } + $cat->idnumber = $idnumber; if ($newstamprequired) { $cat->stamp = make_unique_id_code(); } From 468d7ead0ae34b9c3504b5359795d812b468329b Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 14 Oct 2019 15:27:09 +0100 Subject: [PATCH 2/2] MDL-66796 question bank: fix more bugs with category editing --- question/category_class.php | 52 +++++--- question/tests/category_class_test.php | 178 +++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 17 deletions(-) create mode 100644 question/tests/category_class_test.php diff --git a/question/category_class.php b/question/category_class.php index 6711a833b8d..1b45eb1e931 100644 --- a/question/category_class.php +++ b/question/category_class.php @@ -164,7 +164,7 @@ class question_category_list_item extends list_item { /** - * Class representing q question category + * Class for performing operations on question categories. * * @copyright 1999 onwards Martin Dougiamas {@link http://moodle.com} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -180,7 +180,6 @@ class question_category_object { * @var array nested lists to display categories. */ public $editlists = array(); - public $newtable; public $tab; public $tabsize = 3; @@ -195,12 +194,17 @@ class question_category_object { public $catform; /** - * Constructor + * Constructor. * - * Gets necessary strings and sets relevant path information + * @param int $page page number + * @param moodle_url $pageurl base URL of the display categories page. Used for redirects. + * @param context[] $contexts contexts where the current user can edit categories. + * @param int $currentcat id of the category to be edited. 0 if none. + * @param int|null $defaultcategory id of the current category. null if none. + * @param int $todelete id of the category to delete. 0 if none. + * @param context[] $addcontexts contexts where the current user can add questions. */ public function __construct($page, $pageurl, $contexts, $currentcat, $defaultcategory, $todelete, $addcontexts) { - global $CFG, $COURSE, $OUTPUT; $this->tab = str_repeat(' ', $this->tabsize); @@ -433,7 +437,19 @@ class question_category_object { } /** - * Creates a new category with given params + * Create a new category. + * + * Data is expected to come from question_category_edit_form. + * + * By default redirects on success, unless $return is true. + * + * @param string $newparent 'categoryid,contextid' of the parent category. + * @param string $newcategory the name. + * @param string $newinfo the description. + * @param bool $return if true, return rather than redirecting. + * @param int|string $newinfoformat description format. One of the FORMAT_ constants. + * @param null $idnumber the idnumber. '' is converted to null. + * @return bool|int New category id if successful, else false. */ public function add_category($newparent, $newcategory, $newinfo, $return = false, $newinfoformat = FORMAT_HTML, $idnumber = null) { @@ -487,16 +503,17 @@ class question_category_object { } /** - * Updates an existing category with given params + * Updates an existing category with given params. * - * @param int $updateid - * @param int $newparent - * @param string $newname - * @param string $newinfo - * @param int $newinfoformat - * @param int $idnumber - * @param bool $redirect - * @return int + * Warning! parameter order and meaning confusingly different from add_category in some ways! + * + * @param int $updateid id of the category to update. + * @param int $newparent 'categoryid,contextid' of the parent category to set. + * @param string $newname category name. + * @param string $newinfo category description. + * @param int|string $newinfoformat description format. One of the FORMAT_ constants. + * @param int $idnumber the idnumber. '' is converted to null. + * @param bool $redirect if true, will redirect once the DB is updated (default). */ public function update_category($updateid, $newparent, $newname, $newinfo, $newinfoformat = FORMAT_HTML, $idnumber = null, $redirect = true) { @@ -536,8 +553,9 @@ class question_category_object { $idnumber = null; } else if (!empty($tocontextid)) { // While this check already exists in the form validation, this is a backstop preventing unnecessary errors. - if ($DB->record_exists('question_categories', - ['idnumber' => $idnumber, 'contextid' => $tocontextid])) { + if ($DB->record_exists_select('question_categories', + 'idnumber = ? AND contextid = ? AND id <> ?', + [$idnumber, $tocontextid, $updateid])) { $idnumber = null; } } diff --git a/question/tests/category_class_test.php b/question/tests/category_class_test.php new file mode 100644 index 00000000000..4bfaf200950 --- /dev/null +++ b/question/tests/category_class_test.php @@ -0,0 +1,178 @@ +. + +/** + * Events tests. + * + * @package core_question + * @copyright 2019 the Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; + +require_once($CFG->dirroot . '/question/editlib.php'); +require_once($CFG->dirroot . '/question/category_class.php'); + +class core_question_category_class_testcase extends advanced_testcase { + + /** + * @var question_category_object used in the tests. + */ + protected $qcobject; + + /** + * @var context a context to use. + */ + protected $context; + + /** + * @var stdClass top category in context. + */ + protected $topcat; + + protected function setUp() { + parent::setUp(); + self::setAdminUser(); + $this->resetAfterTest(); + $this->context = context_course::instance(SITEID); + $contexts = new question_edit_contexts($this->context); + $this->topcat = question_get_top_category($this->context->id, true); + $this->qcobject = new question_category_object(null, + new moodle_url('/question/category.php', ['courseid' => SITEID]), + $contexts->having_one_edit_tab_cap('categories'), 0, null, 0, + $contexts->having_cap('moodle/question:add')); + } + + /** + * Test creating a category. + */ + public function test_add_category_no_idnumber() { + global $DB; + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'New category', '', true, FORMAT_HTML, ''); // No idnumber passed as '' to match form data. + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New category', $newcat->name); + $this->assertNull($newcat->idnumber); + } + + /** + * Test creating a category with a tricky idnumber. + */ + public function test_add_category_set_idnumber_0() { + global $DB; + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'New category', '', true, FORMAT_HTML, '0'); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New category', $newcat->name); + $this->assertSame('0', $newcat->idnumber); + } + + /** + * Trying to add a category with duplicate idnumber blanks it. + * (In reality, this would probably get caught by form validation.) + */ + public function test_add_category_try_to_set_duplicate_idnumber() { + global $DB; + + $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'Existing category', '', true, FORMAT_HTML, 'frog'); + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'New category', '', true, FORMAT_HTML, 'frog'); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New category', $newcat->name); + $this->assertNull($newcat->idnumber); + } + + /** + * Test updating a category. + */ + public function test_update_category() { + global $DB; + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'Old name', 'Description', true, FORMAT_HTML, 'frog'); + + $this->qcobject->update_category($id, $this->topcat->id . ',' . $this->topcat->contextid, + 'New name', 'New description', FORMAT_HTML, '0', false); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New name', $newcat->name); + $this->assertSame('0', $newcat->idnumber); + } + + /** + * Test updating a category to remove the idnumber. + */ + public function test_update_category_removing_idnumber() { + global $DB; + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'Old name', 'Description', true, FORMAT_HTML, 'frog'); + + $this->qcobject->update_category($id, $this->topcat->id . ',' . $this->topcat->contextid, + 'New name', 'New description', FORMAT_HTML, '', false); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New name', $newcat->name); + $this->assertNull($newcat->idnumber); + } + + /** + * Test updating a category without changing the idnumber. + */ + public function test_update_category_dont_change_idnumber() { + global $DB; + + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'Old name', 'Description', true, FORMAT_HTML, 'frog'); + + $this->qcobject->update_category($id, $this->topcat->id . ',' . $this->topcat->contextid, + 'New name', 'New description', FORMAT_HTML, 'frog', false); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New name', $newcat->name); + $this->assertSame('frog', $newcat->idnumber); + } + + /** + * Trying to update a category so its idnumber duplicates idnumber blanks it. + * (In reality, this would probably get caught by form validation.) + */ + public function test_update_category_try_to_set_duplicate_idnumber() { + global $DB; + + $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'Existing category', '', true, FORMAT_HTML, 'toad'); + $id = $this->qcobject->add_category($this->topcat->id . ',' . $this->topcat->contextid, + 'old name', '', true, FORMAT_HTML, 'frog'); + + $this->qcobject->update_category($id, $this->topcat->id . ',' . $this->topcat->contextid, + 'New name', '', FORMAT_HTML, 'toad', false); + + $newcat = $DB->get_record('question_categories', ['id' => $id], '*', MUST_EXIST); + $this->assertSame('New name', $newcat->name); + $this->assertNull($newcat->idnumber); + } +}