revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 14:00:56 +01:00
Code Issues Releases Wiki Activity

Allow users to re-accept or re-reject a revision if they have authority over package/project reviewers not yet in the target state

Browse source 
Summary:
To set this up:

  - alice accepts a revision.
  - Something adds a package or project she has authority over as a reviewer.
  - Because alice has already accepted, she can not re-accept, but she should be able to (in order to accept on behalf of the new project or package).

Test Plan:
  - Created a revision.
  - Accepted as user "dog".
  - Added "dog project".
  - Re-accepted.
  - Could not three-accept.
  - Removed "dog project.
  - Rejected.
  - Added "dog project".
  - Re-rejected.
  - Could not three-reject.

Reviewers: chad, eadler

Reviewed By: chad, eadler

Differential Revision: https://secure.phabricator.com/D17226
This commit is contained in:
epriestley 2017-01-18 12:56:49 -08:00
parent b8e04fe041
commit 269dd81f91
3 changed files with 26 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
View file

@ -50,7 +50,7 @@ final class DifferentialRevisionAcceptTransaction
public function generateOldValue($object) { public function generateOldValue($object) {
$actor = $this->getActor(); $actor = $this->getActor();
return $this->isViewerAcceptingReviewer($object, $actor); return $this->isViewerFullyAccepted($object, $actor);
} }
public function applyExternalEffects($object, $value) { public function applyExternalEffects($object, $value) {
@ -79,7 +79,7 @@ final class DifferentialRevisionAcceptTransaction
} }
} }
if ($this->isViewerAcceptingReviewer($object, $viewer)) { if ($this->isViewerFullyAccepted($object, $viewer)) {
throw new Exception( throw new Exception(
pht( pht(
'You can not accept this revision because you have already '. 'You can not accept this revision because you have already '.

4
src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php
View file

@ -46,7 +46,7 @@ final class DifferentialRevisionRejectTransaction
public function generateOldValue($object) { public function generateOldValue($object) {
$actor = $this->getActor(); $actor = $this->getActor();
return $this->isViewerRejectingReviewer($object, $actor); return $this->isViewerFullyRejected($object, $actor);
} }
public function applyExternalEffects($object, $value) { public function applyExternalEffects($object, $value) {
@ -72,7 +72,7 @@ final class DifferentialRevisionRejectTransaction
'not own.')); 'not own.'));
} }
if ($this->isViewerRejectingReviewer($object, $viewer)) { if ($this->isViewerFullyRejected($object, $viewer)) {
throw new Exception( throw new Exception(
pht( pht(
'You can not request changes to this revision because you have '. 'You can not request changes to this revision because you have '.

28
src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
View file

@ -13,10 +13,10 @@ abstract class DifferentialRevisionReviewTransaction
return ($this->getViewerReviewerStatus($revision, $viewer) !== null); return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
} }
protected function isViewerAcceptingReviewer( protected function isViewerFullyAccepted(
DifferentialRevision $revision, DifferentialRevision $revision,
PhabricatorUser $viewer) { PhabricatorUser $viewer) {
return $this->isViewerReviewerStatusAmong( return $this->isViewerReviewerStatusFullyAmong(
$revision, $revision,
$viewer, $viewer,
array( array(
@ -24,10 +24,10 @@ abstract class DifferentialRevisionReviewTransaction
)); ));
} }
protected function isViewerRejectingReviewer( protected function isViewerFullyRejected(
DifferentialRevision $revision, DifferentialRevision $revision,
PhabricatorUser $viewer) { PhabricatorUser $viewer) {
return $this->isViewerReviewerStatusAmong( return $this->isViewerReviewerStatusFullyAmong(
$revision, $revision,
$viewer, $viewer,
array( array(
@ -54,18 +54,34 @@ abstract class DifferentialRevisionReviewTransaction
return null; return null;
} }
protected function isViewerReviewerStatusAmong( protected function isViewerReviewerStatusFullyAmong(
DifferentialRevision $revision, DifferentialRevision $revision,
PhabricatorUser $viewer, PhabricatorUser $viewer,
array $status_list) { array $status_list) {
// If the user themselves is not a reviewer, the reviews they have
// authority over can not all be in any set of states since their own
// personal review has no state.
$status = $this->getViewerReviewerStatus($revision, $viewer); $status = $this->getViewerReviewerStatus($revision, $viewer);
if ($status === null) { if ($status === null) {
return false; return false;
} }
// Otherwise, check that all reviews they have authority over are in
// the desired set of states.
$status_map = array_fuse($status_list); $status_map = array_fuse($status_list);
return isset($status_map[$status]); foreach ($revision->getReviewerStatus() as $reviewer) {
if (!$reviewer->hasAuthority($viewer)) {
continue;
}
$status = $reviewer->getStatus();
if (!isset($status_map[$status])) {
return false;
}
}
return true;
} }
protected function applyReviewerEffect( protected function applyReviewerEffect(