From 1da691113aa8698c3378dc748d243cd1f9938b37 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Nov 2013 17:39:24 -0800 Subject: [PATCH] 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 --- ...duitAPI_differential_creatediff_Method.php | 3 +-- .../constants/DifferentialRevisionStatus.php | 26 +++++++++++++++++++ ...alDoorkeeperRevisionFeedStoryPublisher.php | 8 +----- .../phid/DifferentialPHIDTypeRevision.php | 7 +---- .../query/DifferentialRevisionQuery.php | 19 +++----------- .../search/DifferentialSearchIndexer.php | 3 +-- .../storage/DifferentialRevision.php | 5 ++++ .../view/DifferentialRevisionListView.php | 8 +----- 8 files changed, 39 insertions(+), 40 deletions(-) diff --git a/src/applications/differential/conduit/ConduitAPI_differential_creatediff_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_creatediff_Method.php index 68a6284929..e74408a421 100644 --- a/src/applications/differential/conduit/ConduitAPI_differential_creatediff_Method.php +++ b/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); } } diff --git a/src/applications/differential/constants/DifferentialRevisionStatus.php b/src/applications/differential/constants/DifferentialRevisionStatus.php index 1088ac40e3..95576fc80b 100644 --- a/src/applications/differential/constants/DifferentialRevisionStatus.php +++ b/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()); + } + } diff --git a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php index 74f9117ced..5bfd5ee836 100644 --- a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php +++ b/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) { diff --git a/src/applications/differential/phid/DifferentialPHIDTypeRevision.php b/src/applications/differential/phid/DifferentialPHIDTypeRevision.php index f13bd6770f..f576e0e062 100644 --- a/src/applications/differential/phid/DifferentialPHIDTypeRevision.php +++ b/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); } } diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index de52a5cf74..ffa7bc2ff3 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/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( diff --git a/src/applications/differential/search/DifferentialSearchIndexer.php b/src/applications/differential/search/DifferentialSearchIndexer.php index 01de4dc2e9..a371b6e4e6 100644 --- a/src/applications/differential/search/DifferentialSearchIndexer.php +++ b/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(), diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 350040e773..ba7870dda6 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/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()); + } + } diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php index bc70460b26..86bab4dbc6 100644 --- a/src/applications/differential/view/DifferentialRevisionListView.php +++ b/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) {