Fix "Blocked" task queries with multiple subtasks, and update language

Summary:
Ref T8126. See that task for discussion. This change:

  - Updates language to be more consistent ("Parents", "Subtasks") since I moved us away from the often-confusing "Block" language in T4788.
  - Fixes bugs with finding the wrong set of tasks if tasks have a mixture of open and closed subtasks or parents.

Test Plan:
  - Created four tasks: no subtasks, one closed subtask, one open subtask, mixture of open and closed subtasks.
  - Created four more tasks: no parents, one closed parent, one open parent, mixture of open and closed parents.
  - Searched for all this stuff, got the proper results:

{F1740683}

{F1740684}

{F1740685}

{F1740686}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8126

Differential Revision: https://secure.phabricator.com/D16340

src/applications/maniphest/query/ManiphestTaskQuery.php

@@ -20,6 +20,8 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
   private $subpriorityMin;
   private $subpriorityMax;
   private $bridgedObjectPHIDs;
+  private $hasOpenParents;
+  private $hasOpenSubtasks;
 
   private $fullTextSearch   = '';
 
@@ -51,8 +53,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
 
   private $needSubscriberPHIDs;
   private $needProjectPHIDs;
-  private $blockingTasks;
-  private $blockedTasks;
 
   public function withAuthors(array $authors) {
     $this->authorPHIDs = $authors;
@@ -151,34 +151,16 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     return $this;
   }
 
-  /**
+  public function withOpenSubtasks($value) {
-   * True returns tasks that are blocking other tasks only.
+    $this->hasOpenSubtasks = $value;
-   * False returns tasks that are not blocking other tasks only.
-   * Null returns tasks regardless of blocking status.
-   */
-  public function withBlockingTasks($mode) {
-    $this->blockingTasks = $mode;
     return $this;
   }
 
-  public function shouldJoinBlockingTasks() {
+  public function withOpenParents($value) {
-    return $this->blockingTasks !== null;
+    $this->hasOpenParents = $value;
-  }
-
-  /**
-   * True returns tasks that are blocked by other tasks only.
-   * False returns tasks that are not blocked by other tasks only.
-   * Null returns tasks regardless of blocked by status.
-   */
-  public function withBlockedTasks($mode) {
-    $this->blockedTasks = $mode;
     return $this;
   }
 
-  public function shouldJoinBlockedTasks() {
-    return $this->blockedTasks !== null;
-  }
-
   public function withDateCreatedBefore($date_created_before) {
     $this->dateCreatedBefore = $date_created_before;
     return $this;
@@ -335,7 +317,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     $where = parent::buildWhereClauseParts($conn);
 
     $where[] = $this->buildStatusWhereClause($conn);
-    $where[] = $this->buildDependenciesWhereClause($conn);
     $where[] = $this->buildOwnerWhereClause($conn);
     $where[] = $this->buildFullTextWhereClause($conn);
 
@@ -526,71 +507,65 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
       $fulltext_results);
   }
 
-  private function buildDependenciesWhereClause(
+  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
-    AphrontDatabaseConnection $conn) {
+    $open_statuses = ManiphestTaskStatus::getOpenStatusConstants();
-
-    if (!$this->shouldJoinBlockedTasks() &&
-        !$this->shouldJoinBlockingTasks()) {
-      return null;
-    }
-
-    $parts = array();
-    if ($this->blockingTasks === true) {
-      $parts[] = qsprintf(
-        $conn,
-        'blocking.dst IS NOT NULL AND blockingtask.status IN (%Ls)',
-        ManiphestTaskStatus::getOpenStatusConstants());
-    } else if ($this->blockingTasks === false) {
-      $parts[] = qsprintf(
-        $conn,
-        'blocking.dst IS NULL OR blockingtask.status NOT IN (%Ls)',
-        ManiphestTaskStatus::getOpenStatusConstants());
-    }
-
-    if ($this->blockedTasks === true) {
-      $parts[] = qsprintf(
-        $conn,
-        'blocked.dst IS NOT NULL AND blockedtask.status IN (%Ls)',
-        ManiphestTaskStatus::getOpenStatusConstants());
-    } else if ($this->blockedTasks === false) {
-      $parts[] = qsprintf(
-        $conn,
-        'blocked.dst IS NULL OR blockedtask.status NOT IN (%Ls)',
-        ManiphestTaskStatus::getOpenStatusConstants());
-    }
-
-    return '('.implode(') OR (', $parts).')';
-  }
-
-  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn_r) {
     $edge_table = PhabricatorEdgeConfig::TABLE_NAME_EDGE;
+    $task_table = $this->newResultObject()->getTableName();
 
     $joins = array();
+    if ($this->hasOpenParents !== null) {
+      $parent_type = ManiphestTaskDependedOnByTaskEdgeType::EDGECONST;
+
+      if ($this->hasOpenParents) {
+        $join_type = 'JOIN';
+      } else {
+        $join_type = 'LEFT JOIN';
+      }
 
-    if ($this->shouldJoinBlockingTasks()) {
       $joins[] = qsprintf(
-        $conn_r,
+        $conn,
-        'LEFT JOIN %T blocking ON blocking.src = task.phid '.
+        '%Q %T e_parent
-        'AND blocking.type = %d '.
+          ON e_parent.src = task.phid
-        'LEFT JOIN %T blockingtask ON blocking.dst = blockingtask.phid',
+          AND e_parent.type = %d
+         %Q %T parent
+           ON e_parent.dst = parent.phid
+           AND parent.status IN (%Ls)',
+        $join_type,
         $edge_table,
-        ManiphestTaskDependedOnByTaskEdgeType::EDGECONST,
+        $parent_type,
-        id(new ManiphestTask())->getTableName());
+        $join_type,
+        $task_table,
+        $open_statuses);
     }
-    if ($this->shouldJoinBlockedTasks()) {
+
+    if ($this->hasOpenSubtasks !== null) {
+      $subtask_type = ManiphestTaskDependsOnTaskEdgeType::EDGECONST;
+
+      if ($this->hasOpenSubtasks) {
+        $join_type = 'JOIN';
+      } else {
+        $join_type = 'LEFT JOIN';
+      }
+
       $joins[] = qsprintf(
-        $conn_r,
+        $conn,
-        'LEFT JOIN %T blocked ON blocked.src = task.phid '.
+        '%Q %T e_subtask
-        'AND blocked.type = %d '.
+          ON e_subtask.src = task.phid
-        'LEFT JOIN %T blockedtask ON blocked.dst = blockedtask.phid',
+          AND e_subtask.type = %d
+         %Q %T subtask
+           ON e_subtask.dst = subtask.phid
+           AND subtask.status IN (%Ls)',
+        $join_type,
         $edge_table,
-        ManiphestTaskDependsOnTaskEdgeType::EDGECONST,
+        $subtask_type,
-        id(new ManiphestTask())->getTableName());
+        $join_type,
+        $task_table,
+        $open_statuses);
     }
 
     if ($this->subscriberPHIDs !== null) {
       $joins[] = qsprintf(
-        $conn_r,
+        $conn,
         'JOIN %T e_ccs ON e_ccs.src = task.phid '.
         'AND e_ccs.type = %s '.
         'AND e_ccs.dst in (%Ls)',
@@ -604,7 +579,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
         $ignore_group_phids = $this->getIgnoreGroupedProjectPHIDs();
         if ($ignore_group_phids) {
           $joins[] = qsprintf(
-            $conn_r,
+            $conn,
             'LEFT JOIN %T projectGroup ON task.phid = projectGroup.src
               AND projectGroup.type = %d
               AND projectGroup.dst NOT IN (%Ls)',
@@ -613,29 +588,30 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
             $ignore_group_phids);
         } else {
           $joins[] = qsprintf(
-            $conn_r,
+            $conn,
             'LEFT JOIN %T projectGroup ON task.phid = projectGroup.src
               AND projectGroup.type = %d',
             $edge_table,
             PhabricatorProjectObjectHasProjectEdgeType::EDGECONST);
         }
         $joins[] = qsprintf(
-          $conn_r,
+          $conn,
           'LEFT JOIN %T projectGroupName
             ON projectGroup.dst = projectGroupName.indexedObjectPHID',
           id(new ManiphestNameIndex())->getTableName());
         break;
     }
 
-    $joins[] = parent::buildJoinClauseParts($conn_r);
+    $joins[] = parent::buildJoinClauseParts($conn);
 
     return $joins;
   }
 
   protected function buildGroupClause(AphrontDatabaseConnection $conn_r) {
-    $joined_multiple_rows = $this->shouldJoinBlockingTasks() ||
+    $joined_multiple_rows =
-                            $this->shouldJoinBlockedTasks() ||
+      ($this->hasOpenParents !== null) ||
-                            ($this->shouldGroupQueryResultRows());
+      ($this->hasOpenSubtasks !== null) ||
+      $this->shouldGroupQueryResultRows();
 
     $joined_project_name = ($this->groupBy == self::GROUP_PROJECT);
 
@@ -652,6 +628,30 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery {
     }
   }
 
+
+  protected function buildHavingClauseParts(AphrontDatabaseConnection $conn) {
+    $having = parent::buildHavingClauseParts($conn);
+
+    if ($this->hasOpenParents !== null) {
+      if (!$this->hasOpenParents) {
+        $having[] = qsprintf(
+          $conn,
+          'COUNT(parent.phid) = 0');
+      }
+    }
+
+    if ($this->hasOpenSubtasks !== null) {
+      if (!$this->hasOpenSubtasks) {
+        $having[] = qsprintf(
+          $conn,
+          'COUNT(subtask.phid) = 0');
+      }
+    }
+
+    return $having;
+  }
+
+
   /**
    * Return project PHIDs which we should ignore when grouping tasks by
    * project. For example, if a user issues a query like:

src/applications/maniphest/query/ManiphestTaskSearchEngine.php

@@ -77,19 +77,21 @@ final class ManiphestTaskSearchEngine
         ->setLabel(pht('Contains Words'))
         ->setKey('fulltext'),
       id(new PhabricatorSearchThreeStateField())
-        ->setLabel(pht('Blocking'))
+        ->setLabel(pht('Open Parents'))
-        ->setKey('blocking')
+        ->setKey('hasParents')
+        ->setAliases(array('blocking'))
         ->setOptions(
           pht('(Show All)'),
-          pht('Show Only Tasks Blocking Other Tasks'),
+          pht('Show Only Tasks With Open Parents'),
-          pht('Hide Tasks Blocking Other Tasks')),
+          pht('Show Only Tasks Without Open Parents')),
       id(new PhabricatorSearchThreeStateField())
-        ->setLabel(pht('Blocked'))
+        ->setLabel(pht('Open Subtasks'))
-        ->setKey('blocked')
+        ->setKey('hasSubtasks')
+        ->setAliases(array('blocked'))
         ->setOptions(
           pht('(Show All)'),
-          pht('Show Only Task Blocked By Other Tasks'),
+          pht('Show Only Tasks With Open Subtasks'),
-          pht('Hide Tasks Blocked By Other Tasks')),
+          pht('Show Only Tasks Without Open Subtasks')),
       id(new PhabricatorSearchSelectField())
         ->setLabel(pht('Group By'))
         ->setKey('group')
@@ -121,8 +123,8 @@ final class ManiphestTaskSearchEngine
       'statuses',
       'priorities',
       'fulltext',
-      'blocking',
+      'hasParents',
-      'blocked',
+      'hasSubtasks',
       'group',
       'order',
       'ids',
@@ -182,12 +184,12 @@ final class ManiphestTaskSearchEngine
       $query->withDateModifiedBefore($map['modifiedEnd']);
     }
 
-    if ($map['blocking'] !== null) {
+    if ($map['hasParents'] !== null) {
-      $query->withBlockingTasks($map['blocking']);
+      $query->withOpenParents($map['hasParents']);
     }
 
-    if ($map['blocked'] !== null) {
+    if ($map['hasSubtasks'] !== null) {
-      $query->withBlockedTasks($map['blocked']);
+      $query->withOpenSubtasks($map['hasSubtasks']);
     }
 
     if (strlen($map['fulltext'])) {