From 2cb779575dd593bfdae8d751da90022479d530a8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 Jun 2016 10:24:54 -0700 Subject: [PATCH] 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 | 4 ++ .../PhabricatorAuthStartController.php | 8 +-- .../ManiphestTaskDetailController.php | 26 ++++---- .../ManiphestTaskDependedOnByTaskEdgeType.php | 12 ++-- .../ManiphestTaskDependsOnTaskEdgeType.php | 12 ++-- .../editor/ManiphestTransactionEditor.php | 2 +- .../ManiphestTaskHasParentRelationship.php | 40 ++++++++++++ .../ManiphestTaskHasSubtaskRelationship.php | 40 ++++++++++++ .../storage/ManiphestTransaction.php | 14 ++-- ...habricatorSearchRelationshipController.php | 8 ++- ...habricatorApplicationTransactionEditor.php | 9 ++- .../edges/editor/PhabricatorEdgeEditor.php | 1 - .../PhabricatorUSEnglishTranslation.php | 64 +++++++++---------- 13 files changed, 166 insertions(+), 74 deletions(-) create mode 100644 src/applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php create mode 100644 src/applications/maniphest/relationship/ManiphestTaskHasSubtaskRelationship.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9f2625db8f..68fab974c6 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php index 982252d29f..5cfcb9b9ef 100644 --- a/src/applications/auth/controller/PhabricatorAuthStartController.php +++ b/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 - // task page). After they log in we want to send them back to that main - // object page if we can, since it's confusing to end up on a standalone - // page with only a dialog (particularly if that dialog is another error, + // (for example, they might click "Edit Subtasks" on a Maniphest task + // page). After they log in we want to send them back to that main object + // page if we can, since it's confusing to end up on a standalone page with + // only a dialog (particularly if that dialog is another error, // like a policy exception). $via_header = AphrontRequest::getViaHeaderName(); diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index a2de17c341..bc862a1122 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/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()) - ->setName(pht('Edit Blocking Tasks')) - ->setHref("/search/attach/{$phid}/TASK/blocks/") - ->setIcon('fa-link') - ->setDisabled(!$can_edit) - ->setWorkflow(true); + $relationship_list = PhabricatorObjectRelationshipList::newForObject( + $viewer, + $task); + + $parent_key = ManiphestTaskHasParentRelationship::RELATIONSHIPKEY; + $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 diff --git a/src/applications/maniphest/edge/ManiphestTaskDependedOnByTaskEdgeType.php b/src/applications/maniphest/edge/ManiphestTaskDependedOnByTaskEdgeType.php index a16f1ac530..1688eddace 100644 --- a/src/applications/maniphest/edge/ManiphestTaskDependedOnByTaskEdgeType.php +++ b/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, diff --git a/src/applications/maniphest/edge/ManiphestTaskDependsOnTaskEdgeType.php b/src/applications/maniphest/edge/ManiphestTaskDependsOnTaskEdgeType.php index e49e428add..62ef49be90 100644 --- a/src/applications/maniphest/edge/ManiphestTaskDependsOnTaskEdgeType.php +++ b/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, diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index da0d12db1c..0a4ea8469a 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/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 => diff --git a/src/applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php b/src/applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php new file mode 100644 index 0000000000..8d7d3a539f --- /dev/null +++ b/src/applications/maniphest/relationship/ManiphestTaskHasParentRelationship.php @@ -0,0 +1,40 @@ +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), diff --git a/src/applications/search/controller/PhabricatorSearchRelationshipController.php b/src/applications/search/controller/PhabricatorSearchRelationshipController.php index c97c020b0a..6af121d1ba 100644 --- a/src/applications/search/controller/PhabricatorSearchRelationshipController.php +++ b/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.', - implode(" \xE2\x86\x92 ", $names)); + 'circular dependency:'); + + $list = implode(" \xE2\x86\x92 ", $names); return $this->newDialog() ->setTitle(pht('Circular Dependency')) ->appendParagraph($message) + ->appendParagraph($list) ->addCancelButton($done_uri); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 7443de7edc..1a3dd1c89e 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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 diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index 42909ee6e2..700a11ea36 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -134,7 +134,6 @@ final class PhabricatorEdgeEditor extends Phobject { $caught = $ex; } - if ($caught) { $this->killTransactions(); } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index c49069fcae..23cb079297 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/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 blocking tasks: %3$s.', + '%s added a subtask: %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 blocked tasks: %3$s.', + '%s added a parent task: %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 blocking tasks: %3$s.', + '%s removed a subtask: %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 blocked tasks: %3$s.', + '%s removed a parent task: %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 blocking tasks for %3$s: %4$s.', + '%s added a subtask 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 blocked tasks for %3$s: %4$s.', + '%s added a parent task 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 blocking tasks for %3$s: %4$s.', + '%s removed a subtask 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 blocked tasks for %3$s: %4$s.', + '%s removed a parent task 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 blocking tasks, added: %3$s; removed: %5$s.', + '%s edited subtask(s), added %s: %s; removed %s: %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 blocking tasks for %s, added: %4$s; removed: %6$s.', + '%s edited subtask(s) for %s, added %s: %s; removed %s: %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 blocked tasks, added: %3$s; removed: %5$s.', + '%s edited parent task(s), added %s: %s; removed %s: %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 blocked tasks for %s, added: %4$s; removed: %6$s.', + '%s edited parent task(s) for %s, added %s: %s; removed %s: %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.', -- 2.51.2