From 030726b144de6bbf2be641af70739fc3b71cd1bb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Oct 2012 15:30:51 -0700 Subject: [PATCH] Improve implementation of ManiphestTaskQuery Summary: Currently, we have a single `projectPHIDs` field, and a separate flag which makes it act like AND or OR. This is silly. Make two separate methods for setting `AND` vs `OR` projects. This also simplifies the implmentation. This doesn't change the UI or any behavior (yet), it just makes the API more usable. Test Plan: Loaded homepage, "All Projects" task view, verified queries made sense and returned correct results. Grepped for changed method name. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1610 Differential Revision: https://secure.phabricator.com/D3630 --- .../PhabricatorDirectoryMainController.php | 3 +- .../maniphest/ManiphestTaskQuery.php | 74 ++++++++++++------- .../ManiphestTaskListController.php | 25 ++++--- .../PhabricatorProjectListController.php | 3 +- 4 files changed, 63 insertions(+), 42 deletions(-) diff --git a/src/applications/directory/controller/PhabricatorDirectoryMainController.php b/src/applications/directory/controller/PhabricatorDirectoryMainController.php index ff9a8a5618..43c29cbe99 100644 --- a/src/applications/directory/controller/PhabricatorDirectoryMainController.php +++ b/src/applications/directory/controller/PhabricatorDirectoryMainController.php @@ -247,8 +247,7 @@ final class PhabricatorDirectoryMainController $task_query = new ManiphestTaskQuery(); $task_query->withStatus(ManiphestTaskQuery::STATUS_OPEN); $task_query->withPriority(ManiphestTaskPriority::PRIORITY_TRIAGE); - $task_query->withProjects(mpull($projects, 'getPHID')); - $task_query->withAnyProject(true); + $task_query->withAnyProjects(mpull($projects, 'getPHID')); $task_query->setLimit(10); $tasks = $task_query->execute(); } else { diff --git a/src/applications/maniphest/ManiphestTaskQuery.php b/src/applications/maniphest/ManiphestTaskQuery.php index 6ceaa8a5f6..ab66578a12 100644 --- a/src/applications/maniphest/ManiphestTaskQuery.php +++ b/src/applications/maniphest/ManiphestTaskQuery.php @@ -22,7 +22,7 @@ * * @group maniphest */ -final class ManiphestTaskQuery { +final class ManiphestTaskQuery extends PhabricatorQuery { private $taskIDs = array(); private $authorPHIDs = array(); @@ -31,7 +31,7 @@ final class ManiphestTaskQuery { private $projectPHIDs = array(); private $xprojectPHIDs = array(); private $subscriberPHIDs = array(); - private $anyProject = false; + private $anyProjectPHIDs = array(); private $includeNoProject = null; private $fullTextSearch = ''; @@ -179,8 +179,8 @@ final class ManiphestTaskQuery { return $this->groupByProjectResults; } - public function withAnyProject($any_project) { - $this->anyProject = $any_project; + public function withAnyProjects(array $projects) { + $this->anyProjectPHIDs = $projects; return $this; } @@ -203,18 +203,15 @@ final class ManiphestTaskQuery { $where[] = $this->buildOwnerWhereClause($conn); $where[] = $this->buildSubscriberWhereClause($conn); $where[] = $this->buildProjectWhereClause($conn); + $where[] = $this->buildAnyProjectWhereClause($conn); $where[] = $this->buildXProjectWhereClause($conn); $where[] = $this->buildFullTextWhereClause($conn); - $where = array_filter($where); - if ($where) { - $where = 'WHERE ('.implode(') AND (', $where).')'; - } else { - $where = ''; - } + $where = $this->formatWhereClause($where); $join = array(); $join[] = $this->buildProjectJoinClause($conn); + $join[] = $this->buildAnyProjectJoinClause($conn); $join[] = $this->buildXProjectJoinClause($conn); $join[] = $this->buildSubscriberJoinClause($conn); @@ -228,25 +225,25 @@ final class ManiphestTaskQuery { $having = ''; $count = ''; $group = ''; - if (count($this->projectPHIDs) > 1) { - - // If we're searching for more than one project: - // - We'll get multiple rows for tasks when they join the project table - // multiple times. We use GROUP BY to make them distinct again. - // - We want to treat the query as an intersection query, not a union - // query. We sum the project count and require it be the same as the - // number of projects we're searching for. (If 'anyProject' is set, - // we do union instead.) + if (count($this->projectPHIDs) > 1 || count($this->anyProjectPHIDs) > 1) { + // If we're joining multiple rows, we need to group the results by the + // task IDs. $group = 'GROUP BY task.id'; + } else { + $group = ''; + } - if (!$this->anyProject) { - $count = ', COUNT(project.projectPHID) projectCount'; - $having = qsprintf( - $conn, - 'HAVING projectCount = %d', - count($this->projectPHIDs)); - } + if (count($this->projectPHIDs) > 1) { + // We want to treat the query as an intersection query, not a union + // query. We sum the project count and require it be the same as the + // number of projects we're searching for. + + $count = ', COUNT(project.projectPHID) projectCount'; + $having = qsprintf( + $conn, + 'HAVING projectCount = %d', + count($this->projectPHIDs)); } $order = $this->buildOrderClause($conn); @@ -456,6 +453,29 @@ final class ManiphestTaskQuery { $project_dao->getTableName()); } + private function buildAnyProjectWhereClause($conn) { + if (!$this->anyProjectPHIDs) { + return null; + } + + return qsprintf( + $conn, + 'anyproject.projectPHID IN (%Ls)', + $this->anyProjectPHIDs); + } + + private function buildAnyProjectJoinClause($conn) { + if (!$this->anyProjectPHIDs) { + return null; + } + + $project_dao = new ManiphestTaskProject(); + return qsprintf( + $conn, + 'LEFT JOIN %T anyproject ON anyproject.taskPHID = task.phid', + $project_dao->getTableName()); + } + private function buildXProjectWhereClause($conn) { if (!$this->xprojectPHIDs) { return null; @@ -592,7 +612,7 @@ final class ManiphestTaskQuery { $ii = 0; foreach ($tasks as $key => $task) { $phids = $task->getProjectPHIDs(); - if (!$this->anyProject && $this->projectPHIDs) { + if ($this->projectPHIDs) { $phids = array_diff($phids, $this->projectPHIDs); } if ($phids) { diff --git a/src/applications/maniphest/controller/ManiphestTaskListController.php b/src/applications/maniphest/controller/ManiphestTaskListController.php index e55f88b43c..6f0fbc55cc 100644 --- a/src/applications/maniphest/controller/ManiphestTaskListController.php +++ b/src/applications/maniphest/controller/ManiphestTaskListController.php @@ -406,8 +406,11 @@ final class ManiphestTaskListController extends ManiphestController { $any_project = false; $search_text = $search_query->getParameter('fullTextSearch'); $user_phids = $search_query->getParameter('userPHIDs', array()); - $project_phids = $search_query->getParameter('projectPHIDs', array()); $task_ids = $search_query->getParameter('taskIDs', array()); + $project_phids = $search_query->getParameter('projectPHIDs', array()); + $any_project_phids = $search_query->getParameter( + 'anyProjectPHIDs', + array()); $xproject_phids = $search_query->getParameter( 'excludeProjectPHIDs', array()); @@ -467,18 +470,16 @@ final class ManiphestTaskListController extends ManiphestController { break; case 'projecttriage': $query->withPriority(ManiphestTaskPriority::PRIORITY_TRIAGE); - $any_project = true; + $query->withAnyProjects($any_project_phids); break; case 'projectall': - $any_project = true; + $query->withAnyProjects($any_project_phids); break; case 'custom': $query->withPrioritiesBetween($low_priority, $high_priority); break; } - $query->withAnyProject($any_project); - $query->withFullTextSearch($search_text); $order_map = array( @@ -689,15 +690,16 @@ final class ManiphestTaskListController extends ManiphestController { array($user->getPHID())); if ($this->view == 'projecttriage' || $this->view == 'projectall') { - $project_query = new PhabricatorProjectQuery(); - $project_query->setViewer($user); - $project_query->withMemberPHIDs($user_phids); - $projects = $project_query->execute(); - $project_phids = mpull($projects, 'getPHID'); + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withMemberPHIDs($user_phids) + ->execute(); + $any_project_phids = mpull($projects, 'getPHID'); } else { - $project_phids = $request->getStrList('projects'); + $any_project_phids = $request->getStrList('aprojects'); } + $project_phids = $request->getStrList('projects'); $exclude_project_phids = $request->getStrList('xprojects'); $task_ids = $request->getStrList('tasks'); @@ -739,6 +741,7 @@ final class ManiphestTaskListController extends ManiphestController { 'view' => $this->view, 'userPHIDs' => $user_phids, 'projectPHIDs' => $project_phids, + 'anyProjectPHIDs' => $any_project_phids, 'excludeProjectPHIDs' => $exclude_project_phids, 'ownerPHIDs' => $owner_phids, 'authorPHIDs' => $author_phids, diff --git a/src/applications/project/controller/PhabricatorProjectListController.php b/src/applications/project/controller/PhabricatorProjectListController.php index 83f9cd3fa4..6e12e5c61b 100644 --- a/src/applications/project/controller/PhabricatorProjectListController.php +++ b/src/applications/project/controller/PhabricatorProjectListController.php @@ -84,8 +84,7 @@ final class PhabricatorProjectListController $groups = array(); if ($project_phids) { $query = id(new ManiphestTaskQuery()) - ->withProjects($project_phids) - ->withAnyProject(true) + ->withAnyProjects($project_phids) ->withStatus(ManiphestTaskQuery::STATUS_OPEN) ->setLimit(PHP_INT_MAX);