When publishing project transactions, load ancestor members

Summary: Ref T10010. Fixes T10107. When we publish a transaction about a project, we perform visibility checks for many different users. We need to know all of the ancestors' members to perform these checks.

Test Plan:
  - Before patch: when updating a subproject, daemons fatal trying to publish things because they can not test visibility of parent projects.
  - After patch: daemons successfully publish subproject updates.
  - Also added a unit test.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10010, T10107

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

src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php

@@ -113,6 +113,38 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $this->assertTrue($caught instanceof Exception);
   }
 
+  public function testAncestorMembers() {
+    $user1 = $this->createUser();
+    $user1->save();
+
+    $user2 = $this->createUser();
+    $user2->save();
+
+    $parent = $this->createProject($user1);
+    $child = $this->createProject($user1, $parent);
+
+    $this->joinProject($child, $user1);
+    $this->joinProject($child, $user2);
+
+    $project = id(new PhabricatorProjectQuery())
+      ->setViewer($user1)
+      ->withPHIDs(array($child->getPHID()))
+      ->needAncestorMembers(true)
+      ->executeOne();
+
+    $members = array_fuse($project->getParentProject()->getMemberPHIDs());
+    ksort($members);
+
+    $expect = array_fuse(
+      array(
+        $user1->getPHID(),
+        $user2->getPHID(),
+      ));
+    ksort($expect);
+
+    $this->assertEqual($expect, $members);
+  }
+
   public function testAncestryQueries() {
     $user = $this->createUser();
     $user->save();
@@ -577,24 +609,6 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
     $this->assertEqual($expect, $actual);
   }
 
-  private function attemptProjectEdit(
-    PhabricatorProject $proj,
-    PhabricatorUser $user,
-    $skip_refresh = false) {
-
-    $proj = $this->refreshProject($proj, $user, true);
-
-    $new_name = $proj->getName().' '.mt_rand();
-
-    $xaction = new PhabricatorProjectTransaction();
-    $xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME);
-    $xaction->setNewValue($new_name);
-
-    $this->applyTransactions($proj, $user, array($xaction));
-
-    return true;
-  }
-
   public function testJoinLeaveProject() {
     $user = $this->createUser();
     $user->save();
@@ -794,6 +808,25 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
       pht('Engineering + Scan'));
   }
 
+  private function attemptProjectEdit(
+    PhabricatorProject $proj,
+    PhabricatorUser $user,
+    $skip_refresh = false) {
+
+    $proj = $this->refreshProject($proj, $user, true);
+
+    $new_name = $proj->getName().' '.mt_rand();
+
+    $xaction = new PhabricatorProjectTransaction();
+    $xaction->setTransactionType(PhabricatorProjectTransaction::TYPE_NAME);
+    $xaction->setNewValue($new_name);
+
+    $this->applyTransactions($proj, $user, array($xaction));
+
+    return true;
+  }
+
+
   private function newTask(
     PhabricatorUser $viewer,
     array $projects,

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -582,7 +582,7 @@ final class PhabricatorProjectTransactionEditor
     return id(new PhabricatorProjectQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withPHIDs(array($object->getPHID()))
-      ->needMembers(true)
+      ->needAncestorMembers(true)
       ->executeOne();
   }
 

src/applications/project/query/PhabricatorProjectQuery.php

@@ -30,6 +30,7 @@ final class PhabricatorProjectQuery
 
   private $needSlugs;
   private $needMembers;
+  private $needAncestorMembers;
   private $needWatchers;
   private $needImages;
 
@@ -109,6 +110,11 @@ final class PhabricatorProjectQuery
     return $this;
   }
 
+  public function needAncestorMembers($need_ancestor_members) {
+    $this->needAncestorMembers = $need_ancestor_members;
+    return $this;
+  }
+
   public function needWatchers($need_watchers) {
     $this->needWatchers = $need_watchers;
     return $this;
@@ -220,8 +226,16 @@ final class PhabricatorProjectQuery
       $types[] = $watcher_type;
     }
 
+    $all_graph = $this->getAllReachableAncestors($projects);
+
+    if ($this->needAncestorMembers) {
+      $src_projects = $all_graph;
+    } else {
+      $src_projects = $projects;
+    }
+
     $all_sources = array();
-    foreach ($projects as $project) {
+    foreach ($src_projects as $project) {
       if ($project->isMilestone()) {
         $phid = $project->getParentProjectPHID();
       } else {
@@ -234,10 +248,15 @@ final class PhabricatorProjectQuery
       ->withSourcePHIDs($all_sources)
       ->withEdgeTypes($types);
 
+    $need_all_edges =
+      $this->needMembers ||
+      $this->needWatchers ||
+      $this->needAncestorMembers;
+
     // If we only need to know if the viewer is a member, we can restrict
     // the query to just their PHID.
     $any_edges = true;
-    if (!$this->needMembers && !$this->needWatchers) {
+    if (!$need_all_edges) {
       if ($viewer_phid) {
         $edge_query->withDestinationPHIDs(array($viewer_phid));
       } else {
@@ -253,7 +272,7 @@ final class PhabricatorProjectQuery
     }
 
     $membership_projects = array();
-    foreach ($projects as $project) {
+    foreach ($src_projects as $project) {
       $project_phid = $project->getPHID();
 
       if ($project->isMilestone()) {
@@ -274,7 +293,7 @@ final class PhabricatorProjectQuery
         $membership_projects[$project_phid] = $project;
       }
 
-      if ($this->needMembers) {
+      if ($this->needMembers || $this->needAncestorMembers) {
         $project->attachMemberPHIDs($member_phids);
       }
 
@@ -289,12 +308,15 @@ final class PhabricatorProjectQuery
       }
     }
 
-    $all_graph = $this->getAllReachableAncestors($projects);
-    $member_graph = $this->getAllReachableAncestors($membership_projects);
+    // If we loaded ancestor members, we've already populated membership
+    // lists above, so we can skip this step.
+    if (!$this->needAncestorMembers) {
+      $member_graph = $this->getAllReachableAncestors($membership_projects);
 
-    foreach ($all_graph as $phid => $project) {
-      $is_member = isset($member_graph[$phid]);
-      $project->setIsUserMember($viewer_phid, $is_member);
+      foreach ($all_graph as $phid => $project) {
+        $is_member = isset($member_graph[$phid]);
+        $project->setIsUserMember($viewer_phid, $is_member);
+      }
     }
 
     return $projects;