Page MenuHomePhabricator
Files F13189727

D20266.diff
No OneTemporary
Actions

D20266.diff
View Options

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 @@
-<?php
-
-final class ManiphestTaskTestCase extends PhabricatorTestCase {
-
- protected function getPhabricatorTestCaseConfiguration() {
- return array(
- self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => 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;
}
-
}

File Metadata

Mime Type
text/plain
Expires
Sun, May 12, 6:19 AM (1 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6273637
Default Alt Text
D20266.diff (21 KB)

Event Timeline

Phabricator · Built by Phacility