From f49e35deaf083044de30e5854cf4fae5d20d98c7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Jul 2011 13:18:47 -0700 Subject: [PATCH] Basic task dependencies for Maniphest Summary: This allows you to edit dependencies. It is a better patch than it used to be. It depends on D725. - If you create a cycle, it just throws an exception and aborts the workflow. It should not do this. - Tasks which depend on the current task aren't shown in the UI. Need to add a new table for this. - Transaction text says "attached Task" but should probably say "added a dependency on task". Test Plan: Created valid and invalid dependencies between tasks. Created valid and invalid dependencies between revisions. Reviewed By: tuomaspelkonen Reviewers: davidreuss, jungejason, tuomaspelkonen, aran Commenters: codeblock CC: aran, codeblock, tuomaspelkonen, epriestley Differential Revision: 595 --- src/__celerity_resource_map__.php | 4 +- src/__phutil_library_map__.php | 2 + .../DifferentialRevisionViewController.php | 17 +++++ .../ManiphestTaskDetailController.php | 45 ++++++++---- .../ManiphestTransactionDetailView.php | 9 ++- .../phid/graph/PhabricatorObjectGraph.php | 49 +++++++++++++ src/applications/phid/graph/__init__.php | 15 ++++ .../PhabricatorSearchAttachController.php | 67 ++++++++++++++++-- .../search/controller/attach/__init__.php | 1 + .../css/aphront/headsup-action-list-view.css | 4 ++ webroot/rsrc/image/icon/fatcow/link.png | Bin 0 -> 649 bytes 11 files changed, 191 insertions(+), 22 deletions(-) create mode 100644 src/applications/phid/graph/PhabricatorObjectGraph.php create mode 100644 src/applications/phid/graph/__init__.php create mode 100755 webroot/rsrc/image/icon/fatcow/link.png diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 5ed3779c0a..91719b3888 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -63,7 +63,7 @@ celerity_register_resource_map(array( ), 'aphront-headsup-action-list-view-css' => array( - 'uri' => '/res/af3dff49/rsrc/css/aphront/headsup-action-list-view.css', + 'uri' => '/res/5f89dc44/rsrc/css/aphront/headsup-action-list-view.css', 'type' => 'css', 'requires' => array( @@ -463,7 +463,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-keyboard-navigation' => array( - 'uri' => '/res/3bdfaec7/rsrc/js/application/differential/behavior-keyboard-nav.js', + 'uri' => '/res/e36415a2/rsrc/js/application/differential/behavior-keyboard-nav.js', 'type' => 'js', 'requires' => array( diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7f954e2e57..751c4afcd2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -436,6 +436,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthProviderGithub' => 'applications/auth/oauth/provider/github', 'PhabricatorOAuthRegistrationController' => 'applications/auth/controller/oauthregistration/base', 'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/unlink', + 'PhabricatorObjectGraph' => 'applications/phid/graph', 'PhabricatorObjectHandle' => 'applications/phid/handle', 'PhabricatorObjectHandleData' => 'applications/phid/handle/data', 'PhabricatorObjectSelectorDialog' => 'view/control/objectselector', @@ -979,6 +980,7 @@ phutil_register_library_map(array( 'PhabricatorOAuthProviderGithub' => 'PhabricatorOAuthProvider', 'PhabricatorOAuthRegistrationController' => 'PhabricatorAuthController', 'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController', + 'PhabricatorObjectGraph' => 'AbstractDirectedGraph', 'PhabricatorOwnersController' => 'PhabricatorController', 'PhabricatorOwnersDAO' => 'PhabricatorLiskDAO', 'PhabricatorOwnersDeleteController' => 'PhabricatorOwnersController', diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index e1192c3e7e..fc84fe06f6 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -440,6 +440,16 @@ class DifferentialRevisionViewController extends DifferentialController { $properties['Unit'] = $ustar.' '.$umsg.$utail; + $drevs = $revision->getAttachedPHIDs( + PhabricatorPHIDConstants::PHID_TYPE_DREV); + if ($drevs) { + $links = array(); + foreach ($drevs as $drev_phid) { + $links[] = $handles[$drev_phid]->renderLink(); + } + $properties['Depends On'] = implode('
', $links); + } + if (PhabricatorEnv::getEnvConfig('maniphest.enabled')) { $tasks = $revision->getAttachedPHIDs( PhabricatorPHIDConstants::PHID_TYPE_TASK); @@ -513,6 +523,13 @@ class DifferentialRevisionViewController extends DifferentialController { require_celerity_resource('phabricator-object-selector-css'); require_celerity_resource('javelin-behavior-phabricator-object-selector'); + $links[] = array( + 'class' => 'action-dependencies', + 'name' => 'Edit Dependencies', + 'href' => "/search/attach/{$revision_phid}/DREV/dependencies/", + 'sigil' => 'workflow', + ); + if (PhabricatorEnv::getEnvConfig('maniphest.enabled')) { $links[] = array( 'class' => 'attach-maniphest', diff --git a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php index 596b761ad0..d605bf9d56 100644 --- a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php @@ -129,8 +129,18 @@ class ManiphestTaskDetailController extends ManiphestController { } } - if (idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV)) { - $revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV); + $dtasks = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_TASK); + if ($dtasks) { + $dtask_links = array(); + foreach ($dtasks as $dtask => $info) { + $dtask_links[] = $handles[$dtask]->renderLink(); + } + $dtask_links = implode('
', $dtask_links); + $dict['Depends On'] = $dtask_links; + } + + $revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV); + if ($revs) { $rev_links = array(); foreach ($revs as $rev => $info) { $rev_links[] = $handles[$rev]->renderLink(); @@ -139,23 +149,21 @@ class ManiphestTaskDetailController extends ManiphestController { $dict['Revisions'] = $rev_links; } - if (idx($attached, PhabricatorPHIDConstants::PHID_TYPE_FILE)) { - $file_infos = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_FILE); + $file_infos = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_FILE); + if ($file_infos) { $file_phids = array_keys($file_infos); - if ($file_phids) { - $files = id(new PhabricatorFile())->loadAllWhere( - 'phid IN (%Ls)', - $file_phids); + $files = id(new PhabricatorFile())->loadAllWhere( + 'phid IN (%Ls)', + $file_phids); - $views = array(); - foreach ($files as $file) { - $view = new AphrontFilePreviewView(); - $view->setFile($file); - $views[] = $view->render(); - } - $dict['Files'] = implode('', $views); + $views = array(); + foreach ($files as $file) { + $view = new AphrontFilePreviewView(); + $view->setFile($file); + $views[] = $view->render(); } + $dict['Files'] = implode('', $views); } $dict['Description'] = @@ -212,6 +220,13 @@ class ManiphestTaskDetailController extends ManiphestController { $action->setClass('action-merge'); $actions[] = $action; + $action = new AphrontHeadsupActionView(); + $action->setName('Edit Dependencies'); + $action->setURI('/search/attach/'.$task->getPHID().'/TASK/dependencies/'); + $action->setWorkflow(true); + $action->setClass('action-dependencies'); + $actions[] = $action; + $action = new AphrontHeadsupActionView(); $action->setName('Edit Differential Revisions'); $action->setURI('/search/attach/'.$task->getPHID().'/DREV/'); diff --git a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php index dc2e2a3cc0..2588264a8e 100644 --- a/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php @@ -456,7 +456,9 @@ class ManiphestTransactionDetailView extends ManiphestView { $old_raw = nonempty($old, array()); $new_raw = nonempty($new, array()); - foreach (array(PhabricatorPHIDConstants::PHID_TYPE_DREV, + foreach (array( + PhabricatorPHIDConstants::PHID_TYPE_DREV, + PhabricatorPHIDConstants::PHID_TYPE_TASK, PhabricatorPHIDConstants::PHID_TYPE_FILE) as $type) { $old = array_keys(idx($old_raw, $type, array())); $new = array_keys(idx($new_raw, $type, array())); @@ -480,6 +482,11 @@ class ManiphestTransactionDetailView extends ManiphestView { $singular = 'file'; $plural = 'files'; break; + case PhabricatorPHIDConstants::PHID_TYPE_TASK: + $singular = 'Maniphest Task'; + $plural = 'Maniphest Tasks'; + $dependency = true; + break; } if ($added && !$removed) { diff --git a/src/applications/phid/graph/PhabricatorObjectGraph.php b/src/applications/phid/graph/PhabricatorObjectGraph.php new file mode 100644 index 0000000000..4eedc42321 --- /dev/null +++ b/src/applications/phid/graph/PhabricatorObjectGraph.php @@ -0,0 +1,49 @@ +edgeType = $edge_type; + return $this; + } + + protected function loadEdges(array $nodes) { + if (!$this->edgeType) { + throw new Exception("Set edge type before loading graph!"); + } + + $handle_data = new PhabricatorObjectHandleData($nodes); + $objects = $handle_data->loadObjects(); + + $result = array(); + foreach ($nodes as $phid) { + $object = idx($objects, $phid); + if ($object) { + $result[$phid] = $object->getAttachedPHIDs($this->edgeType); + } else { + $result[$phid] = array(); + } + } + + return $result; + } + +} diff --git a/src/applications/phid/graph/__init__.php b/src/applications/phid/graph/__init__.php new file mode 100644 index 0000000000..7aedfd1a4a --- /dev/null +++ b/src/applications/phid/graph/__init__.php @@ -0,0 +1,15 @@ +phid = $data['phid']; @@ -58,12 +59,23 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { switch ($this->action) { case self::ACTION_MERGE: return $this->performMerge($object, $handle, $phids); + + case self::ACTION_DEPENDENCIES: case self::ACTION_ATTACH: + $two_way = true; + if ($this->action == self::ACTION_DEPENDENCIES) { + $two_way = false; + $this->detectGraphCycles( + $object, + $attach_type, + $phids); + } $this->performAttach( $object_type, $object, $attach_type, - $phids); + $phids, + $two_way); return id(new AphrontReloadResponse())->setURI($handle->getURI()); default: throw new Exception("Unsupported attach action."); @@ -71,6 +83,7 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { } else { switch ($this->action) { case self::ACTION_ATTACH: + case self::ACTION_DEPENDENCIES: $phids = $object->getAttachedPHIDs($attach_type); break; default: @@ -132,7 +145,8 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { $object_type, $object, $attach_type, - array $phids) { + array $phids, + $two_way) { $object_phid = $object->getPHID(); @@ -161,6 +175,10 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { // Update the primary object. $this->writeOutboundEdges($object_type, $object, $attach_type, $phids); + if (!$two_way) { + return; + } + // Loop through all of the attached/detached objects and update them. foreach ($attach_objs as $phid => $attach_obj) { $attached_phids = $attach_obj->getAttachedPHIDs($object_type); @@ -290,6 +308,12 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { "These tasks will be merged into the current task and then closed. ". "The current task will grow stronger."; break; + case self::ACTION_DEPENDENCIES: + $dialog_title = "Edit Dependencies"; + $header_text = "Current Dependencies"; + $button_text = "Save Dependencies"; + $instructions = null; + break; } return array( @@ -302,4 +326,39 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController { ); } + private function detectGraphCycles( + $object, + $attach_type, + array $phids) { + + // Detect graph cycles. + $graph = new PhabricatorObjectGraph(); + $graph->setEdgeType($attach_type); + $graph->addNodes(array( + $object->getPHID() => $phids, + )); + $graph->loadGraph(); + + foreach ($phids as $phid) { + $cycle = $graph->detectCycles($phid); + if (!$cycle) { + continue; + } + + // TODO: Improve this behavior so it's not all-or-nothing? + + $handles = id(new PhabricatorObjectHandleData($cycle)) + ->loadHandles(); + $names = array(); + foreach ($cycle as $cycle_phid) { + $names[] = $handles[$cycle_phid]->getFullName(); + } + $names = implode(" \xE2\x86\x92 ", $names); + $which = $handles[$phid]->getFullName(); + throw new Exception( + "You can not create a dependency on '{$which}' because it ". + "would create a circular dependency: {$names}."); + } + } + } diff --git a/src/applications/search/controller/attach/__init__.php b/src/applications/search/controller/attach/__init__.php index ba58f3cdac..c8bb6ac82e 100644 --- a/src/applications/search/controller/attach/__init__.php +++ b/src/applications/search/controller/attach/__init__.php @@ -15,6 +15,7 @@ phutil_require_module('phabricator', 'applications/maniphest/editor/transaction' phutil_require_module('phabricator', 'applications/maniphest/storage/task'); phutil_require_module('phabricator', 'applications/maniphest/storage/transaction'); phutil_require_module('phabricator', 'applications/phid/constants'); +phutil_require_module('phabricator', 'applications/phid/graph'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'applications/search/controller/search'); phutil_require_module('phabricator', 'view/control/objectselector'); diff --git a/webroot/rsrc/css/aphront/headsup-action-list-view.css b/webroot/rsrc/css/aphront/headsup-action-list-view.css index edd629041a..80a0d0eedf 100644 --- a/webroot/rsrc/css/aphront/headsup-action-list-view.css +++ b/webroot/rsrc/css/aphront/headsup-action-list-view.css @@ -65,3 +65,7 @@ .aphront-headsup-action-list .action-merge { background-image: url(/rsrc/image/icon/fatcow/arrow_merge.png); } + +.aphront-headsup-action-list .action-dependencies { + background-image: url(/rsrc/image/icon/fatcow/link.png); +} diff --git a/webroot/rsrc/image/icon/fatcow/link.png b/webroot/rsrc/image/icon/fatcow/link.png new file mode 100755 index 0000000000000000000000000000000000000000..c4f00cb2231006331f76e8fb34fe98ba3e6c4c5a GIT binary patch literal 649 zcmV;40(Sk0P)R-wVQ6jZWcdI0SI7HLUn+ckeHpfG*^&WNl8M*$>e_m+%~e1e$TbFH z(ca$P8S6G|`k$7O^&jK{CVU1k0`c?bFBpFR`o)l$lXnKleSPl2rS#vw|1$jj^9QU3 zuK|C7fi{2PLWYLgYKAv2UoylcB>fKv2xQp4Z3_cGzu;Pe0a#X1_5bVFum6KX!v1gG zy8ZvEHS7P!CnWywn=ol7P+AV;1xBO*o3?uOnoLGUMh1|?@87=}8tSSU5)xAxUcPwB zpslOV@ZSJml#C3+_wPS|_&dRXX88q- z<2Uc#FtD(3lrLGb_|3`F=ejkuv>3K*-o&7#twS)X7?&1GCf1H)a#g?Xt!HIMLl0ThjLv**n3J9+X%Fi7wZFolVUi|^~|Xai}!M<@(n=>QZp jqDXS@32BBI01#jRVaY0la%Xjb00000NkvXXu0mjfT~I+< literal 0 HcmV?d00001