MDL-34684 tool_health: Course categories checking refinements

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.
This commit is contained in:
David Monllao 2014-11-24 13:57:47 +08:00
parent 8db188c8cc
commit a1ceaf894b
4 changed files with 230 additions and 198 deletions

View 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;
}

View File

@ -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;
}

View File

@ -15,74 +15,63 @@
// 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);
}
}

View 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;
}