MDL-34684 tool_health: Course categories checking refinements
authorDavid Monllao <davidm@moodle.com>
Mon, 24 Nov 2014 05:57:47 +0000 (13:57 +0800)
committerDavid Monllao <davidm@moodle.com>
Thu, 4 Dec 2014 03:37:16 +0000 (11:37 +0800)
Minor changes:
- Updating all course categories that are part of a loop
- Moved functions to tool_health scope
- Moved tests to tool_health scope
- Raised issue importance
- Minor coding style changes

Thanks to Marko Vidberg.

admin/tool/health/index.php
admin/tool/health/locallib.php [new file with mode: 0644]
admin/tool/health/tests/healthlib_test.php [moved from lib/tests/adminlib_test.php with 56% similarity]
lib/adminlib.php

index 6a03291..37fce0a 100644 (file)
@@ -28,6 +28,7 @@
     $extraws = ob_get_clean();
 
     require_once($CFG->libdir.'/adminlib.php');
+    require_once($CFG->dirroot . '/' . $CFG->admin . '/tool/health/locallib.php');
 
     admin_externalpage_setup('toolhealth');
 
@@ -603,10 +604,10 @@ class problem_000017 extends problem_base {
             $categories = $DB->get_records('question_categories', array(), 'id');
 
             // Look for missing parents.
-            $missingparent = health_category_find_missing_parents($categories);
+            $missingparent = tool_health_category_find_missing_parents($categories);
 
             // Look for loops.
-            $loops = health_category_find_loops($categories);
+            $loops = tool_health_category_find_loops($categories);
 
             $answer = array($missingparent, $loops);
         }
@@ -627,8 +628,8 @@ class problem_000017 extends problem_base {
                 ' structures by the question_categories.parent field. Sometimes ' .
                 ' this tree structure gets messed up.</p>';
 
-        $description .= health_category_list_missing_parents($missingparent);
-        $description .= health_category_list_loops($loops);
+        $description .= tool_health_category_list_missing_parents($missingparent);
+        $description .= tool_health_category_list_loops($loops);
 
         return $description;
     }
@@ -639,7 +640,7 @@ class problem_000017 extends problem_base {
      * @link https://tracker.moodle.org/browse/MDL-34684
      * @return string Formatted html to be output to the browser with instructions and sql statements to run
      */
-    function solution() {
+    public function solution() {
         global $CFG;
         list($missingparent, $loops) = $this->find_problems();
 
@@ -664,6 +665,9 @@ class problem_000017 extends problem_base {
 
 /**
  * Check course categories tree structure for problems.
+ *
+ * @copyright  2013 Marko Vidberg
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class problem_000018 extends problem_base {
     /**
@@ -671,7 +675,7 @@ class problem_000018 extends problem_base {
      *
      * @return string Title of problem.
      */
-    function title() {
+    public function title() {
         return 'Course categories tree structure';
     }
 
@@ -681,7 +685,7 @@ class problem_000018 extends problem_base {
      * @uses $DB
      * @return array List of categories that contain missing parents or loops.
      */
-    function find_problems() {
+    public function find_problems() {
         global $DB;
         static $answer = null;
 
@@ -689,10 +693,10 @@ class problem_000018 extends problem_base {
             $categories = $DB->get_records('course_categories', array(), 'id');
 
             // Look for missing parents.
-            $missingparent = health_category_find_missing_parents($categories);
+            $missingparent = tool_health_category_find_missing_parents($categories);
 
             // Look for loops.
-            $loops = health_category_find_loops($categories);
+            $loops = tool_health_category_find_loops($categories);
 
             $answer = array($missingparent, $loops);
         }
@@ -705,7 +709,7 @@ class problem_000018 extends problem_base {
      *
      * @return boolean True if either missing parents or loops found
      */
-    function exists() {
+    public function exists() {
         list($missingparent, $loops) = $this->find_problems();
         return !empty($missingparent) || !empty($loops);
     }
@@ -715,8 +719,8 @@ class problem_000018 extends problem_base {
      *
      * @return constant Problem severity.
      */
-    function severity() {
-        return SEVERITY_ANNOYANCE;
+    public function severity() {
+        return SEVERITY_SIGNIFICANT;
     }
 
     /**
@@ -724,15 +728,15 @@ class problem_000018 extends problem_base {
      *
      * @return string HTML containing details of the problem.
      */
-    function description() {
+    public function description() {
         list($missingparent, $loops) = $this->find_problems();
 
         $description = '<p>The course categories should be arranged into tree ' .
                 ' structures by the course_categories.parent field. Sometimes ' .
                 ' this tree structure gets messed up.</p>';
 
-        $description .= health_category_list_missing_parents($missingparent);
-        $description .= health_category_list_loops($loops);
+        $description .= tool_health_category_list_missing_parents($missingparent);
+        $description .= tool_health_category_list_loops($loops);
 
         return $description;
     }
diff --git a/admin/tool/health/locallib.php b/admin/tool/health/locallib.php
new file mode 100644 (file)
index 0000000..9eb28f9
--- /dev/null
@@ -0,0 +1,128 @@
+<?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/>.
+
+/**
+ * Functions used by the health tool.
+ *
+ * @package    tool_health
+ * @copyright  2013 Marko Vidberg
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Given a list of categories, this function searches for ones
+ * that have a missing parent category.
+ *
+ * @param array $categories List of categories.
+ * @return array List of categories with missing parents.
+ */
+function tool_health_category_find_missing_parents($categories) {
+    $missingparent = array();
+
+    foreach ($categories as $category) {
+        if ($category->parent != 0 && !array_key_exists($category->parent, $categories)) {
+            $missingparent[$category->id] = $category;
+        }
+    }
+
+    return $missingparent;
+}
+
+/**
+ * Generates a list of categories with missing parents.
+ *
+ * @param array $missingparent List of categories with missing parents.
+ * @return string Bullet point list of categories with missing parents.
+ */
+function tool_health_category_list_missing_parents($missingparent) {
+    $description = '';
+
+    if (!empty($missingparent)) {
+        $description .= '<p>The following categories are missing their parents:</p><ul>';
+        foreach ($missingparent as $cat) {
+            $description .= "<li>Category $cat->id: " . s($cat->name) . "</li>\n";
+        }
+        $description .= "</ul>\n";
+    }
+
+    return $description;
+}
+
+/**
+ * Given a list of categories, this function searches for ones
+ * that have loops to previous parent categories.
+ *
+ * @param array $categories List of categories.
+ * @return array List of categories with loops.
+ */
+function tool_health_category_find_loops($categories) {
+    $loops = array();
+
+    while (!empty($categories)) {
+
+        $current = array_pop($categories);
+        $thisloop = array($current->id => $current);
+
+        while (true) {
+            if (isset($thisloop[$current->parent])) {
+                // Loop detected.
+                $loops = $loops + $thisloop;
+                break;
+            } else if ($current->parent === 0) {
+                // Top level.
+                break;
+            } else if (isset($loops[$current->parent])) {
+                // If the parent is in a loop we should also update this category.
+                $loops = $loops + $thisloop;
+                break;
+            } else if (!isset($categories[$current->parent])) {
+                // We already checked this category and is correct.
+                break;
+            } else {
+                // Continue following the path.
+                $current = $categories[$current->parent];
+                $thisloop[$current->id] = $current;
+                unset($categories[$current->id]);
+            }
+        }
+    }
+
+    return $loops;
+}
+
+/**
+ * Generates a list of categories with loops.
+ *
+ * @param array $loops List of categories with loops.
+ * @return string Bullet point list of categories with loops.
+ */
+function tool_health_category_list_loops($loops) {
+    $description = '';
+
+    if (!empty($loops)) {
+        $description .= '<p>The following categories form a loop of parents:</p><ul>';
+        foreach ($loops as $loop) {
+            $description .= "<li>\n";
+            $description .= "Category $loop->id: " . s($loop->name) . " has parent $loop->parent\n";
+            $description .= "</li>\n";
+        }
+        $description .= "</ul>\n";
+    }
+
+    return $description;
+}
similarity index 56%
rename from lib/tests/adminlib_test.php
rename to admin/tool/health/tests/healthlib_test.php
index e5f514e..5142a12 100644 (file)
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
 /**
- * Unit tests for ../adminlib.php
+ * Unit tests for tool_health.
  *
- * @package    core
- * @category   phpunit
- * @copyright  2012 Petr Skoda {@link http://skodak.org}
+ * @package    tool_health
+ * @copyright  2013 Marko Vidberg
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 
 defined('MOODLE_INTERNAL') || die();
 
 global $CFG;
-require_once($CFG->libdir.'/adminlib.php');
+require_once($CFG->dirroot . '/' . $CFG->admin . '/tool/health/locallib.php');
 
-class healthindex_testcase extends advanced_testcase {
+/**
+ * Health lib testcase.
+ *
+ * @package    tool_health
+ * @copyright  2013 Marko Vidberg
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class healthlib_testcase extends advanced_testcase {
 
     /**
-     * Data provider for test_health_category_find_loops
+     * Data provider for test_tool_health_category_find_loops.
      */
     public static function provider_loop_categories() {
         return array(
-           // One item loop including root
+           // One item loop including root.
            0 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 1)
                         ),
-                        array(
-                            '1' => array(
-                                '1' => (object) array('id' => 1, 'parent' => 1)
-                            ),
-                        ),
-                ),
-           // One item loop including root
-           0 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 1)
                         ),
-                        array(
-                            '1' => array(
-                                '1' => (object) array('id' => 1, 'parent' => 1)
-                            ),
-                        ),
                 ),
-           // One item loop not including root
+           // One item loop not including root.
            1 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 0),
                             '2' => (object) array('id' => 2, 'parent' => 2)
                         ),
                         array(
-                            '2' => array(
-                                '2' => (object) array('id' => 2, 'parent' => 2)
-                            ),
+                            '2' => (object) array('id' => 2, 'parent' => 2)
                         ),
                 ),
-           // Two item loop including root
+           // Two item loop including root.
            2 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 2),
                             '2' => (object) array('id' => 2, 'parent' => 1)
                         ),
                         array(
-                            '1' => array(
-                                 '2' => (object) array('id' => 2, 'parent' => 1),
-                                 '1' => (object) array('id' => 1, 'parent' => 2),
-                            ),
+                            '2' => (object) array('id' => 2, 'parent' => 1),
+                            '1' => (object) array('id' => 1, 'parent' => 2),
                         )
                 ),
-           // Two item loop not including root
+           // Two item loop not including root.
            3 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 0),
@@ -90,13 +79,11 @@ class healthindex_testcase extends advanced_testcase {
                             '3' => (object) array('id' => 3, 'parent' => 2),
                         ),
                         array(
-                            '2' => array(
-                                '3' => (object) array('id' => 3, 'parent' => 2),
-                                '2' => (object) array('id' => 2, 'parent' => 3),
-                            ),
+                            '3' => (object) array('id' => 3, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 3),
                         )
                 ),
-           // Three item loop including root
+           // Three item loop including root.
            4 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 2),
@@ -104,14 +91,12 @@ class healthindex_testcase extends advanced_testcase {
                             '3' => (object) array('id' => 3, 'parent' => 1),
                         ),
                         array(
-                            '2' => array(
-                                '3' => (object) array('id' => 3, 'parent' => 1),
-                                '1' => (object) array('id' => 1, 'parent' => 2),
-                                '2' => (object) array('id' => 2, 'parent' => 3),
-                            ),
+                            '3' => (object) array('id' => 3, 'parent' => 1),
+                            '1' => (object) array('id' => 1, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 3),
                         )
                 ),
-           // Three item loop not including root
+           // Three item loop not including root.
            5 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 0),
@@ -120,14 +105,12 @@ class healthindex_testcase extends advanced_testcase {
                             '4' => (object) array('id' => 4, 'parent' => 2)
                         ),
                         array(
-                            '3' => array(
-                                '4' => (object) array('id' => 4, 'parent' => 2),
-                                '2' => (object) array('id' => 2, 'parent' => 3),
-                                '3' => (object) array('id' => 3, 'parent' => 4),
-                            ),
+                            '4' => (object) array('id' => 4, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 3),
+                            '3' => (object) array('id' => 3, 'parent' => 4),
                         )
                 ),
-           // Multi-loop
+           // Multi-loop.
            6 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 2),
@@ -140,41 +123,40 @@ class healthindex_testcase extends advanced_testcase {
                             '8' => (object) array('id' => 8, 'parent' => 7),
                         ),
                         array(
-                            '2' => array(
-                                '1' => (object) array('id' => 1, 'parent' => 2),
-                                '2' => (object) array('id' => 2, 'parent' => 1),
-                                '8' => (object) array('id' => 8, 'parent' => 7),
-                                '7' => (object) array('id' => 7, 'parent' => 1),
-                            ),
-                            '6' => array(
-                                '6' => (object) array('id' => 6, 'parent' => 6),
-                            ),
-                            '4' => array(
-                                '5' => (object) array('id' => 5, 'parent' => 3),
-                                '3' => (object) array('id' => 3, 'parent' => 4),
-                                '4' => (object) array('id' => 4, 'parent' => 5),
-                            ),
+                            '1' => (object) array('id' => 1, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 1),
+                            '8' => (object) array('id' => 8, 'parent' => 7),
+                            '7' => (object) array('id' => 7, 'parent' => 1),
+                            '6' => (object) array('id' => 6, 'parent' => 6),
+                            '5' => (object) array('id' => 5, 'parent' => 3),
+                            '3' => (object) array('id' => 3, 'parent' => 4),
+                            '4' => (object) array('id' => 4, 'parent' => 5),
+                        )
+                ),
+           // Double-loop
+           7 => array(
+                        array(
+                            '1' => (object) array('id' => 1, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 1),
+                            '3' => (object) array('id' => 3, 'parent' => 2),
+                            '4' => (object) array('id' => 4, 'parent' => 2),
+                        ),
+                        array(
+                            '4' => (object) array('id' => 4, 'parent' => 2),
+                            '3' => (object) array('id' => 3, 'parent' => 2),
+                            '2' => (object) array('id' => 2, 'parent' => 1),
+                            '1' => (object) array('id' => 1, 'parent' => 2),
                         )
                 )
         );
     }
 
     /**
-     * Test finding loops between two items referring to each other.
-     *
-     * @dataProvider provider_loop_categories
-     */
-    public function test_health_category_find_loops($categories, $expected) {
-        $loops = health_category_find_loops($categories);
-        $this->assertEquals($expected, $loops);
-    }
-
-    /**
-     * Data provider for test_health_category_find_missing_parents
+     * Data provider for test_tool_health_category_find_missing_parents.
      */
     public static function provider_missing_parent_categories() {
         return array(
-           // Test for two items, both with direct ancestor (parent) missing
+           // Test for two items, both with direct ancestor (parent) missing.
            0 => array(
                         array(
                             '1' => (object) array('id' => 1, 'parent' => 0),
@@ -191,31 +173,46 @@ class healthindex_testcase extends advanced_testcase {
     }
 
     /**
-     * Test finding missing parent categories
+     * Test finding loops between two items referring to each other.
+     *
+     * @param array $categories
+     * @param array $expected
+     * @dataProvider provider_loop_categories
+     */
+    public function test_tool_health_category_find_loops($categories, $expected) {
+        $loops = tool_health_category_find_loops($categories);
+        $this->assertEquals($expected, $loops);
+    }
+
+    /**
+     * Test finding missing parent categories.
+     *
+     * @param array $categories
+     * @param array $expected
      * @dataProvider provider_missing_parent_categories
      */
-    public function test_health_category_find_missing_parents($categories, $expected) {
-        $missingparent = health_category_find_missing_parents($categories);
+    public function test_tool_health_category_find_missing_parents($categories, $expected) {
+        $missingparent = tool_health_category_find_missing_parents($categories);
         $this->assertEquals($expected, $missingparent);
     }
 
     /**
-     * Test listing missing parent categories
+     * Test listing missing parent categories.
      */
-    public function test_health_category_list_missing_parents() {
-        $missingparent = array('1' => (object) array('id' => 2, 'parent' => 3, 'name' => 'test'),
-                               '2' => (object) array('id' => 4, 'parent' => 5, 'name' => 'test2'));
-        $result = health_category_list_missing_parents($missingparent);
+    public function test_tool_health_category_list_missing_parents() {
+        $missingparent = array((object) array('id' => 2, 'parent' => 3, 'name' => 'test'),
+                               (object) array('id' => 4, 'parent' => 5, 'name' => 'test2'));
+        $result = tool_health_category_list_missing_parents($missingparent);
         $this->assertRegExp('/Category 2: test/', $result);
         $this->assertRegExp('/Category 4: test2/', $result);
     }
 
     /**
-     * Test listing loop categories
+     * Test listing loop categories.
      */
-    public function test_health_category_list_loops() {
-        $loops = array('1' => array('1' => (object) array('id' => 2, 'parent' => 3, 'name' => 'test')));
-        $result = health_category_list_loops($loops);
+    public function test_tool_health_category_list_loops() {
+        $loops = array((object) array('id' => 2, 'parent' => 3, 'name' => 'test'));
+        $result = tool_health_category_list_loops($loops);
         $this->assertRegExp('/Category 2: test/', $result);
     }
 }
index 28bce28..0c6aafe 100644 (file)
@@ -8920,100 +8920,3 @@ class admin_setting_php_extension_enabled extends admin_setting {
         return $o;
     }
 }
-
-/**
- * Given a list of categories, this function searches for ones
- * that have a missing parent category.
- *
- * @param array $categories List of categories.
- * @return array List of categories with missing parents.
- */
-function health_category_find_missing_parents($categories) {
-    $missingparent = array();
-
-    foreach ($categories as $category) {
-        if ($category->parent != 0 && !array_key_exists($category->parent, $categories)) {
-            $missingparent[$category->id] = $category;
-        }
-    }
-
-    return $missingparent;
-}
-
-/**
- * Generates a list of categories with missing parents.
- *
- * @param array $missingparent List of categories with missing parents.
- * @return string Bullet point list of categories with missing parents.
- */
-function health_category_list_missing_parents($missingparent) {
-    $description = '';
-
-    if (!empty($missingparent)) {
-        $description .= '<p>The following categories are missing their parents:</p><ul>';
-        foreach ($missingparent as $cat) {
-            $description .= "<li>Category $cat->id: " . s($cat->name) . "</li>\n";
-        }
-        $description .= "</ul>\n";
-    }
-
-    return $description;
-}
-
-/**
- * Given a list of categories, this function searches for ones
- * that have loops to previous parent categories.
- *
- * @param array $categories List of categories.
- * @return array List of categories with loops.
- */
-function health_category_find_loops($categories) {
-    $loops = array();
-
-    // TODO: Improve this code so that if the root node is included in a loop, only children in the actual loop are reported
-    while (!empty($categories)) {
-        $current = array_pop($categories);
-        $thisloop = array($current->id => $current);
-        while (true) {
-            if (isset($thisloop[$current->parent])) {
-                // Loop detected
-                $loops[$current->id] = $thisloop;
-                break;
-            } else if (!isset($categories[$current->parent])) {
-                // Got to the top level, or a category we already know is OK.
-                break;
-            } else {
-                // Continue following the path.
-                $current = $categories[$current->parent];
-                $thisloop[$current->id] = $current;
-                unset($categories[$current->id]);
-            }
-        }
-    }
-
-    return $loops;
-}
-
-/**
- * Generates a list of categories with loops.
- *
- * @param array $loops List of categories with loops.
- * @return string Bullet point list of categories with loops.
- */
-function health_category_list_loops($loops) {
-    $description = '';
-
-    if (!empty($loops)) {
-        $description .= '<p>The following categories form a loop of parents:</p><ul>';
-        foreach ($loops as $loop) {
-            $description .= "<li><ul>\n";
-            foreach ($loop as $cat) {
-                $description .= "<li>Category $cat->id: " . s($cat->name) . " has parent $cat->parent</li>\n";
-            }
-            $description .= "</ul></li>\n";
-        }
-        $description .= "</ul>\n";
-    }
-
-    return $description;
-}