From 6e4f508bebdbb9fb2864747b3b2c0e9366443d48 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 12 Apr 2015 15:16:55 -0700 Subject: [PATCH] Provide "builtin" high-level result orders Summary: Ref T7803. Currently, available high-level orders are spread across Query and SearchEngine classes and implemented separately for each application. Lift the concept of "builtin" (high-level, user-facing, named) orders (similar to "builtin" queries in ApplicationSearch) into the root Query class, and let it drive the SearchEngine implementation. This allows you to define a new order in one place and have it automatically work across the entire stack. This will also let Conduit expose this information in a straightforward way. Test Plan: - Used ApplicationSearch in Diffusion. - Used all result orderings. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7803 Differential Revision: https://secure.phabricator.com/D12379 --- .../PhabricatorOwnersListController.php | 2 +- .../ponder/query/PonderQuestionQuery.php | 22 +-- .../query/PhabricatorRepositoryQuery.php | 46 +++--- .../PhabricatorRepositorySearchEngine.php | 37 +---- .../PhabricatorApplicationSearchEngine.php | 19 +++ ...PhabricatorCursorPagedPolicyAwareQuery.php | 139 +++++++++++++++++- 6 files changed, 184 insertions(+), 81 deletions(-) diff --git a/src/applications/owners/controller/PhabricatorOwnersListController.php b/src/applications/owners/controller/PhabricatorOwnersListController.php index 8a3a1e8ec3..ef7b01ebfb 100644 --- a/src/applications/owners/controller/PhabricatorOwnersListController.php +++ b/src/applications/owners/controller/PhabricatorOwnersListController.php @@ -153,7 +153,7 @@ final class PhabricatorOwnersListController $callsigns = array('' => pht('(Any Repository)')); $repositories = id(new PhabricatorRepositoryQuery()) ->setViewer($user) - ->setOrder(PhabricatorRepositoryQuery::ORDER_CALLSIGN) + ->setOrder('callsign') ->execute(); foreach ($repositories as $repository) { $callsigns[$repository->getCallsign()] = diff --git a/src/applications/ponder/query/PonderQuestionQuery.php b/src/applications/ponder/query/PonderQuestionQuery.php index bd387b62f9..9cb8c81d40 100644 --- a/src/applications/ponder/query/PonderQuestionQuery.php +++ b/src/applications/ponder/query/PonderQuestionQuery.php @@ -3,14 +3,10 @@ final class PonderQuestionQuery extends PhabricatorCursorPagedPolicyAwareQuery { - const ORDER_CREATED = 'order-created'; - const ORDER_HOTTEST = 'order-hottest'; - private $ids; private $phids; private $authorPHIDs; private $answererPHIDs; - private $order = self::ORDER_CREATED; private $status = 'status-any'; const STATUS_ANY = 'status-any'; @@ -55,11 +51,6 @@ final class PonderQuestionQuery return $this; } - public function setOrder($order) { - $this->order = $order; - return $this; - } - private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); @@ -110,17 +101,6 @@ final class PonderQuestionQuery return $this->formatWhereClause($where); } - private function buildOrderByClause(AphrontDatabaseConnection $conn_r) { - switch ($this->order) { - case self::ORDER_HOTTEST: - return qsprintf($conn_r, 'ORDER BY q.heat DESC, q.id DESC'); - case self::ORDER_CREATED: - return qsprintf($conn_r, 'ORDER BY q.id DESC'); - default: - throw new Exception("Unknown order '{$this->order}'!"); - } - } - protected function loadPage() { $question = new PonderQuestion(); $conn_r = $question->establishConnection('r'); @@ -131,7 +111,7 @@ final class PonderQuestionQuery $question->getTableName(), $this->buildJoinsClause($conn_r), $this->buildWhereClause($conn_r), - $this->buildOrderByClause($conn_r), + $this->buildOrderClause($conn_r), $this->buildLimitClause($conn_r)); return $question->loadAllFromArray($data); diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index 05d5bba58a..385ad37eaf 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -23,12 +23,6 @@ final class PhabricatorRepositoryQuery const STATUS_ALL = 'status-all'; private $status = self::STATUS_ALL; - const ORDER_CREATED = 'order-created'; - const ORDER_COMMITTED = 'order-committed'; - const ORDER_CALLSIGN = 'order-callsign'; - const ORDER_NAME = 'order-name'; - const ORDER_SIZE = 'order-size'; - const HOSTED_PHABRICATOR = 'hosted-phab'; const HOSTED_REMOTE = 'hosted-remote'; const HOSTED_ALL = 'hosted-all'; @@ -124,27 +118,25 @@ final class PhabricatorRepositoryQuery return $this; } - public function setOrder($order) { - switch ($order) { - case self::ORDER_CREATED: - $this->setOrderVector(array('id')); - break; - case self::ORDER_COMMITTED: - $this->setOrderVector(array('committed', 'id')); - break; - case self::ORDER_CALLSIGN: - $this->setOrderVector(array('callsign')); - break; - case self::ORDER_NAME: - $this->setOrderVector(array('name', 'id')); - break; - case self::ORDER_SIZE: - $this->setOrderVector(array('size', 'id')); - break; - default: - throw new Exception(pht('Unknown order "%s".', $order)); - } - return $this; + public function getBuiltinOrders() { + return array( + 'committed' => array( + 'vector' => array('committed', 'id'), + 'name' => pht('Most Recent Commit'), + ), + 'name' => array( + 'vector' => array('name', 'id'), + 'name' => pht('Name'), + ), + 'callsign' => array( + 'vector' => array('callsign'), + 'name' => pht('Callsign'), + ), + 'size' => array( + 'vector' => array('size', 'id'), + 'name' => pht('Size'), + ), + ) + parent::getBuiltinOrders(); } public function getIdentifierMap() { diff --git a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php index 103f07c87f..1b8009fec5 100644 --- a/src/applications/repository/query/PhabricatorRepositorySearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositorySearchEngine.php @@ -27,6 +27,7 @@ final class PhabricatorRepositorySearchEngine public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { $query = id(new PhabricatorRepositoryQuery()) + ->setDefaultBuiltinOrder() ->needProjectPHIDs(true) ->needCommitCounts(true) ->needMostRecentCommits(true); @@ -43,11 +44,8 @@ final class PhabricatorRepositorySearchEngine } $order = $saved->getParameter('order'); - $order = idx($this->getOrderValues(), $order); if ($order) { $query->setOrder($order); - } else { - $query->setOrder(head($this->getOrderValues())); } $hosted = $saved->getParameter('hosted'); @@ -125,15 +123,12 @@ final class PhabricatorRepositorySearchEngine $name, isset($types[$key])); } + $form->appendChild($type_control); - $form - ->appendChild($type_control) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setName('order') - ->setLabel(pht('Order')) - ->setValue($saved_query->getParameter('order')) - ->setOptions($this->getOrderOptions())); + $this->appendOrderFieldsToForm( + $form, + $saved_query, + new PhabricatorRepositoryQuery()); } protected function getURI($path) { @@ -180,26 +175,6 @@ final class PhabricatorRepositorySearchEngine ); } - private function getOrderOptions() { - return array( - 'committed' => pht('Most Recent Commit'), - 'name' => pht('Name'), - 'callsign' => pht('Callsign'), - 'created' => pht('Date Created'), - 'size' => pht('Commit Count'), - ); - } - - private function getOrderValues() { - return array( - 'committed' => PhabricatorRepositoryQuery::ORDER_COMMITTED, - 'name' => PhabricatorRepositoryQuery::ORDER_NAME, - 'callsign' => PhabricatorRepositoryQuery::ORDER_CALLSIGN, - 'created' => PhabricatorRepositoryQuery::ORDER_CREATED, - 'size' => PhabricatorRepositoryQuery::ORDER_SIZE, - ); - } - private function getHostedOptions() { return array( '' => pht('Hosted and Remote Repositories'), diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 109bec8533..dec842f1f8 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -9,6 +9,7 @@ * @task builtin Builtin Queries * @task uri Query URIs * @task dates Date Filters + * @task order Result Ordering * @task read Reading Utilities * @task exec Paging and Executing Queries * @task render Rendering Results @@ -577,6 +578,24 @@ abstract class PhabricatorApplicationSearchEngine { } +/* -( Result Ordering )---------------------------------------------------- */ + + protected function appendOrderFieldsToForm( + AphrontFormView $form, + PhabricatorSavedQuery $saved, + PhabricatorCursorPagedPolicyAwareQuery $query) { + + $orders = $query->getBuiltinOrders(); + $orders = ipull($orders, 'name'); + + $form->appendControl( + id(new AphrontFormSelectControl()) + ->setLabel(pht('Order')) + ->setName('order') + ->setOptions($orders) + ->setValue($saved->getParameter('order'))); + } + /* -( Paging and Executing Queries )--------------------------------------- */ diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index b98648529b..068229c9ef 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -17,6 +17,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery protected $applicationSearchOrders = array(); private $internalPaging; private $orderVector; + private $builtinOrder; protected function getPagingValue($result) { if (!is_object($result)) { @@ -427,6 +428,134 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery /** + * Select a result ordering. + * + * This is a high-level method which selects an ordering from a predefined + * list of builtin orders, as provided by @{method:getBuiltinOrders}. These + * options are user-facing and not exhaustive, but are generally convenient + * and meaningful. + * + * You can also use @{method:setOrderVector} to specify a low-level ordering + * across individual orderable columns. This offers greater control but is + * also more involved. + * + * @param string Key of a builtin order supported by this query. + * @return this + * @task order + */ + public function setOrder($order) { + $orders = $this->getBuiltinOrders(); + + if (empty($orders[$order])) { + throw new Exception( + pht( + 'Query "%s" does not support a builtin order "%s". Supported orders '. + 'are: %s.', + get_class($this), + $order, + implode(', ', array_keys($orders)))); + } + + $this->builtinOrder = $order; + $this->orderVector = null; + + return $this; + } + + + /** + * Select the default builtin result ordering. + * + * This sets the result order to the default order among the builtin result + * orders (see @{method:getBuiltinOrders}). This is often the same as the + * query's builtin default order vector, but some objects have different + * default vectors (which are internally-facing) and builtin orders (which + * are user-facing). + * + * For example, repositories sort by ID internally (which is efficient and + * consistent), but sort by most recent commit as a default builtin (which + * better aligns with user expectations). + * + * @return this + */ + public function setDefaultBuiltinOrder() { + return $this->setOrder(head_key($this->getBuiltinOrders())); + } + + + /** + * Get builtin orders for this class. + * + * In application UIs, we want to be able to present users with a small + * selection of meaningful order options (like "Order by Title") rather than + * an exhaustive set of column ordering options. + * + * Meaningful user-facing orders are often really orders across multiple + * columns: for example, a "title" ordering is usually implemented as a + * "title, id" ordering under the hood. + * + * Builtin orders provide a mapping from convenient, understandable + * user-facing orders to implementations. + * + * A builtin order should provide these keys: + * + * - `vector` (`list`): The actual order vector to use. + * - `name` (`string`): Human-readable order name. + * + * @return map Map from builtin order keys to specification. + * @task order + */ + public function getBuiltinOrders() { + $orders = array( + 'newest' => array( + 'vector' => array('id'), + 'name' => pht('Creation (Newest First)'), + 'aliases' => array('created'), + ), + 'oldest' => array( + 'vector' => array('-id'), + 'name' => pht('Creation (Oldest First)'), + ), + ); + + $object = $this->newResultObject(); + if ($object instanceof PhabricatorCustomFieldInterface) { + $list = PhabricatorCustomField::getObjectFields( + $object, + PhabricatorCustomField::ROLE_APPLICATIONSEARCH); + foreach ($list->getFields() as $field) { + $index = $field->buildOrderIndex(); + if (!$index) { + continue; + } + + $key = $field->getFieldKey(); + $digest = $field->getFieldIndex(); + + $full_key = 'custom:'.$key; + $orders[$full_key] = array( + 'vector' => array($full_key, 'id'), + 'name' => $field->getFieldName(), + ); + } + } + + return $orders; + } + + + /** + * Set a low-level column ordering. + * + * This is a low-level method which offers granular control over column + * ordering. In most cases, applications can more easily use + * @{method:setOrder} to choose a high-level builtin order. + * + * To set an order vector, specify a list of order keys as provided by + * @{method:getOrderableColumns}. + * + * @param PhabricatorQueryOrderVector|list List of order keys. + * @return this * @task order */ public function setOrderVector($vector) { @@ -485,11 +614,19 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery /** + * Get the effective order vector. + * + * @return PhabricatorQueryOrderVector Effective vector. * @task order */ protected function getOrderVector() { if (!$this->orderVector) { - $vector = $this->getDefaultOrderVector(); + if ($this->builtinOrder !== null) { + $builtin_order = idx($this->getBuiltinOrders(), $this->builtinOrder); + $vector = $builtin_order['vector']; + } else { + $vector = $this->getDefaultOrderVector(); + } $vector = PhabricatorQueryOrderVector::newFromVector($vector); // We call setOrderVector() here to apply checks to the default vector.