From 8789141faa79605e66b2dcc8d36964b31e08555e Mon Sep 17 00:00:00 2001 From: Laurent David Date: Fri, 14 Nov 2025 15:14:22 +0100 Subject: [PATCH 1/2] MDL-86862 core_course: Add sectionactions::moveafter --- .upgradenotes/MDL-86862-2025111414153226.yml | 6 + .upgradenotes/MDL-86862-2025111414171623.yml | 5 + .upgradenotes/MDL-86862-2025112710562067.yml | 5 + .upgradenotes/MDL-86862-2025112811453460.yml | 5 + .upgradenotes/MDL-86862-2025120809374181.yml | 5 + .../format/classes/local/sectionactions.php | 174 ++++++++- .../tests/local/sectionactions_test.php | 355 ++++++++++++++++++ public/course/lib.php | 63 +--- public/lib/db/caches.php | 3 + 9 files changed, 576 insertions(+), 45 deletions(-) create mode 100644 .upgradenotes/MDL-86862-2025111414153226.yml create mode 100644 .upgradenotes/MDL-86862-2025111414171623.yml create mode 100644 .upgradenotes/MDL-86862-2025112710562067.yml create mode 100644 .upgradenotes/MDL-86862-2025112811453460.yml create mode 100644 .upgradenotes/MDL-86862-2025120809374181.yml diff --git a/.upgradenotes/MDL-86862-2025111414153226.yml b/.upgradenotes/MDL-86862-2025111414153226.yml new file mode 100644 index 00000000000..23488ef1994 --- /dev/null +++ b/.upgradenotes/MDL-86862-2025111414153226.yml @@ -0,0 +1,6 @@ +issueNumber: MDL-86862 +notes: + core_courseformat: + - message: >- + Add a new core_courseformat\sectionactions::move_after to replace the current move_section_to logic. + type: improved diff --git a/.upgradenotes/MDL-86862-2025111414171623.yml b/.upgradenotes/MDL-86862-2025111414171623.yml new file mode 100644 index 00000000000..2c220ad4af0 --- /dev/null +++ b/.upgradenotes/MDL-86862-2025111414171623.yml @@ -0,0 +1,5 @@ +issueNumber: MDL-86862 +notes: + core_courseformat: + - message: Deprecates the move_section_to logic + type: deprecated diff --git a/.upgradenotes/MDL-86862-2025112710562067.yml b/.upgradenotes/MDL-86862-2025112710562067.yml new file mode 100644 index 00000000000..6e7957abb52 --- /dev/null +++ b/.upgradenotes/MDL-86862-2025112710562067.yml @@ -0,0 +1,5 @@ +issueNumber: MDL-86862 +notes: + core_courseformat: + - message: Deprecates and remove all usages of reorder_sections that is used only internally. + type: deprecated diff --git a/.upgradenotes/MDL-86862-2025112811453460.yml b/.upgradenotes/MDL-86862-2025112811453460.yml new file mode 100644 index 00000000000..9fc0631b4b3 --- /dev/null +++ b/.upgradenotes/MDL-86862-2025112811453460.yml @@ -0,0 +1,5 @@ +issueNumber: MDL-86862 +notes: + core_courseformat: + - message: Add sectionactions::move_at to move a section at a given position. + type: improved diff --git a/.upgradenotes/MDL-86862-2025120809374181.yml b/.upgradenotes/MDL-86862-2025120809374181.yml new file mode 100644 index 00000000000..32db8ee246c --- /dev/null +++ b/.upgradenotes/MDL-86862-2025120809374181.yml @@ -0,0 +1,5 @@ +issueNumber: MDL-86862 +notes: + core_course: + - message: Add a new invalidation event for course action state so we can purge the courseactionsinstances cache when needed. + type: improved diff --git a/public/course/format/classes/local/sectionactions.php b/public/course/format/classes/local/sectionactions.php index a9d4c4a3bf0..f4f89c2aefd 100644 --- a/public/course/format/classes/local/sectionactions.php +++ b/public/course/format/classes/local/sectionactions.php @@ -64,7 +64,10 @@ class sectionactions extends baseactions { // Now move it to the specified position. if ($position > 0 && $position <= $lastsection) { - move_section_to($this->course, $sectionrecord->section, $position, true); + rebuild_course_cache($this->course->id, true); + $modinfo = get_fast_modinfo($this->course); + $sectioninfo = $modinfo->get_section_info_by_id($sectionrecord->id); + $this->move_at($sectioninfo, $position); $sectionrecord->section = $position; } @@ -411,6 +414,172 @@ class sectionactions extends baseactions { return true; } + /** + * Move a course section after another one. + * + * @param section_info $section the section to move + * @param section_info $precedingsectioninfo the section after which to move + * @return bool whether section was moved + */ + public function move_after(section_info $section, section_info $precedingsectioninfo): bool { + $precedingsectionposition = $precedingsectioninfo->sectionnum; + $canmove = $section->id != $precedingsectioninfo->id && + ($section->sectionnum != $precedingsectionposition + 1); + $canmove = $canmove && ($precedingsectioninfo->course == $this->course->id); + if (!$canmove) { + return false; + } + if ($section->sectionnum > $precedingsectioninfo->sectionnum) { + $precedingsectionposition += 1; + } + return $this->move_at($section, $precedingsectionposition); + } + + /** + * Move a course section at a given position. The position will be the target position, i.e. if + * we move section 5 to position 2, section 5 will become section 2, and sections 2, will become + * the third section and so on. + * + * @param section_info $section the section to move + * @param int $targetposition the position to move the section to + * @return bool whether section was moved + */ + public function move_at(section_info $section, int $targetposition): bool { + global $DB; + if (!course_get_format($this->course->id)->uses_sections()) { + return false; + } + if ($section->sectionnum == 0 || $targetposition == 0) { + return false; + } + if ($section->course != $this->course->id) { + return false; + } + if ($section->sectionnum == $targetposition) { + return false; + } + $modinfo = get_fast_modinfo($this->course->id); + $allsections = $modinfo->get_section_info_all(); + if (count($allsections) <= $targetposition) { + return false; + } + $sectionid = $section->id; + + // Get all sections for this course and re-order them (2 of them should now share the same section number). + $sections = $DB->get_records_menu( + 'course_sections', + ['course' => $this->course->id], + 'section ASC, id ASC', + 'id, section' + ); + if (!isset($sections[$sectionid])) { + return false; + } + + $sectionposition = $sections[$sectionid]; + $movedsections = $this->reorder_sections($sections, $sectionposition, $targetposition); + + // Update all sections. Do this in 2 steps to avoid breaking database + // uniqueness constraint. + $transaction = $DB->start_delegated_transaction(); + foreach ($movedsections as $id => $position) { + if ((int) $sections[$id] !== $position) { + $DB->set_field('course_sections', 'section', -$position, ['id' => $id]); + // Invalidate the section cache by given section id. + \core_course\modinfo::purge_course_section_cache_by_id($this->course->id, $id); + } + } + foreach ($movedsections as $id => $position) { + if ((int) $sections[$id] !== $position) { + $DB->set_field('course_sections', 'section', $position, ['id' => $id]); + // Invalidate the section cache by given section id. + \core_course\modinfo::purge_course_section_cache_by_id($this->course->id, $id); + } + } + + // If we move the highlighted section itself, then just highlight the destination. + // Adjust the higlighted section location if we move something over it either direction. + $marker = null; + if ($sectionposition == $this->course->marker) { + $marker = $targetposition; + } else if ($sectionposition > $this->course->marker && $this->course->marker >= $targetposition) { + $marker = $this->course->marker + 1; + } else if ($sectionposition < $this->course->marker && $this->course->marker <= $targetposition) { + $marker = $this->course->marker - 1; + } + if ($marker !== null) { + $this->set_marker_internal($marker); + } + + $transaction->allow_commit(); + rebuild_course_cache($this->course->id, true, true); + return true; + } + + /** + * Reorder sections array by moving origin position to target position. + * This is a helper for move_at(). + * + * @param array $sections array of sectionid=>position + * @param int $originposition the position to move + * @param int $targetposition the position to move to + * @return array|null reordered sections array or false on error + */ + protected function reorder_sections(array $sections, int $originposition, int $targetposition): ?array { + if (!is_array($sections)) { + return null; + } + + // We can't move section position 0. + if ($originposition < 1) { + return null; + } + + // Locate origin section in sections array. + if (!$originkey = array_search($originposition, $sections)) { + return null; // Searched position not in sections array. + } + + // Extract origin section. + $originsection = $sections[$originkey]; + unset($sections[$originkey]); + + // Find offset of target position (stupid PHP's array_splice requires offset instead of key index!). + $found = false; + $appendarray = []; + foreach ($sections as $id => $position) { + if ($found) { + $appendarray[$id] = $position; + unset($sections[$id]); + } + if ($position == $targetposition) { + if ($targetposition < $originposition) { + $appendarray[$id] = $position; + unset($sections[$id]); + } + $found = true; + } + } + + // Append moved section. + $sections[$originkey] = $originsection; + + // Append rest of array (if applicable). + if (!empty($appendarray)) { + foreach ($appendarray as $id => $position) { + $sections[$id] = $position; + } + } + + // Renumber positions. + $position = 0; + foreach ($sections as $id => $p) { + $sections[$id] = $position; + $position++; + } + return $sections; + } + /** * Transfer the visibility of the section to the course modules. * @@ -536,5 +705,8 @@ class sectionactions extends baseactions { clearonly: true, partialrebuild: true, ); + $format = $this->get_format(); + $cachekey = "{$this->course->id}_{$format->get_format()}"; + \cache_helper::invalidate_by_event('changesincourseactionstate', [$cachekey]); } } diff --git a/public/course/format/tests/local/sectionactions_test.php b/public/course/format/tests/local/sectionactions_test.php index 2a483100cfe..5a4b75e530a 100644 --- a/public/course/format/tests/local/sectionactions_test.php +++ b/public/course/format/tests/local/sectionactions_test.php @@ -16,6 +16,7 @@ namespace core_courseformat\local; +use core_courseformat\formatactions; use stdClass; /** @@ -1035,4 +1036,358 @@ final class sectionactions_test extends \advanced_testcase { $this->assertEquals(1, $DB->count_records('course_modules', ['course' => $course->id, 'visible' => 1])); $this->assertEquals(1, $DB->count_records('course_modules', ['course' => $course->id, 'visible' => 0])); } + + /** + * Test section move after method + * + * @param int $movedsection The section number to move. + * @param int $previoussection The section number to move after. + * @param bool $expectedreturnvalue The expected return value of the move_after method. + * @param array|null $expectedsections The expected order of sections after the move, or null to use default. + */ + #[\PHPUnit\Framework\Attributes\DataProvider('move_after_provider')] + public function test_move_after( + int $movedsection, + int $previoussection, + bool $expectedreturnvalue, + ?array $expectedsections = null + ): void { + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 4, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $sections = get_fast_modinfo($course)->get_section_info_all(); + + $returnval = $sectionactions->move_after($sections[$movedsection], $sections[$previoussection]); + $sections = array_map( + fn($section) => $section->name, + get_fast_modinfo($course)->get_section_info_all(), + ); + + $this->assertEquals($expectedreturnvalue, $returnval); + $sections = array_map( + fn($section) => $section->name, + get_fast_modinfo($course)->get_section_info_all(), + ); + if (!$expectedreturnvalue) { + $expectedsections = [ + null, + 'Section 1', + 'Section 2', + 'Section 3', + 'Section 4', + ]; + } + $this->assertEquals( + $expectedsections, + $sections + ); + } + + /** + * Test section move after method when trying to move a section after a non-used section. + */ + public function test_move_after_no_use_section(): void { + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course( + ['format' => 'singleactivity', 'numsections' => 3, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $sections = get_fast_modinfo($course)->get_section_info_all(); + $hasmoved = $sectionactions->move_after($sections[1], $sections[2]); + $this->assertFalse($hasmoved); + } + + /** + * Test section move after method when trying to move a section after a section in another course. + */ + public function test_move_after_other_course(): void { + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 2, 'initsections' => true], + ['createsections' => true] + ); + $othercourse = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 2, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $sections = get_fast_modinfo($course)->get_section_info_all(); + $othersections = get_fast_modinfo($othercourse)->get_section_info_all(); + $hasmoved = $sectionactions->move_after($sections[1], $sections[2]); + $this->assertTrue($hasmoved); + $hasmoved = $sectionactions->move_after($sections[1], $othersections[1]); + $this->assertFalse($hasmoved); + } + + /** + * Data provider for test_move_after. + * + * @return \Generator + */ + public static function move_after_provider(): \Generator { + yield 'move section 4 after section 2' => [ + 'movedsection' => 4, + 'previoussection' => 2, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 1', + 'Section 2', + 'Section 4', + 'Section 3', + ], + ]; + yield 'move section 3 after section 4' => [ + 'movedsection' => 3, + 'previoussection' => 4, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 1', + 'Section 2', + 'Section 4', + 'Section 3', + ], + ]; + yield 'move section 3 after section 0' => [ + 'movedsection' => 3, + 'previoussection' => 0, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 3', + 'Section 1', + 'Section 2', + 'Section 4', + ], + ]; + yield 'move section 1 after section 4' => [ + 'movedsection' => 1, + 'previoussection' => 4, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 2', + 'Section 3', + 'Section 4', + 'Section 1', + ], + ]; + yield 'move section 0 to section 4' => [ + 'movedsection' => 0, + 'previoussection' => 4, + 'expectedreturnvalue' => false, + ]; + yield 'move section 2 after section 1 (existing position)' => [ + 'movedsection' => 2, + 'previoussection' => 1, + 'expectedreturnvalue' => false, + ]; + yield 'move section 2 after itself' => [ + 'movedsection' => 2, + 'previoussection' => 2, + 'expectedreturnvalue' => false, + ]; + } + + /** + * Test section move at method + * + * @param int $movedsection The section number to move. + * @param int $position The position to move the section to. + * @param bool $expectedreturnvalue + * @param array|null $expectedsections + */ + #[\PHPUnit\Framework\Attributes\DataProvider('move_at_provider')] + public function test_move_at( + int $movedsection, + int $position, + bool $expectedreturnvalue, + ?array $expectedsections = null + ): void { + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 4, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $sections = get_fast_modinfo($course)->get_section_info_all(); + + $returnvalue = $sectionactions->move_at($sections[$movedsection], $position); + $this->assertEquals($expectedreturnvalue, $returnvalue); + $sections = array_map( + fn($section) => $section->name, + get_fast_modinfo($course)->get_section_info_all(), + ); + if (!$expectedreturnvalue) { + $expectedsections = [ + null, + 'Section 1', + 'Section 2', + 'Section 3', + 'Section 4', + ]; + } + $this->assertEquals( + $expectedsections, + $sections + ); + } + + /** + * Test section move at method when trying to move a section after a non-used section. + */ + public function test_move_at_no_use_section(): void { + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course( + ['format' => 'singleactivity', 'numsections' => 3, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $sections = get_fast_modinfo($course)->get_section_info_all(); + $hasmoved = $sectionactions->move_at($sections[1], 2); + $this->assertFalse($hasmoved); + } + + /** + * Test section move at method when trying to move a section that belongs to another course. + */ + public function test_move_at_other_course(): void { + $this->resetAfterTest(); + $course = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 2, 'initsections' => true], + ['createsections' => true] + ); + $othercourse = $this->getDataGenerator()->create_course( + ['format' => 'topics', 'numsections' => 2, 'initsections' => true], + ['createsections' => true] + ); + $sectionactions = formatactions::section($course); + $othersections = get_fast_modinfo($othercourse)->get_section_info_all(); + $hasmoved = $sectionactions->move_at($othersections[1], 2); + $this->assertFalse($hasmoved); + } + + + /** + * Data provider for test_move_at. + * + * @return \Generator + */ + public static function move_at_provider(): \Generator { + yield 'move section 4 at position 2' => [ + 'movedsection' => 4, + 'position' => 2, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 1', + 'Section 4', + 'Section 2', + 'Section 3', + ], + ]; + yield 'move section 3 at position 4' => [ + 'movedsection' => 3, + 'position' => 4, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 1', + 'Section 2', + 'Section 4', + 'Section 3', + ], + ]; + yield 'move section 3 at position 1' => [ + 'movedsection' => 3, + 'position' => 1, + 'expectedreturnvalue' => true, + 'expectedsections' => [ + null, + 'Section 3', + 'Section 1', + 'Section 2', + 'Section 4', + ], + ]; + yield 'move section 2 at position 2' => [ + 'movedsection' => 2, + 'position' => 2, + 'expectedreturnvalue' => false, + ]; + yield 'move section 2 at position 6' => [ + 'movedsection' => 2, + 'position' => 6, + 'expectedreturnvalue' => false, + ]; + yield 'move section 2 at position 0' => [ + 'movedsection' => 2, + 'position' => 0, + 'expectedreturnvalue' => false, + ]; + yield 'move section 0 at position 2' => [ + 'movedsection' => 2, + 'position' => 0, + 'expectedreturnvalue' => false, + ]; + } + + /** + * Test reorder_sections function. + * Here we just test this private method as removed the equivalent test in the courselib_tests + */ + public function test_reorder_sections(): void { + global $DB; + $this->resetAfterTest(true); + + $this->getDataGenerator()->create_course( + ['numsections' => 5], + ['createsections' => true], + ); + $course = $this->getDataGenerator()->create_course( + ['numsections' => 10], + ['createsections' => true], + ); + $oldsections = []; + $sections = []; + $existingsections = $DB->get_records('course_sections', ['course' => $course->id], 'id'); + foreach ($existingsections as $section) { + $oldsections[$section->section] = $section->id; + $sections[$section->id] = $section->section; + } + ksort($oldsections); + + // Get the reorder_sections function using reflection. + $reflection = new \ReflectionClass(sectionactions::class); + $sectionactions = formatactions::section($course); + $method = $reflection->getMethod('reorder_sections'); + + $neworder = $method->invoke($sectionactions, $sections, 2, 4); + $neworder = array_keys($neworder); + $this->assertEquals($oldsections[0], $neworder[0]); + $this->assertEquals($oldsections[1], $neworder[1]); + $this->assertEquals($oldsections[2], $neworder[4]); + $this->assertEquals($oldsections[3], $neworder[2]); + $this->assertEquals($oldsections[4], $neworder[3]); + $this->assertEquals($oldsections[5], $neworder[5]); + $this->assertEquals($oldsections[6], $neworder[6]); + + $neworder = $method->invoke($sectionactions, $sections, 4, 2); + $neworder = array_keys($neworder); + $this->assertEquals($oldsections[0], $neworder[0]); + $this->assertEquals($oldsections[1], $neworder[1]); + $this->assertEquals($oldsections[2], $neworder[3]); + $this->assertEquals($oldsections[3], $neworder[4]); + $this->assertEquals($oldsections[4], $neworder[2]); + $this->assertEquals($oldsections[5], $neworder[5]); + $this->assertEquals($oldsections[6], $neworder[6]); + } + } diff --git a/public/course/lib.php b/public/course/lib.php index 6db4d1fa672..e4b7152cda8 100644 --- a/public/course/lib.php +++ b/public/course/lib.php @@ -935,64 +935,33 @@ function course_module_calendar_event_update_process($instance, $cm): void { * @param int $destination * @param bool $ignorenumsections * @return bool Result + * @todo see MDL-87419 for the final deprecation in Moodle 6.0. */ +#[\core\attribute\deprecated( + replacement: 'core_courseformat\local\sectionactions', + since: '5.2', + mdl: 'MDL-86862', + reason: 'Replaced by sectionactions::move_after.', +)] function move_section_to($course, $section, $destination, $ignorenumsections = false) { -/// Moves a whole course section up and down within the course - global $USER, $DB; + \core\deprecation::emit_deprecation(__FUNCTION__); if (!$destination && $destination != 0) { return true; } - - // compartibility with course formats using field 'numsections' $courseformatoptions = course_get_format($course)->get_format_options(); if ((!$ignorenumsections && array_key_exists('numsections', $courseformatoptions) && ($destination > $courseformatoptions['numsections'])) || ($destination < 1)) { return false; } - // Get all sections for this course and re-order them (2 of them should now share the same section number) - if (!$sections = $DB->get_records_menu('course_sections', array('course' => $course->id), - 'section ASC, id ASC', 'id, section')) { + $sectionactions = formatactions::section($course); + $modinfo = get_fast_modinfo($course); + $sectioninfo = $modinfo->get_section_info($section); + if (!$sectioninfo) { return false; } - - $movedsections = reorder_sections($sections, $section, $destination); - - // Update all sections. Do this in 2 steps to avoid breaking database - // uniqueness constraint - $transaction = $DB->start_delegated_transaction(); - foreach ($movedsections as $id => $position) { - if ((int) $sections[$id] !== $position) { - $DB->set_field('course_sections', 'section', -$position, ['id' => $id]); - // Invalidate the section cache by given section id. - course_modinfo::purge_course_section_cache_by_id($course->id, $id); - } - } - foreach ($movedsections as $id => $position) { - if ((int) $sections[$id] !== $position) { - $DB->set_field('course_sections', 'section', $position, ['id' => $id]); - // Invalidate the section cache by given section id. - course_modinfo::purge_course_section_cache_by_id($course->id, $id); - } - } - - // If we move the highlighted section itself, then just highlight the destination. - // Adjust the higlighted section location if we move something over it either direction. - if ($section == $course->marker) { - $sectioninfo = get_fast_modinfo($course->id)->get_section_info($destination); - formatactions::section($course->id)->set_marker($sectioninfo, true); - } else if ($section > $course->marker && $course->marker >= $destination) { - $sectioninfo = get_fast_modinfo($course->id)->get_section_info($course->marker + 1); - formatactions::section($course->id)->set_marker($sectioninfo, true); - } else if ($section < $course->marker && $course->marker <= $destination) { - $sectioninfo = get_fast_modinfo($course->id)->get_section_info($course->marker - 1); - formatactions::section($course->id)->set_marker($sectioninfo, true); - } - - $transaction->allow_commit(); - rebuild_course_cache($course->id, true, true); - return true; + return $sectionactions->move_at($sectioninfo, $destination); } /** @@ -1107,7 +1076,13 @@ function course_can_delete_section($course, $section) { * @param int $target_position * @return array|false */ +#[\core\attribute\deprecated( + since: '5.2', + reason: 'Unused after refactoring the move_after function.', + mdl: 'MDL-86862', +)] function reorder_sections($sections, $origin_position, $target_position) { + \core\deprecation::emit_deprecation(__FUNCTION__); if (!is_array($sections)) { return false; } diff --git a/public/lib/db/caches.php b/public/lib/db/caches.php index ad9bb3019ff..376e979fe51 100644 --- a/public/lib/db/caches.php +++ b/public/lib/db/caches.php @@ -239,6 +239,9 @@ $definitions = array( // Executing actions in more than 10 courses usually means executing the same action on each course // so there is no need for caching individual course instances. 'staticaccelerationsize' => 10, + 'invalidationevents' => [ + 'changesincourseactionstate', + ], ], // Used to store data for repositories to avoid repetitive DB queries within one request. 'repositories' => array( From fc45fe4a38b870eadaf87501d8af08a7640c20a2 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Mon, 8 Dec 2025 11:51:50 +0100 Subject: [PATCH 2/2] MDL-86862 core_courseformat: Remove uses of move_section --- public/course/format/classes/base.php | 19 ++++++------------- .../format/tests/local/baseactions_test.php | 3 ++- public/course/rest.php | 7 ++++++- public/course/view.php | 9 ++++++--- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/public/course/format/classes/base.php b/public/course/format/classes/base.php index 82429cb6e7a..77718f3d8ab 100644 --- a/public/course/format/classes/base.php +++ b/public/course/format/classes/base.php @@ -1811,7 +1811,10 @@ abstract class base { $decreasenumsections = $courseformathasnumsections && ($section->section <= $course->numsections); // Move the section to the end. - move_section_to($course, $section->section, $lastsection, true); + $sectionactions = \core_courseformat\formatactions::section($course); + $modinfo = get_fast_modinfo($course); + $sectioninfo = $modinfo->get_section_info($section->section); + $sectionactions->move_at($sectioninfo, $lastsection); // Delete all modules from the section. foreach (preg_split('/,/', $section->sequence, -1, PREG_SPLIT_NO_EMPTY) as $cmid) { @@ -1863,18 +1866,8 @@ abstract class base { if ($section->section == $destination->section || $section->section == $destination->section + 1) { return true; } - // The move_section_to moves relative to the section to move. However, this - // method will move the target section always after the destination. - if ($section->section > $destination->section) { - $newsectionnumber = $destination->section + 1; - } else { - $newsectionnumber = $destination->section; - } - return move_section_to( - $this->get_course(), - $section->section, - $newsectionnumber - ); + $sectionactions = \core_courseformat\formatactions::section($this->get_course()); + return $sectionactions->move_after($section, $destination); } /** diff --git a/public/course/format/tests/local/baseactions_test.php b/public/course/format/tests/local/baseactions_test.php index 3c8dc850016..e5ead7e8ce3 100644 --- a/public/course/format/tests/local/baseactions_test.php +++ b/public/course/format/tests/local/baseactions_test.php @@ -99,7 +99,8 @@ final class baseactions_test extends \advanced_testcase { // Section info should be always the most updated one. course_update_section($course, $originalsection, (object)['name' => 'New name']); - move_section_to($course, 1, 3); + $sectionactions = formatactions::section($course); + $sectionactions->move_at($sectioninfo, 3); $sectioninfo = $method->invoke($baseactions, $originalsection->id); $this->assertInstanceOf(section_info::class, $sectioninfo); diff --git a/public/course/rest.php b/public/course/rest.php index 65aeb2a5028..7454d42bb30 100644 --- a/public/course/rest.php +++ b/public/course/rest.php @@ -62,7 +62,12 @@ if ($class === 'section' && $field === 'move') { } require_capability('moodle/course:movesections', $coursecontext); - move_section_to($course, $id, $value); + + $sectionactions = \core_courseformat\formatactions::section($course); + $modinfo = get_fast_modinfo($course); + $sectioninfo = $modinfo->get_section_info_by_id($id); + $sectionactions->move_at($sectioninfo, $value); + // See if format wants to do something about it. $response = course_get_format($course)->ajax_section_move(); if ($response !== null) { diff --git a/public/course/view.php b/public/course/view.php index b1e8e49b6f4..1dc4b8fa0bd 100644 --- a/public/course/view.php +++ b/public/course/view.php @@ -272,15 +272,18 @@ if ($PAGE->user_allowed_editing()) { 'The move param is deprecated. Please use the standard move modal instead.', DEBUG_DEVELOPER ); - $destsection = $section + $move; - if (move_section_to($course, $section, $destsection)) { + $destsectionnum = $section + $move; + $sectionactions = \core_courseformat\formatactions::section($course); + $modinfo = get_fast_modinfo($course); + $sectioninfo = $modinfo->get_section_info($section); + if ($sectionactions->move_at($sectioninfo, $destsectionnum)) { if ($course->id == SITEID) { redirect($CFG->wwwroot . '/?redirect=0'); } else { if ($format->get_course_display() == COURSE_DISPLAY_MULTIPAGE) { redirect(course_get_url($course)); } else { - redirect(course_get_url($course, $destsection)); + redirect(course_get_url($course, $destsectionnum)); } } } else {