From 9bf4df2c1ddfa55dfbe375b2fa1a078e9d15fb71 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 17 Apr 2018 16:00:51 -0700
Subject: [PATCH] Allow demoted builds to automatically promote if builds pass
 after a restart

Summary:
Ref T13124. See PHI584. When you create a draft revision and it automatically demotes to "Changes Planned + Draft" because builds fail, let it promote to "Needs Review" automatically if builds pass. Usually, this will be because someone restarted the builds and they worked the second time.

Although I'm a little wary about adding even more state transitions to the diagram in T13110#237736, I think this one is reasonably natural and not ambiguous.

Test Plan:
  - Created a failing build plan with a "Throw Exception" step.
  - Created a revision which hit the build plan, saw it demote to "Changes Planned" when Harbormaster failed.
  - Edited the build plan to remove the "Throw Exception" step, restarted the build, got a pass.
  - Saw revision promote again:

{F5526104}

I didn't exhaustively test that the other 40 state transitions still work properly, but I think the scope of this change is small enough that it's unlikely I did much collateral damage.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19380
---
 .../editor/DifferentialTransactionEditor.php  | 36 ++++++++++++++++---
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 50094b02f4..82740459b0 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1555,13 +1555,41 @@ final class DifferentialTransactionEditor
       $auto_undraft = false;
     }
 
-    if ($object->isDraft() && $auto_undraft) {
+    $can_promote = false;
+    $can_demote = false;
+
+    // "Draft" revisions can promote to "Review Requested" after builds pass,
+    // or demote to "Changes Planned" after builds fail.
+    if ($object->isDraft()) {
+      $can_promote = true;
+      $can_demote = true;
+    }
+
+    // See PHI584. "Changes Planned" revisions which are not yet broadcasting
+    // can promote to "Review Requested" if builds pass.
+
+    // This pass is presumably the result of someone restarting the builds and
+    // having them work this time, perhaps because the builds are not perfectly
+    // reliable or perhaps because someone fixed some issue with build hardware
+    // or some other dependency.
+
+    // Currently, there's no legitimate way to end up in this state except
+    // through automatic demotion, so this behavior should not generate an
+    // undue level of confusion or ambiguity. Also note that these changes can
+    // not demote again since they've already been demoted once.
+    if ($object->isChangePlanned()) {
+      if (!$object->getShouldBroadcast()) {
+        $can_promote = true;
+      }
+    }
+
+    if (($can_promote || $can_demote) && $auto_undraft) {
       $status = $this->loadCompletedBuildableStatus($object);
 
       $is_passed = ($status === HarbormasterBuildableStatus::STATUS_PASSED);
       $is_failed = ($status === HarbormasterBuildableStatus::STATUS_FAILED);
 
-      if ($is_passed) {
+      if ($is_passed && $can_promote) {
         // 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.
@@ -1593,7 +1621,7 @@ 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) {
+      } else if ($is_failed && $can_demote) {
         // When demoting a revision, we act as "Harbormaster" instead of
         // the author since this feels a little more natural.
         $harbormaster_phid = id(new PhabricatorHarbormasterApplication())
@@ -1607,8 +1635,6 @@ final class DifferentialTransactionEditor
           ->setNewValue(true);
 
         $this->queueTransaction($xaction);
-
-        // TODO: Notify the author (only) that we did this.
       }
     }