From 6cfeb9e5401436a2e3ffd2ccfcd63a1e256581a9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Aug 2015 10:09:17 -0700 Subject: [PATCH] Further modernize OwnersPackageQuery Summary: Ref T8320. - Add needOwners(). - Split withOwnerPHIDs() [exact owners] and withAuthorityPHIDs() [indirect authority] apart. - Restore searching by path. Test Plan: Browsed pacakges, edited packages, edited paths. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8320 Differential Revision: https://secure.phabricator.com/D13922 --- .../editor/PhabricatorAuditCommentEditor.php | 2 +- .../conduit/OwnersQueryConduitAPIMethod.php | 2 +- .../PhabricatorOwnersDetailController.php | 4 +- .../PhabricatorOwnersEditController.php | 4 +- .../query/PhabricatorOwnersPackageQuery.php | 127 ++++++++++++++---- .../PhabricatorOwnersPackageSearchEngine.php | 24 ++-- .../storage/PhabricatorOwnersPackage.php | 15 ++- 7 files changed, 138 insertions(+), 40 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 9f4bd42f29..03476f70a7 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -20,7 +20,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $owned_packages = id(new PhabricatorOwnersPackageQuery()) ->setViewer($user) - ->withOwnerPHIDs(array($user->getPHID())) + ->withAuthorityPHIDs(array($user->getPHID())) ->execute(); foreach ($owned_packages as $package) { $phids[$package->getPHID()] = true; diff --git a/src/applications/owners/conduit/OwnersQueryConduitAPIMethod.php b/src/applications/owners/conduit/OwnersQueryConduitAPIMethod.php index 6ce996a002..a3b7b52cab 100644 --- a/src/applications/owners/conduit/OwnersQueryConduitAPIMethod.php +++ b/src/applications/owners/conduit/OwnersQueryConduitAPIMethod.php @@ -141,7 +141,7 @@ final class OwnersQueryConduitAPIMethod extends OwnersConduitAPIMethod { $query = id(new PhabricatorOwnersPackageQuery()) ->setViewer($request->getUser()); - $query->withOwnerPHIDs(array($request->getValue('userAffiliated'))); + $query->withAuthorityPHIDs(array($request->getValue('userAffiliated'))); $packages = $query->execute(); } else if ($is_owner_query) { diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 168d7cd07e..008cb715e5 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -14,6 +14,7 @@ final class PhabricatorOwnersDetailController ->setViewer($viewer) ->withIDs(array($request->getURIData('id'))) ->needPaths(true) + ->needOwners(true) ->executeOne(); if (!$package) { return new Aphront404Response(); @@ -150,8 +151,7 @@ final class PhabricatorOwnersDetailController $view = id(new PHUIPropertyListView()) ->setUser($viewer); - // TODO: needOwners() this on the Query. - $owners = $package->loadOwners(); + $owners = $package->getOwners(); if ($owners) { $owner_list = $viewer->renderHandleList(mpull($owners, 'getUserPHID')); } else { diff --git a/src/applications/owners/controller/PhabricatorOwnersEditController.php b/src/applications/owners/controller/PhabricatorOwnersEditController.php index 618f99456d..f601cd3115 100644 --- a/src/applications/owners/controller/PhabricatorOwnersEditController.php +++ b/src/applications/owners/controller/PhabricatorOwnersEditController.php @@ -17,6 +17,7 @@ final class PhabricatorOwnersEditController // TODO: Support this capability. // PhabricatorPolicyCapability::CAN_EDIT, )) + ->needOwners(true) ->executeOne(); if (!$package) { return new Aphront404Response(); @@ -30,8 +31,7 @@ final class PhabricatorOwnersEditController $e_name = true; $v_name = $package->getName(); - // TODO: Pull these off needOwners() on the Query. - $v_owners = mpull($package->loadOwners(), 'getUserPHID'); + $v_owners = mpull($package->getOwners(), 'getUserPHID'); $v_auditing = $package->getAuditingEnabled(); $v_description = $package->getDescription(); diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php index f2691ea3be..74f77f79b7 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -6,21 +6,37 @@ final class PhabricatorOwnersPackageQuery private $ids; private $phids; private $ownerPHIDs; + private $authorityPHIDs; private $repositoryPHIDs; + private $paths; private $namePrefix; - private $needPaths; private $controlMap = array(); private $controlResults; + private $needPaths; + private $needOwners; + + /** - * Owners are direct owners, and members of owning projects. + * Query owner PHIDs exactly. This does not expand authorities, so a user + * PHID will not match projects the user is a member of. */ public function withOwnerPHIDs(array $phids) { $this->ownerPHIDs = $phids; return $this; } + /** + * Query owner authority. This will expand authorities, so a user PHID will + * match both packages they own directly and packages owned by a project they + * are a member of. + */ + public function withAuthorityPHIDs(array $phids) { + $this->authorityPHIDs = $phids; + return $this; + } + public function withPHIDs(array $phids) { $this->phids = $phids; return $this; @@ -36,6 +52,11 @@ final class PhabricatorOwnersPackageQuery return $this; } + public function withPaths(array $paths) { + $this->paths = $paths; + return $this; + } + public function withControl($repository_phid, array $paths) { if (empty($this->controlMap[$repository_phid])) { $this->controlMap[$repository_phid] = array(); @@ -62,6 +83,11 @@ final class PhabricatorOwnersPackageQuery return $this; } + public function needOwners($need_owners) { + $this->needOwners = $need_owners; + return $this; + } + public function newResultObject() { return new PhabricatorOwnersPackage(); } @@ -88,6 +114,19 @@ final class PhabricatorOwnersPackageQuery } } + if ($this->needOwners) { + $package_ids = mpull($packages, 'getID'); + + $owners = id(new PhabricatorOwnersOwner())->loadAllWhere( + 'packageID IN (%Ld)', + $package_ids); + $owners = mgroup($owners, 'getPackageID'); + + foreach ($packages as $package) { + $package->attachOwners(idx($owners, $package->getID(), array())); + } + } + if ($this->controlMap) { $this->controlResults += mpull($packages, null, 'getID'); } @@ -98,14 +137,14 @@ final class PhabricatorOwnersPackageQuery protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { $joins = parent::buildJoinClauseParts($conn); - if ($this->ownerPHIDs !== null) { + if ($this->shouldJoinOwnersTable()) { $joins[] = qsprintf( $conn, 'JOIN %T o ON o.packageID = p.id', id(new PhabricatorOwnersOwner())->getTableName()); } - if ($this->shouldJoinOwnersPathTable()) { + if ($this->shouldJoinPathTable()) { $joins[] = qsprintf( $conn, 'JOIN %T rpath ON rpath.packageID = p.id', @@ -139,21 +178,26 @@ final class PhabricatorOwnersPackageQuery $this->repositoryPHIDs); } - if ($this->ownerPHIDs !== null) { - $base_phids = $this->ownerPHIDs; - - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($this->getViewer()) - ->withMemberPHIDs($base_phids) - ->execute(); - $project_phids = mpull($projects, 'getPHID'); - - $all_phids = array_merge($base_phids, $project_phids); - + if ($this->authorityPHIDs !== null) { + $authority_phids = $this->expandAuthority($this->authorityPHIDs); $where[] = qsprintf( $conn, 'o.userPHID IN (%Ls)', - $all_phids); + $authority_phids); + } + + if ($this->ownerPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'o.userPHID IN (%Ls)', + $this->ownerPHIDs); + } + + if ($this->paths !== null) { + $where[] = qsprintf( + $conn, + 'rpath.path IN (%Ls)', + $this->getFragmentsForPaths($this->paths)); } if (strlen($this->namePrefix)) { @@ -168,12 +212,7 @@ final class PhabricatorOwnersPackageQuery if ($this->controlMap) { $clauses = array(); foreach ($this->controlMap as $repository_phid => $paths) { - $fragments = array(); - foreach ($paths as $path) { - foreach (PhabricatorOwnersPackage::splitPath($path) as $fragment) { - $fragments[$fragment] = $fragment; - } - } + $fragments = $this->getFragmentsForPaths($paths); $clauses[] = qsprintf( $conn, @@ -188,11 +227,11 @@ final class PhabricatorOwnersPackageQuery } protected function shouldGroupQueryResultRows() { - if ($this->shouldJoinOwnersPathTable()) { + if ($this->shouldJoinOwnersTable()) { return true; } - if ($this->ownerPHIDs) { + if ($this->shouldJoinPathTable()) { return true; } @@ -236,11 +275,27 @@ final class PhabricatorOwnersPackageQuery return 'p'; } - private function shouldJoinOwnersPathTable() { + private function shouldJoinOwnersTable() { + if ($this->ownerPHIDs !== null) { + return true; + } + + if ($this->authorityPHIDs !== null) { + return true; + } + + return false; + } + + private function shouldJoinPathTable() { if ($this->repositoryPHIDs !== null) { return true; } + if ($this->paths !== null) { + return true; + } + if ($this->controlMap) { return true; } @@ -248,6 +303,28 @@ final class PhabricatorOwnersPackageQuery return false; } + private function expandAuthority(array $phids) { + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->withMemberPHIDs($phids) + ->execute(); + $project_phids = mpull($projects, 'getPHID'); + + return array_fuse($phids) + array_fuse($project_phids); + } + + private function getFragmentsForPaths(array $paths) { + $fragments = array(); + + foreach ($paths as $path) { + foreach (PhabricatorOwnersPackage::splitPath($path) as $fragment) { + $fragments[$fragment] = $fragment; + } + } + + return $fragments; + } + /* -( Path Control )------------------------------------------------------- */ diff --git a/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php b/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php index fc2f8f620f..a26a9b3c56 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php @@ -18,29 +18,37 @@ final class PhabricatorOwnersPackageSearchEngine protected function buildCustomSearchFields() { return array( id(new PhabricatorSearchDatasourceField()) - ->setLabel(pht('Owners')) - ->setKey('ownerPHIDs') - ->setAliases(array('owner', 'owners')) + ->setLabel(pht('Authority')) + ->setKey('authorityPHIDs') + ->setAliases(array('authority', 'authorities')) ->setDatasource(new PhabricatorProjectOrUserDatasource()), id(new PhabricatorSearchDatasourceField()) ->setLabel(pht('Repositories')) ->setKey('repositoryPHIDs') ->setAliases(array('repository', 'repositories')) ->setDatasource(new DiffusionRepositoryDatasource()), + id(new PhabricatorSearchStringListField()) + ->setLabel(pht('Paths')) + ->setKey('paths') + ->setAliases(array('path')), ); } protected function buildQueryFromParameters(array $map) { $query = $this->newQuery(); - if ($map['ownerPHIDs']) { - $query->withOwnerPHIDs($map['ownerPHIDs']); + if ($map['authorityPHIDs']) { + $query->withAuthorityPHIDs($map['authorityPHIDs']); } if ($map['repositoryPHIDs']) { $query->withRepositoryPHIDs($map['repositoryPHIDs']); } + if ($map['paths']) { + $query->withPaths($map['paths']); + } + return $query; } @@ -52,7 +60,7 @@ final class PhabricatorOwnersPackageSearchEngine $names = array(); if ($this->requireViewer()->isLoggedIn()) { - $names['owned'] = pht('Owned'); + $names['authority'] = pht('Owned'); } $names += array( @@ -69,9 +77,9 @@ final class PhabricatorOwnersPackageSearchEngine switch ($query_key) { case 'all': return $query; - case 'owned': + case 'authority': return $query->setParameter( - 'ownerPHIDs', + 'authorityPHIDs', array($this->requireViewer()->getPHID())); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index bfba5856ca..7d0d8e65a4 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -14,10 +14,13 @@ final class PhabricatorOwnersPackage protected $mailKey; private $paths = self::ATTACHABLE; + private $owners = self::ATTACHABLE; public static function initializeNewPackage(PhabricatorUser $actor) { return id(new PhabricatorOwnersPackage()) - ->setAuditingEnabled(0); + ->setAuditingEnabled(0) + ->attachPaths(array()) + ->attachOwners(array()); } public function getCapabilities() { @@ -249,6 +252,16 @@ final class PhabricatorOwnersPackage return $this->assertAttached($this->paths); } + public function attachOwners(array $owners) { + assert_instances_of($owners, 'PhabricatorOwnersOwner'); + $this->owners = $owners; + return $this; + } + + public function getOwners() { + return $this->assertAttached($this->owners); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */