diff --git a/lib/completionlib.php b/lib/completionlib.php index 77c34bad9cf..94a9271327b 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -1024,6 +1024,20 @@ class completion_info { $userid = $USER->id; } + // Some call completion_info::get_data and pass $cm as an object with ID only. Make sure course is set as well. + if ($cm instanceof stdClass && !isset($cm->course)) { + $cm->course = $this->course_id; + } + // Make sure we're working on a cm_info object. + $cminfo = cm_info::create($cm, $userid); + + // Create an anonymous function to remove the 'other_cm_completion_data_fetched' key. + $returnfilteredvalue = function(array $value): stdClass { + return (object) array_filter($value, function(string $key): bool { + return $key !== 'other_cm_completion_data_fetched'; + }, ARRAY_FILTER_USE_KEY); + }; + // See if requested data is present in cache (use cache for current user only). $usecache = $userid == $USER->id; $cacheddata = array(); @@ -1036,19 +1050,22 @@ class completion_info { if ($cacheddata['cacherev'] != $this->course->cacherev) { // Course structure has been changed since the last caching, forget the cache. $cacheddata = array(); - } else if (isset($cacheddata[$cm->id])) { - return (object)$cacheddata[$cm->id]; + } else if (isset($cacheddata[$cminfo->id])) { + $data = (array) $cacheddata[$cminfo->id]; + if (empty($data['other_cm_completion_data_fetched'])) { + $data += $this->get_other_cm_completion_data($cminfo, $userid); + $data['other_cm_completion_data_fetched'] = true; + + // Put in cache. + $cacheddata[$cminfo->id] = $data; + $completioncache->set($key, $cacheddata); + } + + return $returnfilteredvalue($cacheddata[$cminfo->id]); } } } - // Some call completion_info::get_data and pass $cm as an object with ID only. Make sure course is set as well. - if ($cm instanceof stdClass && !isset($cm->course)) { - $cm->course = $this->course_id; - } - // Make sure we're working on a cm_info object. - $cminfo = cm_info::create($cm, $userid); - // Default data to return when no completion data is found. $defaultdata = [ 'id' => 0, @@ -1086,12 +1103,9 @@ class completion_info { $cacheddata[$data->cmid] = $defaultdata; $cacheddata[$data->cmid]['coursemoduleid'] = $data->cmid; } else { - $cacheddata[$data->cmid] = (array) $data; + unset($data->cmid); + $cacheddata[$data->coursemoduleid] = (array) $data; } - - // Add the other completion data for this user in this module instance. - $othercminfo = $cminfos[$data->cmid]; - $cacheddata[$othercminfo->id] += $this->get_other_cm_completion_data($othercminfo, $userid); } if (!isset($cacheddata[$cminfo->id])) { @@ -1099,6 +1113,7 @@ class completion_info { $this->internal_systemerror($errormessage); } + $data = $cacheddata[$cminfo->id]; } else { // Get single record $data = $DB->get_record('course_modules_completion', array('coursemoduleid' => $cminfo->id, 'userid' => $userid)); @@ -1108,18 +1123,24 @@ class completion_info { // Row not present counts as 'not complete'. $data = $defaultdata; } - // Fill the other completion data for this user in this module instance. - $data += $this->get_other_cm_completion_data($cminfo, $userid); - // Put in cache + // Put in cache. $cacheddata[$cminfo->id] = $data; } + // Fill the other completion data for this user in this module instance. + $data += $this->get_other_cm_completion_data($cminfo, $userid); + $data['other_cm_completion_data_fetched'] = true; + + // Put in cache + $cacheddata[$cminfo->id] = $data; + if ($usecache) { $cacheddata['cacherev'] = $this->course->cacherev; $completioncache->set($key, $cacheddata); } - return (object)$cacheddata[$cminfo->id]; + + return $returnfilteredvalue($cacheddata[$cminfo->id]); } /** @@ -1240,6 +1261,7 @@ class completion_info { $cmcontext = context_module::instance($data->coursemoduleid); $completioncache = cache::make('core', 'completion'); + $cachekey = "{$data->userid}_{$cm->course}"; if ($data->userid == $USER->id) { // Fetch other completion data to cache (e.g. require grade completion status, custom completion rule statues). $cminfo = cm_info::create($cm, $data->userid); // Make sure we're working on a cm_info object. @@ -1249,18 +1271,19 @@ class completion_info { } // Update module completion in user's cache. - if (!($cachedata = $completioncache->get($data->userid . '_' . $cm->course)) + if (!($cachedata = $completioncache->get($cachekey)) || $cachedata['cacherev'] != $this->course->cacherev) { $cachedata = array('cacherev' => $this->course->cacherev); } - $cachedata[$cm->id] = $data; - $completioncache->set($data->userid . '_' . $cm->course, $cachedata); + $cachedata[$cm->id] = (array) $data; + $cachedata[$cm->id]['other_cm_completion_data_fetched'] = true; + $completioncache->set($cachekey, $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); + $completioncache->delete($cachekey); } // For single user actions the code must reevaluate some completion state instantly, see MDL-32103. diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index b393c90dbe8..ecd75696b2f 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -89,6 +89,10 @@ class completionlib_test extends advanced_testcase { public static function assertEquals($expected, $actual, string $message = '', float $delta = 0, int $maxDepth = 10, bool $canonicalize = false, bool $ignoreCase = false): void { // Nasty cheating hack: prevent random failures on timemodified field. + if (is_array($actual) && (is_object($expected) || is_array($expected))) { + $actual = (object) $actual; + $expected = (object) $expected; + } if (is_object($expected) and is_object($actual)) { if (property_exists($expected, 'timemodified') and property_exists($actual, 'timemodified')) { if ($expected->timemodified + 1 == $actual->timemodified) { @@ -804,6 +808,70 @@ class completionlib_test extends advanced_testcase { } } + /** + * @covers ::get_data + */ + public function test_get_data_successive_calls(): void { + global $DB; + + $this->setup_data(); + $this->setUser($this->user); + + $choicegenerator = $this->getDataGenerator()->get_plugin_generator('mod_choice'); + $choice = $choicegenerator->create_instance([ + 'course' => $this->course->id, + 'completion' => COMPLETION_TRACKING_AUTOMATIC, + 'completionview' => true, + 'completionsubmit' => true, + ]); + + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Let's manually create a course completion record instead of going through the hoops to complete an activity. + $cmcompletionrecord = (object) [ + 'coursemoduleid' => $cm->id, + 'userid' => $this->user->id, + 'completionstate' => COMPLETION_NOT_VIEWED, + 'viewed' => 0, + 'overrideby' => null, + 'timemodified' => 0, + ]; + $DB->insert_record('course_modules_completion', $cmcompletionrecord); + + // Mock other completion data. + $completioninfo = new completion_info($this->course); + + $modinfo = get_fast_modinfo($this->course); + $results = []; + foreach ($modinfo->cms as $testcm) { + $result = $completioninfo->get_data($testcm, true); + $this->assertTrue(property_exists($result, 'id')); + $this->assertTrue(property_exists($result, 'coursemoduleid')); + $this->assertTrue(property_exists($result, 'userid')); + $this->assertTrue(property_exists($result, 'completionstate')); + $this->assertTrue(property_exists($result, 'viewed')); + $this->assertTrue(property_exists($result, 'overrideby')); + $this->assertTrue(property_exists($result, 'timemodified')); + $this->assertFalse(property_exists($result, 'other_cm_completion_data_fetched')); + + $this->assertEquals($testcm->id, $result->coursemoduleid); + $this->assertEquals($this->user->id, $result->userid); + $this->assertEquals(0, $result->viewed); + + $results[$testcm->id] = $result; + } + + $result = $completioninfo->get_data($cm); + $this->assertTrue(property_exists($result, 'customcompletion')); + + // The data should match when fetching modules individually. + (cache::make('core', 'completion'))->purge(); + foreach ($modinfo->cms as $testcm) { + $result = $completioninfo->get_data($testcm, false); + $this->assertEquals($result, $results[$testcm->id]); + } + } + /** * Tests for completion_info::get_other_cm_completion_data(). * @@ -909,8 +977,15 @@ class completionlib_test extends advanced_testcase { $this->assertEquals($d1, $data->id); $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)); + $cachevalue = $cache->get("{$data->userid}_{$cm->course}"); + $this->assertEquals([ + 'cacherev' => $this->course->cacherev, + $cm->id => array_merge( + (array) $data, + ['other_cm_completion_data_fetched' => true] + ), + ], + $cachevalue); // 2) Test with existing data and for different user. $forum2 = $this->getDataGenerator()->create_module('forum', array('course' => $this->course->id), $completionauto); @@ -928,7 +1003,11 @@ class completionlib_test extends advanced_testcase { $c->internal_set_data($cm2, $d2); // Cache for current user returns the data. $cachevalue = $cache->get($data->userid . '_' . $cm->course); - $this->assertEquals($data, $cachevalue[$cm->id]); + $this->assertEquals(array_merge( + (array) $data, + ['other_cm_completion_data_fetched' => true] + ), $cachevalue[$cm->id]); + // Cache for another user is not filled. $this->assertEquals(false, $cache->get($d2->userid . '_' . $cm2->course));