From d1eed54d85bc499e80dfaa96987282c203df4c00 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 24 May 2016 06:05:22 -0700
Subject: [PATCH] Fix expansion of projects into lists of user PHIDs

Summary:
Ref T11016. I think I inverted the meaning of this function by accident in D14893.

The intent is to return a list of users: direct users, and all members of all projects.

Prior to this patch actually returns direct users, and all projects they are members of.

Test Plan:
  - Created "Project with Dog".
  - Added user "dog" to project.
  - Created package "X", owning file "/x", with audit enabled.
  - Made "X" owned by "Project with Dog".
  - Modified "/x" and had user "dog" accept it.
  - Landed change.
  - Prior to change: package "X" incorrectly added as auditor.
  - After change: package "X" correctly omitted as auditor, because a member reviewed the change.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11016

Differential Revision: https://secure.phabricator.com/D15971
---
 .../owners/storage/PhabricatorOwnersOwner.php | 44 ++++++++++++-------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/src/applications/owners/storage/PhabricatorOwnersOwner.php b/src/applications/owners/storage/PhabricatorOwnersOwner.php
index 07d5b59a20..ddca80b099 100644
--- a/src/applications/owners/storage/PhabricatorOwnersOwner.php
+++ b/src/applications/owners/storage/PhabricatorOwnersOwner.php
@@ -45,25 +45,37 @@ final class PhabricatorOwnersOwner extends PhabricatorOwnersDAO {
       'packageID IN (%Ls)',
       $package_ids);
 
-    $all_phids = phid_group_by_type(mpull($owners, 'getUserPHID'));
+    $type_user = PhabricatorPeopleUserPHIDType::TYPECONST;
+    $type_project = PhabricatorProjectProjectPHIDType::TYPECONST;
 
-    $user_phids = idx($all_phids,
-      PhabricatorPeopleUserPHIDType::TYPECONST,
-      array());
-
-    if ($user_phids) {
-      $projects = id(new PhabricatorProjectQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withMemberPHIDs($user_phids)
-        ->withIsMilestone(false)
-        ->execute();
-      $project_phids = mpull($projects, 'getPHID');
-    } else {
-      $project_phids = array();
+    $user_phids = array();
+    $project_phids = array();
+    foreach ($owners as $owner) {
+      $owner_phid = $owner->getUserPHID();
+      switch (phid_get_type($owner_phid)) {
+        case PhabricatorPeopleUserPHIDType::TYPECONST:
+          $user_phids[] = $owner_phid;
+          break;
+        case PhabricatorProjectProjectPHIDType::TYPECONST:
+          $project_phids[] = $owner_phid;
+          break;
+      }
     }
 
-    $all_phids = array_fuse($user_phids) + array_fuse($project_phids);
+    if ($project_phids) {
+      $projects = id(new PhabricatorProjectQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs($project_phids)
+        ->needMembers(true)
+        ->execute();
+      foreach ($projects as $project) {
+        foreach ($project->getMemberPHIDs() as $member_phid) {
+          $user_phids[] = $member_phid;
+        }
+      }
+    }
 
-    return array_values($all_phids);
+    $user_phids = array_fuse($user_phids);
+    return array_values($user_phids);
   }
 }