From 72901ac8465fe51fa2e206fe63f9f076e95fa895 Mon Sep 17 00:00:00 2001 From: Martin Gauk Date: Mon, 5 Mar 2018 16:16:41 +0000 Subject: [PATCH] MDL-61519 calendar: do not iterate through all categories Replace calls to \coursecat::get_all() or cache the results. --- .../local/event/data_access/event_vault.php | 6 +- calendar/externallib.php | 101 ++++++++++-------- calendar/lib.php | 47 ++++---- lang/en/cache.php | 1 + lib/coursecatlib.php | 26 +++++ lib/db/caches.php | 11 ++ lib/tests/coursecatlib_test.php | 31 ++++++ 7 files changed, 146 insertions(+), 77 deletions(-) diff --git a/calendar/classes/local/event/data_access/event_vault.php b/calendar/classes/local/event/data_access/event_vault.php index 246cd9f66a0..6c42d3c2e53 100644 --- a/calendar/classes/local/event/data_access/event_vault.php +++ b/calendar/classes/local/event/data_access/event_vault.php @@ -201,10 +201,6 @@ class event_vault implements event_vault_interface { event_interface $afterevent = null, $limitnum = 20 ) { - $categoryids = array_map(function($category) { - return $category->id; - }, \coursecat::get_all()); - $courseids = array_map(function($course) { return $course->id; }, enrol_get_all_users_courses($user->id)); @@ -227,7 +223,7 @@ class event_vault implements event_vault_interface { [$user->id], $groupids ? $groupids : null, $courseids ? $courseids : null, - $categoryids ? $categoryids : null, + null, // All categories. true, true, function ($event) { diff --git a/calendar/externallib.php b/calendar/externallib.php index 049f13d7f35..f35c5beb933 100644 --- a/calendar/externallib.php +++ b/calendar/externallib.php @@ -181,11 +181,17 @@ class core_calendar_external extends external_api { $funcparam = array('courses' => array(), 'groups' => array(), 'categories' => array()); $hassystemcap = has_capability('moodle/calendar:manageentries', context_system::instance()); $warnings = array(); + $coursecategories = array(); - // Let us findout courses that we can return events from. + // Let us find out courses and their categories that we can return events from. if (!$hassystemcap) { - $courses = enrol_get_my_courses('id'); - $courses = array_keys($courses); + $courseobjs = enrol_get_my_courses(); + $courses = array_keys($courseobjs); + + $coursecategories = array_flip(array_map(function($course) { + return $course->category; + }, $courseobjs)); + foreach ($params['events']['courseids'] as $id) { try { $context = context_course::instance($id); @@ -203,6 +209,14 @@ class core_calendar_external extends external_api { } else { $courses = $params['events']['courseids']; $funcparam['courses'] = $courses; + + if (!empty($courses)) { + list($wheresql, $sqlparams) = $DB->get_in_or_equal($courses); + $wheresql = "id $wheresql"; + $coursecategories = array_flip(array_map(function($course) { + return $course->category; + }, $DB->get_records_select('course', $wheresql, $sqlparams, '', 'category'))); + } } // Let us findout groups that we can return events from. @@ -223,56 +237,55 @@ class core_calendar_external extends external_api { $categories = array(); if ($hassystemcap || !empty($courses)) { + // Use the category id as the key in the following array. That way we do not have to remove duplicates and + // have a faster lookup later. + $categories = []; - $coursecategories = array(); - if (!empty($courses)) { - list($wheresql, $sqlparams) = $DB->get_in_or_equal($courses); - $wheresql = "id $wheresql"; - $courseswithcategory = $DB->get_records_select('course', $wheresql, $sqlparams); - - // Grab the list of course categories for the requested course list. - foreach ($courseswithcategory as $course) { - if (empty($course->visible)) { - if (!has_capability('moodle/course:viewhidden', context_course::instance($course->id))) { - continue; + if (!empty($params['events']['categoryids'])) { + $catobjs = \coursecat::get_many(array_merge($params['events']['categoryids'], array_keys($coursecategories))); + foreach ($catobjs as $catobj) { + if (isset($coursecategories[$catobj->id]) || + has_capability('moodle/category:manage', $catobj->get_context())) { + // If the user has access to a course in this category or can manage the category, + // then they can see all parent categories too. + $categories[$catobj->id] = true; + foreach ($catobj->get_parents() as $catid) { + $categories[$catid] = true; } } - $category = \coursecat::get($course->category); - // Fetch parent categories. - $coursecategories = array_merge($coursecategories, [$category->id], $category->get_parents()); } - } - - foreach (\coursecat::get_all() as $category) { - // Skip categories not requested. - if (!empty($params['events']['categoryids'])) { - if (!in_array($category->id, $params['events']['categoryids'])) { - continue; - } - } - - if (has_capability('moodle/category:manage', $category->get_context())) { - // If a user can manage a category, then they can see all child categories. as well as all parent categories. - $categories[] = $category->id; - - foreach (\coursecat::get_all() as $cat) { - if (array_search($category->id, $cat->get_parents()) !== false) { - $categories[] = $cat->id; + $funcparam['categories'] = array_keys($categories); + } else { + // Fetch all categories where this user has any enrolment, and all categories that this user can manage. + $calcatcache = cache::make('core', 'calendar_categories'); + // Do not use cache if the user has the system capability as $coursecategories might not represent the + // courses the user is enrolled in. + $categories = (!$hassystemcap) ? $calcatcache->get('site') : false; + if ($categories !== false) { + // The ids are stored in a list in the cache. + $funcparam['categories'] = $categories; + $categories = array_flip($categories); + } else { + $categories = []; + foreach (\coursecat::get_all() as $category) { + if (isset($coursecategories[$category->id]) || + has_capability('moodle/category:manage', $category->get_context(), $USER, false)) { + // If the user has access to a course in this category or can manage the category, + // then they can see all parent categories too. + $categories[$category->id] = true; + foreach ($category->get_parents() as $catid) { + $categories[$catid] = true; + } } } - $categories = array_merge($categories, $category->get_parents()); - } else if (in_array($category->id, $coursecategories)) { - - // The user has access to a course in this category. - // Fetch all of the parents too. - $categories = array_merge($categories, [$category->id], $category->get_parents()); - $categories[] = $category->id; + $funcparam['categories'] = array_keys($categories); + if (!$hassystemcap) { + $calcatcache->set('site', $funcparam['categories']); + } } } } - $funcparam['categories'] = array_unique($categories); - // Do we need user events? if (!empty($params['options']['userevents'])) { $funcparam['users'] = array($USER->id); @@ -327,7 +340,7 @@ class core_calendar_external extends external_api { // Can the user actually see this event? $eventobj = calendar_event::load($eventobj); if ((($eventobj->courseid == $SITE->id) && (empty($eventobj->categoryid))) || - (!empty($eventobj->categoryid) && in_array($eventobj->categoryid, $categories)) || + (!empty($eventobj->categoryid) && isset($categories[$eventobj->categoryid])) || (!empty($eventobj->groupid) && in_array($eventobj->groupid, $groups)) || (!empty($eventobj->courseid) && in_array($eventobj->courseid, $courses)) || ($USER->id == $eventobj->userid) || diff --git a/calendar/lib.php b/calendar/lib.php index 722800541a1..96c25a0e6d8 100644 --- a/calendar/lib.php +++ b/calendar/lib.php @@ -1118,7 +1118,7 @@ class calendar_information { * course will be used. * * @param stdClass $course The current course being viewed. - * @param int[] $courses The list of all courses currently accessible. + * @param stdClass[] $courses The list of all courses currently accessible. * @param stdClass $category The current category to show. */ public function set_sources(stdClass $course, array $courses, stdClass $category = null) { @@ -1157,17 +1157,9 @@ class calendar_information { // Build the category list. // This includes the current category. - $this->categories = [$category->id]; - - // All of its descendants. - foreach (\coursecat::get_all() as $cat) { - if (array_search($category->id, $cat->get_parents()) !== false) { - $this->categories[] = $cat->id; - } - } - - // And all of its parents. - $this->categories = array_merge($this->categories, $category->get_parents()); + $this->categories = $category->get_parents(); + $this->categories[] = $category->id; + $this->categories = array_merge($this->categories, $category->get_all_children_ids()); } else if (SITEID === $this->courseid) { // The site was requested. // Fetch all categories where this user has any enrolment, and all categories that this user can manage. @@ -1177,26 +1169,25 @@ class calendar_information { return $course->category; }, $courses)); - $categories = []; - foreach (\coursecat::get_all() as $category) { - if (has_capability('moodle/category:manage', $category->get_context(), $USER, false)) { - // If a user can manage a category, then they can see all child categories. as well as all parent categories. - $categories[] = $category->id; - foreach (\coursecat::get_all() as $cat) { - if (array_search($category->id, $cat->get_parents()) !== false) { - $categories[] = $cat->id; + $calcatcache = cache::make('core', 'calendar_categories'); + $this->categories = $calcatcache->get('site'); + if ($this->categories === false) { + // Use the category id as the key in the following array. That way we do not have to remove duplicates. + $categories = []; + foreach (\coursecat::get_all() as $category) { + if (isset($coursecategories[$category->id]) || + has_capability('moodle/category:manage', $category->get_context(), $USER, false)) { + // If the user has access to a course in this category or can manage the category, + // then they can see all parent categories too. + $categories[$category->id] = true; + foreach ($category->get_parents() as $catid) { + $categories[$catid] = true; } } - $categories = array_merge($categories, $category->get_parents()); - } else if (isset($coursecategories[$category->id])) { - // The user has access to a course in this category. - // Fetch all of the parents too. - $categories = array_merge($categories, [$category->id], $category->get_parents()); - $categories[] = $category->id; } + $this->categories = array_keys($categories); + $calcatcache->set('site', $this->categories); } - - $this->categories = array_unique($categories); } } diff --git a/lang/en/cache.php b/lang/en/cache.php index 44de1dc7552..d35ddcb6e41 100644 --- a/lang/en/cache.php +++ b/lang/en/cache.php @@ -37,6 +37,7 @@ $string['caching'] = 'Caching'; $string['cacheadmin'] = 'Cache administration'; $string['cacheconfig'] = 'Configuration'; $string['cachedef_calendar_subscriptions'] = 'Calendar subscriptions'; +$string['cachedef_calendar_categories'] = 'Calendar course categories that a user can access'; $string['cachedef_capabilities'] = 'System capabilities list'; $string['cachedef_config'] = 'Config settings'; $string['cachedef_coursecat'] = 'Course categories lists for particular user'; diff --git a/lib/coursecatlib.php b/lib/coursecatlib.php index 02123a1b702..b5fcff4306b 100644 --- a/lib/coursecatlib.php +++ b/lib/coursecatlib.php @@ -1171,6 +1171,32 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate { return $rv; } + /** + * Returns an array of ids of categories that are (direct and indirect) children + * of this category. + * + * @return int[] + */ + public function get_all_children_ids() { + $coursecattreecache = cache::make('core', 'coursecattree'); + $children = $coursecattreecache->get($this->id . 'allchildren'); + if ($children === false) { + $children = []; + $walk = [$this->id]; + while (count($walk) > 0) { + $catid = array_pop($walk); + $directchildren = self::get_tree($catid); + if ($directchildren !== false && count($directchildren) > 0) { + $walk = array_merge($walk, $directchildren); + $children = array_merge($children, $directchildren); + } + } + $coursecattreecache->set($this->id . 'allchildren', $children); + } + + return $children; + } + /** * Returns true if the user has the manage capability on any category. * diff --git a/lib/db/caches.php b/lib/db/caches.php index fb43616ec14..73da817cd96 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -127,6 +127,17 @@ $definitions = array( 'staticacceleration' => true, ), + // Cache the course categories where the user has any enrolment and all categories that this user can manage. + 'calendar_categories' => array( + 'mode' => cache_store::MODE_SESSION, + 'simplekeys' => true, + 'simpledata' => true, + 'invalidationevents' => array( + 'changesincoursecat', + ), + 'ttl' => 900, + ), + // Cache the capabilities list DB table. See get_all_capabilities in accesslib. 'capabilities' => array( 'mode' => cache_store::MODE_APPLICATION, diff --git a/lib/tests/coursecatlib_test.php b/lib/tests/coursecatlib_test.php index 1dea0e66d63..54b10995477 100644 --- a/lib/tests/coursecatlib_test.php +++ b/lib/tests/coursecatlib_test.php @@ -381,6 +381,37 @@ class core_coursecatlib_testcase extends advanced_testcase { $this->assertEquals(4, $category1->get_children_count()); } + /** + * Test the get_all_children_ids function. + */ + public function test_get_all_children_ids() { + $category1 = coursecat::create(array('name' => 'Cat1')); + $category2 = coursecat::create(array('name' => 'Cat2')); + $category11 = coursecat::create(array('name' => 'Cat11', 'parent' => $category1->id)); + $category12 = coursecat::create(array('name' => 'Cat12', 'parent' => $category1->id)); + $category13 = coursecat::create(array('name' => 'Cat13', 'parent' => $category1->id)); + $category111 = coursecat::create(array('name' => 'Cat111', 'parent' => $category11->id)); + $category112 = coursecat::create(array('name' => 'Cat112', 'parent' => $category11->id)); + $category1121 = coursecat::create(array('name' => 'Cat1121', 'parent' => $category112->id)); + + $this->assertCount(0, $category2->get_all_children_ids()); + $this->assertCount(6, $category1->get_all_children_ids()); + + $cmpchildrencat1 = array($category11->id, $category12->id, $category13->id, $category111->id, $category112->id, + $category1121->id); + $childrencat1 = $category1->get_all_children_ids(); + // Order of values does not matter. Compare sorted arrays. + sort($cmpchildrencat1); + sort($childrencat1); + $this->assertEquals($cmpchildrencat1, $childrencat1); + + $this->assertCount(3, $category11->get_all_children_ids()); + $this->assertCount(0, $category111->get_all_children_ids()); + $this->assertCount(1, $category112->get_all_children_ids()); + + $this->assertEquals(array($category1121->id), $category112->get_all_children_ids()); + } + /** * Test the countall function */