Improve formality of "HarbormasterBuild" states

Summary:
Ref T13072. Currently, Builds have basic states (like "passed" and "failed") and pending states where a command has been issued but not yet executed (pausing, resuming, restarting, and aborting).

These are handled in a bit of an ad-hoc way, and not everything treats them the same way. In particular, the build page can concurrently report a build as "Aborting" and "Pausing", with different icons and colors.

Make everything use the same logic so that a Build can only be in exactly zero or one pending state, and use the same icons and colors.

Also tighten up which transitions are allowed: for example, it doesn't make sense to pause an aborting build.

The tighter rules don't all produce great UX right now (like "You can't pause this build.", when it would be better as "You can't pause a build which is already aborting." or similar), but just leave that alone for now.

Test Plan: Viewed builds, applied various state changes, ran BuildWorker to effect the state changes, grepped for affected methods, tried to issue various out-of-sequence build commands.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13072

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

src/applications/harbormaster/constants/HarbormasterBuildStatus.php

@@ -12,6 +12,11 @@ final class HarbormasterBuildStatus extends Phobject {
   const STATUS_PAUSED = 'paused';
   const STATUS_DEADLOCKED = 'deadlocked';
 
+  const PENDING_PAUSING = 'x-pausing';
+  const PENDING_RESUMING = 'x-resuming';
+  const PENDING_RESTARTING = 'x-restarting';
+  const PENDING_ABORTING = 'x-aborting';
+
   private $key;
   private $properties;
 
@@ -48,6 +53,10 @@ final class HarbormasterBuildStatus extends Phobject {
     return $this->getProperty('isComplete');
   }
 
+  public function isPending() {
+    return $this->getProperty('isPending');
+  }
+
   public function isPassed() {
     return ($this->key === self::STATUS_PASSED);
   }
@@ -56,6 +65,33 @@ final class HarbormasterBuildStatus extends Phobject {
     return ($this->key === self::STATUS_FAILED);
   }
 
+  public function isAborting() {
+    return ($this->key === self::PENDING_ABORTING);
+  }
+
+  public function isRestarting() {
+    return ($this->key === self::PENDING_RESTARTING);
+  }
+
+  public function isResuming() {
+    return ($this->key === self::PENDING_RESUMING);
+  }
+
+  public function isPausing() {
+    return ($this->key === self::PENDING_PAUSING);
+  }
+
+  public function getIconIcon() {
+    return $this->getProperty('icon');
+  }
+
+  public function getIconColor() {
+    return $this->getProperty('color');
+  }
+
+  public function getName() {
+    return $this->getProperty('name');
+  }
 
   /**
    * Get a human readable name for a build status constant.
@@ -134,6 +170,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => false,
         'isComplete' => false,
+        'isPending' => false,
       ),
       self::STATUS_PENDING => array(
         'name' => pht('Pending'),
@@ -142,6 +179,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => true,
         'isComplete' => false,
+        'isPending' => false,
       ),
       self::STATUS_BUILDING => array(
         'name' => pht('Building'),
@@ -150,6 +188,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => true,
         'isComplete' => false,
+        'isPending' => false,
       ),
       self::STATUS_PASSED => array(
         'name' => pht('Passed'),
@@ -158,6 +197,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'green',
         'isBuilding' => false,
         'isComplete' => true,
+        'isPending' => false,
       ),
       self::STATUS_FAILED => array(
         'name' => pht('Failed'),
@@ -166,6 +206,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
+        'isPending' => false,
       ),
       self::STATUS_ABORTED => array(
         'name' => pht('Aborted'),
@@ -174,6 +215,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
+        'isPending' => false,
       ),
       self::STATUS_ERROR => array(
         'name' => pht('Unexpected Error'),
@@ -182,6 +224,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
+        'isPending' => false,
       ),
       self::STATUS_PAUSED => array(
         'name' => pht('Paused'),
@@ -190,6 +233,7 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'yellow',
         'isBuilding' => false,
         'isComplete' => false,
+        'isPending' => false,
       ),
       self::STATUS_DEADLOCKED => array(
         'name' => pht('Deadlocked'),
@@ -198,6 +242,43 @@ final class HarbormasterBuildStatus extends Phobject {
         'color.ansi' => 'red',
         'isBuilding' => false,
         'isComplete' => true,
+        'isPending' => false,
+      ),
+      self::PENDING_PAUSING => array(
+        'name' => pht('Pausing'),
+        'icon' => 'fa-exclamation-triangle',
+        'color' => 'red',
+        'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => false,
+        'isPending' => true,
+      ),
+      self::PENDING_RESUMING => array(
+        'name' => pht('Resuming'),
+        'icon' => 'fa-exclamation-triangle',
+        'color' => 'red',
+        'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => false,
+        'isPending' => true,
+      ),
+      self::PENDING_RESTARTING => array(
+        'name' => pht('Restarting'),
+        'icon' => 'fa-exclamation-triangle',
+        'color' => 'red',
+        'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => false,
+        'isPending' => true,
+      ),
+      self::PENDING_ABORTING => array(
+        'name' => pht('Aborting'),
+        'icon' => 'fa-exclamation-triangle',
+        'color' => 'red',
+        'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => false,
+        'isPending' => true,
       ),
     );
   }

src/applications/harbormaster/controller/HarbormasterBuildViewController.php

@@ -32,21 +32,13 @@ final class HarbormasterBuildViewController
       ->setPolicyObject($build)
       ->setHeaderIcon('fa-cubes');
 
-    $is_restarting = $build->isRestarting();
+    $status = $build->getBuildPendingStatusObject();
 
-    if ($is_restarting) {
+    $status_icon = $status->getIconIcon();
-      $page_header->setStatus(
+    $status_color = $status->getIconColor();
-        'fa-exclamation-triangle', 'red', pht('Restarting'));
+    $status_name = $status->getName();
-    } else if ($build->isPausing()) {
+
-      $page_header->setStatus(
+    $page_header->setStatus($status_icon, $status_color, $status_name);
-        'fa-exclamation-triangle', 'red', pht('Pausing'));
-    } else if ($build->isResuming()) {
-      $page_header->setStatus(
-        'fa-exclamation-triangle', 'red', pht('Resuming'));
-    } else if ($build->isAborting()) {
-      $page_header->setStatus(
-        'fa-exclamation-triangle', 'red', pht('Aborting'));
-    }
 
     $max_generation = (int)$build->getBuildGeneration();
     if ($max_generation === 0) {
@@ -55,7 +47,7 @@ final class HarbormasterBuildViewController
       $min_generation = 1;
     }
 
-    if ($is_restarting) {
+    if ($build->isRestarting()) {
       $max_generation = $max_generation + 1;
     }
 
@@ -624,10 +616,6 @@ final class HarbormasterBuildViewController
       pht('Build Plan'),
       $handles[$build->getBuildPlanPHID()]->renderLink());
 
-    $properties->addProperty(
-      pht('Status'),
-      $this->getStatus($build));
-
     return id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Properties'))
       ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
@@ -681,31 +669,6 @@ final class HarbormasterBuildViewController
       ->setTable($table);
   }
 
-
-  private function getStatus(HarbormasterBuild $build) {
-    $status_view = new PHUIStatusListView();
-
-    $item = new PHUIStatusItemView();
-
-    if ($build->isPausing()) {
-      $status_name = pht('Pausing');
-      $icon = PHUIStatusItemView::ICON_RIGHT;
-      $color = 'dark';
-    } else {
-      $status = $build->getBuildStatus();
-      $status_name =
-        HarbormasterBuildStatus::getBuildStatusName($status);
-      $icon = HarbormasterBuildStatus::getBuildStatusIcon($status);
-      $color = HarbormasterBuildStatus::getBuildStatusColor($status);
-    }
-
-    $item->setTarget($status_name);
-    $item->setIcon($icon, $color);
-    $status_view->addItem($item);
-
-    return $status_view;
-  }
-
   private function buildMessages(array $messages) {
     $viewer = $this->getRequest()->getUser();
 

src/applications/harbormaster/storage/build/HarbormasterBuild.php

@@ -212,6 +212,64 @@ final class HarbormasterBuild extends HarbormasterDAO
     return "/harbormaster/build/{$id}/";
   }
 
+  public function getBuildPendingStatusObject() {
+
+    // NOTE: If a build has multiple unprocessed messages, we'll ignore
+    // messages that are obsoleted by a later or stronger message.
+    //
+    // For example, if a build has both "pause" and "abort" messages in queue,
+    // we just ignore the "pause" message and perform an "abort", since pausing
+    // first wouldn't affect the final state, so we can just skip it.
+    //
+    // Likewise, if a build has both "restart" and "abort" messages, the most
+    // recent message is controlling: we'll take whichever action a command
+    // was most recently issued for.
+
+    $is_restarting = false;
+    $is_aborting = false;
+    $is_pausing = false;
+    $is_resuming = false;
+
+    foreach ($this->getUnprocessedMessages() as $message_object) {
+      $message_type = $message_object->getType();
+      switch ($message_type) {
+        case HarbormasterBuildCommand::COMMAND_RESTART:
+          $is_restarting = true;
+          $is_aborting = false;
+          break;
+        case HarbormasterBuildCommand::COMMAND_ABORT:
+          $is_aborting = true;
+          $is_restarting = false;
+          break;
+        case HarbormasterBuildCommand::COMMAND_PAUSE:
+          $is_pausing = true;
+          $is_resuming = false;
+          break;
+        case HarbormasterBuildCommand::COMMAND_RESUME:
+          $is_resuming = true;
+          $is_pausing = false;
+          break;
+      }
+    }
+
+    $pending_status = null;
+    if ($is_restarting) {
+      $pending_status = HarbormasterBuildStatus::PENDING_RESTARTING;
+    } else if ($is_aborting) {
+      $pending_status = HarbormasterBuildStatus::PENDING_ABORTING;
+    } else if ($is_pausing) {
+      $pending_status = HarbormasterBuildStatus::PENDING_PAUSING;
+    } else if ($is_resuming) {
+      $pending_status = HarbormasterBuildStatus::PENDING_RESUMING;
+    }
+
+    if ($pending_status !== null) {
+      return HarbormasterBuildStatus::newBuildStatusObject($pending_status);
+    }
+
+    return $this->getBuildStatusObject();
+  }
+
   protected function getBuildStatusObject() {
     $status_key = $this->getBuildStatus();
     return HarbormasterBuildStatus::newBuildStatusObject($status_key);
@@ -309,7 +367,9 @@ final class HarbormasterBuild extends HarbormasterDAO
 
     return !$this->isComplete() &&
            !$this->isPaused() &&
-           !$this->isPausing();
+           !$this->isPausing() &&
+           !$this->isRestarting() &&
+           !$this->isAborting();
   }
 
   public function canAbortBuild() {
@@ -317,7 +377,9 @@ final class HarbormasterBuild extends HarbormasterDAO
       return false;
     }
 
-    return !$this->isComplete();
+    return
+      !$this->isComplete() &&
+      !$this->isAborting();
   }
 
   public function canResumeBuild() {
@@ -325,78 +387,27 @@ final class HarbormasterBuild extends HarbormasterDAO
       return false;
     }
 
-    return $this->isPaused() &&
+    return
-           !$this->isResuming();
+      $this->isPaused() &&
+      !$this->isResuming() &&
+      !$this->isRestarting() &&
+      !$this->isAborting();
   }
 
   public function isPausing() {
-    $is_pausing = false;
+    return $this->getBuildPendingStatusObject()->isPausing();
-    foreach ($this->getUnprocessedMessages() as $message_object) {
-      $message_type = $message_object->getType();
-      switch ($message_type) {
-        case HarbormasterBuildCommand::COMMAND_PAUSE:
-          $is_pausing = true;
-          break;
-        case HarbormasterBuildCommand::COMMAND_RESUME:
-        case HarbormasterBuildCommand::COMMAND_RESTART:
-          $is_pausing = false;
-          break;
-        case HarbormasterBuildCommand::COMMAND_ABORT:
-          $is_pausing = true;
-          break;
-      }
-    }
-
-    return $is_pausing;
   }
 
   public function isResuming() {
-    $is_resuming = false;
+    return $this->getBuildPendingStatusObject()->isResuming();
-    foreach ($this->getUnprocessedMessages() as $message_object) {
-      $message_type = $message_object->getType();
-      switch ($message_type) {
-        case HarbormasterBuildCommand::COMMAND_RESTART:
-        case HarbormasterBuildCommand::COMMAND_RESUME:
-          $is_resuming = true;
-          break;
-        case HarbormasterBuildCommand::COMMAND_PAUSE:
-          $is_resuming = false;
-          break;
-        case HarbormasterBuildCommand::COMMAND_ABORT:
-          $is_resuming = false;
-          break;
-      }
-    }
-
-    return $is_resuming;
   }
 
   public function isRestarting() {
-    $is_restarting = false;
+    return $this->getBuildPendingStatusObject()->isRestarting();
-    foreach ($this->getUnprocessedMessages() as $message_object) {
-      $message_type = $message_object->getType();
-      switch ($message_type) {
-        case HarbormasterBuildCommand::COMMAND_RESTART:
-          $is_restarting = true;
-          break;
-      }
-    }
-
-    return $is_restarting;
   }
 
   public function isAborting() {
-    $is_aborting = false;
+    return $this->getBuildPendingStatusObject()->isAborting();
-    foreach ($this->getUnprocessedMessages() as $message_object) {
-      $message_type = $message_object->getType();
-      switch ($message_type) {
-        case HarbormasterBuildCommand::COMMAND_ABORT:
-          $is_aborting = true;
-          break;
-      }
-    }
-
-    return $is_aborting;
   }
 
   public function markUnprocessedMessagesAsProcessed() {