1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-26 08:42:41 +01:00

Allow users to resign if they have authority over any reviewer

Summary:
Ref T11050. The old rule was "you can only resign if you're a reviewer".

With the new behavior of "resign", the rule should be "you can resign if you're a reviewer, or you have authority over any reviewer". Make it so.

Also fixes T12446. I don't know how to reproduce that but I'm pretty sure this'll fix it?

Test Plan:
  - Could not resign from a revision with no authority/reviewer.
  - Resigned from a revision with myself as a reviewer.
  - Resigned from a revision with a package I owned as a reviewer.
  - Could not resign from a revision I had already resigned from.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12446, T11050

Differential Revision: https://secure.phabricator.com/D17558
This commit is contained in:
epriestley 2017-03-24 13:02:10 -07:00
parent daeb94561f
commit 24b6c7d718
3 changed files with 30 additions and 4 deletions

View file

@ -44,7 +44,9 @@ final class DifferentialRevisionResignTransaction
public function generateOldValue($object) { public function generateOldValue($object) {
$actor = $this->getActor(); $actor = $this->getActor();
return !$this->isViewerAnyReviewer($object, $actor); $resigned = DifferentialReviewerStatus::STATUS_RESIGNED;
return ($this->getViewerReviewerStatus($object, $actor) == $resigned);
} }
public function applyExternalEffects($object, $value) { public function applyExternalEffects($object, $value) {
@ -61,12 +63,19 @@ final class DifferentialRevisionResignTransaction
'been closed. You can only resign from open revisions.')); 'been closed. You can only resign from open revisions.'));
} }
if (!$this->isViewerAnyReviewer($object, $viewer)) { $resigned = DifferentialReviewerStatus::STATUS_RESIGNED;
if ($this->getViewerReviewerStatus($object, $viewer) == $resigned) {
throw new Exception(
pht(
'You can not resign from this revision because you have already '.
'resigned.'));
}
if (!$this->isViewerAnyAuthority($object, $viewer)) {
throw new Exception( throw new Exception(
pht( pht(
'You can not resign from this revision because you are not a '. 'You can not resign from this revision because you are not a '.
'reviewer. You can only resign from revisions where you are a '. 'reviewer, and do not have authority over any reviewer.'));
'reviewer.'));
} }
} }

View file

@ -33,6 +33,20 @@ abstract class DifferentialRevisionReviewTransaction
return ($this->getViewerReviewerStatus($revision, $viewer) !== null); return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
} }
protected function isViewerAnyAuthority(
DifferentialRevision $revision,
PhabricatorUser $viewer) {
$reviewers = $revision->getReviewers();
foreach ($revision->getReviewers() as $reviewer) {
if ($reviewer->hasAuthority($viewer)) {
return true;
}
}
return false;
}
protected function isViewerFullyAccepted( protected function isViewerFullyAccepted(
DifferentialRevision $revision, DifferentialRevision $revision,
PhabricatorUser $viewer) { PhabricatorUser $viewer) {

View file

@ -60,6 +60,9 @@ abstract class DifferentialRevisionTransactionType
protected function getActiveDiffPHID(DifferentialRevision $revision) { protected function getActiveDiffPHID(DifferentialRevision $revision) {
try { try {
$diff = $revision->getActiveDiff(); $diff = $revision->getActiveDiff();
if (!$diff) {
return null;
}
return $diff->getPHID(); return $diff->getPHID();
} catch (Exception $ex) { } catch (Exception $ex) {
return null; return null;