1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-20 20:40:56 +01:00

Track "accepted" and "commented" in per-reviewer status

Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.

Test Plan:
  - Commented on a revision, got a comment icon.
  - Accepted a revision, got an accept icon.
  - Commented again, icon stayed as "accept".
  - Faked the "old diff" states.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7229
This commit is contained in:
epriestley 2013-10-04 18:58:35 -07:00
parent 4d8707df13
commit 370c7635a7
3 changed files with 67 additions and 4 deletions

View file

@ -3,6 +3,8 @@
final class DifferentialReviewerStatus { final class DifferentialReviewerStatus {
const STATUS_ADDED = 'added'; const STATUS_ADDED = 'added';
const STATUS_ACCEPTED = 'accepted';
const STATUS_REJECTED = 'rejected'; const STATUS_REJECTED = 'rejected';
const STATUS_COMMENTED = 'commented';
} }

View file

@ -93,7 +93,17 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
} }
public function save() { 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; $revision = $this->revision;
$action = $this->action; $action = $this->action;
$actor_phid = $actor->getPHID(); $actor_phid = $actor->getPHID();
@ -106,7 +116,6 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
'differential.allow-reopen'); 'differential.allow-reopen');
$revision_status = $revision->getStatus(); $revision_status = $revision->getStatus();
$revision->loadRelationships();
$reviewer_phids = $revision->getReviewers(); $reviewer_phids = $revision->getReviewers();
if ($reviewer_phids) { if ($reviewer_phids) {
$reviewer_phids = array_fuse($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 are submitting an empty comment with no action: ".
"you must act on the revision or post a comment."); "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; break;
case DifferentialAction::ACTION_RESIGN: case DifferentialAction::ACTION_RESIGN:
@ -209,7 +239,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
$revision, $revision,
$this->getActor(), $this->getActor(),
$actor_phid, $actor_phid,
DifferentialReviewerStatus::STATUS_ADDED); DifferentialReviewerStatus::STATUS_ACCEPTED);
break; break;
case DifferentialAction::ACTION_REQUEST: case DifferentialAction::ACTION_REQUEST:

View file

@ -27,6 +27,7 @@ final class DifferentialReviewersFieldSpecification
$revision = $this->getRevision(); $revision = $this->getRevision();
$reviewers = $revision->getReviewerStatus(); $reviewers = $revision->getReviewerStatus();
$diff = $revision->loadActiveDiff(); $diff = $revision->loadActiveDiff();
if ($diff) { if ($diff) {
$diff = $diff->getID(); $diff = $diff->getID();
@ -37,14 +38,31 @@ final class DifferentialReviewersFieldSpecification
foreach ($reviewers as $reviewer) { foreach ($reviewers as $reviewer) {
$phid = $reviewer->getReviewerPHID(); $phid = $reviewer->getReviewerPHID();
$is_current = (!$diff) ||
(!$reviewer->getDiffID()) ||
($diff == $reviewer->getDiffID());
$item = new PHUIStatusItemView(); $item = new PHUIStatusItemView();
switch ($reviewer->getStatus()) { switch ($reviewer->getStatus()) {
case DifferentialReviewerStatus::STATUS_ADDED: case DifferentialReviewerStatus::STATUS_ADDED:
$item->setIcon('open-dark', pht('Review Requested')); $item->setIcon('open-dark', pht('Review Requested'));
break; 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: case DifferentialReviewerStatus::STATUS_REJECTED:
if ($reviewer->getDiffID() == $diff) { if ($is_current) {
$item->setIcon( $item->setIcon(
'reject-red', 'reject-red',
pht('Requested Changes')); pht('Requested Changes'));
@ -54,6 +72,19 @@ final class DifferentialReviewersFieldSpecification
pht('Requested Changes to Prior Diff')); pht('Requested Changes to Prior Diff'));
} }
break; 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()); $item->setTarget($handles[$phid]->renderLink());