diff --git a/badges/criteria/award_criteria_activity.php b/badges/criteria/award_criteria_activity.php index 8f4d4b039c6..fa5b5001098 100644 --- a/badges/criteria/award_criteria_activity.php +++ b/badges/criteria/award_criteria_activity.php @@ -37,7 +37,7 @@ class award_criteria_activity extends award_criteria { public $criteriatype = BADGE_CRITERIA_TYPE_ACTIVITY; private $courseid; - private $coursestartdate; + private $course; public $required_param = 'module'; public $optional_params = array('bydate'); @@ -46,11 +46,10 @@ class award_criteria_activity extends award_criteria { global $DB; parent::__construct($record); - $course = $DB->get_record_sql('SELECT b.courseid, c.startdate + $this->course = $DB->get_record_sql('SELECT c.id, c.enablecompletion, c.cacherev, c.startdate FROM {badge} b INNER JOIN {course} c ON b.courseid = c.id WHERE b.id = :badgeid ', array('badgeid' => $this->badgeid)); - $this->courseid = $course->courseid; - $this->coursestartdate = $course->startdate; + $this->courseid = $this->course->id; } /** @@ -107,13 +106,11 @@ class award_criteria_activity extends award_criteria { * */ public function get_options(&$mform) { - global $DB; - $none = true; $existing = array(); $missing = array(); - $course = $DB->get_record('course', array('id' => $this->courseid)); + $course = $this->course; $info = new completion_info($course); $mods = $info->get_activities(); $mids = array_keys($mods); @@ -187,14 +184,12 @@ class award_criteria_activity extends award_criteria { */ public function review($userid, $filtered = false) { $completionstates = array(COMPLETION_COMPLETE, COMPLETION_COMPLETE_PASS); - $course = new stdClass(); - $course->id = $this->courseid; - if ($this->coursestartdate > time()) { + if ($this->course->startdate > time()) { return false; } - $info = new completion_info($course); + $info = new completion_info($this->course); $overall = false; foreach ($this->params as $param) { diff --git a/badges/criteria/award_criteria_course.php b/badges/criteria/award_criteria_course.php index 25823656823..88625077983 100644 --- a/badges/criteria/award_criteria_course.php +++ b/badges/criteria/award_criteria_course.php @@ -39,7 +39,7 @@ class award_criteria_course extends award_criteria { public $criteriatype = BADGE_CRITERIA_TYPE_COURSE; private $courseid; - private $coursestartdate; + private $course; public $required_param = 'course'; public $optional_params = array('grade', 'bydate'); @@ -48,11 +48,10 @@ class award_criteria_course extends award_criteria { global $DB; parent::__construct($record); - $course = $DB->get_record_sql('SELECT b.courseid, c.startdate + $this->course = $DB->get_record_sql('SELECT c.id, c.enablecompletion, c.cacherev, c.startdate FROM {badge} b INNER JOIN {course} c ON b.courseid = c.id WHERE b.id = :badgeid ', array('badgeid' => $this->badgeid)); - $this->courseid = $course->courseid; - $this->coursestartdate = $course->startdate; + $this->courseid = $this->course->id; } /** @@ -180,10 +179,9 @@ class award_criteria_course extends award_criteria { * @return bool Whether criteria is complete */ public function review($userid, $filtered = false) { - $course = new stdClass(); - $course->id = $this->courseid; + $course = $this->course; - if ($this->coursestartdate > time()) { + if ($this->course->startdate > time()) { return false; } diff --git a/lang/en/cache.php b/lang/en/cache.php index 5fac49e8984..3e633792143 100644 --- a/lang/en/cache.php +++ b/lang/en/cache.php @@ -43,6 +43,7 @@ $string['cachedef_coursecatrecords'] = 'Course categories records'; $string['cachedef_coursecontacts'] = 'List of course contacts'; $string['cachedef_coursecattree'] = 'Course categories tree'; $string['cachedef_coursemodinfo'] = 'Accumulated information about modules and sections for each course'; +$string['cachedef_completion'] = 'Activity completion status'; $string['cachedef_databasemeta'] = 'Database meta information'; $string['cachedef_eventinvalidation'] = 'Event invalidation'; $string['cachedef_externalbadges'] = 'External badges for particular user'; @@ -173,4 +174,4 @@ $string['userinputsharingkey_help'] = 'Enter your own private key here. When you // Deprecated since 2.9. $string['lockingmeans'] = 'Locking mechanism'; $string['lockmethod'] = 'Lock method'; -$string['lockmethod_help'] = 'This is the method used for locking when required of this store.'; \ No newline at end of file +$string['lockmethod_help'] = 'This is the method used for locking when required of this store.'; diff --git a/lib/completionlib.php b/lib/completionlib.php index 2e83745084b..62afaf59929 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -119,12 +119,6 @@ define('COMPLETION_VIEWED', 1); */ define('COMPLETION_NOT_VIEWED', 0); -/** - * Cache expiry time in seconds (10 minutes) - * Completion cacheing - */ -define('COMPLETION_CACHE_EXPIRY', 10*60); - /** * Completion details should be ORed together and you should return false if * none apply. @@ -250,7 +244,9 @@ class completion_info { * Constructs with course details. * * When instantiating a new completion info object you must provide a course - * object with at least id, and enablecompletion properties. + * object with at least id, and enablecompletion properties. Property + * cacherev is needed if you check completion of the current user since + * it is used for cache validation. * * @param stdClass $course Moodle course object. */ @@ -290,7 +286,7 @@ class completion_info { // Load data if we do not have enough if (!isset($this->course->enablecompletion)) { - $this->course->enablecompletion = $DB->get_field('course', 'enablecompletion', array('id' => $this->course->id)); + $this->course = get_course($this->course_id); } // Check course completion @@ -553,7 +549,7 @@ class completion_info { * @return void */ public function update_state($cm, $possibleresult=COMPLETION_UNKNOWN, $userid=0) { - global $USER, $SESSION; + global $USER; // Do nothing if completion is not enabled for that activity if (!$this->is_enabled($cm)) { @@ -783,6 +779,9 @@ class completion_info { $DB->delete_records('course_completions', array('course' => $this->course_id)); $DB->delete_records('course_completion_crit_compl', array('course' => $this->course_id)); + + // Difficult to find affected users, just purge all completion cache. + cache::make('core', 'completion')->purge(); } /** @@ -792,20 +791,13 @@ class completion_info { * Used by course reset page. */ public function delete_all_completion_data() { - global $DB, $SESSION; + global $DB; // Delete from database. $DB->delete_records_select('course_modules_completion', 'coursemoduleid IN (SELECT id FROM {course_modules} WHERE course=?)', array($this->course_id)); - // Reset cache for current user. - if (isset($SESSION->completioncache) && - array_key_exists($this->course_id, $SESSION->completioncache)) { - - unset($SESSION->completioncache[$this->course_id]); - } - // Wipe course completion data too. $this->delete_course_completion_data(); } @@ -818,19 +810,11 @@ class completion_info { * @param stdClass|cm_info $cm Activity */ public function delete_all_state($cm) { - global $SESSION, $DB; + global $DB; // Delete from database $DB->delete_records('course_modules_completion', array('coursemoduleid'=>$cm->id)); - // Erase cache data for current user if applicable - if (isset($SESSION->completioncache) && - array_key_exists($cm->course, $SESSION->completioncache) && - array_key_exists($cm->id, $SESSION->completioncache[$cm->course])) { - - unset($SESSION->completioncache[$cm->course][$cm->id]); - } - // Check if there is an associated course completion criteria $criteria = $this->get_criteria(COMPLETION_CRITERIA_TYPE_ACTIVITY); $acriteria = false; @@ -846,6 +830,9 @@ class completion_info { $DB->delete_records('course_completion_crit_compl', array('course' => $this->course_id, 'criteriaid' => $acriteria->id)); $DB->delete_records('course_completions', array('course' => $this->course_id)); } + + // Difficult to find affected users, just purge all completion cache. + cache::make('core', 'completion')->purge(); } /** @@ -876,7 +863,7 @@ class completion_info { } $rs->close(); - // Delete all existing state [also clears session cache for current user] + // Delete all existing state. $this->delete_all_state($cm); // Merge this with list of planned users (according to roles) @@ -894,7 +881,7 @@ class completion_info { /** * Obtains completion data for a particular activity and user (from the - * session cache if available, or by SQL query) + * completion cache if available, or by SQL query) * * @param stcClass|cm_info $cm Activity; only required field is ->id * @param bool $wholecourse If true (default false) then, when necessary to @@ -908,39 +895,33 @@ class completion_info { * @return object Completion data (record from course_modules_completion) */ public function get_data($cm, $wholecourse = false, $userid = 0, $modinfo = null) { - global $USER, $CFG, $SESSION, $DB; + global $USER, $CFG, $DB; + $completioncache = cache::make('core', 'completion'); // Get user ID if (!$userid) { $userid = $USER->id; } - // Is this the current user? - $currentuser = $userid==$USER->id; - - if ($currentuser && is_object($SESSION)) { - // Make sure cache is present and is for current user (loginas - // changes this) - if (!isset($SESSION->completioncache) || $SESSION->completioncacheuserid!=$USER->id) { - $SESSION->completioncache = array(); - $SESSION->completioncacheuserid = $USER->id; + // See if requested data is present in cache (use cache for current user only). + $usecache = $userid == $USER->id; + $cacheddata = array(); + if ($usecache) { + if (!isset($this->course->cacherev)) { + $this->course = get_course($this->course_id); } - // Expire any old data from cache - foreach ($SESSION->completioncache as $courseid=>$activities) { - if (empty($activities['updated']) || $activities['updated'] < time()-COMPLETION_CACHE_EXPIRY) { - unset($SESSION->completioncache[$courseid]); + if ($cacheddata = $completioncache->get($userid . '_' . $this->course->id)) { + if ($cacheddata['cacherev'] != $this->course->cacherev) { + // Course structure has been changed since the last caching, forget the cache. + $cacheddata = array(); + } else if (array_key_exists($cm->id, $cacheddata)) { + return $cacheddata[$cm->id]; } } - // See if requested data is present, if so use cache to get it - if (isset($SESSION->completioncache) && - array_key_exists($this->course->id, $SESSION->completioncache) && - array_key_exists($cm->id, $SESSION->completioncache[$this->course->id])) { - return $SESSION->completioncache[$this->course->id][$cm->id]; - } } // Not there, get via SQL - if ($currentuser && $wholecourse) { + if ($wholecourse) { // Get whole course data for cache $alldatabycmc = $DB->get_records_sql(" SELECT @@ -976,14 +957,12 @@ class completion_info { $data->viewed = 0; $data->timemodified = 0; } - $SESSION->completioncache[$this->course->id][$othercm->id] = $data; + $cacheddata[$othercm->id] = $data; } - $SESSION->completioncache[$this->course->id]['updated'] = time(); - if (!isset($SESSION->completioncache[$this->course->id][$cm->id])) { + if (!isset($cacheddata[$cm->id])) { $this->internal_systemerror("Unexpected error: course-module {$cm->id} could not be found on course {$this->course->id}"); } - return $SESSION->completioncache[$this->course->id][$cm->id]; } else { // Get single record @@ -1000,16 +979,14 @@ class completion_info { } // Put in cache - if ($currentuser) { - $SESSION->completioncache[$this->course->id][$cm->id] = $data; - // For single updates, only set date if it was empty before - if (empty($SESSION->completioncache[$this->course->id]['updated'])) { - $SESSION->completioncache[$this->course->id]['updated'] = time(); - } - } + $cacheddata[$cm->id] = $data; } - return $data; + if ($usecache) { + $cacheddata['cacherev'] = $this->course->cacherev; + $completioncache->set($userid . '_' . $this->course->id, $cacheddata); + } + return $cacheddata[$cm->id]; } /** @@ -1022,7 +999,7 @@ class completion_info { * @param stdClass $data Data about completion for that user */ public function internal_set_data($cm, $data) { - global $USER, $SESSION, $DB; + global $USER, $DB; $transaction = $DB->start_delegated_transaction(); if (!$data->id) { @@ -1054,10 +1031,21 @@ class completion_info { $event->add_record_snapshot('course_modules_completion', $data); $event->trigger(); + $completioncache = cache::make('core', 'completion'); if ($data->userid == $USER->id) { - $SESSION->completioncache[$cm->course][$cm->id] = $data; + // Update module completion in user's cache. + if (!($cachedata = $completioncache->get($data->userid . '_' . $cm->course)) + || $cachedata['cacherev'] != $this->course->cacherev) { + $cachedata = array('cacherev' => $this->course->cacherev); + } + $cachedata[$cm->id] = $data; + $completioncache->set($data->userid . '_' . $cm->course, $cachedata); + // reset modinfo for user (no need to call rebuild_course_cache()) get_fast_modinfo($cm->course, 0, true); + } else { + // Remove another user's completion cache for this course. + $completioncache->delete($data->userid . '_' . $cm->course); } } @@ -1341,13 +1329,4 @@ class completion_info { throw new moodle_exception('err_system','completion', $CFG->wwwroot.'/course/view.php?id='.$this->course->id,null,$error); } - - /** - * For testing only. Wipes information cached in user session. - */ - public static function wipe_session_cache() { - global $SESSION; - unset($SESSION->completioncache); - unset($SESSION->completioncacheuserid); - } } diff --git a/lib/db/caches.php b/lib/db/caches.php index e71dc577fee..98dadff4601 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -213,6 +213,15 @@ $definitions = array( 'ttl' => 3600, ), + // Used to cache activity completion status. + 'completion' => array( + 'mode' => cache_store::MODE_APPLICATION, + 'simplekeys' => true, + 'ttl' => 3600, + 'staticacceleration' => true, + 'staticaccelerationsize' => 2, // Should be current course and site course. + ), + // A simple cache that stores whether a user can expand a course in the navigation. // The key is the course ID and the value will either be 1 or 0 (cast to bool). // The cache isn't always up to date, it should only ever be used to save a costly call to diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index 150bafc789d..0484b7cc3ad 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -325,7 +325,7 @@ class core_completionlib_testcase extends advanced_testcase { } public function test_delete_all_state() { - global $DB, $SESSION; + global $DB; $this->mock_setup(); $course = (object)array('id'=>13); @@ -339,21 +339,6 @@ class core_completionlib_testcase extends advanced_testcase { ->with('course_modules_completion', array('coursemoduleid'=>42)) ->will($this->returnValue(true)); $c->delete_all_state($cm); - - // Build up a session to check it deletes the right bits from it - // (and not other bits). - $SESSION->completioncache = array(); - $SESSION->completioncache[13] = array(); - $SESSION->completioncache[13][42] = 'foo'; - $SESSION->completioncache[13][43] = 'foo'; - $SESSION->completioncache[14] = array(); - $SESSION->completioncache[14][42] = 'foo'; - $DB->expects($this->at(0)) - ->method('delete_records') - ->with('course_modules_completion', array('coursemoduleid'=>42)) - ->will($this->returnValue(true)); - $c->delete_all_state($cm); - $this->assertEquals(array(13=>array(43=>'foo'), 14=>array(42=>'foo')), $SESSION->completioncache); } public function test_reset_all_state() { @@ -396,10 +381,12 @@ class core_completionlib_testcase extends advanced_testcase { } public function test_get_data() { - global $DB, $SESSION; + global $DB; $this->mock_setup(); - $c = new completion_info((object)array('id'=>42)); + $cache = cache::make('core', 'completion'); + + $c = new completion_info((object)array('id'=>42, 'cacherev'=>1)); $cm = (object)array('id'=>13, 'course'=>42); // 1. Not current user, record exists. @@ -412,18 +399,20 @@ class core_completionlib_testcase extends advanced_testcase { ->will($this->returnValue($sillyrecord)); $result = $c->get_data($cm, false, 123); $this->assertEquals($sillyrecord, $result); - $this->assertFalse(isset($SESSION->completioncache)); + $this->assertEquals(false, $cache->get('123_42')); // Not current user is not cached. - // 2. Not current user, default record, whole course (ignored). + // 2. Not current user, default record, whole course. + $cache->purge(); $DB->expects($this->at(0)) - ->method('get_record') - ->with('course_modules_completion', array('coursemoduleid'=>13, 'userid'=>123)) - ->will($this->returnValue(false)); - $result=$c->get_data($cm, true, 123); + ->method('get_records_sql') + ->will($this->returnValue(array())); + $modinfo = new stdClass(); + $modinfo->cms = array((object)array('id'=>13)); + $result=$c->get_data($cm, true, 123, $modinfo); $this->assertEquals((object)array( 'id'=>'0', 'coursemoduleid'=>13, 'userid'=>123, 'completionstate'=>0, 'viewed'=>0, 'timemodified'=>0), $result); - $this->assertFalse(isset($SESSION->completioncache)); + $this->assertEquals(false, $cache->get('123_42')); // Not current user is not cached. // 3. Current user, single record, not from cache. $DB->expects($this->at(0)) @@ -432,34 +421,15 @@ class core_completionlib_testcase extends advanced_testcase { ->will($this->returnValue($sillyrecord)); $result = $c->get_data($cm); $this->assertEquals($sillyrecord, $result); - $this->assertEquals($sillyrecord, $SESSION->completioncache[42][13]); - // When checking time(), allow for second overlaps. - $this->assertTrue(time()-$SESSION->completioncache[42]['updated']<2); + $cachevalue = $cache->get('314159_42'); + $this->assertEquals($sillyrecord, $cachevalue[13]); // 4. Current user, 'whole course', but from cache. $result = $c->get_data($cm, true); $this->assertEquals($sillyrecord, $result); - // 5. Current user, single record, cache expired - $SESSION->completioncache[42]['updated']=37; // Quite a long time ago. - $now = time(); - $SESSION->completioncache[17]['updated']=$now; - $SESSION->completioncache[39]['updated']=72; // Also a long time ago. - $DB->expects($this->at(0)) - ->method('get_record') - ->with('course_modules_completion', array('coursemoduleid'=>13, 'userid'=>314159)) - ->will($this->returnValue($sillyrecord)); - $result = $c->get_data($cm, false); - $this->assertEquals($sillyrecord, $result); - - // Check that updated value is right, then fudge it to make next compare work. - $this->assertTrue(time()-$SESSION->completioncache[42]['updated']<2); - $SESSION->completioncache[42]['updated']=$now; - // Check things got expired from cache. - $this->assertEquals(array(42=>array(13=>$sillyrecord, 'updated'=>$now), 17=>array('updated'=>$now)), $SESSION->completioncache); - - // 6. Current user, 'whole course' and record not in cache. - unset($SESSION->completioncache); + // 5. Current user, 'whole course' and record not in cache. + $cache->purge(); // Scenario: Completion data exists for one CMid. $basicrecord = (object)array('coursemoduleid'=>13); @@ -476,15 +446,15 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals($basicrecord, $result); // Check the cache contents. - $this->assertTrue(time()-$SESSION->completioncache[42]['updated']<2); - $SESSION->completioncache[42]['updated'] = $now; - $this->assertEquals(array(42=>array(13=>$basicrecord, 14=>(object)array( - 'id'=>'0', 'coursemoduleid'=>14, 'userid'=>314159, 'completionstate'=>0, - 'viewed'=>0, 'timemodified'=>0), 'updated'=>$now)), $SESSION->completioncache); + $cachevalue = $cache->get('314159_42'); + $this->assertEquals($basicrecord, $cachevalue[13]); + $this->assertEquals((object)array('id'=>'0', 'coursemoduleid'=>14, + 'userid'=>314159, 'completionstate'=>0, 'viewed'=>0, 'timemodified'=>0), + $cachevalue[14]); } public function test_internal_set_data() { - global $DB, $SESSION; + global $DB; $this->setup_data(); $this->setUser($this->user); @@ -505,10 +475,12 @@ class core_completionlib_testcase extends advanced_testcase { $c->internal_set_data($cm, $data); $d1 = $DB->get_field('course_modules_completion', 'id', array('coursemoduleid' => $cm->id)); $this->assertEquals($d1, $data->id); - $this->assertEquals(array($this->course->id => array($cm->id => $data)), $SESSION->completioncache); + $cache = cache::make('core', 'completion'); + // Cache was not set for another user. + $this->assertEquals(array('cacherev' => $this->course->cacherev, $cm->id => $data), + $cache->get($data->userid . '_' . $cm->course)); - // 2) Test with existing data and for different user (not cached). - unset($SESSION->completioncache); + // 2) Test with existing data and for different user. $forum2 = $this->getDataGenerator()->create_module('forum', array('course' => $this->course->id), $completionauto); $cm2 = get_coursemodule_from_instance('forum', $forum2->id); $newuser = $this->getDataGenerator()->create_user(); @@ -521,7 +493,11 @@ class core_completionlib_testcase extends advanced_testcase { $d2->timemodified = time(); $d2->viewed = COMPLETION_NOT_VIEWED; $c->internal_set_data($cm2, $d2); - $this->assertFalse(isset($SESSION->completioncache)); + // Cache for current user returns the data. + $cachevalue = $cache->get($data->userid . '_' . $cm->course); + $this->assertEquals($data, $cachevalue[$cm->id]); + // Cache for another user is not filled. + $this->assertEquals(false, $cache->get($d2->userid . '_' . $cm2->course)); // 3) Test where it THINKS the data is new (from cache) but actually // in the database it has been set since.