From 68d90ef6986d85dc80d5c13f62a502051fa3301c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Apr 2012 14:59:31 -0700 Subject: [PATCH] Allow revisions to be commandeered Summary: - Adds "Commandeer Revision", to allow you to plunder revisions from those lost to sea (e.g., interns who have left or co-workers who are dealing with a family emergency). - Removes admin-abandon to simplify things, since you can just Commandeer + Abandon now. - There are other workarounds available but this is the natural/expected workflow (and the one everyone always asks for) and there's no real reason not to allow it. Test Plan: Swashbuckled. Reviewers: cpiro, btrahan, vrana, jungejason Reviewed By: cpiro CC: aran, zeeg Differential Revision: https://secure.phabricator.com/D2257 --- .../constants/action/DifferentialAction.php | 3 ++ .../DifferentialRevisionViewController.php | 19 ++++------- .../comment/DifferentialCommentEditor.php | 32 ++++++++++++++++--- .../differential/revision-comment.css | 4 +++ 4 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/applications/differential/constants/action/DifferentialAction.php b/src/applications/differential/constants/action/DifferentialAction.php index 5cc6c38944..b9948cca32 100644 --- a/src/applications/differential/constants/action/DifferentialAction.php +++ b/src/applications/differential/constants/action/DifferentialAction.php @@ -33,6 +33,7 @@ final class DifferentialAction { const ACTION_CREATE = 'create'; const ACTION_ADDREVIEWERS = 'add_reviewers'; const ACTION_ADDCCS = 'add_ccs'; + const ACTION_CLAIM = 'claim'; public static function getActionPastTenseVerb($action) { static $verbs = array( @@ -51,6 +52,7 @@ final class DifferentialAction { self::ACTION_CREATE => 'created', self::ACTION_ADDREVIEWERS => 'added reviewers to', self::ACTION_ADDCCS => 'added CCs to', + self::ACTION_CLAIM => 'commandeered', ); if (!empty($verbs[$action])) { @@ -73,6 +75,7 @@ final class DifferentialAction { self::ACTION_ADDREVIEWERS => 'Add Reviewers', self::ACTION_ADDCCS => 'Add CCs', self::ACTION_COMMIT => 'Mark Committed', + self::ACTION_CLAIM => 'Commandeer Revision', ); if (!empty($verbs[$action])) { diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 3578111a63..f7b3361f05 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -477,17 +477,16 @@ final class DifferentialRevisionViewController extends DifferentialController { $actions = array( DifferentialAction::ACTION_COMMENT => true, ); - $admin_actions = array(); $viewer = $this->getRequest()->getUser(); $viewer_phid = $viewer->getPHID(); - $viewer_is_admin = $viewer->getIsAdmin(); $viewer_is_owner = ($viewer_phid == $revision->getAuthorPHID()); $viewer_is_reviewer = in_array($viewer_phid, $revision->getReviewers()); $viewer_did_accept = ($viewer_phid === $revision->loadReviewedBy()); + $status = $revision->getStatus(); if ($viewer_is_owner) { - switch ($revision->getStatus()) { + switch ($status) { case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: $actions[DifferentialAction::ACTION_ABANDON] = true; $actions[DifferentialAction::ACTION_RETHINK] = true; @@ -509,20 +508,17 @@ final class DifferentialRevisionViewController extends DifferentialController { break; } } else { - switch ($revision->getStatus()) { + switch ($status) { case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - $admin_actions[DifferentialAction::ACTION_ABANDON] = $viewer_is_admin; $actions[DifferentialAction::ACTION_ACCEPT] = true; $actions[DifferentialAction::ACTION_REJECT] = true; $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; break; case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - $admin_actions[DifferentialAction::ACTION_ABANDON] = $viewer_is_admin; $actions[DifferentialAction::ACTION_ACCEPT] = true; $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer; break; case ArcanistDifferentialRevisionStatus::ACCEPTED: - $admin_actions[DifferentialAction::ACTION_ABANDON] = $viewer_is_admin; $actions[DifferentialAction::ACTION_REJECT] = true; $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer && !$viewer_did_accept; @@ -531,22 +527,19 @@ final class DifferentialRevisionViewController extends DifferentialController { case ArcanistDifferentialRevisionStatus::ABANDONED: break; } + if ($status != ArcanistDifferentialRevisionStatus::COMMITTED) { + $actions[DifferentialAction::ACTION_CLAIM] = true; + } } $actions[DifferentialAction::ACTION_ADDREVIEWERS] = true; $actions[DifferentialAction::ACTION_ADDCCS] = true; $actions = array_keys(array_filter($actions)); - $admin_actions = array_keys(array_filter($admin_actions)); $actions_dict = array(); - foreach ($actions as $action) { $actions_dict[$action] = DifferentialAction::getActionVerb($action); } - foreach ($admin_actions as $action) { - $actions_dict[$action] = - '(Admin) ' . DifferentialAction::getActionVerb($action); - } return $actions_dict; } diff --git a/src/applications/differential/editor/comment/DifferentialCommentEditor.php b/src/applications/differential/editor/comment/DifferentialCommentEditor.php index ad439ed1ff..a4cbbb77e4 100644 --- a/src/applications/differential/editor/comment/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/comment/DifferentialCommentEditor.php @@ -67,7 +67,7 @@ final class DifferentialCommentEditor { return $this->changedByCommit; } - public function setAddedReviewers($added_reviewers) { + public function setAddedReviewers(array $added_reviewers) { $this->addedReviewers = $added_reviewers; return $this; } @@ -101,7 +101,6 @@ final class DifferentialCommentEditor { $actor_phid = $this->actorPHID; $actor = id(new PhabricatorUser())->loadOneWhere('PHID = %s', $actor_phid); $actor_is_author = ($actor_phid == $revision->getAuthorPHID()); - $actor_is_admin = $actor->getIsAdmin(); $revision_status = $revision->getStatus(); $revision->loadRelationships(); @@ -147,7 +146,7 @@ final class DifferentialCommentEditor { break; case DifferentialAction::ACTION_ABANDON: - if (!($actor_is_author || $actor_is_admin)) { + if (!$actor_is_author) { throw new Exception('You can only abandon your own revisions.'); } @@ -363,7 +362,6 @@ final class DifferentialCommentEditor { if ($added_reviewers) { $key = DifferentialComment::METADATA_ADDED_REVIEWERS; $metadata[$key] = $added_reviewers; - } else { $user_tried_to_add = count($this->getAddedReviewers()); if ($user_tried_to_add == 0) { @@ -414,6 +412,32 @@ final class DifferentialCommentEditor { "authors, reviewers or CCs."); } } + break; + case DifferentialAction::ACTION_CLAIM: + if ($actor_is_author) { + throw new Exception("You can not commandeer your own revision."); + } + + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::COMMITTED: + throw new DifferentialActionHasNoEffectException( + "You can not commandeer this revision because it has ". + "already been committed."); + break; + } + + $this->setAddedReviewers(array($revision->getAuthorPHID())); + + // NOTE: Set the new author PHID before calling addReviewers(), since it + // doesn't permit the author to become a reviewer. + $revision->setAuthorPHID($actor_phid); + + $added_reviewers = $this->addReviewers(); + if ($added_reviewers) { + $key = DifferentialComment::METADATA_ADDED_REVIEWERS; + $metadata[$key] = $added_reviewers; + } + break; default: throw new Exception('Unsupported action.'); diff --git a/webroot/rsrc/css/application/differential/revision-comment.css b/webroot/rsrc/css/application/differential/revision-comment.css index 6fb291101b..a1693de389 100644 --- a/webroot/rsrc/css/application/differential/revision-comment.css +++ b/webroot/rsrc/css/application/differential/revision-comment.css @@ -104,3 +104,7 @@ .phabricator-transaction-view .differential-comment-action-request_review { border-color: #cc9966; } + +.phabricator-transaction-view .differential-comment-action-claim { + border-color: #000000; +}