From 97cac83e9b0ceaa31101529f59f92624c347c59c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Jan 2017 11:05:49 -0800 Subject: [PATCH] Add a "Needs Verification" state to Audit Summary: Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you". Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added. Test Plan: - Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern". - Verified it showed up properly in bucketing for both users. - Accepted, added a project, accepted again (works now; didn't before). - Audited on behalf of projects / packages. Reviewers: chad Reviewed By: chad Maniphest Tasks: T2393 Differential Revision: https://secure.phabricator.com/D17252 --- src/__phutil_library_map__.php | 2 + .../PhabricatorAuditCommitStatusConstants.php | 11 ++- .../audit/editor/PhabricatorAuditEditor.php | 5 +- ...fusionCommitRequiredActionResultBucket.php | 36 +++++++++ .../DiffusionCommitAcceptTransaction.php | 7 +- .../DiffusionCommitAuditTransaction.php | 48 ++++++++---- .../DiffusionCommitConcernTransaction.php | 24 ++++-- .../DiffusionCommitStateTransaction.php | 6 ++ .../DiffusionCommitVerifyTransaction.php | 73 +++++++++++++++++++ .../storage/PhabricatorRepositoryCommit.php | 11 ++- 10 files changed, 191 insertions(+), 32 deletions(-) create mode 100644 src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 28cf7a83c5..a27cbdd3fc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -674,6 +674,7 @@ phutil_register_library_map(array( 'DiffusionCommitStateTransaction' => 'applications/diffusion/xaction/DiffusionCommitStateTransaction.php', 'DiffusionCommitTagsController' => 'applications/diffusion/controller/DiffusionCommitTagsController.php', 'DiffusionCommitTransactionType' => 'applications/diffusion/xaction/DiffusionCommitTransactionType.php', + 'DiffusionCommitVerifyTransaction' => 'applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php', 'DiffusionCompareController' => 'applications/diffusion/controller/DiffusionCompareController.php', 'DiffusionConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionConduitAPIMethod.php', 'DiffusionController' => 'applications/diffusion/controller/DiffusionController.php', @@ -5382,6 +5383,7 @@ phutil_register_library_map(array( 'DiffusionCommitStateTransaction' => 'DiffusionCommitTransactionType', 'DiffusionCommitTagsController' => 'DiffusionController', 'DiffusionCommitTransactionType' => 'PhabricatorModularTransactionType', + 'DiffusionCommitVerifyTransaction' => 'DiffusionCommitAuditTransaction', 'DiffusionCompareController' => 'DiffusionController', 'DiffusionConduitAPIMethod' => 'ConduitAPIMethod', 'DiffusionController' => 'PhabricatorController', diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index bced1b2dc6..31bb2c22f7 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -7,12 +7,14 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { const CONCERN_RAISED = 2; const PARTIALLY_AUDITED = 3; const FULLY_AUDITED = 4; + const NEEDS_VERIFICATION = 5; public static function getStatusNameMap() { $map = array( self::NONE => pht('No Audits'), self::NEEDS_AUDIT => pht('Audit Required'), self::CONCERN_RAISED => pht('Concern Raised'), + self::NEEDS_VERIFICATION => pht('Needs Verification'), self::PARTIALLY_AUDITED => pht('Partially Audited'), self::FULLY_AUDITED => pht('Audited'), ); @@ -28,6 +30,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { return array( self::CONCERN_RAISED, self::NEEDS_AUDIT, + self::NEEDS_VERIFICATION, self::PARTIALLY_AUDITED, ); } @@ -49,6 +52,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { case self::NONE: $color = 'bluegrey'; break; + case self::NEEDS_VERIFICATION: + $color = 'indigo'; + break; default: $color = null; break; @@ -56,7 +62,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { return $color; } - public static function getStatusIcon($code) { + public static function getStatusIcon($code) { switch ($code) { case self::CONCERN_RAISED: $icon = 'fa-times-circle'; @@ -73,6 +79,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { case self::NONE: $icon = 'fa-check'; break; + case self::NEEDS_VERIFICATION: + $icon = 'fa-refresh'; + break; default: $icon = null; break; diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 2e7efd4c87..fe904913ae 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -11,6 +11,7 @@ final class PhabricatorAuditEditor private $auditorPHIDs = array(); private $didExpandInlineState = false; + private $oldAuditStatus = null; public function addAuditReason($phid, $reason) { if (!isset($this->auditReasonMap[$phid])) { @@ -78,6 +79,8 @@ final class PhabricatorAuditEditor } } + $this->oldAuditStatus = $object->getAuditStatus(); + $object->loadAndAttachAuditAuthority( $this->getActor(), $this->getActingAsPHID()); @@ -269,7 +272,7 @@ final class PhabricatorAuditEditor } } - $old_status = $object->getAuditStatus(); + $old_status = $this->oldAuditStatus; $requests = $object->getAudits(); $object->updateAuditStatus($requests); diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php index ad80cddf39..405e1ecd27 100644 --- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php +++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php @@ -33,6 +33,11 @@ final class DiffusionCommitRequiredActionResultBucket ->setNoDataString(pht('None of your commits have active concerns.')) ->setObjects($this->filterConcernRaised($phids)); + $groups[] = $this->newGroup() + ->setName(pht('Needs Verification')) + ->setNoDataString(pht('No commits are awaiting your verification.')) + ->setObjects($this->filterNeedsVerification($phids)); + $groups[] = $this->newGroup() ->setName(pht('Ready to Audit')) ->setNoDataString(pht('No commits are waiting for you to audit them.')) @@ -82,6 +87,36 @@ final class DiffusionCommitRequiredActionResultBucket return $results; } + private function filterNeedsVerification(array $phids) { + $results = array(); + $objects = $this->objects; + + $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION; + $has_concern = array( + PhabricatorAuditStatusConstants::CONCERNED, + ); + $has_concern = array_fuse($has_concern); + + foreach ($objects as $key => $object) { + if (isset($phids[$object->getAuthorPHID()])) { + continue; + } + + if ($object->getAuditStatus() != $status_verify) { + continue; + } + + if (!$this->hasAuditorsWithStatus($object, $phids, $has_concern)) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + private function filterShouldAudit(array $phids) { $results = array(); $objects = $this->objects; @@ -132,6 +167,7 @@ final class DiffusionCommitRequiredActionResultBucket $status_waiting = array( PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT, + PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION, PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED, ); $status_waiting = array_fuse($status_waiting); diff --git a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php index 76c2de609e..2e0dab2c35 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php @@ -30,11 +30,6 @@ final class DiffusionCommitAcceptTransaction return pht('Accepted'); } - public function generateOldValue($object) { - $actor = $this->getActor(); - return $this->isViewerAcceptingAuditor($object, $actor); - } - public function applyExternalEffects($object, $value) { $status = PhabricatorAuditStatusConstants::ACCEPTED; $actor = $this->getActor(); @@ -54,7 +49,7 @@ final class DiffusionCommitAcceptTransaction } } - if ($this->isViewerAcceptingAuditor($object, $viewer)) { + if ($this->isViewerFullyAccepted($object, $viewer)) { throw new Exception( pht( 'You can not accept this commit because you have already '. diff --git a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php index 99d2ebb1d2..c80301e6ee 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php @@ -7,6 +7,10 @@ abstract class DiffusionCommitAuditTransaction return DiffusionCommitEditEngine::ACTIONGROUP_AUDIT; } + public function generateOldValue($object) { + return false; + } + protected function isViewerAnyAuditor( PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer) { @@ -18,22 +22,23 @@ abstract class DiffusionCommitAuditTransaction PhabricatorUser $viewer) { // This omits various inactive states like "Resigned" and "Not Required". + $active = array( + PhabricatorAuditStatusConstants::AUDIT_REQUIRED, + PhabricatorAuditStatusConstants::CONCERNED, + PhabricatorAuditStatusConstants::ACCEPTED, + PhabricatorAuditStatusConstants::AUDIT_REQUESTED, + ); + $active = array_fuse($active); - return $this->isViewerAuditStatusAmong( - $commit, - $viewer, - array( - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - PhabricatorAuditStatusConstants::CONCERNED, - PhabricatorAuditStatusConstants::ACCEPTED, - PhabricatorAuditStatusConstants::AUDIT_REQUESTED, - )); + $viewer_status = $this->getViewerAuditStatus($commit, $viewer); + + return isset($active[$viewer_status]); } - protected function isViewerAcceptingAuditor( + protected function isViewerFullyAccepted( PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer) { - return $this->isViewerAuditStatusAmong( + return $this->isViewerAuditStatusFullyAmong( $commit, $viewer, array( @@ -41,10 +46,10 @@ abstract class DiffusionCommitAuditTransaction )); } - protected function isViewerRejectingAuditor( + protected function isViewerFullyRejected( PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer) { - return $this->isViewerAuditStatusAmong( + return $this->isViewerAuditStatusFullyAmong( $commit, $viewer, array( @@ -71,7 +76,7 @@ abstract class DiffusionCommitAuditTransaction return null; } - protected function isViewerAuditStatusAmong( + protected function isViewerAuditStatusFullyAmong( PhabricatorRepositoryCommit $commit, PhabricatorUser $viewer, array $status_list) { @@ -82,7 +87,20 @@ abstract class DiffusionCommitAuditTransaction } $status_map = array_fuse($status_list); - return isset($status_map[$status]); + foreach ($commit->getAudits() as $audit) { + if (!$commit->hasAuditAuthority($viewer, $audit)) { + continue; + } + + $status = $audit->getAuditStatus(); + if (isset($status_map[$status])) { + continue; + } + + return false; + } + + return true; } protected function applyAuditorEffect( diff --git a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php index 6bbc27818f..ff1cc9f3c5 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php @@ -30,9 +30,11 @@ final class DiffusionCommitConcernTransaction return pht('Raised Concern'); } - public function generateOldValue($object) { - $actor = $this->getActor(); - return $this->isViewerRejectingAuditor($object, $actor); + public function applyInternalEffects($object, $value) { + // NOTE: We force the commit directly into "Concern Raised" so that we + // override a possible "Needs Verification" state. + $object->setAuditStatus( + PhabricatorAuditCommitStatusConstants::CONCERN_RAISED); } public function applyExternalEffects($object, $value) { @@ -50,11 +52,17 @@ final class DiffusionCommitConcernTransaction 'you did not author.')); } - if ($this->isViewerRejectingAuditor($object, $viewer)) { - throw new Exception( - pht( - 'You can not raise a concern with this commit because you have '. - 'already raised a concern with it.')); + // Even if you've already raised a concern, you can raise again as long + // as the author requsted you verify. + $state_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION; + + if ($this->isViewerFullyRejected($object, $viewer)) { + if ($object->getAuditStatus() != $state_verify) { + throw new Exception( + pht( + 'You can not raise a concern with this commit because you have '. + 'already raised a concern with it.')); + } } } diff --git a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php index 3a6440c364..a6288923f4 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php @@ -31,6 +31,8 @@ final class DiffusionCommitStateTransaction return pht('This commit now requires audit.'); case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED: return pht('This commit now has outstanding concerns.'); + case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION: + return pht('This commit now requires verification by auditors.'); case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED: return pht('All concerns with this commit have now been addressed.'); } @@ -54,6 +56,10 @@ final class DiffusionCommitStateTransaction return pht( '%s now has outstanding concerns.', $this->renderObject()); + case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION: + return pht( + '%s now requires verification by auditors.', + $this->renderObject()); case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED: return pht( 'All concerns with %s have now been addressed.', diff --git a/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php new file mode 100644 index 0000000000..f9b3fc5161 --- /dev/null +++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php @@ -0,0 +1,73 @@ +setAuditStatus( + PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION); + } + + protected function validateAction($object, PhabricatorUser $viewer) { + if (!$this->isViewerCommitAuthor($object, $viewer)) { + throw new Exception( + pht( + 'You can not request verification of this commit because you '. + 'are not the author.')); + } + + $status = $object->getAuditStatus(); + if ($status != PhabricatorAuditCommitStatusConstants::CONCERN_RAISED) { + throw new Exception( + pht( + 'You can not request verification of this commit because no '. + 'auditors have raised conerns with it.')); + } + } + + public function getTitle() { + return pht( + '%s requested verification of this commit.', + $this->renderAuthor()); + } + + public function getTitleForFeed() { + return pht( + '%s requested verification of %s.', + $this->renderAuthor(), + $this->renderObject()); + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 3536cfbd0d..5d826f9118 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -327,8 +327,17 @@ final class PhabricatorRepositoryCommit } } + $current_status = $this->getAuditStatus(); + $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION; + if ($any_concern) { - $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; + if ($current_status == $status_verify) { + // If the change is in "Needs Verification", we keep it there as + // long as any auditors still have concerns. + $status = $status_verify; + } else { + $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; + } } else if ($any_accept) { if ($any_need) { $status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED;