Fix section checking

nathan-catalystit · nathan-catalystit · commit a438ea07e5fc · 2025-07-15T20:49:06.000+10:00
diff --git a/classes/actions.php b/classes/actions.php
@@ -270,21 +270,21 @@ public static function duplicate_to_course(array $modules, int $targetcourseid,
         $sourcemodinfo = get_fast_modinfo($sourcecourseid);
         $targetmodinfo = get_fast_modinfo($targetcourseid);
         $targetformat = course_get_format($targetmodinfo->get_course());
-        $targetsectionnum = $targetformat->get_last_section_number();
+        $lastsectionnum = $targetformat->get_last_section_number();
 
         $filtersectionshook = new filter_sections_different_course($targetcourseid,
                 array_keys($targetmodinfo->get_section_info_all()));
         \core\di::get(\core\hook\manager::class)->dispatch($filtersectionshook);
         $filteredsections = $filtersectionshook->get_sectionnums();
 
-        if ($targetsectionnum == -1 && !$filtersectionshook->is_originsectionkept()) {
+        if ($sectionnum == -1 && !$filtersectionshook->is_originsectionkept()) {
             // The course modules should be in the same section number as in the original course. However, the hook listener(s)
             // disabled this option, so we cancel the operation.
             // This is only a security measure and should not happen unless someone manipulates the UI.
             return;
         }
 
-        if (!in_array($targetsectionnum, $filteredsections)) {
+        if (($sectionnum >= 0) && ($sectionnum <= $lastsectionnum) && !in_array($sectionnum, $filteredsections)) {
             // The target section number has been filtered by a hook callback, thus must not be used.
             // This is only a security measure and should not happen unless someone manipulates the UI.
             return;
@@ -293,16 +293,16 @@ public static function duplicate_to_course(array $modules, int $targetcourseid,
         $canaddsection = has_capability('moodle/course:update', context_course::instance($targetcourseid))
             && $filtersectionshook->is_makesectionallowed();
 
-        // If a new section (that means that $sectionnum of the user is higher than $targetsectionnum), we create one.
-        if ($sectionnum > $targetsectionnum) {
+        // If a new section (that means that $sectionnum of the user is higher than $lastsectionnum), we create one.
+        if ($sectionnum > $lastsectionnum) {
             // No permissions to add section.
             if (!$canaddsection) {
                 return;
             }
 
             $targetformatopt = $targetformat->get_format_options();
             // No course format setting or no orphaned sections exist.
-            if (!isset($targetformatopt['numsections']) || !($targetformatopt['numsections'] < $targetsectionnum)) {
+            if (!isset($targetformatopt['numsections']) || !($targetformatopt['numsections'] < $lastsectionnum)) {
                 course_create_section($targetcourseid);
             }
 
@@ -312,7 +312,7 @@ public static function duplicate_to_course(array $modules, int $targetcourseid,
             }
 
             // Make sure new sectionnum is set accurately.
-            $sectionnum = $targetsectionnum + 1;
+            $sectionnum = $lastsectionnum + 1;
         }
 
         if ($sectionnum == -1) {
@@ -323,7 +323,7 @@ public static function duplicate_to_course(array $modules, int $targetcourseid,
             }, $modules));
 
             // If target course needs sections added but user does not have permission.
-            if ($srcmaxsectionnum > $targetsectionnum && !$canaddsection) {
+            if ($srcmaxsectionnum > $lastsectionnum && !$canaddsection) {
                 return; // No permission to add section.
             }
 
diff --git a/tests/fixtures/filter_sections_different_course_callbacks.php b/tests/fixtures/filter_sections_different_course_callbacks.php
@@ -0,0 +1,49 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace block_massaction;
+
+use block_massaction\hook\filter_sections_different_course;
+
+/**
+ * block_massaction phpunit tests fixture.
+ * Callbacks for the filter_sections_different_course hook.
+ *
+ * @package    block_massaction
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+final class filter_sections_different_course_callbacks {
+    /**
+     * Callback for the filter_sections_different_course hook.
+     *
+     * @param filter_sections_different_course $hook
+     * @return void
+     */
+    public static function filter_sections_different_course(filter_sections_different_course $hook): void {
+        foreach ($hook->get_sectionnums() as $sectionnum) {
+            // Restrict section 3 onward.
+            if ($sectionnum >= 3) {
+                $hook->remove_sectionnum($sectionnum);
+            }
+        }
+
+        // Disable the options to keep the original section.
+        $hook->disable_originsectionkept();
+
+        // Disable the option to create a new section.
+        $hook->disable_makesection(true);
+    }
+}
diff --git a/tests/fixtures/filter_sections_different_course_hooks.php b/tests/fixtures/filter_sections_different_course_hooks.php
@@ -0,0 +1,37 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace block_massaction;
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * block_massaction phpunit tests fixture.
+ * Hook register.
+ *
+ * @package    block_massaction
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+$callbacks = [
+    [
+        'hook' => \block_massaction\hook\filter_sections_different_course::class,
+        'callback' => [
+            \block_massaction\filter_sections_different_course_callbacks::class,
+            'filter_sections_different_course',
+        ],
+    ],
+];
diff --git a/tests/massaction_test.php b/tests/massaction_test.php
@@ -21,6 +21,7 @@
 use base_setting_exception;
 use block_massaction;
 use coding_exception;
+use core\di;
 use core\event\course_module_updated;
 use core\task\manager;
 use dml_exception;
@@ -860,4 +861,101 @@ private function shuffle_modules(): void {
         moveto_module(get_fast_modinfo($this->course->id)->get_cm(get_fast_modinfo($this->course->id)->get_sections()[3][3]),
             get_fast_modinfo($this->course->id)->get_section_info(3));
     }
+
+    /**
+     * Tests the duplication of modules to a course with filter_sections hook.
+     *
+     * @covers \block_massaction\actions::duplicate_to_course
+     * @return void
+     */
+    public function test_duplicate_to_course_with_filter_sections_different_course_hook(): void {
+        $this->resetAfterTest();
+
+        // Load the callback classes.
+        require_once(__DIR__ . '/fixtures/filter_sections_different_course_callbacks.php');
+
+        // Replace the version of the manager in the DI container with a phpunit one.
+        di::set(
+            \core\hook\manager::class,
+            \core\hook\manager::phpunit_get_instance([
+                'test_plugin1' => __DIR__ . '/fixtures/filter_sections_different_course_hooks.php',
+            ]),
+        );
+
+        // Source course.
+        $sourcecourseid = $this->course->id;
+        $sourcecoursemodinfo = get_fast_modinfo($sourcecourseid);
+
+        // Move modules around so that they are not in id order.
+        $this->shuffle_modules();
+
+        // Select some random course modules from different sections to be duplicated.
+        $selectedmoduleids[] = $sourcecoursemodinfo->get_sections()[1][0];
+        $selectedmoduleids[] = $sourcecoursemodinfo->get_sections()[1][1];
+        $selectedmoduleids[] = $sourcecoursemodinfo->get_sections()[3][0];
+        $selectedmoduleids[] = $sourcecoursemodinfo->get_sections()[3][2];
+
+        $selectedmodules = array_filter($this->get_test_course_modules(), function($module) use ($selectedmoduleids) {
+            return in_array($module->id, $selectedmoduleids);
+        });
+
+        // Target course.
+        $targetcourseid = $this->setup_target_course_for_duplicating(3);
+        $targetcoursemodinfo = get_fast_modinfo($targetcourseid);
+        // Four sections (0 1 2 3).
+        $this->assertCount(4, $targetcoursemodinfo->get_section_info_all());
+        // There is no module.
+        $this->assertEmpty($targetcoursemodinfo->get_cms());
+
+        // Test keep origin section.
+        actions::duplicate_to_course($selectedmodules, $targetcourseid, -1);
+        $targetcoursemodinfo = get_fast_modinfo($targetcourseid);
+        // Four sections (0 1 2 3).
+        $this->assertCount(4, $targetcoursemodinfo->get_section_info_all());
+        // There is no module as we cannot keep origin section.
+        $this->assertEmpty($targetcoursemodinfo->get_cms());
+
+        // Test create new section.
+        actions::duplicate_to_course($selectedmodules, $targetcourseid, 4);
+        $targetcoursemodinfo = get_fast_modinfo($targetcourseid);
+        // Still four sections (0 1 2 3).
+        $this->assertCount(4, $targetcoursemodinfo->get_section_info_all());
+        // And still no module.
+        $this->assertEmpty($targetcoursemodinfo->get_cms());
+
+        // Duplicate to section 3, which is restricted by the hook.
+        actions::duplicate_to_course($selectedmodules, $targetcourseid, 3);
+        $targetcoursemodinfo = get_fast_modinfo($targetcourseid);
+        // Still four sections (0 1 2 3).
+        $this->assertCount(4, $targetcoursemodinfo->get_section_info_all());
+        // And still no module.
+        $this->assertEmpty($targetcoursemodinfo->get_cms());
+
+        // Duplicate to section 1, this should work as normal.
+        actions::duplicate_to_course($selectedmodules, $targetcourseid, 1);
+        $targetcoursemodinfo = get_fast_modinfo($targetcourseid);
+        $this->assertCount(4, $targetcoursemodinfo->get_section_info_all());
+        // There should be 4 modules now.
+        $this->assertCount(4, $targetcoursemodinfo->get_cms());
+        // These module ids should be in section 1.
+        $duplicatedmoduleids = $targetcoursemodinfo->get_sections()[1];
+        $this->assertCount(4, $duplicatedmoduleids);
+
+        // Sort name of the modules to be able to compare them.
+        $sourcemodulenames = [];
+        foreach ($selectedmoduleids as $selectedmoduleid) {
+            $sourcemodulenames[] = $sourcecoursemodinfo->get_cm($selectedmoduleid)->name;
+        }
+        sort($sourcemodulenames);
+
+        // Sort name of the duplicated modules to be able to compare them.
+        $duplicatedmodulenames = [];
+        foreach ($duplicatedmoduleids as $moduleid) {
+            $duplicatedmodulenames[] = $targetcoursemodinfo->get_cm($moduleid)->name;
+        }
+        sort($duplicatedmodulenames);
+
+        // The names of the duplicated modules should be the same as the source module names.
+        $this->assertEquals($sourcemodulenames, $duplicatedmodulenames);
+    }
 }
