From 968ac764532d2a863d46c655b5476153fa60b64e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Feb 2016 15:39:08 -0800 Subject: [PATCH] Don't adjust task priority after a workboard drag unless we need to Summary: Fixes T8197. Currently, if you priority-sort a workboard and drag a card to the top or bottom, we change the priority even if we do not need to. For example, if the lowest priority in a column is "Low", and you drag a "Wishlist" task underneath it, we incorrectly increase the priority of the task to "Low", when we do not actually need to touch it. This is bad/confusing. A similar thing happens when dragging a "High" priority task to the top of a column where the highest priority is currently "Normal". Test Plan: - Create a column with a "Normal" task. - Sort workboard by Priority. - Drag a "High" task above it. After patch: task still "High". - Drag a "Wishlist" task below it. After patch: task still "Wishlist". Also dragged a ton of tasks into the middle of other tasks. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8197 Differential Revision: https://secure.phabricator.com/D15240 --- .../maniphest/storage/ManiphestTask.php | 33 ++++ .../PhabricatorProjectMoveController.php | 149 ++++++++++++------ 2 files changed, 133 insertions(+), 49 deletions(-) diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 93a5f5562c..e90f0e806c 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -221,6 +221,39 @@ final class ManiphestTask extends ManiphestDAO ); } + 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(), diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 4aa9e0eec2..d3540a1781 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -90,55 +90,13 @@ final class PhabricatorProjectMoveController 'projectPHID' => $column->getProjectPHID(), )); - $task_phids = array(); - if ($after_phid) { - $task_phids[] = $after_phid; - } - if ($before_phid) { - $task_phids[] = $before_phid; - } - - if ($task_phids && ($order == PhabricatorProjectColumn::ORDER_PRIORITY)) { - $tasks = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withPHIDs($task_phids) - ->needProjectPHIDs(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->execute(); - if (count($tasks) != count($task_phids)) { - return new Aphront404Response(); - } - $tasks = mpull($tasks, null, 'getPHID'); - - $try = array( - array($after_phid, true), - array($before_phid, false), - ); - - $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_PRIORITY) - ->setNewValue($pri); - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) - ->setNewValue($sub); + if ($order == PhabricatorProjectColumn::ORDER_PRIORITY) { + $priority_xactions = $this->getPriorityTransactions( + $object, + $after_phid, + $before_phid); + foreach ($priority_xactions as $xaction) { + $xactions[] = $xaction; } } @@ -179,4 +137,97 @@ final class PhabricatorProjectMoveController return $this->newCardResponse($board_phid, $object_phid); } + private function getPriorityTransactions( + ManiphestTask $task, + $after_phid, + $before_phid) { + + list($after_task, $before_task) = $this->loadPriorityTasks( + $after_phid, + $before_phid); + + $must_move = false; + if ($after_task && !$task->isLowerPriorityThan($after_task)) { + $must_move = true; + } + + if ($before_task && !$task->isHigherPriorityThan($before_task)) { + $must_move = true; + } + + // The move doesn't require a priority change to be valid, so don't + // change the priority since we are not being forced to. + if (!$must_move) { + return array(); + } + + $try = array( + array($after_task, true), + array($before_task, false), + ); + + $pri = null; + $sub = null; + foreach ($try as $spec) { + list($task, $is_after) = $spec; + + if (!$task) { + continue; + } + + list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( + $task, + $is_after); + } + + $xactions = array(); + if ($pri !== null) { + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_PRIORITY) + ->setNewValue($pri); + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY) + ->setNewValue($sub); + } + + return $xactions; + } + + private function loadPriorityTasks($after_phid, $before_phid) { + $viewer = $this->getViewer(); + + $task_phids = array(); + + if ($after_phid) { + $task_phids[] = $after_phid; + } + if ($before_phid) { + $task_phids[] = $before_phid; + } + + if (!$task_phids) { + return array(null, null); + } + + $tasks = id(new ManiphestTaskQuery()) + ->setViewer($viewer) + ->withPHIDs($task_phids) + ->execute(); + $tasks = mpull($tasks, null, 'getPHID'); + + if ($after_phid) { + $after_task = idx($tasks, $after_phid); + } else { + $after_task = null; + } + + if ($before_phid) { + $before_task = idx($tasks, $before_phid); + } else { + $before_task = null; + } + + return array($after_task, $before_task); + } + }