From bae8a951143ab02d3f8d4f3c920784fc4c0f34d1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 7 Sep 2018 09:01:30 -0700 Subject: [PATCH] Continue replacing Commit/Audit status checks with object-based checks Summary: Ref T13195. See PHI851. Continuing down the path toward replacing these legacy numeric constants with more modern string constants. Test Plan: - Raised concern, requested verification, verified. - Looked at commit hovercard with audit status. - Viewed header on a commit page. - (Didn't test the Doorkeeper stuff since it requires linking to Asana and seems unlikely to break.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19647 --- .../PhabricatorAuditCommitStatusConstants.php | 24 ++++++++++++ .../audit/editor/PhabricatorAuditEditor.php | 4 +- .../controller/DiffusionCommitController.php | 13 +++---- ...sionDoorkeeperCommitFeedStoryPublisher.php | 13 +------ .../DiffusionHovercardEngineExtension.php | 10 ++--- ...fusionCommitRequiredActionResultBucket.php | 11 ++---- .../DiffusionCommitConcernTransaction.php | 4 +- .../DiffusionCommitStateTransaction.php | 39 ++++++++++--------- .../DiffusionCommitVerifyTransaction.php | 3 +- .../storage/PhabricatorRepositoryCommit.php | 20 ++++++++++ 10 files changed, 84 insertions(+), 57 deletions(-) diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index e559862c83..50e4180074 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -60,6 +60,30 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { return idx($this->spec, 'name', pht('Unknown ("%s")', $this->key)); } + public function isNoAudit() { + return ($this->key == self::MODERN_NONE); + } + + public function isConcernRaised() { + return ($this->key == self::MODERN_CONCERN_RAISED); + } + + public function isNeedsVerification() { + return ($this->key == self::MODERN_NEEDS_VERIFICATION); + } + + public function isPartiallyAudited() { + return ($this->key == self::MODERN_PARTIALLY_AUDITED); + } + + public function isAudited() { + return ($this->key == self::MODERN_AUDITED); + } + + public function getIsClosed() { + return idx($this->spec, 'closed'); + } + public static function getStatusNameMap() { $map = self::getMap(); return ipull($map, 'name', 'legacy'); diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index c04bb8185b..d5c22fba6b 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -206,12 +206,10 @@ final class PhabricatorAuditEditor $object->writeImportStatusFlag($import_status_flag); } - $partial_status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED; - // If the commit has changed state after this edit, add an informational // transaction about the state change. if ($old_status != $new_status) { - if ($new_status == $partial_status) { + if ($object->isAuditStatusPartiallyAudited()) { // This state isn't interesting enough to get a transaction. The // best way we could lead the user forward is something like "This // commit still requires additional audits." but that's redundant and diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index a6743b6557..283278fcf8 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -171,13 +171,12 @@ final class DiffusionCommitController extends DiffusionController { ->setHeaderIcon('fa-code-fork') ->addTag($commit_tag); - if ($commit->getAuditStatus()) { - $icon = PhabricatorAuditCommitStatusConstants::getStatusIcon( - $commit->getAuditStatus()); - $color = PhabricatorAuditCommitStatusConstants::getStatusColor( - $commit->getAuditStatus()); - $status = PhabricatorAuditCommitStatusConstants::getStatusName( - $commit->getAuditStatus()); + if (!$commit->isAuditStatusNoAudit()) { + $status = $commit->getAuditStatusObject(); + + $icon = $status->getIcon(); + $color = $status->getColor(); + $status = $status->getName(); $header->setStatus($icon, $color, $status); } diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php index 088f4b752e..712b7ae150 100644 --- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php +++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php @@ -31,8 +31,6 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher // After ApplicationTransactions, we could annotate feed stories more // explicitly. - $fully_audited = PhabricatorAuditCommitStatusConstants::FULLY_AUDITED; - $story = $this->getFeedStory(); $xaction = $story->getPrimaryTransaction(); switch ($xaction->getTransactionType()) { @@ -41,7 +39,7 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher case PhabricatorAuditActionConstants::CLOSE: return true; case PhabricatorAuditActionConstants::ACCEPT: - if ($object->getAuditStatus() == $fully_audited) { + if ($object->isAuditStatusAudited()) { return true; } break; @@ -165,14 +163,7 @@ final class DiffusionDoorkeeperCommitFeedStoryPublisher } public function isObjectClosed($object) { - switch ($object->getAuditStatus()) { - case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT: - case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED: - case PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED: - return false; - default: - return true; - } + return $object->getAuditStatusObject()->getIsClosed(); } public function getResponsibilityTitle($object) { diff --git a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php index 0711028796..3ce95cb3c3 100644 --- a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php +++ b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php @@ -41,12 +41,12 @@ final class DiffusionHovercardEngineExtension $hovercard->addField(pht('Date'), phabricator_date($commit->getEpoch(), $viewer)); - if ($commit->getAuditStatus() != - PhabricatorAuditCommitStatusConstants::NONE) { + if (!$commit->isAuditStatusNoAudit()) { + $status = $commit->getAuditStatusObject(); - $hovercard->addField(pht('Audit Status'), - PhabricatorAuditCommitStatusConstants::getStatusName( - $commit->getAuditStatus())); + $hovercard->addField( + pht('Audit Status'), + $status->getName()); } } diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php index 40bc63025d..70584ae243 100644 --- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php +++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php @@ -69,14 +69,12 @@ final class DiffusionCommitRequiredActionResultBucket $results = array(); $objects = $this->objects; - $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; - foreach ($objects as $key => $object) { if (empty($phids[$object->getAuthorPHID()])) { continue; } - if ($object->getAuditStatus() != $status_concern) { + if (!$object->isAuditStatusConcernRaised()) { continue; } @@ -91,7 +89,6 @@ final class DiffusionCommitRequiredActionResultBucket $results = array(); $objects = $this->objects; - $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION; $has_concern = array( PhabricatorAuditStatusConstants::CONCERNED, ); @@ -102,7 +99,7 @@ final class DiffusionCommitRequiredActionResultBucket continue; } - if ($object->getAuditStatus() != $status_verify) { + if (!$object->isAuditStatusNeedsVerification()) { continue; } @@ -147,10 +144,8 @@ final class DiffusionCommitRequiredActionResultBucket $results = array(); $objects = $this->objects; - $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; - foreach ($objects as $key => $object) { - if ($object->getAuditStatus() != $status_concern) { + if (!$object->isAuditStatusConcernRaised()) { continue; } diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php index 30ec8d9f6d..07194a2af3 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php @@ -54,10 +54,8 @@ final class DiffusionCommitConcernTransaction // Even if you've already raised a concern, you can raise again as long // as the author requested you verify. - $state_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION; - if ($this->isViewerFullyRejected($object, $viewer)) { - if ($object->getAuditStatus() != $state_verify) { + if (!$object->isAuditStatusNeedsVerification()) { throw new Exception( pht( 'You can not raise a concern with this commit because you have '. diff --git a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php index a6288923f4..bcf6a9e382 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php @@ -11,29 +11,32 @@ final class DiffusionCommitStateTransaction throw new PhutilMethodNotImplementedException(); } - public function getIcon() { + private function getAuditStatusObject() { $new = $this->getNewValue(); - return PhabricatorAuditCommitStatusConstants::getStatusIcon($new); + return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($new); + } + + public function getIcon() { + return $this->getAuditStatusObject()->getIcon(); } public function getColor() { - $new = $this->getNewValue(); - return PhabricatorAuditCommitStatusConstants::getStatusColor($new); + return $this->getAuditStatusObject()->getColor(); } public function getTitle() { - $new = $this->getNewValue(); + $status = $this->getAuditStatusObject(); - switch ($new) { - case PhabricatorAuditCommitStatusConstants::NONE: + switch ($status->getKey()) { + case PhabricatorAuditCommitStatusConstants::MODERN_NONE: return pht('This commit no longer requires audit.'); - case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT: + case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT: return pht('This commit now requires audit.'); - case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED: + case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED: return pht('This commit now has outstanding concerns.'); - case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION: + case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION: return pht('This commit now requires verification by auditors.'); - case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED: + case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED: return pht('All concerns with this commit have now been addressed.'); } @@ -41,26 +44,26 @@ final class DiffusionCommitStateTransaction } public function getTitleForFeed() { - $new = $this->getNewValue(); + $status = $this->getAuditStatusObject(); - switch ($new) { - case PhabricatorAuditCommitStatusConstants::NONE: + switch ($status->getKey()) { + case PhabricatorAuditCommitStatusConstants::MODERN_NONE: return pht( '%s no longer requires audit.', $this->renderObject()); - case PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT: + case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_AUDIT: return pht( '%s now requires audit.', $this->renderObject()); - case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED: + case PhabricatorAuditCommitStatusConstants::MODERN_CONCERN_RAISED: return pht( '%s now has outstanding concerns.', $this->renderObject()); - case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION: + case PhabricatorAuditCommitStatusConstants::MODERN_NEEDS_VERIFICATION: return pht( '%s now requires verification by auditors.', $this->renderObject()); - case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED: + case PhabricatorAuditCommitStatusConstants::MODERN_AUDITED: return pht( 'All concerns with %s have now been addressed.', $this->renderObject()); diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php index a44261f7a4..3e6abb705f 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php @@ -48,8 +48,7 @@ final class DiffusionCommitVerifyTransaction 'are not the author.')); } - $status = $object->getAuditStatus(); - if ($status != PhabricatorAuditCommitStatusConstants::CONCERN_RAISED) { + if (!$object->isAuditStatusConcernRaised()) { throw new Exception( pht( 'You can not request verification of this commit because no '. diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 447db08e6c..f56962d9f8 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -535,6 +535,26 @@ final class PhabricatorRepositoryCommit return PhabricatorAuditCommitStatusConstants::newForLegacyStatus($status); } + public function isAuditStatusNoAudit() { + return $this->getAuditStatusObject()->isNoAudit(); + } + + public function isAuditStatusConcernRaised() { + return $this->getAuditStatusObject()->isConcernRaised(); + } + + public function isAuditStatusNeedsVerification() { + return $this->getAuditStatusObject()->isNeedsVerification(); + } + + public function isAuditStatusPartiallyAudited() { + return $this->getAuditStatusObject()->isPartiallyAudited(); + } + + public function isAuditStatusAudited() { + return $this->getAuditStatusObject()->isAudited(); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ public function getCapabilities() {