diff --git a/public/course/format/classes/stateactions.php b/public/course/format/classes/stateactions.php index 65acef08f0a..e350cb9536f 100644 --- a/public/course/format/classes/stateactions.php +++ b/public/course/format/classes/stateactions.php @@ -63,8 +63,8 @@ class stateactions { } $this->validate_cms($course, $ids, __FUNCTION__, ['moodle/course:manageactivities']); - // The moveto_module function move elements before a specific target. - // To keep the order the movements must be done in descending order (last activity first). + // To keep the order the movements must be done in descending order (last activity first), so we add them "before" + // in the reverse order (ending up in the original ascending order). $ids = $this->sort_cm_ids_by_course_position($course, $ids, true); // Target cm has more priority than target section. @@ -79,19 +79,24 @@ class stateactions { // The origin sections must be updated as well. $originalsections = []; + $action = formatactions::cm($course); $beforecmdid = $targetcmid; + $modinfo = get_fast_modinfo($course); + $targetsection = $modinfo->get_section_info_by_id($targetsectionid, MUST_EXIST); foreach ($ids as $cmid) { // An updated $modinfo is needed on every loop as activities list change. $modinfo = get_fast_modinfo($course); $cm = $modinfo->get_cm($cmid); $currentsectionid = $cm->section; - $targetsection = $modinfo->get_section_info_by_id($targetsectionid, MUST_EXIST); if ($targetsection->is_delegated() && $cm->get_delegated_section_info()) { throw new moodle_exception('subsectionmoveerror', 'core'); } - $beforecm = (!empty($beforecmdid)) ? $modinfo->get_cm($beforecmdid) : null; - if ($beforecm === null || $beforecm->id != $cmid) { - moveto_module($cm, $targetsection, $beforecm); + if ($beforecmdid != $cmid) { + if ($beforecmdid) { + $action->move_before($cm->id, $beforecmdid); + } else { + $action->move_end_section($cm->id, $targetsection->id); + } } $beforecmdid = $cm->id; $updates->add_cm_put($cm->id); diff --git a/public/course/format/tests/stateactions_test.php b/public/course/format/tests/stateactions_test.php index 5f72ecd0684..c11ae32551a 100644 --- a/public/course/format/tests/stateactions_test.php +++ b/public/course/format/tests/stateactions_test.php @@ -1439,35 +1439,13 @@ final class stateactions_test extends \advanced_testcase { #[\PHPUnit\Framework\Attributes\DataProvider('cm_move_provider')] public function test_cm_move( array $cmtomove, - string $targetsection, array $expectedcoursetree, + ?string $targetsection = null, + ?string $targetmodule = null, ?string $expectedexception = null ): void { $this->resetAfterTest(); - $course = $this->create_course('topics', 4, []); - - $subsection1 = $this->getDataGenerator()->create_module( - 'subsection', ['course' => $course, 'section' => 1, 'name' => 'subsection1'] - ); - $subsection2 = $this->getDataGenerator()->create_module( - 'subsection', ['course' => $course, 'section' => 1, 'name' => 'subsection2'] - ); - $modinfo = get_fast_modinfo($course); - $subsection1info = $modinfo->get_section_info_by_component('mod_subsection', $subsection1->id); - $subsection2info = $modinfo->get_section_info_by_component('mod_subsection', $subsection2->id); - - $references = $this->course_references($course); - // Add some activities to the course. One visible and one hidden in both sections 1 and 2. - $references["cm0"] = $this->create_activity($course->id, 'assign', 0); - $references["cm1"] = $this->create_activity($course->id, 'page', 2); - $references["cm2"] = $this->create_activity($course->id, 'forum', $subsection1info->sectionnum); - $references["subsection1"] = intval($subsection1->cmid); - $references["subsection2"] = intval($subsection2->cmid); - $references["subsection1sectionid"] = $subsection1info->id; - $references["subsection2sectionid"] = $subsection2info->id; - $user = $this->getDataGenerator()->create_user(); - $this->getDataGenerator()->enrol_user($user->id, $course->id, 'editingteacher'); - $this->setUser($user); + [$course, $references] = $this->setup_move_to_course_and_modules(); // Initialise stateupdates. $courseformat = course_get_format($course->id); @@ -1480,14 +1458,15 @@ final class stateactions_test extends \advanced_testcase { $actions = new stateactions(); // We do this to make sure we can reference subsection1 in the course tree (and not just section5 as subsection1 is // both a module and a subsection). - if (str_starts_with($targetsection, 'subsection')) { + if ($targetsection && str_starts_with($targetsection, 'subsection')) { $targetsection = $targetsection . 'sectionid'; } $actions->cm_move( $updates, $course, $this->translate_references($references, $cmtomove), - $references[$targetsection] + $targetsection ? $references[$targetsection] : null, + $targetmodule ? $references[$targetmodule] : null ); $coursetree = $this->get_course_tree($course, $references); @@ -1542,7 +1521,53 @@ final class stateactions_test extends \advanced_testcase { } /** - * Provider for test_section_move. + * Setup a course with subsections and modules to test move to section and move to module. + * + * The original coursetree looks like this: + * 'coursetree' => [ + * 'section0' => ['cm0'], + * 'section1' => ['subsection1' => ['cm2'],'subsection2' => []], + * 'section2' => ['cm1'], + * 'section3' => ['cm4', 'cm5'], + * 'section4' => [], + * ], + * + * @return array containing the course and the references + */ + private function setup_move_to_course_and_modules(): array { + $course = $this->create_course('topics', 4, []); + + $subsection1 = $this->getDataGenerator()->create_module( + 'subsection', + ['course' => $course, 'section' => 1, 'name' => 'subsection1'] + ); + $subsection2 = $this->getDataGenerator()->create_module( + 'subsection', + ['course' => $course, 'section' => 1, 'name' => 'subsection2'] + ); + $modinfo = get_fast_modinfo($course); + $subsection1info = $modinfo->get_section_info_by_component('mod_subsection', $subsection1->id); + $subsection2info = $modinfo->get_section_info_by_component('mod_subsection', $subsection2->id); + + $references = $this->course_references($course); + // Add some activities to the course. One visible and one hidden in both sections 1 and 2. + $references["cm0"] = $this->create_activity($course->id, 'assign', 0); + $references["cm1"] = $this->create_activity($course->id, 'page', 2); + $references["cm2"] = $this->create_activity($course->id, 'forum', $subsection1info->sectionnum); + $references["cm3"] = $this->create_activity($course->id, 'page', 3); + $references["cm4"] = $this->create_activity($course->id, 'page', 3); + $references["subsection1"] = intval($subsection1->cmid); + $references["subsection2"] = intval($subsection2->cmid); + $references["subsection1sectionid"] = $subsection1info->id; + $references["subsection2sectionid"] = $subsection2info->id; + $user = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($user->id, $course->id, 'editingteacher'); + $this->setUser($user); + return [$course, $references]; + } + + /** + * Provider for test_cm_move_to_section. * * * The original coursetree looks like this: @@ -1550,7 +1575,7 @@ final class stateactions_test extends \advanced_testcase { * 'section0' => ['cm0'], * 'section1' => ['subsection1' => ['cm2'],'subsection2' => []], * 'section2' => ['cm1'], - * 'section3' => [], + * 'section3' => ['cm3', 'cm4'], * 'section4' => [], * ], * @@ -1564,7 +1589,7 @@ final class stateactions_test extends \advanced_testcase { 'section0' => [], 'section1' => ['subsection1' => ['cm2'], 'subsection2' => []], 'section2' => ['cm1', 'cm0'], - 'section3' => [], + 'section3' => ['cm3', 'cm4'], 'section4' => [], ], ]; @@ -1581,7 +1606,63 @@ final class stateactions_test extends \advanced_testcase { 'section0' => ['cm0'], 'section1' => ['subsection1' => ['cm2', 'cm1'], 'subsection2' => []], 'section2' => [], - 'section3' => [], + 'section3' => ['cm3', 'cm4'], + 'section4' => [], + ], + ]; + yield 'Move module into the same section' => [ + 'cmtomove' => ['cm3'], + 'targetsection' => 'section3', + 'expectedcoursetree' => [ + 'section0' => ['cm0'], + 'section1' => ['subsection1' => ['cm2'], 'subsection2' => []], + 'section2' => ['cm1'], + 'section3' => ['cm4', 'cm3'], + 'section4' => [], + ], + ]; + yield + 'Move module after cm1' => [ + 'cmtomove' => ['cm0'], + 'targetmodule' => 'cm1', + 'expectedcoursetree' => [ + 'section0' => [], + 'section1' => ['subsection1' => ['cm2'], 'subsection2' => []], + 'section2' => ['cm0', 'cm1'], + 'section3' => ['cm3', 'cm4'], + 'section4' => [], + ], + ]; + yield 'Move module into a module that is in a subsection' => [ + 'cmtomove' => ['cm1'], + 'targetmodule' => 'cm2', + 'expectedcoursetree' => [ + 'section0' => ['cm0'], + 'section1' => ['subsection1' => ['cm1', 'cm2'], 'subsection2' => []], + 'section2' => [], + 'section3' => ['cm3', 'cm4'], + 'section4' => [], + ], + ]; + yield 'Move module before module in the same section (order kept as was already before)' => [ + 'cmtomove' => ['cm3'], + 'targetmodule' => 'cm4', + 'expectedcoursetree' => [ + 'section0' => ['cm0'], + 'section1' => ['subsection1' => ['cm2'], 'subsection2' => []], + 'section2' => ['cm1'], + 'section3' => ['cm3', 'cm4'], + 'section4' => [], + ], + ]; + yield 'Move module before module in the same section' => [ + 'cmtomove' => ['cm4'], + 'targetmodule' => 'cm3', + 'expectedcoursetree' => [ + 'section0' => ['cm0'], + 'section1' => ['subsection1' => ['cm2'], 'subsection2' => []], + 'section2' => ['cm1'], + 'section3' => ['cm4', 'cm3'], 'section4' => [], ], ];