Support custom fields in "Order By" for Maniphest

Summary: Resolves T4659. This implements support for sorting tasks by custom fields. Some of this feels hacky in the way it's hooked up to the Maniphest search engine and task query. Test Plan: Queryed on a custom date field, with a small page size, and moved back and forth through the result set. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T4659 Differential Revision: https://secure.phabricator.com/D10106
2024-11-10 08:52:39 +01:00 · 2014-08-02 18:22:16 +10:00 · 2014-08-02 18:22:16 +10:00 · 46b4fa85d0
commit 46b4fa85d0
parent 950eeef4c0
9 changed files with 290 additions and 74 deletions
--- a/src/applications/maniphest/query/ManiphestTaskQuery.php
+++ b/src/applications/maniphest/query/ManiphestTaskQuery.php
@ -572,6 +572,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
  }

  private function buildCustomOrderClause(AphrontDatabaseConnection $conn) {
+    $reverse = ($this->getBeforeID() xor $this->getReversePaging());
+
    $order = array();

    switch ($this->groupBy) {
@ -593,33 +595,35 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
        throw new Exception("Unknown group query '{$this->groupBy}'!");
    }

-    switch ($this->orderBy) {
-      case self::ORDER_PRIORITY:
-        $order[] = 'priority';
-        $order[] = 'subpriority';
-        $order[] = 'dateModified';
-        break;
-      case self::ORDER_CREATED:
-        $order[] = 'id';
-        break;
-      case self::ORDER_MODIFIED:
-        $order[] = 'dateModified';
-        break;
-      case self::ORDER_TITLE:
-        $order[] = 'title';
-        break;
-      default:
-        throw new Exception("Unknown order query '{$this->orderBy}'!");
+    $app_order = $this->buildApplicationSearchOrders($conn, $reverse);
+
+    if (!$app_order) {
+      switch ($this->orderBy) {
+        case self::ORDER_PRIORITY:
+          $order[] = 'priority';
+          $order[] = 'subpriority';
+          $order[] = 'dateModified';
+          break;
+        case self::ORDER_CREATED:
+          $order[] = 'id';
+          break;
+        case self::ORDER_MODIFIED:
+          $order[] = 'dateModified';
+          break;
+        case self::ORDER_TITLE:
+          $order[] = 'title';
+          break;
+        default:
+          throw new Exception("Unknown order query '{$this->orderBy}'!");
+      }
    }

    $order = array_unique($order);

-    if (empty($order)) {
+    if (empty($order) && empty($app_order)) {
      return null;
    }

-    $reverse = ($this->getBeforeID() xor $this->getReversePaging());
-
    foreach ($order as $k => $column) {
      switch ($column) {
        case 'subpriority':
@ -653,6 +657,18 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
      }
    }

+    if ($app_order) {
+      foreach ($app_order as $order_by) {
+        $order[] = $order_by;
+      }
+
+      if ($reverse) {
+        $order[] = 'task.id ASC';
+      } else {
+        $order[] = 'task.id DESC';
+      }
+    }
+
    return 'ORDER BY '.implode(', ', $order);
  }

@ -903,55 +919,65 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
        throw new Exception("Unknown group query '{$this->groupBy}'!");
    }

-    switch ($this->orderBy) {
-      case self::ORDER_PRIORITY:
-        if ($this->groupBy != self::GROUP_PRIORITY) {
+    $app_columns = $this->buildApplicationSearchPagination($conn_r, $cursor);
+    if ($app_columns) {
+      $columns = array_merge($columns, $app_columns);
+      $columns[] = array(
+        'name' => 'task.id',
+        'value' => (int)$cursor->getID(),
+        'type' => 'int',
+      );
+    } else {
+      switch ($this->orderBy) {
+        case self::ORDER_PRIORITY:
+          if ($this->groupBy != self::GROUP_PRIORITY) {
+            $columns[] = array(
+              'name' => 'task.priority',
+              'value' => (int)$cursor->getPriority(),
+              'type' => 'int',
+            );
+          }
          $columns[] = array(
-            'name' => 'task.priority',
-            'value' => (int)$cursor->getPriority(),
+            'name' => 'task.subpriority',
+            'value' => (int)$cursor->getSubpriority(),
+            'type' => 'int',
+            'reverse' => true,
+          );
+          $columns[] = array(
+            'name' => 'task.dateModified',
+            'value' => (int)$cursor->getDateModified(),
            'type' => 'int',
          );
-        }
-        $columns[] = array(
-          'name' => 'task.subpriority',
-          'value' => (int)$cursor->getSubpriority(),
-          'type' => 'int',
-          'reverse' => true,
-        );
-        $columns[] = array(
-          'name' => 'task.dateModified',
-          'value' => (int)$cursor->getDateModified(),
-          'type' => 'int',
-        );
-        break;
-      case self::ORDER_CREATED:
-        $columns[] = array(
-          'name' => 'task.id',
-          'value' => (int)$cursor->getID(),
-          'type' => 'int',
-        );
-        break;
-      case self::ORDER_MODIFIED:
-        $columns[] = array(
-          'name' => 'task.dateModified',
-          'value' => (int)$cursor->getDateModified(),
-          'type' => 'int',
-        );
-        break;
-      case self::ORDER_TITLE:
-        $columns[] = array(
-          'name' => 'task.title',
-          'value' => $cursor->getTitle(),
-          'type' => 'string',
-        );
-        $columns[] = array(
-          'name' => 'task.id',
-          'value' => $cursor->getID(),
-          'type' => 'int',
-        );
-        break;
-      default:
-        throw new Exception("Unknown order query '{$this->orderBy}'!");
+          break;
+        case self::ORDER_CREATED:
+          $columns[] = array(
+            'name' => 'task.id',
+            'value' => (int)$cursor->getID(),
+            'type' => 'int',
+          );
+          break;
+        case self::ORDER_MODIFIED:
+          $columns[] = array(
+            'name' => 'task.dateModified',
+            'value' => (int)$cursor->getDateModified(),
+            'type' => 'int',
+          );
+          break;
+        case self::ORDER_TITLE:
+          $columns[] = array(
+            'name' => 'task.title',
+            'value' => $cursor->getTitle(),
+            'type' => 'string',
+          );
+          $columns[] = array(
+            'name' => 'task.id',
+            'value' => $cursor->getID(),
+            'type' => 'int',
+          );
+          break;
+        default:
+          throw new Exception("Unknown order query '{$this->orderBy}'!");
+      }
    }

    return $this->buildPagingClauseFromMultipleColumns(
--- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php
+++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php
@ -151,13 +151,10 @@ final class ManiphestTaskSearchEngine
      $query->withPriorities($priorities);
    }

-    $order = $saved->getParameter('order');
-    $order = idx($this->getOrderValues(), $order);
-    if ($order) {
-      $query->setOrderBy($order);
-    } else {
-      $query->setOrderBy(head($this->getOrderValues()));
-    }
+    $this->applyOrderByToQuery(
+      $query,
+      $this->getOrderValues(),
+      $saved->getParameter('order'));

    $group = $saved->getParameter('group');
    $group = idx($this->getGroupValues(), $group);
@ -306,6 +303,10 @@ final class ManiphestTaskSearchEngine

    $ids = $saved->getParameter('ids', array());

+    $builtin_orders = $this->getOrderOptions();
+    $custom_orders = $this->getCustomFieldOrderOptions();
+    $all_orders = $builtin_orders + $custom_orders;
+
    $form
      ->appendChild(
        id(new AphrontFormTokenizerControl())
@ -385,7 +386,7 @@ final class ManiphestTaskSearchEngine
            ->setName('order')
            ->setLabel(pht('Order By'))
            ->setValue($saved->getParameter('order'))
-            ->setOptions($this->getOrderOptions()));
+            ->setOptions($all_orders));
    }

    $form
--- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
+++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php
@ -764,6 +764,65 @@ abstract class PhabricatorApplicationSearchEngine {
    }
  }

+  protected function applyOrderByToQuery(
+    PhabricatorCursorPagedPolicyAwareQuery $query,
+    array $standard_values,
+    $order) {
+
+    if (substr($order, 0, 7) === 'custom:') {
+      $list = $this->getCustomFieldList();
+      if (!$list) {
+        $query->setOrderBy(head($standard_values));
+        return;
+      }
+
+      foreach ($list->getFields() as $field) {
+        $key = $this->getKeyForCustomField($field);
+
+        if ($key === $order) {
+          $index = $field->buildOrderIndex();
+
+          if ($index === null) {
+            $query->setOrderBy(head($standard_values));
+            return;
+          }
+
+          $query->withApplicationSearchOrder(
+            $field,
+            $index,
+            false);
+          break;
+        }
+      }
+    } else {
+      $order = idx($standard_values, $order);
+      if ($order) {
+        $query->setOrderBy($order);
+      } else {
+        $query->setOrderBy(head($standard_values));
+      }
+    }
+  }
+
+
+  protected function getCustomFieldOrderOptions() {
+    $list = $this->getCustomFieldList();
+    if (!$list) {
+      return;
+    }
+
+    $custom_order = array();
+    foreach ($list->getFields() as $field) {
+      if ($field->shouldAppearInApplicationSearch()) {
+        if ($field->buildOrderIndex() !== null) {
+          $key = $this->getKeyForCustomField($field);
+          $custom_order[$key] = $field->getFieldName();
+        }
+      }
+    }
+
+    return $custom_order;
+  }

  /**
   * Get a unique key identifying a field.
--- a/src/infrastructure/customfield/field/PhabricatorCustomField.php
+++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php
@ -620,6 +620,28 @@ abstract class PhabricatorCustomField {
  }


+  /**
+   * Return an index against which this field can be meaningfully ordered
+   * against to implement ApplicationSearch.
+   *
+   * This should be a single index, normally built using
+   * @{method:newStringIndex} and @{method:newNumericIndex}.
+   *
+   * The value of the index is not used.
+   *
+   * Return null from this method if the field can not be ordered.
+   *
+   * @return PhabricatorCustomFieldIndexStorage A single index to order by.
+   * @task appsearch
+   */
+  public function buildOrderIndex() {
+    if ($this->proxy) {
+      return $this->proxy->buildOrderIndex();
+    }
+    return null;
+  }
+
+
  /**
   * Build a new empty storage object for storing string indexes. Normally,
   * this should be a concrete subclass of
--- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php
+++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php
@ -260,6 +260,10 @@ abstract class PhabricatorStandardCustomField
    return array();
  }

+  public function buildOrderIndex() {
+    return null;
+  }
+
  public function readApplicationSearchValueFromRequest(
    PhabricatorApplicationSearchEngine $engine,
    AphrontRequest $request) {
--- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php
+++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php
@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldBool
    return $indexes;
  }

+  public function buildOrderIndex() {
+    return $this->newNumericIndex(0);
+  }
+
  public function getValueForStorage() {
    $value = $this->getFieldValue();
    if (strlen($value)) {
--- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php
+++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php
@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldDate
    return $indexes;
  }

+  public function buildOrderIndex() {
+    return $this->newNumericIndex(0);
+  }
+
  public function getValueForStorage() {
    $value = $this->getFieldValue();
    if (strlen($value)) {
--- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php
+++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php
@ -18,6 +18,10 @@ final class PhabricatorStandardCustomFieldInt
    return $indexes;
  }

+  public function buildOrderIndex() {
+    return $this->newNumericIndex(0);
+  }
+
  public function getValueForStorage() {
    $value = $this->getFieldValue();
    if (strlen($value)) {
--- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
@ -12,6 +12,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
  private $afterID;
  private $beforeID;
  private $applicationSearchConstraints = array();
+  private $applicationSearchOrders = array();
  private $internalPaging;

  protected function getPagingColumn() {
@ -359,6 +360,32 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
  }


+  /**
+   * Order the results by an ApplicationSearch index.
+   *
+   * @param PhabricatorCustomField Field to which the index belongs.
+   * @param PhabricatorCustomFieldIndexStorage Table where the index is stored.
+   * @param bool True to sort ascending.
+   * @return this
+   * @task appsearch
+   */
+  public function withApplicationSearchOrder(
+    PhabricatorCustomField $field,
+    PhabricatorCustomFieldIndexStorage $index,
+    $ascending) {
+
+    $this->applicationSearchOrders[] = array(
+      'key' => $field->getFieldKey(),
+      'type' => $index->getIndexValueType(),
+      'table' => $index->getTableName(),
+      'index' => $index->getIndexKey(),
+      'ascending' => $ascending,
+    );
+
+    return $this;
+  }
+
+
  /**
   * Get the name of the query's primary object PHID column, for constructing
   * JOIN clauses. Normally (and by default) this is just `"phid"`, but if the
@ -535,7 +562,72 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
      }
    }

+    foreach ($this->applicationSearchOrders as $key => $order) {
+      $table = $order['table'];
+      $alias = 'appsearch_order_'.$key;
+      $index = $order['index'];
+      $phid_column = $this->getApplicationSearchObjectPHIDColumn();
+
+      $joins[] = qsprintf(
+        $conn_r,
+        'JOIN %T %T ON %T.objectPHID = %Q
+          AND %T.indexKey = %s',
+        $table,
+        $alias,
+        $alias,
+        $phid_column,
+        $alias,
+        $index);
+    }
+
    return implode(' ', $joins);
  }

+  protected function buildApplicationSearchOrders(
+    AphrontDatabaseConnection $conn_r,
+    $reverse) {
+
+    $orders = array();
+    foreach ($this->applicationSearchOrders as $key => $order) {
+      $alias = 'appsearch_order_'.$key;
+
+      if ($order['ascending'] xor $reverse) {
+        $orders[] = qsprintf($conn_r, '%T.indexValue ASC', $alias);
+      } else {
+        $orders[] = qsprintf($conn_r, '%T.indexValue DESC', $alias);
+      }
+    }
+
+    return $orders;
+  }
+
+  protected function buildApplicationSearchPagination(
+    AphrontDatabaseConnection $conn_r,
+    $cursor) {
+
+    // We have to get the current field values on the cursor object.
+    $fields = PhabricatorCustomField::getObjectFields(
+      $cursor,
+      PhabricatorCustomField::ROLE_APPLICATIONSEARCH);
+    $fields->setViewer($this->getViewer());
+    $fields->readFieldsFromStorage($cursor);
+
+    $fields = mpull($fields->getFields(), null, 'getFieldKey');
+
+    $columns = array();
+    foreach ($this->applicationSearchOrders as $key => $order) {
+      $alias = 'appsearch_order_'.$key;
+
+      $field = idx($fields, $order['key']);
+
+      $columns[] = array(
+        'name' => $alias.'.indexValue',
+        'value' => $field->getValueForStorage(),
+        'type' => $order['type'],
+      );
+    }
+
+    return $columns;
+  }
+
 }