diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 59c645c746..0d0a135e76 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -30,6 +30,7 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setViewer($request->getUser()) ->needRelationships(true) ->needReviewerStatus(true) + ->needReviewerAuthority(true) ->executeOne(); if (!$revision) { diff --git a/src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php b/src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php index 53aa040560..8cfbdf8a5f 100644 --- a/src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialProjectReviewersFieldSpecification.php @@ -27,24 +27,10 @@ final class DifferentialProjectReviewersFieldSpecification return null; } - $highlight = array(); - if ($this->getUser()->getPHID() != $this->getRevision()->getAuthorPHID()) { - // Determine which of these projects the viewer is a member of, so we can - // highlight them. (If the viewer is the author, skip this since they - // can't review.) - $phids = mpull($reviewers, 'getReviewerPHID'); - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($this->getUser()) - ->withPHIDs($phids) - ->withMemberPHIDs(array($this->getUser()->getPHID())) - ->execute(); - $highlight = mpull($projects, 'getPHID'); - } - $view = id(new DifferentialReviewersView()) + ->setUser($this->getUser()) ->setReviewers($reviewers) - ->setHandles($this->getLoadedHandles()) - ->setHighlightPHIDs($highlight); + ->setHandles($this->getLoadedHandles()); $diff = $this->getRevision()->loadActiveDiff(); if ($diff) { diff --git a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php index f7fcf2056c..b6d00ce7a8 100644 --- a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php @@ -32,8 +32,8 @@ final class DifferentialReviewersFieldSpecification } $view = id(new DifferentialReviewersView()) + ->setUser($this->getUser()) ->setReviewers($reviewers) - ->setHighlightPHIDs(array($this->getUser()->getPHID())) ->setHandles($this->getLoadedHandles()); $diff = $this->getRevision()->loadActiveDiff(); diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 8dfcd60648..90284fefb9 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -59,6 +59,7 @@ final class DifferentialRevisionQuery private $needCommitPHIDs = false; private $needHashes = false; private $needReviewerStatus = false; + private $needReviewerAuthority; private $buildingGlobalOrder; @@ -347,6 +348,21 @@ final class DifferentialRevisionQuery } + /** + * Request information about the viewer's authority to act on behalf of each + * reviewer. In particular, they have authority to act on behalf of projects + * they are a member of. + * + * @param bool True to load and attach authority. + * @return this + * @task config + */ + public function needReviewerAuthority($need_reviewer_authority) { + $this->needReviewerAuthority = $need_reviewer_authority; + return $this; + } + + /* -( Query Execution )---------------------------------------------------- */ @@ -450,7 +466,7 @@ final class DifferentialRevisionQuery $this->loadHashes($conn_r, $revisions); } - if ($this->needReviewerStatus) { + if ($this->needReviewerStatus || $this->needReviewerAuthority) { $this->loadReviewers($conn_r, $revisions); } @@ -1040,15 +1056,42 @@ final class DifferentialRevisionQuery ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST) ->execute(); + $viewer = $this->getViewer(); + $viewer_phid = $viewer->getPHID(); + + // Figure out which of these reviewers the viewer has authority to act as. + if ($this->needReviewerAuthority && $viewer_phid) { + $allow_key = 'differential.allow-self-accept'; + $allow_self = PhabricatorEnv::getEnvConfig($allow_key); + $authority = $this->loadReviewerAuthority( + $revisions, + $edges, + $allow_self); + } + foreach ($revisions as $revision) { $revision_edges = $edges[$revision->getPHID()][$edge_type]; $reviewers = array(); - foreach ($revision_edges as $user_phid => $edge) { - $data = $edge['data']; - $reviewers[] = new DifferentialReviewer( - $user_phid, - idx($data, 'status'), - idx($data, 'diff')); + foreach ($revision_edges as $reviewer_phid => $edge) { + $reviewer = new DifferentialReviewer($reviewer_phid, $edge['data']); + + if ($this->needReviewerAuthority) { + if (!$viewer_phid) { + // Logged-out users never have authority. + $has_authority = false; + } else if ((!$allow_self) && + ($revision->getAuthorPHID() == $viewer_phid)) { + // The author can never have authority unless we allow self-accept. + $has_authority = false; + } else { + // Otherwise, look up whether th viewer has authority. + $has_authority = isset($authority[$reviewer_phid]); + } + + $reviewer->attachAuthority($viewer, $has_authority); + } + + $reviewers[$reviewer_phid] = $reviewer; } $revision->attachReviewerStatus($reviewers); @@ -1091,5 +1134,55 @@ final class DifferentialRevisionQuery return array($blocking, $active, $waiting); } + private function loadReviewerAuthority( + array $revisions, + array $edges, + $allow_self) { + + $revision_map = mpull($revisions, null, 'getPHID'); + $viewer_phid = $this->getViewer()->getPHID(); + + // Find all the project reviewers which the user may have authority over. + $project_phids = array(); + $project_type = PhabricatorProjectPHIDTypeProject::TYPECONST; + $edge_type = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER; + foreach ($edges as $src => $types) { + if (!$allow_self) { + if ($revision_map[$src]->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 + // data about these reviewers. + continue; + } + } + $edge_data = idx($types, $edge_type, array()); + foreach ($edge_data as $dst => $data) { + if (phid_get_type($dst) == $project_type) { + $project_phids[] = $dst; + } + } + } + + // Now, figure out which of these projects the viewer is actually a + // member of. + $project_authority = array(); + if ($project_phids) { + $project_authority = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($project_phids) + ->withMemberPHIDs(array($viewer_phid)) + ->execute(); + $project_authority = mpull($project_authority, 'getPHID'); + } + + // Finally, the viewer has authority over themselves. + return array( + $viewer_phid => true, + ) + array_fuse($project_authority); + } + + + } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index e6597ccaf7..62ae27d8f8 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -2,14 +2,15 @@ final class DifferentialReviewer { - protected $reviewerPHID; - protected $status; - protected $diffID; + private $reviewerPHID; + private $status; + private $diffID; + private $authority = array(); - public function __construct($reviewer_phid, $status, $diff_id = null) { + public function __construct($reviewer_phid, array $edge_data) { $this->reviewerPHID = $reviewer_phid; - $this->status = $status; - $this->diffID = $diff_id; + $this->status = idx($edge_data, 'status'); + $this->diffID = idx($edge_data, 'diff'); } public function getReviewerPHID() { @@ -29,4 +30,20 @@ final class DifferentialReviewer { 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("You must attachAuthority() first!"); + } + return $this->authority[$viewer_phid]; + } + } diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 851af0afaa..09179e57bf 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -5,7 +5,6 @@ final class DifferentialReviewersView extends AphrontView { private $reviewers; private $handles; private $diff; - private $highlightPHIDs = array(); public function setReviewers(array $reviewers) { assert_instances_of($reviewers, 'DifferentialReviewer'); @@ -24,16 +23,10 @@ final class DifferentialReviewersView extends AphrontView { return $this; } - public function setHighlightPHIDs(array $phids) { - $this->highlightPHIDs = $phids; - return $this; - } - public function render() { + $viewer = $this->getUser(); + $view = new PHUIStatusListView(); - - $highlighted = array_fuse($this->highlightPHIDs); - foreach ($this->reviewers as $reviewer) { $phid = $reviewer->getReviewerPHID(); $handle = $this->handles[$phid]; @@ -46,9 +39,7 @@ final class DifferentialReviewersView extends AphrontView { $item = new PHUIStatusItemView(); - if (isset($highlighted[$phid])) { - $item->setHighlighted(true); - } + $item->setHighlighted($reviewer->hasAuthority($viewer)); switch ($reviewer->getStatus()) { case DifferentialReviewerStatus::STATUS_ADDED: