Skip to content

Commit

Permalink
MDL-32215 Course: Ensure section entries are unique on course, section
Browse files Browse the repository at this point in the history
  • Loading branch information
@sammarshallou
sammarshallou committed Apr 19, 2012
1 parent 668a499 commit cf76b33
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 21 deletions.
27 changes: 20 additions & 7 deletions course/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2984,8 +2984,11 @@ function move_section($course, $section, $move) {
return false;
}

$DB->set_field("course_sections", "section", $sectiondest, array("id"=>$sectionrecord->id));
// Three-step change ensures that the section always remains unique (there is
// a unique index now)
$DB->set_field("course_sections", "section", -$sectiondest, array("id"=>$sectionrecord->id));
$DB->set_field("course_sections", "section", $section, array("id"=>$sectiondestrecord->id));
$DB->set_field("course_sections", "section", $sectiondest, array("id"=>$sectionrecord->id));

// Update highlighting if the move affects highlighted section
if ($course->marker == $section) {
Expand All @@ -2999,8 +3002,8 @@ function move_section($course, $section, $move) {
course_set_display($course->id, $sectiondest);
}

// Check for duplicates and fix order if needed.
// There is a very rare case that some sections in the same course have the same section id.
// Fix order if needed. The database prevents duplicate sections, but it is
// possible there could be a gap in the numbering.
$sections = $DB->get_records('course_sections', array('course'=>$course->id), 'section ASC');
$n = 0;
foreach ($sections as $section) {
Expand Down Expand Up @@ -3040,17 +3043,27 @@ function move_section_to($course, $section, $destination) {
return false;
}

$sections = reorder_sections($sections, $section, $destination);
$movedsections = reorder_sections($sections, $section, $destination);

// Update all sections
foreach ($sections as $id => $position) {
$DB->set_field('course_sections', 'section', $position, array('id' => $id));
// 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 ($sections[$id] !== $position) {
$DB->set_field('course_sections', 'section', -$position, array('id' => $id));
}
}
foreach ($movedsections as $id => $position) {
if ($sections[$id] !== $position) {
$DB->set_field('course_sections', 'section', $position, array('id' => $id));
}
}

// if the focus is on the section that is being moved, then move the focus along
if (course_get_display($course->id) == $section) {
course_set_display($course->id, $destination);
}
$transaction->allow_commit();
return true;
}

Expand Down
29 changes: 17 additions & 12 deletions course/simpletest/testcourselib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class courselib_test extends UnitTestCase {
function setUp() {
global $DB;
Mock::generate(get_class($DB), 'mockDB');
Mock::generate('moodle_transaction', 'mock_transaction');
$this->realDB = $DB;
$DB = new mockDB();
}
Expand All @@ -61,21 +62,25 @@ function testMoveSection() {
$sections = array(20 => 0, 21 => 1, 22 => 2, 23 => 3, 24 => 4, 25 => 5);

$DB->setReturnValueAt(0, 'get_records_menu', $sections);
$DB->expectAt(0, 'set_field', array('course_sections', 'section', 0, array('id' => 20)));
$DB->expectAt(1, 'set_field', array('course_sections', 'section', 1, array('id' => 21)));
$DB->expectAt(2, 'set_field', array('course_sections', 'section', 2, array('id' => 23)));
$DB->expectAt(3, 'set_field', array('course_sections', 'section', 3, array('id' => 24)));
$DB->expectAt(4, 'set_field', array('course_sections', 'section', 4, array('id' => 22)));
$DB->expectAt(5, 'set_field', array('course_sections', 'section', 5, array('id' => 25)));
$DB->setReturnValueAt(0, 'start_delegated_transaction', new mock_transaction());
$DB->expectAt(0, 'set_field', array('course_sections', 'section', 100002, array('id' => 23)));
$DB->expectAt(1, 'set_field', array('course_sections', 'section', 100003, array('id' => 24)));
$DB->expectAt(2, 'set_field', array('course_sections', 'section', 100004, array('id' => 22)));
$DB->expectAt(3, 'set_field', array('course_sections', 'section', 2, array('id' => 23)));
$DB->expectAt(4, 'set_field', array('course_sections', 'section', 3, array('id' => 24)));
$DB->expectAt(5, 'set_field', array('course_sections', 'section', 4, array('id' => 22)));
move_section_to($course, 2, 4);

$DB->setReturnValueAt(1, 'get_records_menu', $sections);
$DB->expectAt(6, 'set_field', array('course_sections', 'section', 0, array('id' => 20)));
$DB->expectAt(7, 'set_field', array('course_sections', 'section', 1, array('id' => 24)));
$DB->expectAt(8, 'set_field', array('course_sections', 'section', 2, array('id' => 21)));
$DB->expectAt(9, 'set_field', array('course_sections', 'section', 3, array('id' => 22)));
$DB->expectAt(10, 'set_field', array('course_sections', 'section', 4, array('id' => 23)));
$DB->expectAt(11, 'set_field', array('course_sections', 'section', 5, array('id' => 25)));
$DB->setReturnValueAt(1, 'start_delegated_transaction', new mock_transaction());
$DB->expectAt(6, 'set_field', array('course_sections', 'section', 100001, array('id' => 24)));
$DB->expectAt(7, 'set_field', array('course_sections', 'section', 100002, array('id' => 21)));
$DB->expectAt(8, 'set_field', array('course_sections', 'section', 100003, array('id' => 22)));
$DB->expectAt(9, 'set_field', array('course_sections', 'section', 100004, array('id' => 23)));
$DB->expectAt(10, 'set_field', array('course_sections', 'section', 1, array('id' => 24)));
$DB->expectAt(11, 'set_field', array('course_sections', 'section', 2, array('id' => 21)));
$DB->expectAt(12, 'set_field', array('course_sections', 'section', 3, array('id' => 22)));
$DB->expectAt(13, 'set_field', array('course_sections', 'section', 4, array('id' => 23)));
move_section_to($course, 4, 0);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/db/install.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@
<KEY NAME="primary" TYPE="primary" FIELDS="id"/>
</KEYS>
<INDEXES>
<INDEX NAME="course_section" UNIQUE="false" FIELDS="course, section"/>
<INDEX NAME="course_section" UNIQUE="true" FIELDS="course, section"/>
</INDEXES>
</TABLE>
<TABLE NAME="course_request" COMMENT="course requests" PREVIOUS="course_sections" NEXT="filter_active">
Expand Down
43 changes: 43 additions & 0 deletions lib/db/upgrade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,49 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2012032300.02);
}

if ($oldversion < 2012041200.03) {
// This change makes the course_section index unique.

// xmldb does not allow changing index uniqueness - instead we must drop
// index then add it again
$table = new xmldb_table('course_sections');
$index = new xmldb_index('course_section', XMLDB_INDEX_NOTUNIQUE, array('course', 'section'));

// Conditionally launch drop index course_section
if ($dbman->index_exists($table, $index)) {
$dbman->drop_index($table, $index);
}

// Look for any duplicate course_sections entries. There should not be
// any but on some busy systems we found a few, maybe due to previous
// bugs.
$transaction = $DB->start_delegated_transaction();
$rs = $DB->get_recordset_sql('
SELECT DISTINCT
cs.id, cs.course
FROM
{course_sections} cs
INNER JOIN {course_sections} older
ON cs.course = older.course AND cs.section = older.section
AND older.id < cs.id');
foreach ($rs as $rec) {
$DB->delete_records('course_sections', array('id' => $rec->id));
rebuild_course_cache($rec->course, true);
}
$rs->close();
$transaction->allow_commit();

// Define index course_section (unique) to be added to course_sections
$index = new xmldb_index('course_section', XMLDB_INDEX_UNIQUE, array('course', 'section'));

// Conditionally launch add index course_section
if (!$dbman->index_exists($table, $index)) {
$dbman->add_index($table, $index);
}

// Main savepoint reached
upgrade_main_savepoint(true, 2012041200.03);
}

return true;
}
2 changes: 1 addition & 1 deletion version.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
defined('MOODLE_INTERNAL') || die();


$version = 2012041200.00; // YYYYMMDD = weekly release date of this DEV branch
$version = 2012041200.03; // YYYYMMDD = weekly release date of this DEV branch
// RR = release increments - 00 in DEV branches
// .XX = incremental changes

Expand Down

0 comments on commit cf76b33

Please sign in to comment.