diff --git a/.upgradenotes/MDL-86021-2025090910325560.yml b/.upgradenotes/MDL-86021-2025090910325560.yml new file mode 100644 index 00000000000..95a55d46a5d --- /dev/null +++ b/.upgradenotes/MDL-86021-2025090910325560.yml @@ -0,0 +1,10 @@ +issueNumber: MDL-86021 +notes: + core_courseformat: + - message: >- + Add a new modinfo::get_instance_of() to retrieve an instance of a cm via + its name and instance id. Add a new modinfo::sort_cm_array() to sort an + array of cms in their order of appearance in the course page. Replaces + calls to get_course_and_cm_from_instance() and get_instances_of() + whenever it was just used to retrieve a single instance of a cm. + type: improved diff --git a/public/course/classes/modinfo.php b/public/course/classes/modinfo.php index 11912e56450..adb3b4921ee 100644 --- a/public/course/classes/modinfo.php +++ b/public/course/classes/modinfo.php @@ -101,6 +101,13 @@ class modinfo { */ private ?array $delegatedbycm = null; + /** + * Contains the course content weights so they can be sorted accordingly. + * + * @var array|null + */ + private ?array $weights = null; + /** * User ID * @var int @@ -292,6 +299,40 @@ class modinfo { return $this->instances[$modname]; } + /** + * Obtains a single instance of a particular module on this course. + * + * @param string $modname Name of module (not full frankenstyle) e.g. 'label' + * @param int $instanceid Instance id + * @param int $strictness Use IGNORE_MISSING to return null if not found, or MUST_EXIST to throw exception + * @return cm_info|null cm_info for the instance on this course or null if not found + * @throws moodle_exception If the instance is not found + */ + public function get_instance_of(string $modname, int $instanceid, int $strictness = IGNORE_MISSING): ?cm_info { + if (empty($this->instances[$modname]) || empty($this->instances[$modname][$instanceid])) { + if ($strictness === IGNORE_MISSING) { + return null; + } + throw new moodle_exception('invalidmoduleid', 'error', '', $instanceid); + } + return $this->instances[$modname][$instanceid]; + } + + /** + * Sorts the given array of course modules according to the order they appear on the course page. + * + * @param cm_info[] $cms Array of cm_info objects to sort by reference + * @return void + */ + public function sort_cm_array(array &$cms): void { + $weights = $this->get_content_weights(); + uasort($cms, function ($a, $b) use ($weights) { + $weighta = $weights['cm' . $a->id] ?? PHP_INT_MAX; + $weightb = $weights['cm' . $b->id] ?? PHP_INT_MAX; + return $weighta <=> $weightb; + }); + } + /** * Groups that the current user belongs to organised by grouping id. Calculated on the first request. * @return int[][] array of grouping id => array of group id => group id. Includes grouping id 0 for 'all groups' @@ -1193,6 +1234,58 @@ class modinfo { public static function is_mod_type_visible_on_course(string $modname): bool { return plugin_supports('mod', $modname, FEATURE_CAN_DISPLAY, true); } + + /** + * Get content weights for all sections and modules in the course. + * + * The weights are calculated based on the order of sections and modules + * as they appear on the course page, including delegated sections. + * + * @return array Associative array with keys 'section{sectionid}' and 'cm{cmid}' and integer weights as values. + */ + private function get_content_weights(): array { + if ($this->weights !== null) { + return $this->weights; + } + $result = []; + foreach ($this->sectioninfobynum as $section) { + // Delegated sections are always at the end of the course and they will + // be added only if they are part of any section sequence. + if ($section->is_delegated()) { + continue; + } + $sortedelements = $this->calculate_section_weights($section, count($result)); + $result += $sortedelements; + } + $this->weights = $result; + return $result; + } + + /** + * Calculate weights for a section and its modules, including delegated sections. + * + * @param section_info $section The section to calculate weights for. + * @param int $currentweight The starting weight to use for this section. + * @return section_info[] Associative array of section_info objects, indexed by the cmid of the delegating module. + */ + private function calculate_section_weights(section_info $section, int $currentweight = 0): array { + $delegatedcms = $this->get_sections_delegated_by_cm(); + + $weights = [ + 'section' . $section->id => $currentweight++, + ]; + + foreach ($section->get_sequence_cm_infos() as $cm) { + $weights['cm' . $cm->id] = $currentweight++; + + if (array_key_exists($cm->id, $delegatedcms)) { + $subweights = $this->calculate_section_weights($delegatedcms[$cm->id], $currentweight); + $weights += $subweights; + $currentweight += count($subweights); + } + } + return $weights; + } } // Alias this class to the old name. diff --git a/public/course/format/classes/output/local/overview/overviewtable.php b/public/course/format/classes/output/local/overview/overviewtable.php index bccd9f8ea8b..f9f23100c07 100644 --- a/public/course/format/classes/output/local/overview/overviewtable.php +++ b/public/course/format/classes/output/local/overview/overviewtable.php @@ -160,9 +160,12 @@ class overviewtable implements externable, named_templatable, renderable { private function get_related_course_modules(): array { $modinfo = get_fast_modinfo($this->course->id); if ($this->modname == 'resource') { - return $this->get_all_resource_intances($modinfo); + $result = $this->get_all_resource_intances($modinfo); + } else { + $result = $modinfo->get_instances_of($this->modname); } - return $modinfo->get_instances_of($this->modname); + $modinfo->sort_cm_array($result); + return $result; } /** diff --git a/public/course/format/classes/sectiondelegatemodule.php b/public/course/format/classes/sectiondelegatemodule.php index 18bae324e42..73e1e7be544 100644 --- a/public/course/format/classes/sectiondelegatemodule.php +++ b/public/course/format/classes/sectiondelegatemodule.php @@ -50,14 +50,15 @@ abstract class sectiondelegatemodule extends sectiondelegate { protected section_info $sectioninfo ) { parent::__construct($sectioninfo); - + /** @var \core_course\modinfo $modinfo */ + $modinfo = $sectioninfo->modinfo; try { // Disabled or missing plugins can throw exceptions. - [$this->course, $this->cm] = get_course_and_cm_from_instance( - $this->sectioninfo->itemid, + $this->cm = $modinfo->get_instance_of( $this->get_module_name(), - $this->sectioninfo->course, + $this->sectioninfo->itemid, ); + $this->course = $modinfo->get_course(); } catch (\Exception $e) { $this->cm = null; $this->course = null; diff --git a/public/course/tests/modinfo_test.php b/public/course/tests/modinfo_test.php index 3977e077ada..e700e99d4d6 100644 --- a/public/course/tests/modinfo_test.php +++ b/public/course/tests/modinfo_test.php @@ -1225,4 +1225,87 @@ final class modinfo_test extends \advanced_testcase { // Sections delegated by a block shouldn't be returned. $this->assertCount(1, $delegatedsections); } + + + /** + * Test for sort_cm_array method. + */ + public function test_sort_cm_array(): void { + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + // Create a course with 4 sections. + $course = $generator->create_course(['numsections' => 3]); + $generator->create_module('page', ['name' => 'Page s1', 'course' => $course->id, 'section' => 0]); + $generator->create_module('page', ['name' => 'Page s2', 'course' => $course->id, 'section' => 1]); + $generator->create_module('assign', ['name' => 'Assign s3', 'course' => $course->id, 'section' => 2]); + $generator->create_module('page', ['name' => 'Page s3', 'course' => $course->id, 'section' => 3]); + // Check we return all cms in order. + $cms = get_fast_modinfo($course)->get_instances_of('page'); + get_fast_modinfo($course)->sort_cm_array($cms); + $this->assertCount(3, $cms); + $this->assertEquals(['Page s1', 'Page s2', 'Page s3'], array_column($cms, 'name')); + + // Generate some delegated sections (not listed). + $module = $this->getDataGenerator()->create_module('subsection', (object) ['course' => $course->id, 'section' => 1]); + $sub1 = get_fast_modinfo($course)->get_section_info_by_component('mod_subsection', $module->id); + $generator->create_module('page', ['name' => 'Page sub1', 'course' => $course->id, 'section' => $sub1->sectionnum]); + $generator->create_module('page', ['name' => 'Page sub2', 'course' => $course->id, 'section' => $sub1->sectionnum]); + $generator->create_module('assign', ['name' => 'Assign sub1', 'course' => $course->id, 'section' => $sub1->sectionnum]); + + $cms = get_fast_modinfo($course)->get_instances_of('page'); + get_fast_modinfo($course)->sort_cm_array($cms); + $this->assertCount(5, $cms); + $this->assertEquals(['Page s1', 'Page s2', 'Page sub1', 'Page sub2', 'Page s3'], array_column($cms, 'name')); + + $cms = get_fast_modinfo($course)->get_instances_of('assign'); + get_fast_modinfo($course)->sort_cm_array($cms); + $this->assertCount(2, $cms); + $this->assertEquals(['Assign sub1', 'Assign s3'], array_column($cms, 'name')); + } + + /** + * Test for get_instance_of method. + */ + public function test_get_instance_of(): void { + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + // Create a course with 4 sections. + $course = $generator->create_course(['numsections' => 3]); + $generator->create_module('page', ['name' => 'Page s1', 'course' => $course->id, 'section' => 0]); + $generator->create_module('page', ['name' => 'Page s2', 'course' => $course->id, 'section' => 1]); + $generator->create_module('assign', ['name' => 'Assign s3', 'course' => $course->id, 'section' => 2]); + + $modinfo = get_fast_modinfo($course); + $pagecms = array_values($modinfo->get_instances_of('page')); + $assigncms = array_values($modinfo->get_instances_of('assign')); + $this->assertCount(2, $pagecms); + $this->assertCount(1, $assigncms); + + $this->assertEquals('Page s1', $modinfo->get_instance_of('page', $pagecms[0]->instance)->name); + $this->assertEquals('Page s2', $modinfo->get_instance_of('page', $pagecms[1]->instance)->name); + $this->assertEquals('Assign s3', $modinfo->get_instance_of('assign', $assigncms[0]->instance)->name); + + $this->assertNull($modinfo->get_instance_of('page', 99999)); + $this->assertNull($modinfo->get_instance_of('assign', 99999)); + $this->assertNull($modinfo->get_instance_of('nonexisting', 99999)); + } + + /** + * Test for get_instance_of method when asking for a non existing module with MUST_EXIST. + */ + public function test_get_instance_of_exception(): void { + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + // Create a course with 4 sections. + $course = $generator->create_course(['numsections' => 3]); + $generator->create_module('page', ['name' => 'Page s1', 'course' => $course->id, 'section' => 0]); + $generator->create_module('page', ['name' => 'Page s2', 'course' => $course->id, 'section' => 1]); + $generator->create_module('assign', ['name' => 'Assign s3', 'course' => $course->id, 'section' => 2]); + + $modinfo = get_fast_modinfo($course); + + $this->expectException(moodle_exception::class); + $this->expectExceptionMessage('Invalid module ID: 99999'); + $modinfo->get_instance_of('page', 99999, MUST_EXIST); + } } diff --git a/public/lib/modinfolib.php b/public/lib/modinfolib.php index f08f2d72e6a..1465b68d42b 100644 --- a/public/lib/modinfolib.php +++ b/public/lib/modinfolib.php @@ -230,11 +230,8 @@ function get_course_and_cm_from_instance($instanceorid, $modulename, $courseorid // Get cm from get_fast_modinfo. $modinfo = get_fast_modinfo($course, $userid); - $instances = $modinfo->get_instances_of($modulename); - if (!array_key_exists($instanceid, $instances)) { - throw new moodle_exception('invalidmoduleid', 'error', '', $instanceid); - } - return [$course, $instances[$instanceid]]; + $instance = $modinfo->get_instance_of($modulename, $instanceid, MUST_EXIST); + return [$course, $instance]; } diff --git a/public/mod/subsection/tests/behat/subsection_overview_report.feature b/public/mod/subsection/tests/behat/subsection_overview_report.feature new file mode 100644 index 00000000000..22cb8f1cc21 --- /dev/null +++ b/public/mod/subsection/tests/behat/subsection_overview_report.feature @@ -0,0 +1,28 @@ +@mod @mod_subsection +Feature: The course overview report should show activities in order within subsections + In order to have a better overview of the activities in a course + As a teacher + I want to see the activities in subsections in the same order as they appear on the course page + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + And the following "courses" exist: + | fullname | shortname | category | numsections | initsections | + | Course 1 | C1 | 0 | 2 | 1 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + And the following "activities" exist: + | activity | name | course | idnumber | section | + | assign | Assignment 1 | C1 | assignment1 | 1 | + | subsection | Subsection 1 | C1 | subsection1 | 1 | + | assign | Assignment in subsection | C1 | assignment3 | 3 | + | assign | Assignment 3 | C1 | assignment3 | 1 | + And I log in as "teacher1" + + Scenario: Activities in subsections appear in the course activities report in the same order of the course page. + When I am on the "Course 1" "course > activities > assign" page logged in as "teacher1" + Then "Assignment in subsection" "text" should appear after "Assignment 1" "text" in the "Table listing all Assignment activities" table + And "Assignment in subsection" "text" should appear before "Assignment 3" "text" in the "Table listing all Assignment activities" table