Simplify ProjectQuery handling of viewer membership

Summary:
Ref T10010. Currently, we do an unusual JOIN to make testing for viewer membership in projects a little cheaper.

This won't work as-is once we have subprojects, so standardize, simplify, and cover it with more tests for now. (I may be able to get a similar optimization later, but want a correct implementation first.)

Test Plan:
This change should create no behavioral differences.

  - Added tests.
  - Ran tests.
  - Viewed projects.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010

Differential Revision: https://secure.phabricator.com/D14859

src/__phutil_library_map__.php

@@ -2834,6 +2834,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectConfigOptions' => 'applications/project/config/PhabricatorProjectConfigOptions.php',
     'PhabricatorProjectConfiguredCustomField' => 'applications/project/customfield/PhabricatorProjectConfiguredCustomField.php',
     'PhabricatorProjectController' => 'applications/project/controller/PhabricatorProjectController.php',
+    'PhabricatorProjectCoreTestCase' => 'applications/project/__tests__/PhabricatorProjectCoreTestCase.php',
     'PhabricatorProjectCustomField' => 'applications/project/customfield/PhabricatorProjectCustomField.php',
     'PhabricatorProjectCustomFieldNumericIndex' => 'applications/project/storage/PhabricatorProjectCustomFieldNumericIndex.php',
     'PhabricatorProjectCustomFieldStorage' => 'applications/project/storage/PhabricatorProjectCustomFieldStorage.php',
@@ -2843,7 +2844,6 @@ phutil_register_library_map(array(
     'PhabricatorProjectDescriptionField' => 'applications/project/customfield/PhabricatorProjectDescriptionField.php',
     'PhabricatorProjectEditDetailsController' => 'applications/project/controller/PhabricatorProjectEditDetailsController.php',
     'PhabricatorProjectEditPictureController' => 'applications/project/controller/PhabricatorProjectEditPictureController.php',
-    'PhabricatorProjectEditorTestCase' => 'applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php',
     'PhabricatorProjectFeedController' => 'applications/project/controller/PhabricatorProjectFeedController.php',
     'PhabricatorProjectFulltextEngine' => 'applications/project/search/PhabricatorProjectFulltextEngine.php',
     'PhabricatorProjectHeraldAction' => 'applications/project/herald/PhabricatorProjectHeraldAction.php',
@@ -7168,6 +7168,7 @@ phutil_register_library_map(array(
       'PhabricatorStandardCustomFieldInterface',
     ),
     'PhabricatorProjectController' => 'PhabricatorController',
+    'PhabricatorProjectCoreTestCase' => 'PhabricatorTestCase',
     'PhabricatorProjectCustomField' => 'PhabricatorCustomField',
     'PhabricatorProjectCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage',
     'PhabricatorProjectCustomFieldStorage' => 'PhabricatorCustomFieldStorage',
@@ -7177,7 +7178,6 @@ phutil_register_library_map(array(
     'PhabricatorProjectDescriptionField' => 'PhabricatorProjectStandardCustomField',
     'PhabricatorProjectEditDetailsController' => 'PhabricatorProjectController',
     'PhabricatorProjectEditPictureController' => 'PhabricatorProjectController',
-    'PhabricatorProjectEditorTestCase' => 'PhabricatorTestCase',
     'PhabricatorProjectFeedController' => 'PhabricatorProjectController',
     'PhabricatorProjectFulltextEngine' => 'PhabricatorFulltextEngine',
     'PhabricatorProjectHeraldAction' => 'HeraldAction',

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -1,6 +1,6 @@
 <?php
 
-final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
+final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
 
   protected function getPhabricatorTestCaseConfiguration() {
     return array(
@@ -39,6 +39,49 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
     $this->assertFalse((bool)$this->refreshProject($proj, $user2));
   }
 
+  public function testIsViewerMemberOrWatcher() {
+    $user1 = $this->createUser()
+      ->save();
+
+    $user2 = $this->createUser()
+      ->save();
+
+    $user3 = $this->createUser()
+      ->save();
+
+    $proj1 = $this->createProject($user1);
+    $proj1 = $this->refreshProject($proj1, $user1);
+
+    $this->joinProject($proj1, $user1);
+    $this->joinProject($proj1, $user3);
+    $this->watchProject($proj1, $user3);
+
+    $proj1 = $this->refreshProject($proj1, $user1);
+
+    $this->assertTrue($proj1->isUserMember($user1->getPHID()));
+
+    $proj1 = $this->refreshProject($proj1, $user1, false, true);
+
+    $this->assertTrue($proj1->isUserMember($user1->getPHID()));
+    $this->assertFalse($proj1->isUserWatcher($user1->getPHID()));
+
+    $proj1 = $this->refreshProject($proj1, $user1, true, false);
+
+    $this->assertTrue($proj1->isUserMember($user1->getPHID()));
+    $this->assertFalse($proj1->isUserMember($user2->getPHID()));
+    $this->assertTrue($proj1->isUserMember($user3->getPHID()));
+
+    $proj1 = $this->refreshProject($proj1, $user1, true, true);
+
+    $this->assertTrue($proj1->isUserMember($user1->getPHID()));
+    $this->assertFalse($proj1->isUserMember($user2->getPHID()));
+    $this->assertTrue($proj1->isUserMember($user3->getPHID()));
+
+    $this->assertFalse($proj1->isUserWatcher($user1->getPHID()));
+    $this->assertFalse($proj1->isUserWatcher($user2->getPHID()));
+    $this->assertTrue($proj1->isUserWatcher($user3->getPHID()));
+  }
+
   public function testEditProject() {
     $user = $this->createUser();
     $user->save();
@@ -210,11 +253,13 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
   private function refreshProject(
     PhabricatorProject $project,
     PhabricatorUser $viewer,
-    $need_members = false) {
+    $need_members = false,
+    $need_watchers = false) {
 
     $results = id(new PhabricatorProjectQuery())
       ->setViewer($viewer)
       ->needMembers($need_members)
+      ->needWatchers($need_watchers)
       ->withIDs(array($project->getID()))
       ->execute();
 
@@ -255,19 +300,54 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
   private function joinProject(
     PhabricatorProject $project,
     PhabricatorUser $user) {
-    $this->joinOrLeaveProject($project, $user, '+');
+    return $this->joinOrLeaveProject($project, $user, '+');
   }
 
   private function leaveProject(
     PhabricatorProject $project,
     PhabricatorUser $user) {
-    $this->joinOrLeaveProject($project, $user, '-');
+    return $this->joinOrLeaveProject($project, $user, '-');
+  }
+
+  private function watchProject(
+    PhabricatorProject $project,
+    PhabricatorUser $user) {
+    return $this->watchOrUnwatchProject($project, $user, '+');
+  }
+
+  private function unwatchProject(
+    PhabricatorProject $project,
+    PhabricatorUser $user) {
+    return $this->watchOrUnwatchProject($project, $user, '-');
   }
 
   private function joinOrLeaveProject(
     PhabricatorProject $project,
     PhabricatorUser $user,
     $operation) {
+    return $this->applyProjectEdgeTransaction(
+      $project,
+      $user,
+      $operation,
+      PhabricatorProjectProjectHasMemberEdgeType::EDGECONST);
+  }
+
+  private function watchOrUnwatchProject(
+    PhabricatorProject $project,
+    PhabricatorUser $user,
+    $operation) {
+    return $this->applyProjectEdgeTransaction(
+      $project,
+      $user,
+      $operation,
+      PhabricatorObjectHasWatcherEdgeType::EDGECONST);
+  }
+
+  private function applyProjectEdgeTransaction(
+    PhabricatorProject $project,
+    PhabricatorUser $user,
+    $operation,
+    $edge_type) {
 
     $spec = array(
       $operation => array($user->getPHID() => $user->getPHID()),
@@ -276,9 +356,7 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
     $xactions = array();
     $xactions[] = id(new PhabricatorProjectTransaction())
       ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-      ->setMetadataValue(
+      ->setMetadataValue('edge:type', $edge_type)
-        'edge:type',
-        PhabricatorProjectProjectHasMemberEdgeType::EDGECONST)
       ->setNewValue($spec);
 
     $editor = id(new PhabricatorProjectTransactionEditor())
@@ -286,6 +364,9 @@ final class PhabricatorProjectEditorTestCase extends PhabricatorTestCase {
       ->setContentSource(PhabricatorContentSource::newConsoleSource())
       ->setContinueOnNoEffect(true)
       ->applyTransactions($project, $xactions);
+
+    return $project;
   }
 
+
 }

src/applications/project/query/PhabricatorProjectQuery.php

@@ -126,59 +126,57 @@ final class PhabricatorProjectQuery
   }
 
   protected function loadPage() {
-    $table = new PhabricatorProject();
+    return $this->loadStandardPage($this->newResultObject());
-    $data = $this->loadStandardPageRows($table);
+  }
-    $projects = $table->loadAllFromArray($data);
 
-    if ($projects) {
+  protected function willFilterPage(array $projects) {
-      $viewer_phid = $this->getViewer()->getPHID();
+    $viewer_phid = $this->getViewer()->getPHID();
-      $project_phids = mpull($projects, 'getPHID');
+    $project_phids = mpull($projects, 'getPHID');
 
-      $member_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST;
+    $member_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST;
-      $watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST;
+    $watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST;
+
+    $types = array();
+    $types[] = $member_type;
+    if ($this->needWatchers) {
+      $types[] = $watcher_type;
+    }
+
+    $edge_query = id(new PhabricatorEdgeQuery())
+      ->withSourcePHIDs($project_phids)
+      ->withEdgeTypes($types);
+
+    // If we only need to know if the viewer is a member, we can restrict
+    // the query to just their PHID.
+    if (!$this->needMembers && !$this->needWatchers) {
+      $edge_query->withDestinationPHIDs(array($viewer_phid));
+    }
+
+    $edge_query->execute();
+
+    foreach ($projects as $project) {
+      $project_phid = $project->getPHID();
+
+      $member_phids = $edge_query->getDestinationPHIDs(
+        array($project_phid),
+        array($member_type));
+
+      $project->setIsUserMember(
+        $viewer_phid,
+        in_array($viewer_phid, $member_phids));
 
-      $need_edge_types = array();
       if ($this->needMembers) {
-        $need_edge_types[] = $member_type;
+        $project->attachMemberPHIDs($member_phids);
-      } else {
-        foreach ($data as $row) {
-          $projects[$row['id']]->setIsUserMember(
-            $viewer_phid,
-            ($row['viewerIsMember'] !== null));
-        }
       }
 
       if ($this->needWatchers) {
-        $need_edge_types[] = $watcher_type;
+        $watcher_phids = $edge_query->getDestinationPHIDs(
-      }
+          array($project_phid),
-
+          array($watcher_type));
-      if ($need_edge_types) {
+        $project->attachWatcherPHIDs($watcher_phids);
-        $edges = id(new PhabricatorEdgeQuery())
+        $project->setIsUserWatcher(
-          ->withSourcePHIDs($project_phids)
+          $viewer_phid,
-          ->withEdgeTypes($need_edge_types)
+          in_array($viewer_phid, $watcher_phids));
-          ->execute();
-
-        if ($this->needMembers) {
-          foreach ($projects as $project) {
-            $phid = $project->getPHID();
-            $project->attachMemberPHIDs(
-              array_keys($edges[$phid][$member_type]));
-            $project->setIsUserMember(
-              $viewer_phid,
-              isset($edges[$phid][$member_type][$viewer_phid]));
-          }
-        }
-
-        if ($this->needWatchers) {
-          foreach ($projects as $project) {
-            $phid = $project->getPHID();
-            $project->attachWatcherPHIDs(
-              array_keys($edges[$phid][$watcher_type]));
-            $project->setIsUserWatcher(
-              $viewer_phid,
-              isset($edges[$phid][$watcher_type][$viewer_phid]));
-          }
-        }
       }
     }
 
@@ -231,20 +229,6 @@ final class PhabricatorProjectQuery
     return $projects;
   }
 
-  protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
-    $select = parent::buildSelectClauseParts($conn);
-
-    // NOTE: Because visibility checks for projects depend on whether or not
-    // the user is a project member, we always load their membership. If we're
-    // loading all members anyway we can piggyback on that; otherwise we
-    // do an explicit join.
-    if (!$this->needMembers) {
-      $select[] = 'vm.dst viewerIsMember';
-    }
-
-    return $select;
-  }
-
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
     $where = parent::buildWhereClauseParts($conn);
 
@@ -336,15 +320,6 @@ final class PhabricatorProjectQuery
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
     $joins = parent::buildJoinClauseParts($conn);
 
-    if (!$this->needMembers !== null) {
-      $joins[] = qsprintf(
-        $conn,
-        'LEFT JOIN %T vm ON vm.src = p.phid AND vm.type = %d AND vm.dst = %s',
-        PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-        PhabricatorProjectProjectHasMemberEdgeType::EDGECONST,
-        $this->getViewer()->getPHID());
-    }
-
     if ($this->memberPHIDs !== null) {
       $joins[] = qsprintf(
         $conn,