Modernize some Harbormaster Controller/Policy infrastructure

Summary:
Ref T8095. This is just general groundwork for more exciting changes:

  - Use more modern conventions around controllers, UI elements, and dialogs.
  - Provide real CAN_EDIT policies and policy checks (they just don't do anything yet).

Test Plan:
  - Used all affected controllers.
  - Faked CAN_EDIT to POLICY_NOONE and verified everything was greyed out and unselectable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8095

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

src/applications/harbormaster/controller/HarbormasterController.php

@@ -2,21 +2,4 @@
 
 abstract class HarbormasterController extends PhabricatorController {
 
-  protected function buildApplicationCrumbs() {
-    $crumbs = parent::buildApplicationCrumbs();
-
-    $can_create = $this->hasApplicationCapability(
-        HarbormasterManagePlansCapability::CAPABILITY);
-
-    $crumbs->addAction(
-      id(new PHUIListItemView())
-        ->setName(pht('New Build Plan'))
-        ->setHref($this->getApplicationURI('plan/edit/'))
-        ->setDisabled(!$can_create)
-        ->setWorkflow(!$can_create)
-        ->setIcon('fa-plus-square'));
-
-    return $crumbs;
-  }
-
 }

src/applications/harbormaster/controller/HarbormasterPlanDisableController.php

@@ -3,22 +3,20 @@
 final class HarbormasterPlanDisableController
   extends HarbormasterPlanController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
-      ->withIDs(array($this->id))
+      ->withIDs(array($request->getURIData('id')))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
       ->executeOne();
     if (!$plan) {
       return new Aphront404Response();
@@ -63,14 +61,11 @@ final class HarbormasterPlanDisableController
       $button = pht('Disable Plan');
     }
 
-    $dialog = id(new AphrontDialogView())
+    return $this->newDialog()
-      ->setUser($viewer)
       ->setTitle($title)
       ->appendChild($body)
       ->addSubmitButton($button)
       ->addCancelButton($plan_uri);
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/applications/harbormaster/controller/HarbormasterPlanEditController.php

@@ -2,23 +2,22 @@
 
 final class HarbormasterPlanEditController extends HarbormasterPlanController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function willProcessRequest(array $data) {
-    $this->id = idx($data, 'id');
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
-    if ($this->id) {
+    $id = $request->getURIData('id');
+    if ($id) {
       $plan = id(new HarbormasterBuildPlanQuery())
         ->setViewer($viewer)
-        ->withIDs(array($this->id))
+        ->withIDs(array($id))
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
         ->executeOne();
       if (!$plan) {
         return new Aphront404Response();
@@ -43,6 +42,7 @@ final class HarbormasterPlanEditController extends HarbormasterPlanController {
 
       $editor = id(new HarbormasterBuildPlanEditor())
         ->setActor($viewer)
+        ->setContinueOnNoEffect(true)
         ->setContentSourceFromRequest($request);
 
       try {

src/applications/harbormaster/controller/HarbormasterPlanListController.php

@@ -2,19 +2,13 @@
 
 final class HarbormasterPlanListController extends HarbormasterPlanController {
 
-  private $queryKey;
-
   public function shouldAllowPublic() {
     return true;
   }
 
-  public function willProcessRequest(array $data) {
+  public function handleRequest(AphrontRequest $request) {
-    $this->queryKey = idx($data, 'queryKey');
-  }
-
-  public function processRequest() {
     $controller = id(new PhabricatorApplicationSearchController())
-      ->setQueryKey($this->queryKey)
+      ->setQueryKey($request->getURIData('queryKey'))
       ->setSearchEngine(new HarbormasterBuildPlanSearchEngine())
       ->setNavigation($this->buildSideNavView());
 
@@ -44,4 +38,22 @@ final class HarbormasterPlanListController extends HarbormasterPlanController {
     return $this->buildSideNavView(true)->getMenu();
   }
 
+  protected function buildApplicationCrumbs() {
+    $crumbs = parent::buildApplicationCrumbs();
+
+    $can_create = $this->hasApplicationCapability(
+      HarbormasterManagePlansCapability::CAPABILITY);
+
+    $crumbs->addAction(
+      id(new PHUIListItemView())
+        ->setName(pht('New Build Plan'))
+        ->setHref($this->getApplicationURI('plan/edit/'))
+        ->setDisabled(!$can_create)
+        ->setWorkflow(!$can_create)
+        ->setIcon('fa-plus-square'));
+
+    return $crumbs;
+  }
+
+
 }

src/applications/harbormaster/controller/HarbormasterPlanRunController.php

@@ -2,20 +2,18 @@
 
 final class HarbormasterPlanRunController extends HarbormasterController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
-    $plan_id = $this->id;
+    $plan_id = $request->getURIData('id');
+
+    // NOTE: At least for now, this only requires the "Can Manage Plans"
+    // capability, not the "Can Edit" capability. Possibly it should have
+    // a more stringent requirement, though.
+
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
       ->withIDs(array($plan_id))
@@ -87,15 +85,12 @@ final class HarbormasterPlanRunController extends HarbormasterController {
           ->setError($e_name)
           ->setValue($v_name));
 
-    $dialog = id(new AphrontDialogView())
+    return $this->newDialog()
       ->setWidth(AphrontDialogView::WIDTH_FULL)
-      ->setUser($viewer)
       ->setTitle($title)
       ->appendChild($form)
       ->addCancelButton($cancel_uri)
       ->addSubmitButton($save_button);
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/applications/harbormaster/controller/HarbormasterPlanViewController.php

@@ -2,17 +2,10 @@
 
 final class HarbormasterPlanViewController extends HarbormasterPlanController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getviewer();
 
-  public function willProcessRequest(array $data) {
+    $id = $request->getURIData('id');
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-
-    $id = $this->id;
 
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
@@ -79,11 +72,9 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
   }
 
   private function buildStepList(HarbormasterBuildPlan $plan) {
-    $request = $this->getRequest();
+    $viewer = $this->getViewer();
-    $viewer = $request->getUser();
 
-    $run_order =
+    $run_order = HarbormasterBuildGraph::determineDependencyExecution($plan);
-      HarbormasterBuildGraph::determineDependencyExecution($plan);
 
     $steps = id(new HarbormasterBuildStepQuery())
       ->setViewer($viewer)
@@ -91,9 +82,16 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->execute();
     $steps = mpull($steps, null, 'getPHID');
 
-    $can_edit = $this->hasApplicationCapability(
+    $has_manage = $this->hasApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
+    $has_edit = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $plan,
+      PhabricatorPolicyCapability::CAN_EDIT);
+
+    $can_edit = ($has_manage && $has_edit);
+
     $step_list = id(new PHUIObjectItemListView())
       ->setUser($viewer)
       ->setNoDataString(
@@ -222,12 +220,32 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       $step_list->addItem($item);
     }
 
-    return array($step_list, $has_any_conflicts, $is_deadlocking);
+    $step_list->setFlush(true);
+
+    $plan_id = $plan->getID();
+
+    $header = id(new PHUIHeaderView())
+      ->setHeader(pht('Build Steps'))
+      ->addActionLink(
+        id(new PHUIButtonView())
+          ->setText(pht('Add Build Step'))
+          ->setHref($this->getApplicationURI("step/add/{$plan_id}/"))
+          ->setTag('a')
+          ->setIcon(
+            id(new PHUIIconView())
+              ->setIconFont('fa-plus'))
+          ->setDisabled(!$can_edit)
+          ->setWorkflow(true));
+
+    $step_box = id(new PHUIObjectBoxView())
+      ->setHeader($header)
+      ->appendChild($step_list);
+
+    return array($step_box, $has_any_conflicts, $is_deadlocking);
   }
 
   private function buildActionList(HarbormasterBuildPlan $plan) {
-    $request = $this->getRequest();
+    $viewer = $this->getViewer();
-    $viewer = $request->getUser();
     $id = $plan->getID();
 
     $list = id(new PhabricatorActionListView())
@@ -235,9 +253,16 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
       ->setObject($plan)
       ->setObjectURI($this->getApplicationURI("plan/{$id}/"));
 
-    $can_edit = $this->hasApplicationCapability(
+    $has_manage = $this->hasApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
+    $has_edit = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $plan,
+      PhabricatorPolicyCapability::CAN_EDIT);
+
+    $can_edit = ($has_manage && $has_edit);
+
     $list->addAction(
       id(new PhabricatorActionView())
         ->setName(pht('Edit Plan'))
@@ -264,20 +289,12 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController {
           ->setIcon('fa-ban'));
     }
 
-    $list->addAction(
-      id(new PhabricatorActionView())
-        ->setName(pht('Add Build Step'))
-        ->setHref($this->getApplicationURI("step/add/{$id}/"))
-        ->setWorkflow(true)
-        ->setDisabled(!$can_edit)
-        ->setIcon('fa-plus'));
-
     $list->addAction(
       id(new PhabricatorActionView())
         ->setName(pht('Run Plan Manually'))
         ->setHref($this->getApplicationURI("plan/run/{$id}/"))
         ->setWorkflow(true)
-        ->setDisabled(!$can_edit)
+        ->setDisabled(!$has_manage)
         ->setIcon('fa-play-circle'));
 
     return $list;

src/applications/harbormaster/controller/HarbormasterStepAddController.php

@@ -2,22 +2,20 @@
 
 final class HarbormasterStepAddController extends HarbormasterController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
     $plan = id(new HarbormasterBuildPlanQuery())
       ->setViewer($viewer)
-      ->withIDs(array($this->id))
+      ->withIDs(array($request->getURIData('id')))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
       ->executeOne();
     if (!$plan) {
       return new Aphront404Response();

src/applications/harbormaster/controller/HarbormasterStepDeleteController.php

@@ -2,27 +2,25 @@
 
 final class HarbormasterStepDeleteController extends HarbormasterController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-
+    $viewer = $this->getViewer();
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
-    $id = $this->id;
+    $id = $request->getURIData('id');
 
     $step = id(new HarbormasterBuildStepQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
       ->executeOne();
-    if ($step === null) {
+    if (!$step) {
-      throw new Exception(pht('Build step not found!'));
+      return new Aphront404Response();
     }
 
     $plan_id = $step->getBuildPlan()->getID();
@@ -33,19 +31,14 @@ final class HarbormasterStepDeleteController extends HarbormasterController {
       return id(new AphrontRedirectResponse())->setURI($done_uri);
     }
 
-    $dialog = new AphrontDialogView();
+    return $this->newDialog()
-    $dialog->setTitle(pht('Really Delete Step?'))
+      ->setTitle(pht('Really Delete Step?'))
-            ->setUser($viewer)
+      ->appendParagraph(
-            ->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!")));
+          "This can't be undone!"))
-    return id(new AphrontDialogResponse())->setDialog($dialog);
+      ->addCancelButton($done_uri)
+      ->addSubmitButton(pht('Delete Build Step'));
   }
 
 }

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -2,27 +2,22 @@
 
 final class HarbormasterStepEditController extends HarbormasterController {
 
-  private $id;
+  public function handleRequest(AphrontRequest $request) {
-  private $planID;
+    $viewer = $this->getViewer();
-  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() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
 
     $this->requireApplicationCapability(
       HarbormasterManagePlansCapability::CAPABILITY);
 
-    if ($this->id) {
+    $id = $request->getURIData('id');
+    if ($id) {
       $step = id(new HarbormasterBuildStepQuery())
         ->setViewer($viewer)
-        ->withIDs(array($this->id))
+        ->withIDs(array($id))
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
         ->executeOne();
       if (!$step) {
         return new Aphront404Response();
@@ -31,23 +26,30 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
       $is_new = false;
     } else {
+      $plan_id = $request->getURIData('plan');
+      $class = $request->getURIData('class');
+
       $plan = id(new HarbormasterBuildPlanQuery())
-          ->setViewer($viewer)
+        ->setViewer($viewer)
-          ->withIDs(array($this->planID))
+        ->withIDs(array($plan_id))
-          ->executeOne();
+        ->requireCapabilities(
+          array(
+            PhabricatorPolicyCapability::CAN_VIEW,
+            PhabricatorPolicyCapability::CAN_EDIT,
+          ))
+        ->executeOne();
       if (!$plan) {
         return new Aphront404Response();
       }
 
-      $impl = HarbormasterBuildStepImplementation::getImplementation(
+      $impl = HarbormasterBuildStepImplementation::getImplementation($class);
-        $this->className);
       if (!$impl) {
         return new Aphront404Response();
       }
 
       $step = HarbormasterBuildStep::initializeNewStep($viewer)
         ->setBuildPlanPHID($plan->getPHID())
-        ->setClassName($this->className);
+        ->setClassName($class);
 
       $is_new = true;
     }

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

@@ -102,6 +102,7 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   public function getCapabilities() {
     return array(
       PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
     );
   }
 
@@ -109,6 +110,10 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
     switch ($capability) {
       case PhabricatorPolicyCapability::CAN_VIEW:
         return PhabricatorPolicies::getMostOpenPolicy();
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        // NOTE: In practice, this policy is always limited by the "Mangage
+        // Build Plans" policy.
+        return PhabricatorPolicies::getMostOpenPolicy();
     }
   }
 
@@ -119,4 +124,5 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
   public function describeAutomaticCapability($capability) {
     return null;
   }
+
 }

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

@@ -118,6 +118,7 @@ final class HarbormasterBuildStep extends HarbormasterDAO
   public function getCapabilities() {
     return array(
       PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
     );
   }