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

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;

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) {

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->getOwners();
-    $owners = $package->loadOwners();
     if ($owners) {
       $owner_list = $viewer->renderHandleList(mpull($owners, 'getUserPHID'));
     } else {

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->getOwners(), 'getUserPHID');
-    $v_owners = mpull($package->loadOwners(), 'getUserPHID');
     $v_auditing = $package->getAuditingEnabled();
     $v_description = $package->getDescription();
 

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) {
+    if ($this->authorityPHIDs !== null) {
-      $base_phids = $this->ownerPHIDs;
+      $authority_phids = $this->expandAuthority($this->authorityPHIDs);
-
-      $projects = id(new PhabricatorProjectQuery())
-        ->setViewer($this->getViewer())
-        ->withMemberPHIDs($base_phids)
-        ->execute();
-      $project_phids = mpull($projects, 'getPHID');
-
-      $all_phids = array_merge($base_phids, $project_phids);
-
       $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();
+        $fragments = $this->getFragmentsForPaths($paths);
-        foreach ($paths as $path) {
-          foreach (PhabricatorOwnersPackage::splitPath($path) as $fragment) {
-            $fragments[$fragment] = $fragment;
-          }
-        }
 
         $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  )------------------------------------------------------- */
 

src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php

@@ -18,29 +18,37 @@ final class PhabricatorOwnersPackageSearchEngine
   protected function buildCustomSearchFields() {
     return array(
       id(new PhabricatorSearchDatasourceField())
-        ->setLabel(pht('Owners'))
+        ->setLabel(pht('Authority'))
-        ->setKey('ownerPHIDs')
+        ->setKey('authorityPHIDs')
-        ->setAliases(array('owner', 'owners'))
+        ->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']) {
+    if ($map['authorityPHIDs']) {
-      $query->withOwnerPHIDs($map['ownerPHIDs']);
+      $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()));
     }
 

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  )------------------------- */