From 83db5965ab6c640864fb8b66d813369274d40594 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 18 Dec 2014 13:53:45 -0800 Subject: [PATCH] Maniphest - introduce needProjectPHIDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Ref T5245. This is some of the associated cleanup there. Test Plan: foreach ManiphestTaskQuery site, I made the change (or not) and tested as follows: === Call sites where added needProjectPHIDs === - PhabricatorHomeMainController - loaded the home page - ManiphestBatchEditController - batch edited some tasks (added a project) - ManiphestConduitAPIMethod - tested implicitly when tested ManiphestUpdateConduitAPIMethod - ManiphestInfoConduitAPIMethod - used the method via conduit console with input id : 1 - ManiphestQueryConduitAPIMethod - used the method via conduit console with input ids : [1, 2] - ManiphestUpdateConduitAPIMethod - used the method via conduit with input id : 1 and comment : “asdasds" - ManiphestReportController - viewed “By User” and “By Project” - ManiphestSubpriorityController - changed the priority of a task via a drag on manphest home - ManiphestTaskMailReceiver - updated Task 1 via bin/mail receive-test with a comment that is the README - ManiphestTaskSearchEngine - loaded Manifest home page - ManiphestTaskEditController - edited a task - ManiphestTransactionEditor - closed a blocking task - ManiphestTransactionSaveController - commented on a task - PhabricatorProjectProfileController - viewed project with id of 1 that has a few tasks in it - PhabricatorSearchAttachController - merged tasks together - DifferentialTransactionEditor - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated - PhabricatorRepositoryCommitMessageParserWorker - submit a diff that references a task; commit the diff (thus closing the diff) and the task gets updated === Calls sites where *did not* add needProjectPHIDs (they do not appear in this revision) === - PhabricatorManiphestApplication - loaded the home page - ManiphestGetTaskTransactionsConduitAPIMethod - used the method via conduit console with input ids : [1, 2] ManiphestTaskDetailController - viewed a task with and without associated projects; finished workflow creating a task with a parent - ManiphestTransactionPreviewController - verified transaction preview showed up properly - PhabricatorProjectBoardViewController - viewed a board - PhabricatorProjectMoveController - moved a task around - ManiphestRemarkupRule - made a task reference like {T123} - ManiphestTaskQuery - executed a custom query for all tasks with page size of 2 and paginated through some tasks - ManiphestTaskPHIDType - nothing random seems broken? =D === Call sites where had to do something funky === - ManiphestHovercardEventListener - loaded hover cards from task mentions Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T5245 Differential Revision: https://secure.phabricator.com/D11004 --- .../PhabricatorHomeMainController.php | 3 ++ .../conduit/ManiphestConduitAPIMethod.php | 1 + .../conduit/ManiphestInfoConduitAPIMethod.php | 1 + .../ManiphestQueryConduitAPIMethod.php | 8 ++--- .../ManiphestUpdateConduitAPIMethod.php | 17 ++++------ .../ManiphestBatchEditController.php | 1 + .../controller/ManiphestReportController.php | 6 ++++ .../ManiphestSubpriorityController.php | 1 + .../ManiphestTaskEditController.php | 2 ++ .../ManiphestTransactionSaveController.php | 1 + .../editor/ManiphestTransactionEditor.php | 1 + .../event/ManiphestHovercardEventListener.php | 17 ++++------ .../mail/ManiphestTaskMailReceiver.php | 1 + .../maniphest/query/ManiphestTaskQuery.php | 34 +++++++++++-------- .../query/ManiphestTaskSearchEngine.php | 3 +- .../PhabricatorProjectProfileController.php | 1 + ...torRepositoryCommitMessageParserWorker.php | 1 + .../PhabricatorSearchAttachController.php | 7 +++- 18 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/applications/home/controller/PhabricatorHomeMainController.php b/src/applications/home/controller/PhabricatorHomeMainController.php index 5fa7684ffa..e9cab61439 100644 --- a/src/applications/home/controller/PhabricatorHomeMainController.php +++ b/src/applications/home/controller/PhabricatorHomeMainController.php @@ -130,6 +130,7 @@ final class PhabricatorHomeMainController extends PhabricatorHomeController { ->setViewer($user) ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()) ->withPriorities(array($unbreak_now)) + ->needProjectPHIDs(true) ->setLimit(10); $tasks = $task_query->execute(); @@ -173,6 +174,7 @@ final class PhabricatorHomeMainController extends PhabricatorHomeController { ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()) ->withPriorities(array($needs_triage)) ->withAnyProjects(mpull($projects, 'getPHID')) + ->needProjectPHIDs(true) ->setLimit(10); $tasks = $task_query->execute(); } else { @@ -269,6 +271,7 @@ final class PhabricatorHomeMainController extends PhabricatorHomeController { ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()) ->setGroupBy(ManiphestTaskQuery::GROUP_PRIORITY) ->withOwners(array($user_phid)) + ->needProjectPHIDs(true) ->setLimit(10); $tasks = $task_query->execute(); diff --git a/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php index 727333a573..aaaf4c377b 100644 --- a/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php @@ -237,6 +237,7 @@ abstract class ManiphestConduitAPIMethod extends ConduitAPIMethod { ->setViewer($request->getUser()) ->withPHIDs(array($task->getPHID())) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->executeOne(); } diff --git a/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php index 4b3eeb273c..b998bb627c 100644 --- a/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php @@ -33,6 +33,7 @@ final class ManiphestInfoConduitAPIMethod extends ManiphestConduitAPIMethod { ->setViewer($request->getUser()) ->withIDs(array($task_id)) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->executeOne(); if (!$task) { throw new ConduitException('ERR_BAD_TASK'); diff --git a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php index 8278333e98..3375f30e5a 100644 --- a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php @@ -61,10 +61,10 @@ class ManiphestQueryConduitAPIMethod extends ManiphestConduitAPIMethod { } protected function execute(ConduitAPIRequest $request) { - $query = new ManiphestTaskQuery(); - - $query->setViewer($request->getUser()); - $query->needSubscriberPHIDs(true); + $query = id(new ManiphestTaskQuery()) + ->setViewer($request->getUser()) + ->needProjectPHIDs(true) + ->needSubscriberPHIDs(true); $task_ids = $request->getValue('ids'); if ($task_ids) { diff --git a/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php index 849d73e2e3..667857a189 100644 --- a/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestUpdateConduitAPIMethod.php @@ -34,19 +34,16 @@ final class ManiphestUpdateConduitAPIMethod extends ManiphestConduitAPIMethod { throw new Exception("Specify exactly one of 'id' and 'phid'."); } + $query = id (new ManiphestTaskQuery()) + ->setViewer($request->getUser()) + ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true); if ($id) { - $task = id(new ManiphestTaskQuery()) - ->setViewer($request->getUser()) - ->withIDs(array($id)) - ->needSubscriberPHIDs(true) - ->executeOne(); + $query->withIDs(array($id)); } else { - $task = id(new ManiphestTaskQuery()) - ->setViewer($request->getUser()) - ->withPHIDs(array($phid)) - ->needSubscriberPHIDs(true) - ->executeOne(); + $query->withPHIDs(array($phid)); } + $task = $query->executeOne(); $params = $request->getAllParameters(); unset($params['id']); diff --git a/src/applications/maniphest/controller/ManiphestBatchEditController.php b/src/applications/maniphest/controller/ManiphestBatchEditController.php index c540e448ba..81122cee51 100644 --- a/src/applications/maniphest/controller/ManiphestBatchEditController.php +++ b/src/applications/maniphest/controller/ManiphestBatchEditController.php @@ -19,6 +19,7 @@ final class ManiphestBatchEditController extends ManiphestController { PhabricatorPolicyCapability::CAN_EDIT, )) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->execute(); $actions = $request->getStr('actions'); diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php index fdf41722cd..ba2c227bf5 100644 --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -396,6 +396,12 @@ final class ManiphestReportController extends ManiphestController { ->setViewer($user) ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()); + switch ($this->view) { + case 'project': + $query->needProjectPHIDs(true); + break; + } + $project_phid = $request->getStr('project'); $project_handle = null; if ($project_phid) { diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php index 5218b9a4b1..8601947a34 100644 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ b/src/applications/maniphest/controller/ManiphestSubpriorityController.php @@ -13,6 +13,7 @@ final class ManiphestSubpriorityController extends ManiphestController { $task = id(new ManiphestTaskQuery()) ->setViewer($user) ->withIDs(array($request->getInt('task'))) + ->needProjectPHIDs(true) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index d310d49ce6..5d91917d4e 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -39,6 +39,7 @@ final class ManiphestTaskEditController extends ManiphestController { )) ->withIDs(array($this->id)) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->executeOne(); if (!$task) { return new Aphront404Response(); @@ -446,6 +447,7 @@ final class ManiphestTaskEditController extends ManiphestController { ->setViewer($user) ->withIDs(array($template_id)) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->executeOne(); if ($template_task) { $cc_phids = array_unique(array_merge( diff --git a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php index a1738429c9..cc5be68982 100644 --- a/src/applications/maniphest/controller/ManiphestTransactionSaveController.php +++ b/src/applications/maniphest/controller/ManiphestTransactionSaveController.php @@ -10,6 +10,7 @@ final class ManiphestTransactionSaveController extends ManiphestController { ->setViewer($user) ->withIDs(array($request->getStr('taskID'))) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->executeOne(); if (!$task) { return new Aphront404Response(); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 197c729d6c..1d44b056b1 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -370,6 +370,7 @@ final class ManiphestTransactionEditor ->setViewer($this->getActor()) ->withPHIDs($blocked_phids) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->execute(); $old = $unblock_xaction->getOldValue(); diff --git a/src/applications/maniphest/event/ManiphestHovercardEventListener.php b/src/applications/maniphest/event/ManiphestHovercardEventListener.php index dab66739b9..197e5ddd19 100644 --- a/src/applications/maniphest/event/ManiphestHovercardEventListener.php +++ b/src/applications/maniphest/event/ManiphestHovercardEventListener.php @@ -25,6 +25,7 @@ final class ManiphestHovercardEventListener extends PhabricatorEventListener { return; } + $e_project = PhabricatorProjectObjectHasProjectEdgeType::EDGECONST; // Fun with "Unbeta Pholio", hua hua $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK; $e_dep_by = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK; @@ -33,6 +34,7 @@ final class ManiphestHovercardEventListener extends PhabricatorEventListener { ->withSourcePHIDs(array($phid)) ->withEdgeTypes( array( + $e_project, $e_dep_on, $e_dep_by, )); @@ -40,12 +42,10 @@ final class ManiphestHovercardEventListener extends PhabricatorEventListener { $edge_phids = $edge_query->getDestinationPHIDs(); $owner_phid = $task->getOwnerPHID(); - $project_phids = $task->getProjectPHIDs(); $phids = array_filter(array_merge( array($owner_phid), - $edge_phids, - $project_phids)); + $edge_phids)); $viewer_handles = $this->loadHandles($phids, $viewer); @@ -58,17 +58,12 @@ final class ManiphestHovercardEventListener extends PhabricatorEventListener { } $hovercard->addField(pht('Assigned to'), $owner); - if ($project_phids) { - $hovercard->addField(pht('Projects'), - implode_selected_handle_links(', ', $viewer_handles, $project_phids)); - } if ($edge_phids) { $edge_types = array( - PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK - => pht('Dependent Tasks'), - PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK - => pht('Depends On'), + $e_project => pht('Projects'), + $e_dep_by => pht('Dependent Tasks'), + $e_dep_on => pht('Depends On'), ); $max_count = 6; diff --git a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php index 527e482691..59d97cfcc8 100644 --- a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php @@ -18,6 +18,7 @@ final class ManiphestTaskMailReceiver extends PhabricatorObjectMailReceiver { ->setViewer($viewer) ->withIDs(array($id)) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->execute(); return head($results); diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index ec6ee0ce30..b57e6187f6 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -51,6 +51,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { const ORDER_TITLE = 'order-title'; private $needSubscriberPHIDs; + private $needProjectPHIDs; const DEFAULT_PAGE_SIZE = 1000; @@ -185,6 +186,11 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $this; } + public function needProjectPHIDs($bool) { + $this->needProjectPHIDs = $bool; + return $this; + } + public function loadPage() { // TODO: (T603) It is possible for a user to find the PHID of a project // they can't see, then query for tasks in that project and deduce the @@ -340,22 +346,20 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { protected function didFilterPage(array $tasks) { $phids = mpull($tasks, 'getPHID'); - // TODO: Eventually, we should make this optional and introduce a - // needProjectPHIDs() method, but for now there's a lot of code which - // assumes the data is always populated. + if ($this->needProjectPHIDs) { + $edge_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($phids) + ->withEdgeTypes( + array( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + )); + $edge_query->execute(); - $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($phids) - ->withEdgeTypes( - array( - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, - )); - $edge_query->execute(); - - foreach ($tasks as $task) { - $project_phids = $edge_query->getDestinationPHIDs( - array($task->getPHID())); - $task->attachProjectPHIDs($project_phids); + foreach ($tasks as $task) { + $project_phids = $edge_query->getDestinationPHIDs( + array($task->getPHID())); + $task->attachProjectPHIDs($project_phids); + } } if ($this->needSubscriberPHIDs) { diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 409d94e5f7..7af51b74f5 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -119,7 +119,8 @@ final class ManiphestTaskSearchEngine } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new ManiphestTaskQuery()); + $query = id(new ManiphestTaskQuery()) + ->needProjectPHIDs(true); $author_phids = $saved->getParameter('authorPHIDs'); if ($author_phids) { diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index 49c18af813..e0dfeac4d6 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -143,6 +143,7 @@ final class PhabricatorProjectProfileController ->withAnyProjects(array($project->getPHID())) ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()) ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) + ->needProjectPHIDs(true) ->setLimit(10); $tasks = $query->execute(); diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index f97fefc4c6..36b180e8a2 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -459,6 +459,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $tasks = id(new ManiphestTaskQuery()) ->setViewer($actor) ->withIDs(array_keys($task_statuses)) + ->needProjectPHIDs(true) ->execute(); foreach ($tasks as $task_id => $task) { diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index 00ab3cdddc..360319ce6f 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/src/applications/search/controller/PhabricatorSearchAttachController.php @@ -144,6 +144,7 @@ final class PhabricatorSearchAttachController ->setViewer($user) ->withPHIDs(array_keys($phids)) ->needSubscriberPHIDs(true) + ->needProjectPHIDs(true) ->execute(); if (empty($targets)) { @@ -158,9 +159,13 @@ final class PhabricatorSearchAttachController $cc_vector = array(); // since we loaded this via a generic object query, go ahead and get the - // attach the cc phids now + // attach the subscriber and project phids now $task->attachSubscriberPHIDs( PhabricatorSubscribersQuery::loadSubscribersForPHID($task->getPHID())); + $task->attachProjectPHIDs( + PhabricatorEdgeQuery::loadDestinationPHIDs($task->getPHID(), + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)); + $cc_vector[] = $task->getSubscriberPHIDs(); foreach ($targets as $target) { $cc_vector[] = $target->getSubscriberPHIDs();