From cf76b3353cc59e41ee68fa0e11dcab3e49a01c00 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Fri, 13 Apr 2012 17:51:20 +0100 Subject: [PATCH] MDL-32215 Course: Ensure section entries are unique on course, section --- course/lib.php | 27 +++++++++++++----- course/simpletest/testcourselib.php | 29 +++++++++++-------- lib/db/install.xml | 2 +- lib/db/upgrade.php | 43 +++++++++++++++++++++++++++++ version.php | 2 +- 5 files changed, 82 insertions(+), 21 deletions(-) diff --git a/course/lib.php b/course/lib.php index 42edb6a2562..6fd26e1cbb1 100644 --- a/course/lib.php +++ b/course/lib.php @@ -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) { @@ -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) { @@ -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; } diff --git a/course/simpletest/testcourselib.php b/course/simpletest/testcourselib.php index 857d13e4ba8..5a59bdccf6c 100644 --- a/course/simpletest/testcourselib.php +++ b/course/simpletest/testcourselib.php @@ -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(); } @@ -61,21 +62,25 @@ class courselib_test extends UnitTestCase { $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); } diff --git a/lib/db/install.xml b/lib/db/install.xml index 7f53bb40c65..85a2ee10999 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -387,7 +387,7 @@ - + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 2651e790657..49fe07f95ae 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -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; } diff --git a/version.php b/version.php index 39f12a59ac2..b196bca330e 100644 --- a/version.php +++ b/version.php @@ -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