From cc9ee66ef37fc9ae2f967c8dfb0c5837e1fee32d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 May 2014 12:23:17 -0700 Subject: [PATCH] When a task changes status, update blocked tasks Summary: Ref T5008. Three notes: - I'm not hiding these even if the status change is open -> open or closed -> closed. I think these are OK, but might be a little spammy. - These show in feed, but shouldn't, since they're very redundant with stories which will almost always appear adjacently. Probably a bit spammy, see TODO. We can't hide them from feed without also squelching the notifications right now, which I //don't// want to do. - You get a notification even if you're on the original task which changed status. This is definitely spammy, see other TODO. Test Plan: {F156217} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5008 Differential Revision: https://secure.phabricator.com/D9166 --- .../editor/ManiphestTransactionEditor.php | 59 ++++++++++++++++ .../storage/ManiphestTransaction.php | 70 ++++++++++++++++++- 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index f7d9a957b1..41485b2ac9 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -19,6 +19,7 @@ final class ManiphestTransactionEditor $types[] = ManiphestTransaction::TYPE_EDGE; $types[] = ManiphestTransaction::TYPE_SUBPRIORITY; $types[] = ManiphestTransaction::TYPE_PROJECT_COLUMN; + $types[] = ManiphestTransaction::TYPE_UNBLOCK; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -84,6 +85,7 @@ final class ManiphestTransactionEditor case ManiphestTransaction::TYPE_EDGE: case ManiphestTransaction::TYPE_SUBPRIORITY: case ManiphestTransaction::TYPE_PROJECT_COLUMN: + case ManiphestTransaction::TYPE_UNBLOCK: return $xaction->getNewValue(); } } @@ -244,6 +246,63 @@ final class ManiphestTransactionEditor } } + protected function applyFinalEffects( + PhabricatorLiskDAO $object, + array $xactions) { + + // When we change the status of a task, update tasks this tasks blocks + // with a message to the effect of "alincoln resolved blocking task Txxx." + $unblock_xaction = null; + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_STATUS: + $unblock_xaction = $xaction; + break; + } + } + + if ($unblock_xaction !== null) { + $blocked_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $object->getPHID(), + PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK); + if ($blocked_phids) { + // In theory we could apply these through policies, but that seems a + // little bit surprising. For now, use the actor's vision. + $blocked_tasks = id(new ManiphestTaskQuery()) + ->setViewer($this->getActor()) + ->withPHIDs($blocked_phids) + ->execute(); + + $old = $unblock_xaction->getOldValue(); + $new = $unblock_xaction->getNewValue(); + + foreach ($blocked_tasks as $blocked_task) { + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_UNBLOCK) + ->setOldValue(array($object->getPHID() => $old)) + ->setNewValue(array($object->getPHID() => $new)); + + // TODO: We should avoid notifiying users about these indirect + // changes if they are getting a notification about the current + // change, so you don't get a pile of extra notifications if you are + // subscribed to this task. + + id(new ManiphestTransactionEditor()) + ->setActor($this->getActor()) + ->setContentSource($this->getContentSource()) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($blocked_task, $xactions); + } + } + } + + return $xactions; + } + + protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 5ae50e38cb..685d56f393 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -14,6 +14,8 @@ final class ManiphestTransaction const TYPE_SUBPRIORITY = 'subpriority'; const TYPE_PROJECT_COLUMN = 'projectcolumn'; + const TYPE_UNBLOCK = 'unblock'; + // NOTE: this type is deprecated. Keep it around for legacy installs // so any transactions render correctly. const TYPE_ATTACH = 'attach'; @@ -34,6 +36,7 @@ final class ManiphestTransaction switch ($this->getTransactionType()) { case self::TYPE_PROJECT_COLUMN: case self::TYPE_EDGE: + case self::TYPE_UNBLOCK: return false; } @@ -87,7 +90,11 @@ final class ManiphestTransaction array_keys(idx($old, 'FILE', array())), )); break; - + case self::TYPE_UNBLOCK: + foreach (array_keys($new) as $phid) { + $phids[] = $phid; + } + break; } return $phids; @@ -233,6 +240,22 @@ final class ManiphestTransaction case self::TYPE_EDGE: case self::TYPE_ATTACH: return pht('Attached'); + + case self::TYPE_UNBLOCK: + $old_status = head($old); + $new_status = head($new); + + $old_closed = ManiphestTaskStatus::isClosedStatus($old_status); + $new_closed = ManiphestTaskStatus::isClosedStatus($new_status); + + if ($old_closed && !$new_closed) { + return pht('Block'); + } else if (!$old_closed && $new_closed) { + return pht('Unblock'); + } else { + return pht('Blocker'); + } + } return parent::getActionName(); @@ -290,6 +313,9 @@ final class ManiphestTransaction case self::TYPE_ATTACH: return 'fa-thumb-tack'; + case self::TYPE_UNBLOCK: + return 'fa-shield'; + } return parent::getIcon(); @@ -352,6 +378,38 @@ final class ManiphestTransaction $new_name); } + case self::TYPE_UNBLOCK: + $blocker_phid = key($new); + $old_status = head($old); + $new_status = head($new); + + $old_closed = ManiphestTaskStatus::isClosedStatus($old_status); + $new_closed = ManiphestTaskStatus::isClosedStatus($new_status); + + $old_name = ManiphestTaskStatus::getTaskStatusName($old_status); + $new_name = ManiphestTaskStatus::getTaskStatusName($new_status); + + if ($old_closed && !$new_closed) { + return pht( + '%s reopened blocking task %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".', + $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".', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($blocker_phid), + $old_name, + $new_name); + } + case self::TYPE_OWNER: if ($author_phid == $new) { return pht( @@ -551,6 +609,16 @@ final class ManiphestTransaction $new_name); } + case self::TYPE_UNBLOCK: + + // TODO: We should probably not show these in feed; they're highly + // redundant. For now, just use the normal titles. Right now, we can't + // publish something to noficiations without also publishing it to feed. + // Fix that, then stop these from rendering in feed only. + + break; + + case self::TYPE_OWNER: if ($author_phid == $new) { return pht(