Normalize the definition of "closed" revision statuses

Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.

The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.

To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.

Also delete the very old COMMITTED status, which has been obsolete for over a year.

Test Plan: Browsed around most/all of the affected interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

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

src/applications/differential/conduit/ConduitAPI_differential_creatediff_Method.php

@@ -68,8 +68,7 @@ final class ConduitAPI_differential_creatediff_Method extends ConduitAPIMethod {
         ->execute();
       if ($parent_rev) {
         $parent_rev = head($parent_rev);
-        if ($parent_rev->getStatus() !=
-            ArcanistDifferentialRevisionStatus::CLOSED) {
+        if (!$parent_rev->isClosed()) {
           $diff->setParentRevisionID($parent_id);
         }
       }

src/applications/differential/constants/DifferentialRevisionStatus.php

@@ -70,4 +70,30 @@ final class DifferentialRevisionStatus {
 
     return $tag;
   }
+
+  public static function getClosedStatuses() {
+    return array(
+      ArcanistDifferentialRevisionStatus::CLOSED,
+      ArcanistDifferentialRevisionStatus::ABANDONED,
+    );
+  }
+
+  public static function getOpenStatuses() {
+    return array_diff(self::getAllStatuses(), self::getClosedStatuses());
+  }
+
+  public static function getAllStatuses() {
+    return array(
+      ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
+      ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
+      ArcanistDifferentialRevisionStatus::ACCEPTED,
+      ArcanistDifferentialRevisionStatus::CLOSED,
+      ArcanistDifferentialRevisionStatus::ABANDONED,
+    );
+  }
+
+  public static function isClosedStatus($status) {
+    return in_array($status, self::getClosedStatuses());
+  }
+
 }

src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php

@@ -78,13 +78,7 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher
   }
 
   public function isObjectClosed($object) {
-    switch ($object->getStatus()) {
-      case ArcanistDifferentialRevisionStatus::CLOSED:
-      case ArcanistDifferentialRevisionStatus::ABANDONED:
-        return true;
-      default:
-        return false;
-    }
+    return $object->isClosed();
   }
 
   public function getResponsibilityTitle($object) {

src/applications/differential/phid/DifferentialPHIDTypeRevision.php

@@ -29,11 +29,6 @@ final class DifferentialPHIDTypeRevision extends PhabricatorPHIDType {
     array $handles,
     array $objects) {
 
-    static $closed_statuses = array(
-      ArcanistDifferentialRevisionStatus::CLOSED => true,
-      ArcanistDifferentialRevisionStatus::ABANDONED => true,
-    );
-
     foreach ($handles as $phid => $handle) {
       $revision = $objects[$phid];
 
@@ -45,7 +40,7 @@ final class DifferentialPHIDTypeRevision extends PhabricatorPHIDType {
       $handle->setURI("/D{$id}");
       $handle->setFullName("D{$id}: {$title}");
 
-      if (isset($closed_statuses[$status])) {
+      if ($revision->isClosed()) {
         $handle->setStatus(PhabricatorObjectHandleStatus::STATUS_CLOSED);
       }
     }

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -23,8 +23,7 @@ final class DifferentialRevisionQuery
   const STATUS_ACCEPTED       = 'status-accepted';
   const STATUS_NEEDS_REVIEW   = 'status-needs-review';
   const STATUS_NEEDS_REVISION = 'status-needs-revision';
-  const STATUS_CLOSED         = 'status-closed';    // NOTE: Same as 'committed'
-  const STATUS_COMMITTED      = 'status-committed'; // TODO: Remove.
+  const STATUS_CLOSED         = 'status-closed';
   const STATUS_ABANDONED      = 'status-abandoned';
 
   private $authors = array();
@@ -772,11 +771,7 @@ final class DifferentialRevisionQuery
         $where[] = qsprintf(
           $conn_r,
           'r.status IN (%Ld)',
-          array(
-            ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
-            ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
-            ArcanistDifferentialRevisionStatus::ACCEPTED,
-          ));
+          DifferentialRevisionStatus::getOpenStatuses());
         break;
       case self::STATUS_NEEDS_REVIEW:
         $where[] = qsprintf(
@@ -802,19 +797,11 @@ final class DifferentialRevisionQuery
             ArcanistDifferentialRevisionStatus::ACCEPTED,
           ));
         break;
-      case self::STATUS_COMMITTED:
-        phlog(
-          "WARNING: DifferentialRevisionQuery using deprecated ".
-          "STATUS_COMMITTED constant. This will be removed soon. ".
-          "Use STATUS_CLOSED.");
-        // fallthrough
       case self::STATUS_CLOSED:
         $where[] = qsprintf(
           $conn_r,
           'r.status IN (%Ld)',
-          array(
-            ArcanistDifferentialRevisionStatus::CLOSED,
-          ));
+          DifferentialRevisionStatus::getClosedStatuses());
         break;
       case self::STATUS_ABANDONED:
         $where[] = qsprintf(

src/applications/differential/search/DifferentialSearchIndexer.php

@@ -44,8 +44,7 @@ final class DifferentialSearchIndexer
       PhabricatorPeoplePHIDTypeUser::TYPECONST,
       $rev->getDateCreated());
 
-    if ($rev->getStatus() != ArcanistDifferentialRevisionStatus::CLOSED &&
-        $rev->getStatus() != ArcanistDifferentialRevisionStatus::ABANDONED) {
+    if (!$rev->isClosed()) {
       $doc->addRelationship(
         PhabricatorSearchRelationship::RELATIONSHIP_OPEN,
         $rev->getPHID(),

src/applications/differential/storage/DifferentialRevision.php

@@ -407,4 +407,9 @@ final class DifferentialRevision extends DifferentialDAO
     $this->repository = $repository;
     return $this;
   }
+
+  public function isClosed() {
+    return DifferentialRevisionStatus::isClosedStatus($this->getStatus());
+  }
+
 }

src/applications/differential/view/DifferentialRevisionListView.php

@@ -114,11 +114,6 @@ final class DifferentialRevisionListView extends AphrontView {
     $list = new PHUIObjectItemListView();
     $list->setCards(true);
 
-    $do_not_display_age = array(
-      ArcanistDifferentialRevisionStatus::CLOSED => true,
-      ArcanistDifferentialRevisionStatus::ABANDONED => true,
-    );
-
     foreach ($this->revisions as $revision) {
       $item = id(new PHUIObjectItemView())
         ->setUser($user);
@@ -146,8 +141,7 @@ final class DifferentialRevisionListView extends AphrontView {
       $status = $revision->getStatus();
       $show_age = ($fresh || $stale) &&
                   $this->highlightAge &&
-                  empty($do_not_display_age[$status]);
-
+                  !$revision->isClosed();
 
       $object_age = PHUIObjectItemView::AGE_FRESH;
       foreach ($this->fields as $field) {