1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00

Restore "Accept", "Reject" and "Resign" actions to Differential on EditEngine

Summary:
Ref T11114. Some rough edges, but this largely makes Accept, Reject and Resign work in the new EditEngine comment area.

Ref T11050. This lays a little bit of groundwork for having "resign" mean "I don't want to review this, even if projects or packages I'm a member of need to", not just "remove me personally as a user reviewer".

Test Plan: Accepted, rejected and resigned from revisions without any major state issues.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11114, T11050

Differential Revision: https://secure.phabricator.com/D17113
This commit is contained in:
epriestley 2016-12-29 10:35:47 -08:00
parent 8b74cd481a
commit 5a6643f36f
7 changed files with 337 additions and 1 deletions

View file

@ -506,6 +506,7 @@ phutil_register_library_map(array(
'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php',
'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
'DifferentialRevisionAbandonTransaction' => 'applications/differential/xaction/DifferentialRevisionAbandonTransaction.php',
'DifferentialRevisionAcceptTransaction' => 'applications/differential/xaction/DifferentialRevisionAcceptTransaction.php',
'DifferentialRevisionActionTransaction' => 'applications/differential/xaction/DifferentialRevisionActionTransaction.php',
'DifferentialRevisionAffectedFilesHeraldField' => 'applications/differential/herald/DifferentialRevisionAffectedFilesHeraldField.php',
'DifferentialRevisionAuthorHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorHeraldField.php',
@ -545,6 +546,7 @@ phutil_register_library_map(array(
'DifferentialRevisionPlanChangesTransaction' => 'applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php',
'DifferentialRevisionQuery' => 'applications/differential/query/DifferentialRevisionQuery.php',
'DifferentialRevisionReclaimTransaction' => 'applications/differential/xaction/DifferentialRevisionReclaimTransaction.php',
'DifferentialRevisionRejectTransaction' => 'applications/differential/xaction/DifferentialRevisionRejectTransaction.php',
'DifferentialRevisionRelationship' => 'applications/differential/relationships/DifferentialRevisionRelationship.php',
'DifferentialRevisionRelationshipSource' => 'applications/search/relationship/DifferentialRevisionRelationshipSource.php',
'DifferentialRevisionReopenTransaction' => 'applications/differential/xaction/DifferentialRevisionReopenTransaction.php',
@ -553,6 +555,7 @@ phutil_register_library_map(array(
'DifferentialRevisionRepositoryTransaction' => 'applications/differential/xaction/DifferentialRevisionRepositoryTransaction.php',
'DifferentialRevisionRequestReviewTransaction' => 'applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php',
'DifferentialRevisionRequiredActionResultBucket' => 'applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php',
'DifferentialRevisionResignTransaction' => 'applications/differential/xaction/DifferentialRevisionResignTransaction.php',
'DifferentialRevisionResultBucket' => 'applications/differential/query/DifferentialRevisionResultBucket.php',
'DifferentialRevisionReviewersHeraldField' => 'applications/differential/herald/DifferentialRevisionReviewersHeraldField.php',
'DifferentialRevisionReviewersTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewersTransaction.php',
@ -5180,6 +5183,7 @@ phutil_register_library_map(array(
'PhabricatorConduitResultInterface',
),
'DifferentialRevisionAbandonTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionActionTransaction' => 'DifferentialRevisionTransactionType',
'DifferentialRevisionAffectedFilesHeraldField' => 'DifferentialRevisionHeraldField',
'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField',
@ -5219,6 +5223,7 @@ phutil_register_library_map(array(
'DifferentialRevisionPlanChangesTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'DifferentialRevisionReclaimTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionRelationship' => 'PhabricatorObjectRelationship',
'DifferentialRevisionRelationshipSource' => 'PhabricatorObjectRelationshipSource',
'DifferentialRevisionReopenTransaction' => 'DifferentialRevisionActionTransaction',
@ -5227,6 +5232,7 @@ phutil_register_library_map(array(
'DifferentialRevisionRepositoryTransaction' => 'DifferentialRevisionTransactionType',
'DifferentialRevisionRequestReviewTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionRequiredActionResultBucket' => 'DifferentialRevisionResultBucket',
'DifferentialRevisionResignTransaction' => 'DifferentialRevisionActionTransaction',
'DifferentialRevisionResultBucket' => 'PhabricatorSearchResultBucket',
'DifferentialRevisionReviewersHeraldField' => 'DifferentialRevisionHeraldField',
'DifferentialRevisionReviewersTransaction' => 'DifferentialRevisionTransactionType',

View file

@ -9,6 +9,7 @@ final class DifferentialReviewerStatus extends Phobject {
const STATUS_COMMENTED = 'commented';
const STATUS_ACCEPTED_OLDER = 'accepted-older';
const STATUS_REJECTED_OLDER = 'rejected-older';
const STATUS_RESIGNED = 'resigned';
/**
* Returns the relative strength of a status, used to pick a winner when a
@ -34,6 +35,7 @@ final class DifferentialReviewerStatus extends Phobject {
self::STATUS_ACCEPTED => 5,
self::STATUS_REJECTED => 5,
self::STATUS_RESIGNED => 5,
);
return idx($map, $constant, 0);

View file

@ -38,7 +38,8 @@ final class DifferentialRevisionEditEngine
protected function newObjectQuery() {
return id(new DifferentialRevisionQuery())
->needActiveDiffs(true)
->needReviewerStatus(true);
->needReviewerStatus(true)
->needReviewerAuthority(true);
}
protected function getObjectCreateTitleText($object) {

View file

@ -0,0 +1,77 @@
<?php
final class DifferentialRevisionAcceptTransaction
extends DifferentialRevisionActionTransaction {
const TRANSACTIONTYPE = 'differential.revision.accept';
const ACTIONKEY = 'accept';
protected function getRevisionActionLabel() {
return pht("Accept Revision \xE2\x9C\x94");
}
protected function getRevisionActionDescription() {
return pht('These changes will be approved.');
}
public function getIcon() {
return 'fa-check-circle-o';
}
public function getColor() {
return 'green';
}
public function generateOldValue($object) {
$actor = $this->getActor();
return $this->isViewerAcceptingReviewer($object, $actor);
}
public function applyExternalEffects($object, $value) {
$status = DifferentialReviewerStatus::STATUS_ACCEPTED;
$actor = $this->getActor();
$this->applyReviewerEffect($object, $actor, $value, $status);
}
protected function validateAction($object, PhabricatorUser $viewer) {
if ($object->isClosed()) {
throw new Exception(
pht(
'You can not accept this revision because it has already been '.
'closed. Only open revisions can be accepted.'));
}
$config_key = 'differential.allow-self-accept';
if (!PhabricatorEnv::getEnvConfig($config_key)) {
if ($this->isViewerRevisionAuthor($object, $viewer)) {
throw new Exception(
pht(
'You can not accept this revision because you are the revision '.
'author. You can only accept revisions you do not own. You can '.
'change this behavior by adjusting the "%s" setting in Config.',
$config_key));
}
}
if ($this->isViewerAcceptingReviewer($object, $viewer)) {
throw new Exception(
pht(
'You can not accept this revision because you have already '.
'accepted it.'));
}
}
public function getTitle() {
return pht(
'%s accepted this revision.',
$this->renderAuthor());
}
public function getTitleForFeed() {
return pht(
'%s accepted %s.',
$this->renderAuthor(),
$this->renderObject());
}
}

View file

@ -41,6 +41,116 @@ abstract class DifferentialRevisionActionTransaction
return ($viewer->getPHID() === $revision->getAuthorPHID());
}
protected function isViewerAnyReviewer(
DifferentialRevision $revision,
PhabricatorUser $viewer) {
return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
}
protected function isViewerAcceptingReviewer(
DifferentialRevision $revision,
PhabricatorUser $viewer) {
return $this->isViewerReviewerStatusAmong(
$revision,
$viewer,
array(
DifferentialReviewerStatus::STATUS_ACCEPTED,
));
}
protected function isViewerRejectingReviewer(
DifferentialRevision $revision,
PhabricatorUser $viewer) {
return $this->isViewerReviewerStatusAmong(
$revision,
$viewer,
array(
DifferentialReviewerStatus::STATUS_REJECTED,
));
}
protected function getViewerReviewerStatus(
DifferentialRevision $revision,
PhabricatorUser $viewer) {
if (!$viewer->getPHID()) {
return null;
}
foreach ($revision->getReviewerStatus() as $reviewer) {
if ($reviewer->getReviewerPHID() != $viewer->getPHID()) {
continue;
}
return $reviewer->getStatus();
}
return null;
}
protected function isViewerReviewerStatusAmong(
DifferentialRevision $revision,
PhabricatorUser $viewer,
array $status_list) {
$status = $this->getViewerReviewerStatus($revision, $viewer);
if ($status === null) {
return false;
}
$status_map = array_fuse($status_list);
return isset($status_map[$status]);
}
protected function applyReviewerEffect(
DifferentialRevision $revision,
PhabricatorUser $viewer,
$value,
$status) {
$map = array();
// When you accept or reject, you may accept or reject on behalf of all
// reviewers you have authority for. When you resign, you only affect
// yourself.
$with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED);
if ($with_authority) {
foreach ($revision->getReviewerStatus() as $reviewer) {
if ($reviewer->hasAuthority($viewer)) {
$map[$reviewer->getReviewerPHID()] = $status;
}
}
}
// In all cases, you affect yourself.
$map[$viewer->getPHID()] = $status;
// Convert reviewer statuses into edge data.
foreach ($map as $reviewer_phid => $reviewer_status) {
$map[$reviewer_phid] = array(
'data' => array(
'status' => $reviewer_status,
),
);
}
$src_phid = $revision->getPHID();
$edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
$editor = new PhabricatorEdgeEditor();
foreach ($map as $dst_phid => $edge_data) {
if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
// TODO: For now, we just remove these reviewers. In the future, we will
// store resignations explicitly.
$editor->removeEdge($src_phid, $edge_type, $dst_phid);
} else {
$editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data);
}
}
$editor->save();
}
public function newEditField(
DifferentialRevision $revision,
PhabricatorUser $viewer) {

View file

@ -0,0 +1,74 @@
<?php
final class DifferentialRevisionRejectTransaction
extends DifferentialRevisionActionTransaction {
const TRANSACTIONTYPE = 'differential.revision.reject';
const ACTIONKEY = 'reject';
protected function getRevisionActionLabel() {
return pht("Request Changes \xE2\x9C\x98");
}
protected function getRevisionActionDescription() {
return pht('This revision will be returned to the author for updates.');
}
public function getIcon() {
return 'fa-times-circle-o';
}
public function getColor() {
return 'red';
}
public function generateOldValue($object) {
$actor = $this->getActor();
return $this->isViewerRejectingReviewer($object, $actor);
}
public function applyExternalEffects($object, $value) {
$status = DifferentialReviewerStatus::STATUS_REJECTED;
$actor = $this->getActor();
$this->applyReviewerEffect($object, $actor, $value, $status);
}
protected function validateAction($object, PhabricatorUser $viewer) {
if ($object->isClosed()) {
throw new Exception(
pht(
'You can not request changes to this revision because it has '.
'already been closed. You can only request changes to open '.
'revisions.'));
}
if ($this->isViewerRevisionAuthor($object, $viewer)) {
throw new Exception(
pht(
'You can not request changes to this revision because you are the '.
'revision author. You can only request changes to revisions you do '.
'not own.'));
}
if ($this->isViewerRejectingReviewer($object, $viewer)) {
throw new Exception(
pht(
'You can not request changes to this revision because you have '.
'already requested changes.'));
}
}
public function getTitle() {
return pht(
'%s requested changes to this revision.',
$this->renderAuthor());
}
public function getTitleForFeed() {
return pht(
'%s requested changes to %s.',
$this->renderAuthor(),
$this->renderObject());
}
}

View file

@ -0,0 +1,66 @@
<?php
final class DifferentialRevisionResignTransaction
extends DifferentialRevisionActionTransaction {
const TRANSACTIONTYPE = 'differential.revision.resign';
const ACTIONKEY = 'resign';
protected function getRevisionActionLabel() {
return pht('Resign as Reviewer');
}
protected function getRevisionActionDescription() {
return pht('You will resign as a reviewer for this change.');
}
public function getIcon() {
return 'fa-flag';
}
public function getColor() {
return 'orange';
}
public function generateOldValue($object) {
$actor = $this->getActor();
return !$this->isViewerAnyReviewer($object, $actor);
}
public function applyExternalEffects($object, $value) {
$status = DifferentialReviewerStatus::STATUS_RESIGNED;
$actor = $this->getActor();
$this->applyReviewerEffect($object, $actor, $value, $status);
}
protected function validateAction($object, PhabricatorUser $viewer) {
if ($object->isClosed()) {
throw new Exception(
pht(
'You can not resign from this revision because it has already '.
'been closed. You can only resign from open revisions.'));
}
if (!$this->isViewerAnyReviewer($object, $viewer)) {
throw new Exception(
pht(
'You can not resign from this revision because you are not a '.
'reviewer. You can only resign from revisions where you are a '.
'reviewer.'));
}
}
public function getTitle() {
return pht(
'%s resigned from this revision.',
$this->renderAuthor());
}
public function getTitleForFeed() {
return pht(
'%s resigned from %s.',
$this->renderAuthor(),
$this->renderObject());
}
}