Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F15428973
D17252.id.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
15 KB
Referenced Files
None
Subscribers
None
D17252.id.diff
View Options
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -674,6 +674,7 @@
'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 @@
'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
--- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
+++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php
@@ -7,12 +7,14 @@
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 @@
return array(
self::CONCERN_RAISED,
self::NEEDS_AUDIT,
+ self::NEEDS_VERIFICATION,
self::PARTIALLY_AUDITED,
);
}
@@ -49,6 +52,9 @@
case self::NONE:
$color = 'bluegrey';
break;
+ case self::NEEDS_VERIFICATION:
+ $color = 'indigo';
+ break;
default:
$color = null;
break;
@@ -56,7 +62,7 @@
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 @@
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
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -11,6 +11,7 @@
private $auditorPHIDs = array();
private $didExpandInlineState = false;
+ private $oldAuditStatus = null;
public function addAuditReason($phid, $reason) {
if (!isset($this->auditReasonMap[$phid])) {
@@ -78,6 +79,8 @@
}
}
+ $this->oldAuditStatus = $object->getAuditStatus();
+
$object->loadAndAttachAuditAuthority(
$this->getActor(),
$this->getActingAsPHID());
@@ -269,7 +272,7 @@
}
}
- $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
--- a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
+++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php
@@ -34,6 +34,11 @@
->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.'))
->setObjects($this->filterShouldAudit($phids));
@@ -82,6 +87,36 @@
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 @@
$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
--- a/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php
@@ -30,11 +30,6 @@
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 @@
}
}
- 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
--- a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php
@@ -7,6 +7,10 @@
return DiffusionCommitEditEngine::ACTIONGROUP_AUDIT;
}
+ public function generateOldValue($object) {
+ return false;
+ }
+
protected function isViewerAnyAuditor(
PhabricatorRepositoryCommit $commit,
PhabricatorUser $viewer) {
@@ -18,22 +22,23 @@
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 @@
));
}
- protected function isViewerRejectingAuditor(
+ protected function isViewerFullyRejected(
PhabricatorRepositoryCommit $commit,
PhabricatorUser $viewer) {
- return $this->isViewerAuditStatusAmong(
+ return $this->isViewerAuditStatusFullyAmong(
$commit,
$viewer,
array(
@@ -71,7 +76,7 @@
return null;
}
- protected function isViewerAuditStatusAmong(
+ protected function isViewerAuditStatusFullyAmong(
PhabricatorRepositoryCommit $commit,
PhabricatorUser $viewer,
array $status_list) {
@@ -82,7 +87,20 @@
}
$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
--- a/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php
@@ -30,9 +30,11 @@
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 @@
'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
--- a/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php
@@ -31,6 +31,8 @@
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 @@
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
--- /dev/null
+++ b/src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php
@@ -0,0 +1,73 @@
+<?php
+
+final class DiffusionCommitVerifyTransaction
+ extends DiffusionCommitAuditTransaction {
+
+ const TRANSACTIONTYPE = 'diffusion.commit.verify';
+ const ACTIONKEY = 'verify';
+
+ protected function getCommitActionLabel() {
+ return pht('Request Verification');
+ }
+
+ protected function getCommitActionDescription() {
+ return pht(
+ 'Auditors will be asked to verify that concerns have been addressed.');
+ }
+
+ protected function getCommitActionGroupKey() {
+ return DiffusionCommitEditEngine::ACTIONGROUP_COMMIT;
+ }
+
+ public function getIcon() {
+ return 'fa-refresh';
+ }
+
+ public function getColor() {
+ return 'indigo';
+ }
+
+ protected function getCommitActionOrder() {
+ return 600;
+ }
+
+ public function getActionName() {
+ return pht('Requested Verification');
+ }
+
+ public function applyInternalEffects($object, $value) {
+ $object->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
--- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php
@@ -327,8 +327,17 @@
}
}
+ $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;
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Mon, Mar 24, 11:49 PM (6 d, 23 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7564887
Default Alt Text
D17252.id.diff (15 KB)
Attached To
Mode
D17252: Add a "Needs Verification" state to Audit
Attached
Detach File
Event Timeline
Log In to Comment