MDL-32215 Course: Ensure section entries are unique on course, section

diff --git a/course/lib.php b/course/lib.php
index 42edb6a..6fd26e1 100644 (file)
--- 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));\r
 
     // 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) {\r
+        if ($sections[$id] !== $position) {\r
+            $DB->set_field('course_sections', 'section', $position, array('id' => $id));\r
+        }\r
     }
 
     // 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 857d13e..5a59bdc 100644 (file)
--- 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 7f53bb4..85a2ee1 100644 (file)
--- a/lib/db/install.xml
+++ b/lib/db/install.xml
@@ -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">

diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php
index 2651e79..49fe07f 100644 (file)
--- 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.\r
+\r
+        // xmldb does not allow changing index uniqueness - instead we must drop
+        // index then add it again\r
+        $table = new xmldb_table('course_sections');\r
+        $index = new xmldb_index('course_section', XMLDB_INDEX_NOTUNIQUE, array('course', 'section'));
+\r
+        // Conditionally launch drop index course_section\r
+        if ($dbman->index_exists($table, $index)) {\r
+            $dbman->drop_index($table, $index);\r
+        }\r
+
+        // 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);\r
+    }
 
     return true;
 }

diff --git a/version.php b/version.php
index 39f12a5..b196bca 100644 (file)
--- 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