Unify task drop-down menu between different views

15 files changed, 152 insertions, 171 deletions

diff --git a/ChangeLog b/ChangeLog
index e33bee40..27b4a3dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -11,6 +11,7 @@ New features:
 
 Improvements:
 
+* Unify task drop-down menu between different views
 * Improve LDAP user group membership synchronization
 * Category and user filters do not append anymore in search field
 * Added more template hooks
diff --git a/app/Controller/BoardPopover.php b/app/Controller/BoardPopover.php
index 63dab302..10584137 100644
--- a/app/Controller/BoardPopover.php
+++ b/app/Controller/BoardPopover.php
@@ -44,7 +44,7 @@ class BoardPopover extends Base
             $this->flash->failure(t('Unable to update your task.'));
         }
 
-        $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $values['project_id'])));
+        $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $values['project_id'])), true);
     }
 
     /**
@@ -81,7 +81,7 @@ class BoardPopover extends Base
             $this->flash->failure(t('Unable to update your task.'));
         }
 
-        $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $values['project_id'])));
+        $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $values['project_id'])), true);
     }
 
     /**
diff --git a/app/Controller/Task.php b/app/Controller/Task.php
index 902a32d6..072df87b 100644
--- a/app/Controller/Task.php
+++ b/app/Controller/Task.php
@@ -151,7 +151,7 @@ class Task extends Base
     {
         $task = $this->getTask();
 
-        if (! $this->taskPermission->canRemoveTask($task)) {
+        if (! $this->helper->user->canRemoveTask($task)) {
             $this->forbidden();
         }
 
@@ -164,7 +164,7 @@ class Task extends Base
                 $this->flash->failure(t('Unable to remove this task.'));
             }
 
-            $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $task['project_id'])));
+            $this->response->redirect($this->helper->url->to('board', 'show', array('project_id' => $task['project_id'])), true);
         }
 
         $this->response->html($this->template->render('task/remove', array(
diff --git a/app/Core/Base.php b/app/Core/Base.php
index 2b619af5..c065ea2a 100644
--- a/app/Core/Base.php
+++ b/app/Core/Base.php
@@ -92,7 +92,6 @@ use Pimple\Container;
  * @property \Kanboard\Model\TaskFinder                                 $taskFinder
  * @property \Kanboard\Model\TaskLink                                   $taskLink
  * @property \Kanboard\Model\TaskModification                           $taskModification
- * @property \Kanboard\Model\TaskPermission                             $taskPermission
  * @property \Kanboard\Model\TaskPosition                               $taskPosition
  * @property \Kanboard\Model\TaskStatus                                 $taskStatus
  * @property \Kanboard\Model\TaskMetadata                               $taskMetadata
diff --git a/app/Helper/UserHelper.php b/app/Helper/UserHelper.php
index c3369dfd..b39d7b62 100644
--- a/app/Helper/UserHelper.php
+++ b/app/Helper/UserHelper.php
@@ -3,6 +3,7 @@
 namespace Kanboard\Helper;
 
 use Kanboard\Core\Base;
+use Kanboard\Core\Security\Role;
 
 /**
  * User helpers
@@ -42,6 +43,17 @@ class UserHelper extends Base
     }
 
     /**
+     * Return the user full name
+     *
+     * @param  array    $user   User properties
+     * @return string
+     */
+    public function getFullname(array $user = array())
+    {
+        return $this->user->getFullname(empty($user) ? $this->userSession->getAll() : $user);
+    }
+
+    /**
      * Get user id
      *
      * @access public
@@ -149,13 +161,24 @@ class UserHelper extends Base
     }
 
     /**
-     * Return the user full name
+     * Return true if the user can remove a task
      *
-     * @param  array    $user   User properties
-     * @return string
+     * Regular users can't remove tasks from other people
+     *
+     * @public
+     * @param  array $task
+     * @return bool
      */
-    public function getFullname(array $user = array())
+    public function canRemoveTask(array $task)
     {
-        return $this->user->getFullname(empty($user) ? $this->userSession->getAll() : $user);
+        if (isset($task['creator_id']) && $task['creator_id'] == $this->userSession->getId()) {
+            return true;
+        }
+        
+        if ($this->userSession->isAdmin() || $this->getProjectUserRole($task['project_id']) === Role::PROJECT_MANAGER) {
+            return true;
+        }
+
+        return false;
     }
 }
diff --git a/app/Model/TaskFinder.php b/app/Model/TaskFinder.php
index a1aa0f58..28ddb88f 100644
--- a/app/Model/TaskFinder.php
+++ b/app/Model/TaskFinder.php
@@ -71,6 +71,8 @@ class TaskFinder extends Base
                         'tasks.priority',
                         'tasks.time_spent',
                         'tasks.time_estimated',
+                        'tasks.is_active',
+                        'tasks.creator_id',
                         'projects.name AS project_name'
                     )
                     ->join(Project::TABLE, 'id', 'project_id')
diff --git a/app/Model/TaskPermission.php b/app/Model/TaskPermission.php
deleted file mode 100644
index b1e02589..00000000
--- a/app/Model/TaskPermission.php
+++ /dev/null
@@ -1,34 +0,0 @@
-<?php
-
-namespace Kanboard\Model;
-
-use Kanboard\Core\Security\Role;
-
-/**
- * Task permission model
- *
- * @package  model
- * @author   Frederic Guillot
- */
-class TaskPermission extends Base
-{
-    /**
-     * Return true if the user can remove a task
-     *
-     * Regular users can't remove tasks from other people
-     *
-     * @public
-     * @param  array $task
-     * @return bool
-     */
-    public function canRemoveTask(array $task)
-    {
-        if ($this->userSession->isAdmin() || $this->projectUserRole->getUserRole($task['project_id'], $this->userSession->getId()) === Role::PROJECT_MANAGER) {
-            return true;
-        } elseif (isset($task['creator_id']) && $task['creator_id'] == $this->userSession->getId()) {
-            return true;
-        }
-
-        return false;
-    }
-}
diff --git a/app/ServiceProvider/ClassProvider.php b/app/ServiceProvider/ClassProvider.php
index 18c1d578..54e2ad78 100644
--- a/app/ServiceProvider/ClassProvider.php
+++ b/app/ServiceProvider/ClassProvider.php
@@ -63,7 +63,6 @@ class ClassProvider implements ServiceProviderInterface
             'TaskFile',
             'TaskLink',
             'TaskModification',
-            'TaskPermission',
             'TaskPosition',
             'TaskStatus',
             'TaskMetadata',
diff --git a/app/Template/board/task_menu.php b/app/Template/board/task_menu.php
deleted file mode 100644
index c0d97cda..00000000
--- a/app/Template/board/task_menu.php
+++ /dev/null
@@ -1,18 +0,0 @@
-<span class="dropdown">
-    <a href="#" class="dropdown-menu"><?= '#'.$task['id'] ?> <i class="fa fa-caret-down"></i></a>
-    <ul>
-        <li><i class="fa fa-user fa-fw"></i>&nbsp;<?= $this->url->link(t('Change assignee'), 'BoardPopover', 'changeAssignee', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-tag fa-fw"></i>&nbsp;<?= $this->url->link(t('Change category'), 'BoardPopover', 'changeCategory', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-align-left fa-fw"></i>&nbsp;<?= $this->url->link(t('Change description'), 'taskmodification', 'description', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-pencil-square-o fa-fw"></i>&nbsp;<?= $this->url->link(t('Edit this task'), 'taskmodification', 'edit', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-comment-o fa-fw"></i>&nbsp;<?= $this->url->link(t('Add a comment'), 'comment', 'create', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-code-fork fa-fw"></i>&nbsp;<?= $this->url->link(t('Add internal link'), 'TaskInternalLink', 'create', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-external-link fa-fw"></i>&nbsp;<?= $this->url->link(t('Add external link'), 'TaskExternalLink', 'find', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <li><i class="fa fa-camera fa-fw"></i>&nbsp;<?= $this->url->link(t('Add a screenshot'), 'BoardPopover', 'screenshot', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <?php if ($task['is_active'] == 1): ?>
-            <li><i class="fa fa-close fa-fw"></i>&nbsp;<?= $this->url->link(t('Close this task'), 'taskstatus', 'close', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <?php else: ?>
-            <li><i class="fa fa-check-square-o fa-fw"></i>&nbsp;<?= $this->url->link(t('Open this task'), 'taskstatus', 'open', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?></li>
-        <?php endif ?>
-    </ul>
-</span>
\ No newline at end of file
diff --git a/app/Template/board/task_private.php b/app/Template/board/task_private.php
index 19bcbcfa..57623042 100644
--- a/app/Template/board/task_private.php
+++ b/app/Template/board/task_private.php
@@ -17,7 +17,7 @@
         <div class="task-board-collapsed">
             <div class="task-board-saving-icon" style="display: none;"><i class="fa fa-spinner fa-pulse"></i></div>
             <?php if ($this->user->hasProjectAccess('taskmodification', 'edit', $task['project_id'])): ?>
-                <?= $this->render('board/task_menu', array('task' => $task)) ?>
+                <?= $this->render('task/dropdown', array('task' => $task)) ?>
             <?php else: ?>
                 <strong><?= '#'.$task['id'] ?></strong>
             <?php endif ?>
@@ -33,7 +33,7 @@
         <div class="task-board-expanded">
             <div class="task-board-saving-icon" style="display: none;"><i class="fa fa-spinner fa-pulse fa-2x"></i></div>
             <?php if ($this->user->hasProjectAccess('taskmodification', 'edit', $task['project_id'])): ?>
-                <?= $this->render('board/task_menu', array('task' => $task)) ?>
+                <?= $this->render('task/dropdown', array('task' => $task)) ?>
             <?php else: ?>
                 <strong><?= '#'.$task['id'] ?></strong>
             <?php endif ?>
diff --git a/app/Template/task/dropdown.php b/app/Template/task/dropdown.php
index 6fea3728..567249df 100644
--- a/app/Template/task/dropdown.php
+++ b/app/Template/task/dropdown.php
@@ -8,6 +8,14 @@
         </li>
         <?php endif ?>
         <li>
+            <i class="fa fa-user fa-fw"></i>
+            <?= $this->url->link(t('Change assignee'), 'BoardPopover', 'changeAssignee', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
+        </li>
+        <li>
+            <i class="fa fa-tag fa-fw"></i>
+            <?= $this->url->link(t('Change category'), 'BoardPopover', 'changeCategory', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
+        </li>
+        <li>
             <i class="fa fa-pencil-square-o fa-fw"></i>
             <?= $this->url->link(t('Edit the task'), 'taskmodification', 'edit', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
         </li>
@@ -32,6 +40,10 @@
             <?= $this->url->link(t('Add a comment'), 'comment', 'create', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
         </li>
         <li>
+            <i class="fa fa-camera fa-fw"></i>
+            <?= $this->url->link(t('Add a screenshot'), 'BoardPopover', 'screenshot', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
+        </li>
+        <li>
             <i class="fa fa-files-o fa-fw"></i>
             <?= $this->url->link(t('Duplicate'), 'taskduplication', 'duplicate', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
         </li>
@@ -43,6 +55,12 @@
             <i class="fa fa-clone fa-fw"></i>
             <?= $this->url->link(t('Move to another project'), 'taskduplication', 'move', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
         </li>
+        <?php if ($this->user->canRemoveTask($task)): ?>
+            <li>
+                <i class="fa fa-trash-o fa-fw"></i>
+                <?= $this->url->link(t('Remove'), 'task', 'remove', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
+            </li>
+        <?php endif ?>
         <?php if (isset($task['is_active'])): ?>
         <li>
             <?php if ($task['is_active'] == 1): ?>
diff --git a/app/Template/task/remove.php b/app/Template/task/remove.php
index eb0809b1..b869b646 100644
--- a/app/Template/task/remove.php
+++ b/app/Template/task/remove.php
@@ -8,8 +8,8 @@
     </p>
 
     <div class="form-actions">
-        <?= $this->url->link(t('Yes'), 'task', 'remove', array('task_id' => $task['id'], 'project_id' => $task['project_id'], 'confirmation' => 'yes'), true, 'btn btn-red') ?>
+        <?= $this->url->link(t('Yes'), 'task', 'remove', array('task_id' => $task['id'], 'project_id' => $task['project_id'], 'confirmation' => 'yes'), true, 'btn btn-red popover-link') ?>
         <?= t('or') ?>
         <?= $this->url->link(t('cancel'), 'task', 'show', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'close-popover') ?>
     </div>
-</div>
\ No newline at end of file
+</div>
diff --git a/app/Template/task/sidebar.php b/app/Template/task/sidebar.php
index a2d73b8c..46f9e1a2 100644
--- a/app/Template/task/sidebar.php
+++ b/app/Template/task/sidebar.php
@@ -87,7 +87,7 @@
                 <?= $this->url->link(t('Open this task'), 'taskstatus', 'open', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
             <?php endif ?>
         </li>
-        <?php if ($this->task->canRemove($task)): ?>
+        <?php if ($this->user->canRemoveTask($task)): ?>
             <li>
                 <i class="fa fa-trash-o fa-fw"></i>
                 <?= $this->url->link(t('Remove'), 'task', 'remove', array('task_id' => $task['id'], 'project_id' => $task['project_id']), false, 'popover') ?>
diff --git a/tests/units/Helper/UserHelperTest.php b/tests/units/Helper/UserHelperTest.php
index 9a9832b2..135f8ca6 100644
--- a/tests/units/Helper/UserHelperTest.php
+++ b/tests/units/Helper/UserHelperTest.php
@@ -2,11 +2,15 @@
 
 require_once __DIR__.'/../Base.php';
 
+use Kanboard\Core\User\UserSession;
 use Kanboard\Helper\UserHelper;
 use Kanboard\Model\Project;
 use Kanboard\Model\ProjectUserRole;
+use Kanboard\Model\TaskCreation;
+use Kanboard\Model\TaskFinder;
 use Kanboard\Model\User as UserModel;
 use Kanboard\Core\Security\Role;
+use Kanboard\Model\User;
 
 class UserHelperTest extends Base
 {
@@ -228,4 +232,94 @@ class UserHelperTest extends Base
         $this->assertFalse($helper->hasProjectAccess('task', 'show', 2));
         $this->assertFalse($helper->hasProjectAccess('taskcreation', 'save', 2));
     }
+
+    public function testCanRemoveTask()
+    {
+        $taskCreationModel = new TaskCreation($this->container);
+        $taskFinderModel = new TaskFinder($this->container);
+        $helper = new UserHelper($this->container);
+        $projectModel = new Project($this->container);
+        $userModel = new User($this->container);
+        $userSessionModel = new UserSession($this->container);
+
+        $this->assertNotFalse($userModel->create(array('username' => 'toto', 'password' => '123456')));
+        $this->assertNotFalse($userModel->create(array('username' => 'toto2', 'password' => '123456')));
+        $this->assertEquals(1, $projectModel->create(array('name' => 'Project #1')));
+        $this->assertEquals(1, $taskCreationModel->create(array('title' => 'Task #1', 'project_id' => 1, 'creator_id' => 1)));
+        $this->assertEquals(2, $taskCreationModel->create(array('title' => 'Task #2', 'project_id' => 1, 'creator_id' => 2)));
+        $this->assertEquals(3, $taskCreationModel->create(array('title' => 'Task #3', 'project_id' => 1, 'creator_id' => 3)));
+        $this->assertEquals(4, $taskCreationModel->create(array('title' => 'Task #4', 'project_id' => 1)));
+
+        // User #1 can remove everything
+        $user = $userModel->getById(1);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(1);
+        $this->assertNotEmpty($task);
+        $this->assertTrue($helper->canRemoveTask($task));
+
+        // User #2 can't remove the task #1
+        $user = $userModel->getById(2);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(1);
+        $this->assertNotEmpty($task);
+        $this->assertFalse($helper->canRemoveTask($task));
+
+        // User #1 can remove everything
+        $user = $userModel->getById(1);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(2);
+        $this->assertNotEmpty($task);
+        $this->assertTrue($helper->canRemoveTask($task));
+
+        // User #2 can remove his own task
+        $user = $userModel->getbyId(2);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(2);
+        $this->assertNotEmpty($task);
+        $this->assertTrue($helper->canRemoveTask($task));
+
+        // User #1 can remove everything
+        $user = $userModel->getById(1);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(3);
+        $this->assertNotEmpty($task);
+        $this->assertTrue($helper->canRemoveTask($task));
+
+        // User #2 can't remove the task #3
+        $user = $userModel->getById(2);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(3);
+        $this->assertNotEmpty($task);
+        $this->assertFalse($helper->canRemoveTask($task));
+
+        // User #1 can remove everything
+        $user = $userModel->getById(1);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(4);
+        $this->assertNotEmpty($task);
+        $this->assertTrue($helper->canRemoveTask($task));
+
+        // User #2 can't remove the task #4
+        $user = $userModel->getById(2);
+        $this->assertNotEmpty($user);
+        $userSessionModel->initialize($user);
+
+        $task = $taskFinderModel->getById(4);
+        $this->assertNotEmpty($task);
+        $this->assertFalse($helper->canRemoveTask($task));
+    }
 }
diff --git a/tests/units/Model/TaskPermissionTest.php b/tests/units/Model/TaskPermissionTest.php
deleted file mode 100644
index 82cd581e..00000000
--- a/tests/units/Model/TaskPermissionTest.php
+++ /dev/null
@@ -1,103 +0,0 @@
-<?php
-
-require_once __DIR__.'/../Base.php';
-
-use Kanboard\Model\TaskCreation;
-use Kanboard\Model\TaskFinder;
-use Kanboard\Model\TaskPermission;
-use Kanboard\Model\Project;
-use Kanboard\Model\User;
-use Kanboard\Core\User\UserSession;
-
-class TaskPermissionTest extends Base
-{
-    public function testPrepareCreation()
-    {
-        $tc = new TaskCreation($this->container);
-        $tf = new TaskFinder($this->container);
-        $tp = new TaskPermission($this->container);
-        $p = new Project($this->container);
-        $u = new User($this->container);
-        $us = new UserSession($this->container);
-
-        $this->assertNotFalse($u->create(array('username' => 'toto', 'password' => '123456')));
-        $this->assertNotFalse($u->create(array('username' => 'toto2', 'password' => '123456')));
-        $this->assertEquals(1, $p->create(array('name' => 'Project #1')));
-        $this->assertEquals(1, $tc->create(array('title' => 'Task #1', 'project_id' => 1, 'creator_id' => 1)));
-        $this->assertEquals(2, $tc->create(array('title' => 'Task #2', 'project_id' => 1, 'creator_id' => 2)));
-        $this->assertEquals(3, $tc->create(array('title' => 'Task #3', 'project_id' => 1, 'creator_id' => 3)));
-        $this->assertEquals(4, $tc->create(array('title' => 'Task #4', 'project_id' => 1)));
-
-        // User #1 can remove everything
-        $user = $u->getbyId(1);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(1);
-        $this->assertNotEmpty($task);
-        $this->assertTrue($tp->canRemoveTask($task));
-
-        // User #2 can't remove the task #1
-        $user = $u->getbyId(2);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(1);
-        $this->assertNotEmpty($task);
-        $this->assertFalse($tp->canRemoveTask($task));
-
-        // User #1 can remove everything
-        $user = $u->getbyId(1);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(2);
-        $this->assertNotEmpty($task);
-        $this->assertTrue($tp->canRemoveTask($task));
-
-        // User #2 can remove his own task
-        $user = $u->getbyId(2);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(2);
-        $this->assertNotEmpty($task);
-        $this->assertTrue($tp->canRemoveTask($task));
-
-        // User #1 can remove everything
-        $user = $u->getbyId(1);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(3);
-        $this->assertNotEmpty($task);
-        $this->assertTrue($tp->canRemoveTask($task));
-
-        // User #2 can't remove the task #3
-        $user = $u->getbyId(2);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(3);
-        $this->assertNotEmpty($task);
-        $this->assertFalse($tp->canRemoveTask($task));
-
-        // User #1 can remove everything
-        $user = $u->getbyId(1);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(4);
-        $this->assertNotEmpty($task);
-        $this->assertTrue($tp->canRemoveTask($task));
-
-        // User #2 can't remove the task #4
-        $user = $u->getbyId(2);
-        $this->assertNotEmpty($user);
-        $us->initialize($user);
-
-        $task = $tf->getbyId(4);
-        $this->assertNotEmpty($task);
-        $this->assertFalse($tp->canRemoveTask($task));
-    }
-}