From 6e3c17e6f9fa556331cf3688eacc2a04db02d2a4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 25 Mar 2014 16:12:05 -0700 Subject: [PATCH] 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 --- .../PhabricatorApplicationHarbormaster.php | 2 + .../HarbormasterStepAddController.php | 47 ++++---- .../HarbormasterStepEditController.php | 100 +++++++++++++----- .../configuration/HarbormasterBuildStep.php | 4 + 4 files changed, 98 insertions(+), 55 deletions(-) diff --git a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php index da102ccc79..4e3c61a64f 100644 --- a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php +++ b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php @@ -50,6 +50,8 @@ final class PhabricatorApplicationHarbormaster extends PhabricatorApplication { => 'HarbormasterBuildableListController', 'step/' => array( 'add/(?:(?P\d+)/)?' => 'HarbormasterStepAddController', + 'new/(?P\d+)/(?P[^/]+)/' + => 'HarbormasterStepEditController', 'edit/(?:(?P\d+)/)?' => 'HarbormasterStepEditController', 'delete/(?:(?P\d+)/)?' => 'HarbormasterStepDeleteController', ), diff --git a/src/applications/harbormaster/controller/HarbormasterStepAddController.php b/src/applications/harbormaster/controller/HarbormasterStepAddController.php index 0e0eb0538d..f9ca0a1649 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepAddController.php +++ b/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); } diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index 973e590e54..9d97fe364b 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/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(); - - $xaction_view = id(new PhabricatorApplicationTransactionView()) - ->setUser($viewer) - ->setObjectPHID($step->getPHID()) - ->setTransactions($xactions) - ->setShouldTerminate(true); + 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); + } return $this->buildApplicationPage( array( diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index f76dc67327..e54f51ea92 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/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, -- 2.51.2