From d82e135cde781e76bb22a1a3e12222e471eb396b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 May 2013 14:09:37 -0700 Subject: [PATCH] Implement generalized application search in Macros Summary: Ref T2625. Works out the last kinks of generalization and gives Macros the more powerful new query engine. Overall, this feels pretty good to me. Test Plan: Executed, saved and edited a bunch of Macro queries. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2625 Differential Revision: https://secure.phabricator.com/D6078 --- src/__phutil_library_map__.php | 8 +- .../PhabricatorApplicationMacro.php | 2 +- .../controller/PhabricatorMacroController.php | 15 +- .../PhabricatorMacroListController.php | 150 +++++------------- .../macro/query/PhabricatorMacroQuery.php | 28 +++- .../query/PhabricatorMacroSearchEngine.php | 122 ++++++++++++++ .../query/PhabricatorPasteSearchEngine.php | 9 +- ...PhabricatorApplicationSearchController.php | 34 +++- .../search/storage/PhabricatorSavedQuery.php | 9 +- 9 files changed, 226 insertions(+), 151 deletions(-) create mode 100644 src/applications/macro/query/PhabricatorMacroSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fe4b191c08..7651781343 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1088,6 +1088,7 @@ phutil_register_library_map(array( 'PhabricatorMacroMemeDialogController' => 'applications/macro/controller/PhabricatorMacroMemeDialogController.php', 'PhabricatorMacroQuery' => 'applications/macro/query/PhabricatorMacroQuery.php', 'PhabricatorMacroReplyHandler' => 'applications/macro/mail/PhabricatorMacroReplyHandler.php', + 'PhabricatorMacroSearchEngine' => 'applications/macro/query/PhabricatorMacroSearchEngine.php', 'PhabricatorMacroTransaction' => 'applications/macro/storage/PhabricatorMacroTransaction.php', 'PhabricatorMacroTransactionComment' => 'applications/macro/storage/PhabricatorMacroTransactionComment.php', 'PhabricatorMacroTransactionQuery' => 'applications/macro/query/PhabricatorMacroTransactionQuery.php', @@ -2874,12 +2875,17 @@ phutil_register_library_map(array( 'PhabricatorMacroDisableController' => 'PhabricatorMacroController', 'PhabricatorMacroEditController' => 'PhabricatorMacroController', 'PhabricatorMacroEditor' => 'PhabricatorApplicationTransactionEditor', - 'PhabricatorMacroListController' => 'PhabricatorMacroController', + 'PhabricatorMacroListController' => + array( + 0 => 'PhabricatorMacroController', + 1 => 'PhabricatorApplicationSearchResultsControllerInterface', + ), 'PhabricatorMacroMailReceiver' => 'PhabricatorObjectMailReceiver', 'PhabricatorMacroMemeController' => 'PhabricatorMacroController', 'PhabricatorMacroMemeDialogController' => 'PhabricatorMacroController', 'PhabricatorMacroQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorMacroReplyHandler' => 'PhabricatorMailReplyHandler', + 'PhabricatorMacroSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorMacroTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorMacroTransactionComment' => 'PhabricatorApplicationTransactionComment', 'PhabricatorMacroTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/macro/application/PhabricatorApplicationMacro.php b/src/applications/macro/application/PhabricatorApplicationMacro.php index eb6a23ccc4..0233710c10 100644 --- a/src/applications/macro/application/PhabricatorApplicationMacro.php +++ b/src/applications/macro/application/PhabricatorApplicationMacro.php @@ -25,7 +25,7 @@ final class PhabricatorApplicationMacro extends PhabricatorApplication { public function getRoutes() { return array( '/macro/' => array( - '((?Pall|active|my)/)?' => 'PhabricatorMacroListController', + '(query/(?P[^/]+)/)?' => 'PhabricatorMacroListController', 'create/' => 'PhabricatorMacroEditController', 'view/(?P[1-9]\d*)/' => 'PhabricatorMacroViewController', 'comment/(?P[1-9]\d*)/' => 'PhabricatorMacroCommentController', diff --git a/src/applications/macro/controller/PhabricatorMacroController.php b/src/applications/macro/controller/PhabricatorMacroController.php index 0a4ac3ccf4..5f0c698364 100644 --- a/src/applications/macro/controller/PhabricatorMacroController.php +++ b/src/applications/macro/controller/PhabricatorMacroController.php @@ -3,7 +3,7 @@ abstract class PhabricatorMacroController extends PhabricatorController { - protected function buildSideNavView($for_app = false, $has_search = false) { + protected function buildSideNavView($for_app = false) { $nav = new AphrontSideNavFilterView(); $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); @@ -14,16 +14,9 @@ abstract class PhabricatorMacroController $this->getApplicationURI('/create/')); } - $nav->addLabel(pht('Macros')); - $nav->addFilter('active', pht('Active Macros')); - $nav->addFilter('all', pht('All Macros')); - $nav->addFilter('my', pht('My Macros')); - if ($has_search) { - $nav->addFilter('search', - pht('Search'), - $this->getRequest()->getRequestURI()); - } - + id(new PhabricatorMacroSearchEngine()) + ->setViewer($this->getRequest()->getUser()) + ->addNavigationItems($nav); return $nav; } diff --git a/src/applications/macro/controller/PhabricatorMacroListController.php b/src/applications/macro/controller/PhabricatorMacroListController.php index f7889d397e..95a0ab1f41 100644 --- a/src/applications/macro/controller/PhabricatorMacroListController.php +++ b/src/applications/macro/controller/PhabricatorMacroListController.php @@ -1,97 +1,39 @@ filter = idx($data, 'filter', 'active'); + $this->key = idx($data, 'key', 'active'); } public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $controller = id(new PhabricatorApplicationSearchController($request)) + ->setQueryKey($this->key) + ->setSearchEngine(new PhabricatorMacroSearchEngine()) + ->setNavigation($this->buildSideNavView()); - $pager = id(new AphrontCursorPagerView()) - ->readFromRequest($request); + return $this->delegateToController($controller); + } - $query = new PhabricatorMacroQuery(); - $query->setViewer($viewer); - - $filter = $request->getStr('name'); - if (strlen($filter)) { - $query->withNameLike($filter); - } - - $authors = $request->getArr('authors'); - - if ($authors) { - $query->withAuthorPHIDs($authors); - } - - $has_search = $filter || $authors; - - if ($this->filter == 'my') { - $query->withAuthorPHIDs(array($viewer->getPHID())); - // For pre-filling the tokenizer - $authors = array($viewer->getPHID()); - } - - if ($this->filter == 'active') { - $query->withStatus(PhabricatorMacroQuery::STATUS_ACTIVE); - } - - $macros = $query->executeWithCursorPager($pager); - if ($has_search) { - $nodata = pht('There are no macros matching the filter.'); - } else { - $nodata = pht('There are no image macros yet.'); - } - - if ($authors) { - $author_phids = array_fuse($authors); - } else { - $author_phids = array(); - } - - $author_phids += mpull($macros, 'getAuthorPHID', 'getAuthorPHID'); + public function renderResultsList(array $macros) { + assert_instances_of($macros, 'PhabricatorFileImageMacro'); + $viewer = $this->getRequest()->getUser(); + $author_phids = mpull($macros, 'getAuthorPHID', 'getAuthorPHID'); $this->loadHandles($author_phids); - $author_handles = array_select_keys($this->getLoadedHandles(), $authors); - - $filter_form = id(new AphrontFormView()) - ->setMethod('GET') - ->setUser($request->getUser()) - ->setNoShading(true) - ->appendChild( - id(new AphrontFormTextControl()) - ->setName('name') - ->setLabel(pht('Name')) - ->setValue($filter)) - ->appendChild( - id(new AphrontFormTokenizerControl()) - ->setName('authors') - ->setLabel(pht('Authors')) - ->setDatasource('/typeahead/common/users/') - ->setValue(mpull($author_handles, 'getFullName'))) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Filter Image Macros'))); - - $filter_view = new AphrontListFilterView(); - $filter_view->appendChild($filter_form); - - $nav = $this->buildSideNavView( - $for_app = false, - $has_search); - $nav->selectFilter($has_search ? 'search' : $this->filter); - - $nav->appendChild($filter_view); + $author_handles = array_select_keys( + $this->getLoadedHandles(), + $author_phids); $pinboard = new PhabricatorPinboardView(); - $pinboard->setNoDataString($nodata); foreach ($macros as $macro) { $file = $macro->getFile(); @@ -108,6 +50,14 @@ final class PhabricatorMacroListController 'div', array(), pht('Created on %s', $datetime))); + } else { + // Very old macros don't have a creation date. Rendering something + // keeps all the pins at the same height and avoids flow issues. + $item->appendChild( + phutil_tag( + 'div', + array(), + pht('Created in ages long past'))); } if ($macro->getAuthorPHID()) { @@ -117,45 +67,17 @@ final class PhabricatorMacroListController } $item->setURI($this->getApplicationURI('/view/'.$macro->getID().'/')); - $item->setHeader($macro->getName()); + + $name = $macro->getName(); + if ($macro->getIsDisabled()) { + $name = pht('%s (Disabled)', $name); + } + $item->setHeader($name); $pinboard->addItem($item); } - $nav->appendChild($pinboard); - if (!$has_search) { - $nav->appendChild($pager); - switch ($this->filter) { - case 'all': - $name = pht('All Macros'); - break; - case 'my': - $name = pht('My Macros'); - break; - case 'active': - $name = pht('Active Macros'); - break; - default: - throw new Exception("Unknown filter $this->filter"); - break; - } - } else { - $name = pht('Search'); - } + return $pinboard; - $crumbs = $this->buildApplicationCrumbs(); - $crumbs->addCrumb( - id(new PhabricatorCrumbView()) - ->setName($name) - ->setHref($request->getRequestURI())); - $nav->setCrumbs($crumbs); - - return $this->buildApplicationPage( - $nav, - array( - 'device' => true, - 'title' => pht('Image Macros'), - 'dust' => true, - )); } } diff --git a/src/applications/macro/query/PhabricatorMacroQuery.php b/src/applications/macro/query/PhabricatorMacroQuery.php index 894450f5b1..afab94e07f 100644 --- a/src/applications/macro/query/PhabricatorMacroQuery.php +++ b/src/applications/macro/query/PhabricatorMacroQuery.php @@ -15,6 +15,15 @@ final class PhabricatorMacroQuery private $status = 'status-any'; const STATUS_ANY = 'status-any'; const STATUS_ACTIVE = 'status-active'; + const STATUS_DISABLED = 'status-disabled'; + + public static function getStatusOptions() { + return array( + self::STATUS_ACTIVE => pht('Active Macros'), + self::STATUS_DISABLED => pht('Disabled Macros'), + self::STATUS_ANY => pht('Active and Disabled Macros'), + ); + } public function withIDs(array $ids) { $this->ids = $ids; @@ -99,10 +108,21 @@ final class PhabricatorMacroQuery $this->names); } - if ($this->status == self::STATUS_ACTIVE) { - $where[] = qsprintf( - $conn, - 'm.isDisabled = 0'); + switch ($this->status) { + case self::STATUS_ACTIVE: + $where[] = qsprintf( + $conn, + 'm.isDisabled = 0'); + break; + case self::STATUS_DISABLED: + $where[] = qsprintf( + $conn, + 'm.isDisabled = 1'); + break; + case self::STATUS_ANY: + break; + default: + throw new Exception("Unknown status '{$this->status}'!"); } $where[] = $this->buildPagingClause($conn); diff --git a/src/applications/macro/query/PhabricatorMacroSearchEngine.php b/src/applications/macro/query/PhabricatorMacroSearchEngine.php new file mode 100644 index 0000000000..e070c58307 --- /dev/null +++ b/src/applications/macro/query/PhabricatorMacroSearchEngine.php @@ -0,0 +1,122 @@ +setParameter( + 'authorPHIDs', + array_values($request->getArr('authors'))); + + $saved->setParameter('status', $request->getStr('status')); + $saved->setParameter('names', $request->getStrList('names')); + $saved->setParameter('nameLike', $request->getStr('nameLike')); + + return $saved; + } + + public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { + $query = id(new PhabricatorMacroQuery()) + ->withIDs($saved->getParameter('ids', array())) + ->withPHIDs($saved->getParameter('phids', array())) + ->withAuthorPHIDs($saved->getParameter('authorPHIDs', array())); + + $status = $saved->getParameter('status'); + $options = PhabricatorMacroQuery::getStatusOptions(); + if (empty($options[$status])) { + $status = head_key($options); + } + $query->withStatus($status); + + $names = $saved->getParameter('names', array()); + if ($names) { + $query->withNames($names); + } + + $like = $saved->getParameter('nameLike'); + if (strlen($like)) { + $query->withNameLike($like); + } + + return $query; + } + + public function buildSearchForm( + AphrontFormView $form, + PhabricatorSavedQuery $saved_query) { + + $phids = $saved_query->getParameter('authorPHIDs', array()); + $handles = id(new PhabricatorObjectHandleData($phids)) + ->setViewer($this->requireViewer()) + ->loadHandles(); + $author_tokens = mpull($handles, 'getFullName', 'getPHID'); + + $status = $saved_query->getParameter('status'); + $names = implode(', ', $saved_query->getParameter('names', array())); + $like = $saved_query->getParameter('nameLike'); + + $form + ->appendChild( + id(new AphrontFormSelectControl()) + ->setName('status') + ->setLabel(pht('Status')) + ->setOptions(PhabricatorMacroQuery::getStatusOptions()) + ->setValue($status)) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setDatasource('/typeahead/common/users/') + ->setName('authors') + ->setLabel(pht('Authors')) + ->setValue($author_tokens)) + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('nameLike') + ->setLabel(pht('Name Contains')) + ->setValue($like)) + ->appendChild( + id(new AphrontFormTextControl()) + ->setName('names') + ->setLabel(pht('Exact Names')) + ->setValue($names)); + } + + protected function getURI($path) { + return '/macro/'.$path; + } + + public function getBuiltinQueryNames() { + $names = array( + 'active' => pht('Active'), + 'all' => pht('All'), + ); + + if ($this->requireViewer()->isLoggedIn()) { + $names['authored'] = pht('Authored'); + } + + return $names; + } + + public function buildSavedQueryFromBuiltin($query_key) { + + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + switch ($query_key) { + case 'active': + return $query; + case 'all': + return $query->setParameter( + 'status', + PhabricatorMacroQuery::STATUS_ANY); + case 'authored': + return $query->setParameter( + 'authorPHIDs', + array($this->requireViewer()->getPHID())); + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + +} diff --git a/src/applications/paste/query/PhabricatorPasteSearchEngine.php b/src/applications/paste/query/PhabricatorPasteSearchEngine.php index 737a65e7e6..efed23a171 100644 --- a/src/applications/paste/query/PhabricatorPasteSearchEngine.php +++ b/src/applications/paste/query/PhabricatorPasteSearchEngine.php @@ -20,14 +20,6 @@ final class PhabricatorPasteSearchEngine 'authorPHIDs', array_values($request->getArr('authors'))); - try { - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $saved->save(); - unset($unguarded); - } catch (AphrontQueryDuplicateKeyException $ex) { - // Ignore, this is just a repeated search. - } - return $saved; } @@ -39,6 +31,7 @@ final class PhabricatorPasteSearchEngine */ public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { $query = id(new PhabricatorPasteQuery()) + ->needContent(true) ->withIDs($saved->getParameter('ids', array())) ->withPHIDs($saved->getParameter('phids', array())) ->withAuthorPHIDs($saved->getParameter('authorPHIDs', array())) diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index f8f29bb1a7..ded01539d6 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -84,9 +84,10 @@ final class PhabricatorApplicationSearchController $nav = $this->getNavigation(); if ($request->isFormPost()) { + $saved_query = $engine->buildSavedQueryFromRequest($request); + $this->saveQuery($saved_query); return id(new AphrontRedirectResponse())->setURI( - $engine->getQueryResultsPageURI( - $engine->buildSavedQueryFromRequest($request)->getQueryKey())); + $engine->getQueryResultsPageURI($saved_query->getQueryKey())); } $named_query = null; @@ -163,20 +164,24 @@ final class PhabricatorApplicationSearchController $nav->appendChild($filter_view); if ($run_query) { - $query = id(new PhabricatorPasteSearchEngine()) - ->buildQueryFromSavedQuery($saved_query); + $query = $engine->buildQueryFromSavedQuery($saved_query); $pager = new AphrontCursorPagerView(); $pager->readFromRequest($request); - $pastes = $query->setViewer($request->getUser()) - ->needContent(true) + $objects = $query->setViewer($request->getUser()) ->executeWithCursorPager($pager); - $list = $parent->renderResultsList($pastes); - $list->setPager($pager); + $list = $parent->renderResultsList($objects); $list->setNoDataString(pht("No results found for this query.")); $nav->appendChild($list); + + // TODO: This is a bit hacky. + if ($list instanceof PhabricatorObjectItemListView) { + $list->setPager($pager); + } else { + $nav->appendChild($pager); + } } if ($named_query) { @@ -269,4 +274,17 @@ final class PhabricatorApplicationSearchController 'dust' => true, )); } + + private function saveQuery(PhabricatorSavedQuery $query) { + $query->setEngineClassName(get_class($this->getSearchEngine())); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + try { + $query->save(); + } catch (AphrontQueryDuplicateKeyException $ex) { + // Ignore, this is just a repeated search. + } + unset($unguarded); + } + } diff --git a/src/applications/search/storage/PhabricatorSavedQuery.php b/src/applications/search/storage/PhabricatorSavedQuery.php index 45da12fe46..2421eae11a 100644 --- a/src/applications/search/storage/PhabricatorSavedQuery.php +++ b/src/applications/search/storage/PhabricatorSavedQuery.php @@ -7,14 +7,15 @@ final class PhabricatorSavedQuery extends PhabricatorSearchDAO implements PhabricatorPolicyInterface { protected $parameters = array(); - protected $queryKey = ""; - protected $engineClassName = "PhabricatorPasteSearchEngine"; + protected $queryKey; + protected $engineClassName; public function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( - 'parameters' => self::SERIALIZATION_JSON), ) - + parent::getConfiguration(); + 'parameters' => self::SERIALIZATION_JSON, + ), + ) + parent::getConfiguration(); } public function setParameter($key, $value) {