Support natural ordering of workboards

Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.

When users drag tasks on a "natural"-ordered workboard, leave things where they put them.

This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).

Test Plan:
  - Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
  - Dragged stuff across priorities, no zany priority changes (in "natural" mode).
  - Created new tasks, they show up at the top.
  - Tagged new tasks, they show up at the top of backlog.
  - Swapped to "priority" mode and got sorting and the old priority-altering reordering.
  - Added tasks in priority mode.
  - Viewed task transactions for correctness/sanity.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: chad, epriestley

Maniphest Tasks: T4807

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

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -387,10 +387,23 @@ final class ManiphestTaskEditController extends ManiphestController {
                 ->withPHIDs($task_phids)
                 ->execute();
 
-              $sort_map = mpull(
+              if ($order == PhabricatorProjectColumn::ORDER_NATURAL) {
-                $column_tasks,
+                // TODO: This is a little bit awkward, because PHP and JS use
-                'getPrioritySortVector',
+                // slightly different sort order parameters to achieve the same
-                'getPHID');
+                // effect. It would be unify this a bit at some point.
+                $sort_map = array();
+                foreach ($positions as $position) {
+                  $sort_map[$position->getObjectPHID()] = array(
+                    -$position->getSequence(),
+                    $position->getID(),
+                  );
+                }
+              } else {
+                $sort_map = mpull(
+                  $column_tasks,
+                  'getPrioritySortVector',
+                  'getPHID');
+              }
 
               $data = array(
                 'sortMap' => $sort_map,

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -190,34 +190,144 @@ final class ManiphestTransactionEditor
             pht("Expected 'projectPHID' in column transaction."));
         }
 
+        $old_phids = idx($xaction->getOldValue(), 'columnPHIDs', array());
         $new_phids = idx($xaction->getNewValue(), 'columnPHIDs', array());
         if (count($new_phids) !== 1) {
           throw new Exception(
             pht("Expected exactly one 'columnPHIDs' in column transaction."));
         }
 
+        $columns = id(new PhabricatorProjectColumnQuery())
+          ->setViewer($this->requireActor())
+          ->withPHIDs($new_phids)
+          ->execute();
+        $columns = mpull($columns, null, 'getPHID');
+
         $positions = id(new PhabricatorProjectColumnPositionQuery())
           ->setViewer($this->requireActor())
           ->withObjectPHIDs(array($object->getPHID()))
           ->withBoardPHIDs(array($board_phid))
           ->execute();
 
+        $before_phid = idx($xaction->getNewValue(), 'beforePHID');
+        $after_phid = idx($xaction->getNewValue(), 'afterPHID');
+
+        if (!$before_phid && !$after_phid && ($old_phids == $new_phids)) {
+          // If we are not moving the object between columns and also not
+          // reordering the position, this is a move on some other order
+          // (like priority). We can leave the positions untouched and just
+          // bail, there's no work to be done.
+          return;
+        }
+
+        // Otherwise, we're either moving between columns or adjusting the
+        // object's position in the "natural" ordering, so we do need to update
+        // some rows.
+
         // Remove all existing column positions on the board.
 
         foreach ($positions as $position) {
           $position->delete();
         }
 
-        // Add the new column position.
+        // Add the new column positions.
 
         foreach ($new_phids as $phid) {
-          id(new PhabricatorProjectColumnPosition())
+          $column = idx($columns, $phid);
+          if (!$column) {
+            throw new Exception(
+              pht('No such column "%s" exists!', $phid));
+          }
+
+          // Load the other object positions in the column. Note that we must
+          // skip implicit column creation to avoid generating a new position
+          // if the target column is a backlog column.
+
+          $other_positions = id(new PhabricatorProjectColumnPositionQuery())
+            ->setViewer($this->requireActor())
+            ->withColumns(array($column))
+            ->withBoardPHIDs(array($board_phid))
+            ->setSkipImplicitCreate(true)
+            ->execute();
+          $other_positions = msort($other_positions, 'getOrderingKey');
+
+          // Set up the new position object. We're going to figure out the
+          // right sequence number and then persist this object with that
+          // sequence number.
+          $new_position = id(new PhabricatorProjectColumnPosition())
             ->setBoardPHID($board_phid)
-            ->setColumnPHID($phid)
+            ->setColumnPHID($column->getPHID())
-            ->setObjectPHID($object->getPHID())
+            ->setObjectPHID($object->getPHID());
-            // TODO: Do real sequence stuff.
+
-            ->setSequence(0)
+          $updates = array();
-            ->save();
+          $sequence = 0;
+
+          // If we're just dropping this into the column without any specific
+          // position information, put it at the top.
+          if (!$before_phid && !$after_phid) {
+            $new_position->setSequence($sequence)->save();
+            $sequence++;
+          }
+
+          foreach ($other_positions as $position) {
+            $object_phid = $position->getObjectPHID();
+
+            // If this is the object we're moving before and we haven't
+            // saved yet, insert here.
+            if (($before_phid == $object_phid) && !$new_position->getID()) {
+              $new_position->setSequence($sequence)->save();
+              $sequence++;
+            }
+
+            // This object goes here in the sequence; we might need to update
+            // the row.
+            if ($sequence != $position->getSequence()) {
+              $updates[$position->getID()] = $sequence;
+            }
+            $sequence++;
+
+            // If this is the object we're moving after and we haven't saved
+            // yet, insert here.
+            if (($after_phid == $object_phid) && !$new_position->getID()) {
+              $new_position->setSequence($sequence)->save();
+              $sequence++;
+            }
+          }
+
+          // We should have found a place to put it.
+          if (!$new_position->getID()) {
+            throw new Exception(
+              pht('Unable to find a place to insert object on column!'));
+          }
+
+          // If we changed other objects' column positions, bulk reorder them.
+
+          if ($updates) {
+            $position = new PhabricatorProjectColumnPosition();
+            $conn_w = $position->establishConnection('w');
+
+            $pairs = array();
+            foreach ($updates as $id => $sequence) {
+              // This is ugly because MySQL gets upset with us if it is
+              // configured strictly and we attempt inserts which can't work.
+              // We'll never actually do these inserts since they'll always
+              // collide (triggering the ON DUPLICATE KEY logic), so we just
+              // provide dummy values in order to get there.
+
+              $pairs[] = qsprintf(
+                $conn_w,
+                '(%d, %d, "", "", "")',
+                $id,
+                $sequence);
+            }
+
+            queryfx(
+              $conn_w,
+              'INSERT INTO %T (id, sequence, boardPHID, columnPHID, objectPHID)
+                VALUES %Q ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)',
+              $position->getTableName(),
+              implode(', ', $pairs));
+          }
         }
         break;
       default:

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -125,6 +125,16 @@ final class ManiphestTransaction
         break;
       case self::TYPE_SUBPRIORITY:
         return true;
+      case self::TYPE_PROJECT_COLUMN:
+        $old_cols = idx($this->getOldValue(), 'columnPHIDs');
+        $new_cols = idx($this->getNewValue(), 'columnPHIDs');
+
+        $old_cols = array_values($old_cols);
+        $new_cols = array_values($new_cols);
+        sort($old_cols);
+        sort($new_cols);
+
+        return ($old_cols === $new_cols);
     }
 
     return parent::shouldHide();

src/applications/project/controller/PhabricatorProjectBoardViewController.php

@@ -178,6 +178,23 @@ final class PhabricatorProjectBoardViewController
       $task_map[$column_phid][] = $task_phid;
     }
 
+    // If we're showing the board in "natural" order, sort columns by their
+    // column positions.
+    if ($this->sortKey == PhabricatorProjectColumn::ORDER_NATURAL) {
+      foreach ($task_map as $column_phid => $task_phids) {
+        $order = array();
+        foreach ($task_phids as $task_phid) {
+          if (isset($positions[$task_phid])) {
+            $order[$task_phid] = $positions[$task_phid]->getOrderingKey();
+          } else {
+            $order[$task_phid] = 0;
+          }
+        }
+        asort($order);
+        $task_map[$column_phid] = array_keys($order);
+      }
+    }
+
     $task_can_edit_map = id(new PhabricatorPolicyFilter())
       ->setViewer($viewer)
       ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT))

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -17,6 +17,8 @@ final class PhabricatorProjectMoveController
     $object_phid = $request->getStr('objectPHID');
     $after_phid = $request->getStr('afterPHID');
     $before_phid = $request->getStr('beforePHID');
+    $order = $request->getStr('order', PhabricatorProjectColumn::DEFAULT_ORDER);
+
 
     $project = id(new PhabricatorProjectQuery())
       ->setViewer($viewer)
@@ -65,13 +67,22 @@ final class PhabricatorProjectMoveController
 
     $xactions = array();
 
+    if ($order == PhabricatorProjectColumn::ORDER_NATURAL) {
+      $order_params = array(
+        'afterPHID' => $after_phid,
+        'beforePHID' => $before_phid,
+      );
+    } else {
+      $order_params = array();
+    }
+
     $xactions[] = id(new ManiphestTransaction())
       ->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN)
       ->setNewValue(
         array(
           'columnPHIDs' => array($column->getPHID()),
           'projectPHID' => $column->getProjectPHID(),
-        ))
+        ) + $order_params)
       ->setOldValue(
         array(
           'columnPHIDs' => mpull($positions, 'getColumnPHID'),
@@ -86,7 +97,7 @@ final class PhabricatorProjectMoveController
       $task_phids[] = $before_phid;
     }
 
-    if ($task_phids) {
+    if ($task_phids && ($order == PhabricatorProjectColumn::ORDER_PRIORITY)) {
       $tasks = id(new ManiphestTaskQuery())
         ->setViewer($viewer)
         ->withPHIDs($task_phids)

src/applications/project/query/PhabricatorProjectColumnPositionQuery.php

@@ -9,6 +9,7 @@ final class PhabricatorProjectColumnPositionQuery
   private $columns;
 
   private $needColumns;
+  private $skipImplicitCreate;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -46,6 +47,21 @@ final class PhabricatorProjectColumnPositionQuery
     return $this;
   }
 
+
+  /**
+   * Skip implicit creation of column positions which are implied but do not
+   * yet exist.
+   *
+   * This is primarily useful internally.
+   *
+   * @param bool  True to skip implicit creation of column positions.
+   * @return this
+   */
+  public function setSkipImplicitCreate($skip) {
+    $this->skipImplicitCreate = $skip;
+    return $this;
+  }
+
   // NOTE: For now, boards are always attached to projects. However, they might
   // not be in the future. This generalization just anticipates a future where
   // we let other types of objects (like users) have boards, or let boards
@@ -93,7 +109,7 @@ final class PhabricatorProjectColumnPositionQuery
     // column and put it in the default column.
 
     $must_type_filter = false;
-    if ($this->columns) {
+    if ($this->columns && !$this->skipImplicitCreate) {
       $default_map = array();
       foreach ($this->columns as $column) {
         if ($column->isDefaultColumn()) {

src/applications/project/storage/PhabricatorProjectColumnPosition.php

@@ -25,6 +25,17 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO
     return $this;
   }
 
+  public function getOrderingKey() {
+    // Low sequence numbers go above high sequence numbers.
+    // High position IDs go above low position IDs.
+    // Broadly, this makes newly added stuff float to the top.
+
+    return sprintf(
+      '~%012d%012d',
+      $this->getSequence(),
+      ((1 << 31) - $this->getID()));
+  }
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
   public function getCapabilities() {