From fe646ec328289afd6c52ef38659b92cc8d6bd211 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Sep 2017 13:25:55 -0700 Subject: [PATCH] Mark Owners package reviewers which own nothing in the current diff Summary: Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant. Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not. This is a rough cut to get the feature working, design could probably use some tweaking if it sticks. Test Plan: {F5204309} Reviewers: amckinley Reviewed By: amckinley Subscribers: jboning Differential Revision: https://secure.phabricator.com/D18663 --- .../controller/DifferentialController.php | 130 ++++++++++++------ .../DifferentialDiffViewController.php | 2 + .../DifferentialRevisionViewController.php | 10 ++ .../storage/DifferentialReviewer.php | 10 ++ .../view/DifferentialReviewersView.php | 6 + 5 files changed, 118 insertions(+), 40 deletions(-) diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php index 2258c4fdcb..fb8a6249a7 100644 --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -2,6 +2,10 @@ abstract class DifferentialController extends PhabricatorController { + private $packageChangesetMap; + private $pathPackageMap; + private $authorityPackages; + public function buildSideNavView($for_app = false) { $viewer = $this->getRequest()->getUser(); @@ -21,6 +25,88 @@ abstract class DifferentialController extends PhabricatorController { return $this->buildSideNavView(true)->getMenu(); } + protected function buildPackageMaps(array $changesets) { + assert_instances_of($changesets, 'DifferentialChangeset'); + + $this->packageChangesetMap = array(); + $this->pathPackageMap = array(); + $this->authorityPackages = array(); + + if (!$changesets) { + return; + } + + $viewer = $this->getViewer(); + + $have_owners = PhabricatorApplication::isClassInstalledForViewer( + 'PhabricatorOwnersApplication', + $viewer); + if (!$have_owners) { + return; + } + + $changeset = head($changesets); + $diff = $changeset->getDiff(); + $repository_phid = $diff->getRepositoryPHID(); + if (!$repository_phid) { + return; + } + + if ($viewer->getPHID()) { + $packages = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withAuthorityPHIDs(array($viewer->getPHID())) + ->execute(); + $this->authorityPackages = $packages; + } + + $paths = mpull($changesets, 'getOwnersFilename'); + + $control_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withControl($repository_phid, $paths); + $control_query->execute(); + + foreach ($changesets as $changeset) { + $changeset_path = $changeset->getOwnersFilename(); + + $packages = $control_query->getControllingPackagesForPath( + $repository_phid, + $changeset_path); + + $this->pathPackageMap[$changeset_path] = $packages; + foreach ($packages as $package) { + $this->packageChangesetMap[$package->getPHID()][] = $changeset; + } + } + } + + protected function getAuthorityPackages() { + if ($this->authorityPackages === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + return $this->authorityPackages; + } + + protected function getChangesetPackages(DifferentialChangeset $changeset) { + if ($this->pathPackageMap === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + + $path = $changeset->getOwnersFilename(); + return idx($this->pathPackageMap, $path, array()); + } + + protected function getPackageChangesets($package_phid) { + if ($this->packageChangesetMap === null) { + throw new PhutilInvalidStateException('buildPackageMaps'); + } + + return idx($this->packageChangesetMap, $package_phid, array()); + } + protected function buildTableOfContents( array $changesets, array $visible_changesets, @@ -29,40 +115,8 @@ abstract class DifferentialController extends PhabricatorController { $toc_view = id(new PHUIDiffTableOfContentsListView()) ->setViewer($viewer) - ->setBare(true); - - $have_owners = PhabricatorApplication::isClassInstalledForViewer( - 'PhabricatorOwnersApplication', - $viewer); - if ($have_owners) { - $repository_phid = null; - if ($changesets) { - $changeset = head($changesets); - $diff = $changeset->getDiff(); - $repository_phid = $diff->getRepositoryPHID(); - } - - if (!$repository_phid) { - $have_owners = false; - } else { - if ($viewer->getPHID()) { - $packages = id(new PhabricatorOwnersPackageQuery()) - ->setViewer($viewer) - ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) - ->withAuthorityPHIDs(array($viewer->getPHID())) - ->execute(); - $toc_view->setAuthorityPackages($packages); - } - - $paths = mpull($changesets, 'getOwnersFilename'); - - $control_query = id(new PhabricatorOwnersPackageQuery()) - ->setViewer($viewer) - ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) - ->withControl($repository_phid, $paths); - $control_query->execute(); - } - } + ->setBare(true) + ->setAuthorityPackages($this->getAuthorityPackages()); foreach ($changesets as $changeset_id => $changeset) { $is_visible = isset($visible_changesets[$changeset_id]); @@ -78,12 +132,8 @@ abstract class DifferentialController extends PhabricatorController { ->setCoverage(idx($coverage, $filename)) ->setCoverageID($coverage_id); - if ($have_owners) { - $packages = $control_query->getControllingPackagesForPath( - $repository_phid, - $changeset->getOwnersFilename()); - $item->setPackages($packages); - } + $packages = $this->getChangesetPackages($changeset); + $item->setPackages($packages); $toc_view->addItem($item); } diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index 7340687bc4..e56eb27d99 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -118,6 +118,8 @@ final class DifferentialDiffViewController extends DifferentialController { $changesets = $diff->loadChangesets(); $changesets = msort($changesets, 'getSortKey'); + $this->buildPackageMaps($changesets); + $table_of_contents = $this->buildTableOfContents( $changesets, $changesets, diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 06d8cf0a3f..e1da5ed32e 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -326,11 +326,21 @@ final class DifferentialRevisionViewController extends DifferentialController { $other_view = $this->renderOtherRevisions($other_revisions); } + $this->buildPackageMaps($changesets); + $toc_view = $this->buildTableOfContents( $changesets, $visible_changesets, $target->loadCoverageMap($viewer)); + // Attach changesets to each reviewer so we can show which Owners package + // reviewers own no files. + foreach ($revision->getReviewers() as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + $reviewer_changesets = $this->getPackageChangesets($reviewer_phid); + $reviewer->attachChangesets($reviewer_changesets); + } + $tab_group = id(new PHUITabGroupView()) ->addTab( id(new PHUITabView()) diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 3aa9bf6362..9df149e788 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -12,6 +12,7 @@ final class DifferentialReviewer protected $voidedPHID; private $authority = array(); + private $changesets = self::ATTACHABLE; protected function getConfiguration() { return array( @@ -54,6 +55,15 @@ final class DifferentialReviewer return $this->assertAttachedKey($this->authority, $cache_fragment); } + public function attachChangesets(array $changesets) { + $this->changesets = $changesets; + return $this; + } + + public function getChangesets() { + return $this->assertAttached($this->changesets); + } + public function isResigned() { $status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED; return ($this->getReviewerStatus() == $status_resigned); diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index 3fcb1c0ccf..33aad25289 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -150,6 +150,12 @@ final class DifferentialReviewersView extends AphrontView { $item->setIcon($icon, $color, $label); $item->setTarget($handle->renderHovercardLink()); + if ($reviewer->isPackage()) { + if (!$reviewer->getChangesets()) { + $item->setNote(pht('(Owns No Changed Paths)')); + } + } + $view->addItem($item); }