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

@@ -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(

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',

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('<br />', $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',

src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php

@@ -129,8 +129,18 @@ class ManiphestTaskDetailController extends ManiphestController {
       }
     }
 
-    if (idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV)) {
+    $dtasks = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_TASK);
-      $revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV);
+    if ($dtasks) {
+      $dtask_links = array();
+      foreach ($dtasks as $dtask => $info) {
+        $dtask_links[] = $handles[$dtask]->renderLink();
+      }
+      $dtask_links = implode('<br />', $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(
-        $files = id(new PhabricatorFile())->loadAllWhere(
+        'phid IN (%Ls)',
-          'phid IN (%Ls)',
+        $file_phids);
-          $file_phids);
 
-        $views = array();
+      $views = array();
-        foreach ($files as $file) {
+      foreach ($files as $file) {
-          $view = new AphrontFilePreviewView();
+        $view = new AphrontFilePreviewView();
-          $view->setFile($file);
+        $view->setFile($file);
-          $views[] = $view->render();
+        $views[] = $view->render();
-        }
-        $dict['Files'] = implode('', $views);
       }
+      $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/');

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) {

src/applications/phid/graph/PhabricatorObjectGraph.php

@@ -0,0 +1,49 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class PhabricatorObjectGraph extends AbstractDirectedGraph {
+
+  private $edgeType;
+
+  public function setEdgeType($edge_type) {
+    $this->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;
+  }
+
+}

src/applications/phid/graph/__init__.php

@@ -0,0 +1,15 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/phid/handle/data');
+
+phutil_require_module('phutil', 'utils');
+phutil_require_module('phutil', 'utils/abstractgraph');
+
+
+phutil_require_source('PhabricatorObjectGraph.php');

src/applications/search/controller/attach/PhabricatorSearchAttachController.php

@@ -22,8 +22,9 @@ class PhabricatorSearchAttachController extends PhabricatorSearchController {
   private $type;
   private $action;
 
-  const ACTION_ATTACH = 'attach';
+  const ACTION_ATTACH       = 'attach';
-  const ACTION_MERGE  = 'merge';
+  const ACTION_MERGE        = 'merge';
+  const ACTION_DEPENDENCIES = 'dependencies';
 
   public function willProcessRequest(array $data) {
     $this->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}.");
+    }
+  }
+
 }

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');

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);
+}