diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1782,7 +1782,6 @@ 'ManiphestTaskSubpriorityTransaction' => 'applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php', 'ManiphestTaskSubtaskController' => 'applications/maniphest/controller/ManiphestTaskSubtaskController.php', 'ManiphestTaskSubtypeDatasource' => 'applications/maniphest/typeahead/ManiphestTaskSubtypeDatasource.php', - 'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php', 'ManiphestTaskTitleHeraldField' => 'applications/maniphest/herald/ManiphestTaskTitleHeraldField.php', 'ManiphestTaskTitleTransaction' => 'applications/maniphest/xaction/ManiphestTaskTitleTransaction.php', 'ManiphestTaskTransactionType' => 'applications/maniphest/xaction/ManiphestTaskTransactionType.php', @@ -7501,7 +7500,6 @@ 'ManiphestTaskSubpriorityTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskSubtaskController' => 'ManiphestController', 'ManiphestTaskSubtypeDatasource' => 'PhabricatorTypeaheadDatasource', - 'ManiphestTaskTestCase' => 'PhabricatorTestCase', 'ManiphestTaskTitleHeraldField' => 'ManiphestTaskHeraldField', 'ManiphestTaskTitleTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskTransactionType' => 'PhabricatorModularTransactionType', diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php deleted file mode 100644 --- a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php +++ /dev/null @@ -1,255 +0,0 @@ - true, - ); - } - - public function testTaskReordering() { - $viewer = $this->generateNewTestUser(); - - $t1 = $this->newTask($viewer, pht('Task 1')); - $t2 = $this->newTask($viewer, pht('Task 2')); - $t3 = $this->newTask($viewer, pht('Task 3')); - - $auto_base = min(mpull(array($t1, $t2, $t3), 'getID')); - - - // Default order should be reverse creation. - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 to the middle. - $this->moveTask($viewer, $t3, $t2, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 3, 1), array_keys($tasks)); - - - // Move T3 to the end. - $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Repeat the move above, there should be no overall change in order. - $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Move T3 to the first slot in the priority. - $this->movePriority($viewer, $t3, $t3->getPriority(), false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 to the last slot in the priority. - $this->movePriority($viewer, $t3, $t3->getPriority(), true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Move T3 before T2. - $this->moveTask($viewer, $t3, $t2, false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 before T1. - $this->moveTask($viewer, $t3, $t1, false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 3, 1), array_keys($tasks)); - - } - - public function testTaskAdjacentBlocks() { - $viewer = $this->generateNewTestUser(); - - $t = array(); - for ($ii = 1; $ii < 10; $ii++) { - $t[$ii] = $this->newTask($viewer, pht('Task Block %d', $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), - pht('Expected subpriorities to be distributed.')); - - // Move task 9 to the end. - $this->moveTask($viewer, $t[9], $t[1], true); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 3, 2, 1, 9), - array_keys($tasks)); - - // Move task 3 to the beginning. - $this->moveTask($viewer, $t[3], $t[8], false); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(3, 8, 7, 6, 5, 4, 2, 1, 9), - array_keys($tasks)); - - // Move task 3 to the end. - $this->moveTask($viewer, $t[3], $t[9], true); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 2, 1, 9, 3), - array_keys($tasks)); - - // Move task 5 to before task 4 (this is its current position). - $this->moveTask($viewer, $t[5], $t[4], false); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 2, 1, 9, 3), - array_keys($tasks)); - } - - private function newTask(PhabricatorUser $viewer, $title) { - $task = ManiphestTask::initializeNewTask($viewer); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskTitleTransaction::TRANSACTIONTYPE) - ->setNewValue($title); - - - $this->applyTaskTransactions($viewer, $task, $xactions); - - return $task; - } - - private function loadTasks(PhabricatorUser $viewer, $auto_base) { - $tasks = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) - ->execute(); - - // 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. - - $map = array(); - foreach ($tasks as $task) { - $map[($task->getID() - $auto_base) + 1] = $task; - } - - return $map; - } - - private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) { - list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $dst, - $is_after); - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head($keyword_map[$pri]); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - return $this->applyTaskTransactions($viewer, $src, $xactions); - } - - private function movePriority( - PhabricatorUser $viewer, - $src, - $target_priority, - $is_end) { - - list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( - $target_priority, - $is_end); - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head($keyword_map[$pri]); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - return $this->applyTaskTransactions($viewer, $src, $xactions); - } - - private function applyTaskTransactions( - PhabricatorUser $viewer, - ManiphestTask $task, - array $xactions) { - - $content_source = $this->newContentSource(); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($viewer) - ->setContentSource($content_source) - ->setContinueOnNoEffect(true) - ->applyTransactions($task, $xactions); - - return $task; - } - -} diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -297,251 +297,6 @@ return $copy; } - /** - * Get priorities for moving a task to a new priority. - */ - public static function getEdgeSubpriority( - $priority, - $is_end) { - - $query = id(new ManiphestTaskQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPriorities(array($priority)) - ->setLimit(1); - - if ($is_end) { - $query->setOrderVector(array('-priority', '-subpriority', '-id')); - } else { - $query->setOrderVector(array('priority', 'subpriority', 'id')); - } - - $result = $query->executeOne(); - $step = (double)(2 << 32); - - if ($result) { - $base = $result->getSubpriority(); - if ($is_end) { - $sub = ($base - $step); - } else { - $sub = ($base + $step); - } - } else { - $sub = 0; - } - - return array($priority, $sub); - } - - - /** - * Get priorities for moving a task before or after another task. - */ - public static function getAdjacentSubpriority( - ManiphestTask $dst, - $is_after) { - - $query = id(new ManiphestTaskQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) - ->withPriorities(array($dst->getPriority())) - ->setLimit(1); - - if ($is_after) { - $query->setAfterID($dst->getID()); - } else { - $query->setBeforeID($dst->getID()); - } - - $adjacent = $query->executeOne(); - - $base = $dst->getSubpriority(); - $step = (double)(2 << 32); - - // If we find an adjacent task, we average the two subpriorities and - // return the result. - if ($adjacent) { - $epsilon = 1.0; - - // If the adjacent task has a subpriority that is identical or very - // close to the task we're looking at, we're going to spread out all - // the nearby tasks. - - $adjacent_sub = $adjacent->getSubpriority(); - if ((abs($adjacent_sub - $base) < $epsilon)) { - $base = self::disperseBlock( - $dst, - $epsilon * 2); - if ($is_after) { - $sub = $base - $epsilon; - } else { - $sub = $base + $epsilon; - } - } else { - $sub = ($adjacent_sub + $base) / 2; - } - } else { - // Otherwise, we take a step away from the target's subpriority and - // use that. - if ($is_after) { - $sub = ($base - $step); - } else { - $sub = ($base + $step); - } - } - - return array($dst->getPriority(), $sub); - } - - /** - * Distribute a cluster of tasks with similar subpriorities. - */ - private static function disperseBlock( - ManiphestTask $task, - $spacing) { - - $conn = $task->establishConnection('w'); - - // Find a block of subpriority space which is, on average, sparse enough - // to hold all the tasks that are inside it with a reasonable level of - // separation between them. - - // We'll start by looking near the target task for a range of numbers - // which has more space available than tasks. For example, if the target - // task has subpriority 33 and we want to separate each task by at least 1, - // we might start by looking in the range [23, 43]. - - // If we find fewer than 20 tasks there, we have room to reassign them - // with the desired level of separation. We space them out, then we're - // done. - - // However: if we find more than 20 tasks, we don't have enough room to - // distribute them. We'll widen our search and look in a bigger range, - // maybe [13, 53]. This range has more space, so if we find fewer than - // 40 tasks in this range we can spread them out. If we still find too - // many tasks, we keep widening the search. - - $base = $task->getSubpriority(); - - $scale = 4.0; - while (true) { - $range = ($spacing * $scale) / 2.0; - $min = ($base - $range); - $max = ($base + $range); - - $result = queryfx_one( - $conn, - 'SELECT COUNT(*) N FROM %T WHERE priority = %d AND - subpriority BETWEEN %f AND %f', - $task->getTableName(), - $task->getPriority(), - $min, - $max); - - $count = $result['N']; - if ($count < $scale) { - // We have found a block which we can make sparse enough, so bail and - // continue below with our selection. - break; - } - - // This block had too many tasks for its size, so try again with a - // bigger block. - $scale *= 2.0; - } - - $rows = queryfx_all( - $conn, - 'SELECT id FROM %T WHERE priority = %d AND - subpriority BETWEEN %f AND %f - ORDER BY priority, subpriority, id', - $task->getTableName(), - $task->getPriority(), - $min, - $max); - - $task_id = $task->getID(); - $result = null; - - // NOTE: In strict mode (which we encourage enabling) we can't structure - // this bulk update as an "INSERT ... ON DUPLICATE KEY UPDATE" unless we - // provide default values for ALL of the columns that don't have defaults. - - // This is gross, but we may be moving enough rows that individual - // queries are unreasonably slow. An alternate construction which might - // be worth evaluating is to use "CASE". Another approach is to disable - // strict mode for this query. - - $default_str = qsprintf($conn, '%s', ''); - $default_int = qsprintf($conn, '%d', 0); - - $extra_columns = array( - 'phid' => $default_str, - 'authorPHID' => $default_str, - 'status' => $default_str, - 'priority' => $default_int, - 'title' => $default_str, - 'description' => $default_str, - 'dateCreated' => $default_int, - 'dateModified' => $default_int, - 'mailKey' => $default_str, - 'viewPolicy' => $default_str, - 'editPolicy' => $default_str, - 'ownerOrdering' => $default_str, - 'spacePHID' => $default_str, - 'bridgedObjectPHID' => $default_str, - 'properties' => $default_str, - 'points' => $default_int, - 'subtype' => $default_str, - ); - - $sql = array(); - $offset = 0; - - // Often, we'll have more room than we need in the range. Distribute the - // tasks evenly over the whole range so that we're less likely to end up - // with tasks spaced exactly the minimum distance apart, which may - // get shifted again later. We have one fewer space to distribute than we - // have tasks. - $divisor = (double)(count($rows) - 1.0); - if ($divisor > 0) { - $available_distance = (($max - $min) / $divisor); - } else { - $available_distance = 0.0; - } - - foreach ($rows as $row) { - $subpriority = $min + ($offset * $available_distance); - - // If this is the task that we're spreading out relative to, keep track - // of where it is ending up so we can return the new subpriority. - $id = $row['id']; - if ($id == $task_id) { - $result = $subpriority; - } - - $sql[] = qsprintf( - $conn, - '(%d, %LQ, %f)', - $id, - $extra_columns, - $subpriority); - - $offset++; - } - - foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { - queryfx( - $conn, - 'INSERT INTO %T (id, %LC, subpriority) VALUES %LQ - ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)', - $task->getTableName(), - array_keys($extra_columns), - $chunk); - } - - return $result; - } - protected function validateAllTransactions( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php @@ -14,7 +14,6 @@ $author = id(new PhabricatorUser()) ->loadOneWhere('phid = %s', $author_phid); $task = ManiphestTask::initializeNewTask($author) - ->setSubPriority($this->generateTaskSubPriority()) ->setTitle($this->generateTitle()); $content_source = $this->getLipsumContentSource(); @@ -106,10 +105,6 @@ return $keyword; } - public function generateTaskSubPriority() { - return rand(2 << 16, 2 << 32); - } - public function generateTaskStatus() { $statuses = array_keys(ManiphestTaskStatus::getTaskStatusMap()); // Make sure 4/5th of all generated Tasks are open diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -435,13 +435,6 @@ $this->priorities); } - if ($this->subpriorities !== null) { - $where[] = qsprintf( - $conn, - 'task.subpriority IN (%Lf)', - $this->subpriorities); - } - if ($this->bridgedObjectPHIDs !== null) { $where[] = qsprintf( $conn, @@ -844,7 +837,7 @@ public function getBuiltinOrders() { $orders = array( 'priority' => array( - 'vector' => array('priority', 'subpriority', 'id'), + 'vector' => array('priority', 'id'), 'name' => pht('Priority'), 'aliases' => array(self::ORDER_PRIORITY), ), @@ -919,11 +912,6 @@ 'type' => 'string', 'reverse' => true, ), - 'subpriority' => array( - 'table' => 'task', - 'column' => 'subpriority', - 'type' => 'float', - ), 'updated' => array( 'table' => 'task', 'column' => 'dateModified', @@ -948,7 +936,6 @@ $map = array( 'id' => $task->getID(), 'priority' => $task->getPriority(), - 'subpriority' => $task->getSubpriority(), 'owner' => $task->getOwnerOrdering(), 'status' => $task->getStatus(), 'title' => $task->getTitle(), diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -267,39 +267,6 @@ return ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD; } - private function comparePriorityTo(ManiphestTask $other) { - $upri = $this->getPriority(); - $vpri = $other->getPriority(); - - if ($upri != $vpri) { - return ($upri - $vpri); - } - - $usub = $this->getSubpriority(); - $vsub = $other->getSubpriority(); - - if ($usub != $vsub) { - return ($usub - $vsub); - } - - $uid = $this->getID(); - $vid = $other->getID(); - - if ($uid != $vid) { - return ($uid - $vid); - } - - return 0; - } - - public function isLowerPriorityThan(ManiphestTask $other) { - return ($this->comparePriorityTo($other) < 0); - } - - public function isHigherPriorityThan(ManiphestTask $other) { - return ($this->comparePriorityTo($other) > 0); - } - public function getWorkboardProperties() { return array( 'status' => $this->getStatus(), @@ -540,7 +507,6 @@ $priority_value = (int)$this->getPriority(); $priority_info = array( 'value' => $priority_value, - 'subpriority' => (double)$this->getSubpriority(), 'name' => ManiphestTaskPriority::getTaskPriorityName($priority_value), 'color' => ManiphestTaskPriority::getTaskPriorityColor($priority_value), ); diff --git a/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php --- a/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php @@ -6,16 +6,17 @@ const TRANSACTIONTYPE = 'subpriority'; public function generateOldValue($object) { - return $object->getSubpriority(); + return null; } public function applyInternalEffects($object, $value) { - $object->setSubpriority($value); + // This transaction is obsolete, but we're keeping the class around so it + // is hidden from timelines until we destroy the actual transaction data. + throw new PhutilMethodNotImplementedException(); } public function shouldHide() { return true; } - }