Don't create invalid build steps while adding them

Summary:
Ref T1049. Currently, the "add" dialog lets you select a build step type, but then immediately creates one. If you "cancel" from the edit screen, you end up with an empty (and almost certainly invalid) build step.

Instead, don't create the step until it's valid.

Test Plan: Add Step -> Pick Type -> Add Step -> Cancel no longer creates empty step.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

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

src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php

@@ -50,6 +50,8 @@ final class PhabricatorApplicationHarbormaster extends PhabricatorApplication {
           => 'HarbormasterBuildableListController',
         'step/' => array(
           'add/(?:(?P<id>\d+)/)?' => 'HarbormasterStepAddController',
+          'new/(?P<plan>\d+)/(?P<class>[^/]+)/'
+            => 'HarbormasterStepEditController',
           'edit/(?:(?P<id>\d+)/)?' => 'HarbormasterStepEditController',
           'delete/(?:(?P<id>\d+)/)?' => 'HarbormasterStepDeleteController',
         ),

src/applications/harbormaster/controller/HarbormasterStepAddController.php

@@ -16,47 +16,32 @@ final class HarbormasterStepAddController
     $this->requireApplicationCapability(
       HarbormasterCapabilityManagePlans::CAPABILITY);
 
-    $id = $this->id;
-
     $plan = id(new HarbormasterBuildPlanQuery())
         ->setViewer($viewer)
-        ->withIDs(array($id))
+        ->withIDs(array($this->id))
         ->executeOne();
     if (!$plan) {
       return new Aphront404Response();
     }
 
-    $cancel_uri = $this->getApplicationURI('plan/'.$plan->getID().'/');
+    $plan_id = $plan->getID();
+    $cancel_uri = $this->getApplicationURI("plan/{$plan_id}/");
 
-    if ($request->isDialogFormPost()) {
-      $class = $request->getStr('step-type');
+    $errors = array();
+    if ($request->isFormPost()) {
+      $class = $request->getStr('class');
       if (!HarbormasterBuildStepImplementation::getImplementation($class)) {
-        return $this->createDialog($cancel_uri);
+        $errors[] = pht(
+          'Choose the type of build step you want to add.');
+      }
+      if (!$errors) {
+        $new_uri = $this->getApplicationURI("step/new/{$plan_id}/{$class}/");
+        return id(new AphrontRedirectResponse())->setURI($new_uri);
       }
-
-      $steps = $plan->loadOrderedBuildSteps();
-
-      $step = new HarbormasterBuildStep();
-      $step->setBuildPlanPHID($plan->getPHID());
-      $step->setClassName($class);
-      $step->setDetails(array());
-      $step->setSequence(count($steps) + 1);
-      $step->save();
-
-      $edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/");
-
-      return id(new AphrontRedirectResponse())->setURI($edit_uri);
     }
 
-    return $this->createDialog($cancel_uri);
-  }
-
-  private function createDialog($cancel_uri) {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-
     $control = id(new AphrontFormRadioButtonControl())
-      ->setName('step-type');
+      ->setName('class');
 
     $all = HarbormasterBuildStepImplementation::getImplementations();
     foreach ($all as $class => $implementation) {
@@ -66,10 +51,16 @@ final class HarbormasterStepAddController
         $implementation->getGenericDescription());
     }
 
+    if ($errors) {
+      $errors = id(new AphrontErrorView())
+        ->setErrors($errors);
+    }
+
     return $this->newDialog()
       ->setTitle(pht('Add New Step'))
       ->addSubmitButton(pht('Add Build Step'))
       ->addCancelButton($cancel_uri)
+      ->appendChild($errors)
       ->appendParagraph(pht('Choose a type of build step to add:'))
       ->appendChild($control);
   }

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -4,9 +4,13 @@ final class HarbormasterStepEditController
   extends HarbormasterController {
 
   private $id;
+  private $planID;
+  private $className;
 
   public function willProcessRequest(array $data) {
     $this->id = idx($data, 'id');
+    $this->planID = idx($data, 'plan');
+    $this->className = idx($data, 'class');
   }
 
   public function processRequest() {
@@ -16,15 +20,40 @@ final class HarbormasterStepEditController
     $this->requireApplicationCapability(
       HarbormasterCapabilityManagePlans::CAPABILITY);
 
-    $step = id(new HarbormasterBuildStepQuery())
-      ->setViewer($viewer)
-      ->withIDs(array($this->id))
-      ->executeOne();
-    if (!$step) {
-      return new Aphront404Response();
+    if ($this->id) {
+      $step = id(new HarbormasterBuildStepQuery())
+        ->setViewer($viewer)
+        ->withIDs(array($this->id))
+        ->executeOne();
+      if (!$step) {
+        return new Aphront404Response();
+      }
+      $plan = $step->getBuildPlan();
+
+      $is_new = false;
+    } else {
+      $plan = id(new HarbormasterBuildPlanQuery())
+          ->setViewer($viewer)
+          ->withIDs(array($this->planID))
+          ->executeOne();
+      if (!$plan) {
+        return new Aphront404Response();
+      }
+
+      $impl = HarbormasterBuildStepImplementation::getImplementation(
+        $this->className);
+      if (!$impl) {
+        return new Aphront404Response();
+      }
+
+      $step = HarbormasterBuildStep::initializeNewStep($viewer)
+        ->setBuildPlanPHID($plan->getPHID())
+        ->setClassName($this->className);
+
+      $is_new = true;
     }
 
-    $plan = $step->getBuildPlan();
+    $plan_uri = $this->getApplicationURI('plan/'.$plan->getID().'/');
 
     $implementation = $step->getStepImplementation();
 
@@ -47,10 +76,16 @@ final class HarbormasterStepEditController
         ->setContinueOnNoEffect(true)
         ->setContentSourceFromRequest($request);
 
+      if ($is_new) {
+        // This is okay, but a little iffy. We should move it inside the editor
+        // if we create plans elsewhere.
+        $steps = $plan->loadOrderedBuildSteps();
+        $step->setSequence(count($steps) + 1);
+      }
+
       try {
         $editor->applyTransactions($step, $xactions);
-        return id(new AphrontRedirectResponse())
-          ->setURI($this->getApplicationURI('plan/'.$plan->getID().'/'));
+        return id(new AphrontRedirectResponse())->setURI($plan_uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
         $validation_exception = $ex;
       }
@@ -61,36 +96,47 @@ final class HarbormasterStepEditController
 
     $field_list->appendFieldsToForm($form);
 
+    if ($is_new) {
+      $submit = pht('Create Build Step');
+      $header = pht('New Step: %s', $implementation->getName());
+      $crumb = pht('Add Step');
+    } else {
+      $submit = pht('Save Build Step');
+      $header = pht('Edit Step: %s', $implementation->getName());
+      $crumb = pht('Edit Step');
+    }
+
     $form->appendChild(
       id(new AphrontFormSubmitControl())
-        ->setValue(pht('Save Build Step'))
-        ->addCancelButton(
-          $this->getApplicationURI('plan/'.$plan->getID().'/')));
+        ->setValue($submit)
+        ->addCancelButton($plan_uri));
 
     $box = id(new PHUIObjectBoxView())
-      ->setHeaderText('Edit Step: '.$implementation->getName())
+      ->setHeaderText($header)
       ->setValidationException($validation_exception)
       ->setForm($form);
 
     $crumbs = $this->buildApplicationCrumbs();
     $id = $plan->getID();
-    $crumbs->addTextCrumb(
-      pht("Plan %d", $id),
-      $this->getApplicationURI("plan/{$id}/"));
-    $crumbs->addTextCrumb(pht('Edit Step'));
+    $crumbs->addTextCrumb(pht('Plan %d', $id), $plan_uri);
+    $crumbs->addTextCrumb($crumb);
 
     $variables = $this->renderBuildVariablesTable();
 
-    $xactions = id(new HarbormasterBuildStepTransactionQuery())
-      ->setViewer($viewer)
-      ->withObjectPHIDs(array($step->getPHID()))
-      ->execute();
+    if ($is_new) {
+      $xaction_view = null;
+    } else {
+      $xactions = id(new HarbormasterBuildStepTransactionQuery())
+        ->setViewer($viewer)
+        ->withObjectPHIDs(array($step->getPHID()))
+        ->execute();
 
-    $xaction_view = id(new PhabricatorApplicationTransactionView())
-      ->setUser($viewer)
-      ->setObjectPHID($step->getPHID())
-      ->setTransactions($xactions)
-      ->setShouldTerminate(true);
+      $xaction_view = id(new PhabricatorApplicationTransactionView())
+        ->setUser($viewer)
+        ->setObjectPHID($step->getPHID())
+        ->setTransactions($xactions)
+        ->setShouldTerminate(true);
+    }
 
     return $this->buildApplicationPage(
       array(

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

@@ -14,6 +14,10 @@ final class HarbormasterBuildStep extends HarbormasterDAO
   private $customFields = self::ATTACHABLE;
   private $implementation;
 
+  public static function initializeNewStep(PhabricatorUser $actor) {
+    return id(new HarbormasterBuildStep());
+  }
+
   public function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,