From a6d4a36388d5fca3ce47bbef11d841cee4e008d4 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 14 Aug 2015 12:27:26 +0800 Subject: [PATCH 1/2] MDL-41042 coursecat: better handle changes in coursecontact --- lib/adminlib.php | 2 ++ lib/coursecatlib.php | 11 +---------- lib/db/caches.php | 1 + 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/lib/adminlib.php b/lib/adminlib.php index debcc5f1e21..e3a584797be 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -4922,6 +4922,8 @@ class admin_setting_special_coursecontact extends admin_setting_pickroles { parent::__construct('coursecontact', get_string('coursecontact', 'admin'), get_string('coursecontact_desc', 'admin'), array('editingteacher')); + $this->set_updatedcallback(create_function('', + "cache::make('core', 'coursecontacts')->purge();")); } } diff --git a/lib/coursecatlib.php b/lib/coursecatlib.php index ee93258afea..69f3b8ce887 100644 --- a/lib/coursecatlib.php +++ b/lib/coursecatlib.php @@ -708,16 +708,7 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate { } $managerroles = explode(',', $CFG->coursecontact); $cache = cache::make('core', 'coursecontacts'); - $cacheddata = $cache->get_many(array_merge(array('basic'), array_keys($courses))); - // Check if cache was set for the current course contacts and it is not yet expired. - if (empty($cacheddata['basic']) || $cacheddata['basic']['roles'] !== $CFG->coursecontact || - $cacheddata['basic']['lastreset'] < time() - self::CACHE_COURSE_CONTACTS_TTL) { - // Reset cache. - $keys = $DB->get_fieldset_select('course', 'id', ''); - $cache->delete_many($keys); - $cache->set('basic', array('roles' => $CFG->coursecontact, 'lastreset' => time())); - $cacheddata = $cache->get_many(array_merge(array('basic'), array_keys($courses))); - } + $cacheddata = $cache->get_many(array_keys($courses)); $courseids = array(); foreach (array_keys($courses) as $id) { if ($cacheddata[$id] !== false) { diff --git a/lib/db/caches.php b/lib/db/caches.php index 89406dea48b..7d90e55efeb 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -189,6 +189,7 @@ $definitions = array( 'mode' => cache_store::MODE_APPLICATION, 'staticacceleration' => true, 'simplekeys' => true, + 'ttl' => 3600, ), // Used to store data for repositories to avoid repetitive DB queries within one request. 'repositories' => array( From 5667e60249aa9522b57fc1c249476c382ab5111b Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 14 Aug 2015 12:28:09 +0800 Subject: [PATCH 2/2] MDL-41042 coursecat: reset course contacts cache more often --- enrol/manual/locallib.php | 2 + enrol/upgrade.txt | 1 + lib/accesslib.php | 8 +++- lib/coursecatlib.php | 82 +++++++++++++++++++++++++++++++-- lib/enrollib.php | 14 +++++- lib/tests/coursecatlib_test.php | 14 ++++++ 6 files changed, 116 insertions(+), 5 deletions(-) diff --git a/enrol/manual/locallib.php b/enrol/manual/locallib.php index 472897f76f0..89b99df1a0a 100644 --- a/enrol/manual/locallib.php +++ b/enrol/manual/locallib.php @@ -270,6 +270,8 @@ class enrol_manual_editselectedusers_operation extends enrol_bulk_enrolment_oper $event->trigger(); } } + // Delete cached course contacts for this course because they may be affected. + cache::make('core', 'coursecontacts')->delete($manager->get_context()->instanceid); return true; } diff --git a/enrol/upgrade.txt b/enrol/upgrade.txt index d4090255037..1b9cc210a05 100644 --- a/enrol/upgrade.txt +++ b/enrol/upgrade.txt @@ -6,6 +6,7 @@ information provided here is intended especially for developers. * Added new events enrol_instance_created, enrol_instance_updated and enrol_instance_deleted . Always trigger them when changing records in the DB table 'enrol'. +* Constant CACHE_COURSE_CONTACTS_TTL was deleted. === 2.9 === diff --git a/lib/accesslib.php b/lib/accesslib.php index dfd3341d607..447535704a5 100644 --- a/lib/accesslib.php +++ b/lib/accesslib.php @@ -1673,7 +1673,7 @@ function get_roles_with_capability($capability, $permission = null, $context = n * @return int new/existing id of the assignment */ function role_assign($roleid, $userid, $contextid, $component = '', $itemid = 0, $timemodified = '') { - global $USER, $DB; + global $USER, $DB, $CFG; // first of all detect if somebody is using old style parameters if ($contextid === 0 or is_numeric($component)) { @@ -1754,6 +1754,9 @@ function role_assign($roleid, $userid, $contextid, $component = '', $itemid = 0, reload_all_capabilities(); } + require_once($CFG->libdir . '/coursecatlib.php'); + coursecat::role_assignment_changed($roleid, $context); + $event = \core\event\role_assigned::create(array( 'context' => $context, 'objectid' => $ra->roleid, @@ -1811,6 +1814,7 @@ function role_unassign($roleid, $userid, $contextid, $component = '', $itemid = */ function role_unassign_all(array $params, $subcontexts = false, $includemanual = false) { global $USER, $CFG, $DB; + require_once($CFG->libdir . '/coursecatlib.php'); if (!$params) { throw new coding_exception('Missing parameters in role_unsassign_all() call'); @@ -1861,6 +1865,7 @@ function role_unassign_all(array $params, $subcontexts = false, $includemanual = )); $event->add_record_snapshot('role_assignments', $ra); $event->trigger(); + coursecat::role_assignment_changed($ra->roleid, $context); } } unset($ras); @@ -1892,6 +1897,7 @@ function role_unassign_all(array $params, $subcontexts = false, $includemanual = 'other'=>array('id'=>$ra->id, 'component'=>$ra->component, 'itemid'=>$ra->itemid))); $event->add_record_snapshot('role_assignments', $ra); $event->trigger(); + coursecat::role_assignment_changed($ra->roleid, $context); } } } diff --git a/lib/coursecatlib.php b/lib/coursecatlib.php index 69f3b8ce887..df83db66717 100644 --- a/lib/coursecatlib.php +++ b/lib/coursecatlib.php @@ -52,9 +52,6 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate { /** @var coursecat stores pseudo category with id=0. Use coursecat::get(0) to retrieve */ protected static $coursecat0; - /** Do not fetch course contacts more often than once per hour. */ - const CACHE_COURSE_CONTACTS_TTL = 3600; - /** @var array list of all fields and their short name and default value for caching */ protected static $coursecatfields = array( 'id' => array('id', 0), @@ -673,6 +670,85 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate { array('contextcoursecat' => CONTEXT_COURSECAT) + $params); } + /** + * Resets course contact caches when role assignments were changed + * + * @param int $roleid role id that was given or taken away + * @param context $context context where role assignment has been changed + */ + public static function role_assignment_changed($roleid, $context) { + global $CFG, $DB; + + if ($context->contextlevel > CONTEXT_COURSE) { + // No changes to course contacts if role was assigned on the module/block level. + return; + } + + if (!$CFG->coursecontact || !in_array($roleid, explode(',', $CFG->coursecontact))) { + // The role is not one of course contact roles. + return; + } + + // Remove from cache course contacts of all affected courses. + $cache = cache::make('core', 'coursecontacts'); + if ($context->contextlevel == CONTEXT_COURSE) { + $cache->delete($context->instanceid); + } else if ($context->contextlevel == CONTEXT_SYSTEM) { + $cache->purge(); + } else { + $sql = "SELECT ctx.instanceid + FROM {context} ctx + WHERE ctx.path LIKE ? AND ctx.contextlevel = ?"; + $params = array($context->path . '/%', CONTEXT_COURSE); + if ($courses = $DB->get_fieldset_sql($sql, $params)) { + $cache->delete_many($courses); + } + } + } + + /** + * Executed when user enrolment was changed to check if course + * contacts cache needs to be cleared + * + * @param int $courseid course id + * @param int $userid user id + * @param int $status new enrolment status (0 - active, 1 - suspended) + * @param int $timestart new enrolment time start + * @param int $timeend new enrolment time end + */ + public static function user_enrolment_changed($courseid, $userid, + $status, $timestart = null, $timeend = null) { + $cache = cache::make('core', 'coursecontacts'); + $contacts = $cache->get($courseid); + if ($contacts === false) { + // The contacts for the affected course were not cached anyway. + return; + } + $enrolmentactive = ($status == 0) && + (!$timestart || $timestart < time()) && + (!$timeend || $timeend > time()); + if (!$enrolmentactive) { + $isincontacts = false; + foreach ($contacts as $contact) { + if ($contact->id == $userid) { + $isincontacts = true; + } + } + if (!$isincontacts) { + // Changed user's enrolment does not exist or is not active, + // and he is not in cached course contacts, no changes to be made. + return; + } + } + // Either enrolment of manager was deleted/suspended + // or user enrolment was added or activated. + // In order to see if the course contacts for this course need + // changing we would need to make additional queries, they will + // slow down bulk enrolment changes. It is better just to remove + // course contacts cache for this course. + $cache->delete($courseid); + } + /** * Given list of DB records from table course populates each record with list of users with course contact roles * diff --git a/lib/enrollib.php b/lib/enrollib.php index 593cafac215..c69fb391ea7 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -1330,6 +1330,10 @@ abstract class enrol_plugin { ) ); $event->trigger(); + // Check if course contacts cache needs to be cleared. + require_once($CFG->libdir . '/coursecatlib.php'); + coursecat::user_enrolment_changed($courseid, $ue->userid, + $ue->status, $ue->timestart, $ue->timeend); } if ($roleid) { @@ -1370,7 +1374,7 @@ abstract class enrol_plugin { * @return void */ public function update_user_enrol(stdClass $instance, $userid, $status = NULL, $timestart = NULL, $timeend = NULL) { - global $DB, $USER; + global $DB, $USER, $CFG; $name = $this->get_name(); @@ -1420,6 +1424,10 @@ abstract class enrol_plugin { ) ); $event->trigger(); + + require_once($CFG->libdir . '/coursecatlib.php'); + coursecat::user_enrolment_changed($instance->courseid, $ue->userid, + $ue->status, $ue->timestart, $ue->timeend); } /** @@ -1501,6 +1509,10 @@ abstract class enrol_plugin { // reset all enrol caches $context->mark_dirty(); + // Check if courrse contacts cache needs to be cleared. + require_once($CFG->libdir . '/coursecatlib.php'); + coursecat::user_enrolment_changed($courseid, $ue->userid, ENROL_USER_SUSPENDED); + // reset current user enrolment caching if ($userid == $USER->id) { if (isset($USER->enrol['enrolled'][$courseid])) { diff --git a/lib/tests/coursecatlib_test.php b/lib/tests/coursecatlib_test.php index 5c5b1f356ba..d930044d676 100644 --- a/lib/tests/coursecatlib_test.php +++ b/lib/tests/coursecatlib_test.php @@ -561,6 +561,12 @@ class core_coursecatlib_testcase extends advanced_testcase { $manual = enrol_get_plugin('manual'); + // Nobody is enrolled now and course contacts are empty. + $allcourses = coursecat::get(0)->get_courses(array('recursive' => true, 'coursecontacts' => true, 'sort' => array('idnumber' => 1))); + foreach ($allcourses as $onecourse) { + $this->assertEmpty($onecourse->get_course_contacts()); + } + // Cat1 (user2 has teacher role) role_assign($teacherrole->id, $user[2], context_coursecat::instance($category[1])); // course21 (user2 is enrolled as manager) @@ -616,6 +622,14 @@ class core_coursecatlib_testcase extends advanced_testcase { // -- course12 (user1 has teacher role) | $this->assertSame('', $contacts[1][2]); + // Suspend user 4 and make sure he is no longer in contacts of course 1 in category 4. + $manual->enrol_user($enrol[4][1], $user[4], $teacherrole->id, 0, 0, ENROL_USER_SUSPENDED); + $allcourses = coursecat::get(0)->get_courses(array('recursive' => true, 'coursecontacts' => true, 'sort' => array('idnumber' => 1))); + $contacts = $allcourses[$course[4][1]]->get_course_contacts(); + $this->assertCount(1, $contacts); + $contact = reset($contacts); + $this->assertEquals('F5 L5', $contact['username']); + $CFG->coursecontact = $oldcoursecontact; }