Fix bug when moving subtasks with non consecutive positions

app/Model/Subtask.php

@@ -227,6 +227,46 @@ class Subtask extends Base
         return $result;
     }
 
+    /**
+     * Get subtasks with consecutive positions
+     *
+     * If you remove a subtask, the positions are not anymore consecutives
+     *
+     * @access public
+     * @param  integer  $task_id
+     * @return array
+     */
+    public function getNormalizedPositions($task_id)
+    {
+        $subtasks = $this->db->hashtable(self::TABLE)->eq('task_id', $task_id)->asc('position')->getAll('id', 'position');
+        $position = 1;
+
+        foreach ($subtasks as $subtask_id => $subtask_position) {
+            $subtasks[$subtask_id] = $position++;
+        }
+
+        return $subtasks;
+    }
+
+    /**
+     * Save the new positions for a set of subtasks
+     *
+     * @access public
+     * @param  array   $subtasks    Hashmap of column_id/column_position
+     * @return boolean
+     */
+    public function savePositions(array $subtasks)
+    {
+        return $this->db->transaction(function ($db) use ($subtasks) {
+
+            foreach ($subtasks as $subtask_id => $position) {
+                if (! $db->table(self::TABLE)->eq('id', $subtask_id)->update(array('position' => $position))) {
+                    return false;
+                }
+            }
+        });
+    }
+
     /**
      * Move a subtask down, increment the position value
      *
@@ -237,7 +277,7 @@ class Subtask extends Base
      */
     public function moveDown($task_id, $subtask_id)
     {
-        $subtasks = $this->db->hashtable(self::TABLE)->eq('task_id', $task_id)->asc('position')->getAll('id', 'position');
+        $subtasks = $this->getNormalizedPositions($task_id);
         $positions = array_flip($subtasks);
 
         if (isset($subtasks[$subtask_id]) && $subtasks[$subtask_id] < count($subtasks)) {
@@ -245,12 +285,7 @@ class Subtask extends Base
             $position = ++$subtasks[$subtask_id];
             $subtasks[$positions[$position]]--;
 
-            $this->db->startTransaction();
+            return $this->savePositions($subtasks);
-            $this->db->table(self::TABLE)->eq('id', $subtask_id)->update(array('position' => $position));
-            $this->db->table(self::TABLE)->eq('id', $positions[$position])->update(array('position' => $subtasks[$positions[$position]]));
-            $this->db->closeTransaction();
-
-            return true;
         }
 
         return false;
@@ -266,7 +301,7 @@ class Subtask extends Base
      */
     public function moveUp($task_id, $subtask_id)
     {
-        $subtasks = $this->db->hashtable(self::TABLE)->eq('task_id', $task_id)->asc('position')->getAll('id', 'position');
+        $subtasks = $this->getNormalizedPositions($task_id);
         $positions = array_flip($subtasks);
 
         if (isset($subtasks[$subtask_id]) && $subtasks[$subtask_id] > 1) {
@@ -274,12 +309,7 @@ class Subtask extends Base
             $position = --$subtasks[$subtask_id];
             $subtasks[$positions[$position]]++;
 
-            $this->db->startTransaction();
+            return $this->savePositions($subtasks);
-            $this->db->table(self::TABLE)->eq('id', $subtask_id)->update(array('position' => $position));
-            $this->db->table(self::TABLE)->eq('id', $positions[$position])->update(array('position' => $subtasks[$positions[$position]]));
-            $this->db->closeTransaction();
-
-            return true;
         }
 
         return false;

app/Template/subtask/show.php

@@ -1,4 +1,8 @@
 <?php if (! empty($subtasks)): ?>
+
+<?php $first_position = $subtasks[0]['position']; ?>
+<?php $last_position = $subtasks[count($subtasks) - 1]['position']; ?>
+
 <div id="subtasks" class="task-show-section">
 
     <div class="page-header">
@@ -40,12 +44,12 @@
             <?php if (! isset($not_editable)): ?>
                 <td>
                     <ul>
-                        <?php if ($subtask['position'] > 1): ?>
+                        <?php if ($subtask['position'] != $first_position): ?>
                             <li>
                                 <?= $this->a(t('Move Up'), 'subtask', 'movePosition', array('project_id' => $project['id'], 'task_id' => $subtask['task_id'], 'subtask_id' => $subtask['id'], 'direction' => 'up'), true) ?>
                             </li>
                         <?php endif ?>
-                        <?php if ($subtask['position'] != 0 && $subtask['position'] != count($subtasks)): ?>
+                        <?php if ($subtask['position'] != $last_position): ?>
                             <li>
                                 <?= $this->a(t('Move Down'), 'subtask', 'movePosition', array('project_id' => $project['id'], 'task_id' => $subtask['task_id'], 'subtask_id' => $subtask['id'], 'direction' => 'down'), true) ?>
                             </li>

tests/units/SubtaskTest.php

@@ -24,6 +24,7 @@ class SubTaskTest extends Base
         $this->assertEquals(2, $s->create(array('title' => 'subtask #2', 'task_id' => 1)));
         $this->assertEquals(3, $s->create(array('title' => 'subtask #3', 'task_id' => 1)));
 
+        // Check positions
         $subtask = $s->getById(1);
         $this->assertNotEmpty($subtask);
         $this->assertEquals(1, $subtask['position']);
@@ -36,8 +37,10 @@ class SubTaskTest extends Base
         $this->assertNotEmpty($subtask);
         $this->assertEquals(3, $subtask['position']);
 
+        // Move up
         $this->assertTrue($s->moveUp(1, 2));
 
+        // Check positions
         $subtask = $s->getById(1);
         $this->assertNotEmpty($subtask);
         $this->assertEquals(2, $subtask['position']);
@@ -50,7 +53,24 @@ class SubTaskTest extends Base
         $this->assertNotEmpty($subtask);
         $this->assertEquals(3, $subtask['position']);
 
+        // We can't move up #2
         $this->assertFalse($s->moveUp(1, 2));
+
+        // Test remove
+        $this->assertTrue($s->remove(1));
+        $this->assertTrue($s->moveUp(1, 3));
+
+        // Check positions
+        $subtask = $s->getById(1);
+        $this->assertEmpty($subtask);
+
+        $subtask = $s->getById(2);
+        $this->assertNotEmpty($subtask);
+        $this->assertEquals(2, $subtask['position']);
+
+        $subtask = $s->getById(3);
+        $this->assertNotEmpty($subtask);
+        $this->assertEquals(1, $subtask['position']);
     }
 
     public function testMoveDown()
@@ -66,8 +86,10 @@ class SubTaskTest extends Base
         $this->assertEquals(2, $s->create(array('title' => 'subtask #2', 'task_id' => 1)));
         $this->assertEquals(3, $s->create(array('title' => 'subtask #3', 'task_id' => 1)));
 
+        // Move down #1
         $this->assertTrue($s->moveDown(1, 1));
 
+        // Check positions
         $subtask = $s->getById(1);
         $this->assertNotEmpty($subtask);
         $this->assertEquals(2, $subtask['position']);
@@ -80,7 +102,24 @@ class SubTaskTest extends Base
         $this->assertNotEmpty($subtask);
         $this->assertEquals(3, $subtask['position']);
 
+        // We can't move down #3
         $this->assertFalse($s->moveDown(1, 3));
+
+        // Test remove
+        $this->assertTrue($s->remove(1));
+        $this->assertTrue($s->moveDown(1, 2));
+
+        // Check positions
+        $subtask = $s->getById(1);
+        $this->assertEmpty($subtask);
+
+        $subtask = $s->getById(2);
+        $this->assertNotEmpty($subtask);
+        $this->assertEquals(2, $subtask['position']);
+
+        $subtask = $s->getById(3);
+        $this->assertNotEmpty($subtask);
+        $this->assertEquals(1, $subtask['position']);
     }
 
     public function testDuplicate()