From 2e36653965a013b671e8f3bcf0e1c07c3dc98c2b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Aug 2017 05:29:49 -0700 Subject: [PATCH] Reduce callsites to "ArcanistDifferentialRevisionStatus" in Phabricator Summary: Ref T2543. These are currently numeric values, like "0" and "3". I want to replace them with strings, like "accepted", and move definitions from Arcanist to Phabricator. To set the stage for this, reduce the number of callsites where Phabricator invokes `ArcanistDifferentialRevisionStatus`. This is just the easy ones. I'll hold this until the release cut. Test Plan: - Called `differential.find`. - Called `differential.getrevision`. - Called `differential.query`. - Removed all reviewers from a revision, saw warning. - Abandoned the no-reviewers revision, no more warning. - Attached a revision to a task to get it to show the state icon with the status on a tooltip. - Viewed revision bucketing on dashboard. - Used `bin/search index` to reindex a revision. - Hit the "Land Revision" endpoint. I didn't explicitly test these cases: - Doorkeeper Asana integration, since setup takes a thousand years. - Disambiguation logic when multiple hashes match, since setup is also very involved. - Releeph because it's Releeph. Reviewers: chad Reviewed By: chad Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18339 --- .../DifferentialFindConduitAPIMethod.php | 4 +- ...ifferentialGetRevisionConduitAPIMethod.php | 4 +- .../DifferentialQueryConduitAPIMethod.php | 4 +- .../customfield/DifferentialBranchField.php | 9 ++-- .../DifferentialReviewersField.php | 3 +- ...alDoorkeeperRevisionFeedStoryPublisher.php | 6 +-- .../phid/DifferentialRevisionPHIDType.php | 3 +- ...tialRevisionRequiredActionResultBucket.php | 15 ++---- .../DifferentialRevisionResultBucket.php | 3 +- .../DifferentialRevisionSearchEngine.php | 4 +- .../DifferentialRevisionFulltextEngine.php | 3 +- .../storage/DifferentialRevision.php | 5 ++ .../DiffusionLowLevelCommitFieldsQuery.php | 3 +- .../DrydockLandRepositoryOperation.php | 49 +++++++++---------- ...entialReleephRequestFieldSpecification.php | 3 +- 15 files changed, 49 insertions(+), 69 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php index 92dce067fe..79d3fc5d7d 100644 --- a/src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php @@ -88,9 +88,7 @@ final class DifferentialFindConduitAPIMethod 'uri' => PhabricatorEnv::getProductionURI('/D'.$id), 'dateCreated' => $revision->getDateCreated(), 'authorPHID' => $revision->getAuthorPHID(), - 'statusName' => - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( - $revision->getStatus()), + 'statusName' => $revision->getStatusDisplayName(), 'sourcePath' => $diff->getSourcePath(), ); } diff --git a/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php index b05efb5c15..9109849292 100644 --- a/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php @@ -83,9 +83,7 @@ final class DifferentialGetRevisionConduitAPIMethod 'uri' => PhabricatorEnv::getURI('/D'.$revision->getID()), 'title' => $revision->getTitle(), 'status' => $revision->getStatus(), - 'statusName' => - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( - $revision->getStatus()), + 'statusName' => $revision->getStatusDisplayName(), 'summary' => $revision->getSummary(), 'testPlan' => $revision->getTestPlan(), 'lineCount' => $revision->getLineCount(), diff --git a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php index 3b88087a67..1051983324 100644 --- a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php @@ -221,9 +221,7 @@ final class DifferentialQueryConduitAPIMethod 'dateModified' => $revision->getDateModified(), 'authorPHID' => $revision->getAuthorPHID(), 'status' => $revision->getStatus(), - 'statusName' => - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( - $revision->getStatus()), + 'statusName' => $revision->getStatusDisplayName(), 'properties' => $revision->getProperties(), 'branch' => $diff->getBranch(), 'summary' => $revision->getSummary(), diff --git a/src/applications/differential/customfield/DifferentialBranchField.php b/src/applications/differential/customfield/DifferentialBranchField.php index 16be0ce0c9..2387e3cd3f 100644 --- a/src/applications/differential/customfield/DifferentialBranchField.php +++ b/src/applications/differential/customfield/DifferentialBranchField.php @@ -76,16 +76,17 @@ final class DifferentialBranchField PhabricatorApplicationTransactionEditor $editor, array $xactions) { - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + $revision = $this->getObject(); // Show the "BRANCH" section only if there's a new diff or the revision // is "Accepted". - if ((!$editor->getDiffUpdateTransaction($xactions)) && - ($this->getObject()->getStatus() != $status_accepted)) { + $is_update = (bool)$editor->getDiffUpdateTransaction($xactions); + $is_accepted = $revision->isAccepted(); + if (!$is_update && !$is_accepted) { return; } - $branch = $this->getBranchDescription($this->getObject()->getActiveDiff()); + $branch = $this->getBranchDescription($revision->getActiveDiff()); if ($branch === null) { return; } diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index 7633bfb492..f835854e2f 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -68,8 +68,7 @@ final class DifferentialReviewersField public function getWarningsForRevisionHeader(array $handles) { $revision = $this->getObject(); - $status_needs_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - if ($revision->getStatus() != $status_needs_review) { + if (!$revision->isNeedsReview()) { return array(); } diff --git a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php index 9583486c98..59223f20da 100644 --- a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php +++ b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php @@ -35,8 +35,7 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher } public function getActiveUserPHIDs($object) { - $status = $object->getStatus(); - if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { + if ($object->isNeedsReview()) { return $object->getReviewerPHIDs(); } else { return array(); @@ -44,8 +43,7 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher } public function getPassiveUserPHIDs($object) { - $status = $object->getStatus(); - if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { + if ($object->isNeedsReview()) { return array(); } else { return $object->getReviewerPHIDs(); diff --git a/src/applications/differential/phid/DifferentialRevisionPHIDType.php b/src/applications/differential/phid/DifferentialRevisionPHIDType.php index 5a6cf701ae..8547ec9e83 100644 --- a/src/applications/differential/phid/DifferentialRevisionPHIDType.php +++ b/src/applications/differential/phid/DifferentialRevisionPHIDType.php @@ -50,8 +50,7 @@ final class DifferentialRevisionPHIDType extends PhabricatorPHIDType { $icon = DifferentialRevisionStatus::getRevisionStatusIcon($status); $color = DifferentialRevisionStatus::getRevisionStatusColor($status); - $name = ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( - $status); + $name = $revision->getStatusDisplayName(); $handle ->setStateIcon($icon) diff --git a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php index 6d83d97a47..3e6daa6317 100644 --- a/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php @@ -134,13 +134,11 @@ final class DifferentialRevisionRequiredActionResultBucket } private function filterShouldLand(array $phids) { - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { - if ($object->getStatus() != $status_accepted) { + if (!$object->isAccepted()) { continue; } @@ -175,13 +173,11 @@ final class DifferentialRevisionRequiredActionResultBucket } private function filterWaitingForReview(array $phids) { - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - $objects = $this->getRevisionsAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { - if ($object->getStatus() != $status_review) { + if (!$object->isNeedsReview()) { continue; } @@ -217,16 +213,11 @@ final class DifferentialRevisionRequiredActionResultBucket } private function filterWaitingOnOtherReviewers(array $phids) { - $statuses = array( - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, - ); - $statuses = array_fuse($statuses); - $objects = $this->getRevisionsNotAuthored($this->objects, $phids); $results = array(); foreach ($objects as $key => $object) { - if (!isset($statuses[$object->getStatus()])) { + if (!$object->isNeedsReview()) { continue; } diff --git a/src/applications/differential/query/DifferentialRevisionResultBucket.php b/src/applications/differential/query/DifferentialRevisionResultBucket.php index 762e2d97f4..54705649eb 100644 --- a/src/applications/differential/query/DifferentialRevisionResultBucket.php +++ b/src/applications/differential/query/DifferentialRevisionResultBucket.php @@ -15,9 +15,8 @@ abstract class DifferentialRevisionResultBucket $objects = $this->getRevisionsNotAuthored($objects, $phids); - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; foreach ($objects as $key => $object) { - if ($object->getStatus() != $status_review) { + if (!$object->isNeedsReview()) { continue; } diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index 4248119cb9..ef88c0ea1c 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -235,11 +235,9 @@ final class DifferentialRevisionSearchEngine } private function loadUnlandedDependencies(array $revisions) { - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - $phids = array(); foreach ($revisions as $revision) { - if ($revision->getStatus() != $status_accepted) { + if (!$revision->isAccepted()) { continue; } diff --git a/src/applications/differential/search/DifferentialRevisionFulltextEngine.php b/src/applications/differential/search/DifferentialRevisionFulltextEngine.php index 45639edbbe..cdb9f1a86d 100644 --- a/src/applications/differential/search/DifferentialRevisionFulltextEngine.php +++ b/src/applications/differential/search/DifferentialRevisionFulltextEngine.php @@ -34,8 +34,7 @@ final class DifferentialRevisionFulltextEngine // If a revision needs review, the owners are the reviewers. Otherwise, the // owner is the author (e.g., accepted, rejected, closed). - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - if ($revision->getStatus() == $status_review) { + if ($revision->isNeedsReview()) { $reviewers = $revision->getReviewerPHIDs(); $reviewers = array_fuse($reviewers); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 7ad01c02ca..cd6df4b6e5 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -626,6 +626,11 @@ final class DifferentialRevision extends DifferentialDAO return ($this->getStatus() == $status_accepted); } + public function isNeedsReview() { + $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; + return ($this->getStatus() == $status_review); + } + public function getStatusIcon() { $map = array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php index ab8f5e3e2e..a0548d5178 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php @@ -122,9 +122,8 @@ final class DiffusionLowLevelCommitFieldsQuery $revisions = array_reverse($revisions); // Try to find an accepted revision first. - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; foreach ($revisions as $revision) { - if ($revision->getStatus() == $status_accepted) { + if ($revision->isAccepted()) { return $revision; } } diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index 4e306772a3..1ccc82eb7b 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -289,31 +289,30 @@ final class DrydockLandRepositoryOperation ); } - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - if ($revision->getStatus() != $status_accepted) { - switch ($revision->getStatus()) { - case ArcanistDifferentialRevisionStatus::CLOSED: - return array( - 'title' => pht('Revision Closed'), - 'body' => pht( - 'This revision has already been closed. Only open, accepted '. - 'revisions may land.'), - ); - case ArcanistDifferentialRevisionStatus::ABANDONED: - return array( - 'title' => pht('Revision Abandoned'), - 'body' => pht( - 'This revision has been abandoned. Only accepted revisions '. - 'may land.'), - ); - default: - return array( - 'title' => pht('Revision Not Accepted'), - 'body' => pht( - 'This revision is still under review. Only revisions which '. - 'have been accepted may land.'), - ); - } + if ($revision->isAccepted()) { + // We can land accepted revisions, so continue below. Otherwise, raise + // an error with tailored messaging for the most common cases. + } else if ($revision->isAbandoned()) { + return array( + 'title' => pht('Revision Abandoned'), + 'body' => pht( + 'This revision has been abandoned. Only accepted revisions '. + 'may land.'), + ); + } else if ($revision->isClosed()) { + return array( + 'title' => pht('Revision Closed'), + 'body' => pht( + 'This revision has already been closed. Only open, accepted '. + 'revisions may land.'), + ); + } else { + return array( + 'title' => pht('Revision Not Accepted'), + 'body' => pht( + 'This revision is still under review. Only revisions which '. + 'have been accepted may land.'), + ); } // Check for other operations. Eventually this should probably be more diff --git a/src/applications/releeph/differential/DifferentialReleephRequestFieldSpecification.php b/src/applications/releeph/differential/DifferentialReleephRequestFieldSpecification.php index bdb63e6c9c..dd67f5bc59 100644 --- a/src/applications/releeph/differential/DifferentialReleephRequestFieldSpecification.php +++ b/src/applications/releeph/differential/DifferentialReleephRequestFieldSpecification.php @@ -79,8 +79,7 @@ final class DifferentialReleephRequestFieldSpecification extends Phobject { return null; } - $status = $this->getRevision()->getStatus(); - if ($status == ArcanistDifferentialRevisionStatus::CLOSED) { + if ($this->getRevision()->isClosed()) { $verb = $tense[$this->releephAction]['past']; } else { $verb = $tense[$this->releephAction]['future'];