Page MenuHomePhabricator
Files F14034335

D8333.diff
No OneTemporary
Actions

D8333.diff
View Options

Index: src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
===================================================================
--- src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
+++ src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
@@ -21,6 +21,7 @@
->setViewer($viewer)
->withIDs(array($this->id))
->needReviewerStatus(true)
+ ->needReviewerAuthority(true)
->executeOne();
if (!$revision) {
return new Aphront404Response();
Index: src/applications/differential/editor/DifferentialTransactionEditor.php
===================================================================
--- src/applications/differential/editor/DifferentialTransactionEditor.php
+++ src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -69,7 +69,36 @@
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
$status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
- switch ($xaction->getNewValue()) {
+ $action_type = $xaction->getNewValue();
+ switch ($action_type) {
+ case DifferentialAction::ACTION_ACCEPT:
+ case DifferentialAction::ACTION_REJECT:
+ if ($action_type == DifferentialAction::ACTION_ACCEPT) {
+ $new_status = DifferentialReviewerStatus::STATUS_ACCEPTED;
+ } else {
+ $new_status = DifferentialReviewerStatus::STATUS_REJECTED;
+ }
+
+ $actor = $this->getActor();
+ $actor_phid = $actor->getPHID();
+
+ // These transactions can cause effects in to ways: by altering the
+ // status of an existing reviewer; or by adding the actor as a new
+ // reviewer.
+
+ $will_add_reviewer = true;
+ foreach ($object->getReviewerStatus() as $reviewer) {
+ if ($reviewer->hasAuthority($actor)) {
+ if ($reviewer->getStatus() != $new_status) {
+ return true;
+ }
+ }
+ if ($reviewer->getReviewerPHID() == $actor_phid) {
+ $will_add_reviwer = false;
+ }
+ }
+
+ return $will_add_reviewer;
case DifferentialAction::ACTION_CLOSE:
return ($object->getStatus() != $status_closed);
case DifferentialAction::ACTION_ABANDON:
@@ -116,8 +145,7 @@
case DifferentialTransaction::TYPE_INLINE:
return;
case PhabricatorTransactions::TYPE_EDGE:
- // TODO: When removing reviewers, we may be able to move the revision
- // to "Accepted".
+ // TODO: Update review status.
return;
case DifferentialTransaction::TYPE_ACTION:
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
@@ -125,38 +153,37 @@
switch ($xaction->getNewValue()) {
case DifferentialAction::ACTION_RESIGN:
- // TODO: Update review status?
- break;
+ case DifferentialAction::ACTION_ACCEPT:
+ case DifferentialAction::ACTION_REJECT:
+ // These have no direct effects, and affect review status only
+ // indirectly by altering reviewers with TYPE_EDGE transactions.
+ return;
case DifferentialAction::ACTION_ABANDON:
$object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
- break;
+ return;
case DifferentialAction::ACTION_RETHINK:
$object->setStatus($status_revision);
- break;
+ return;
case DifferentialAction::ACTION_RECLAIM:
$object->setStatus($status_review);
// TODO: Update review status?
- break;
+ return;
case DifferentialAction::ACTION_REOPEN:
$object->setStatus($status_review);
// TODO: Update review status?
- break;
+ return;
case DifferentialAction::ACTION_REQUEST:
$object->setStatus($status_review);
// TODO: Update review status?
- break;
+ return;
case DifferentialAction::ACTION_CLOSE:
$object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
- break;
+ return;
case DifferentialAction::ACTION_CLAIM:
$object->setAuthorPHID($this->getActor()->getPHID());
- break;
- default:
- // TODO: For now, we're just shipping the rest of these through
- // without acting on them.
- break;
+ return;
}
- return null;
+ break;
}
return parent::applyCustomInternalTransaction($object, $xaction);
@@ -168,13 +195,52 @@
$results = parent::expandTransaction($object, $xaction);
switch ($xaction->getTransactionType()) {
+ // TODO: If the user comments and is a reviewer, we should upgrade their
+ // edge metadata to STATUS_COMMENTED.
+
case DifferentialTransaction::TYPE_ACTION:
- $actor_phid = $this->getActor()->getPHID();
+ $actor = $this->getActor();
+ $actor_phid = $actor->getPHID();
$type_edge = PhabricatorTransactions::TYPE_EDGE;
$edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
+ $action_type = $xaction->getNewValue();
+
+ switch ($action_type) {
+ case DifferentialAction::ACTION_ACCEPT:
+ case DifferentialAction::ACTION_REJECT:
+ if ($action_type == DifferentialAction::ACTION_ACCEPT) {
+ $data = array(
+ 'status' => DifferentialReviewerStatus::STATUS_ACCEPTED,
+ );
+ } else {
+ $data = array(
+ 'status' => DifferentialReviewerStatus::STATUS_REJECTED,
+ );
+ }
+
+ $edits = array();
+
+ foreach ($object->getReviewerStatus() as $reviewer) {
+ if ($reviewer->hasAuthority($actor)) {
+ $edits[$reviewer->getReviewerPHID()] = array(
+ 'data' => $data,
+ );
+ }
+ }
+
+ // Also either update or add the actor themselves as a reviewer.
+ $edits[$actor_phid] = array(
+ 'data' => $data,
+ );
+
+ $results[] = id(new DifferentialTransaction())
+ ->setTransactionType($type_edge)
+ ->setMetadataValue('edge:type', $edge_reviewer)
+ ->setIgnoreOnNoEffect(true)
+ ->setNewValue(array('+' => $edits));
+ break;
- switch ($xaction->getNewValue()) {
case DifferentialAction::ACTION_CLAIM:
// If the user is commandeering, add the previous owner as a
// reviewer and remove the actor.
@@ -294,6 +360,9 @@
$config_reopen_key = 'differential.allow-reopen';
$allow_reopen = PhabricatorEnv::getEnvConfig($config_reopen_key);
+ $config_self_accept_key = 'differential.allow-self-accept';
+ $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key);
+
$revision_status = $revision->getStatus();
$status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
@@ -301,6 +370,44 @@
$status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
switch ($action) {
+ case DifferentialAction::ACTION_ACCEPT:
+ if ($actor_is_author && !$allow_self_accept) {
+ return pht(
+ 'You can not accept this revision because you are the owner.');
+ }
+
+ if ($revision_status == $status_abandoned) {
+ return pht(
+ 'You can not accept this revision because it has been '.
+ 'abandoned.');
+ }
+
+ if ($revision_status == $status_closed) {
+ return pht(
+ 'You can not accept this revision because it has already been '.
+ 'closed.');
+ }
+ break;
+
+ case DifferentialAction::ACTION_REJECT:
+ if ($actor_is_author) {
+ return pht(
+ 'You can not request changes to your own revision.');
+ }
+
+ if ($revision_status == $status_abandoned) {
+ return pht(
+ 'You can not request changes to this revision because it has been '.
+ 'abandoned.');
+ }
+
+ if ($revision_status == $status_closed) {
+ return pht(
+ 'You can not request changes to this revision because it has '.
+ 'already been closed.');
+ }
+ break;
+
case DifferentialAction::ACTION_RESIGN:
// You can always resign from a revision if you're a reviewer. If you
// aren't, this is a no-op rather than invalid.
Index: src/applications/differential/storage/DifferentialTransaction.php
===================================================================
--- src/applications/differential/storage/DifferentialTransaction.php
+++ src/applications/differential/storage/DifferentialTransaction.php
@@ -151,6 +151,12 @@
return pht(
'You can not commandeer this revision because you already own '.
'it.');
+ case DifferentialAction::ACTION_ACCEPT:
+ return pht(
+ 'You have already accepted this revision.');
+ case DifferentialAction::ACTION_REJECT:
+ return pht(
+ 'You have already requested changes to this revision.');
}
break;
}
Index: src/applications/transactions/storage/PhabricatorApplicationTransaction.php
===================================================================
--- src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -438,13 +438,17 @@
$this->renderHandleLink($author_phid),
count($add),
$this->renderHandleList($add));
- } else {
+ } else if ($rem) {
$string = PhabricatorEdgeConfig::getRemoveStringForEdgeType($type);
return pht(
$string,
$this->renderHandleLink($author_phid),
count($rem),
$this->renderHandleList($rem));
+ } else {
+ return pht(
+ '%s edited edge metadata.',
+ $this->renderHandleLink($author_phid));
}
case PhabricatorTransactions::TYPE_CUSTOMFIELD:

File Metadata

Mime Type
text/plain
Expires
Sun, Nov 10, 11:41 PM (1 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6731883
Default Alt Text
D8333.diff (10 KB)

Event Timeline

Phabricator · Built by Phacility