D17252.diff
No OneTemporary
Actions

Size

15 KB

Referenced Files

None

Subscribers

None

D17252.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

Mime Type: text/plain
Expires: Thu, May 16, 1:58 AM (3 w, 5 d ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 6290186
Default Alt Text: D17252.diff (15 KB)

D17252.diffNo OneTemporaryActions

D17252.diffView Options

File Metadata

Event Timeline

D17252.diff
No OneTemporary
Actions

D17252.diff
View Options