Fix errant rules for associating projects when dragging tasks within a milestone column

Summary:
Fixes T10912. When you drag tasks within a milestone, we currently apply an overbroad, API-focused rule and add the parent board's project. This logic was added fairly recently, as part of T6027, to improve the behavior of API-originated moves.

Later on, this causes the task to toggle in and out of the parent project on every alternate drag.

This logic is also partially duplicated in the `MoveController`.

  - Add test coverage for this interaction.
  - Fix the logic so it accounts for subproject / milestone columns correctly.
  - Put all of the logic into the TransactionEditor, so the API gets the exact same rules.

Test Plan:
  - Added a failing test and made it pass.
  - Dragged tasks around within a milestone column:
    - Before patch: they got bogus project swaps on every other move.
    - After patch: projects didn't change (correct).
  - Dragged tasks around between normal and milestone columns.
    - Before patch: worked properly.
    - After patch: still works properly.

Here's what the bad changes look like, the task is swapping projects with every other move:

{F1255957}

The "every other" is because the logic was trying to do this:

  - Add both the parent and milestone project.
  - Whichever one exists already gets dropped from the change list because it would have no effect.
  - The other one then applies.
  - In applying, it forces removal of the first one.
  - Then this process repeats in the other direction the next time through.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10912

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

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -868,23 +868,9 @@ final class ManiphestTransactionEditor
     switch ($type) {
       case PhabricatorTransactions::TYPE_COLUMNS:
         try {
-          $this->buildMoveTransaction($object, $xaction);
+          $more_xactions = $this->buildMoveTransaction($object, $xaction);
-
+          foreach ($more_xactions as $more_xaction) {
-          // Implicilty add the task to any boards that we're moving it
+            $results[] = $more_xaction;
-          // on, since moves on a board the task isn't part of are not
-          // meaningful.
-          $board_phids = ipull($xaction->getNewValue(), 'boardPHID');
-          if ($board_phids) {
-            $results[] = id(new ManiphestTransaction())
-              ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-              ->setMetadataValue(
-                'edge:type',
-                PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)
-              ->setIgnoreOnNoEffect(true)
-              ->setNewValue(
-                array(
-                  '+' => array_fuse($board_phids),
-                ));
           }
         } catch (Exception $ex) {
           $error = new PhabricatorApplicationTransactionValidationError(
@@ -1098,6 +1084,89 @@ final class ManiphestTransactionEditor
 
     $new = array_values($new);
     $xaction->setNewValue($new);
+
+
+    $more = array();
+
+    // If we're moving the object into a column and it does not already belong
+    // in the column, add the appropriate board. For normal columns, this
+    // is the board PHID. For proxy columns, it is the proxy PHID, unless the
+    // object is already a member of some descendant of the proxy PHID.
+
+    // The major case where this can happen is moves via the API, but it also
+    // happens when a user drags a task from the "Backlog" to a milestone
+    // column.
+
+    if ($object_phid) {
+      $current_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
+        $object_phid,
+        PhabricatorProjectObjectHasProjectEdgeType::EDGECONST);
+      $current_phids = array_fuse($current_phids);
+    } else {
+      $current_phids = array();
+    }
+
+    $add_boards = array();
+    foreach ($new as $move) {
+      $column_phid = $move['columnPHID'];
+      $board_phid = $move['boardPHID'];
+      $column = $columns[$column_phid];
+      $proxy_phid = $column->getProxyPHID();
+
+      // If this is a normal column, add the board if the object isn't already
+      // associated.
+      if (!$proxy_phid) {
+        if (!isset($current_phids[$board_phid])) {
+          $add_boards[] = $board_phid;
+        }
+        continue;
+      }
+
+      // If this is a proxy column but the object is already associated with
+      // the proxy board, we don't need to do anything.
+      if (isset($current_phids[$proxy_phid])) {
+        continue;
+      }
+
+      // If this a proxy column and the object is already associated with some
+      // descendant of the proxy board, we also don't need to do anything.
+      $descendants = id(new PhabricatorProjectQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withAncestorProjectPHIDs(array($proxy_phid))
+        ->execute();
+
+      $found_descendant = false;
+      foreach ($descendants as $descendant) {
+        if (isset($current_phids[$descendant->getPHID()])) {
+          $found_descendant = true;
+          break;
+        }
+      }
+
+      if ($found_descendant) {
+        continue;
+      }
+
+      // Otherwise, we're moving the object to a proxy column which it is not
+      // a member of yet, so add an association to the column's proxy board.
+
+      $add_boards[] = $proxy_phid;
+    }
+
+    if ($add_boards) {
+      $more[] = id(new ManiphestTransaction())
+        ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+        ->setMetadataValue(
+          'edge:type',
+          PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)
+        ->setIgnoreOnNoEffect(true)
+        ->setNewValue(
+          array(
+            '+' => array_fuse($add_boards),
+          ));
+    }
+
+    return $more;
   }
 
   private function applyBoardMove($object, array $move) {

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -1011,6 +1011,39 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
       $column->getPHID(),
     );
     $this->assertColumns($expect, $user, $board, $task);
+
+
+    // Move the task within the "Milestone" column. This should not affect
+    // the projects the task is tagged with. See T10912.
+    $task_a = $task;
+
+    $task_b = $this->newTask($user, array($backlog));
+    $this->moveToColumn($user, $board, $task_b, $backlog, $column);
+
+    $a_options = array(
+      'beforePHID' => $task_b->getPHID(),
+    );
+
+    $b_options = array(
+      'beforePHID' => $task_a->getPHID(),
+    );
+
+    $old_projects = $this->getTaskProjects($task);
+
+    // Move the target task to the top.
+    $this->moveToColumn($user, $board, $task_a, $column, $column, $a_options);
+    $new_projects = $this->getTaskProjects($task_a);
+    $this->assertEqual($old_projects, $new_projects);
+
+    // Move the other task.
+    $this->moveToColumn($user, $board, $task_b, $column, $column, $b_options);
+    $new_projects = $this->getTaskProjects($task_a);
+    $this->assertEqual($old_projects, $new_projects);
+
+    // Move the target task again.
+    $this->moveToColumn($user, $board, $task_a, $column, $column, $a_options);
+    $new_projects = $this->getTaskProjects($task_a);
+    $this->assertEqual($old_projects, $new_projects);
   }
 
   public function testColumnExtendedPolicies() {

src/applications/project/controller/PhabricatorProjectMoveController.php

@@ -96,32 +96,6 @@ final class PhabricatorProjectMoveController
       }
     }
 
-    $proxy = $column->getProxy();
-    if ($proxy) {
-      // We're moving the task into a subproject or milestone column, so add
-      // the subproject or milestone.
-      $add_projects = array($proxy->getPHID());
-    } else if ($project->getHasSubprojects() || $project->getHasMilestones()) {
-      // We're moving the task into the "Backlog" column on the parent project,
-      // so add the parent explicitly. This gets rid of any subproject or
-      // milestone tags.
-      $add_projects = array($project->getPHID());
-    } else {
-      $add_projects = array();
-    }
-
-    if ($add_projects) {
-      $project_type = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST;
-
-      $xactions[] = id(new ManiphestTransaction())
-        ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-        ->setMetadataValue('edge:type', $project_type)
-        ->setNewValue(
-          array(
-            '+' => array_fuse($add_projects),
-          ));
-    }
-
     $editor = id(new ManiphestTransactionEditor())
       ->setActor($viewer)
       ->setContinueOnMissingFields(true)