From 3d3d3b6d80054cf2f0a1420d8d4031dd9575a30b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 6 Oct 2013 17:08:14 -0700 Subject: [PATCH] Move determination of reviewer authority into DifferentialRevisionQuery Summary: Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons: - It puts queries very close to the display layer. - We have to query for each revision if we want to figure out authority for several. - We need to figure it out in several places, so we'll end up with copies of this logic. - The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration). - We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place. Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects. Test Plan: - Looked at some revisions, verified the correct lines were highlighted. - Looked at a revision I created and verified that projects I was a member of were not highlighted. - With self-accept enabled, these //are// highlighted. - Looked at a revision I did not create and verified that projects I was a member of were highlighted. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1279 Differential Revision: https://secure.phabricator.com/D7241 --- .../DifferentialRevisionViewController.php | 1 + ...tialProjectReviewersFieldSpecification.php | 18 +-- ...ifferentialReviewersFieldSpecification.php | 2 +- .../query/DifferentialRevisionQuery.php | 107 ++++++++++++++++-- .../storage/DifferentialReviewer.php | 29 ++++- .../view/DifferentialReviewersView.php | 15 +-- 6 files changed, 130 insertions(+), 42 deletions(-) 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: