Implement interface for adding, editing and deleting build steps on plans.

Summary: This implements an interface for adding new build steps, editing existing build steps and deleting build steps from build plans.  It uses the settings definitions on the build implementation to work out what fields should be displayed on the edit page.

Test Plan:
See screenshots:

{F78529}

{F78532}

{F78528}

{F78531}

{F78527}

{F78530}

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1049

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

src/__phutil_library_map__.php

@@ -687,6 +687,9 @@ phutil_register_library_map(array(
     'HarbormasterPlanViewController' => 'applications/harbormaster/controller/HarbormasterPlanViewController.php',
     'HarbormasterRemarkupRule' => 'applications/harbormaster/remarkup/HarbormasterRemarkupRule.php',
     'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php',
+    'HarbormasterStepAddController' => 'applications/harbormaster/controller/HarbormasterStepAddController.php',
+    'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php',
+    'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php',
     'HeraldAction' => 'applications/herald/storage/HeraldAction.php',
     'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php',
     'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php',
@@ -2919,6 +2922,9 @@ phutil_register_library_map(array(
     'HarbormasterPlanViewController' => 'HarbormasterPlanController',
     'HarbormasterRemarkupRule' => 'PhabricatorRemarkupRuleObject',
     'HarbormasterScratchTable' => 'HarbormasterDAO',
+    'HarbormasterStepAddController' => 'HarbormasterController',
+    'HarbormasterStepDeleteController' => 'HarbormasterController',
+    'HarbormasterStepEditController' => 'HarbormasterController',
     'HeraldAction' => 'HeraldDAO',
     'HeraldApplyTranscript' => 'HeraldDAO',
     'HeraldCapabilityManageGlobalRules' => 'PhabricatorPolicyCapability',

src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php

@@ -46,6 +46,11 @@ final class PhabricatorApplicationHarbormaster extends PhabricatorApplication {
           'edit/(?:(?P<id>\d+)/)?' => 'HarbormasterBuildableEditController',
           'apply/(?:(?P<id>\d+)/)?' => 'HarbormasterBuildableApplyController',
         ),
+        'step/' => array(
+          'add/(?:(?P<id>\d+)/)?' => 'HarbormasterStepAddController',
+          'edit/(?:(?P<id>\d+)/)?' => 'HarbormasterStepEditController',
+          'delete/(?:(?P<id>\d+)/)?' => 'HarbormasterStepDeleteController',
+        ),
         'plan/' => array(
           '(?:query/(?P<queryKey>[^/]+)/)?'
             => 'HarbormasterPlanListController',

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

@@ -62,6 +62,10 @@ final class HarbormasterBuildableViewController
           $item->setBarColor('red');
           $item->addAttribute(pht('Failed'));
           break;
+        case HarbormasterBuild::STATUS_ERROR:
+          $item->setBarColor('red');
+          $item->addAttribute(pht('Unexpected Error'));
+          break;
       }
       $build_list->addItem($item);
     }

src/applications/harbormaster/controller/HarbormasterPlanViewController.php

@@ -55,10 +55,13 @@ final class HarbormasterPlanViewController
       id(new PhabricatorCrumbView())
         ->setName(pht("Plan %d", $id)));
 
+    $step_list = $this->buildStepList($plan);
+
     return $this->buildApplicationPage(
       array(
         $crumbs,
         $box,
+        $step_list,
         $xaction_view,
       ),
       array(
@@ -67,6 +70,56 @@ final class HarbormasterPlanViewController
       ));
   }
 
+  private function buildStepList(HarbormasterBuildPlan $plan) {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $steps = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withBuildPlanPHIDs(array($plan->getPHID()))
+      ->execute();
+
+    $can_edit = $this->hasApplicationCapability(
+      HarbormasterCapabilityManagePlans::CAPABILITY);
+
+    $i = 1;
+    $step_list = id(new PHUIObjectItemListView())
+      ->setUser($viewer);
+    foreach ($steps as $step) {
+      $implementation = $step->getStepImplementation();
+      $item = id(new PHUIObjectItemView())
+        ->setObjectName("Step ".$i++)
+        ->setHeader($implementation->getName());
+
+      if (!$implementation->validateSettings()) {
+        $item
+          ->setBarColor('red')
+          ->addAttribute(pht('This step is not configured correctly.'));
+      } else {
+        $item->addAttribute($implementation->getDescription());
+      }
+
+      if ($can_edit) {
+        $edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/");
+        $item
+          ->setHref($edit_uri)
+          ->addAction(
+            id(new PHUIListItemView())
+              ->setIcon('delete')
+              ->addSigil('harbormaster-build-step-delete')
+              ->setWorkflow(true)
+              ->setRenderNameAsTooltip(true)
+              ->setName(pht("Delete"))
+              ->setHref(
+                $this->getApplicationURI("step/delete/".$step->getID()."/")));
+      }
+
+      $step_list->addItem($item);
+    }
+
+    return $step_list;
+  }
+
   private function buildActionList(HarbormasterBuildPlan $plan) {
     $request = $this->getRequest();
     $viewer = $request->getUser();
@@ -88,6 +141,14 @@ final class HarbormasterPlanViewController
         ->setDisabled(!$can_edit)
         ->setIcon('edit'));
 
+    $list->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Add Build Step'))
+        ->setHref($this->getApplicationURI("step/add/{$id}/"))
+        ->setWorkflow($can_edit)
+        ->setDisabled(!$can_edit)
+        ->setIcon('new'));
+
     return $list;
   }
 

src/applications/harbormaster/controller/HarbormasterStepAddController.php

@@ -0,0 +1,84 @@
+<?php
+
+final class HarbormasterStepAddController
+  extends HarbormasterController {
+
+  private $id;
+
+  public function willProcessRequest(array $data) {
+    $this->id = $data['id'];
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $this->requireApplicationCapability(
+      HarbormasterCapabilityManagePlans::CAPABILITY);
+
+    $id = $this->id;
+
+    $plan = id(new HarbormasterBuildPlanQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($id))
+        ->executeOne();
+    if ($plan === null) {
+      throw new Exception("Build plan not found!");
+    }
+
+    $implementations = BuildStepImplementation::getImplementations();
+
+    $cancel_uri = $this->getApplicationURI('plan/'.$plan->getID().'/');
+
+    if ($request->isDialogFormPost()) {
+      $class = $request->getStr('step-type');
+      if (!in_array($class, $implementations)) {
+        return $this->createDialog($implementations);
+      }
+
+      $step = new HarbormasterBuildStep();
+      $step->setBuildPlanPHID($plan->getPHID());
+      $step->setClassName($class);
+      $step->setDetails(array());
+      $step->save();
+
+      $edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/");
+
+      return id(new AphrontRedirectResponse())->setURI($edit_uri);
+    }
+
+    return $this->createDialog($implementations, $cancel_uri);
+  }
+
+  function createDialog(array $implementations, $cancel_uri) {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $control = id(new AphrontFormRadioButtonControl())
+      ->setName('step-type');
+
+    foreach ($implementations as $implementation_name) {
+      $implementation = new $implementation_name();
+      $control
+        ->addButton(
+          $implementation_name,
+          $implementation->getName(),
+          $implementation->getGenericDescription());
+    }
+
+    $dialog = new AphrontDialogView();
+    $dialog->setTitle(pht('Add New Step'))
+            ->setUser($viewer)
+            ->addSubmitButton(pht('Add Build Step'))
+            ->addCancelButton($cancel_uri);
+    $dialog->appendChild(
+      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/controller/HarbormasterStepDeleteController.php

@@ -0,0 +1,52 @@
+<?php
+
+final class HarbormasterStepDeleteController
+  extends HarbormasterController {
+
+  private $id;
+
+  public function willProcessRequest(array $data) {
+    $this->id = $data['id'];
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $this->requireApplicationCapability(
+      HarbormasterCapabilityManagePlans::CAPABILITY);
+
+    $id = $this->id;
+
+    $step = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->executeOne();
+    if ($step === null) {
+      throw new Exception("Build step not found!");
+    }
+
+    $plan_id = $step->getBuildPlan()->getID();
+    $done_uri = $this->getApplicationURI('plan/'.$plan_id.'/');
+
+    if ($request->isDialogFormPost()) {
+      $step->delete();
+      return id(new AphrontRedirectResponse())->setURI($done_uri);
+    }
+
+    $dialog = new AphrontDialogView();
+    $dialog->setTitle(pht('Really Delete Step?'))
+            ->setUser($viewer)
+            ->addSubmitButton(pht('Delete Build Step'))
+            ->addCancelButton($done_uri);
+    $dialog->appendChild(
+      phutil_tag(
+        'p',
+        array(),
+        pht(
+          'Are you sure you want to delete this '.
+          'step? This can\'t be undone!')));
+    return id(new AphrontDialogResponse())->setDialog($dialog);
+  }
+
+}

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -0,0 +1,156 @@
+<?php
+
+final class HarbormasterStepEditController
+  extends HarbormasterController {
+
+  private $id;
+
+  public function willProcessRequest(array $data) {
+    $this->id = idx($data, 'id');
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $this->requireApplicationCapability(
+      HarbormasterCapabilityManagePlans::CAPABILITY);
+
+    $step = id(new HarbormasterBuildStepQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
+    if (!$step) {
+      return new Aphront404Response();
+    }
+
+    $plan = id(new HarbormasterBuildPlanQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($step->getBuildPlanPHID()))
+      ->executeOne();
+    if (!$plan) {
+      return new Aphront404Response();
+    }
+
+    $implementation = $step->getStepImplementation();
+    $implementation->validateSettingDefinitions();
+    $settings = $implementation->getSettings();
+
+    $errors = array();
+    if ($request->isFormPost()) {
+      foreach ($implementation->getSettingDefinitions() as $name => $opt) {
+        $readable_name = $this->getReadableName($name, $opt);
+        $value = $this->getValueFromRequest($request, $name, $opt['type']);
+
+        // TODO: This won't catch any validation issues unless the field
+        // is missing completely.  How should we check if the user is
+        // required to enter an integer?
+        if ($value === null) {
+          $errors[] = $readable_name.' is not valid.';
+        } else {
+          $step->setDetail($name, $value);
+        }
+      }
+
+      if (count($errors) === 0) {
+        $step->save();
+
+        return id(new AphrontRedirectResponse())
+          ->setURI($this->getApplicationURI('plan/'.$plan->getID().'/'));
+      }
+    }
+
+    $form = id(new AphrontFormView())
+      ->setUser($viewer);
+
+    // We need to render out all of the fields for the settings that
+    // the implementation has.
+    foreach ($implementation->getSettingDefinitions() as $name => $opt) {
+      if ($request->isFormPost()) {
+        $value = $this->getValueFromRequest($request, $name, $opt['type']);
+      } else {
+        $value = $settings[$name];
+      }
+
+      switch ($opt['type']) {
+        case BuildStepImplementation::SETTING_TYPE_STRING:
+        case BuildStepImplementation::SETTING_TYPE_INTEGER:
+          $control = id(new AphrontFormTextControl())
+            ->setLabel($this->getReadableName($name, $opt))
+            ->setName($name)
+            ->setValue($value);
+          break;
+        case BuildStepImplementation::SETTING_TYPE_BOOLEAN:
+          $control = id(new AphrontFormCheckboxControl())
+            ->setLabel($this->getReadableName($name, $opt))
+            ->setName($name)
+            ->setValue($value);
+          break;
+        default:
+          throw new Exception("Unable to render field with unknown type.");
+      }
+
+      if (isset($opt['description'])) {
+        $control->setCaption($opt['description']);
+      }
+
+      $form->appendChild($control);
+    }
+
+    $form->appendChild(
+      id(new AphrontFormSubmitControl())
+        ->setValue(pht('Save Step Configuration'))
+        ->addCancelButton(
+          $this->getApplicationURI('plan/'.$plan->getID().'/')));
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText('Edit Step: '.$implementation->getName())
+      ->setValidationException(null)
+      ->setForm($form);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $id = $plan->getID();
+    $crumbs->addCrumb(
+      id(new PhabricatorCrumbView())
+        ->setName(pht("Plan %d", $id))
+        ->setHref($this->getApplicationURI("plan/{$id}/")));
+    $crumbs->addCrumb(
+      id(new PhabricatorCrumbView())
+        ->setName(pht('Edit Step')));
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $box,
+      ),
+      array(
+        'title' => $implementation->getName(),
+        'device' => true,
+      ));
+  }
+
+  public function getReadableName($name, $opt) {
+    $readable_name = $name;
+    if (isset($opt['name'])) {
+      $readable_name = $opt['name'];
+    }
+    return $readable_name;
+  }
+
+  public function getValueFromRequest(AphrontRequest $request, $name, $type) {
+    switch ($type) {
+      case BuildStepImplementation::SETTING_TYPE_STRING:
+        return $request->getStr($name);
+        break;
+      case BuildStepImplementation::SETTING_TYPE_INTEGER:
+        return $request->getInt($name);
+        break;
+      case BuildStepImplementation::SETTING_TYPE_BOOLEAN:
+        return $request->getBool($name);
+        break;
+      default:
+        throw new Exception("Unsupported setting type '".$type."'.");
+    }
+  }
+
+}

src/applications/harbormaster/query/HarbormasterBuildStepQuery.php

@@ -22,6 +22,14 @@ final class HarbormasterBuildStepQuery
     return $this;
   }
 
+  public function getPagingColumn() {
+    return 'id';
+  }
+
+  public function getReversePaging() {
+    return true;
+  }
+
   protected function loadPage() {
     $table = new HarbormasterBuildStep();
     $conn_r = $table->establishConnection('r');

src/applications/harbormaster/step/BuildStepImplementation.php

@@ -4,13 +4,32 @@ abstract class BuildStepImplementation {
 
   private $settings;
 
+  const SETTING_TYPE_STRING = 'string';
+  const SETTING_TYPE_INTEGER = 'integer';
+  const SETTING_TYPE_BOOLEAN = 'boolean';
+
+  public static function getImplementations() {
+    $symbols = id(new PhutilSymbolLoader())
+      ->setAncestorClass("BuildStepImplementation")
+      ->setConcreteOnly(true)
+      ->selectAndLoadSymbols();
+    return ipull($symbols, 'name');
+  }
+
   /**
    * The name of the implementation.
    */
   abstract public function getName();
 
   /**
-   * The description of the implementation.
+   * The generic description of the implementation.
+   */
+  public function getGenericDescription() {
+    return '';
+  }
+
+  /**
+   * The description of the implementation, based on the current settings.
    */
   public function getDescription() {
     return '';
@@ -24,21 +43,42 @@ abstract class BuildStepImplementation {
   /**
    * Gets the settings for this build step.
    */
-  protected function getSettings() {
+  public function getSettings() {
     return $this->settings;
   }
 
+  /**
+   * Validate the current settings of this build step.
+   */
+  public function validate() {
+    return true;
+  }
+
   /**
    * Loads the settings for this build step implementation from the build step.
    */
   public final function loadSettings(HarbormasterBuildStep $build_step) {
     $this->settings = array();
+    $this->validateSettingDefinitions();
     foreach ($this->getSettingDefinitions() as $name => $opt) {
       $this->settings[$name] = $build_step->getDetail($name);
     }
     return $this->settings;
   }
 
+  /**
+   * Validates that the setting definitions for this implementation are valid.
+   */
+  public final function validateSettingDefinitions() {
+    foreach ($this->getSettingDefinitions() as $name => $opt) {
+      if (!isset($opt['type'])) {
+        throw new Exception(
+          'Setting definition \''.$name.
+          '\' is missing type requirement.');
+      }
+    }
+  }
+
   /**
    * Return an array of settings for this step implementation.
    */

src/applications/harbormaster/step/SleepBuildStepImplementation.php

@@ -6,19 +6,40 @@ final class SleepBuildStepImplementation extends BuildStepImplementation {
     return pht('Sleep');
   }
 
-  public function getDescription() {
+  public function getGenericDescription() {
     return pht('Sleep for a specified number of seconds.');
   }
 
+  public function getDescription() {
+    $settings = $this->getSettings();
+
+    return pht('Sleep for %s seconds.', $settings['seconds']);
+  }
+
   public function execute(HarbormasterBuild $build) {
     $settings = $this->getSettings();
 
     sleep($settings['seconds']);
   }
 
+  public function validateSettings() {
+    $settings = $this->getSettings();
+
+    if ($settings['seconds'] === null) {
+      return false;
+    }
+    if (!is_int($settings['seconds'])) {
+      return false;
+    }
+    return true;
+  }
+
   public function getSettingDefinitions() {
     return array(
-      'seconds' => array());
+      'seconds' => array(
+        'name' => 'Seconds',
+        'description' => 'The number of seconds to sleep for.',
+        'type' => BuildStepImplementation::SETTING_TYPE_INTEGER));
   }
 
 }

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

@@ -40,6 +40,11 @@ final class HarbormasterBuild extends HarbormasterDAO
    */
   const STATUS_FAILED = 'failed';
 
+  /**
+   * The build encountered an unexpected error.
+   */
+  const STATUS_ERROR = 'error';
+
   public static function initializeNewBuild(PhabricatorUser $actor) {
     return id(new HarbormasterBuild())
       ->setBuildStatus(self::STATUS_INACTIVE);

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

@@ -46,9 +46,16 @@ final class HarbormasterBuildStep extends HarbormasterDAO
       throw new Exception("No implementation set for the given step.");
     }
 
-    // TODO: We should look up the class in phutil's system to ensure
-    // that it actually extends BuildStepImplementation.
+    static $implementations = null;
+    if ($implementations === null) {
+      $implementations = BuildStepImplementation::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;

src/applications/harbormaster/worker/HarbormasterBuildWorker.php

@@ -40,6 +40,10 @@ final class HarbormasterBuildWorker extends PhabricatorWorker {
       // Perform the build.
       foreach ($steps as $step) {
         $implementation = $step->getStepImplementation();
+        if (!$implementation->validateSettings()) {
+          $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
+          break;
+        }
         $implementation->execute($build);
         if ($build->getBuildStatus() !== HarbormasterBuild::STATUS_BUILDING) {
           break;
@@ -56,7 +60,7 @@ final class HarbormasterBuildWorker extends PhabricatorWorker {
       // If any exception is raised, the build is marked as a failure and
       // the exception is re-thrown (this ensures we don't leave builds
       // in an inconsistent state).
-      $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED);
+      $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR);
       $build->save();
       throw $e;
     }