diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index 3bd38353f6..6c0f05ebbb 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/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, diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 7fa83b3fb0..d6f70cac39 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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 List of project PHIDs. + * @return list 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; + } + } diff --git a/src/docs/user/userguide/projects.diviner b/src/docs/user/userguide/projects.diviner index b1456a9e77..7d014f3fb7 100644 --- a/src/docs/user/userguide/projects.diviner +++ b/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 ===============