Improving performance during task position change (3 times faster than before)

5 files changed, 81 insertions, 127 deletions

diff --git a/ChangeLog b/ChangeLog
index a062848e..c14457c0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -6,6 +6,10 @@ New features:
 - Added report to compare working hours between open and closed tasks
 - Added the possiblity to define custom routes from plugins
 
+Improvements:
+
+- Improving performance during task position change (3 times faster than before)
+
 Bug fixes:
 
 - Fix wrong constant name that cause a PHP error in project management section
diff --git a/app/Model/TaskPosition.php b/app/Model/TaskPosition.php
index da363cb3..0a8f6c4c 100644
--- a/app/Model/TaskPosition.php
+++ b/app/Model/TaskPosition.php
@@ -26,28 +26,34 @@ class TaskPosition extends Base
      */
     public function movePosition($project_id, $task_id, $column_id, $position, $swimlane_id = 0, $fire_events = true)
     {
+        $result = false;
+
         if ($position < 1) {
-            return false;
+            return $result;
         }
 
         $task = $this->taskFinder->getById($task_id);
 
-        // Ignore closed tasks
         if ($task['is_active'] == Task::STATUS_CLOSED) {
             return true;
         }
 
-        $result = false;
+        $this->db->startTransaction();
 
-        if ($task['swimlane_id'] != $swimlane_id) {
-            $result = $this->saveSwimlaneChange($project_id, $task_id, $position, $task['column_id'], $column_id, $task['swimlane_id'], $swimlane_id);
-        } elseif ($task['column_id'] != $column_id) {
-            $result = $this->saveColumnChange($project_id, $task_id, $position, $swimlane_id, $task['column_id'], $column_id);
+        if ($task['swimlane_id'] != $swimlane_id || $task['column_id'] != $column_id) {
+            $result = $this->moveTaskToAnotherColumn($task, $swimlane_id, $column_id, $position);
         } elseif ($task['position'] != $position) {
-            $result = $this->savePositionChange($project_id, $task_id, $position, $column_id, $swimlane_id);
+            $result = $this->moveTaskWithinSameColumn($task, $position);
         }
 
-        if ($result && $fire_events) {
+        if (! $result) {
+            $this->db->cancelTransaction();
+            return false;
+        }
+
+        $this->db->closeTransaction();
+
+        if ($fire_events) {
             $this->fireEvents($task, $column_id, $position, $swimlane_id);
         }
 
@@ -55,145 +61,87 @@ class TaskPosition extends Base
     }
 
     /**
-     * Move a task to another swimlane
+     * Move a task to another column/swimlane
      *
      * @access private
-     * @param  integer    $project_id
-     * @param  integer    $task_id
-     * @param  integer    $position
-     * @param  integer    $original_column_id
-     * @param  integer    $new_column_id
-     * @param  integer    $original_swimlane_id
-     * @param  integer    $new_swimlane_id
+     * @param  array   $task
+     * @param  integer $swimlane_id
+     * @param  integer $column_id
+     * @param  integer $position
      * @return boolean
      */
-    private function saveSwimlaneChange($project_id, $task_id, $position, $original_column_id, $new_column_id, $original_swimlane_id, $new_swimlane_id)
+    private function moveTaskToAnotherColumn(array $task, $swimlane_id, $column_id, $position)
     {
-        $this->db->startTransaction();
-        $r1 = $this->saveTaskPositions($project_id, $task_id, 0, $original_column_id, $original_swimlane_id);
-        $r2 = $this->saveTaskPositions($project_id, $task_id, $position, $new_column_id, $new_swimlane_id);
-        $this->db->closeTransaction();
+        $results = array();
+        $max = $this->getQuery($task['project_id'], $swimlane_id, $column_id)->count();
+        $position = $max > 0 && $position > $max ? $max + 1 : $position;
 
-        return $r1 && $r2;
-    }
+        $results[] = $this->getQuery($task['project_id'], $task['swimlane_id'], $task['column_id'])->gt('position', $task['position'])->decrement('position', 1);
+        $results[] = $this->getQuery($task['project_id'], $swimlane_id, $column_id)->gte('position', $position)->increment('position', 1);
+        $results[] = $this->updateTaskPosition($task['id'], $swimlane_id, $column_id, $position);
 
-    /**
-     * Move a task to another column
-     *
-     * @access private
-     * @param  integer    $project_id
-     * @param  integer    $task_id
-     * @param  integer    $position
-     * @param  integer    $swimlane_id
-     * @param  integer    $original_column_id
-     * @param  integer    $new_column_id
-     * @return boolean
-     */
-    private function saveColumnChange($project_id, $task_id, $position, $swimlane_id, $original_column_id, $new_column_id)
-    {
-        $this->db->startTransaction();
-        $r1 = $this->saveTaskPositions($project_id, $task_id, 0, $original_column_id, $swimlane_id);
-        $r2 = $this->saveTaskPositions($project_id, $task_id, $position, $new_column_id, $swimlane_id);
-        $this->db->closeTransaction();
-
-        return $r1 && $r2;
+        return !in_array(false, $results, true);
     }
 
     /**
-     * Move a task to another position in the same column
+     * Move a task within the same column
      *
      * @access private
-     * @param  integer    $project_id
-     * @param  integer    $task_id
-     * @param  integer    $position
-     * @param  integer    $column_id
-     * @param  integer    $swimlane_id
+     * @param  array   $task
+     * @param  integer $position
      * @return boolean
      */
-    private function savePositionChange($project_id, $task_id, $position, $column_id, $swimlane_id)
+    private function moveTaskWithinSameColumn(array $task, $position)
     {
-        $this->db->startTransaction();
-        $result = $this->saveTaskPositions($project_id, $task_id, $position, $column_id, $swimlane_id);
-        $this->db->closeTransaction();
+        $results = array();
+        $max = $this->getQuery($task['project_id'], $task['swimlane_id'], $task['column_id'])->count();
+        $position = $max > 0 && $position > $max ? $max : $position;
+
+        if ($position >= $max) {
+            $results[] = $this->getQuery($task['project_id'], $task['swimlane_id'], $task['column_id'])->lte('position', $position)->decrement('position', 1);
+        } else {
+            $results[] = $this->getQuery($task['project_id'], $task['swimlane_id'], $task['column_id'])->gte('position', $position)->increment('position', 1);
+        }
 
-        return $result;
+        $results[] = $this->updateTaskPosition($task['id'], $task['swimlane_id'], $task['column_id'], $position);
+
+        return !in_array(false, $results, true);
     }
 
     /**
-     * Save all task positions for one column
+     * Update final task position
      *
      * @access private
-     * @param  integer    $project_id
-     * @param  integer    $task_id
-     * @param  integer    $position
-     * @param  integer    $column_id
-     * @param  integer    $swimlane_id
+     * @param  integer $task_id
+     * @param  integer $swimlane_id
+     * @param  integer $column_id
+     * @param  integer $position
      * @return boolean
      */
-    private function saveTaskPositions($project_id, $task_id, $position, $column_id, $swimlane_id)
+    private function updateTaskPosition($task_id, $swimlane_id, $column_id, $position)
     {
-        $tasks_ids = $this->db->table(Task::TABLE)
+        return $this->db->table(Task::TABLE)
+            ->eq('id', $task_id)
             ->eq('is_active', 1)
-            ->eq('swimlane_id', $swimlane_id)
-            ->eq('project_id', $project_id)
-            ->eq('column_id', $column_id)
-            ->neq('id', $task_id)
-            ->asc('position')
-            ->asc('id')
-            ->findAllByColumn('id');
-
-        $offset = 1;
-
-        foreach ($tasks_ids as $current_task_id) {
-
-            // Insert the new task
-            if ($position == $offset) {
-                if (! $this->saveTaskPosition($task_id, $offset, $column_id, $swimlane_id)) {
-                    return false;
-                }
-                $offset++;
-            }
-
-            // Rewrite other tasks position
-            if (! $this->saveTaskPosition($current_task_id, $offset, $column_id, $swimlane_id)) {
-                return false;
-            }
-
-            $offset++;
-        }
-
-        // Insert the new task at the bottom and normalize bad position
-        if ($position >= $offset && ! $this->saveTaskPosition($task_id, $offset, $column_id, $swimlane_id)) {
-            return false;
-        }
-
-        return true;
+            ->update(array('position' => $position, 'column_id' => $column_id, 'swimlane_id' => $swimlane_id));
     }
 
     /**
-     * Save new task position
+     * Get common query
      *
      * @access private
-     * @param  integer    $task_id
-     * @param  integer    $position
-     * @param  integer    $column_id
-     * @param  integer    $swimlane_id
-     * @return boolean
+     * @param  integer $project_id
+     * @param  integer $swimlane_id
+     * @param  integer $column_id
+     * @return \PicoDb\Table
      */
-    private function saveTaskPosition($task_id, $position, $column_id, $swimlane_id)
+    private function getQuery($project_id, $swimlane_id, $column_id)
     {
-        $result = $this->db->table(Task::TABLE)->eq('id', $task_id)->update(array(
-            'position' => $position,
-            'column_id' => $column_id,
-            'swimlane_id' => $swimlane_id,
-        ));
-
-        if (! $result) {
-            $this->db->cancelTransaction();
-            return false;
-        }
-
-        return true;
+        return $this->db->table(Task::TABLE)
+            ->eq('project_id', $project_id)
+            ->eq('swimlane_id', $swimlane_id)
+            ->eq('column_id', $column_id)
+            ->eq('is_active', 1);
     }
 
     /**
diff --git a/composer.json b/composer.json
index 06637abc..f75a4829 100644
--- a/composer.json
+++ b/composer.json
@@ -16,7 +16,7 @@
         "erusev/parsedown" : "1.6.0",
         "fabiang/xmpp" : "0.6.1",
         "fguillot/json-rpc" : "1.0.3",
-        "fguillot/picodb" : "1.0.3",
+        "fguillot/picodb" : "dev-master",
         "fguillot/simpleLogger" : "1.0.0",
         "fguillot/simple-validator" : "1.0.0",
         "league/html-to-markdown" : "~4.0",
diff --git a/composer.lock b/composer.lock
index 4af69a06..228805d0 100644
--- a/composer.lock
+++ b/composer.lock
@@ -4,8 +4,8 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",
         "This file is @generated automatically"
     ],
-    "hash": "a003844ed3fef854d64490d07064c66f",
-    "content-hash": "2189f3f4d11741a635dbd27a5750b71e",
+    "hash": "964754ad3b7c394f96169a14865ffeee",
+    "content-hash": "c379b500a20d7025a0b872c7ec8837d3",
     "packages": [
         {
             "name": "christian-riesen/base32",
@@ -297,16 +297,16 @@
         },
         {
             "name": "fguillot/picodb",
-            "version": "v1.0.3",
+            "version": "dev-master",
             "source": {
                 "type": "git",
                 "url": "https://github.com/fguillot/picoDb.git",
-                "reference": "0b4640ebe531a89d2faa36dfabe30a194f1fe903"
+                "reference": "cd377f1bc944086600f8b1bab975981e3d27f7f1"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/fguillot/picoDb/zipball/0b4640ebe531a89d2faa36dfabe30a194f1fe903",
-                "reference": "0b4640ebe531a89d2faa36dfabe30a194f1fe903",
+                "url": "https://api.github.com/repos/fguillot/picoDb/zipball/cd377f1bc944086600f8b1bab975981e3d27f7f1",
+                "reference": "cd377f1bc944086600f8b1bab975981e3d27f7f1",
                 "shasum": ""
             },
             "require": {
@@ -330,7 +330,7 @@
             ],
             "description": "Minimalist database query builder",
             "homepage": "https://github.com/fguillot/picoDb",
-            "time": "2015-11-30 02:57:18"
+            "time": "2015-12-23 13:56:10"
         },
         {
             "name": "fguillot/simple-validator",
@@ -884,7 +884,9 @@
     ],
     "aliases": [],
     "minimum-stability": "stable",
-    "stability-flags": [],
+    "stability-flags": {
+        "fguillot/picodb": 20
+    },
     "prefer-stable": false,
     "prefer-lowest": false,
     "platform": {
diff --git a/tests/units/Model/TaskPositionTest.php b/tests/units/Model/TaskPositionTest.php
index 42612f44..5f045768 100644
--- a/tests/units/Model/TaskPositionTest.php
+++ b/tests/units/Model/TaskPositionTest.php
@@ -106,7 +106,7 @@ class TaskPositionTest extends Base
         $this->assertEquals(1, $tc->create(array('title' => 'Task #1', 'project_id' => 1, 'column_id' => 1)));
         $this->assertEquals(2, $tc->create(array('title' => 'Task #2', 'project_id' => 1, 'column_id' => 1)));
 
-        // We move the task 2 to the column 3
+        // We move the task 1 to the column 3
         $this->assertTrue($tp->movePosition(1, 1, 3, 1));
 
         // Check tasks position
@@ -235,7 +235,7 @@ class TaskPositionTest extends Base
         $this->assertEquals(3, $tc->create(array('title' => 'Task #3', 'project_id' => 1, 'column_id' => 1)));
         $this->assertEquals(4, $tc->create(array('title' => 'Task #4', 'project_id' => 1, 'column_id' => 1)));
 
-        // Move the last task to the bottom
+        // Move the first task to the bottom
         $this->assertTrue($tp->movePosition(1, 1, 1, 4));
 
         // Check tasks position