Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F15420536
D18412.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
16 KB
Referenced Files
None
Subscribers
None
D18412.diff
View Options
diff --git a/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php
--- a/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php
+++ b/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php
@@ -52,8 +52,9 @@
$xactions = array();
$xactions[] = id(new DifferentialTransaction())
- ->setTransactionType(DifferentialTransaction::TYPE_ACTION)
- ->setNewValue(DifferentialAction::ACTION_CLOSE);
+ ->setTransactionType(
+ DifferentialRevisionCloseTransaction::TRANSACTIONTYPE)
+ ->setNewValue(true);
$content_source = $request->newContentSource();
diff --git a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
--- a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
+++ b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php
@@ -63,6 +63,7 @@
'resign' => DifferentialRevisionResignTransaction::TRANSACTIONTYPE,
'request_review' =>
DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE,
+ 'rethink' => DifferentialRevisionPlanChangesTransaction::TRANSACTIONTYPE,
);
$action = $request->getValue('action');
@@ -76,9 +77,10 @@
case 'none':
break;
default:
- $xactions[] = id(new DifferentialTransaction())
- ->setTransactionType(DifferentialTransaction::TYPE_ACTION)
- ->setNewValue($action);
+ throw new Exception(
+ pht(
+ 'Unsupported action "%s".',
+ $action));
break;
}
}
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
@@ -69,7 +69,6 @@
$types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
$types[] = PhabricatorTransactions::TYPE_INLINESTATE;
- $types[] = DifferentialTransaction::TYPE_ACTION;
$types[] = DifferentialTransaction::TYPE_INLINE;
$types[] = DifferentialTransaction::TYPE_UPDATE;
@@ -81,8 +80,6 @@
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
- case DifferentialTransaction::TYPE_ACTION:
- return null;
case DifferentialTransaction::TYPE_INLINE:
return null;
case DifferentialTransaction::TYPE_UPDATE:
@@ -101,7 +98,6 @@
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
- case DifferentialTransaction::TYPE_ACTION:
case DifferentialTransaction::TYPE_UPDATE:
return $xaction->getNewValue();
case DifferentialTransaction::TYPE_INLINE:
@@ -120,28 +116,6 @@
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;
- $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
-
- $action_type = $xaction->getNewValue();
- switch ($action_type) {
- 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_plan);
- case DifferentialAction::ACTION_CLAIM:
- return ($actor_phid != $object->getAuthorPHID());
- }
}
return parent::transactionHasEffect($object, $xaction);
@@ -183,38 +157,6 @@
// TODO: Update the `diffPHID` once we add that.
return;
- case DifferentialTransaction::TYPE_ACTION:
- switch ($xaction->getNewValue()) {
- case DifferentialAction::ACTION_ABANDON:
- $object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
- return;
- case DifferentialAction::ACTION_RETHINK:
- $object->setStatus($status_plan);
- return;
- case DifferentialAction::ACTION_RECLAIM:
- $object->setStatus($status_review);
- return;
- case DifferentialAction::ACTION_REOPEN:
- $object->setStatus($status_review);
- return;
- case DifferentialAction::ACTION_CLOSE:
- $old_status = $object->getStatus();
- $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
- $was_accepted = ($old_status == $status_accepted);
- $object->setProperty(
- DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED,
- $was_accepted);
- return;
- case DifferentialAction::ACTION_CLAIM:
- $object->setAuthorPHID($this->getActingAsPHID());
- return;
- default:
- throw new Exception(
- pht(
- 'Differential action "%s" is not a valid action!',
- $xaction->getNewValue()));
- }
- break;
}
return parent::applyCustomInternalTransaction($object, $xaction);
@@ -356,16 +298,6 @@
case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE:
$is_commandeer = true;
break;
-
- case DifferentialTransaction::TYPE_ACTION:
- $action_type = $xaction->getNewValue();
-
- switch ($action_type) {
- case DifferentialAction::ACTION_CLAIM:
- $is_commandeer = true;
- break;
- }
- break;
}
if ($is_commandeer) {
@@ -375,7 +307,6 @@
if (!$this->didExpandInlineState) {
switch ($xaction->getTransactionType()) {
case PhabricatorTransactions::TYPE_COMMENT:
- case DifferentialTransaction::TYPE_ACTION:
case DifferentialTransaction::TYPE_UPDATE:
case DifferentialTransaction::TYPE_INLINE:
$this->didExpandInlineState = true;
@@ -421,8 +352,6 @@
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
- case DifferentialTransaction::TYPE_ACTION:
- return;
case DifferentialTransaction::TYPE_INLINE:
$reply = $xaction->getComment()->getReplyToComment();
if ($reply && !$reply->getHasReplies()) {
@@ -660,172 +589,12 @@
$xaction);
}
break;
- 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->getActingAsPHID();
- $actor_is_author = ($author_phid == $actor_phid);
-
- $config_abandon_key = 'differential.always-allow-abandon';
- $always_allow_abandon = PhabricatorEnv::getEnvConfig($config_abandon_key);
-
- $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);
-
- $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;
- $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED;
- $status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
-
- switch ($action) {
- case DifferentialAction::ACTION_CLAIM:
- // You can claim a revision if you're not the owner. If you are, this
- // is a no-op rather than invalid.
-
- if ($revision_status == $status_closed) {
- return pht(
- 'You can not commandeer this revision because it has already been '.
- 'closed.');
- }
- break;
-
- case DifferentialAction::ACTION_ABANDON:
- if (!$actor_is_author && !$always_allow_abandon) {
- 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 changes 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::CHANGES_PLANNED:
- // Let this through, it's a no-op.
- 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_CLOSE:
- // We force revisions closed when we discover a corresponding commit.
- // In this case, revisions are allowed to transition to closed from
- // any state. This is an automated action taken by the daemons.
-
- if (!$this->getIsCloseByCommit()) {
- 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) {
$xactions = parent::sortTransactions($xactions);
@@ -1233,14 +1002,6 @@
// When users commandeer revisions, we may need to trigger
// signatures or author-based rules.
return true;
- case DifferentialTransaction::TYPE_ACTION:
- switch ($xaction->getNewValue()) {
- case DifferentialAction::ACTION_CLAIM:
- // When users commandeer revisions, we may need to trigger
- // signatures or author-based rules.
- return true;
- }
- break;
}
}
diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php
@@ -46,6 +46,12 @@
}
protected function validateAction($object, PhabricatorUser $viewer) {
+ if ($this->getEditor()->getIsCloseByCommit()) {
+ // If we're closing a revision because we discovered a commit, we don't
+ // care what state it was in.
+ return;
+ }
+
if ($object->isClosed()) {
throw new Exception(
pht(
@@ -74,9 +80,50 @@
}
public function getTitle() {
- return pht(
- '%s closed this revision.',
- $this->renderAuthor());
+ if (!$this->getMetadataValue('isCommitClose')) {
+ return pht(
+ '%s closed this revision.',
+ $this->renderAuthor());
+ }
+
+ $commit_phid = $this->getMetadataValue('commitPHID');
+ $committer_phid = $this->getMetadataValue('committerPHID');
+ $author_phid = $this->getMetadataValue('authorPHID');
+
+ if ($committer_phid) {
+ $committer_name = $this->renderHandle($committer_phid);
+ } else {
+ $committer_name = $this->getMetadataValue('committerName');
+ }
+
+ if ($author_phid) {
+ $author_name = $this->renderHandle($author_phid);
+ } else {
+ $author_name = $this->getMetadatavalue('authorName');
+ }
+
+ $same_phid =
+ strlen($committer_phid) &&
+ strlen($author_phid) &&
+ ($committer_phid == $author_phid);
+
+ $same_name =
+ !strlen($committer_phid) &&
+ !strlen($author_phid) &&
+ ($committer_name == $author_name);
+
+ if ($same_name || $same_phid) {
+ return pht(
+ 'Closed by commit %s (authored by %s).',
+ $this->renderHandle($commit_phid),
+ $author_name);
+ } else {
+ return pht(
+ 'Closed by commit %s (authored by %s, committed by %s).',
+ $this->renderHandle($commit_phid),
+ $author_name,
+ $committer_name);
+ }
}
public function getTitleForFeed() {
diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
--- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
+++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php
@@ -205,9 +205,11 @@
$should_close = !$revision->isPublished() && $should_autoclose;
if ($should_close) {
- $commit_close_xaction = id(new DifferentialTransaction())
- ->setTransactionType(DifferentialTransaction::TYPE_ACTION)
- ->setNewValue(DifferentialAction::ACTION_CLOSE)
+ $type_close = DifferentialRevisionCloseTransaction::TRANSACTIONTYPE;
+
+ $commit_close_xaction = id(new DifferentialTransaction())
+ ->setTransactionType($type_close)
+ ->setNewValue(true)
->setMetadataValue('isCommitClose', true);
$commit_close_xaction->setMetadataValue(
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Sat, Mar 22, 2:44 PM (1 d, 21 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7717126
Default Alt Text
D18412.diff (16 KB)
Attached To
Mode
D18412: Modularize remaining TYPE_ACTION transactions in Differential, reducing calls to ArcanistDifferentialRevisionStatus
Attached
Detach File
Event Timeline
Log In to Comment