Models refactoring/improvements

app/Controller/Base.php

@@ -269,7 +269,7 @@ abstract class Base
      */
     protected function getTask()
     {
-        $task = $this->task->getById($this->request->getIntegerParam('task_id'), true);
+        $task = $this->task->getDetails($this->request->getIntegerParam('task_id'));
 
         if (! $task) {
             $this->notfound();

app/Controller/Task.php

@@ -60,7 +60,7 @@ class Task extends Base
             $this->forbidden(true);
         }
 
-        $task = $this->task->getById($this->request->getIntegerParam('task_id'), true);
+        $task = $this->task->getDetails($this->request->getIntegerParam('task_id'));
 
         if (! $task) {
             $this->notfound(true);

app/Controller/User.php

@@ -353,7 +353,7 @@ class User extends Base
         $this->response->html($this->layout('user_edit', array(
             'values' => $values,
             'errors' => $errors,
-            'projects' => $this->projectPermission->getAllowedProjects($user['id']),
+            'projects' => $this->projectPermission->filterProjects($this->project->getList(), $user['id']),
             'user' => $user,
         )));
     }

app/Event/CommentNotificationListener.php

@@ -23,7 +23,7 @@ class CommentNotificationListener extends BaseNotificationListener
     {
         $values = array();
         $values['comment'] = $this->notification->comment->getById($data['id']);
-        $values['task'] = $this->notification->task->getById($values['comment']['task_id'], true);
+        $values['task'] = $this->notification->task->getDetails($values['comment']['task_id']);
 
         return $values;
     }

app/Event/FileNotificationListener.php

@@ -23,7 +23,7 @@ class FileNotificationListener extends BaseNotificationListener
     {
         $values = array();
         $values['file'] = $data;
-        $values['task'] = $this->notification->task->getById($data['task_id'], true);
+        $values['task'] = $this->notification->task->getDetails($data['task_id']);
 
         return $values;
     }

app/Event/SubTaskNotificationListener.php

@@ -23,7 +23,7 @@ class SubTaskNotificationListener extends BaseNotificationListener
     {
         $values = array();
         $values['subtask'] = $this->notification->subtask->getById($data['id'], true);
-        $values['task'] = $this->notification->task->getById($data['task_id'], true);
+        $values['task'] = $this->notification->task->getDetails($data['task_id']);
 
         return $values;
     }

app/Event/TaskNotificationListener.php

@@ -22,7 +22,7 @@ class TaskNotificationListener extends BaseNotificationListener
     public function getTemplateData(array $data)
     {
         $values = array();
-        $values['task'] = $this->notification->task->getById($data['task_id'], true);
+        $values['task'] = $this->notification->task->getDetails($data['task_id']);
 
         return $values;
     }

app/Model/Base.php

@@ -19,6 +19,7 @@ use PicoDb\Database;
  * @property \Model\Board              $board
  * @property \Model\Category           $category
  * @property \Model\Comment            $comment
+ * @property \Model\CommentHistory     $commentHistory
  * @property \Model\Color              $color
  * @property \Model\Config             $config
  * @property \Model\DateParser         $dateParser
@@ -28,6 +29,7 @@ use PicoDb\Database;
  * @property \Model\Project            $project
  * @property \Model\ProjectPermission  $projectPermission
  * @property \Model\SubTask            $subTask
+ * @property \Model\SubtaskHistory     $subtaskHistory
  * @property \Model\Task               $task
  * @property \Model\TaskExport         $taskExport
  * @property \Model\TaskHistory        $taskHistory
@@ -85,4 +87,52 @@ abstract class Base
     {
         return Tool::loadModel($this->registry, $name);
     }
+
+    /**
+     * Remove keys from an array
+     *
+     * @access public
+     * @param  array     $values    Input array
+     * @param  array     $keys      List of keys to remove
+     */
+    public function removeFields(array &$values, array $keys)
+    {
+        foreach ($keys as $key) {
+            if (isset($values[$key])) {
+                unset($values[$key]);
+            }
+        }
+    }
+
+    /**
+     * Force some fields to be at 0 if empty
+     *
+     * @access public
+     * @param  array     $values    Input array
+     * @param  array     $keys      List of keys
+     */
+    public function resetFields(array &$values, array $keys)
+    {
+        foreach ($keys as $key) {
+            if (isset($values[$key]) && empty($values[$key])) {
+                $values[$key] = 0;
+            }
+        }
+    }
+
+    /**
+     * Force some fields to be integer
+     *
+     * @access public
+     * @param  array     $values    Input array
+     * @param  array     $keys      List of keys
+     */
+    public function convertIntegerFields(array &$values, array $keys)
+    {
+        foreach ($keys as $key) {
+            if (isset($values[$key])) {
+                $values[$key] = (int) $values[$key];
+            }
+        }
+    }
 }

app/Model/SubTask.php

@@ -128,17 +128,8 @@ class SubTask extends Base
      */
     public function prepare(array &$values)
     {
-        if (isset($values['another_subtask'])) {
+        $this->removeFields($values, array('another_subtask'));
-            unset($values['another_subtask']);
+        $this->resetFields($values, array('time_estimated', 'time_spent'));
-        }
-
-        if (isset($values['time_estimated']) && empty($values['time_estimated'])) {
-            $values['time_estimated'] = 0;
-        }
-
-        if (isset($values['time_spent']) && empty($values['time_spent'])) {
-            $values['time_spent'] = 0;
-        }
     }
 
     /**

app/Model/Task.php

@@ -41,8 +41,6 @@ class Task extends Base
     const EVENT_CREATE_UPDATE   = 'task.create_update';
     const EVENT_ASSIGNEE_CHANGE = 'task.assignee_change';
 
-
-
     /**
      * Get a list of due tasks for all projects
      *
@@ -72,59 +70,63 @@ class Task extends Base
         return $tasks;
     }
 
+    /**
+     * Get task details (fetch more information from other tables)
+     *
+     * @access public
+     * @param  integer   $task_id   Task id
+     * @return array
+     */
+    public function getDetails($task_id)
+    {
+        $sql = '
+            SELECT
+            tasks.id,
+            tasks.title,
+            tasks.description,
+            tasks.date_creation,
+            tasks.date_completed,
+            tasks.date_modification,
+            tasks.date_due,
+            tasks.color_id,
+            tasks.project_id,
+            tasks.column_id,
+            tasks.owner_id,
+            tasks.creator_id,
+            tasks.position,
+            tasks.is_active,
+            tasks.score,
+            tasks.category_id,
+            project_has_categories.name AS category_name,
+            projects.name AS project_name,
+            columns.title AS column_title,
+            users.username AS assignee_username,
+            users.name AS assignee_name,
+            creators.username AS creator_username,
+            creators.name AS creator_name
+            FROM tasks
+            LEFT JOIN users ON users.id = tasks.owner_id
+            LEFT JOIN users AS creators ON creators.id = tasks.creator_id
+            LEFT JOIN project_has_categories ON project_has_categories.id = tasks.category_id
+            LEFT JOIN projects ON projects.id = tasks.project_id
+            LEFT JOIN columns ON columns.id = tasks.column_id
+            WHERE tasks.id = ?
+        ';
+
+        $rq = $this->db->execute($sql, array($task_id));
+        return $rq->fetch(PDO::FETCH_ASSOC);
+    }
+
     /**
      * Fetch one task
      *
      * @access public
      * @param  integer   $task_id   Task id
-     * @param  boolean   $more      If true, fetch all related information
      * @return array
      */
-    public function getById($task_id, $more = false)
+    public function getById($task_id)
     {
-        if ($more) {
+        return $this->db->table(self::TABLE)->eq('id', $task_id)->findOne();
-
-            $sql = '
-                SELECT
-                tasks.id,
-                tasks.title,
-                tasks.description,
-                tasks.date_creation,
-                tasks.date_completed,
-                tasks.date_modification,
-                tasks.date_due,
-                tasks.color_id,
-                tasks.project_id,
-                tasks.column_id,
-                tasks.owner_id,
-                tasks.creator_id,
-                tasks.position,
-                tasks.is_active,
-                tasks.score,
-                tasks.category_id,
-                project_has_categories.name AS category_name,
-                projects.name AS project_name,
-                columns.title AS column_title,
-                users.username AS assignee_username,
-                users.name AS assignee_name,
-                creators.username AS creator_username,
-                creators.name AS creator_name
-                FROM tasks
-                LEFT JOIN users ON users.id = tasks.owner_id
-                LEFT JOIN users AS creators ON creators.id = tasks.creator_id
-                LEFT JOIN project_has_categories ON project_has_categories.id = tasks.category_id
-                LEFT JOIN projects ON projects.id = tasks.project_id
-                LEFT JOIN columns ON columns.id = tasks.column_id
-                WHERE tasks.id = ?
-            ';
-
-            $rq = $this->db->execute($sql, array($task_id));
-            return $rq->fetch(PDO::FETCH_ASSOC);
-        }
-        else {
-
-            return $this->db->table(self::TABLE)->eq('id', $task_id)->findOne();
-        }
     }
 
     /**
@@ -161,6 +163,25 @@ class Task extends Base
                     ->count();
     }
 
+    /**
+     * Count the number of tasks for a given column and status
+     *
+     * @access public
+     * @param  integer   $project_id   Project id
+     * @param  integer   $column_id    Column id
+     * @param  array     $status       List of status id
+     * @return integer
+     */
+    public function countByColumnId($project_id, $column_id, array $status = array(self::STATUS_OPEN))
+    {
+        return $this->db
+                    ->table(self::TABLE)
+                    ->eq('project_id', $project_id)
+                    ->eq('column_id', $column_id)
+                    ->in('is_active', $status)
+                    ->count();
+    }
+
     /**
      * Get tasks that match defined filters
      *
@@ -226,25 +247,6 @@ class Task extends Base
         return $table->findAll();
     }
 
-    /**
-     * Count the number of tasks for a given column and status
-     *
-     * @access public
-     * @param  integer   $project_id   Project id
-     * @param  integer   $column_id    Column id
-     * @param  array     $status       List of status id
-     * @return integer
-     */
-    public function countByColumnId($project_id, $column_id, array $status = array(self::STATUS_OPEN))
-    {
-        return $this->db
-                    ->table(self::TABLE)
-                    ->eq('project_id', $project_id)
-                    ->eq('column_id', $column_id)
-                    ->in('is_active', $status)
-                    ->count();
-    }
-
     /**
      * Generic method to duplicate a task
      *
@@ -347,40 +349,23 @@ class Task extends Base
      */
     public function prepare(array &$values)
     {
-        if (isset($values['another_task'])) {
-            unset($values['another_task']);
-        }
-
         if (! empty($values['date_due']) && ! is_numeric($values['date_due'])) {
             $values['date_due'] = $this->dateParser->getTimestamp($values['date_due']);
         }
 
-        // Force integer fields at 0 (for Postgresql)
+        $this->removeFields($values, array('another_task', 'id'));
-        if (isset($values['date_due']) && empty($values['date_due'])) {
+        $this->resetFields($values, array('date_due', 'score', 'category_id'));
-            $values['date_due'] = 0;
+        $this->convertIntegerFields($values, array('is_active'));
-        }
-
-        if (isset($values['score']) && empty($values['score'])) {
-            $values['score'] = 0;
-        }
-
-        if (isset($values['is_active'])) {
-            $values['is_active'] = (int) $values['is_active'];
-        }
     }
 
     /**
-     * Create a task
+     * Prepare data before task creation
      *
      * @access public
-     * @param  array    $values   Form values
+     * @param  array    $values    Form values
-     * @return boolean
      */
-    public function create(array $values)
+    public function prepareCreation(array &$values)
     {
-        $this->db->startTransaction();
-
-        // Prepare data
         $this->prepare($values);
 
         if (empty($values['column_id'])) {
@@ -395,8 +380,33 @@ class Task extends Base
         $values['date_creation'] = time();
         $values['date_modification'] = $values['date_creation'];
         $values['position'] = $this->countByColumnId($values['project_id'], $values['column_id']) + 1;
+    }
+
+    /**
+     * Prepare data before task modification
+     *
+     * @access public
+     * @param  array    $values    Form values
+     */
+    public function prepareModification(array &$values)
+    {
+        $this->prepare($values);
+        $values['date_modification'] = time();
+    }
+
+    /**
+     * Create a task
+     *
+     * @access public
+     * @param  array    $values   Form values
+     * @return boolean|integer
+     */
+    public function create(array $values)
+    {
+        $this->db->startTransaction();
+
+        $this->prepareCreation($values);
 
-        // Save task
         if (! $this->db->table(self::TABLE)->save($values)) {
             $this->db->cancelTransaction();
             return false;
@@ -431,10 +441,8 @@ class Task extends Base
         }
 
         // Prepare data
-        $this->prepare($values);
         $updated_task = $values;
-        $updated_task['date_modification'] = time();
+        $this->prepareModification($updated_task);
-        unset($updated_task['id']);
 
         $result = $this->db->table(self::TABLE)->eq('id', $values['id'])->update($updated_task);
 

app/Model/User.php

@@ -163,21 +163,8 @@ class User extends Base
             }
         }
 
-        if (isset($values['confirmation'])) {
+        $this->removeFields($values, array('confirmation', 'current_password'));
-            unset($values['confirmation']);
+        $this->resetFields($values, array('is_admin', 'is_ldap_user'));
-        }
-
-        if (isset($values['current_password'])) {
-            unset($values['current_password']);
-        }
-
-        if (isset($values['is_admin']) && empty($values['is_admin'])) {
-            $values['is_admin'] = 0;
-        }
-
-        if (isset($values['is_ldap_user']) && empty($values['is_ldap_user'])) {
-            $values['is_ldap_user'] = 0;
-        }
     }
 
     /**

tests/units/SubtaskTest.php

@@ -24,8 +24,8 @@ class SubTaskTest extends Base
         $this->assertEquals(2, $t->create(array('title' => 'test 2', 'project_id' => 1, 'column_id' => 1, 'owner_id' => 0)));
 
         // We create many subtasks for the first task
-        $this->assertEquals(1, $s->create(array('title' => 'subtask #1', 'task_id' => 1, 'time_estimated' => 5, 'time_spent' => 3, 'status' => 1)));
+        $this->assertEquals(1, $s->create(array('title' => 'subtask #1', 'task_id' => 1, 'time_estimated' => 5, 'time_spent' => 3, 'status' => 1, 'another_subtask' => 'on')));
-        $this->assertEquals(2, $s->create(array('title' => 'subtask #2', 'task_id' => 1, 'time_estimated' => 0, 'time_spent' => 0, 'status' => 2, 'user_id' => 1)));
+        $this->assertEquals(2, $s->create(array('title' => 'subtask #2', 'task_id' => 1, 'time_estimated' => '', 'time_spent' => '', 'status' => 2, 'user_id' => 1)));
 
         // We duplicate our subtasks
         $this->assertTrue($s->duplicate(1, 2));

tests/units/TaskTest.php

@@ -10,6 +10,103 @@ use Model\User;
 
 class TaskTest extends Base
 {
+    public function testPrepareCreation()
+    {
+        $t = new Task($this->registry);
+        $p = new Project($this->registry);
+
+        $this->assertEquals(1, $p->create(array('name' => 'Project #1')));
+
+        $input = array(
+            'title' => 'youpi',
+            'description' => '',
+            'project_id' => '1',
+            'owner_id' => '0',
+            'category_id' => '0',
+            'column_id' => '2',
+            'color_id' => 'yellow',
+            'score' => '',
+            'date_due' => '',
+            'creator_id' => '1',
+            'another_task' => '1',
+        );
+
+        $t->prepareCreation($input);
+
+        $this->assertInternalType('integer', $input['date_due']);
+        $this->assertEquals(0, $input['date_due']);
+
+        $this->assertInternalType('integer', $input['score']);
+        $this->assertEquals(0, $input['score']);
+
+        $this->assertArrayNotHasKey('another_task', $input);
+
+        $this->assertArrayHasKey('date_creation', $input);
+        $this->assertEquals(time(), $input['date_creation']);
+
+        $this->assertArrayHasKey('date_modification', $input);
+        $this->assertEquals(time(), $input['date_modification']);
+
+        $this->assertArrayHasKey('position', $input);
+        $this->assertGreaterThan(0, $input['position']);
+
+        $input = array(
+            'title' => 'youpi',
+            'project_id' => '1',
+        );
+
+        $t->prepareCreation($input);
+
+        $this->assertArrayNotHasKey('date_due', $input);
+        $this->assertArrayNotHasKey('score', $input);
+
+        $this->assertArrayHasKey('date_creation', $input);
+        $this->assertEquals(time(), $input['date_creation']);
+
+        $this->assertArrayHasKey('date_modification', $input);
+        $this->assertEquals(time(), $input['date_modification']);
+
+        $this->assertArrayHasKey('position', $input);
+        $this->assertGreaterThan(0, $input['position']);
+
+        $this->assertArrayHasKey('color_id', $input);
+        $this->assertEquals('yellow', $input['color_id']);
+
+        $this->assertArrayHasKey('column_id', $input);
+        $this->assertEquals(1, $input['column_id']);
+
+        $input = array(
+            'title' => 'youpi',
+            'project_id' => '1',
+            'date_due' => '2014-09-15',
+        );
+
+        $t->prepareCreation($input);
+
+        $this->assertArrayHasKey('date_due', $input);
+        $this->assertInternalType('integer', $input['date_due']);
+        $this->assertEquals('2014-09-15', date('Y-m-d', $input['date_due']));
+    }
+
+    public function testPrepareModification()
+    {
+        $t = new Task($this->registry);
+        $p = new Project($this->registry);
+
+        $this->assertEquals(1, $p->create(array('name' => 'Project #1')));
+
+        $input = array(
+            'id' => '1',
+            'description' => 'Boo',
+        );
+
+        $t->prepareModification($input);
+
+        $this->assertArrayNotHasKey('id', $input);
+        $this->assertArrayHasKey('date_modification', $input);
+        $this->assertEquals(time(), $input['date_modification']);
+    }
+
     public function testCreation()
     {
         $t = new Task($this->registry);

tests/units/UserTest.php

@@ -8,6 +8,57 @@ use Model\Project;
 
 class UserTest extends Base
 {
+    public function testPrepare()
+    {
+        $u = new User($this->registry);
+
+        $input = array(
+            'username' => 'user1',
+            'password' => '1234',
+            'confirmation' => '1234',
+            'name' => 'me',
+            'is_admin' => '',
+        );
+
+        $u->prepare($input);
+        $this->assertArrayNotHasKey('confirmation', $input);
+
+        $this->assertArrayHasKey('password', $input);
+        $this->assertNotEquals('1234', $input['password']);
+        $this->assertNotEmpty($input['password']);
+
+        $this->assertArrayHasKey('is_admin', $input);
+        $this->assertInternalType('integer', $input['is_admin']);
+
+        $input = array(
+            'username' => 'user1',
+            'password' => '1234',
+            'current_password' => 'bla',
+            'confirmation' => '1234',
+            'name' => 'me',
+            'is_ldap_user' => '1',
+        );
+
+        $u->prepare($input);
+        $this->assertArrayNotHasKey('confirmation', $input);
+        $this->assertArrayNotHasKey('current_password', $input);
+
+        $this->assertArrayHasKey('password', $input);
+        $this->assertNotEquals('1234', $input['password']);
+        $this->assertNotEmpty($input['password']);
+
+        $this->assertArrayHasKey('is_ldap_user', $input);
+        $this->assertEquals(1, $input['is_ldap_user']);
+
+        $input = array(
+            'id' => 2,
+            'name' => 'me',
+        );
+
+        $u->prepare($input);
+        $this->assertEquals(array('id' => 2, 'name' => 'me'), $input);
+    }
+
     public function testCreate()
     {
         $u = new User($this->registry);