1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 05:50:55 +01:00

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
This commit is contained in:
epriestley 2017-09-29 13:25:55 -07:00
parent a39f5e1113
commit fe646ec328
5 changed files with 118 additions and 40 deletions

View file

@ -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);
}

View file

@ -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,

View file

@ -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())

View file

@ -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);

View file

@ -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);
}