diff --git a/src/applications/differential/constants/DifferentialReviewerStatus.php b/src/applications/differential/constants/DifferentialReviewerStatus.php index 23fc393c93..02b716f28a 100644 --- a/src/applications/differential/constants/DifferentialReviewerStatus.php +++ b/src/applications/differential/constants/DifferentialReviewerStatus.php @@ -3,6 +3,8 @@ final class DifferentialReviewerStatus { const STATUS_ADDED = 'added'; + const STATUS_ACCEPTED = 'accepted'; const STATUS_REJECTED = 'rejected'; + const STATUS_COMMENTED = 'commented'; } diff --git a/src/applications/differential/editor/DifferentialCommentEditor.php b/src/applications/differential/editor/DifferentialCommentEditor.php index 126aae1852..22a86c0ebf 100644 --- a/src/applications/differential/editor/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/DifferentialCommentEditor.php @@ -93,7 +93,17 @@ final class DifferentialCommentEditor extends PhabricatorEditor { } public function save() { - $actor = $this->requireActor(); + $actor = $this->requireActor(); + + // Reload the revision to pick up reviewer status, until we can lift this + // out of here. + $this->revision = id(new DifferentialRevisionQuery()) + ->setViewer($actor) + ->withIDs(array($this->revision->getID())) + ->needRelationships(true) + ->needReviewerStatus(true) + ->executeOne(); + $revision = $this->revision; $action = $this->action; $actor_phid = $actor->getPHID(); @@ -106,7 +116,6 @@ final class DifferentialCommentEditor extends PhabricatorEditor { 'differential.allow-reopen'); $revision_status = $revision->getStatus(); - $revision->loadRelationships(); $reviewer_phids = $revision->getReviewers(); if ($reviewer_phids) { $reviewer_phids = array_fuse($reviewer_phids); @@ -128,6 +137,27 @@ final class DifferentialCommentEditor extends PhabricatorEditor { "You are submitting an empty comment with no action: ". "you must act on the revision or post a comment."); } + + // If the actor is a reviewer, and their status is "added" (that is, + // they haven't accepted or requested changes to the revision), + // upgrade their status to "commented". If they have a stronger status + // already, don't overwrite it. + if (isset($reviewer_phids[$actor_phid])) { + $status_added = DifferentialReviewerStatus::STATUS_ADDED; + $reviewer_status = $revision->getReviewerStatus(); + foreach ($reviewer_status as $reviewer) { + if ($reviewer->getReviewerPHID() == $actor_phid) { + if ($reviewer->getStatus() == $status_added) { + DifferentialRevisionEditor::updateReviewerStatus( + $revision, + $this->getActor(), + $actor_phid, + DifferentialReviewerStatus::STATUS_COMMENTED); + } + } + } + } + break; case DifferentialAction::ACTION_RESIGN: @@ -209,7 +239,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $revision, $this->getActor(), $actor_phid, - DifferentialReviewerStatus::STATUS_ADDED); + DifferentialReviewerStatus::STATUS_ACCEPTED); break; case DifferentialAction::ACTION_REQUEST: diff --git a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php index ca7bde3e87..24860ea951 100644 --- a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php @@ -27,6 +27,7 @@ final class DifferentialReviewersFieldSpecification $revision = $this->getRevision(); $reviewers = $revision->getReviewerStatus(); + $diff = $revision->loadActiveDiff(); if ($diff) { $diff = $diff->getID(); @@ -37,14 +38,31 @@ final class DifferentialReviewersFieldSpecification foreach ($reviewers as $reviewer) { $phid = $reviewer->getReviewerPHID(); + $is_current = (!$diff) || + (!$reviewer->getDiffID()) || + ($diff == $reviewer->getDiffID()); + $item = new PHUIStatusItemView(); switch ($reviewer->getStatus()) { case DifferentialReviewerStatus::STATUS_ADDED: $item->setIcon('open-dark', pht('Review Requested')); break; + + case DifferentialReviewerStatus::STATUS_ACCEPTED: + if ($is_current) { + $item->setIcon( + 'accept-green', + pht('Accept')); + } else { + $item->setIcon( + 'accept-dark', + pht('Accepted Prior Diff')); + } + break; + case DifferentialReviewerStatus::STATUS_REJECTED: - if ($reviewer->getDiffID() == $diff) { + if ($is_current) { $item->setIcon( 'reject-red', pht('Requested Changes')); @@ -54,6 +72,19 @@ final class DifferentialReviewersFieldSpecification pht('Requested Changes to Prior Diff')); } break; + + case DifferentialReviewerStatus::STATUS_COMMENTED: + if ($is_current) { + $item->setIcon( + 'info-blue', + pht('Commented')); + } else { + $item->setIcon( + 'info-dark', + pht('Commented Previously')); + } + break; + } $item->setTarget($handles[$phid]->renderLink());