This commit is contained in:
Amaia Anabitarte
2026-01-30 12:11:29 +07:00
committed by Huong Nguyen
13 changed files with 596 additions and 63 deletions
@@ -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
@@ -0,0 +1,5 @@
issueNumber: MDL-86862
notes:
core_courseformat:
- message: Deprecates the move_section_to logic
type: deprecated
@@ -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
@@ -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
@@ -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
+6 -13
View File
@@ -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);
}
/**
@@ -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]);
}
}
@@ -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);
@@ -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]);
}
}
+19 -44
View File
@@ -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;
}
+6 -1
View File
@@ -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) {
+6 -3
View File
@@ -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 {
+3
View File
@@ -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(