diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php index 38fcb30caa..fc9aa17381 100644 --- a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php +++ b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php @@ -15,9 +15,11 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { $t2 = $this->newTask($viewer, 'Task 2'); $t3 = $this->newTask($viewer, 'Task 3'); + $auto_base = min(mpull(array($t1, $t2, $t3), 'getID')); + // Default order should be reverse creation. - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -26,7 +28,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 to the middle. $this->moveTask($viewer, $t3, $t2, true); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -35,7 +37,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 to the end. $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -44,7 +46,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Repeat the move above, there should be no overall change in order. $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -53,7 +55,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 to the first slot in the priority. $this->movePriority($viewer, $t3, $t3->getPriority(), false); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -62,7 +64,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 to the last slot in the priority. $this->movePriority($viewer, $t3, $t3->getPriority(), true); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -71,7 +73,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 before T2. $this->moveTask($viewer, $t3, $t2, false); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -80,7 +82,7 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { // Move T3 before T1. $this->moveTask($viewer, $t3, $t1, false); - $tasks = $this->loadTasks($viewer); + $tasks = $this->loadTasks($viewer, $auto_base); $t1 = $tasks[1]; $t2 = $tasks[2]; $t3 = $tasks[3]; @@ -88,6 +90,43 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { } + public function testTaskAdjacentBlocks() { + $viewer = $this->generateNewTestUser(); + + $t = array(); + for ($ii = 1; $ii < 10; $ii++) { + $t[$ii] = $this->newTask($viewer, "Task Block {$ii}"); + + // This makes sure this test remains meaningful if we begin assigning + // subpriorities when tasks are created. + $t[$ii]->setSubpriority(0)->save(); + } + + $auto_base = min(mpull($t, 'getID')); + + $tasks = $this->loadTasks($viewer, $auto_base); + $this->assertEqual( + array(9, 8, 7, 6, 5, 4, 3, 2, 1), + array_keys($tasks)); + + $this->moveTask($viewer, $t[9], $t[8], true); + $tasks = $this->loadTasks($viewer, $auto_base); + $this->assertEqual( + array(8, 9, 7, 6, 5, 4, 3, 2, 1), + array_keys($tasks)); + + // When there is a large block of tasks which all have the same + // subpriority, they should be assigned distinct subpriorities as a + // side effect of having a task moved into the block. + + $subpri = mpull($tasks, 'getSubpriority'); + $unique_subpri = array_unique($subpri); + $this->assertEqual( + 9, + count($subpri), + 'Expected subpriorities to be distributed.'); + } + private function newTask(PhabricatorUser $viewer, $title) { $task = ManiphestTask::initializeNewTask($viewer); @@ -103,15 +142,23 @@ final class ManiphestTaskTestCase extends PhabricatorTestCase { return $task; } - private function loadTasks(PhabricatorUser $viewer) { + private function loadTasks(PhabricatorUser $viewer, $auto_base) { $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) ->execute(); - $tasks = mpull($tasks, null, 'getID'); + // NOTE: AUTO_INCREMENT changes survive ROLLBACK, and we can't throw them + // away without committing the current transaction, so we adjust the + // apparent task IDs as though the first one had been ID 1. This makes the + // tests a little easier to understand. - return $tasks; + $map = array(); + foreach ($tasks as $task) { + $map[($task->getID() - $auto_base) + 1] = $task; + } + + return $map; } private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) { diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index d9ce3dfbba..c091198905 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -679,21 +679,57 @@ final class ManiphestTransactionEditor // return the result. if ($adjacent) { // If the adjacent task has a subpriority that is identical or very - // close to the task we're looking at, we're going to move it a little - // farther down the subpriority list. + // close to the task we're looking at, we're going to move it and all + // tasks with the same subpriority a little farther down the subpriority + // scale. if (abs($adjacent->getSubpriority() - $base) < 0.01) { + $conn_w = $adjacent->establishConnection('w'); + + // Get all of the tasks with the same subpriority as the adjacent + // task, including the adjacent task itself. + $shift_base = $adjacent->getSubpriority(); + $shift_all = id(new ManiphestTaskQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->withPriorities(array($adjacent->getPriority())) + ->withSubpriorities(array($shift_base)) + ->setReversePaging(!$is_after) + ->execute(); + $shift_last = last($shift_all); + + // Find the subpriority before or after the task at the end of the + // block. list($shift_pri, $shift_sub) = self::getAdjacentSubpriority( - $adjacent, + $shift_last, $is_after); - queryfx( - $adjacent->establishConnection('r'), - 'UPDATE %T SET subpriority = %f WHERE id = %d', - $adjacent->getTableName(), - $shift_sub, - $adjacent->getID()); + $delta = ($shift_sub - $shift_base); + $count = count($shift_all); - $adjacent->setSubpriority($shift_sub); + $shift = array(); + $cursor = 1; + foreach ($shift_all as $shift_task) { + $shift_target = $shift_base + (($cursor / $count) * $delta); + $cursor++; + + queryfx( + $conn_w, + 'UPDATE %T SET subpriority = %f WHERE id = %d', + $adjacent->getTableName(), + $shift_target, + $shift_task->getID()); + + // If we're shifting the adjacent task, update it. + if ($shift_task->getID() == $adjacent->getID()) { + $adjacent->setSubpriority($shift_target); + } + + // If we're shifting the original target task, update the base + // subpriority. + if ($shift_task->getID() == $dst->getID()) { + $base = $shift_target; + } + } } $sub = ($adjacent->getSubpriority() + $base) / 2; diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 585ff27ee9..dc1f52a22c 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -37,6 +37,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $statuses; private $priorities; + private $subpriorities; private $groupBy = 'group-none'; const GROUP_NONE = 'group-none'; @@ -137,6 +138,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $this; } + public function withSubpriorities(array $subpriorities) { + $this->subpriorities = $subpriorities; + return $this; + } + public function withSubscribers(array $subscribers) { $this->subscriberPHIDs = $subscribers; return $this; @@ -506,6 +512,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $this->priorities); } + if ($this->subpriorities) { + return qsprintf( + $conn, + 'task.subpriority IN (%Lf)', + $this->subpriorities); + } + return null; } diff --git a/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php b/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php index 8edd4ade01..d92e8b206a 100644 --- a/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php +++ b/src/infrastructure/testing/fixture/PhabricatorStorageFixtureScopeGuard.php @@ -11,7 +11,7 @@ final class PhabricatorStorageFixtureScopeGuard { $this->name = $name; execx( - 'php %s upgrade --force --namespace %s', + 'php %s upgrade --force --no-adjust --namespace %s', $this->getStorageBinPath(), $this->name);