Workboards - make priority changes less aggressive and generally better

Summary: Fixes T4641.

Test Plan: Dragged a "normal" task between "high" and "low" tasks and it stayed as "normal". Generally seems correct when playing around.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: mbishopim3, Beltran-rubo, epriestley, Korvin

Maniphest Tasks: T4641

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

resources/celerity/map.php

@@ -404,7 +404,7 @@ return array(
     'rsrc/js/application/policy/behavior-policy-control.js' => 'c01153ea',
     'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '263aeb8c',
     'rsrc/js/application/ponder/behavior-votebox.js' => '327dbe61',
-    'rsrc/js/application/projects/behavior-project-boards.js' => '04c59172',
+    'rsrc/js/application/projects/behavior-project-boards.js' => 'd8e135db',
     'rsrc/js/application/projects/behavior-project-create.js' => '065227cc',
     'rsrc/js/application/releeph/releeph-preview-branch.js' => '9eb2cedb',
     'rsrc/js/application/releeph/releeph-request-state-change.js' => 'fe7fc914',
@@ -614,7 +614,7 @@ return array(
     'javelin-behavior-policy-control' => 'c01153ea',
     'javelin-behavior-policy-rule-editor' => '263aeb8c',
     'javelin-behavior-ponder-votebox' => '327dbe61',
-    'javelin-behavior-project-boards' => '04c59172',
+    'javelin-behavior-project-boards' => 'd8e135db',
     'javelin-behavior-project-create' => '065227cc',
     'javelin-behavior-refresh-csrf' => 'c4b31646',
     'javelin-behavior-releeph-preview-branch' => '9eb2cedb',
@@ -843,15 +843,6 @@ return array(
       3 => 'javelin-vector',
       4 => 'javelin-install',
     ),
-    '04c59172' =>
-    array(
-      0 => 'javelin-behavior',
-      1 => 'javelin-dom',
-      2 => 'javelin-util',
-      3 => 'javelin-stratcom',
-      4 => 'javelin-workflow',
-      5 => 'phabricator-draggable-list',
-    ),
     '065227cc' =>
     array(
       0 => 'javelin-behavior',
@@ -1752,6 +1743,15 @@ return array(
       3 => 'javelin-dom',
       4 => 'phabricator-keyboard-shortcut',
     ),
+    'd8e135db' =>
+    array(
+      0 => 'javelin-behavior',
+      1 => 'javelin-dom',
+      2 => 'javelin-util',
+      3 => 'javelin-stratcom',
+      4 => 'javelin-workflow',
+      5 => 'phabricator-draggable-list',
+    ),
     'd8ef8659' =>
     array(
       0 => 'javelin-behavior',

src/applications/maniphest/controller/ManiphestSubpriorityController.php

@@ -45,7 +45,8 @@ final class ManiphestSubpriorityController extends ManiphestController {
       ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
       ->setNewValue(array(
         'newPriority' => $after_pri,
-        'newSubpriorityBase' => $after_sub)));
+        'newSubpriorityBase' => $after_sub,
+        'direction' => '>')));
     $editor = id(new ManiphestTransactionEditor())
       ->setActor($user)
       ->setContinueOnMissingFields(true)

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -165,7 +165,8 @@ final class ManiphestTransactionEditor
         $data = $xaction->getNewValue();
         $new_sub = $this->getNextSubpriority(
           $data['newPriority'],
-          $data['newSubpriorityBase']);
+          $data['newSubpriorityBase'],
+          $data['direction']);
         $object->setSubpriority($new_sub);
         return;
       case ManiphestTransaction::TYPE_PROJECT_COLUMN:
@@ -441,26 +442,55 @@ final class ManiphestTransactionEditor
     return $copy;
   }
 
-  private function getNextSubpriority($pri, $sub) {
+  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;
+    }
+
+    if ($sub === null) {
+      $base = 0;
+    } else {
+      $base = $sub;
+    }
 
     if ($sub === null) {
       $next = id(new ManiphestTask())->loadOneWhere(
-        'priority = %d ORDER BY subpriority ASC LIMIT 1',
-        $pri);
+        '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));
+        }
       }
     } else {
       $next = id(new ManiphestTask())->loadOneWhere(
-        'priority = %d AND subpriority > %s ORDER BY subpriority ASC LIMIT 1',
+        'priority = %d AND subpriority %Q %f ORDER BY subpriority %Q LIMIT 1',
         $pri,
-        $sub);
+        $dir,
+        $sub,
+        $order);
       if ($next) {
         return ($sub + $next->getSubpriority()) / 2;
       }
     }
 
-    return (double)(2 << 32);
+    if ($dir == '>') {
+      return $base + (double)(2 << 32);
+    } else {
+      return $base - (double)(2 << 32);
+    }
   }
 
 }

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -16,6 +16,7 @@ final class PhabricatorProjectMoveController
     $column_phid = $request->getStr('columnPHID');
     $object_phid = $request->getStr('objectPHID');
     $after_phid = $request->getStr('afterPHID');
+    $before_phid = $request->getStr('beforePHID');
 
     $project = id(new PhabricatorProjectQuery())
       ->setViewer($viewer)
@@ -78,23 +79,57 @@ final class PhabricatorProjectMoveController
         'columnPHIDs' => $edge_phids,
         'projectPHID' => $column->getProjectPHID()));
 
+    $task_phids = array();
     if ($after_phid) {
-      $after_task = id(new ManiphestTaskQuery())
+      $task_phids[] = $after_phid;
+    }
+    if ($before_phid) {
+      $task_phids[] = $before_phid;
+    }
+    if ($task_phids) {
+      $tasks = id(new ManiphestTaskQuery())
         ->setViewer($viewer)
-        ->withPHIDs(array($after_phid))
+        ->withPHIDs($task_phids)
         ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT))
-        ->executeOne();
-      if (!$after_task) {
+        ->execute();
+      if (count($tasks) != count($task_phids)) {
         return new Aphront404Response();
       }
-      $after_pri = $after_task->getPriority();
-      $after_sub = $after_task->getSubpriority();
+      $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()))) {
+
+        $after_pri = $a_task->getPriority();
+        $after_sub = $a_task->getSubpriority();
 
         $xactions[] = id(new ManiphestTransaction())
           ->setTransactionType(ManiphestTransaction::TYPE_SUBPRIORITY)
           ->setNewValue(array(
             'newPriority' => $after_pri,
-          'newSubpriorityBase' => $after_sub));
+            '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' => '<'));
+      }
    }
 
     $editor = id(new ManiphestTransactionEditor())

webroot/rsrc/js/application/projects/behavior-project-boards.js

@@ -28,13 +28,40 @@ JX.behavior('project-boards', function(config) {
     list.lock();
     JX.DOM.alterClass(item, 'drag-sending', true);
 
+    var item_phid = JX.Stratcom.getData(item).objectPHID;
     var data = {
-      objectPHID: JX.Stratcom.getData(item).objectPHID,
+      objectPHID: item_phid,
       columnPHID: JX.Stratcom.getData(list.getRootNode()).columnPHID
     };
 
+    var after_phid = null;
+    var items = finditems(list.getRootNode());
     if (after) {
-      data.afterPHID = JX.Stratcom.getData(after).objectPHID;
+      after_phid = JX.Stratcom.getData(after).objectPHID;
+      data.afterPHID = after_phid;
+    }
+    var ii;
+    var ii_item;
+    var ii_item_phid;
+    var ii_prev_item_phid = null;
+    var before_phid = null;
+    for (ii = 0; ii < items.length; ii++) {
+      ii_item = items[ii];
+      ii_item_phid = JX.Stratcom.getData(ii_item).objectPHID;
+      if (ii_item_phid == item_phid) {
+        // skip the item we just dropped
+        continue;
+      }
+      // note this handles when there is no after phid - we are at the top of
+      // the list - quite nicely
+      if (ii_prev_item_phid == after_phid) {
+        before_phid = ii_item_phid;
+        break;
+      }
+      ii_prev_item_phid = ii_item_phid;
+    }
+    if (before_phid) {
+      data.beforePHID = before_phid;
     }
 
     var workflow = new JX.Workflow(config.moveURI, data)