Page MenuHomePhabricator
Files F14003863

D8307.id19756.diff
No OneTemporary
Actions

D8307.id19756.diff
View Options

diff --git a/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php b/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
--- a/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
+++ b/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php
@@ -133,6 +133,9 @@
return id(new PhabricatorApplicationTransactionNoEffectResponse())
->setCancelURI($revision_uri)
->setException($ex);
+ } catch (PhabricatorApplicationTransactionValidationException $ex) {
+ // TODO: Provide a nice Response for rendering these in a clean way.
+ throw $ex;
}
$user = $request->getUser();
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -63,6 +63,26 @@
switch ($xaction->getTransactionType()) {
case DifferentialTransaction::TYPE_INLINE:
return $xaction->hasComment();
+ case DifferentialTransaction::TYPE_ACTION:
+ $status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
+ $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED;
+ $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
+ $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+
+ switch ($xaction->getNewValue()) {
+ case DifferentialAction::ACTION_CLOSE:
+ return ($object->getStatus() != $status_closed);
+ case DifferentialAction::ACTION_ABANDON:
+ return ($object->getStatus() != $status_abandoned);
+ case DifferentialAction::ACTION_RECLAIM:
+ return ($object->getStatus() == $status_abandoned);
+ case DifferentialAction::ACTION_REOPEN:
+ return ($object->getStatus() == $status_closed);
+ case DifferentialAction::ACTION_RETHINK:
+ return ($object->getStatus() != $status_revision);
+ case DifferentialAction::ACTION_REQUEST:
+ return ($object->getStatus() != $status_review);
+ }
}
return parent::transactionHasEffect($object, $xaction);
@@ -89,8 +109,41 @@
// to "Accepted".
return;
case DifferentialTransaction::TYPE_ACTION:
- // TODO: For now, we're just shipping these through without acting
- // on them.
+ $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
+ $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+
+ switch ($xaction->getNewValue()) {
+ case DifferentialAction::ACTION_ABANDON:
+ $object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
+ break;
+ case DifferentialAction::ACTION_RETHINK:
+ $object->setStatus($status_revision);
+ break;
+ case DifferentialAction::ACTION_RECLAIM:
+ $object->setStatus($status_review);
+ // TODO: Update review status?
+ break;
+ case DifferentialAction::ACTION_REOPEN:
+ $object->setStatus($status_review);
+ // TODO: Update review status?
+ break;
+ case DifferentialAction::ACTION_REQUEST:
+ $object->setStatus($status_review);
+ // TODO: Update review status?
+ break;
+ case DifferentialAction::ACTION_CLOSE:
+ if (!$object->getDateCommitted()) {
+ // TODO: Can we remove this? It is probably no longer used by
+ // anything anymore. See also T4434.
+ $object->setDateCommitted(time());
+ }
+ $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
+ break;
+ default:
+ // TODO: For now, we're just shipping the rest of these through
+ // without acting on them.
+ break;
+ }
return null;
}
@@ -123,12 +176,185 @@
$errors = parent::validateTransaction($object, $type, $xactions);
- switch ($type) {
+ foreach ($xactions as $xaction) {
+ switch ($type) {
+ case DifferentialTransaction::TYPE_ACTION:
+ $error = $this->validateDifferentialAction(
+ $object,
+ $type,
+ $xaction,
+ $xaction->getNewValue());
+ if ($error) {
+ $errors[] = new PhabricatorApplicationTransactionValidationError(
+ $type,
+ pht('Invalid'),
+ $error,
+ $xaction);
+ }
+ break;
+ }
}
return $errors;
}
+ private function validateDifferentialAction(
+ DifferentialRevision $revision,
+ $type,
+ DifferentialTransaction $xaction,
+ $action) {
+
+ $author_phid = $revision->getAuthorPHID();
+ $actor_phid = $this->getActor()->getPHID();
+ $actor_is_author = ($author_phid == $actor_phid);
+
+ $config_close_key = 'differential.always-allow-close';
+ $always_allow_close = PhabricatorEnv::getEnvConfig($config_close_key);
+
+ $config_reopen_key = 'differential.allow-reopen';
+ $allow_reopen = PhabricatorEnv::getEnvConfig($config_reopen_key);
+
+ $revision_status = $revision->getStatus();
+
+ $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+ $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED;
+ $status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
+
+ switch ($action) {
+ case DifferentialAction::ACTION_ABANDON:
+ if (!$actor_is_author) {
+ return pht(
+ "You can not abandon this revision because you do not own it. ".
+ "You can only abandon revisions you own.");
+ }
+
+ if ($revision_status == $status_closed) {
+ return pht(
+ "You can not abandon this revision because it has already been ".
+ "closed.");
+ }
+
+ // NOTE: Abandons of already-abandoned revisions are treated as no-op
+ // instead of invalid. Other abandons are OK.
+
+ break;
+
+ case DifferentialAction::ACTION_RECLAIM:
+ if (!$actor_is_author) {
+ return pht(
+ "You can not reclaim this revision because you do not own ".
+ "it. You can only reclaim revisions you own.");
+ }
+
+ if ($revision_status == $status_closed) {
+ return pht(
+ "You can not reclaim this revision because it has already been ".
+ "closed.");
+ }
+
+ // NOTE: Reclaims of other non-abandoned revisions are treated as no-op
+ // instead of invalid.
+
+ break;
+
+ case DifferentialAction::ACTION_REOPEN:
+ if (!$allow_reopen) {
+ return pht(
+ 'The reopen action is not enabled on this Phabricator install. '.
+ 'Adjust your configuration to enable it.');
+ }
+
+ // NOTE: If the revision is not closed, this is caught as a no-op
+ // instead of an invalid transaction.
+
+ break;
+
+ case DifferentialAction::ACTION_RETHINK:
+ if (!$actor_is_author) {
+ return pht(
+ "You can not plan changes to this revision because you do not ".
+ "own it. To plan chagnes to a revision, you must be its owner.");
+ }
+
+ switch ($revision_status) {
+ case ArcanistDifferentialRevisionStatus::ACCEPTED:
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+ // These are OK.
+ break;
+ case ArcanistDifferentialRevisionStatus::ABANDONED:
+ return pht(
+ "You can not plan changes to this revision because it has ".
+ "been abandoned.");
+ case ArcanistDifferentialRevisionStatus::CLOSED:
+ return pht(
+ "You can not plan changes to this revision because it has ".
+ "already been closed.");
+ default:
+ throw new Exception(
+ pht(
+ 'Encountered unexpected revision status ("%s") when '.
+ 'validating "%s" action.',
+ $revision_status,
+ $action));
+ }
+ break;
+
+ case DifferentialAction::ACTION_REQUEST:
+ if (!$actor_is_author) {
+ return pht(
+ "You can not request review of this revision because you do ".
+ "not own it. To request review of a revision, you must be its ".
+ "owner.");
+ }
+
+ switch ($revision_status) {
+ case ArcanistDifferentialRevisionStatus::ACCEPTED:
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+ // These are OK.
+ break;
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+ // This will be caught as "no effect" later on.
+ break;
+ case ArcanistDifferentialRevisionStatus::ABANDONED:
+ return pht(
+ "You can not request review of this revision because it has ".
+ "been abandoned. Instead, reclaim it.");
+ case ArcanistDifferentialRevisionStatus::CLOSED:
+ return pht(
+ "You can not request review of this revision because it has ".
+ "already been closed.");
+ default:
+ throw new Exception(
+ pht(
+ 'Encountered unexpected revision status ("%s") when '.
+ 'validating "%s" action.',
+ $revision_status,
+ $action));
+ }
+ break;
+
+ case DifferentialAction::ACTION_CLOSE:
+
+ // TODO: Permit the daemons to take this action in all cases.
+
+ if (!$actor_is_author && !$always_allow_close) {
+ return pht(
+ "You can not close this revision because you do not own it. To ".
+ "close a revision, you must be its owner.");
+ }
+
+ if ($revision_status != $status_accepted) {
+ return pht(
+ "You can not close this revision because it has not been ".
+ "accepted. You can only close accepted revisions.");
+ }
+ break;
+ }
+
+ return null;
+ }
+
protected function sortTransactions(array $xactions) {
$head = array();
$tail = array();
diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php
--- a/src/applications/differential/storage/DifferentialTransaction.php
+++ b/src/applications/differential/storage/DifferentialTransaction.php
@@ -123,6 +123,27 @@
return pht(
'Those reviewers are already reviewing this revision.');
}
+ break;
+ case DifferentialTransaction::TYPE_ACTION:
+ switch ($this->getNewValue()) {
+ case DifferentialAction::ACTION_CLOSE:
+ return pht('This revision is already closed.');
+ case DifferentialAction::ACTION_ABANDON:
+ return pht('This revision has already been abandoned.');
+ case DifferentialAction::ACTION_RECLAIM:
+ return pht(
+ 'You can not reclaim this revision because his revision is '.
+ 'not abandoned.');
+ case DifferentialAction::ACTION_REOPEN:
+ return pht(
+ 'You can not reopen this revision because this revision is '.
+ 'not closed.');
+ case DifferentialAction::ACTION_RETHINK:
+ return pht('This revision already requires changes.');
+ case DifferentialAction::ACTION_REQUEST:
+ return pht('Review is already requested for this revision.');
+ }
+ break;
}
return parent::getNoEffectDescription();

File Metadata

Mime Type
text/plain
Expires
Sun, Oct 27, 11:41 AM (3 w, 2 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6752289
Default Alt Text
D8307.id19756.diff (11 KB)

Event Timeline

Phabricator · Built by Phacility