From f350b9e4646bd13a79a63524f137e1609bea8177 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 3 Apr 2018 07:54:27 -0700
Subject: [PATCH] Explicitly condition Differential draft promotion on only
 "impactful" builds

Summary:
Depends on D19281. This increases consistency between build timeline publishing and revision draft promotion.

There's no real behavioral change here (switching how publishing worked already changed the beahvior) but this sends more callsites down the same code paths.

Since the builds we're looking at include completed builds, change the term "active" to "impactful". This describes the same set of builds, but hopefully describes them more accurately.

Test Plan: Created a local revision, saw it plausibly interact with draft status and promote. There are a lot of moving parts here and some stuff may well have slipped through.

Differential Revision: https://secure.phabricator.com/D19282
---
 .../customfield/DifferentialDraftField.php    |  2 +-
 .../editor/DifferentialTransactionEditor.php  | 23 ++++++-----
 .../storage/DifferentialRevision.php          | 39 +++++++++----------
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php
index 8147708bca..70f7291148 100644
--- a/src/applications/differential/customfield/DifferentialDraftField.php
+++ b/src/applications/differential/customfield/DifferentialDraftField.php
@@ -58,7 +58,7 @@ final class DifferentialDraftField
     );
     $blocking_map = array_fuse($blocking_map);
 
-    $builds = $revision->loadActiveBuilds($viewer);
+    $builds = $revision->loadImpactfulBuilds($viewer);
 
     $waiting = array();
     $blocking = array();
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 9882246608..ccd83892db 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1539,8 +1539,12 @@ final class DifferentialTransactionEditor
     }
 
     if ($object->isDraft() && $auto_undraft) {
-      $active_builds = $this->hasActiveBuilds($object);
-      if (!$active_builds) {
+      $status = $this->loadCompletedBuildableStatus($object);
+
+      $is_passed = ($status === HarbormasterBuildableStatus::STATUS_PASSED);
+      $is_failed = ($status === HarbormasterBuildableStatus::STATUS_FAILED);
+
+      if ($is_passed) {
         // When Harbormaster moves a revision out of the draft state, we
         // attribute the action to the revision author since this is more
         // natural and more useful.
@@ -1572,6 +1576,9 @@ final class DifferentialTransactionEditor
         // batch of transactions finishes so that Herald can fire on the new
         // revision state. See T13027 for discussion.
         $this->queueTransaction($xaction);
+      } else if ($is_failed) {
+        // TODO: Change to "Changes Planned + Draft", notify the author (only)
+        // of the build failure.
       }
     }
 
@@ -1604,15 +1611,11 @@ final class DifferentialTransactionEditor
     return $xactions;
   }
 
-  private function hasActiveBuilds($object) {
+  private function loadCompletedBuildableStatus(
+    DifferentialRevision $revision) {
     $viewer = $this->requireActor();
-
-    $builds = $object->loadActiveBuilds($viewer);
-    if (!$builds) {
-      return false;
-    }
-
-    return true;
+    $builds = $revision->loadImpactfulBuilds($viewer);
+    return $revision->newBuildableStatusForBuilds($builds);
   }
 
   private function requireReviewers(DifferentialRevision $revision) {
diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 446d180018..05b82ed089 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -779,23 +779,14 @@ final class DifferentialRevision extends DifferentialDAO
     // when computing build status. Differential only cares about remote
     // builds when making publishing and undrafting decisions.
 
-    $builds = id(new HarbormasterBuildQuery())
-      ->setViewer($viewer)
-      ->withBuildablePHIDs(array($phid))
-      ->withAutobuilds(false)
-      ->withBuildStatuses(
-        array(
-          HarbormasterBuildStatus::STATUS_INACTIVE,
-          HarbormasterBuildStatus::STATUS_PENDING,
-          HarbormasterBuildStatus::STATUS_BUILDING,
-          HarbormasterBuildStatus::STATUS_FAILED,
-          HarbormasterBuildStatus::STATUS_ABORTED,
-          HarbormasterBuildStatus::STATUS_ERROR,
-          HarbormasterBuildStatus::STATUS_PAUSED,
-          HarbormasterBuildStatus::STATUS_DEADLOCKED,
-        ))
-      ->execute();
+    $builds = $this->loadImpactfulBuildsForBuildablePHIDs(
+      $viewer,
+      array($phid));
 
+    return $this->newBuildableStatusForBuilds($builds);
+  }
+
+  public function newBuildableStatusForBuilds(array $builds) {
     // If we have nothing but passing builds, the buildable passes.
     if (!$builds) {
       return HarbormasterBuildableStatus::STATUS_PASSED;
@@ -803,8 +794,7 @@ final class DifferentialRevision extends DifferentialDAO
 
     // If we have any completed, non-passing builds, the buildable fails.
     foreach ($builds as $build) {
-      $status = $build->getBuildStatusObject();
-      if ($status->isComplete()) {
+      if ($build->isComplete()) {
         return HarbormasterBuildableStatus::STATUS_FAILED;
       }
     }
@@ -813,7 +803,7 @@ final class DifferentialRevision extends DifferentialDAO
     return null;
   }
 
-  public function loadActiveBuilds(PhabricatorUser $viewer) {
+  public function loadImpactfulBuilds(PhabricatorUser $viewer) {
     $diff = $this->getActiveDiff();
 
     // NOTE: We can't use `withContainerPHIDs()` here because the container
@@ -827,9 +817,18 @@ final class DifferentialRevision extends DifferentialDAO
       return array();
     }
 
+    return $this->loadImpactfulBuildsForBuildablePHIDs(
+      $viewer,
+      mpull($buildables, 'getPHID'));
+  }
+
+  private function loadImpactfulBuildsForBuildablePHIDs(
+    PhabricatorUser $viewer,
+    array $phids) {
+
     return id(new HarbormasterBuildQuery())
       ->setViewer($viewer)
-      ->withBuildablePHIDs(mpull($buildables, 'getPHID'))
+      ->withBuildablePHIDs($phids)
       ->withAutobuilds(false)
       ->withBuildStatuses(
         array(