From 16d8e806a0bcb17480668bd5d58179e8451de464 Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Wed, 23 Dec 2015 03:42:42 -0800 Subject: [PATCH] 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 | 4 +- .../PhabricatorProjectCoreTestCase.php} | 95 +++++++++++++-- .../project/query/PhabricatorProjectQuery.php | 113 +++++++----------- 3 files changed, 134 insertions(+), 78 deletions(-) rename src/applications/project/{editor/__tests__/PhabricatorProjectEditorTestCase.php => __tests__/PhabricatorProjectCoreTestCase.php} (74%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 88334bba88..4f583f4b4a 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php similarity index 74% rename from src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php rename to src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index 4d19812fe3..fd42902090 100644 --- a/src/applications/project/editor/__tests__/PhabricatorProjectEditorTestCase.php +++ b/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( - 'edge:type', - PhabricatorProjectProjectHasMemberEdgeType::EDGECONST) + ->setMetadataValue('edge:type', $edge_type) ->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; } + } diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index e534e1c4bf..f1ccad105c 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -126,59 +126,57 @@ final class PhabricatorProjectQuery } protected function loadPage() { - $table = new PhabricatorProject(); - $data = $this->loadStandardPageRows($table); - $projects = $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); + } - if ($projects) { - $viewer_phid = $this->getViewer()->getPHID(); - $project_phids = mpull($projects, 'getPHID'); + protected function willFilterPage(array $projects) { + $viewer_phid = $this->getViewer()->getPHID(); + $project_phids = mpull($projects, 'getPHID'); - $member_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST; - $watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST; + $member_type = PhabricatorProjectProjectHasMemberEdgeType::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; - } else { - foreach ($data as $row) { - $projects[$row['id']]->setIsUserMember( - $viewer_phid, - ($row['viewerIsMember'] !== null)); - } + $project->attachMemberPHIDs($member_phids); } if ($this->needWatchers) { - $need_edge_types[] = $watcher_type; - } - - if ($need_edge_types) { - $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($project_phids) - ->withEdgeTypes($need_edge_types) - ->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])); - } - } + $watcher_phids = $edge_query->getDestinationPHIDs( + array($project_phid), + array($watcher_type)); + $project->attachWatcherPHIDs($watcher_phids); + $project->setIsUserWatcher( + $viewer_phid, + in_array($viewer_phid, $watcher_phids)); } } @@ -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,