Merge branch 'MDL-66796' of git://github.com/timhunt/moodle

diff --git a/question/category_class.php b/question/category_class.php
index 929ab9e..1713cad 100644 (file)
--- 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 (file)
index 0000000..4bfaf20
--- /dev/null
+++ b/question/tests/category_class_test.php
@@ -0,0 +1,178 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * 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);
+    }
+}