@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator

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

+49 -69
+1 -3
src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php
··· 88 88 'uri' => PhabricatorEnv::getProductionURI('/D'.$id), 89 89 'dateCreated' => $revision->getDateCreated(), 90 90 'authorPHID' => $revision->getAuthorPHID(), 91 - 'statusName' => 92 - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( 93 - $revision->getStatus()), 91 + 'statusName' => $revision->getStatusDisplayName(), 94 92 'sourcePath' => $diff->getSourcePath(), 95 93 ); 96 94 }
+1 -3
src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php
··· 83 83 'uri' => PhabricatorEnv::getURI('/D'.$revision->getID()), 84 84 'title' => $revision->getTitle(), 85 85 'status' => $revision->getStatus(), 86 - 'statusName' => 87 - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( 88 - $revision->getStatus()), 86 + 'statusName' => $revision->getStatusDisplayName(), 89 87 'summary' => $revision->getSummary(), 90 88 'testPlan' => $revision->getTestPlan(), 91 89 'lineCount' => $revision->getLineCount(),
+1 -3
src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php
··· 221 221 'dateModified' => $revision->getDateModified(), 222 222 'authorPHID' => $revision->getAuthorPHID(), 223 223 'status' => $revision->getStatus(), 224 - 'statusName' => 225 - ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( 226 - $revision->getStatus()), 224 + 'statusName' => $revision->getStatusDisplayName(), 227 225 'properties' => $revision->getProperties(), 228 226 'branch' => $diff->getBranch(), 229 227 'summary' => $revision->getSummary(),
+5 -4
src/applications/differential/customfield/DifferentialBranchField.php
··· 76 76 PhabricatorApplicationTransactionEditor $editor, 77 77 array $xactions) { 78 78 79 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 79 + $revision = $this->getObject(); 80 80 81 81 // Show the "BRANCH" section only if there's a new diff or the revision 82 82 // is "Accepted". 83 - if ((!$editor->getDiffUpdateTransaction($xactions)) && 84 - ($this->getObject()->getStatus() != $status_accepted)) { 83 + $is_update = (bool)$editor->getDiffUpdateTransaction($xactions); 84 + $is_accepted = $revision->isAccepted(); 85 + if (!$is_update && !$is_accepted) { 85 86 return; 86 87 } 87 88 88 - $branch = $this->getBranchDescription($this->getObject()->getActiveDiff()); 89 + $branch = $this->getBranchDescription($revision->getActiveDiff()); 89 90 if ($branch === null) { 90 91 return; 91 92 }
+1 -2
src/applications/differential/customfield/DifferentialReviewersField.php
··· 68 68 public function getWarningsForRevisionHeader(array $handles) { 69 69 $revision = $this->getObject(); 70 70 71 - $status_needs_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 72 - if ($revision->getStatus() != $status_needs_review) { 71 + if (!$revision->isNeedsReview()) { 73 72 return array(); 74 73 } 75 74
+2 -4
src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php
··· 35 35 } 36 36 37 37 public function getActiveUserPHIDs($object) { 38 - $status = $object->getStatus(); 39 - if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { 38 + if ($object->isNeedsReview()) { 40 39 return $object->getReviewerPHIDs(); 41 40 } else { 42 41 return array(); ··· 44 43 } 45 44 46 45 public function getPassiveUserPHIDs($object) { 47 - $status = $object->getStatus(); 48 - if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { 46 + if ($object->isNeedsReview()) { 49 47 return array(); 50 48 } else { 51 49 return $object->getReviewerPHIDs();
+1 -2
src/applications/differential/phid/DifferentialRevisionPHIDType.php
··· 50 50 51 51 $icon = DifferentialRevisionStatus::getRevisionStatusIcon($status); 52 52 $color = DifferentialRevisionStatus::getRevisionStatusColor($status); 53 - $name = ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( 54 - $status); 53 + $name = $revision->getStatusDisplayName(); 55 54 56 55 $handle 57 56 ->setStateIcon($icon)
+3 -12
src/applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php
··· 134 134 } 135 135 136 136 private function filterShouldLand(array $phids) { 137 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 138 - 139 137 $objects = $this->getRevisionsAuthored($this->objects, $phids); 140 138 141 139 $results = array(); 142 140 foreach ($objects as $key => $object) { 143 - if ($object->getStatus() != $status_accepted) { 141 + if (!$object->isAccepted()) { 144 142 continue; 145 143 } 146 144 ··· 175 173 } 176 174 177 175 private function filterWaitingForReview(array $phids) { 178 - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 179 - 180 176 $objects = $this->getRevisionsAuthored($this->objects, $phids); 181 177 182 178 $results = array(); 183 179 foreach ($objects as $key => $object) { 184 - if ($object->getStatus() != $status_review) { 180 + if (!$object->isNeedsReview()) { 185 181 continue; 186 182 } 187 183 ··· 217 213 } 218 214 219 215 private function filterWaitingOnOtherReviewers(array $phids) { 220 - $statuses = array( 221 - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW, 222 - ); 223 - $statuses = array_fuse($statuses); 224 - 225 216 $objects = $this->getRevisionsNotAuthored($this->objects, $phids); 226 217 227 218 $results = array(); 228 219 foreach ($objects as $key => $object) { 229 - if (!isset($statuses[$object->getStatus()])) { 220 + if (!$object->isNeedsReview()) { 230 221 continue; 231 222 } 232 223
+1 -2
src/applications/differential/query/DifferentialRevisionResultBucket.php
··· 15 15 16 16 $objects = $this->getRevisionsNotAuthored($objects, $phids); 17 17 18 - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 19 18 foreach ($objects as $key => $object) { 20 - if ($object->getStatus() != $status_review) { 19 + if (!$object->isNeedsReview()) { 21 20 continue; 22 21 } 23 22
+1 -3
src/applications/differential/query/DifferentialRevisionSearchEngine.php
··· 235 235 } 236 236 237 237 private function loadUnlandedDependencies(array $revisions) { 238 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 239 - 240 238 $phids = array(); 241 239 foreach ($revisions as $revision) { 242 - if ($revision->getStatus() != $status_accepted) { 240 + if (!$revision->isAccepted()) { 243 241 continue; 244 242 } 245 243
+1 -2
src/applications/differential/search/DifferentialRevisionFulltextEngine.php
··· 34 34 35 35 // If a revision needs review, the owners are the reviewers. Otherwise, the 36 36 // owner is the author (e.g., accepted, rejected, closed). 37 - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 38 - if ($revision->getStatus() == $status_review) { 37 + if ($revision->isNeedsReview()) { 39 38 $reviewers = $revision->getReviewerPHIDs(); 40 39 $reviewers = array_fuse($reviewers); 41 40
+5
src/applications/differential/storage/DifferentialRevision.php
··· 626 626 return ($this->getStatus() == $status_accepted); 627 627 } 628 628 629 + public function isNeedsReview() { 630 + $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; 631 + return ($this->getStatus() == $status_review); 632 + } 633 + 629 634 public function getStatusIcon() { 630 635 $map = array( 631 636 ArcanistDifferentialRevisionStatus::NEEDS_REVIEW
+1 -2
src/applications/diffusion/query/lowlevel/DiffusionLowLevelCommitFieldsQuery.php
··· 122 122 $revisions = array_reverse($revisions); 123 123 124 124 // Try to find an accepted revision first. 125 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 126 125 foreach ($revisions as $revision) { 127 - if ($revision->getStatus() == $status_accepted) { 126 + if ($revision->isAccepted()) { 128 127 return $revision; 129 128 } 130 129 }
+24 -25
src/applications/drydock/operation/DrydockLandRepositoryOperation.php
··· 289 289 ); 290 290 } 291 291 292 - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; 293 - if ($revision->getStatus() != $status_accepted) { 294 - switch ($revision->getStatus()) { 295 - case ArcanistDifferentialRevisionStatus::CLOSED: 296 - return array( 297 - 'title' => pht('Revision Closed'), 298 - 'body' => pht( 299 - 'This revision has already been closed. Only open, accepted '. 300 - 'revisions may land.'), 301 - ); 302 - case ArcanistDifferentialRevisionStatus::ABANDONED: 303 - return array( 304 - 'title' => pht('Revision Abandoned'), 305 - 'body' => pht( 306 - 'This revision has been abandoned. Only accepted revisions '. 307 - 'may land.'), 308 - ); 309 - default: 310 - return array( 311 - 'title' => pht('Revision Not Accepted'), 312 - 'body' => pht( 313 - 'This revision is still under review. Only revisions which '. 314 - 'have been accepted may land.'), 315 - ); 316 - } 292 + if ($revision->isAccepted()) { 293 + // We can land accepted revisions, so continue below. Otherwise, raise 294 + // an error with tailored messaging for the most common cases. 295 + } else if ($revision->isAbandoned()) { 296 + return array( 297 + 'title' => pht('Revision Abandoned'), 298 + 'body' => pht( 299 + 'This revision has been abandoned. Only accepted revisions '. 300 + 'may land.'), 301 + ); 302 + } else if ($revision->isClosed()) { 303 + return array( 304 + 'title' => pht('Revision Closed'), 305 + 'body' => pht( 306 + 'This revision has already been closed. Only open, accepted '. 307 + 'revisions may land.'), 308 + ); 309 + } else { 310 + return array( 311 + 'title' => pht('Revision Not Accepted'), 312 + 'body' => pht( 313 + 'This revision is still under review. Only revisions which '. 314 + 'have been accepted may land.'), 315 + ); 317 316 } 318 317 319 318 // Check for other operations. Eventually this should probably be more
+1 -2
src/applications/releeph/differential/DifferentialReleephRequestFieldSpecification.php
··· 79 79 return null; 80 80 } 81 81 82 - $status = $this->getRevision()->getStatus(); 83 - if ($status == ArcanistDifferentialRevisionStatus::CLOSED) { 82 + if ($this->getRevision()->isClosed()) { 84 83 $verb = $tense[$this->releephAction]['past']; 85 84 } else { 86 85 $verb = $tense[$this->releephAction]['future'];