mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-26 16:52:41 +01:00
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
This commit is contained in:
epriestley
parent
6dc721009d
commit
bae8a95114
10 changed files with 84 additions and 57 deletions
|
|
@ -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');
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 '.
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
|
|
|
|||
|
|
@ -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 '.
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
Loading…
Reference in a new issue