Implement subproject/milestone conflict resolution rules

Summary:
Ref T10010. When you try to add "Sprint 35" to a task, remove "Sprint 34", etc. Briefly:

  - A task can't be in Sprint 3 and Sprint 4.
  - A task can't be in "A" and "A > B" (but "A > B" and "A > C" are fine).
  - When a user makes an edit which would violate one of these rules, preserve the last tag in each group of conflicts.

Test Plan:
  - Added fairly comprehensive tests.
  - Added a bunch of different tags to things, saw them properly exclude conflicting tags.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010

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

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -808,6 +808,114 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
       pht('Engineering + Scan'));
   }
 
+  public function testTagAncestryConflicts() {
+    $user = $this->createUser();
+    $user->save();
+
+    $stonework = $this->createProject($user);
+    $stonework_masonry = $this->createProject($user, $stonework);
+    $stonework_sculpting = $this->createProject($user, $stonework);
+
+    $task = $this->newTask($user, array());
+    $this->assertEqual(array(), $this->getTaskProjects($task));
+
+    $this->addProjectTags($user, $task, array($stonework->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // Adding a descendant should remove the parent.
+    $this->addProjectTags($user, $task, array($stonework_masonry->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework_masonry->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // Adding an ancestor should remove the descendant.
+    $this->addProjectTags($user, $task, array($stonework->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // Adding two tags in the same hierarchy which are not mutual ancestors
+    // should remove the ancestor but otherwise work fine.
+    $this->addProjectTags(
+      $user,
+      $task,
+      array(
+        $stonework_masonry->getPHID(),
+        $stonework_sculpting->getPHID(),
+      ));
+
+    $expect = array(
+      $stonework_masonry->getPHID(),
+      $stonework_sculpting->getPHID(),
+    );
+    sort($expect);
+
+    $this->assertEqual($expect,  $this->getTaskProjects($task));
+  }
+
+  public function testTagMilestoneConflicts() {
+    $user = $this->createUser();
+    $user->save();
+
+    $stonework = $this->createProject($user);
+    $stonework_1 = $this->createProject($user, $stonework, true);
+    $stonework_2 = $this->createProject($user, $stonework, true);
+
+    $task = $this->newTask($user, array());
+    $this->assertEqual(array(), $this->getTaskProjects($task));
+
+    $this->addProjectTags($user, $task, array($stonework->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // Adding a milesone should remove the parent.
+    $this->addProjectTags($user, $task, array($stonework_1->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework_1->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // Adding the parent should remove the milestone.
+    $this->addProjectTags($user, $task, array($stonework->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+
+    // First, add one milestone.
+    $this->addProjectTags($user, $task, array($stonework_1->getPHID()));
+    // Now, adding a second milestone should remove the first milestone.
+    $this->addProjectTags($user, $task, array($stonework_2->getPHID()));
+    $this->assertEqual(
+      array(
+        $stonework_2->getPHID(),
+      ),
+      $this->getTaskProjects($task));
+  }
+
+  private function getTaskProjects(ManiphestTask $task) {
+    $project_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
+      $task->getPHID(),
+      PhabricatorProjectObjectHasProjectEdgeType::EDGECONST);
+
+    sort($project_phids);
+
+    return $project_phids;
+  }
+
   private function attemptProjectEdit(
     PhabricatorProject $proj,
     PhabricatorUser $user,
@@ -827,6 +935,30 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
   }
 
 
+  private function addProjectTags(
+    PhabricatorUser $viewer,
+    ManiphestTask $task,
+    array $phids) {
+
+    $xactions = array();
+
+    $xactions[] = id(new ManiphestTransaction())
+      ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+      ->setMetadataValue(
+        'edge:type',
+        PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)
+      ->setNewValue(
+        array(
+          '+' => array_fuse($phids),
+        ));
+
+    $editor = id(new ManiphestTransactionEditor())
+      ->setActor($viewer)
+      ->setContentSource(PhabricatorContentSource::newConsoleSource())
+      ->setContinueOnNoEffect(true)
+      ->applyTransactions($task, $xactions);
+  }
+
   private function newTask(
     PhabricatorUser $viewer,
     array $projects,

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -400,7 +400,15 @@ abstract class PhabricatorApplicationTransactionEditor
           return $space_phid;
         }
       case PhabricatorTransactions::TYPE_EDGE:
-        return $this->getEdgeTransactionNewValue($xaction);
+        $new_value = $this->getEdgeTransactionNewValue($xaction);
+
+        $edge_type = $xaction->getMetadataValue('edge:type');
+        $type_project = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST;
+        if ($edge_type == $type_project) {
+          $new_value = $this->applyProjectConflictRules($new_value);
+        }
+
+        return $new_value;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->getNewValueFromApplicationTransactions($xaction);
@@ -3346,4 +3354,127 @@ abstract class PhabricatorApplicationTransactionEditor
     return $state;
   }
 
+
+  /**
+   * Remove conflicts from a list of projects.
+   *
+   * Objects aren't allowed to be tagged with multiple milestones in the same
+   * group, nor projects such that one tag is the ancestor of any other tag.
+   * If the list of PHIDs include mutually exclusive projects, remove the
+   * conflicting projects.
+   *
+   * @param list<phid> List of project PHIDs.
+   * @return list<phid> List with conflicts removed.
+   */
+  private function applyProjectConflictRules(array $phids) {
+    if (!$phids) {
+      return array();
+    }
+
+    // Overall, the last project in the list wins in cases of conflict (so when
+    // you add something, the thing you just added sticks and removes older
+    // values).
+
+    // Beyond that, there are two basic cases:
+
+    // Milestones: An object can't be in "A > Sprint 3" and "A > Sprint 4".
+    // If multiple projects are milestones of the same parent, we only keep the
+    // last one.
+
+    // Ancestor: You can't be in "A" and "A > B". If "A > B" comes later
+    // in the list, we remove "A" and keep "A > B". If "A" comes later, we
+    // remove "A > B" and keep "A".
+
+    // Note that it's OK to be in "A > B" and "A > C". There's only a conflict
+    // if one project is an ancestor of another. It's OK to have something
+    // tagged with multiple projects which share a common ancestor, so long as
+    // they are not mutual ancestors.
+
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $projects = id(new PhabricatorProjectQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array_keys($phids))
+      ->execute();
+    $projects = mpull($projects, null, 'getPHID');
+
+    // We're going to build a map from each project with milestones to the last
+    // milestone in the list. This last milestone is the milestone we'll keep.
+    $milestone_map = array();
+
+    // We're going to build a set of the projects which have no descendants
+    // later in the list. This allows us to apply both ancestor rules.
+    $ancestor_map = array();
+
+    foreach ($phids as $phid => $ignored) {
+      $project = idx($projects, $phid);
+      if (!$project) {
+        continue;
+      }
+
+      // This is the last milestone we've seen, so set it as the selection for
+      // the project's parent. This might be setting a new value or overwriting
+      // an earlier value.
+      if ($project->isMilestone()) {
+        $parent_phid = $project->getParentProjectPHID();
+        $milestone_map[$parent_phid] = $phid;
+      }
+
+      // Since this is the last item in the list we've examined so far, add it
+      // to the set of projects with no later descendants.
+      $ancestor_map[$phid] = $phid;
+
+      // Remove any ancestors from the set, since this is a later descendant.
+      foreach ($project->getAncestorProjects() as $ancestor) {
+        $ancestor_phid = $ancestor->getPHID();
+        unset($ancestor_map[$ancestor_phid]);
+      }
+    }
+
+    // Now that we've built the maps, we can throw away all the projects which
+    // have conflicts.
+    foreach ($phids as $phid => $ignored) {
+      $project = idx($projects, $phid);
+
+      if (!$project) {
+        // If a PHID is invalid, we just leave it as-is. We could clean it up,
+        // but leaving it untouched is less likely to cause collateral damage.
+        continue;
+      }
+
+      // If this was a milestone, check if it was the last milestone from its
+      // group in the list. If not, remove it from the list.
+      if ($project->isMilestone()) {
+        $parent_phid = $project->getParentProjectPHID();
+        if ($milestone_map[$parent_phid] !== $phid) {
+          unset($phids[$phid]);
+          continue;
+        }
+      }
+
+      // If a later project in the list is a subproject of this one, it will
+      // have removed ancestors from the map. If this project does not point
+      // at itself in the ancestor map, it should be discarded in favor of a
+      // subproject that comes later.
+      if (idx($ancestor_map, $phid) !== $phid) {
+        unset($phids[$phid]);
+        continue;
+      }
+
+      // If a later project in the list is an ancestor of this one, it will
+      // have added itself to the map. If any ancestor of this project points
+      // at itself in the map, this project should be dicarded in favor of
+      // that later ancestor.
+      foreach ($project->getAncestorProjects() as $ancestor) {
+        $ancestor_phid = $ancestor->getPHID();
+        if (isset($ancestor_map[$ancestor_phid])) {
+          unset($phids[$phid]);
+          continue 2;
+        }
+      }
+    }
+
+    return $phids;
+  }
+
 }

src/docs/user/userguide/projects.diviner

@@ -184,6 +184,26 @@ subproject rules.
 
 Subprojects can have normal workboards.
 
+The maximum subproject depth is 16. This limit is intended to grossly exceed
+the depth necessary in normal usage.
+
+Objects may not be tagged with multiple projects that are ancestors or
+descendants of one another. For example, a task may not be tagged with both
+{nav Stonework} and {nav Stonework > Masonry}.
+
+When a project tag is added that is the ancestor or descendant of one or more
+existing tags, the old tags are replaced. For example, adding
+{nav Stonework > Masonry} to a task tagged with {nav Stonework} will replace
+{nav Stonework} with the newer, more specific tag.
+
+This restriction does not apply to projects which share some common ancestor
+but are not themselves mutual ancestors. For example, a task may be tagged
+with both {nav Stonework > Masonry} and {nav Stonework > Sculpting}.
+
+This restriction //does// apply when the descendant is a milestone. For
+example, a task may not be tagged with both {nav Stonework} and
+{nav Stonework > Iteration II}.
+
 
 Milestones
 ==========
@@ -204,6 +224,19 @@ By default, Milestones do not have their own hashtags.
 
 Milestones can have normal workboards.
 
+Objects may not be tagged with two different milestones of the same parent
+project. For example, a task may not be tagged with both {nav Stonework >
+Iteration III} and {nav Stonework > Iteration V}.
+
+When a milestone tag is added to an object which already has a tag from the
+same series of milestones, the old tag is removed. For example, adding the
+{nav Stonework > Iteration V} tag to a task which already has the
+{nav Stonework > Iteration III} tag will remove the {nav Iteration III} tag.
+
+This restriction does not apply to milestones which are not part of the same
+series. For example, a task may be tagged with both
+{nav Stonework > Iteration V} and {nav Heraldry > Iteration IX}.
+
 
 Parent Projects
 ===============