Split "Edit Blocking Tasks" into "Edit Parent Tasks" and "Edit Subtasks"

Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").

This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:

  - Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
  - Align language with "Create Subtask".
  - To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.

Fixes T6815. I think I narrowed this down to two issues:

  - Just throwing a bare exeception (we now return a dialog explicitly).
  - Not killing open transactions when the cyclec check fails (we now kill them).

Test Plan:
  - Edited parent tasks.
  - Edited subtasks.
  - Tried to introduce graph cycles, got a nice error dialog.

{F1697087}

{F1697088}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6815, T11179

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

src/__phutil_library_map__.php

@@ -1422,8 +1422,10 @@ phutil_register_library_map(array(
     'ManiphestTaskHasCommitRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasCommitRelationship.php',
     'ManiphestTaskHasMockEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasMockEdgeType.php',
     'ManiphestTaskHasMockRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasMockRelationship.php',
+    'ManiphestTaskHasParentRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php',
     'ManiphestTaskHasRevisionEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php',
     'ManiphestTaskHasRevisionRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasRevisionRelationship.php',
+    'ManiphestTaskHasSubtaskRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasSubtaskRelationship.php',
     'ManiphestTaskHeraldField' => 'applications/maniphest/herald/ManiphestTaskHeraldField.php',
     'ManiphestTaskHeraldFieldGroup' => 'applications/maniphest/herald/ManiphestTaskHeraldFieldGroup.php',
     'ManiphestTaskListController' => 'applications/maniphest/controller/ManiphestTaskListController.php',
@@ -5913,8 +5915,10 @@ phutil_register_library_map(array(
     'ManiphestTaskHasCommitRelationship' => 'ManiphestTaskRelationship',
     'ManiphestTaskHasMockEdgeType' => 'PhabricatorEdgeType',
     'ManiphestTaskHasMockRelationship' => 'ManiphestTaskRelationship',
+    'ManiphestTaskHasParentRelationship' => 'ManiphestTaskRelationship',
     'ManiphestTaskHasRevisionEdgeType' => 'PhabricatorEdgeType',
     'ManiphestTaskHasRevisionRelationship' => 'ManiphestTaskRelationship',
+    'ManiphestTaskHasSubtaskRelationship' => 'ManiphestTaskRelationship',
     'ManiphestTaskHeraldField' => 'HeraldField',
     'ManiphestTaskHeraldFieldGroup' => 'HeraldFieldGroup',
     'ManiphestTaskListController' => 'ManiphestController',

src/applications/auth/controller/PhabricatorAuthStartController.php

@@ -225,10 +225,10 @@ final class PhabricatorAuthStartController
     }
 
     // Often, users end up here by clicking a disabled action link in the UI
-    // (for example, they might click "Edit Blocking Tasks" on a Maniphest
+    // (for example, they might click "Edit Subtasks" on a Maniphest task
-    // task page). After they log in we want to send them back to that main
+    // page). After they log in we want to send them back to that main object
-    // object page if we can, since it's confusing to end up on a standalone
+    // page if we can, since it's confusing to end up on a standalone page with
-    // page with only a dialog (particularly if that dialog is another error,
+    // only a dialog (particularly if that dialog is another error,
     // like a policy exception).
 
     $via_header = AphrontRequest::getViaHeaderName();

src/applications/maniphest/controller/ManiphestTaskDetailController.php

@@ -195,12 +195,18 @@ final class ManiphestTaskDetailController extends ManiphestController {
       ->setDisabled(!$can_create)
       ->setWorkflow(!$can_create);
 
-    $task_submenu[] = id(new PhabricatorActionView())
+    $relationship_list = PhabricatorObjectRelationshipList::newForObject(
-      ->setName(pht('Edit Blocking Tasks'))
+      $viewer,
-      ->setHref("/search/attach/{$phid}/TASK/blocks/")
+      $task);
-      ->setIcon('fa-link')
+
-      ->setDisabled(!$can_edit)
+    $parent_key = ManiphestTaskHasParentRelationship::RELATIONSHIPKEY;
-      ->setWorkflow(true);
+    $subtask_key = ManiphestTaskHasSubtaskRelationship::RELATIONSHIPKEY;
+
+    $task_submenu[] = $relationship_list->getRelationship($parent_key)
+      ->newAction($task);
+
+    $task_submenu[] = $relationship_list->getRelationship($subtask_key)
+      ->newAction($task);
 
     $task_submenu[] = id(new PhabricatorActionView())
       ->setName(pht('Merge Duplicates In'))
@@ -215,10 +221,6 @@ final class ManiphestTaskDetailController extends ManiphestController {
         ->setIcon('fa-anchor')
         ->setSubmenu($task_submenu));
 
-    $relationship_list = PhabricatorObjectRelationshipList::newForObject(
-      $viewer,
-      $task);
-
     $relationship_submenu = $relationship_list->newActionMenu();
     if ($relationship_submenu) {
       $curtain->addAction($relationship_submenu);
@@ -288,9 +290,9 @@ final class ManiphestTaskDetailController extends ManiphestController {
 
     $edge_types = array(
       ManiphestTaskDependedOnByTaskEdgeType::EDGECONST
-        => pht('Blocks'),
+        => pht('Parent Tasks'),
       ManiphestTaskDependsOnTaskEdgeType::EDGECONST
-        => pht('Blocked By'),
+        => pht('Subtasks'),
       ManiphestTaskHasRevisionEdgeType::EDGECONST
         => pht('Differential Revisions'),
       ManiphestTaskHasMockEdgeType::EDGECONST

src/applications/maniphest/edge/ManiphestTaskDependedOnByTaskEdgeType.php

@@ -17,7 +17,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $add_edges) {
 
     return pht(
-      '%s added %s blocked task(s): %s.',
+      '%s added %s parent task(s): %s.',
       $actor,
       $add_count,
       $add_edges);
@@ -29,7 +29,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s removed %s blocked task(s): %s.',
+      '%s removed %s parent task(s): %s.',
       $actor,
       $rem_count,
       $rem_edges);
@@ -44,7 +44,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s edited blocked task(s), added %s: %s; removed %s: %s.',
+      '%s edited parent task(s), added %s: %s; removed %s: %s.',
       $actor,
       $add_count,
       $add_edges,
@@ -59,7 +59,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $add_edges) {
 
     return pht(
-      '%s added %s blocked task(s) for %s: %s.',
+      '%s added %s parent task(s) for %s: %s.',
       $actor,
       $add_count,
       $object,
@@ -73,7 +73,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s removed %s blocked task(s) for %s: %s.',
+      '%s removed %s parent task(s) for %s: %s.',
       $actor,
       $rem_count,
       $object,
@@ -90,7 +90,7 @@ final class ManiphestTaskDependedOnByTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s edited blocked task(s) for %s, added %s: %s; removed %s: %s.',
+      '%s edited parent task(s) for %s, added %s: %s; removed %s: %s.',
       $actor,
       $object,
       $add_count,

src/applications/maniphest/edge/ManiphestTaskDependsOnTaskEdgeType.php

@@ -22,7 +22,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $add_edges) {
 
     return pht(
-      '%s added %s blocking task(s): %s.',
+      '%s added %s subtask(s): %s.',
       $actor,
       $add_count,
       $add_edges);
@@ -34,7 +34,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s removed %s blocking task(s): %s.',
+      '%s removed %s subtask(s): %s.',
       $actor,
       $rem_count,
       $rem_edges);
@@ -49,7 +49,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s edited blocking task(s), added %s: %s; removed %s: %s.',
+      '%s edited subtask(s), added %s: %s; removed %s: %s.',
       $actor,
       $add_count,
       $add_edges,
@@ -64,7 +64,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $add_edges) {
 
     return pht(
-      '%s added %s blocking task(s) for %s: %s.',
+      '%s added %s subtask(s) for %s: %s.',
       $actor,
       $add_count,
       $object,
@@ -78,7 +78,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s removed %s blocking task(s) for %s: %s.',
+      '%s removed %s subtask(s) for %s: %s.',
       $actor,
       $rem_count,
       $object,
@@ -95,7 +95,7 @@ final class ManiphestTaskDependsOnTaskEdgeType extends PhabricatorEdgeType {
     $rem_edges) {
 
     return pht(
-      '%s edited blocking task(s) for %s, added %s: %s; removed %s: %s.',
+      '%s edited subtask(s) for %s, added %s: %s; removed %s: %s.',
       $actor,
       $object,
       $add_count,

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -325,7 +325,7 @@ final class ManiphestTransactionEditor
       ManiphestTransaction::MAILTAG_PROJECTS =>
         pht("A task's associated projects change."),
       ManiphestTransaction::MAILTAG_UNBLOCK =>
-        pht('One of the tasks a task is blocked by changes status.'),
+        pht("One of a task's subtasks changes status."),
       ManiphestTransaction::MAILTAG_COLUMN =>
         pht('A task is moved between columns on a workboard.'),
       ManiphestTransaction::MAILTAG_COMMENT =>

src/applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php

@@ -0,0 +1,40 @@
+<?php
+
+final class ManiphestTaskHasParentRelationship
+  extends ManiphestTaskRelationship {
+
+  const RELATIONSHIPKEY = 'task.has-parent';
+
+  public function getEdgeConstant() {
+    return ManiphestTaskDependedOnByTaskEdgeType::EDGECONST;
+  }
+
+  protected function getActionName() {
+    return pht('Edit Parent Tasks');
+  }
+
+  protected function getActionIcon() {
+    return 'fa-chevron-circle-up';
+  }
+
+  public function canRelateObjects($src, $dst) {
+    return ($dst instanceof ManiphestTask);
+  }
+
+  public function shouldAppearInActionMenu() {
+    return false;
+  }
+
+  public function getDialogTitleText() {
+    return pht('Edit Parent Tasks');
+  }
+
+  public function getDialogHeaderText() {
+    return pht('Current Parent Tasks');
+  }
+
+  public function getDialogButtonText() {
+    return pht('Save Parent Tasks');
+  }
+
+}

src/applications/maniphest/relationship/ManiphestTaskHasSubtaskRelationship.php

@@ -0,0 +1,40 @@
+<?php
+
+final class ManiphestTaskHasSubtaskRelationship
+  extends ManiphestTaskRelationship {
+
+  const RELATIONSHIPKEY = 'task.has-subtask';
+
+  public function getEdgeConstant() {
+    return ManiphestTaskDependsOnTaskEdgeType::EDGECONST;
+  }
+
+  protected function getActionName() {
+    return pht('Edit Subtasks');
+  }
+
+  protected function getActionIcon() {
+    return 'fa-chevron-circle-down';
+  }
+
+  public function canRelateObjects($src, $dst) {
+    return ($dst instanceof ManiphestTask);
+  }
+
+  public function shouldAppearInActionMenu() {
+    return false;
+  }
+
+  public function getDialogTitleText() {
+    return pht('Edit Subtasks');
+  }
+
+  public function getDialogHeaderText() {
+    return pht('Current Subtasks');
+  }
+
+  public function getDialogButtonText() {
+    return pht('Save Subtasks');
+  }
+
+}

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -500,24 +500,24 @@ final class ManiphestTransaction
 
         if ($this->getMetadataValue('blocker.new')) {
           return pht(
-            '%s created blocking task %s.',
+            '%s created subtask %s.',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid));
         } else if ($old_closed && !$new_closed) {
           return pht(
-            '%s reopened blocking task %s as "%s".',
+            '%s reopened subtask %s as "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),
             $new_name);
         } else if (!$old_closed && $new_closed) {
           return pht(
-            '%s closed blocking task %s as "%s".',
+            '%s closed subtask %s as "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),
             $new_name);
         } else {
           return pht(
-            '%s changed the status of blocking task %s from "%s" to "%s".',
+            '%s changed the status of subtask %s from "%s" to "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),
             $old_name,
@@ -753,21 +753,21 @@ final class ManiphestTransaction
 
         if ($old_closed && !$new_closed) {
           return pht(
-            '%s reopened %s, a task blocking %s, as "%s".',
+            '%s reopened %s, a subtask of %s, as "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),
             $this->renderHandleLink($object_phid),
             $new_name);
         } else if (!$old_closed && $new_closed) {
           return pht(
-            '%s closed %s, a task blocking %s, as "%s".',
+            '%s closed %s, a subtask of %s, as "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),
             $this->renderHandleLink($object_phid),
             $new_name);
         } else {
           return pht(
-            '%s changed the status of %s, a task blocking %s, '.
+            '%s changed the status of %s, a subtasktask of %s, '.
             'from "%s" to "%s".',
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($blocker_phid),

src/applications/search/controller/PhabricatorSearchRelationshipController.php

@@ -140,6 +140,8 @@ final class PhabricatorSearchRelationshipController
       ManiphestTaskHasCommitEdgeType::EDGECONST => 'CMIT',
       ManiphestTaskHasMockEdgeType::EDGECONST => 'MOCK',
       ManiphestTaskHasRevisionEdgeType::EDGECONST => 'DREV',
+      ManiphestTaskDependsOnTaskEdgeType::EDGECONST => 'TASK',
+      ManiphestTaskDependedOnByTaskEdgeType::EDGECONST => 'TASK',
     );
 
     $edge_type = $relationship->getEdgeConstant();
@@ -180,12 +182,14 @@ final class PhabricatorSearchRelationshipController
 
     $message = pht(
       'You can not create that relationship because it would create a '.
-      'circular dependency: %s.',
+      'circular dependency:');
-      implode(" \xE2\x86\x92 ", $names));
+
+    $list = implode(" \xE2\x86\x92 ", $names);
 
     return $this->newDialog()
       ->setTitle(pht('Circular Dependency'))
       ->appendParagraph($message)
+      ->appendParagraph($list)
       ->addCancelButton($done_uri);
   }
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -931,6 +931,7 @@ abstract class PhabricatorApplicationTransactionEditor
       $object->openTransaction();
     }
 
+    try {
       foreach ($xactions as $xaction) {
         $this->applyInternalEffects($object, $xaction);
       }
@@ -940,8 +941,6 @@ abstract class PhabricatorApplicationTransactionEditor
       try {
         $object->save();
       } catch (AphrontDuplicateKeyQueryException $ex) {
-        $object->killTransaction();
-
         // This callback has an opportunity to throw a better exception,
         // so execution may end here.
         $this->didCatchDuplicateKeyException($object, $xactions, $ex);
@@ -973,7 +972,11 @@ abstract class PhabricatorApplicationTransactionEditor
         $read_locking = false;
       }
 
-    $object->saveTransaction();
+      $object->saveTransaction();
+    } catch (Exception $ex) {
+      $object->killTransaction();
+      throw $ex;
+    }
 
     // Now that we've completely applied the core transaction set, try to apply
     // Herald rules. Herald rules are allowed to either take direct actions on

src/infrastructure/edges/editor/PhabricatorEdgeEditor.php

@@ -134,7 +134,6 @@ final class PhabricatorEdgeEditor extends Phobject {
       $caught = $ex;
     }
 
-
     if ($caught) {
       $this->killTransactions();
     }

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -206,73 +206,73 @@ final class PhabricatorUSEnglishTranslation
         ),
       ),
 
-      '%s added %s blocking task(s): %s.' => array(
+      '%s added %s subtask(s): %s.' => array(
         array(
-          '%s added a blocking task: %3$s.',
+          '%s added a subtask: %3$s.',
-          '%s added blocking tasks: %3$s.',
+          '%s added subtasks: %3$s.',
         ),
       ),
 
-      '%s added %s blocked task(s): %s.' => array(
+      '%s added %s parent task(s): %s.' => array(
         array(
-          '%s added a blocked task: %3$s.',
+          '%s added a parent task: %3$s.',
-          '%s added blocked tasks: %3$s.',
+          '%s added parent tasks: %3$s.',
         ),
       ),
 
-      '%s removed %s blocking task(s): %s.' => array(
+      '%s removed %s subtask(s): %s.' => array(
         array(
-          '%s removed a blocking task: %3$s.',
+          '%s removed a subtask: %3$s.',
-          '%s removed blocking tasks: %3$s.',
+          '%s removed subtasks: %3$s.',
         ),
       ),
 
-      '%s removed %s blocked task(s): %s.' => array(
+      '%s removed %s parent task(s): %s.' => array(
         array(
-          '%s removed a blocked task: %3$s.',
+          '%s removed a parent task: %3$s.',
-          '%s removed blocked tasks: %3$s.',
+          '%s removed parent tasks: %3$s.',
         ),
       ),
 
-      '%s added %s blocking task(s) for %s: %s.' => array(
+      '%s added %s subtask(s) for %s: %s.' => array(
         array(
-          '%s added a blocking task for %3$s: %4$s.',
+          '%s added a subtask for %3$s: %4$s.',
-          '%s added blocking tasks for %3$s: %4$s.',
+          '%s added subtasks for %3$s: %4$s.',
         ),
       ),
 
-      '%s added %s blocked task(s) for %s: %s.' => array(
+      '%s added %s parent task(s) for %s: %s.' => array(
         array(
-          '%s added a blocked task for %3$s: %4$s.',
+          '%s added a parent task for %3$s: %4$s.',
-          '%s added blocked tasks for %3$s: %4$s.',
+          '%s added parent tasks for %3$s: %4$s.',
         ),
       ),
 
-      '%s removed %s blocking task(s) for %s: %s.' => array(
+      '%s removed %s subtask(s) for %s: %s.' => array(
         array(
-          '%s removed a blocking task for %3$s: %4$s.',
+          '%s removed a subtask for %3$s: %4$s.',
-          '%s removed blocking tasks for %3$s: %4$s.',
+          '%s removed subtasks for %3$s: %4$s.',
         ),
       ),
 
-      '%s removed %s blocked task(s) for %s: %s.' => array(
+      '%s removed %s parent task(s) for %s: %s.' => array(
         array(
-          '%s removed a blocked task for %3$s: %4$s.',
+          '%s removed a parent task for %3$s: %4$s.',
-          '%s removed blocked tasks for %3$s: %4$s.',
+          '%s removed parent tasks for %3$s: %4$s.',
         ),
       ),
 
-      '%s edited blocking task(s), added %s: %s; removed %s: %s.' =>
+      '%s edited subtask(s), added %s: %s; removed %s: %s.' =>
-        '%s edited blocking tasks, added: %3$s; removed: %5$s.',
+        '%s edited subtasks, added: %3$s; removed: %5$s.',
 
-      '%s edited blocking task(s) for %s, added %s: %s; removed %s: %s.' =>
+      '%s edited subtask(s) for %s, added %s: %s; removed %s: %s.' =>
-        '%s edited blocking tasks for %s, added: %4$s; removed: %6$s.',
+        '%s edited subtasks for %s, added: %4$s; removed: %6$s.',
 
-      '%s edited blocked task(s), added %s: %s; removed %s: %s.' =>
+      '%s edited parent task(s), added %s: %s; removed %s: %s.' =>
-        '%s edited blocked tasks, added: %3$s; removed: %5$s.',
+        '%s edited parent tasks, added: %3$s; removed: %5$s.',
 
-      '%s edited blocked task(s) for %s, added %s: %s; removed %s: %s.' =>
+      '%s edited parent task(s) for %s, added %s: %s; removed %s: %s.' =>
-        '%s edited blocked tasks for %s, added: %4$s; removed: %6$s.',
+        '%s edited parent tasks for %s, added: %4$s; removed: %6$s.',
 
       '%s edited answer(s), added %s: %s; removed %d: %s.' =>
         '%s edited answers, added: %3$s; removed: %5$s.',