From 10db3a0b0a8ff84b9a018e12011fb59fe08a5392 Mon Sep 17 00:00:00 2001 From: Nathan Nguyen Date: Mon, 31 Aug 2020 14:59:59 +1000 Subject: [PATCH] MDL-69573 core_course: Make MAX_COURSES_IN_CATEGORY configurable --- config-dist.php | 14 ++++++ course/classes/category.php | 3 +- course/lib.php | 2 +- lib/datalib.php | 35 ++++++++++---- lib/db/install.php | 2 +- lib/tests/datalib_test.php | 93 +++++++++++++++++++++++++++++++++++++ 6 files changed, 138 insertions(+), 11 deletions(-) diff --git a/config-dist.php b/config-dist.php index ecd56b6b6fa..5d45c866ae4 100644 --- a/config-dist.php +++ b/config-dist.php @@ -709,6 +709,20 @@ $CFG->admin = 'admin'; // // $CFG->forumpostcountchunksize = 5000; // +// Course and category sorting +// +// If the number of courses in a category exceeds $CFG->maxcoursesincategory (10000 by default), it may lead to duplicate +// sort orders of courses in separated categories. For example: +// - Category A has the sort order of 10000, and has 10000 courses. The last course will have the sort order of 20000. +// - Category B has the sort order of 20000, and has a course with the sort order of 20001. +// - If we add another course in category A, it will have a sort order of 20001, +// which is the same as the course in category B +// The duplicate will cause sorting issue and hence we need to increase $CFG->maxcoursesincategory +// to fix the duplicate sort order +// Please also make sure $CFG->maxcoursesincategory * MAX_COURSE_CATEGORIES less than max integer. +// +// $CFG->maxcoursesincategory = 10000; +// //========================================================================= // 7. SETTINGS FOR DEVELOPMENT SERVERS - not intended for production use!!! //========================================================================= diff --git a/course/classes/category.php b/course/classes/category.php index abe5d565733..187f85123ed 100644 --- a/course/classes/category.php +++ b/course/classes/category.php @@ -2319,7 +2319,8 @@ class core_course_category implements renderable, cacheable_object, IteratorAggr $context->update_moved($newparent); // Now make it last in new category. - $DB->set_field('course_categories', 'sortorder', MAX_COURSES_IN_CATEGORY * MAX_COURSE_CATEGORIES, ['id' => $this->id]); + $DB->set_field('course_categories', 'sortorder', + get_max_courses_in_category() * MAX_COURSE_CATEGORIES, ['id' => $this->id]); if ($hidecat) { fix_course_sortorder(); diff --git a/course/lib.php b/course/lib.php index f8bd18954d8..a834ba73222 100644 --- a/course/lib.php +++ b/course/lib.php @@ -2103,7 +2103,7 @@ function move_courses($courseids, $categoryid) { $course->id = $dbcourse->id; $course->timemodified = time(); $course->category = $category->id; - $course->sortorder = $category->sortorder + MAX_COURSES_IN_CATEGORY - $i++; + $course->sortorder = $category->sortorder + get_max_courses_in_category() - $i++; if ($category->visible == 0) { // Hide the course when moving into hidden category, do not update the visibleold flag - we want to get // to previous state if somebody unhides the category. diff --git a/lib/datalib.php b/lib/datalib.php index 2ca8e54d80c..613f82f68e7 100644 --- a/lib/datalib.php +++ b/lib/datalib.php @@ -784,7 +784,6 @@ function get_courses_search($searchterms, $sort, $page, $recordsperpage, &$total * * @global object * @global object - * @uses MAX_COURSES_IN_CATEGORY * @uses MAX_COURSE_CATEGORIES * @uses SITEID * @uses CONTEXT_COURSE @@ -801,7 +800,8 @@ function fix_course_sortorder() { if ($unsorted = $DB->get_records('course_categories', array('sortorder'=>0))) { //move all categories that are not sorted yet to the end - $DB->set_field('course_categories', 'sortorder', MAX_COURSES_IN_CATEGORY*MAX_COURSE_CATEGORIES, array('sortorder'=>0)); + $DB->set_field('course_categories', 'sortorder', + get_max_courses_in_category() * MAX_COURSE_CATEGORIES, array('sortorder' => 0)); $cacheevents['changesincoursecat'] = true; } @@ -901,7 +901,7 @@ function fix_course_sortorder() { $categories = array(); foreach ($updatecounts as $cat) { $cat->coursecount = $cat->newcount; - if ($cat->coursecount >= MAX_COURSES_IN_CATEGORY) { + if ($cat->coursecount >= get_max_courses_in_category()) { $categories[] = $cat->id; } unset($cat->newcount); @@ -909,7 +909,11 @@ function fix_course_sortorder() { } if (!empty($categories)) { $str = implode(', ', $categories); - debugging("The number of courses (category id: $str) has reached MAX_COURSES_IN_CATEGORY (" . MAX_COURSES_IN_CATEGORY . "), it will cause a sorting performance issue, please increase the value of MAX_COURSES_IN_CATEGORY in lib/datalib.php file. See tracker issue: MDL-25669", DEBUG_DEVELOPER); + debugging("The number of courses (category id: $str) has reached max number of courses " . + "in a category (" . get_max_courses_in_category() . "). It will cause a sorting performance issue. " . + "Please set higher value for \$CFG->maxcoursesincategory in config.php. " . + "Please also make sure \$CFG->maxcoursesincategory * MAX_COURSE_CATEGORIES less than max integer. " . + "See tracker issues: MDL-25669 and MDL-69573", DEBUG_DEVELOPER); } $cacheevents['changesincoursecat'] = true; } @@ -918,13 +922,13 @@ function fix_course_sortorder() { $sql = "SELECT DISTINCT cc.id, cc.sortorder FROM {course_categories} cc JOIN {course} c ON c.category = cc.id - WHERE c.sortorder < cc.sortorder OR c.sortorder > cc.sortorder + ".MAX_COURSES_IN_CATEGORY; + WHERE c.sortorder < cc.sortorder OR c.sortorder > cc.sortorder + " . get_max_courses_in_category(); if ($fixcategories = $DB->get_records_sql($sql)) { //fix the course sortorder ranges foreach ($fixcategories as $cat) { $sql = "UPDATE {course} - SET sortorder = ".$DB->sql_modulo('sortorder', MAX_COURSES_IN_CATEGORY)." + ? + SET sortorder = ".$DB->sql_modulo('sortorder', get_max_courses_in_category())." + ? WHERE category = ?"; $DB->execute($sql, array($cat->sortorder, $cat->id)); } @@ -992,7 +996,6 @@ function fix_course_sortorder() { * @todo Document the arguments of this function better * * @global object - * @uses MAX_COURSES_IN_CATEGORY * @uses CONTEXT_COURSECAT * @param array $children * @param int $sortorder @@ -1009,7 +1012,7 @@ function _fix_course_cats($children, &$sortorder, $parent, $depth, $path, &$fixc $changesmade = false; foreach ($children as $cat) { - $sortorder = $sortorder + MAX_COURSES_IN_CATEGORY; + $sortorder = $sortorder + get_max_courses_in_category(); $update = false; if ($parent != $cat->parent or $depth != $cat->depth or $path.'/'.$cat->id != $cat->path) { $cat->parent = $parent; @@ -1804,3 +1807,19 @@ function decompose_update_into_safe_changes(array $newvalues, $unusedvalue) { return $safechanges; } + +/** + * Return maximum number of courses in a category + * + * @uses MAX_COURSES_IN_CATEGORY + * @return int number of courses + */ +function get_max_courses_in_category() { + global $CFG; + // Use default MAX_COURSES_IN_CATEGORY if $CFG->maxcoursesincategory is not set or invalid. + if (!isset($CFG->maxcoursesincategory) || clean_param($CFG->maxcoursesincategory, PARAM_INT) == 0) { + return MAX_COURSES_IN_CATEGORY; + } else { + return $CFG->maxcoursesincategory; + } +} diff --git a/lib/db/install.php b/lib/db/install.php index 0722a2aa92c..5d4d5565f0e 100644 --- a/lib/db/install.php +++ b/lib/db/install.php @@ -107,7 +107,7 @@ function xmldb_main_install() { $cat = new stdClass(); $cat->name = get_string('miscellaneous'); $cat->depth = 1; - $cat->sortorder = MAX_COURSES_IN_CATEGORY; + $cat->sortorder = get_max_courses_in_category(); $cat->timemodified = time(); $catid = $DB->insert_record('course_categories', $cat); $DB->set_field('course_categories', 'path', '/'.$catid, array('id'=>$catid)); diff --git a/lib/tests/datalib_test.php b/lib/tests/datalib_test.php index b334a3f61c7..76250b27649 100644 --- a/lib/tests/datalib_test.php +++ b/lib/tests/datalib_test.php @@ -653,4 +653,97 @@ class core_datalib_testcase extends advanced_testcase { $this->assertInstanceOf('coding_exception', $e); } } + + /** + * Test max courses in category + */ + public function test_max_courses_in_category() { + global $CFG; + $this->resetAfterTest(); + + // Default settings. + $this->assertEquals(MAX_COURSES_IN_CATEGORY, get_max_courses_in_category()); + + // Misc category. + $misc = core_course_category::get_default(); + $this->assertEquals(MAX_COURSES_IN_CATEGORY, $misc->sortorder); + + $category1 = $this->getDataGenerator()->create_category(); + $category2 = $this->getDataGenerator()->create_category(); + + // Check category sort orders. + $this->assertEquals(MAX_COURSES_IN_CATEGORY, core_course_category::get($misc->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2, core_course_category::get($category1->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3, core_course_category::get($category2->id)->sortorder); + + // Create courses. + $course1 = $this->getDataGenerator()->create_course(['category' => $category1->id]); + $course2 = $this->getDataGenerator()->create_course(['category' => $category2->id]); + $course3 = $this->getDataGenerator()->create_course(['category' => $category1->id]); + $course4 = $this->getDataGenerator()->create_course(['category' => $category2->id]); + + // Check course sort orders. + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2 + 2, get_course($course1->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3 + 2, get_course($course2->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2 + 1, get_course($course3->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3 + 1, get_course($course4->id)->sortorder); + + // Increase max course in category. + $CFG->maxcoursesincategory = 20000; + $this->assertEquals(20000, get_max_courses_in_category()); + + // The sort order has not yet fixed, these sort orders should be the same as before. + // Categories. + $this->assertEquals(MAX_COURSES_IN_CATEGORY, core_course_category::get($misc->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2, core_course_category::get($category1->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3, core_course_category::get($category2->id)->sortorder); + // Courses in category 1. + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2 + 2, get_course($course1->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 2 + 1, get_course($course3->id)->sortorder); + // Courses in category 2. + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3 + 2, get_course($course2->id)->sortorder); + $this->assertEquals(MAX_COURSES_IN_CATEGORY * 3 + 1, get_course($course4->id)->sortorder); + + // Create new category so that the sort orders are applied. + $category3 = $this->getDataGenerator()->create_category(); + // Categories. + $this->assertEquals(20000, core_course_category::get($misc->id)->sortorder); + $this->assertEquals(20000 * 2, core_course_category::get($category1->id)->sortorder); + $this->assertEquals(20000 * 3, core_course_category::get($category2->id)->sortorder); + $this->assertEquals(20000 * 4, core_course_category::get($category3->id)->sortorder); + // Courses in category 1. + $this->assertEquals(20000 * 2 + 2, get_course($course1->id)->sortorder); + $this->assertEquals(20000 * 2 + 1, get_course($course3->id)->sortorder); + // Courses in category 2. + $this->assertEquals(20000 * 3 + 2, get_course($course2->id)->sortorder); + $this->assertEquals(20000 * 3 + 1, get_course($course4->id)->sortorder); + } + + /** + * Test debug message for max courses in category + */ + public function test_debug_max_courses_in_category() { + global $CFG; + $this->resetAfterTest(); + + // Set to small value so that we can check the debug message. + $CFG->maxcoursesincategory = 3; + $this->assertEquals(3, get_max_courses_in_category()); + + $category1 = $this->getDataGenerator()->create_category(); + + // There is only one course, no debug message. + $this->getDataGenerator()->create_course(['category' => $category1->id]); + $this->assertDebuggingNotCalled(); + // There are two courses, no debug message. + $this->getDataGenerator()->create_course(['category' => $category1->id]); + $this->assertDebuggingNotCalled(); + // There is debug message when number of courses reaches the maximum number. + $this->getDataGenerator()->create_course(['category' => $category1->id]); + $this->assertDebuggingCalled("The number of courses (category id: $category1->id) has reached max number of courses " . + "in a category (" . get_max_courses_in_category() . "). It will cause a sorting performance issue. " . + "Please set higher value for \$CFG->maxcoursesincategory in config.php. " . + "Please also make sure \$CFG->maxcoursesincategory * MAX_COURSE_CATEGORIES less than max integer. " . + "See tracker issues: MDL-25669 and MDL-69573"); + } }