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

@@ -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',

src/applications/macro/application/PhabricatorApplicationMacro.php

@@ -25,7 +25,7 @@ final class PhabricatorApplicationMacro extends PhabricatorApplication {
   public function getRoutes() {
     return array(
       '/macro/' => array(
-        '((?P<filter>all|active|my)/)?' => 'PhabricatorMacroListController',
+        '(query/(?P<key>[^/]+)/)?' => 'PhabricatorMacroListController',
         'create/' => 'PhabricatorMacroEditController',
         'view/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroViewController',
         'comment/(?P<id>[1-9]\d*)/' => 'PhabricatorMacroCommentController',

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'));
+    id(new PhabricatorMacroSearchEngine())
-    $nav->addFilter('active', pht('Active Macros'));
+      ->setViewer($this->getRequest()->getUser())
-    $nav->addFilter('all', pht('All Macros'));
+      ->addNavigationItems($nav);
-    $nav->addFilter('my', pht('My Macros'));
-    if ($has_search) {
-      $nav->addFilter('search',
-        pht('Search'),
-        $this->getRequest()->getRequestURI());
-    }
-
 
     return $nav;
   }

src/applications/macro/controller/PhabricatorMacroListController.php

@@ -1,97 +1,39 @@
 <?php
 
-final class PhabricatorMacroListController
+final class PhabricatorMacroListController extends PhabricatorMacroController
-  extends PhabricatorMacroController {
+  implements PhabricatorApplicationSearchResultsControllerInterface {
 
-  private $filter;
+  private $key;
+
+  public function shouldAllowPublic() {
+    return true;
+  }
 
   public function willProcessRequest(array $data) {
-    $this->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())
+    return $this->delegateToController($controller);
-      ->readFromRequest($request);
-
-    $query = new PhabricatorMacroQuery();
-    $query->setViewer($viewer);
-
-    $filter = $request->getStr('name');
-    if (strlen($filter)) {
-      $query->withNameLike($filter);
   }
 
-    $authors = $request->getArr('authors');
+  public function renderResultsList(array $macros) {
-
+    assert_instances_of($macros, 'PhabricatorFileImageMacro');
-    if ($authors) {
+    $viewer = $this->getRequest()->getUser();
-      $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');
 
+    $author_phids = mpull($macros, 'getAuthorPHID', 'getAuthorPHID');
     $this->loadHandles($author_phids);
-    $author_handles = array_select_keys($this->getLoadedHandles(), $authors);
+    $author_handles = array_select_keys(
-
+      $this->getLoadedHandles(),
-    $filter_form = id(new AphrontFormView())
+      $author_phids);
-      ->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);
 
     $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) {
+    return $pinboard;
-      $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');
-    }
 
-    $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,
-      ));
   }
 }

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) {
+    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);

src/applications/macro/query/PhabricatorMacroSearchEngine.php

@@ -0,0 +1,122 @@
+<?php
+
+final class PhabricatorMacroSearchEngine
+  extends PhabricatorApplicationSearchEngine {
+
+  public function buildSavedQueryFromRequest(AphrontRequest $request) {
+    $saved = new PhabricatorSavedQuery();
+    $saved->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);
+  }
+
+}

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()))

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->getQueryResultsPageURI($saved_query->getQueryKey()));
-          $engine->buildSavedQueryFromRequest($request)->getQueryKey()));
     }
 
     $named_query = null;
@@ -163,20 +164,24 @@ final class PhabricatorApplicationSearchController
     $nav->appendChild($filter_view);
 
     if ($run_query) {
-      $query = id(new PhabricatorPasteSearchEngine())
+      $query = $engine->buildQueryFromSavedQuery($saved_query);
-        ->buildQueryFromSavedQuery($saved_query);
 
       $pager = new AphrontCursorPagerView();
       $pager->readFromRequest($request);
-      $pastes = $query->setViewer($request->getUser())
+      $objects = $query->setViewer($request->getUser())
-        ->needContent(true)
         ->executeWithCursorPager($pager);
 
-      $list = $parent->renderResultsList($pastes);
+      $list = $parent->renderResultsList($objects);
-      $list->setPager($pager);
       $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);
+  }
+
 }

src/applications/search/storage/PhabricatorSavedQuery.php

@@ -7,14 +7,15 @@ final class PhabricatorSavedQuery extends PhabricatorSearchDAO
   implements PhabricatorPolicyInterface {
 
   protected $parameters = array();
-  protected $queryKey = "";
+  protected $queryKey;
-  protected $engineClassName = "PhabricatorPasteSearchEngine";
+  protected $engineClassName;
 
   public function getConfiguration() {
     return array(
       self::CONFIG_SERIALIZATION => array(
-        'parameters' => self::SERIALIZATION_JSON), )
+        'parameters' => self::SERIALIZATION_JSON,
-      + parent::getConfiguration();
+      ),
+    ) + parent::getConfiguration();
   }
 
   public function setParameter($key, $value) {