diff --git a/question/category_class.php b/question/category_class.php index 929ab9e9ded..1713cadea5c 100644 --- a/question/category_class.php +++ b/question/category_class.php @@ -169,7 +169,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 @@ -185,7 +185,6 @@ class question_category_object { * @var array nested lists to display categories. */ public $editlists = array(); - public $newtable; public $tab; public $tabsize = 3; @@ -200,12 +199,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); @@ -438,7 +442,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) { @@ -475,7 +491,6 @@ class question_category_object { $cat->sortorder = 999; $cat->stamp = make_unique_id_code(); $cat->idnumber = $idnumber; - $categoryid = $DB->insert_record("question_categories", $cat); // Log the creation of this category. @@ -493,16 +508,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) { @@ -538,15 +554,14 @@ class question_category_object { } } - $updateidnumber = true; if ((string) $idnumber === '') { $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; - $updateidnumber = false; } } @@ -558,9 +573,7 @@ class question_category_object { $cat->infoformat = $newinfoformat; $cat->parent = $parentid; $cat->contextid = $tocontextid; - if ($updateidnumber) { - $cat->idnumber = $idnumber; - } + $cat->idnumber = $idnumber; if ($newstamprequired) { $cat->stamp = make_unique_id_code(); } 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); + } +}