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

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(),

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);
+  }
+
 }