Despecialize most task status handling

Summary: Ref T1812. Moves most specialized status handling into `ManiphestTaskStatus`. The only real missing case is reports.

Test Plan:
Browsed most of the affected interfaces. Changed task status:

{F132697}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1812

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

src/applications/maniphest/constants/ManiphestTaskStatus.php

@@ -1,8 +1,5 @@
 <?php
 
-/**
- * @group maniphest
- */
 final class ManiphestTaskStatus extends ManiphestConstants {
 
   const STATUS_OPEN               = 0;
@@ -23,7 +20,7 @@ final class ManiphestTaskStatus extends ManiphestConstants {
     $duplicate = pht('Duplicate');
     $spite = pht('Spite');
 
-    return array(
+    $statuses = array(
       self::STATUS_OPEN                 => $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,

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) {
+    // Don't show an option to change to the current status, or to change to
-      $resolution_types = array_select_keys(
+    // the duplicate status explicitly.
-        $resolution_types,
+    unset($resolution_types[$task->getStatus()]);
-        array(
+    unset($resolution_types[ManiphestTaskStatus::getDuplicateStatus()]);
-          ManiphestTaskStatus::STATUS_CLOSED_RESOLVED,
+
-          ManiphestTaskStatus::STATUS_CLOSED_WONTFIX,
+    // Don't show owner/priority changes for closed tasks, as they don't make
-          ManiphestTaskStatus::STATUS_CLOSED_INVALID,
+    // much sense.
-          ManiphestTaskStatus::STATUS_CLOSED_SPITE,
+    if ($task->isClosed()) {
-        ));
-    } else {
-      $resolution_types = array(
-        ManiphestTaskStatus::STATUS_OPEN => 'Reopened',
-      );
-      $transaction_types[ManiphestTransaction::TYPE_STATUS] =
-        'Reopen Task';
       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')

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::isClosedStatus($resolution)) {
-            ManiphestTaskStatus::STATUS_OPEN) {
           // Closing an unassigned task. Assign the user as the owner of
           // this task.
           $assign = new ManiphestTransaction();

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;

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

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

src/applications/maniphest/search/ManiphestSearchIndexer.php

@@ -31,9 +31,9 @@ final class ManiphestSearchIndexer
       $task->getDateCreated());
 
     $doc->addRelationship(
-      (ManiphestTaskStatus::isOpenStatus($task->getStatus()))
+      $task->isClosed()
-        ? PhabricatorSearchRelationship::RELATIONSHIP_OPEN
+        ? PhabricatorSearchRelationship::RELATIONSHIP_CLOSED
-        : PhabricatorSearchRelationship::RELATIONSHIP_CLOSED,
+        : PhabricatorSearchRelationship::RELATIONSHIP_OPEN,
       $task->getPHID(),
       ManiphestPHIDTypeTask::TYPECONST,
       time());

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(

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -139,11 +139,11 @@ final class ManiphestTransaction
         }
 
       case self::TYPE_STATUS:
-        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
+        $color = ManiphestTaskStatus::getStatusColor($new);
-          return 'green';
+        if ($color !== null) {
-        } else {
+          return $color;
-          return 'black';
         }
+        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');
+        $action = ManiphestTaskStatus::getStatusActionName($new);
-          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+        if ($action) {
-            return pht('Merged');
+          return $action;
-          default:
+        }
+
+        $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) {
+        if ($old === null) {
-          case ManiphestTaskStatus::STATUS_OPEN:
           return 'create';
-          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
+        }
-            return 'dislike';
+
-          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+        $action = ManiphestTaskStatus::getStatusIcon($new);
-            return 'delete';
+        if ($action !== null) {
-          default:
+          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));
         }
 
-          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
+        $old_closed = ManiphestTaskStatus::isClosedStatus($old);
-            return pht(
+        $new_closed = ManiphestTaskStatus::isClosedStatus($new);
-              '%s closed this task out of spite.',
+
-              $this->renderHandleLink($author_phid));
+        $old_name = ManiphestTaskStatus::getTaskStatusName($old);
-          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+        $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:
+          } else {
-            $status_name = idx(
-              ManiphestTaskStatus::getTaskStatusMap(),
-              $new,
-              '???');
             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));
         }
 
-          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
+        $old_closed = ManiphestTaskStatus::isClosedStatus($old);
-            return pht(
+        $new_closed = ManiphestTaskStatus::isClosedStatus($new);
-              '%s closed %s out of spite.',
+
-              $this->renderHandleLink($author_phid),
+        $old_name = ManiphestTaskStatus::getTaskStatusName($old);
-              $this->renderHandleLink($object_phid));
+        $new_name = ManiphestTaskStatus::getTaskStatusName($new);
-          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+
+        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:
+          } else {
-            $status_name = idx(
-              ManiphestTaskStatus::getTaskStatusMap(),
-              $new,
-              '???');
             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:

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

src/applications/maniphest/view/ManiphestView.php

@@ -1,8 +1,5 @@
 <?php
 
-/**
- * @group maniphest
- */
 abstract class ManiphestView extends AphrontView {
 
   public static function renderTagForTask(ManiphestTask $task) {

src/applications/search/controller/PhabricatorSearchAttachController.php

@@ -178,7 +178,7 @@ final class PhabricatorSearchAttachController
 
       $close_task = id(new ManiphestTransaction())
         ->setTransactionType(ManiphestTransaction::TYPE_STATUS)
-        ->setNewValue(ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE);
+        ->setNewValue(ManiphestTaskStatus::getDuplicateStatus());
 
       $merge_comment = id(new ManiphestTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)

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