MDL-53507 quiz editing: was possible to lose questions

If you dragged a question to the top of the quiz, it could
disappear. Several other cases of dragging questions to the
same place in sequence, but to a different page/section were
also failing, and have been fixed.

diff --git a/mod/quiz/classes/structure.php b/mod/quiz/classes/structure.php
index a9c4fe2..ba7f3a3 100644 (file)
--- a/mod/quiz/classes/structure.php
+++ b/mod/quiz/classes/structure.php
@@ -708,6 +708,17 @@ class structure {
             $moveafterslotnumber = (int) $this->slots[$idmoveafter]->slot;
         }
 
+        // If the action came in as moving a slot to itself, normalise this to
+        // moving the slot to after the previous slot.
+        if ($moveafterslotnumber == $movingslotnumber) {
+            $moveafterslotnumber = $moveafterslotnumber - 1;
+        }
+
+        $followingslotnumber = $moveafterslotnumber + 1;
+        if ($followingslotnumber == $movingslotnumber) {
+            $followingslotnumber += 1;
+        }
+
         // Check the target page number is OK.
         if ($page == 0) {
             $page = 1;
@@ -716,16 +727,10 @@ class structure {
                 $page < 1) {
             throw new \coding_exception('The target page number is too small.');
         } else if (!$this->is_last_slot_in_quiz($moveafterslotnumber) &&
-                $page > $this->get_page_number_for_slot($moveafterslotnumber + 1)) {
+                $page > $this->get_page_number_for_slot($followingslotnumber)) {
             throw new \coding_exception('The target page number is too large.');
         }
 
-        // If the action came in as moving a slot to itself, normalise this to
-        // moving the slot to after the previosu slot.
-        if ($moveafterslotnumber == $movingslotnumber) {
-            $moveafterslotnumber = $moveafterslotnumber - 1;
-        }
-
         // Work out how things are being moved.
         $slotreorder = array();
         if ($moveafterslotnumber > $movingslotnumber) {
@@ -768,10 +773,12 @@ class structure {
                 $headingmoveafter = $movingslotnumber;
                 $headingmovebefore = $movingslotnumber + 2;
                 $headingmovedirection = -1;
-            } else {
+            } else if ($page < $movingslot->page) {
                 $headingmoveafter = $movingslotnumber - 1;
                 $headingmovebefore = $movingslotnumber + 1;
                 $headingmovedirection = 1;
+            } else {
+                return; // Nothing to do.
             }
         }
 

diff --git a/mod/quiz/db/upgrade.php b/mod/quiz/db/upgrade.php
index c8590e3..db25466 100644 (file)
--- a/mod/quiz/db/upgrade.php
+++ b/mod/quiz/db/upgrade.php
@@ -165,5 +165,30 @@ function xmldb_quiz_upgrade($oldversion) {
     // Moodle v3.0.0 release upgrade line.
     // Put any upgrade step following this.
 
+    if ($oldversion < 2016032100) {
+        // Update quiz_sections to repair quizzes what were broken by MDL-53507.
+        $problemquizzes = $DB->get_records_sql("
+                SELECT quizid, MIN(firstslot) AS firstsectionfirstslot
+                FROM {quiz_sections}
+                GROUP BY quizid
+                HAVING MIN(firstslot) > 1");
+
+        if ($problemquizzes) {
+            $pbar = new progress_bar('upgradegroupmembersonly', 500, true);
+            $total = count($problemquizzes);
+            $done = 0;
+            foreach ($problemquizzes as $problemquiz) {
+                $DB->set_field('quiz_sections', 'firstslot', 1,
+                        array('quizid' => $problemquiz->quizid,
+                        'firstslot' => $problemquiz->firstsectionfirstslot));
+                $done += 1;
+                $pbar->update($done, $total, "Fixing quiz layouts - {$done}/{$total}.");
+            }
+        }
+
+        // Quiz savepoint reached.
+        upgrade_mod_savepoint(true, 2016032100, 'quiz');
+    }
+
     return true;
 }

diff --git a/mod/quiz/tests/structure_test.php b/mod/quiz/tests/structure_test.php
index 6210f81..a8e39f8 100644 (file)
--- a/mod/quiz/tests/structure_test.php
+++ b/mod/quiz/tests/structure_test.php
@@ -515,7 +515,7 @@ class mod_quiz_structure_testcase extends advanced_testcase {
             ), $structure);
     }
 
-    public function test_move_slot_to_down_start_of_second_section() {
+    public function test_move_slot_down_to_start_of_second_section() {
         $quizobj = $this->create_test_quiz(array(
                 'Heading 1',
                 array('TF1', 1, 'truefalse'),
@@ -539,6 +539,63 @@ class mod_quiz_structure_testcase extends advanced_testcase {
             ), $structure);
     }
 
+    public function test_move_first_slot_down_to_start_of_page_2() {
+        $quizobj = $this->create_test_quiz(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 2, 'truefalse'),
+            ));
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+
+        $idtomove = $structure->get_question_in_slot(1)->slotid;
+        $structure->move_slot($idtomove, 0, '2');
+
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+        $this->assert_quiz_layout(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 1, 'truefalse'),
+            ), $structure);
+    }
+
+    public function test_move_first_slot_to_same_place_on_page_1() {
+        $quizobj = $this->create_test_quiz(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 2, 'truefalse'),
+            ));
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+
+        $idtomove = $structure->get_question_in_slot(1)->slotid;
+        $structure->move_slot($idtomove, 0, '1');
+
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+        $this->assert_quiz_layout(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 2, 'truefalse'),
+            ), $structure);
+    }
+
+    public function test_move_first_slot_to_before_page_1() {
+        $quizobj = $this->create_test_quiz(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 2, 'truefalse'),
+            ));
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+
+        $idtomove = $structure->get_question_in_slot(1)->slotid;
+        $structure->move_slot($idtomove, 0, '');
+
+        $structure = \mod_quiz\structure::create_for_quiz($quizobj);
+        $this->assert_quiz_layout(array(
+                'Heading 1',
+                array('TF1', 1, 'truefalse'),
+                array('TF2', 2, 'truefalse'),
+            ), $structure);
+    }
+
     public function test_move_slot_up_to_start_of_second_section() {
         $quizobj = $this->create_test_quiz(array(
                 'Heading 1',

diff --git a/mod/quiz/version.php b/mod/quiz/version.php
index 57b48ce..bcca3fb 100644 (file)
--- a/mod/quiz/version.php
+++ b/mod/quiz/version.php
@@ -24,7 +24,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2015111607;
+$plugin->version   = 2016032100;
 $plugin->requires  = 2015111000;
 $plugin->component = 'mod_quiz';
 $plugin->cron      = 60;