Make various minor Harbormaster UI improvements

Summary: Ref T1049. Tweaks some of the UI and code to improve / clean it up a bit.

Test Plan: Ran build plans, browsed UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

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

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

@@ -20,97 +20,21 @@ final class HarbormasterBuildableViewController
       ->withIDs(array($id))
       ->needBuildableHandles(true)
       ->needContainerHandles(true)
-      ->needBuilds(true)
       ->executeOne();
     if (!$buildable) {
       return new Aphront404Response();
     }
 
-    $build_list = id(new PHUIObjectItemListView())
+    // Pull builds and build targets.
-      ->setUser($viewer);
+    $builds = id(new HarbormasterBuildQuery())
-    foreach ($buildable->getBuilds() as $build) {
+      ->setViewer($viewer)
-      $view_uri = $this->getApplicationURI('/build/'.$build->getID().'/');
+      ->withBuildablePHIDs(array($buildable->getPHID()))
-      $item = id(new PHUIObjectItemView())
+      ->needBuildTargets(true)
-        ->setObjectName(pht('Build %d', $build->getID()))
+      ->execute();
-        ->setHeader($build->getName())
-        ->setHref($view_uri);
 
-      switch ($build->getBuildStatus()) {
+    $buildable->attachBuilds($builds);
-        case HarbormasterBuild::STATUS_INACTIVE:
-          $item->setBarColor('grey');
-          $item->addAttribute(pht('Inactive'));
-          break;
-        case HarbormasterBuild::STATUS_PENDING:
-          $item->setBarColor('blue');
-          $item->addAttribute(pht('Pending'));
-          break;
-        case HarbormasterBuild::STATUS_WAITING:
-          $item->setBarColor('violet');
-          $item->addAttribute(pht('Waiting'));
-          break;
-        case HarbormasterBuild::STATUS_BUILDING:
-          $item->setBarColor('yellow');
-          $item->addAttribute(pht('Building'));
-          break;
-        case HarbormasterBuild::STATUS_PASSED:
-          $item->setBarColor('green');
-          $item->addAttribute(pht('Passed'));
-          break;
-        case HarbormasterBuild::STATUS_FAILED:
-          $item->setBarColor('red');
-          $item->addAttribute(pht('Failed'));
-          break;
-        case HarbormasterBuild::STATUS_ERROR:
-          $item->setBarColor('red');
-          $item->addAttribute(pht('Unexpected Error'));
-          break;
-        case HarbormasterBuild::STATUS_STOPPED:
-          $item->setBarColor('black');
-          $item->addAttribute(pht('Stopped'));
-          break;
-      }
 
-      if ($build->isRestarting()) {
+    $build_list = $this->buildBuildList($buildable);
-        $item->addIcon('backward', pht('Restarting'));
-      } else if ($build->isStopping()) {
-        $item->addIcon('stop', pht('Stopping'));
-      } else if ($build->isResuming()) {
-        $item->addIcon('play', pht('Resuming'));
-      }
-
-      $build_id = $build->getID();
-
-      $restart_uri = "build/restart/{$build_id}/buildable/";
-      $resume_uri = "build/resume/{$build_id}/buildable/";
-      $stop_uri = "build/stop/{$build_id}/buildable/";
-
-      $item->addAction(
-        id(new PHUIListItemView())
-          ->setIcon('backward')
-          ->setName(pht('Restart'))
-          ->setHref($this->getApplicationURI($restart_uri))
-          ->setWorkflow(true)
-          ->setDisabled(!$build->canRestartBuild()));
-
-      if ($build->canResumeBuild()) {
-        $item->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('play')
-            ->setName(pht('Resume'))
-            ->setHref($this->getApplicationURI($resume_uri))
-            ->setWorkflow(true));
-      } else {
-        $item->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('stop')
-            ->setName(pht('Stop'))
-            ->setHref($this->getApplicationURI($stop_uri))
-            ->setWorkflow(true)
-            ->setDisabled(!$build->canStopBuild()));
-      }
-
-      $build_list->addItem($item);
-    }
 
     $title = pht("Buildable %d", $id);
 
@@ -233,4 +157,137 @@ final class HarbormasterBuildableViewController
 
   }
 
+  private function buildBuildList(HarbormasterBuildable $buildable) {
+    $viewer = $this->getRequest()->getUser();
+
+    $build_list = id(new PHUIObjectItemListView())
+      ->setUser($viewer);
+    foreach ($buildable->getBuilds() as $build) {
+      $view_uri = $this->getApplicationURI('/build/'.$build->getID().'/');
+      $item = id(new PHUIObjectItemView())
+        ->setObjectName(pht('Build %d', $build->getID()))
+        ->setHeader($build->getName())
+        ->setHref($view_uri);
+
+      switch ($build->getBuildStatus()) {
+        case HarbormasterBuild::STATUS_INACTIVE:
+          $item->setBarColor('grey');
+          $item->addAttribute(pht('Inactive'));
+          break;
+        case HarbormasterBuild::STATUS_PENDING:
+          $item->setBarColor('blue');
+          $item->addAttribute(pht('Pending'));
+          break;
+        case HarbormasterBuild::STATUS_WAITING:
+          $item->setBarColor('violet');
+          $item->addAttribute(pht('Waiting'));
+          break;
+        case HarbormasterBuild::STATUS_BUILDING:
+          $item->setBarColor('yellow');
+          $item->addAttribute(pht('Building'));
+          break;
+        case HarbormasterBuild::STATUS_PASSED:
+          $item->setBarColor('green');
+          $item->addAttribute(pht('Passed'));
+          break;
+        case HarbormasterBuild::STATUS_FAILED:
+          $item->setBarColor('red');
+          $item->addAttribute(pht('Failed'));
+          break;
+        case HarbormasterBuild::STATUS_ERROR:
+          $item->setBarColor('red');
+          $item->addAttribute(pht('Unexpected Error'));
+          break;
+        case HarbormasterBuild::STATUS_STOPPED:
+          $item->setBarColor('black');
+          $item->addAttribute(pht('Stopped'));
+          break;
+      }
+
+      if ($build->isRestarting()) {
+        $item->addIcon('backward', pht('Restarting'));
+      } else if ($build->isStopping()) {
+        $item->addIcon('stop', pht('Stopping'));
+      } else if ($build->isResuming()) {
+        $item->addIcon('play', pht('Resuming'));
+      }
+
+      $build_id = $build->getID();
+
+      $restart_uri = "build/restart/{$build_id}/buildable/";
+      $resume_uri = "build/resume/{$build_id}/buildable/";
+      $stop_uri = "build/stop/{$build_id}/buildable/";
+
+      $item->addAction(
+        id(new PHUIListItemView())
+          ->setIcon('backward')
+          ->setName(pht('Restart'))
+          ->setHref($this->getApplicationURI($restart_uri))
+          ->setWorkflow(true)
+          ->setDisabled(!$build->canRestartBuild()));
+
+      if ($build->canResumeBuild()) {
+        $item->addAction(
+          id(new PHUIListItemView())
+            ->setIcon('play')
+            ->setName(pht('Resume'))
+            ->setHref($this->getApplicationURI($resume_uri))
+            ->setWorkflow(true));
+      } else {
+        $item->addAction(
+          id(new PHUIListItemView())
+            ->setIcon('stop')
+            ->setName(pht('Stop'))
+            ->setHref($this->getApplicationURI($stop_uri))
+            ->setWorkflow(true)
+            ->setDisabled(!$build->canStopBuild()));
+      }
+
+      $targets = $build->getBuildTargets();
+
+      if ($targets) {
+        $target_list = id(new PHUIStatusListView());
+        foreach ($targets as $target) {
+          switch ($target->getTargetStatus()) {
+            case HarbormasterBuildTarget::STATUS_PENDING:
+              $icon = 'time-green';
+              break;
+            case HarbormasterBuildTarget::STATUS_PASSED:
+              $icon = 'accept-green';
+              break;
+            case HarbormasterBuildTarget::STATUS_FAILED:
+              $icon = 'reject-red';
+              break;
+            default:
+              $icon = 'question';
+              break;
+          }
+
+          try {
+            $impl = $target->getImplementation();
+            $name = $impl->getName();
+          } catch (Exception $ex) {
+            $name = $target->getClassName();
+          }
+
+          $target_list->addItem(
+            id(new PHUIStatusItemView())
+              ->setIcon($icon)
+              ->setTarget(pht('Target %d', $target->getID()))
+              ->setNote($name));
+        }
+
+        $target_box = id(new PHUIBoxView())
+          ->addPadding(PHUI::PADDING_SMALL)
+          ->appendChild($target_list);
+
+        $item->appendChild($target_box);
+      }
+
+      $build_list->addItem($item);
+    }
+
+    return $build_list;
+  }
+
 }

src/applications/harbormaster/controller/HarbormasterStepAddController.php

@@ -22,19 +22,16 @@ final class HarbormasterStepAddController
         ->setViewer($viewer)
         ->withIDs(array($id))
         ->executeOne();
-    if ($plan === null) {
+    if (!$plan) {
-      throw new Exception("Build plan not found!");
+      return new Aphront404Response();
     }
 
-    $implementations =
-      HarbormasterBuildStepImplementation::getImplementations();
-
     $cancel_uri = $this->getApplicationURI('plan/'.$plan->getID().'/');
 
     if ($request->isDialogFormPost()) {
       $class = $request->getStr('step-type');
-      if (!in_array($class, $implementations)) {
+      if (!HarbormasterBuildStepImplementation::getImplementation($class)) {
-        return $this->createDialog($implementations, $cancel_uri);
+        return $this->createDialog($cancel_uri);
       }
 
       $steps = $plan->loadOrderedBuildSteps();
@@ -51,38 +48,30 @@ final class HarbormasterStepAddController
       return id(new AphrontRedirectResponse())->setURI($edit_uri);
     }
 
-    return $this->createDialog($implementations, $cancel_uri);
+    return $this->createDialog($cancel_uri);
   }
 
-  function createDialog(array $implementations, $cancel_uri) {
+  private function createDialog($cancel_uri) {
     $request = $this->getRequest();
     $viewer = $request->getUser();
 
     $control = id(new AphrontFormRadioButtonControl())
       ->setName('step-type');
 
-    foreach ($implementations as $implementation_name) {
+    $all = HarbormasterBuildStepImplementation::getImplementations();
-      $implementation = new $implementation_name();
+    foreach ($all as $class => $implementation) {
-      $control
+      $control->addButton(
-        ->addButton(
+        $class,
-          $implementation_name,
+        $implementation->getName(),
-          $implementation->getName(),
+        $implementation->getGenericDescription());
-          $implementation->getGenericDescription());
     }
 
-    $dialog = new AphrontDialogView();
+    return $this->newDialog()
-    $dialog->setTitle(pht('Add New Step'))
+      ->setTitle(pht('Add New Step'))
-            ->setUser($viewer)
+      ->addSubmitButton(pht('Add Build Step'))
-            ->addSubmitButton(pht('Add Build Step'))
+      ->addCancelButton($cancel_uri)
-            ->addCancelButton($cancel_uri);
+      ->appendParagraph(pht('Choose a type of build step to add:'))
-    $dialog->appendChild(
+      ->appendChild($control);
-      phutil_tag(
-        'p',
-        array(),
-        pht(
-          'Select what type of build step you want to add: ')));
-    $dialog->appendChild($control);
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/applications/harbormaster/query/HarbormasterBuildQuery.php

@@ -8,6 +8,7 @@ final class HarbormasterBuildQuery
   private $buildStatuses;
   private $buildablePHIDs;
   private $buildPlanPHIDs;
+  private $needBuildTargets;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -34,6 +35,11 @@ final class HarbormasterBuildQuery
     return $this;
   }
 
+  public function needBuildTargets($need_targets) {
+    $this->needBuildTargets = $need_targets;
+    return $this;
+  }
+
   protected function loadPage() {
     $table = new HarbormasterBuild();
     $conn_r = $table->establishConnection('r');
@@ -102,6 +108,24 @@ final class HarbormasterBuildQuery
       $build->attachUnprocessedCommands($unprocessed_commands);
     }
 
+    if ($this->needBuildTargets) {
+      $targets = id(new HarbormasterBuildTargetQuery())
+        ->setViewer($this->getViewer())
+        ->setParentQuery($this)
+        ->withBuildPHIDs($build_phids)
+        ->execute();
+
+      // TODO: Some day, when targets have dependencies, we should toposort
+      // these. For now, just put them into chronological order.
+      $targets = array_reverse($targets);
+
+      $targets = mgroup($targets, 'getBuildPHID');
+      foreach ($page as $build) {
+        $build_targets = idx($targets, $build->getPHID(), array());
+        $build->attachBuildTargets($build_targets);
+      }
+    }
+
     return $page;
   }
 

src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php

@@ -3,11 +3,32 @@
 abstract class HarbormasterBuildStepImplementation {
 
   public static function getImplementations() {
-    $symbols = id(new PhutilSymbolLoader())
+    return id(new PhutilSymbolLoader())
       ->setAncestorClass('HarbormasterBuildStepImplementation')
-      ->setConcreteOnly(true)
+      ->loadObjects();
-      ->selectAndLoadSymbols();
+  }
-    return ipull($symbols, 'name');
+
+  public static function getImplementation($class) {
+    $base = idx(self::getImplementations(), $class);
+
+    if ($base) {
+      return (clone $base);
+    }
+
+    return null;
+  }
+
+  public static function requireImplementation($class) {
+    if (!$class) {
+      throw new Exception(pht('No implementation is specified!'));
+    }
+
+    $implementation = self::getImplementation($class);
+    if (!$implementation) {
+      throw new Exception(pht('No such implementation "%s" exists!', $class));
+    }
+
+    return $implementation;
   }
 
   /**
@@ -163,4 +184,16 @@ abstract class HarbormasterBuildStepImplementation {
     return array();
   }
 
+  protected function formatSettingForDescription($key, $default = null) {
+    return $this->formatValueForDescription($this->getSetting($key, $default));
+  }
+
+  protected function formatValueForDescription($value) {
+    if (strlen($value)) {
+      return phutil_tag('strong', array(), $value);
+    } else {
+      return phutil_tag('em', array(), pht('(null)'));
+    }
+  }
+
 }

src/applications/harbormaster/step/HarbormasterCommandBuildStepImplementation.php

@@ -12,12 +12,10 @@ final class HarbormasterCommandBuildStepImplementation
   }
 
   public function getDescription() {
-    $settings = $this->getSettings();
-
     return pht(
-      'Run \'%s\' on \'%s\'.',
+      'Run command %s on host %s.',
-      $settings['command'],
+      $this->formatSettingForDescription('command'),
-      $settings['hostartifact']);
+      $this->formatSettingForDescription('hostartifact'));
   }
 
   public function execute(

src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php

@@ -12,11 +12,16 @@ final class HarbormasterHTTPRequestBuildStepImplementation
   }
 
   public function getDescription() {
-    $settings = $this->getSettings();
+    $domain = null;
+    $uri = $this->getSetting('uri');
+    if ($uri) {
+      $domain = id(new PhutilURI($uri))->getDomain();
+    }
 
-    $uri = new PhutilURI($settings['uri']);
+    return pht(
-    $domain = $uri->getDomain();
+      'Make an HTTP %s request to %s.',
-    return pht('Make an HTTP %s request to %s', $settings['method'], $domain);
+      $this->formatSettingForDescription('method', 'POST'),
+      $this->formatValueForDescription($domain));
   }
 
   public function execute(

src/applications/harbormaster/step/HarbormasterLeaseHostBuildStepImplementation.php

@@ -11,16 +11,6 @@ final class HarbormasterLeaseHostBuildStepImplementation
     return pht('Obtain a lease on a Drydock host for performing builds.');
   }
 
-  public function getDescription() {
-    $settings = $this->getSettings();
-
-    return pht(
-      'Obtain a lease on a Drydock host whose platform is \'%s\' and store '.
-      'the resulting lease in a host artifact called \'%s\'.',
-      $settings['platform'],
-      $settings['name']);
-  }
-
   public function execute(
     HarbormasterBuild $build,
     HarbormasterBuildTarget $build_target) {

src/applications/harbormaster/step/HarbormasterPublishFragmentBuildStepImplementation.php

@@ -12,12 +12,10 @@ final class HarbormasterPublishFragmentBuildStepImplementation
   }
 
   public function getDescription() {
-    $settings = $this->getSettings();
-
     return pht(
-      'Publish file artifact \'%s\' to the fragment path \'%s\'.',
+      'Publish file artifact %s as fragment %s.',
-      $settings['artifact'],
+      $this->formatSettingForDescription('artifact'),
-      $settings['path']);
+      $this->formatSettingForDescription('path'));
   }
 
   public function execute(

src/applications/harbormaster/step/HarbormasterSleepBuildStepImplementation.php

@@ -12,9 +12,9 @@ final class HarbormasterSleepBuildStepImplementation
   }
 
   public function getDescription() {
-    $settings = $this->getSettings();
+    return pht(
-
+      'Sleep for %s seconds.',
-    return pht('Sleep for %s seconds.', $settings['seconds']);
+      $this->formatSettingForDescription('seconds'));
   }
 
   public function execute(

src/applications/harbormaster/step/HarbormasterThrowExceptionBuildStep.php

@@ -11,10 +11,6 @@ final class HarbormasterThrowExceptionBuildStep
     return pht('Throw an exception.');
   }
 
-  public function getDescription() {
-    return pht('Throw an exception.');
-  }
-
   public function execute(
     HarbormasterBuild $build,
     HarbormasterBuildTarget $build_target) {

src/applications/harbormaster/step/HarbormasterUploadArtifactBuildStepImplementation.php

@@ -4,20 +4,18 @@ final class HarbormasterUploadArtifactBuildStepImplementation
   extends HarbormasterBuildStepImplementation {
 
   public function getName() {
-    return pht('Upload Artifact');
+    return pht('Upload File');
   }
 
   public function getGenericDescription() {
-    return pht('Upload an artifact from a Drydock host to Phabricator.');
+    return pht('Upload a file from a host to Phabricator.');
   }
 
   public function getDescription() {
-    $settings = $this->getSettings();
-
     return pht(
-      'Upload artifact located at \'%s\' on \'%s\'.',
+      'Upload %s from %s.',
-      $settings['path'],
+      $this->formatSettingForDescription('path'),
-      $settings['hostartifact']);
+      $this->formatSettingForDescription('hostartifact'));
   }
 
   public function execute(

src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php

@@ -13,12 +13,6 @@ final class HarbormasterWaitForPreviousBuildStepImplementation
       'before continuing.');
   }
 
-  public function getDescription() {
-    return pht(
-      'Wait for previous commits to finish building the current plan '.
-      'before continuing.');
-  }
-
   public function execute(
     HarbormasterBuild $build,
     HarbormasterBuildTarget $build_target) {

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

@@ -9,6 +9,7 @@ final class HarbormasterBuild extends HarbormasterDAO
 
   private $buildable = self::ATTACHABLE;
   private $buildPlan = self::ATTACHABLE;
+  private $buildTargets = self::ATTACHABLE;
   private $unprocessedCommands = self::ATTACHABLE;
 
   /**
@@ -102,6 +103,15 @@ final class HarbormasterBuild extends HarbormasterDAO
     return $this->assertAttached($this->buildPlan);
   }
 
+  public function getBuildTargets() {
+    return $this->assertAttached($this->buildTargets);
+  }
+
+  public function attachBuildTargets(array $targets) {
+    $this->buildTargets = $targets;
+    return $this;
+  }
+
   public function isBuilding() {
     return $this->getBuildStatus() === self::STATUS_PENDING ||
       $this->getBuildStatus() === self::STATUS_WAITING ||

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

@@ -16,6 +16,7 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
 
   private $build = self::ATTACHABLE;
   private $buildStep = self::ATTACHABLE;
+  private $implementation;
 
   public static function initializeNewBuildTarget(
     HarbormasterBuild $build,
@@ -82,24 +83,14 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
   }
 
   public function getImplementation() {
-    if ($this->className === null) {
+    if ($this->implementation === null) {
-      throw new Exception("No implementation set for the given target.");
+      $obj = HarbormasterBuildStepImplementation::requireImplementation(
+        $this->className);
+      $obj->loadSettings($this);
+      $this->implementation = $obj;
     }
 
-    static $implementations = null;
+    return $this->implementation;
-    if ($implementations === null) {
-      $implementations =
-        HarbormasterBuildStepImplementation::getImplementations();
-    }
-
-    $class = $this->className;
-    if (!in_array($class, $implementations)) {
-      throw new Exception(
-        "Class name '".$class."' does not extend BuildStepImplementation.");
-    }
-    $implementation = newv($class, array());
-    $implementation->loadSettings($this);
-    return $implementation;
   }
 
 
@@ -147,8 +138,7 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
   }
 
   public function describeAutomaticCapability($capability) {
-    return pht(
+    return pht('Users must be able to see a build to view its build targets.');
-      'Users must be able to see a build to view its build targets.');
   }
 
 }

src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php

@@ -12,6 +12,7 @@ final class HarbormasterBuildStep extends HarbormasterDAO
 
   private $buildPlan = self::ATTACHABLE;
   private $customFields = self::ATTACHABLE;
+  private $implementation;
 
   public function getConfiguration() {
     return array(
@@ -46,24 +47,14 @@ final class HarbormasterBuildStep extends HarbormasterDAO
   }
 
   public function getStepImplementation() {
-    if ($this->className === null) {
+    if ($this->implementation === null) {
-      throw new Exception("No implementation set for the given step.");
+      $obj = HarbormasterBuildStepImplementation::requireImplementation(
+        $this->className);
+      $obj->loadSettings($this);
+      $this->implementation = $obj;
     }
 
-    static $implementations = null;
+    return $this->implementation;
-    if ($implementations === null) {
-      $implementations =
-        HarbormasterBuildStepImplementation::getImplementations();
-    }
-
-    $class = $this->className;
-    if (!in_array($class, $implementations)) {
-      throw new Exception(
-        "Class name '".$class."' does not extend BuildStepImplementation.");
-    }
-    $implementation = newv($class, array());
-    $implementation->loadSettings($this);
-    return $implementation;
   }