diff --git a/completion/tests/coverage.php b/completion/tests/coverage.php new file mode 100644 index 00000000000..2a758a79d66 --- /dev/null +++ b/completion/tests/coverage.php @@ -0,0 +1,47 @@ +. + +defined('MOODLE_INTERNAL') || die(); + +/** + * Coverage information for the core_completion. + * + * @package core + * @category phpunit + * @copyright 2022 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +/** + * Coverage information for the core subsystem. + * + * @copyright 2018 Andrew Nicols + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +return new class extends phpunit_coverage_info { + /** @var array The list of folders relative to the plugin root to include in coverage generation. */ + protected $includelistfolders = [ + 'criteria', + ]; + + /** @var array The list of files relative to the plugin root to include in coverage generation. */ + protected $includelistfiles = [ + 'completion_aggregation.php', + 'completion_completion.php', + 'completion_criteria_completion.php', + 'data_object.php' + ]; +}; 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 00ae923f469..ecd75696b2f 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -29,7 +29,17 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->libdir.'/completionlib.php'); -class core_completionlib_testcase extends advanced_testcase { +/** + * Completion tests. + * + * @package core_completion + * @category phpunit + * @copyright 2008 Sam Marshall + * @copyright 2013 Frédéric Massart + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \completion_info + */ +class completionlib_test extends advanced_testcase { protected $course; protected $user; protected $module1; @@ -79,6 +89,10 @@ class core_completionlib_testcase 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) { @@ -90,6 +104,10 @@ class core_completionlib_testcase extends advanced_testcase { parent::assertEquals($expected, $actual, $message, $delta, $maxDepth, $canonicalize, $ignoreCase); } + /** + * @covers ::is_enabled_for_site + * @covers ::is_enabled + */ public function test_is_enabled() { global $CFG; $this->mock_setup(); @@ -125,6 +143,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(COMPLETION_TRACKING_AUTOMATIC, $c->is_enabled($cm)); } + /** + * @covers ::update_state + */ public function test_update_state() { $this->mock_setup(); @@ -348,6 +369,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param int $completionusegrade * @param string $unsetfield * @param int $expectedstate + * @covers ::internal_get_state */ public function test_internal_get_state(int $completionview, int $completionusegrade, string $unsetfield, int $expectedstate) { $this->setup_data(); @@ -379,6 +401,7 @@ class core_completionlib_testcase extends advanced_testcase { * Provider for the test_internal_get_state_with_grade_criteria. * * @return array + * @covers ::internal_get_state */ public function test_internal_get_state_with_grade_criteria_provider() { return [ @@ -432,6 +455,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param array $completioncriteria The completion criteria to use * @param int|null $studentgrade Grade to assign to student * @param int $expectedstate Expected completion state + * @covers ::internal_get_state */ public function test_internal_get_state_with_grade_criteria(array $completioncriteria, ?int $studentgrade, int $expectedstate) { $this->setup_data(); @@ -472,6 +496,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Covers the case where internal_get_state() is being called for a user different from the logged in user. + * + * @covers ::internal_get_state */ public function test_internal_get_state_with_different_user() { $this->setup_data(); @@ -514,6 +540,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for internal_get_state() for an activity that supports custom completion. + * + * @covers ::internal_get_state */ public function test_internal_get_state_with_custom_completion() { $this->setup_data(); @@ -540,6 +568,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(COMPLETION_COMPLETE, $completion); } + /** + * @covers ::set_module_viewed + */ public function test_set_module_viewed() { $this->mock_setup(); @@ -595,6 +626,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->set_module_viewed($cm, 1337); } + /** + * @covers ::count_user_data + */ public function test_count_user_data() { global $DB; $this->mock_setup(); @@ -611,6 +645,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(666, $c->count_user_data($cm)); } + /** + * @covers ::delete_all_state + */ public function test_delete_all_state() { global $DB; $this->mock_setup(); @@ -628,6 +665,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->delete_all_state($cm); } + /** + * @covers ::reset_all_state + */ public function test_reset_all_state() { global $DB; $this->mock_setup(); @@ -703,6 +743,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param bool $sameuser Whether the user calling get_data() is the user itself. * @param bool $hasrecord Whether to create a course_modules_completion record. * @param int $completion The completion state expected. + * @covers ::get_data */ public function test_get_data(bool $wholecourse, bool $sameuser, bool $hasrecord, int $completion) { global $DB; @@ -760,26 +801,6 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals(0, $result->id); } - // Check caching. - $key = "{$user->id}_{$this->course->id}"; - $cache = cache::make('core', 'completion'); - if ($iscached) { - // If we expect this to be cached, then fetching the result must match the cached data. - $this->assertEquals($result, (object)$cache->get($key)[$cm->id]); - - // Check cached data for other course modules in the course. - // The sample module created in setup_data() should suffice to confirm this. - $othercm = get_coursemodule_from_instance('forum', $this->module1->id); - if ($wholecourse) { - $this->assertArrayHasKey($othercm->id, $cache->get($key)); - } else { - $this->assertArrayNotHasKey($othercm->id, $cache->get($key)); - } - } else { - // Otherwise, this should not be cached. - $this->assertFalse($cache->get($key)); - } - // Check that we are including relevant completion data for the module. if (!$wholecourse) { $this->assertTrue(property_exists($result, 'viewed')); @@ -787,8 +808,74 @@ class core_completionlib_testcase 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(). + * + * @covers ::get_other_cm_completion_data */ public function test_get_other_cm_completion_data() { global $DB; @@ -862,6 +949,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEmpty($choice2completiondata); } + /** + * @covers ::internal_set_data + */ public function test_internal_set_data() { global $DB; $this->setup_data(); @@ -887,8 +977,15 @@ class core_completionlib_testcase 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); @@ -906,7 +1003,11 @@ class core_completionlib_testcase 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)); @@ -958,6 +1059,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals($this->user->id, reset($actual)->userid); } + /** + * @covers ::get_progress_all + */ public function test_get_progress_all_few() { global $DB; $this->mock_setup(); @@ -992,6 +1096,9 @@ class core_completionlib_testcase extends advanced_testcase { ), $c->get_progress_all(false)); } + /** + * @covers ::get_progress_all + */ public function test_get_progress_all_lots() { global $DB; $this->mock_setup(); @@ -1047,6 +1154,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertCount(count($tracked), $result); } + /** + * @covers ::inform_grade_changed + */ public function test_inform_grade_changed() { $this->mock_setup(); @@ -1115,6 +1225,9 @@ class core_completionlib_testcase extends advanced_testcase { $c->inform_grade_changed($cm, $item, $grade, true); } + /** + * @covers ::internal_get_grade_state + */ public function test_internal_get_grade_state() { $this->mock_setup(); @@ -1157,6 +1270,9 @@ class core_completionlib_testcase extends advanced_testcase { completion_info::internal_get_grade_state($item, $grade)); } + /** + * @test ::get_activities + */ public function test_get_activities() { global $CFG; $this->resetAfterTest(); @@ -1198,6 +1314,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertFalse(isset($activities[$data2->cmid])); } + /** + * @test ::has_activities + */ public function test_has_activities() { global $CFG; $this->resetAfterTest(); @@ -1223,7 +1342,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test that data is cleaned up when we delete courses that are set as completion criteria for other courses * - * @return void + * @covers ::delete_course_completion_data + * @covers ::delete_all_completion_data */ public function test_course_delete_prerequisite() { global $DB; @@ -1260,6 +1380,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course module completion update event. + * + * @covers \core\event\course_module_completion_updated */ public function test_course_module_completion_updated_event() { global $USER, $CFG; @@ -1297,6 +1419,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed event. + * + * @covers \core\event\course_completed */ public function test_course_completed_event() { global $USER; @@ -1326,6 +1450,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed message. + * + * @covers \core\event\course_completed */ public function test_course_completed_message() { $this->setup_data(); @@ -1352,6 +1478,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test course completed event. + * + * @covers \core\event\course_completion_updated */ public function test_course_completion_updated_event() { $this->setup_data(); @@ -1378,6 +1506,9 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEventLegacyLogData($expectedlegacylog, $event); } + /** + * @covers \completion_can_view_data + */ public function test_completion_can_view_data() { $this->setup_data(); @@ -1413,6 +1544,7 @@ class core_completionlib_testcase extends advanced_testcase { * @param int|null $passinggrade Passing grade to set for the test activity. * @param string|null $expectedexception Expected exception. * @param int|null $expectedresult The expected completion status. + * @covers ::get_grade_completion */ public function test_get_grade_completion(bool $completionusegrade, bool $hasgrade, ?int $passinggrade, ?string $expectedexception, ?int $expectedresult) { @@ -1445,6 +1577,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test the return value for cases when the activity module does not have associated grade_item. + * + * @covers ::get_grade_completion */ public function test_get_grade_completion_without_grade_item() { global $DB; @@ -1483,6 +1617,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for aggregate_completions(). + * + * @covers \aggregate_completions */ public function test_aggregate_completions() { global $DB; @@ -1577,6 +1713,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::_save(). + * + * @covers \completion_completion::_save */ public function test_save() { global $DB; @@ -1621,6 +1759,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_enrolled(). + * + * @covers \completion_completion::mark_enrolled */ public function test_mark_enrolled() { global $DB; @@ -1662,6 +1802,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_inprogress(). + * + * @covers \completion_completion::mark_inprogress */ public function test_mark_inprogress() { global $DB; @@ -1703,6 +1845,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_completion::mark_complete(). + * + * @covers \completion_completion::mark_complete */ public function test_mark_complete() { global $DB; @@ -1743,6 +1887,8 @@ class core_completionlib_testcase extends advanced_testcase { /** * Test for completion_criteria_completion::mark_complete(). + * + * @covers \completion_criteria_completion::mark_complete */ public function test_criteria_mark_complete() { global $DB;