Skip to content

Commit

Permalink
MDL-61519 calendar: do not iterate through all categories
Browse files Browse the repository at this point in the history 
Replace calls to \coursecat::get_all() or cache the results.
  • Loading branch information
@MartinGauk
MartinGauk committed Mar 16, 2018
1 parent b63a3b0 commit c417207
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 78 deletions.
6 changes: 1 addition & 5 deletions calendar/classes/local/event/data_access/event_vault.php
View file
Expand Up @@ -201,10 +201,6 @@ public function get_action_events_by_timesort(
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));
Expand All @@ -227,7 +223,7 @@ public function get_action_events_by_timesort(
[$user->id],
$groupids ? $groupids : null,
$courseids ? $courseids : null,
$categoryids ? $categoryids : null,
null, // All categories.
true,
true,
function ($event) {
Expand Down
103 changes: 58 additions & 45 deletions calendar/externallib.php
View file
Expand Up @@ -181,11 +181,17 @@ public static function get_calendar_events($events = array(), $options = array()
$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);
Expand All @@ -203,6 +209,14 @@ public static function get_calendar_events($events = array(), $options = array()
} 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.
Expand All @@ -223,56 +237,55 @@ public static function get_calendar_events($events = array(), $options = array()

$categories = array();
if ($hassystemcap || !empty($courses)) {

$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;
// 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 = [];

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);
Expand Down Expand Up @@ -327,7 +340,7 @@ public static function get_calendar_events($events = array(), $options = array()
// 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) ||
Expand Down
47 changes: 19 additions & 28 deletions calendar/lib.php
View file
Expand Up @@ -1132,7 +1132,7 @@ public function prepare_for_view(stdClass $course, array $coursestoload, $ignore
* 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) {
Expand Down Expand Up @@ -1171,17 +1171,9 @@ public function set_sources(stdClass $course, array $courses, stdClass $category

// 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.
Expand All @@ -1191,26 +1183,25 @@ public function set_sources(stdClass $course, array $courses, stdClass $category
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);
}
}

Expand Down
1 change: 1 addition & 0 deletions lang/en/cache.php
View file
Expand Up @@ -37,6 +37,7 @@
$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';
Expand Down
26 changes: 26 additions & 0 deletions lib/coursecatlib.php
View file
Expand Up @@ -1171,6 +1171,32 @@ public function get_children($options = array()) {
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.
*
Expand Down
11 changes: 11 additions & 0 deletions lib/db/caches.php
View file
Expand Up @@ -127,6 +127,17 @@
'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,
Expand Down
31 changes: 31 additions & 0 deletions lib/tests/coursecatlib_test.php
View file
Expand Up @@ -381,6 +381,37 @@ public function test_get_children() {
$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
*/
Expand Down

0 comments on commit c417207

Please sign in to comment.