Page MenuHomePhabricator
Files F14416298

D12121.diff
No OneTemporary
Actions

D12121.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
@@ -1053,6 +1053,7 @@
'ManiphestTaskStatus' => 'applications/maniphest/constants/ManiphestTaskStatus.php',
'ManiphestTaskStatusDatasource' => 'applications/maniphest/typeahead/ManiphestTaskStatusDatasource.php',
'ManiphestTaskStatusTestCase' => 'applications/maniphest/constants/__tests__/ManiphestTaskStatusTestCase.php',
+ 'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php',
'ManiphestTransaction' => 'applications/maniphest/storage/ManiphestTransaction.php',
'ManiphestTransactionComment' => 'applications/maniphest/storage/ManiphestTransactionComment.php',
'ManiphestTransactionEditor' => 'applications/maniphest/editor/ManiphestTransactionEditor.php',
@@ -4292,6 +4293,7 @@
'ManiphestTaskStatus' => 'ManiphestConstants',
'ManiphestTaskStatusDatasource' => 'PhabricatorTypeaheadDatasource',
'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
+ 'ManiphestTaskTestCase' => 'PhabricatorTestCase',
'ManiphestTransaction' => 'PhabricatorApplicationTransaction',
'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment',
'ManiphestTransactionEditor' => 'PhabricatorApplicationTransactionEditor',
diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
new file mode 100644
--- /dev/null
+++ b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php
@@ -0,0 +1,174 @@
+<?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, 'Task 1');
+ $t2 = $this->newTask($viewer, 'Task 2');
+ $t3 = $this->newTask($viewer, 'Task 3');
+
+
+ // Default order should be reverse creation.
+ $tasks = $this->loadTasks($viewer);
+ $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);
+ $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);
+ $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);
+ $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);
+ $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);
+ $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);
+ $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);
+ $t1 = $tasks[1];
+ $t2 = $tasks[2];
+ $t3 = $tasks[3];
+ $this->assertEqual(array(2, 3, 1), array_keys($tasks));
+
+ }
+
+ private function newTask(PhabricatorUser $viewer, $title) {
+ $task = ManiphestTask::initializeNewTask($viewer);
+
+ $xactions = array();
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_TITLE)
+ ->setNewValue($title);
+
+
+ $this->applyTaskTransactions($viewer, $task, $xactions);
+
+ return $task;
+ }
+
+ private function loadTasks(PhabricatorUser $viewer) {
+ $tasks = id(new ManiphestTaskQuery())
+ ->setViewer($viewer)
+ ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
+ ->execute();
+
+ $tasks = mpull($tasks, null, 'getID');
+
+ return $tasks;
+ }
+
+ private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) {
+ list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority(
+ $dst,
+ $is_after);
+
+ $xactions = array();
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY)
+ ->setNewValue($pri);
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
+ ->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);
+
+ $xactions = array();
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY)
+ ->setNewValue($pri);
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
+ ->setNewValue($sub);
+
+ return $this->applyTaskTransactions($viewer, $src, $xactions);
+ }
+
+ private function applyTaskTransactions(
+ PhabricatorUser $viewer,
+ ManiphestTask $task,
+ array $xactions) {
+
+ $content_source = PhabricatorContentSource::newConsoleSource();
+
+ $editor = id(new ManiphestTransactionEditor())
+ ->setActor($viewer)
+ ->setContentSource($content_source)
+ ->setContinueOnNoEffect(true)
+ ->applyTransactions($task, $xactions);
+
+ return $task;
+ }
+
+}
diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php
--- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php
+++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php
@@ -32,21 +32,25 @@
if (!$after_task) {
return new Aphront404Response();
}
- $after_pri = $after_task->getPriority();
- $after_sub = $after_task->getSubpriority();
+ list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority(
+ $after_task,
+ $is_after = true);
} else {
- $after_pri = $request->getInt('priority');
- $after_sub = null;
+ list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority(
+ $request->getInt('priority'),
+ $is_end = false);
}
- $xactions = array(id(new ManiphestTransaction())
+ $xactions = array();
+
+ $xactions[] = id(new ManiphestTransaction())
+ ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY)
+ ->setNewValue($pri);
+
+ $xactions[] = id(new ManiphestTransaction())
->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
- ->setNewValue(array(
- 'newPriority' => $after_pri,
- 'newSubpriorityBase' => $after_sub,
- 'direction' => '>',
- )),
- );
+ ->setNewValue($sub);
+
$editor = id(new ManiphestTransactionEditor())
->setActor($user)
->setContinueOnMissingFields(true)
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
@@ -147,12 +147,7 @@
return $object->setOwnerPHID($phid);
case ManiphestTransaction::TYPE_SUBPRIORITY:
- $data = $xaction->getNewValue();
- $new_sub = $this->getNextSubpriority(
- $data['newPriority'],
- $data['newSubpriorityBase'],
- $data['direction']);
- $object->setSubpriority($new_sub);
+ $object->setSubpriority($xaction->getNewValue());
return;
case ManiphestTransaction::TYPE_PROJECT_COLUMN:
// these do external (edge) updates
@@ -165,28 +160,6 @@
}
}
- protected function expandTransaction(
- PhabricatorLiskDAO $object,
- PhabricatorApplicationTransaction $xaction) {
-
- $xactions = parent::expandTransaction($object, $xaction);
- switch ($xaction->getTransactionType()) {
- case ManiphestTransaction::TYPE_SUBPRIORITY:
- $data = $xaction->getNewValue();
- $new_pri = $data['newPriority'];
- if ($new_pri != $object->getPriority()) {
- $xactions[] = id(new ManiphestTransaction())
- ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY)
- ->setNewValue($new_pri);
- }
- break;
- default:
- break;
- }
-
- return $xactions;
- }
-
protected function applyCustomExternalTransaction(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
@@ -643,54 +616,99 @@
return $copy;
}
- private function getNextSubpriority($pri, $sub, $dir = '>') {
- switch ($dir) {
- case '>':
- $order = 'ASC';
- break;
- case '<':
- $order = 'DESC';
- break;
- default:
- throw new Exception('$dir must be ">" or "<".');
- break;
+ /**
+ * Get priorities for moving a task to a new priority.
+ */
+ public static function getEdgeSubpriority(
+ $priority,
+ $is_end) {
+
+ $query = id(new ManiphestTaskQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
+ ->withPriorities(array($priority))
+ ->setLimit(1);
+
+ if ($is_end) {
+ $query->setReversePaging(true);
}
- if ($sub === null) {
- $base = 0;
+ $result = $query->executeOne();
+ $step = (double)(2 << 32);
+
+ if ($result) {
+ $base = $result->getSubpriority();
+ if ($is_end) {
+ $sub = ($base - $step);
+ } else {
+ $sub = ($base + $step);
+ }
} else {
- $base = $sub;
+ $sub = 0;
}
- if ($sub === null) {
- $next = id(new ManiphestTask())->loadOneWhere(
- 'priority = %d ORDER BY subpriority %Q LIMIT 1',
- $pri,
- $order);
- if ($next) {
- if ($dir == '>') {
- return $next->getSubpriority() - ((double)(2 << 16));
- } else {
- return $next->getSubpriority() + ((double)(2 << 16));
- }
- }
+ 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())
+ ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY)
+ ->withPriorities(array($dst->getPriority()))
+ ->setLimit(1);
+
+ if ($is_after) {
+ $query->setAfterID($dst->getID());
} else {
- $next = id(new ManiphestTask())->loadOneWhere(
- 'priority = %d AND subpriority %Q %f ORDER BY subpriority %Q LIMIT 1',
- $pri,
- $dir,
- $sub,
- $order);
- if ($next) {
- return ($sub + $next->getSubpriority()) / 2;
- }
+ $query->setBeforeID($dst->getID());
}
- if ($dir == '>') {
- return $base + (double)(2 << 32);
+ $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) {
+ // 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.
+ if (abs($adjacent->getSubpriority() - $base) < 0.01) {
+ list($shift_pri, $shift_sub) = self::getAdjacentSubpriority(
+ $adjacent,
+ $is_after);
+
+ queryfx(
+ $adjacent->establishConnection('r'),
+ 'UPDATE %T SET subpriority = %f WHERE id = %d',
+ $adjacent->getTableName(),
+ $shift_sub,
+ $adjacent->getID());
+
+ $adjacent->setSubpriority($shift_sub);
+ }
+
+ $sub = ($adjacent->getSubpriority() + $base) / 2;
} else {
- return $base - (double)(2 << 32);
+ // 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);
}
+
}
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
@@ -21,6 +21,7 @@
private $dateCreatedBefore;
private $dateModifiedAfter;
private $dateModifiedBefore;
+ private $reversePaging;
private $fullTextSearch = '';
@@ -765,12 +766,12 @@
foreach ($app_order as $order_by) {
$order[] = $order_by;
}
+ }
- if ($reverse) {
- $order[] = 'task.id ASC';
- } else {
- $order[] = 'task.id DESC';
- }
+ if ($reverse) {
+ $order[] = 'task.id ASC';
+ } else {
+ $order[] = 'task.id DESC';
}
return 'ORDER BY '.implode(', ', $order);
@@ -1068,7 +1069,6 @@
'name' => 'task.subpriority',
'value' => $cursor->getSubpriority(),
'type' => 'float',
- 'reverse' => true,
);
$columns[] = array(
'name' => 'task.dateModified',
@@ -1120,4 +1120,13 @@
return 'PhabricatorManiphestApplication';
}
+ public function setReversePaging($reverse_paging) {
+ $this->reversePaging = $reverse_paging;
+ return $this;
+ }
+
+ protected function getReversePaging() {
+ return $this->reversePaging;
+ }
+
}
diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php
--- a/src/applications/project/controller/PhabricatorProjectMoveController.php
+++ b/src/applications/project/controller/PhabricatorProjectMoveController.php
@@ -112,40 +112,31 @@
}
$tasks = mpull($tasks, null, 'getPHID');
- $a_task = idx($tasks, $after_phid);
- $b_task = idx($tasks, $before_phid);
-
- if ($a_task &&
- (($a_task->getPriority() < $object->getPriority()) ||
- ($a_task->getPriority() == $object->getPriority() &&
- $a_task->getSubpriority() >= $object->getSubpriority()))) {
+ $try = array(
+ array($after_phid, true),
+ array($before_phid, false),
+ );
- $after_pri = $a_task->getPriority();
- $after_sub = $a_task->getSubpriority();
+ $pri = null;
+ $sub = null;
+ foreach ($try as $spec) {
+ list($task_phid, $is_after) = $spec;
+ $task = idx($tasks, $task_phid);
+ if ($task) {
+ list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority(
+ $task,
+ $is_after);
+ break;
+ }
+ }
+ if ($pri !== null) {
$xactions[] = id(new ManiphestTransaction())
- ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
- ->setNewValue(array(
- 'newPriority' => $after_pri,
- 'newSubpriorityBase' => $after_sub,
- 'direction' => '>',
- ));
-
- } else if ($b_task &&
- (($b_task->getPriority() > $object->getPriority()) ||
- ($b_task->getPriority() == $object->getPriority() &&
- $b_task->getSubpriority() <= $object->getSubpriority()))) {
-
- $before_pri = $b_task->getPriority();
- $before_sub = $b_task->getSubpriority();
-
+ ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY)
+ ->setNewValue($pri);
$xactions[] = id(new ManiphestTransaction())
->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
- ->setNewValue(array(
- 'newPriority' => $before_pri,
- 'newSubpriorityBase' => $before_sub,
- 'direction' => '<',
- ));
+ ->setNewValue($sub);
}
}

File Metadata

Mime Type
text/plain
Expires
Wed, Dec 25, 9:25 PM (11 h, 17 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6927664
Default Alt Text
D12121.diff (16 KB)

Event Timeline

Phabricator · Built by Phacility