Make "Land Revision" button state consistent, prevent non-accepted lands

Summary:
Ref T182. Make the disabled state of the button more accurately reflect whether clicking it will work.

Don't allow "land" to proceed unless the revision is accepted.

Test Plan: Saw button in disabled state, clicked it, got "only accepted revisions" message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182

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

src/applications/differential/controller/DifferentialRevisionOperationController.php

@@ -18,86 +18,13 @@ final class DifferentialRevisionOperationController
 
     $detail_uri = "/D{$id}";
 
-    $repository = $revision->getRepository();
-    if (!$repository) {
-      return $this->rejectOperation(
-        $revision,
-        pht('No Repository'),
-        pht(
-          'This revision is not associated with a known repository. Only '.
-          'revisions associated with a tracked repository can be landed '.
-          'automatically.'));
-    }
-
-    if (!$repository->canPerformAutomation()) {
-      return $this->rejectOperation(
-        $revision,
-        pht('No Repository Automation'),
-        pht(
-          'The repository this revision is associated with ("%s") is not '.
-          'configured to support automation. Configure automation for the '.
-          'repository to enable revisions to be landed automatically.',
-          $repository->getMonogram()));
-    }
-
-    // TODO: At some point we should allow installs to give "land reviewed
-    // code" permission to more users than "push any commit", because it is
-    // a much less powerful operation. For now, just require push so this
-    // doesn't do anything users can't do on their own.
-    $can_push = PhabricatorPolicyFilter::hasCapability(
-      $viewer,
-      $repository,
-      DiffusionPushCapability::CAPABILITY);
-    if (!$can_push) {
-      return $this->rejectOperation(
-        $revision,
-        pht('Unable to Push'),
-        pht(
-          'You do not have permission to push to the repository this '.
-          'revision is associated with ("%s"), so you can not land it.',
-          $repository->getMonogram()));
-    }
-
     $op = new DrydockLandRepositoryOperation();
-
+    $barrier = $op->getBarrierToLanding($viewer, $revision);
-    // Check for other operations. Eventually this should probably be more
+    if ($barrier) {
-    // general (e.g., it's OK to land to multiple different branches
+      return $this->newDialog()
-    // simultaneously) but just put this in as a sanity check for now.
+        ->setTitle($barrier['title'])
-    $other_operations = id(new DrydockRepositoryOperationQuery())
+        ->appendParagraph($barrier['body'])
-      ->setViewer($viewer)
+        ->addCancelButton($detail_uri);
-      ->withObjectPHIDs(array($revision->getPHID()))
-      ->withOperationTypes(
-        array(
-          $op->getOperationConstant(),
-        ))
-      ->withOperationStates(
-        array(
-          DrydockRepositoryOperation::STATE_WAIT,
-          DrydockRepositoryOperation::STATE_WORK,
-          DrydockRepositoryOperation::STATE_DONE,
-        ))
-      ->execute();
-
-    if ($other_operations) {
-      $any_done = false;
-      foreach ($other_operations as $operation) {
-        if ($operation->isDone()) {
-          $any_done = true;
-          break;
-        }
-      }
-
-      if ($any_done) {
-        return $this->rejectOperation(
-          $revision,
-          pht('Already Complete'),
-          pht('This revision has already landed.'));
-      } else {
-        return $this->rejectOperation(
-          $revision,
-          pht('Already In Flight'),
-          pht('This revision is already landing.'));
-      }
     }
 
     if ($request->isFormPost()) {
@@ -106,6 +33,7 @@ final class DifferentialRevisionOperationController
       // occurs.
 
       $diff = $revision->getActiveDiff();
+      $repository = $revision->getRepository();
 
       $operation = DrydockRepositoryOperation::initializeNewOperation($op)
         ->setAuthorPHID($viewer->getPHID())
@@ -136,18 +64,4 @@ final class DifferentialRevisionOperationController
       ->addSubmitButton(pht('Mutate Repository Unpredictably'));
   }
 
-  private function rejectOperation(
-    DifferentialRevision $revision,
-    $title,
-    $body) {
-
-    $id = $revision->getID();
-    $detail_uri = "/D{$id}";
-
-    return $this->newDialog()
-      ->setTitle($title)
-      ->appendParagraph($body)
-      ->addCancelButton($detail_uri);
-  }
-
 }

src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php

@@ -26,7 +26,9 @@ final class DifferentialLandingActionMenuEventListener
   }
 
   private function renderRevisionAction(PhutilEvent $event) {
-    if (!$this->canUseApplication($event->getUser())) {
+    $viewer = $event->getUser();
+
+    if (!$this->canUseApplication($viewer)) {
       return null;
     }
 
@@ -40,11 +42,22 @@ final class DifferentialLandingActionMenuEventListener
     if ($repository->canPerformAutomation()) {
       $revision_id = $revision->getID();
 
+      $op = new DrydockLandRepositoryOperation();
+      $barrier = $op->getBarrierToLanding($viewer, $revision);
+
+      if ($barrier) {
+        $can_land = false;
+      } else {
+        $can_land = true;
+      }
+
       $action = id(new PhabricatorActionView())
-        ->setWorkflow(true)
         ->setName(pht('Land Revision'))
         ->setIcon('fa-fighter-jet')
-        ->setHref("/differential/revision/operation/{$revision_id}/");
+        ->setHref("/differential/revision/operation/{$revision_id}/")
+        ->setWorkflow(true)
+        ->setDisabled(!$can_land);
+
 
       $this->addActionMenuItems($event, $action);
     }
@@ -54,7 +67,6 @@ final class DifferentialLandingActionMenuEventListener
       ->execute();
 
     foreach ($strategies as $strategy) {
-      $viewer = $event->getUser();
       $action = $strategy->createMenuItem($viewer, $revision, $repository);
       if ($action == null) {
         continue;

src/applications/drydock/operation/DrydockLandRepositoryOperation.php

@@ -189,4 +189,101 @@ final class DrydockLandRepositoryOperation
     return $diff;
   }
 
+  public function getBarrierToLanding(
+    PhabricatorUser $viewer,
+    DifferentialRevision $revision) {
+
+    $repository = $revision->getRepository();
+    if (!$repository) {
+      return array(
+        'title' => pht('No Repository'),
+        'body' => pht(
+          'This revision is not associated with a known repository. Only '.
+          'revisions associated with a tracked repository can be landed '.
+          'automatically.'),
+      );
+    }
+
+    if (!$repository->canPerformAutomation()) {
+      return array(
+        'title' => pht('No Repository Automation'),
+        'body' => pht(
+          'The repository this revision is associated with ("%s") is not '.
+          'configured to support automation. Configure automation for the '.
+          'repository to enable revisions to be landed automatically.',
+          $repository->getMonogram()),
+      );
+    }
+
+    // TODO: At some point we should allow installs to give "land reviewed
+    // code" permission to more users than "push any commit", because it is
+    // a much less powerful operation. For now, just require push so this
+    // doesn't do anything users can't do on their own.
+    $can_push = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $repository,
+      DiffusionPushCapability::CAPABILITY);
+    if (!$can_push) {
+      return array(
+        'title' => pht('Unable to Push'),
+        'body' => pht(
+          'You do not have permission to push to the repository this '.
+          'revision is associated with ("%s"), so you can not land it.',
+          $repository->getMonogram()),
+      );
+    }
+
+    $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+    if ($revision->getStatus() !== $status_accepted) {
+      return array(
+        'title' => pht('Revision Not Accepted'),
+        'body' => pht(
+          'This revision is still under review. Only revisions which have '.
+          'been accepted may land.'),
+      );
+    }
+
+    // Check for other operations. Eventually this should probably be more
+    // general (e.g., it's OK to land to multiple different branches
+    // simultaneously) but just put this in as a sanity check for now.
+    $other_operations = id(new DrydockRepositoryOperationQuery())
+      ->setViewer($viewer)
+      ->withObjectPHIDs(array($revision->getPHID()))
+      ->withOperationTypes(
+        array(
+          $this->getOperationConstant(),
+        ))
+      ->withOperationStates(
+        array(
+          DrydockRepositoryOperation::STATE_WAIT,
+          DrydockRepositoryOperation::STATE_WORK,
+          DrydockRepositoryOperation::STATE_DONE,
+        ))
+      ->execute();
+
+    if ($other_operations) {
+      $any_done = false;
+      foreach ($other_operations as $operation) {
+        if ($operation->isDone()) {
+          $any_done = true;
+          break;
+        }
+      }
+
+      if ($any_done) {
+        return array(
+          'title' => pht('Already Complete'),
+          'body' => pht('This revision has already landed.'),
+        );
+      } else {
+        return array(
+          'title' => pht('Already In Flight'),
+          'body' => pht('This revision is already landing.'),
+        );
+      }
+    }
+
+    return null;
+  }
+
 }