Use tabs on build targets and allow build steps to have a description

Summary:
Ref T1049. This uses tabs on build targets to hide the configuration details and variables by default, instead promoting the target name, it's status and a description of the build step.  The description is a new field on each build step.

The primary advantage of having a description on build steps is that DevOps can configure appropriate description information (including any troubleshooting information for build failures) on build steps, and developers who have builds fail against their code review can then look at this information.

Test Plan: Viewed a build plan and saw the appropriate information.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T1049

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

resources/sql/autopatches/20140731.harbormasterstepdesc.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildstep
+  ADD description LONGTEXT NOT NULL COLLATE utf8_bin;

src/applications/harbormaster/controller/HarbormasterBuildViewController.php

@@ -52,6 +52,7 @@ final class HarbormasterBuildViewController
 
     $build_targets = id(new HarbormasterBuildTargetQuery())
       ->setViewer($viewer)
+      ->needBuildSteps(true)
       ->withBuildPHIDs(array($build->getPHID()))
       ->execute();
 
@@ -69,32 +70,68 @@ final class HarbormasterBuildViewController
     $targets = array();
     foreach ($build_targets as $build_target) {
       $header = id(new PHUIHeaderView())
-        ->setHeader(pht(
-          'Build Target %d (%s)',
-          $build_target->getID(),
-          $build_target->getName()))
+        ->setHeader($build_target->getName())
         ->setUser($viewer);
+
+      $target_box = id(new PHUIObjectBoxView())
+        ->setHeader($header);
+
       $properties = new PHUIPropertyListView();
+      $status_view = new PHUIStatusListView();
+
+      $item = new PHUIStatusItemView();
+
+      $status = $build_target->getTargetStatus();
+      $status_name =
+        HarbormasterBuildTarget::getBuildTargetStatusName($status);
+      $icon = HarbormasterBuildTarget::getBuildTargetStatusIcon($status);
+      $color = HarbormasterBuildTarget::getBuildTargetStatusColor($status);
+
+      $item->setTarget($status_name);
+      $item->setIcon($icon, $color);
+      $status_view->addItem($item);
+
+      $properties->addProperty(pht('Name'), $build_target->getName());
+      $properties->addProperty(pht('Status'), $status_view);
+
+      $target_box->addPropertyList($properties, pht('Overview'));
+
+      $description = $build_target->getBuildStep()->getDescription();
+      if ($description) {
+        $rendered = PhabricatorMarkupEngine::renderOneObject(
+          id(new PhabricatorMarkupOneOff())
+            ->setContent($description)
+            ->setPreserveLinebreaks(true),
+          'default',
+          $viewer);
+
+        $properties->addSectionHeader(pht('Description'));
+        $properties->addTextContent($rendered);
+      }
 
       $details = $build_target->getDetails();
       if ($details) {
-        $properties->addSectionHeader(pht('Configuration Details'));
+        $properties = new PHUIPropertyListView();
         foreach ($details as $key => $value) {
           $properties->addProperty($key, $value);
         }
+        $target_box->addPropertyList($properties, pht('Configuration'));
       }
 
       $variables = $build_target->getVariables();
       if ($variables) {
-        $properties->addSectionHeader(pht('Variables'));
+        $properties = new PHUIPropertyListView();
         foreach ($variables as $key => $value) {
           $properties->addProperty($key, $value);
         }
+        $target_box->addPropertyList($properties, pht('Variables'));
       }
 
-      $targets[] = id(new PHUIObjectBoxView())
-        ->setHeader($header)
-        ->addPropertyList($properties);
+      $properties = new PHUIPropertyListView();
+      $properties->addProperty('Build Target ID', $build_target->getID());
+      $target_box->addPropertyList($properties, pht('Metadata'));
+
+      $targets[] = $target_box;
 
       $build_messages = idx($messages, $build_target->getPHID(), array());
       if ($build_messages) {

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -65,6 +65,8 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
     $e_name = true;
     $v_name = $step->getName();
+    $e_description = true;
+    $v_description = $step->getDescription();
     $e_depends_on = true;
     $raw_depends_on = $step->getDetail('dependsOn', array());
 
@@ -78,6 +80,8 @@ final class HarbormasterStepEditController extends HarbormasterController {
     if ($request->isFormPost()) {
       $e_name = null;
       $v_name = $request->getStr('name');
+      $e_description = null;
+      $v_description = $request->getStr('description');
       $e_depends_on = null;
       $v_depends_on = $request->getArr('dependsOn');
 
@@ -101,6 +105,12 @@ final class HarbormasterStepEditController extends HarbormasterController {
         ->setNewValue($v_depends_on);
       array_unshift($xactions, $depends_on_xaction);
 
+      $description_xaction = id(new HarbormasterBuildStepTransaction())
+        ->setTransactionType(
+          HarbormasterBuildStepTransaction::TYPE_DESCRIPTION)
+        ->setNewValue($v_description);
+      array_unshift($xactions, $description_xaction);
+
       if ($is_new) {
         // When creating a new step, make sure we have a create transaction
         // so we'll apply the transactions even if the step has no
@@ -142,6 +152,14 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
     $field_list->appendFieldsToForm($form);
 
+    $form
+      ->appendChild(
+        id(new PhabricatorRemarkupControl())
+          ->setName('description')
+          ->setLabel(pht('Description'))
+          ->setError($e_description)
+          ->setValue($v_description));
+
     if ($is_new) {
       $submit = pht('Create Build Step');
       $header = pht('New Step: %s', $implementation->getName());

src/applications/harbormaster/editor/HarbormasterBuildStepEditor.php

@@ -9,6 +9,7 @@ final class HarbormasterBuildStepEditor
     $types[] = HarbormasterBuildStepTransaction::TYPE_CREATE;
     $types[] = HarbormasterBuildStepTransaction::TYPE_NAME;
     $types[] = HarbormasterBuildStepTransaction::TYPE_DEPENDS_ON;
+    $types[] = HarbormasterBuildStepTransaction::TYPE_DESCRIPTION;
 
     return $types;
   }
@@ -30,6 +31,11 @@ final class HarbormasterBuildStepEditor
           return null;
         }
         return $object->getDetail('dependsOn', array());
+      case HarbormasterBuildStepTransaction::TYPE_DESCRIPTION:
+        if ($this->getIsNewObject()) {
+          return null;
+        }
+        return $object->getDescription();
     }
 
     return parent::getCustomTransactionOldValue($object, $xaction);
@@ -44,6 +50,7 @@ final class HarbormasterBuildStepEditor
         return true;
       case HarbormasterBuildStepTransaction::TYPE_NAME:
       case HarbormasterBuildStepTransaction::TYPE_DEPENDS_ON:
+      case HarbormasterBuildStepTransaction::TYPE_DESCRIPTION:
         return $xaction->getNewValue();
     }
 
@@ -61,6 +68,8 @@ final class HarbormasterBuildStepEditor
         return $object->setName($xaction->getNewValue());
       case HarbormasterBuildStepTransaction::TYPE_DEPENDS_ON:
         return $object->setDetail('dependsOn', $xaction->getNewValue());
+      case HarbormasterBuildStepTransaction::TYPE_DESCRIPTION:
+        return $object->setDescription($xaction->getNewValue());
     }
 
     return parent::applyCustomInternalTransaction($object, $xaction);
@@ -74,6 +83,7 @@ final class HarbormasterBuildStepEditor
       case HarbormasterBuildStepTransaction::TYPE_CREATE:
       case HarbormasterBuildStepTransaction::TYPE_NAME:
       case HarbormasterBuildStepTransaction::TYPE_DEPENDS_ON:
+      case HarbormasterBuildStepTransaction::TYPE_DESCRIPTION:
         return;
     }
 

src/applications/harbormaster/query/HarbormasterBuildTargetQuery.php

@@ -6,6 +6,7 @@ final class HarbormasterBuildTargetQuery
   private $ids;
   private $phids;
   private $buildPHIDs;
+  private $needBuildSteps;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -22,6 +23,11 @@ final class HarbormasterBuildTargetQuery
     return $this;
   }
 
+  public function needBuildSteps($need_build_steps) {
+    $this->needBuildSteps = $need_build_steps;
+    return $this;
+  }
+
   protected function loadPage() {
     $table = new HarbormasterBuildTarget();
     $conn_r = $table->establishConnection('r');
@@ -66,6 +72,31 @@ final class HarbormasterBuildTargetQuery
     return $this->formatWhereClause($where);
   }
 
+  protected function didFilterPage(array $page) {
+    if ($this->needBuildSteps) {
+      $step_phids = array();
+
+      foreach ($page as $target) {
+        $step_phids[] = $target->getBuildStepPHID();
+      }
+
+      $steps = id(new HarbormasterBuildStepQuery())
+        ->setViewer($this->getViewer())
+        ->setParentQuery($this)
+        ->withPHIDs($step_phids)
+        ->execute();
+
+      $steps = mpull($steps, null, 'getPHID');
+
+      foreach ($page as $target) {
+        $target->attachBuildStep(
+          $steps[$target->getBuildStepPHID()]);
+      }
+    }
+
+    return $page;
+  }
+
   protected function willFilterPage(array $page) {
     $builds = array();
 

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

@@ -21,6 +21,54 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
   private $buildStep = self::ATTACHABLE;
   private $implementation;
 
+  public static function getBuildTargetStatusName($status) {
+    switch ($status) {
+      case self::STATUS_PENDING:
+        return pht('Pending');
+      case self::STATUS_BUILDING:
+        return pht('Building');
+      case self::STATUS_WAITING:
+        return pht('Waiting for Message');
+      case self::STATUS_PASSED:
+        return pht('Passed');
+      case self::STATUS_FAILED:
+        return pht('Failed');
+      default:
+        return pht('Unknown');
+    }
+  }
+
+  public static function getBuildTargetStatusIcon($status) {
+    switch ($status) {
+      case self::STATUS_PENDING:
+        return PHUIStatusItemView::ICON_OPEN;
+      case self::STATUS_BUILDING:
+      case self::STATUS_WAITING:
+        return PHUIStatusItemView::ICON_RIGHT;
+      case self::STATUS_PASSED:
+        return PHUIStatusItemView::ICON_ACCEPT;
+      case self::STATUS_FAILED:
+        return PHUIStatusItemView::ICON_REJECT;
+      default:
+        return PHUIStatusItemView::ICON_QUESTION;
+    }
+  }
+
+  public static function getBuildTargetStatusColor($status) {
+    switch ($status) {
+      case self::STATUS_PENDING:
+      case self::STATUS_BUILDING:
+      case self::STATUS_WAITING:
+        return 'blue';
+      case self::STATUS_PASSED:
+        return 'green';
+      case self::STATUS_FAILED:
+        return 'red';
+      default:
+        return 'bluegrey';
+    }
+  }
+
   public static function initializeNewBuildTarget(
     HarbormasterBuild $build,
     HarbormasterBuildStep $build_step,

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

@@ -6,6 +6,7 @@ final class HarbormasterBuildStep extends HarbormasterDAO
     PhabricatorCustomFieldInterface {
 
   protected $name;
+  protected $description;
   protected $buildPlanPHID;
   protected $className;
   protected $details = array();

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

@@ -6,6 +6,7 @@ final class HarbormasterBuildStepTransaction
   const TYPE_CREATE = 'harbormaster:step:create';
   const TYPE_NAME = 'harbormaster:step:name';
   const TYPE_DEPENDS_ON = 'harbormaster:step:depends';
+  const TYPE_DESCRIPTION = 'harbormaster:step:description';
 
   public function getApplicationName() {
     return 'harbormaster';