diff --git a/admin/tool/health/index.php b/admin/tool/health/index.php index 6a03291e689..37fce0ae125 100644 --- a/admin/tool/health/index.php +++ b/admin/tool/health/index.php @@ -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.

'; - $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 = '

The course categories should be arranged into tree ' . ' structures by the course_categories.parent field. Sometimes ' . ' this tree structure gets messed up.

'; - $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 index 00000000000..9eb28f99d96 --- /dev/null +++ b/admin/tool/health/locallib.php @@ -0,0 +1,128 @@ +. + +/** + * 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 .= '

The following categories are missing their parents:

\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 .= '

The following categories form a loop of parents:

\n"; + } + + return $description; +} diff --git a/lib/tests/adminlib_test.php b/admin/tool/health/tests/healthlib_test.php similarity index 56% rename from lib/tests/adminlib_test.php rename to admin/tool/health/tests/healthlib_test.php index e5f514e0b3e..5142a1210ab 100644 --- a/lib/tests/adminlib_test.php +++ b/admin/tool/health/tests/healthlib_test.php @@ -15,74 +15,63 @@ // along with Moodle. If not, see . /** - * 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); } } diff --git a/lib/adminlib.php b/lib/adminlib.php index 28bce284ebc..0c6aafe9d38 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -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 .= '

The following categories are missing their parents:

\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 .= '

The following categories form a loop of parents:

\n"; - } - - return $description; -}