Fix literally thousands of drag-to-reorder priority bugs

Summary:
Fixes T7563. Fixes T5201. Reframe this as two separate operations:

  - Move before or after a task.
  - Move to the beginning or end of a priority.

Then:

  - Make all the order queries unambiguous and properly reversible, with an explicit `id` order.
  - Just reuse `ManiphestTask` to get results in the correct order.
  - Simplify the actual transaction apply logic.
  - Detect and recover from cases where tasks have identical or similar subpriorities.

Test Plan:
  - Wrote and executed unit tests.
  - Dragged and dropped tasks within priorities and between priorities in the main Maniphest view.
  - Dragged and dropped tasks within priorities in the workboard view, when ordered by priority.
  - Also poked at the "natural" order, but that shouldn't be affected.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: chad, epriestley

Maniphest Tasks: T5201, T7563

Differential Revision: https://secure.phabricator.com/D12121

src/__phutil_library_map__.php

@@ -1053,6 +1053,7 @@ phutil_register_library_map(array(
     '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 @@ phutil_register_library_map(array(
     'ManiphestTaskStatus' => 'ManiphestConstants',
     'ManiphestTaskStatusDatasource' => 'PhabricatorTypeaheadDatasource',
     'ManiphestTaskStatusTestCase' => 'PhabricatorTestCase',
+    'ManiphestTaskTestCase' => 'PhabricatorTestCase',
     'ManiphestTransaction' => 'PhabricatorApplicationTransaction',
     'ManiphestTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'ManiphestTransactionEditor' => 'PhabricatorApplicationTransactionEditor',

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

src/applications/maniphest/controller/ManiphestSubpriorityController.php

@@ -32,21 +32,25 @@ final class ManiphestSubpriorityController extends ManiphestController {
       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)

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -147,12 +147,7 @@ final class ManiphestTransactionEditor
 
         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 @@ final class ManiphestTransactionEditor
     }
   }
 
-  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 @@ final class ManiphestTransactionEditor
     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;
-    } else {
-      $base = $sub;
-    }
+    $result = $query->executeOne();
+    $step = (double)(2 << 32);
 
-    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));
-        }
+    if ($result) {
+      $base = $result->getSubpriority();
+      if ($is_end) {
+        $sub = ($base - $step);
+      } else {
+        $sub = ($base + $step);
       }
     } 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;
-      }
+      $sub = 0;
     }
 
-    if ($dir == '>') {
-      return $base + (double)(2 << 32);
-    } else {
-      return $base - (double)(2 << 32);
-    }
+    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 {
+      $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) {
+      // 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 {
+      // 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);
+  }
+
+
 }

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -21,6 +21,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
   private $dateCreatedBefore;
   private $dateModifiedAfter;
   private $dateModifiedBefore;
+  private $reversePaging;
 
   private $fullTextSearch   = '';
 
@@ -765,12 +766,12 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
       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 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
             'name' => 'task.subpriority',
             'value' => $cursor->getSubpriority(),
             'type' => 'float',
-            'reverse' => true,
           );
           $columns[] = array(
             'name' => 'task.dateModified',
@@ -1120,4 +1120,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return 'PhabricatorManiphestApplication';
   }
 
+  public function setReversePaging($reverse_paging) {
+    $this->reversePaging = $reverse_paging;
+    return $this;
+  }
+
+  protected function getReversePaging() {
+    return $this->reversePaging;
+  }
+
 }

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -112,40 +112,31 @@ final class PhabricatorProjectMoveController
       }
       $tasks = mpull($tasks, null, 'getPHID');
 
-      $a_task = idx($tasks, $after_phid);
-      $b_task = idx($tasks, $before_phid);
+      $try = array(
+        array($after_phid, true),
+        array($before_phid, false),
+      );
 
-      if ($a_task &&
-         (($a_task->getPriority() < $object->getPriority()) ||
-          ($a_task->getPriority() == $object->getPriority() &&
-           $a_task->getSubpriority() >= $object->getSubpriority()))) {
-
-        $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_PRIORITY)
+          ->setNewValue($pri);
         $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();
-
-        $xactions[] = id(new ManiphestTransaction())
-          ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
-          ->setNewValue(array(
-            'newPriority' => $before_pri,
-            'newSubpriorityBase' => $before_sub,
-            'direction' => '<',
-          ));
+          ->setNewValue($sub);
       }
    }