From e38f50dc5d633174b40a9040c0904ed4f62e7f31 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Thu, 30 Nov 2023 11:38:49 +0100 Subject: [PATCH 1/3] MDL-77924 core: Fix get_cm when module has been disabled * This fixes an issue when trying to delete an activity and the activity is disabled as get_cm was not able to retrieve the information for this module --- course/tests/behat/course_deletion.feature | 53 +++++++++++ lib/modinfolib.php | 100 +++++++++++++++++---- lib/tests/moodlelib_test.php | 80 +++++++++++++++++ 3 files changed, 214 insertions(+), 19 deletions(-) create mode 100644 course/tests/behat/course_deletion.feature diff --git a/course/tests/behat/course_deletion.feature b/course/tests/behat/course_deletion.feature new file mode 100644 index 00000000000..c7ab2e6fda7 --- /dev/null +++ b/course/tests/behat/course_deletion.feature @@ -0,0 +1,53 @@ +@core @core_course +Feature: Teachers can delete courses with some modules disabled. + In order to delete a course + As moodle admin + I should be able to delete the course even when modules are disabled. + + Background: + Given the following "categories" exist: + | name | category | idnumber | + | Cat 1 | 0 | CAT1 | + # We use group mode here to cover all possible case for which we might need to get a cm_info. + And the following "courses" exist: + | fullname | shortname | category | groupmode | + | Course 1 | C1 | CAT1 | 1 | + And the following "activities" exist: + | activity | name | intro | course | idnumber | groupmode | + | bigbluebuttonbn | B1 | BigBlueButton B1 | C1 | b1 | 1 | + | assign | A1 | Assignment 1 | C1 | ass1 | 1 | + And the following "groups" exist: + | name | course | idnumber | + | Group 1 | C1 | G1 | + | Group 2 | C1 | G2 | + And the following "groupings" exist: + | name | course | idnumber | + | Grouping 1 | C1 | GG1 | + | Grouping 2 | C1 | GG2 | + + Scenario: Test deleting a course when course has modules disabled + Given I am logged in as "admin" + And I navigate to "Plugins > Activity modules > Manage activities" in site administration + And I click on "Disable Assignment" "link" + And I should see "Assignment disabled." + And I click on "Disable BigBlueButton" "link" + And I should see "BigBlueButton disabled." + And I navigate to "Courses > Manage courses and categories" in site administration + And I click on "Cat 1" "link" in the "course-category-listings" "region" + And I click on "Delete" "link" in the "course-listing" "region" + And I should see "Confirm" + And I press "Delete" + And I should see "C1 has been completely deleted" + When I navigate to "Courses > Manage courses and categories" in site administration + Then I should not see "Course 1" + + Scenario: Test deleting a course when all modules are enabled + Given I am logged in as "admin" + And I navigate to "Courses > Manage courses and categories" in site administration + And I click on "Cat 1" "link" in the "course-category-listings" "region" + And I click on "Delete" "link" in the "course-listing" "region" + And I should see "Confirm" + And I press "Delete" + And I should see "C1 has been completely deleted" + When I navigate to "Courses > Manage courses and categories" in site administration + Then I should not see "Course 1" diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 06ed6cc8ddc..cc3f462cce3 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -112,6 +112,12 @@ class course_modinfo { */ private $cms; + /** + * Array from int (cm id) => cm_info object + * @var cm_info[] + */ + private $disabledcms; + /** * Array from string (modname) => int (instance id) => cm_info object * @var cm_info[][] @@ -247,7 +253,10 @@ class course_modinfo { */ public function get_cm($cmid) { if (empty($this->cms[$cmid])) { - throw new moodle_exception('invalidcoursemoduleid', 'error', '', $cmid); + if (empty($this->disabledcms[$cmid])) { + throw new moodle_exception('invalidcoursemoduleid', 'error', '', $cmid); + } + return $this->disabledcms[$cmid]; } return $this->cms[$cmid]; } @@ -607,24 +616,11 @@ class course_modinfo { } // Loop through each piece of module data, constructing it - static $modexists = array(); foreach ($coursemodinfo->modinfo as $mod) { - if (!isset($mod->name) || strval($mod->name) === '') { - // something is wrong here - continue; - } - - // Skip modules which don't exist - if (!array_key_exists($mod->mod, $modexists)) { - $modexists[$mod->mod] = file_exists("$CFG->dirroot/mod/$mod->mod/lib.php"); - } - if (!$modexists[$mod->mod]) { + $cm = $this->load_cm_info_from_data($mod); + if (!$cm) { continue; } - - // Construct info for this module - $cm = new cm_info($this, null, $mod, null); - // Store module in instances and cms array if (!isset($this->instances[$cm->modname])) { $this->instances[$cm->modname] = array(); @@ -639,6 +635,14 @@ class course_modinfo { $this->sectionmodules[$cm->sectionnum][] = $cm->id; } + // Load disabled modules info. + foreach ($coursemodinfo->disabledmodinfo as $mod) { + $cminfo = $this->load_cm_info_from_data($mod); + if ($cminfo) { + $this->disabledcms[$cminfo->id] = $cminfo; + } + } + // Expand section objects $this->sectioninfobynum = []; $this->sectioninfobyid = []; @@ -780,6 +784,7 @@ class course_modinfo { // Retrieve all information about activities and sections. $coursemodinfo = new stdClass(); $coursemodinfo->modinfo = self::get_array_of_activities($course, $partialrebuild); + $coursemodinfo->disabledmodinfo = self::inner_get_array_of_activities($course, $partialrebuild, true); $coursemodinfo->sectioncache = self::build_course_section_cache($course, $partialrebuild); foreach (self::$cachedfields as $key) { $coursemodinfo->$key = $course->$key; @@ -925,13 +930,40 @@ class course_modinfo { * @return array list of activities */ public static function get_array_of_activities(stdClass $course, bool $usecache = false): array { - global $CFG, $DB; + return self::inner_get_array_of_activities($course, $usecache); + } + + /** + * Internal implementation of get_array_of_activities. + * + * This one is not public facing because we also deal with possible non visible modules. + * + * @param stdClass $course + * @param bool $usecache + * @param bool $disabledonly + * @return array + * @throws dml_exception + * @throws moodle_exception + */ + private static function inner_get_array_of_activities(stdClass $course, bool $usecache, bool $disabledonly = false): array { + global $DB, $CFG; if (empty($course)) { throw new moodle_exception('courseidnotfound'); } - $rawmods = get_course_mods($course->id); + if ($disabledonly) { + global $DB; + $rawmods = $DB->get_records_sql( + "SELECT cm.*, m.name as modname + FROM {modules} m, {course_modules} cm + WHERE cm.course = ? AND cm.module = m.id AND m.visible = 0", + [$course->id] + ); + } else { + $rawmods = get_course_mods($course->id); + } + if (empty($rawmods)) { return []; } @@ -942,7 +974,12 @@ class course_modinfo { $cachecoursemodinfo = cache::make('core', 'coursemodinfo'); $coursemodinfo = $cachecoursemodinfo->get_versioned($course->id, $course->cacherev); if ($coursemodinfo !== false) { - $mods = $coursemodinfo->modinfo; + if ($disabledonly) { + $mods = $coursemodinfo->disabledmodinfo ?? []; // If we have updated and not rebuilt the cache, this could lead + // to a warning if not checked. + } else { + $mods = $coursemodinfo->modinfo; + } } } @@ -1114,6 +1151,31 @@ class course_modinfo { // Because this is a versioned cache, there is no need to actually delete the cache item, // only increase the required version number. } + + /** + * Load the course module information from the coursemodinfo object. + * + * @param stdClass $mod + * @return void + */ + private function load_cm_info_from_data(stdClass $mod): ?cm_info { + global $CFG; + $modexistcache = cache::make_from_params(cache_store::MODE_REQUEST, 'core', 'coursemodinfoexists'); + // If the name is empty, something is wrong here, so exit early. + if (!isset($mod->name) || strval($mod->name) === '') { + return null; + } + // Register modules that don't exist (never or not anymore). + if (!$modexistcache->has($mod->mod)) { + $modexistcache->set($mod->mod, file_exists("$CFG->dirroot/mod/$mod->mod/lib.php")); + } + // Still nothing, so exit early. + if (!$modexistcache->get($mod->mod)) { + return null; + } + // Construct info for this module. + return new cm_info($this, null, $mod, null); + } } diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index a04c9e1faa8..d204fb706c6 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -3736,6 +3736,86 @@ EOF; $this->assertFalse(note_load($note->id)); } + /** + * Test remove_course_content delete plugins correctly + * + * @param string $activitytype + * @return void + * @dataProvider activity_type_provider + * @covers \remove_course_contents + */ + public function test_remove_course_contents_for_activity(string $activitytype): void { + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $user = $this->getDataGenerator()->create_and_enrol($course, 'teacher'); + $this->setUser($user); + $activity = $this->getDataGenerator()->create_module($activitytype, ['course' => $course]); + remove_course_contents($course->id, false); + $this->assertFalse(get_coursemodule_from_id($activitytype, $activity->cmid)); + } + + /** + * Test remove_course_content when an activity type has been disabled + * + * @param string $activitytype + * @return void + * @dataProvider activity_type_provider + * @covers \remove_course_contents + */ + public function test_remove_course_contents_for_disabled_activity(string $activitytype): void { + global $DB; + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $user = $this->getDataGenerator()->create_and_enrol($course, 'teacher'); + $this->setUser($user); + $activity = $this->getDataGenerator()->create_module($activitytype, ['course' => $course]); + $pluginman = \core_plugin_manager::instance(); + $plugininfo = $pluginman->get_plugin_info($activitytype); + $plugininfo::enable_plugin($activitytype, false); + remove_course_contents($course->id, false); + $this->assertFalse(get_coursemodule_from_id($activitytype, $activity->cmid)); + $this->assertFalse($DB->record_exists($activitytype, ['id' => $activity->id])); + } + + /** + * List all activities types + * + * @return array + */ + public static function activity_type_provider(): array { + $modules = [ + 'assign', + 'bigbluebuttonbn', + 'book', + 'chat', + 'choice', + 'data', + 'feedback', + 'folder', + 'forum', + 'glossary', + 'imscp', + 'label', + 'lesson', + 'lti', + 'page', + 'quiz', + 'resource', + 'scorm', + 'survey', + 'url', + 'wiki', + 'workshop', + ]; + return array_combine($modules, + array_map(function($modname) { + return [$modname]; + }, $modules) + ); + } + /** * Test function username_load_fields_from_object(). */ -- 2.25.1 From 83d660017a64663c5aed3ae747edd9794ff303b2 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Mon, 29 Apr 2024 15:05:28 +0200 Subject: [PATCH 2/3] MDL-77924 mod_bigbluebuttonbn: Fix phpunit when mock disabled * When mock server is not enabled in config.php we should not try to call any remote server via the API. --- mod/bigbluebuttonbn/classes/local/proxy/proxy_base.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/bigbluebuttonbn/classes/local/proxy/proxy_base.php b/mod/bigbluebuttonbn/classes/local/proxy/proxy_base.php index ae21aa13f41..8abc180d6c6 100644 --- a/mod/bigbluebuttonbn/classes/local/proxy/proxy_base.php +++ b/mod/bigbluebuttonbn/classes/local/proxy/proxy_base.php @@ -184,7 +184,7 @@ abstract class proxy_base { array $metadata = [], ?int $instanceid = null ) { - if (PHPUNIT_TEST && !defined('TEST_MOD_BIGBLUEBUTTONBN_MOCK_SERVER')) { + if ((PHPUNIT_TEST || defined('BEHAT_SITE_RUNNING')) && !defined('TEST_MOD_BIGBLUEBUTTONBN_MOCK_SERVER')) { return true; // In case we still use fetch and mock server is not defined, this prevents // an error. This can happen if a function from lib.php is called in test from other modules // for example. -- 2.25.1 From be0351568292d112cf3369823df04d6ca5644f9e Mon Sep 17 00:00:00 2001 From: Laurent David Date: Mon, 29 Apr 2024 14:57:51 +0200 Subject: [PATCH 3/3] MDL-77924 mod_quiz: Fix quiz_delete_instance when disabled * Quiz delete instance assumes that the quiz module is not disabled. We should aim to use quiz_settings::create_for_cmid instead of quiz:create to avoid issue with module retrieval. --- mod/quiz/lib.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index db6b8294e5d..bed691d6c00 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -193,7 +193,8 @@ function quiz_delete_instance($id) { quiz_delete_all_attempts($quiz); // Delete all overrides, and for performance do not log or check permissions. - $quizobj = quiz_settings::create($quiz->id); + $cm = get_coursemodule_from_instance('quiz', $quiz->id); + $quizobj = quiz_settings::create_for_cmid($cm->id); $quizobj->get_override_manager()->delete_all_overrides(shouldlog: false); quiz_delete_references($quiz->id); -- 2.25.1