diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php index c6e053efc3..f4df0e4fdc 100644 --- a/src/applications/maniphest/constants/ManiphestTaskStatus.php +++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php @@ -1,8 +1,5 @@ $open, self::STATUS_CLOSED_RESOLVED => $resolved, self::STATUS_CLOSED_WONTFIX => $wontfix, @@ -31,6 +28,17 @@ final class ManiphestTaskStatus extends ManiphestConstants { self::STATUS_CLOSED_DUPLICATE => $duplicate, self::STATUS_CLOSED_SPITE => $spite, ); + + $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); + if (!$is_serious) { + $statuses[self::STATUS_CLOSED_SPITE] = pht('Spite'); + } + + return $statuses; + } + + public static function getTaskStatusName($status) { + return idx(self::getTaskStatusMap(), $status, pht('Unknown Status')); } public static function getTaskStatusFullName($status) { @@ -103,12 +111,26 @@ final class ManiphestTaskStatus extends ManiphestConstants { return self::STATUS_OPEN; } + public static function getDefaultClosedStatus() { + return self::STATUS_CLOSED_RESOLVED; + } + + public static function getDuplicateStatus() { + return self::STATUS_CLOSED_DUPLICATE; + } + public static function getOpenStatusConstants() { return array( self::STATUS_OPEN, ); } + public static function getClosedStatusConstants() { + $all = array_keys(self::getTaskStatusMap()); + $open = self::getOpenStatusConstants(); + return array_diff($all, $open); + } + public static function isOpenStatus($status) { foreach (self::getOpenStatusConstants() as $constant) { if ($status == $constant) { @@ -118,6 +140,35 @@ final class ManiphestTaskStatus extends ManiphestConstants { return false; } + public static function isClosedStatus($status) { + return !self::isOpenStatus($status); + } + + public static function getStatusActionName($status) { + switch ($status) { + case self::STATUS_CLOSED_SPITE: + return pht('Spited'); + } + return null; + } + + public static function getStatusColor($status) { + if (self::isOpenStatus($status)) { + return 'green'; + } + return 'black'; + } + + public static function getStatusIcon($status) { + switch ($status) { + case ManiphestTaskStatus::STATUS_CLOSED_SPITE: + return 'dislike'; + case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE: + return 'delete'; + } + } + + public static function getStatusPrefixMap() { return array( 'resolve' => self::STATUS_CLOSED_RESOLVED, diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 8d64cb12d7..b123ae96d9 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -151,7 +151,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $transaction_types = array( PhabricatorTransactions::TYPE_COMMENT => pht('Comment'), - ManiphestTransaction::TYPE_STATUS => pht('Close Task'), + ManiphestTransaction::TYPE_STATUS => pht('Change Status'), ManiphestTransaction::TYPE_OWNER => pht('Reassign / Claim'), ManiphestTransaction::TYPE_CCS => pht('Add CCs'), ManiphestTransaction::TYPE_PRIORITY => pht('Change Priority'), @@ -180,21 +180,14 @@ final class ManiphestTaskDetailController extends ManiphestController { } } - if ($task->getStatus() == ManiphestTaskStatus::STATUS_OPEN) { - $resolution_types = array_select_keys( - $resolution_types, - array( - ManiphestTaskStatus::STATUS_CLOSED_RESOLVED, - ManiphestTaskStatus::STATUS_CLOSED_WONTFIX, - ManiphestTaskStatus::STATUS_CLOSED_INVALID, - ManiphestTaskStatus::STATUS_CLOSED_SPITE, - )); - } else { - $resolution_types = array( - ManiphestTaskStatus::STATUS_OPEN => 'Reopened', - ); - $transaction_types[ManiphestTransaction::TYPE_STATUS] = - 'Reopen Task'; + // Don't show an option to change to the current status, or to change to + // the duplicate status explicitly. + unset($resolution_types[$task->getStatus()]); + unset($resolution_types[ManiphestTaskStatus::getDuplicateStatus()]); + + // Don't show owner/priority changes for closed tasks, as they don't make + // much sense. + if ($task->isClosed()) { unset($transaction_types[ManiphestTransaction::TYPE_PRIORITY]); unset($transaction_types[ManiphestTransaction::TYPE_OWNER]); } @@ -215,12 +208,6 @@ final class ManiphestTaskDetailController extends ManiphestController { $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); - if ($is_serious) { - // Prevent tasks from being closed "out of spite" in serious business - // installs. - unset($resolution_types[ManiphestTaskStatus::STATUS_CLOSED_SPITE]); - } - $comment_form = new AphrontFormView(); $comment_form ->setUser($user) @@ -236,7 +223,7 @@ final class ManiphestTaskDetailController extends ManiphestController { ->setID('transaction-action')) ->appendChild( id(new AphrontFormSelectControl()) - ->setLabel(pht('Resolution')) + ->setLabel(pht('Status')) ->setName('resolution') ->setControlID('resolution') ->setControlStyle('display: none') diff --git a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php index b44060f461..52420fc394 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php @@ -147,9 +147,9 @@ final class ManiphestTransactionSaveController extends ManiphestController { $added_ccs[] = $task->getOwnerPHID(); break; case ManiphestTransaction::TYPE_STATUS: + $resolution = $request->getStr('resolution'); if (!$task->getOwnerPHID() && - $request->getStr('resolution') != - ManiphestTaskStatus::STATUS_OPEN) { + ManiphestTaskStatus::isClosedStatus($resolution)) { // Closing an unassigned task. Assign the user as the owner of // this task. $assign = new ManiphestTransaction(); diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php index e83698c976..ccd80bdab3 100644 --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -82,7 +82,7 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { switch ($command) { case 'close': $ttype = ManiphestTransaction::TYPE_STATUS; - $new_value = ManiphestTaskStatus::STATUS_CLOSED_RESOLVED; + $new_value = ManiphestTaskStatus::getDefaultClosedStatus(); break; case 'claim': $ttype = ManiphestTransaction::TYPE_OWNER; diff --git a/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php b/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php index 99bcd1d8ac..7e1145fbd1 100644 --- a/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php +++ b/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php @@ -38,7 +38,7 @@ final class ManiphestPHIDTypeTask extends PhabricatorPHIDType { $handle->setFullName("T{$id}: {$title}"); $handle->setURI("/T{$id}"); - if (!ManiphestTaskStatus::isOpenStatus($task->getStatus())) { + if ($task->isClosed()) { $handle->setStatus(PhabricatorObjectHandleStatus::STATUS_CLOSED); } } diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 740856b4aa..7f791b3661 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -349,9 +349,15 @@ final class ManiphestTaskQuery case self::STATUS_ANY: return null; case self::STATUS_OPEN: - return 'status = 0'; + return qsprintf( + $conn, + 'status IN (%Ld)', + ManiphestTaskStatus::getOpenStatusConstants()); case self::STATUS_CLOSED: - return 'status > 0'; + return qsprintf( + $conn, + 'status IN (%Ld)', + ManiphestTaskStatus::getClosedStatusConstants()); default: $constant = idx($map, $this->status); if (!$constant) { diff --git a/src/applications/maniphest/search/ManiphestSearchIndexer.php b/src/applications/maniphest/search/ManiphestSearchIndexer.php index 41e699d762..9e88ccb267 100644 --- a/src/applications/maniphest/search/ManiphestSearchIndexer.php +++ b/src/applications/maniphest/search/ManiphestSearchIndexer.php @@ -31,9 +31,9 @@ final class ManiphestSearchIndexer $task->getDateCreated()); $doc->addRelationship( - (ManiphestTaskStatus::isOpenStatus($task->getStatus())) - ? PhabricatorSearchRelationship::RELATIONSHIP_OPEN - : PhabricatorSearchRelationship::RELATIONSHIP_CLOSED, + $task->isClosed() + ? PhabricatorSearchRelationship::RELATIONSHIP_CLOSED + : PhabricatorSearchRelationship::RELATIONSHIP_OPEN, $task->getPHID(), ManiphestPHIDTypeTask::TYPECONST, time()); diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index f8d2e9b645..0ad93dc04e 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -156,6 +156,9 @@ final class ManiphestTask extends ManiphestDAO return $result; } + public function isClosed() { + return ManiphestTaskStatus::isClosedStatus($this->getStatus()); + } /* -( Markup Interface )--------------------------------------------------- */ @@ -247,6 +250,7 @@ final class ManiphestTask extends ManiphestDAO /* -( PhabricatorTokenReceiverInterface )---------------------------------- */ + public function getUsersToNotifyOfTokenGiven() { // Sort of ambiguous who this was intended for; just let them both know. return array_filter( diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index f708fa843a..f6617530e9 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -139,11 +139,11 @@ final class ManiphestTransaction } case self::TYPE_STATUS: - if ($new == ManiphestTaskStatus::STATUS_OPEN) { - return 'green'; - } else { - return 'black'; + $color = ManiphestTaskStatus::getStatusColor($new); + if ($color !== null) { + return $color; } + break; case self::TYPE_PRIORITY: if ($old == ManiphestTaskPriority::getDefaultPriority()) { @@ -168,19 +168,24 @@ final class ManiphestTransaction return pht('Retitled'); case self::TYPE_STATUS: - switch ($new) { - case ManiphestTaskStatus::STATUS_OPEN: - if ($old === null) { - return pht('Created'); - } else { - return pht('Reopened'); - } - case ManiphestTaskStatus::STATUS_CLOSED_SPITE: - return pht('Spited'); - case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE: - return pht('Merged'); - default: - return pht('Closed'); + if ($old === null) { + return pht('Created'); + } + + $action = ManiphestTaskStatus::getStatusActionName($new); + if ($action) { + return $action; + } + + $old_closed = ManiphestTaskStatus::isClosedStatus($old); + $new_closed = ManiphestTaskStatus::isClosedStatus($new); + + if ($new_closed && !$old_closed) { + return pht('Closed'); + } else if (!$new_closed && $old_closed) { + return pht('Reopened'); + } else { + return pht('Changed Status'); } case self::TYPE_DESCRIPTION: @@ -238,15 +243,19 @@ final class ManiphestTransaction return 'edit'; case self::TYPE_STATUS: - switch ($new) { - case ManiphestTaskStatus::STATUS_OPEN: - return 'create'; - case ManiphestTaskStatus::STATUS_CLOSED_SPITE: - return 'dislike'; - case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE: - return 'delete'; - default: - return 'check'; + if ($old === null) { + return 'create'; + } + + $action = ManiphestTaskStatus::getStatusIcon($new); + if ($action !== null) { + return $action; + } + + if (ManiphestTaskStatus::isClosedStatus($new)) { + return 'check'; + } else { + return 'edit'; } case self::TYPE_DESCRIPTION: @@ -299,35 +308,40 @@ final class ManiphestTransaction $this->renderHandleLink($author_phid)); case self::TYPE_STATUS: - switch ($new) { - case ManiphestTaskStatus::STATUS_OPEN: - if ($old === null) { - return pht( - '%s created this task.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s reopened this task.', - $this->renderHandleLink($author_phid)); - } + if ($old === null) { + return pht( + '%s created this task.', + $this->renderHandleLink($author_phid)); + } - case ManiphestTaskStatus::STATUS_CLOSED_SPITE: - return pht( - '%s closed this task out of spite.', - $this->renderHandleLink($author_phid)); - case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE: + $old_closed = ManiphestTaskStatus::isClosedStatus($old); + $new_closed = ManiphestTaskStatus::isClosedStatus($new); + + $old_name = ManiphestTaskStatus::getTaskStatusName($old); + $new_name = ManiphestTaskStatus::getTaskStatusName($new); + + if ($new_closed && !$old_closed) { + if ($new == ManiphestTaskStatus::getDuplicateStatus()) { return pht( '%s closed this task as a duplicate.', $this->renderHandleLink($author_phid)); - default: - $status_name = idx( - ManiphestTaskStatus::getTaskStatusMap(), - $new, - '???'); + } else { return pht( '%s closed this task as "%s".', $this->renderHandleLink($author_phid), - $status_name); + $new_name); + } + } else if (!$new_closed && $old_closed) { + return pht( + '%s reopened this task as "%s".', + $this->renderHandleLink($author_phid), + $new_name); + } else { + return pht( + '%s changed the task status from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old_name, + $new_name); } case self::TYPE_OWNER: @@ -488,40 +502,45 @@ final class ManiphestTransaction $this->renderHandleLink($object_phid)); case self::TYPE_STATUS: - switch ($new) { - case ManiphestTaskStatus::STATUS_OPEN: - if ($old === null) { - return pht( - '%s created %s.', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid)); - } else { - return pht( - '%s reopened %s.', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid)); - } + if ($old === null) { + return pht( + '%s created %s.', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid)); + } - case ManiphestTaskStatus::STATUS_CLOSED_SPITE: - return pht( - '%s closed %s out of spite.', - $this->renderHandleLink($author_phid), - $this->renderHandleLink($object_phid)); - case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE: + $old_closed = ManiphestTaskStatus::isClosedStatus($old); + $new_closed = ManiphestTaskStatus::isClosedStatus($new); + + $old_name = ManiphestTaskStatus::getTaskStatusName($old); + $new_name = ManiphestTaskStatus::getTaskStatusName($new); + + if ($new_closed && !$old_closed) { + if ($new == ManiphestTaskStatus::getDuplicateStatus()) { return pht( '%s closed %s as a duplicate.', $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid)); - default: - $status_name = idx( - ManiphestTaskStatus::getTaskStatusMap(), - $new, - '???'); + } else { return pht( '%s closed %s as "%s".', $this->renderHandleLink($author_phid), $this->renderHandleLink($object_phid), - $status_name); + $new_name); + } + } else if (!$new_closed && $old_closed) { + return pht( + '%s reopened %s as "%s".', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid), + $new_name); + } else { + return pht( + '%s changed the status of %s from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $this->renderHandleLink($object_phid), + $old_name, + $new_name); } case self::TYPE_OWNER: diff --git a/src/applications/maniphest/view/ManiphestTaskListView.php b/src/applications/maniphest/view/ManiphestTaskListView.php index 7d63cddddb..16c5be5dfb 100644 --- a/src/applications/maniphest/view/ManiphestTaskListView.php +++ b/src/applications/maniphest/view/ManiphestTaskListView.php @@ -58,7 +58,7 @@ final class ManiphestTaskListView extends ManiphestView { } $status = $task->getStatus(); - if ($status != ManiphestTaskStatus::STATUS_OPEN) { + if ($task->isClosed()) { $item->setDisabled(true); } diff --git a/src/applications/maniphest/view/ManiphestView.php b/src/applications/maniphest/view/ManiphestView.php index e15765507f..ec121f86dd 100644 --- a/src/applications/maniphest/view/ManiphestView.php +++ b/src/applications/maniphest/view/ManiphestView.php @@ -1,8 +1,5 @@ setTransactionType(ManiphestTransaction::TYPE_STATUS) - ->setNewValue(ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE); + ->setNewValue(ManiphestTaskStatus::getDuplicateStatus()); $merge_comment = id(new ManiphestTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) diff --git a/webroot/rsrc/js/application/maniphest/behavior-batch-editor.js b/webroot/rsrc/js/application/maniphest/behavior-batch-editor.js index 538a1f944e..9345607939 100644 --- a/webroot/rsrc/js/application/maniphest/behavior-batch-editor.js +++ b/webroot/rsrc/js/application/maniphest/behavior-batch-editor.js @@ -24,7 +24,7 @@ JX.behavior('maniphest-batch-editor', function(config) { 'add_project': 'Add Projects', 'remove_project' : 'Remove Projects', 'priority': 'Change Priority', - 'status': 'Open / Close', + 'status': 'Change Status', 'add_comment': 'Comment', 'assign': 'Assign', 'add_ccs' : 'Add CCs',