From 63e6b2553e0ecdb0d10cde7c1845529a6d53e836 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 20 Oct 2017 11:20:02 -0700
Subject: [PATCH] Simply how Differential drafts ignore Harbormaster autobuilds

Summary:
Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything.

In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them).

The way we do this has some issues:

  - Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong.
  - We have to load targets but don't really care about them, which is more work than we really need to do.
  - And it's kind of complex, too.

Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN.

Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review".

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18721
---
 .../storage/DifferentialRevision.php          | 25 +--------
 .../query/HarbormasterBuildQuery.php          | 55 +++++++++++++++++--
 2 files changed, 51 insertions(+), 29 deletions(-)

diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 76c7c4da95..da7e18ce05 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -721,9 +721,10 @@ final class DifferentialRevision extends DifferentialDAO
       return array();
     }
 
-    $builds = id(new HarbormasterBuildQuery())
+    return id(new HarbormasterBuildQuery())
       ->setViewer($viewer)
       ->withBuildablePHIDs(mpull($buildables, 'getPHID'))
+      ->withAutobuilds(false)
       ->withBuildStatuses(
         array(
           HarbormasterBuildStatus::STATUS_INACTIVE,
@@ -735,29 +736,7 @@ final class DifferentialRevision extends DifferentialDAO
           HarbormasterBuildStatus::STATUS_PAUSED,
           HarbormasterBuildStatus::STATUS_DEADLOCKED,
         ))
-      ->needBuildTargets(true)
       ->execute();
-    if (!$builds) {
-      return array();
-    }
-
-    $active = array();
-    foreach ($builds as $key => $build) {
-      foreach ($build->getBuildTargets() as $target) {
-        if ($target->isAutotarget()) {
-          // Ignore autotargets when looking for active of failed builds. If
-          // local tests fail and you continue anyway, you don't need to
-          // double-confirm them.
-          continue;
-        }
-
-        // This build has at least one real target that's doing something.
-        $active[$key] = $build;
-        break;
-      }
-    }
-
-    return $active;
   }
 
 
diff --git a/src/applications/harbormaster/query/HarbormasterBuildQuery.php b/src/applications/harbormaster/query/HarbormasterBuildQuery.php
index cf6db6115b..770ca4413b 100644
--- a/src/applications/harbormaster/query/HarbormasterBuildQuery.php
+++ b/src/applications/harbormaster/query/HarbormasterBuildQuery.php
@@ -10,6 +10,7 @@ final class HarbormasterBuildQuery
   private $buildPlanPHIDs;
   private $initiatorPHIDs;
   private $needBuildTargets;
+  private $autobuilds;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -41,6 +42,11 @@ final class HarbormasterBuildQuery
     return $this;
   }
 
+  public function withAutobuilds($with_autobuilds) {
+    $this->autobuilds = $with_autobuilds;
+    return $this;
+  }
+
   public function needBuildTargets($need_targets) {
     $this->needBuildTargets = $need_targets;
     return $this;
@@ -141,50 +147,87 @@ final class HarbormasterBuildQuery
     if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
-        'id IN (%Ld)',
+        'b.id IN (%Ld)',
         $this->ids);
     }
 
     if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
-        'phid in (%Ls)',
+        'b.phid in (%Ls)',
         $this->phids);
     }
 
     if ($this->buildStatuses !== null) {
       $where[] = qsprintf(
         $conn,
-        'buildStatus in (%Ls)',
+        'b.buildStatus in (%Ls)',
         $this->buildStatuses);
     }
 
     if ($this->buildablePHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'buildablePHID IN (%Ls)',
+        'b.buildablePHID IN (%Ls)',
         $this->buildablePHIDs);
     }
 
     if ($this->buildPlanPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'buildPlanPHID IN (%Ls)',
+        'b.buildPlanPHID IN (%Ls)',
         $this->buildPlanPHIDs);
     }
 
     if ($this->initiatorPHIDs !== null) {
       $where[] = qsprintf(
         $conn,
-        'initiatorPHID IN (%Ls)',
+        'b.initiatorPHID IN (%Ls)',
         $this->initiatorPHIDs);
     }
 
+    if ($this->autobuilds !== null) {
+      if ($this->autobuilds) {
+        $where[] = qsprintf(
+          $conn,
+          'p.planAutoKey IS NOT NULL');
+      } else {
+        $where[] = qsprintf(
+          $conn,
+          'p.planAutoKey IS NULL');
+      }
+    }
+
     return $where;
   }
 
+  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
+    $joins = parent::buildJoinClauseParts($conn);
+
+    if ($this->shouldJoinPlanTable()) {
+      $joins[] = qsprintf(
+        $conn,
+        'JOIN %T p ON b.buildPlanPHID = p.phid',
+        id(new HarbormasterBuildPlan())->getTableName());
+    }
+
+    return $joins;
+  }
+
+  private function shouldJoinPlanTable() {
+    if ($this->autobuilds !== null) {
+      return true;
+    }
+
+    return false;
+  }
+
   public function getQueryApplicationClass() {
     return 'PhabricatorHarbormasterApplication';
   }
 
+  protected function getPrimaryTableAlias() {
+    return 'b';
+  }
+
 }