diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -494,7 +494,6 @@
'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php',
'DifferentialReviewerDatasource' => 'applications/differential/typeahead/DifferentialReviewerDatasource.php',
'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php',
- 'DifferentialReviewerProxy' => 'applications/differential/storage/DifferentialReviewerProxy.php',
'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php',
'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php',
'DifferentialReviewersAddBlockingSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php',
@@ -5254,7 +5253,6 @@
'DifferentialReviewer' => 'DifferentialDAO',
'DifferentialReviewerDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType',
- 'DifferentialReviewerProxy' => 'Phobject',
'DifferentialReviewerStatus' => 'Phobject',
'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction',
'DifferentialReviewersAddBlockingSelfHeraldAction' => 'DifferentialReviewersHeraldAction',
diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php
--- a/src/applications/differential/customfield/DifferentialCustomField.php
+++ b/src/applications/differential/customfield/DifferentialCustomField.php
@@ -70,6 +70,15 @@
return array();
}
+ protected function getActiveDiff() {
+ $object = $this->getObject();
+ try {
+ return $object->getActiveDiff();
+ } catch (Exception $ex) {
+ return null;
+ }
+ }
+
public function getRequiredHandlePHIDsForRevisionHeaderWarnings() {
return array();
}
diff --git a/src/applications/differential/customfield/DifferentialProjectReviewersField.php b/src/applications/differential/customfield/DifferentialProjectReviewersField.php
--- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php
+++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php
@@ -42,7 +42,10 @@
->setReviewers($reviewers)
->setHandles($handles);
- // TODO: Active diff stuff.
+ $diff = $this->getActiveDiff();
+ if ($diff) {
+ $view->setActiveDiff($diff);
+ }
return $view;
}
diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php
--- a/src/applications/differential/customfield/DifferentialReviewersField.php
+++ b/src/applications/differential/customfield/DifferentialReviewersField.php
@@ -43,7 +43,10 @@
->setReviewers($reviewers)
->setHandles($handles);
- // TODO: Active diff stuff.
+ $diff = $this->getActiveDiff();
+ if ($diff) {
+ $view->setActiveDiff($diff);
+ }
return $view;
}
diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php
--- a/src/applications/differential/query/DifferentialRevisionQuery.php
+++ b/src/applications/differential/query/DifferentialRevisionQuery.php
@@ -605,11 +605,11 @@
if ($this->reviewers) {
$joins[] = qsprintf(
$conn_r,
- 'JOIN %T e_reviewers ON e_reviewers.src = r.phid '.
- 'AND e_reviewers.type = %s '.
- 'AND e_reviewers.dst in (%Ls)',
- PhabricatorEdgeConfig::TABLE_NAME_EDGE,
- DifferentialRevisionHasReviewerEdgeType::EDGECONST,
+ 'JOIN %T reviewer ON reviewer.revisionPHID = r.phid
+ AND reviewer.reviewerStatus != %s
+ AND reviewer.reviewerPHID in (%Ls)',
+ id(new DifferentialReviewer())->getTableName(),
+ DifferentialReviewerStatus::STATUS_RESIGNED,
$this->reviewers);
}
@@ -972,21 +972,28 @@
}
private function loadReviewers(
- AphrontDatabaseConnection $conn_r,
+ AphrontDatabaseConnection $conn,
array $revisions) {
assert_instances_of($revisions, 'DifferentialRevision');
- $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
- $edges = id(new PhabricatorEdgeQuery())
- ->withSourcePHIDs(mpull($revisions, 'getPHID'))
- ->withEdgeTypes(array($edge_type))
- ->needEdgeData(true)
- ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST)
- ->execute();
+ $reviewer_table = new DifferentialReviewer();
+ $reviewer_rows = queryfx_all(
+ $conn,
+ 'SELECT * FROM %T WHERE revisionPHID IN (%Ls)
+ ORDER BY id ASC',
+ $reviewer_table->getTableName(),
+ mpull($revisions, 'getPHID'));
+ $reviewer_list = $reviewer_table->loadAllFromArray($reviewer_rows);
+ $reviewer_map = mgroup($reviewer_list, 'getRevisionPHID');
+
+ foreach ($reviewer_map as $key => $reviewers) {
+ $reviewer_map[$key] = mpull($reviewers, null, 'getReviewerPHID');
+ }
$viewer = $this->getViewer();
$viewer_phid = $viewer->getPHID();
+
$allow_key = 'differential.allow-self-accept';
$allow_self = PhabricatorEnv::getEnvConfig($allow_key);
@@ -994,18 +1001,13 @@
if ($this->needReviewerAuthority && $viewer_phid) {
$authority = $this->loadReviewerAuthority(
$revisions,
- $edges,
+ $reviewer_map,
$allow_self);
}
foreach ($revisions as $revision) {
- $revision_edges = $edges[$revision->getPHID()][$edge_type];
- $reviewers = array();
- foreach ($revision_edges as $reviewer_phid => $edge) {
- $reviewer = new DifferentialReviewerProxy(
- $reviewer_phid,
- $edge['data']);
-
+ $reviewers = idx($reviewer_map, $revision->getPHID(), array());
+ foreach ($reviewers as $reviewer_phid => $reviewer) {
if ($this->needReviewerAuthority) {
if (!$viewer_phid) {
// Logged-out users never have authority.
@@ -1031,7 +1033,7 @@
private function loadReviewerAuthority(
array $revisions,
- array $edges,
+ array $reviewers,
$allow_self) {
$revision_map = mpull($revisions, null, 'getPHID');
@@ -1045,9 +1047,9 @@
$package_type = PhabricatorOwnersPackagePHIDType::TYPECONST;
$edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
- foreach ($edges as $src => $types) {
+ foreach ($reviewers as $revision_phid => $reviewer_list) {
if (!$allow_self) {
- if ($revision_map[$src]->getAuthorPHID() == $viewer_phid) {
+ if ($revision_map[$revision_phid]->getAuthorPHID() == $viewer_phid) {
// If self-review isn't permitted, the user will never have
// authority over projects on revisions they authored because you
// can't accept your own revisions, so we don't need to load any
@@ -1055,14 +1057,14 @@
continue;
}
}
- $edge_data = idx($types, $edge_type, array());
- foreach ($edge_data as $dst => $data) {
- $phid_type = phid_get_type($dst);
+
+ foreach ($reviewer_list as $reviewer_phid => $reviewer) {
+ $phid_type = phid_get_type($reviewer_phid);
if ($phid_type == $project_type) {
- $project_phids[] = $dst;
+ $project_phids[] = $reviewer_phid;
}
if ($phid_type == $package_type) {
- $package_phids[] = $dst;
+ $package_phids[] = $reviewer_phid;
}
}
}
diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php
--- a/src/applications/differential/storage/DifferentialReviewer.php
+++ b/src/applications/differential/storage/DifferentialReviewer.php
@@ -6,10 +6,11 @@
protected $revisionPHID;
protected $reviewerPHID;
protected $reviewerStatus;
-
protected $lastActionDiffPHID;
protected $lastCommentDiffPHID;
+ private $authority = array();
+
protected function getConfiguration() {
return array(
self::CONFIG_COLUMN_SCHEMA => array(
@@ -26,4 +27,25 @@
) + parent::getConfiguration();
}
+ public function getStatus() {
+ // TODO: This is an older method for compatibility with some callers
+ // which have not yet been cleaned up.
+ return $this->getReviewerStatus();
+ }
+
+ public function isUser() {
+ $user_type = PhabricatorPeopleUserPHIDType::TYPECONST;
+ return (phid_get_type($this->getReviewerPHID()) == $user_type);
+ }
+
+ public function attachAuthority(PhabricatorUser $user, $has_authority) {
+ $this->authority[$user->getCacheFragment()] = $has_authority;
+ return $this;
+ }
+
+ public function hasAuthority(PhabricatorUser $viewer) {
+ $cache_fragment = $viewer->getCacheFragment();
+ return $this->assertAttachedKey($this->authority, $cache_fragment);
+ }
+
}
diff --git a/src/applications/differential/storage/DifferentialReviewerProxy.php b/src/applications/differential/storage/DifferentialReviewerProxy.php
deleted file mode 100644
--- a/src/applications/differential/storage/DifferentialReviewerProxy.php
+++ /dev/null
@@ -1,56 +0,0 @@
-<?php
-
-final class DifferentialReviewerProxy extends Phobject {
-
- private $reviewerPHID;
- private $status;
- private $diffID;
- private $authority = array();
-
- public function __construct($reviewer_phid, array $edge_data) {
- $this->reviewerPHID = $reviewer_phid;
- $this->status = idx($edge_data, 'status');
- $this->diffID = idx($edge_data, 'diff');
- }
-
- public function getReviewerPHID() {
- return $this->reviewerPHID;
- }
-
- public function getStatus() {
- return $this->status;
- }
-
- public function getDiffID() {
- return $this->diffID;
- }
-
- public function isUser() {
- $user_type = PhabricatorPeopleUserPHIDType::TYPECONST;
- return (phid_get_type($this->getReviewerPHID()) == $user_type);
- }
-
- public function attachAuthority(PhabricatorUser $user, $has_authority) {
- $this->authority[$user->getPHID()] = $has_authority;
- return $this;
- }
-
- public function hasAuthority(PhabricatorUser $viewer) {
- // It would be nice to use assertAttachedKey() here, but we don't extend
- // PhabricatorLiskDAO, and faking that seems sketchy.
-
- $viewer_phid = $viewer->getPHID();
- if (!array_key_exists($viewer_phid, $this->authority)) {
- throw new Exception(pht('You must %s first!', 'attachAuthority()'));
- }
- return $this->authority[$viewer_phid];
- }
-
- public function getEdgeData() {
- return array(
- 'status' => $this->status,
- 'diffID' => $this->diffID,
- );
- }
-
-}
diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -296,15 +296,6 @@
return idx($this->relationships, $relation, array());
}
- public function getPrimaryReviewer() {
- $reviewers = $this->getReviewers();
- $last = $this->lastReviewerPHID;
- if (!$last || !in_array($last, $reviewers)) {
- return head($this->getReviewers());
- }
- return $last;
- }
-
public function getHashes() {
return $this->assertAttached($this->hashes);
}
@@ -406,8 +397,7 @@
}
public function attachReviewerStatus(array $reviewers) {
- assert_instances_of($reviewers, 'DifferentialReviewerProxy');
-
+ assert_instances_of($reviewers, 'DifferentialReviewer');
$this->reviewerStatus = $reviewers;
return $this;
}
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
@@ -212,13 +212,6 @@
$tags[] = self::MAILTAG_UPDATED;
}
break;
- case PhabricatorTransactions::TYPE_EDGE:
- switch ($this->getMetadataValue('edge:type')) {
- case DifferentialRevisionHasReviewerEdgeType::EDGECONST:
- $tags[] = self::MAILTAG_REVIEWERS;
- break;
- }
- break;
case PhabricatorTransactions::TYPE_COMMENT:
case self::TYPE_INLINE:
$tags[] = self::MAILTAG_COMMENT;
@@ -598,14 +591,6 @@
public function getNoEffectDescription() {
switch ($this->getTransactionType()) {
- case PhabricatorTransactions::TYPE_EDGE:
- switch ($this->getMetadataValue('edge:type')) {
- case DifferentialRevisionHasReviewerEdgeType::EDGECONST:
- return pht(
- 'The reviewers you are trying to add are already reviewing '.
- 'this revision.');
- }
- break;
case self::TYPE_ACTION:
switch ($this->getNewValue()) {
case DifferentialAction::ACTION_CLOSE:
@@ -624,18 +609,10 @@
return pht('This revision already requires changes.');
case DifferentialAction::ACTION_REQUEST:
return pht('Review is already requested for this revision.');
- case DifferentialAction::ACTION_RESIGN:
- return pht(
- 'You can not resign from this revision because you are not '.
- 'a reviewer.');
case DifferentialAction::ACTION_CLAIM:
return pht(
'You can not commandeer this revision because you already own '.
'it.');
- case DifferentialAction::ACTION_ACCEPT:
- return pht('You have already accepted this revision.');
- case DifferentialAction::ACTION_REJECT:
- return pht('You have already requested changes to this revision.');
}
break;
}
diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php
--- a/src/applications/differential/view/DifferentialReviewersView.php
+++ b/src/applications/differential/view/DifferentialReviewersView.php
@@ -7,7 +7,7 @@
private $diff;
public function setReviewers(array $reviewers) {
- assert_instances_of($reviewers, 'DifferentialReviewerProxy');
+ assert_instances_of($reviewers, 'DifferentialReviewer');
$this->reviewers = $reviewers;
return $this;
}
@@ -31,47 +31,54 @@
$phid = $reviewer->getReviewerPHID();
$handle = $this->handles[$phid];
- // If we're missing either the diff or action information for the
- // reviewer, render information as current.
- $is_current = (!$this->diff) ||
- (!$reviewer->getDiffID()) ||
- ($this->diff->getID() == $reviewer->getDiffID());
+ $action_phid = $reviewer->getLastActionDiffPHID();
+ $is_current_action = $this->isCurrent($action_phid);
+
+ $comment_phid = $reviewer->getLastCommentDiffPHID();
+ $is_current_comment = $this->isCurrent($comment_phid);
$item = new PHUIStatusItemView();
$item->setHighlighted($reviewer->hasAuthority($viewer));
- switch ($reviewer->getStatus()) {
+ switch ($reviewer->getReviewerStatus()) {
case DifferentialReviewerStatus::STATUS_ADDED:
- $item->setIcon(
- PHUIStatusItemView::ICON_OPEN,
- 'bluegrey',
- pht('Review Requested'));
+ if ($comment_phid) {
+ if ($is_current_comment) {
+ $item->setIcon(
+ 'fa-comment',
+ 'blue',
+ pht('Commented'));
+ } else {
+ $item->setIcon(
+ 'fa-comment-o',
+ 'bluegrey',
+ pht('Commented Previously'));
+ }
+ } else {
+ $item->setIcon(
+ PHUIStatusItemView::ICON_OPEN,
+ 'bluegrey',
+ pht('Review Requested'));
+ }
break;
case DifferentialReviewerStatus::STATUS_ACCEPTED:
- if ($is_current) {
+ if ($is_current_action) {
$item->setIcon(
PHUIStatusItemView::ICON_ACCEPT,
'green',
pht('Accepted'));
} else {
$item->setIcon(
- PHUIStatusItemView::ICON_ACCEPT,
+ 'fa-check-circle-o',
'bluegrey',
pht('Accepted Prior Diff'));
}
break;
- case DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER:
- $item->setIcon(
- 'fa-check-circle-o',
- 'bluegrey',
- pht('Accepted Prior Diff'));
- break;
-
case DifferentialReviewerStatus::STATUS_REJECTED:
- if ($is_current) {
+ if ($is_current_action) {
$item->setIcon(
PHUIStatusItemView::ICON_REJECT,
'red',
@@ -84,27 +91,6 @@
}
break;
- case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
- $item->setIcon(
- 'fa-times-circle-o',
- 'bluegrey',
- pht('Rejected Prior Diff'));
- break;
-
- case DifferentialReviewerStatus::STATUS_COMMENTED:
- if ($is_current) {
- $item->setIcon(
- 'fa-question-circle',
- 'blue',
- pht('Commented'));
- } else {
- $item->setIcon(
- 'fa-question-circle-o',
- 'bluegrey',
- pht('Commented Previously'));
- }
- break;
-
case DifferentialReviewerStatus::STATUS_BLOCKING:
$item->setIcon(
PHUIStatusItemView::ICON_MINUS,
@@ -116,7 +102,7 @@
$item->setIcon(
PHUIStatusItemView::ICON_QUESTION,
'bluegrey',
- pht('%s?', $reviewer->getStatus()));
+ pht('%s?', $reviewer->getReviewerStatus()));
break;
}
@@ -128,4 +114,26 @@
return $view;
}
+ private function isCurrent($action_phid) {
+ if (!$this->diff) {
+ echo "A\n";
+ return true;
+ }
+
+ if (!$action_phid) {
+ return true;
+ }
+
+ $diff_phid = $this->diff->getPHID();
+ if (!$diff_phid) {
+ return true;
+ }
+
+ if ($diff_phid == $action_phid) {
+ return true;
+ }
+
+ return false;
+ }
+
}