Add Disabled mode to landing revision ui

Summary:
Fixes T4066.

add `isActionDisabled()` to DifferentialLandingStrategy, which also explains why it is so.
Make an appropriate pop-up in the controller.

Also make the whole UI "workflow", and convert `createMenuItems()` to `createMenuItem()` (Singular).

Test Plan: Click "Land to..." button in all kinds of revisions.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4066

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

src/applications/differential/controller/DifferentialRevisionLandController.php

@@ -68,6 +68,26 @@ final class DifferentialRevisionLandController extends DifferentialController {
       return id(new AphrontDialogResponse())->setDialog($dialog);
     }
 
+    $is_disabled = $this->pushStrategy->isActionDisabled(
+      $viewer,
+      $revision,
+      $revision->getRepository());
+    if ($is_disabled) {
+      if (is_string($is_disabled)) {
+        $explain = $is_disabled;
+      } else {
+        $explain = pht("This action is not currently enabled.");
+      }
+      $dialog = id(new AphrontDialogView())
+        ->setUser($viewer)
+        ->setTitle(pht("Can't land revision"))
+        ->appendChild($explain)
+        ->addCancelButton('/D'.$revision_id);
+
+      return id(new AphrontDialogResponse())->setDialog($dialog);
+    }
+
+
     $prompt = hsprintf('%s<br><br>%s',
       pht(
         'This will squash and rebase revision %s, and push it to '.

src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * This class adds a "Land this" button to revision view.
+ */
 final class DifferentialLandingActionMenuEventListener
   extends PhabricatorEventListener {
 
@@ -17,13 +20,9 @@ final class DifferentialLandingActionMenuEventListener
 
   private function handleActionsEvent(PhutilEvent $event) {
     $object = $event->getValue('object');
-
-    $actions = null;
     if ($object instanceof DifferentialRevision) {
-      $actions = $this->renderRevisionAction($event);
+      $this->renderRevisionAction($event);
     }
-
-    $this->addActionMenuItems($event, $actions);
   }
 
   private function renderRevisionAction(PhutilEvent $event) {
@@ -42,13 +41,15 @@ final class DifferentialLandingActionMenuEventListener
       ->setAncestorClass('DifferentialLandingStrategy')
       ->loadObjects();
     foreach ($strategies as $strategy) {
-      $actions = $strategy->createMenuItems(
+      $viewer = $event->getUser();
-          $event->getUser(),
+      $action = $strategy->createMenuItem($viewer, $revision, $repository);
-          $revision,
+      if ($action == null)
-          $repository);
+        continue;
-      $this->addActionMenuItems($event, $actions);
+      if ($strategy->isActionDisabled($viewer, $revision, $repository)) {
+        $action->setDisabled(true);
+      }
+      $this->addActionMenuItems($event, $action);
     }
   }
 
 }
-

src/applications/differential/landing/DifferentialLandingStrategy.php

@@ -8,9 +8,9 @@ abstract class DifferentialLandingStrategy {
     PhabricatorRepository $repository);
 
   /**
-   * returns PhabricatorActionView or an array of PhabricatorActionView or null.
+   * returns PhabricatorActionView or null.
    */
-  abstract function createMenuItems(
+  abstract function createMenuItem(
     PhabricatorUser $viewer,
     DifferentialRevision $revision,
     PhabricatorRepository $repository);
@@ -18,14 +18,43 @@ abstract class DifferentialLandingStrategy {
   /**
    * returns PhabricatorActionView which can be attached to the revision view.
    */
-  protected function createActionView($revision, $name, $disabled = false) {
+  protected function createActionView($revision, $name) {
     $strategy = get_class($this);
     $revision_id = $revision->getId();
     return id(new PhabricatorActionView())
       ->setRenderAsForm(true)
+      ->setWorkflow(true)
       ->setName($name)
-      ->setHref("/differential/revision/land/{$revision_id}/{$strategy}/")
+      ->setHref("/differential/revision/land/{$revision_id}/{$strategy}/");
-      ->setDisabled($disabled);
+  }
+
+  /**
+   * Check if this action should be disabled, and explain why.
+   *
+   * By default, this method checks for push permissions, and for the
+   * revision being Accepted.
+   *
+   * @return FALSE for "not disabled";
+   *         Human-readable text explaining why, if it is disabled;
+   */
+  public function isActionDisabled(
+    PhabricatorUser $viewer,
+    DifferentialRevision $revision,
+    PhabricatorRepository $repository) {
+
+    $status = $revision->getStatus();
+    if ($status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
+      return pht("Only Accepted revisions can be landed.");
+    }
+
+    if (!PhabricatorPolicyFilter::hasCapability(
+        $viewer,
+        $repository,
+        DiffusionCapabilityPush::CAPABILITY)) {
+      return pht("You do not have permissions to push to this repository.");
+    }
+
+    return false;
   }
 
   /**

src/applications/differential/landing/DifferentialLandingToGitHub.php

@@ -47,7 +47,7 @@ final class DifferentialLandingToGitHub
   /**
    * returns PhabricatorActionView or an array of PhabricatorActionView or null.
    */
-  public function createMenuItems(
+  public function createMenuItem(
     PhabricatorUser $viewer,
     DifferentialRevision $revision,
     PhabricatorRepository $repository) {

src/applications/differential/landing/DifferentialLandingToHostedGit.php

@@ -105,7 +105,7 @@ final class DifferentialLandingToHostedGit
     $workspace->execxLocal("push origin HEAD:master");
   }
 
-  public function createMenuItems(
+  public function createMenuItem(
     PhabricatorUser $viewer,
     DifferentialRevision $revision,
     PhabricatorRepository $repository) {
@@ -123,14 +123,8 @@ final class DifferentialLandingToHostedGit
       return;
     }
 
-    $can_push = PhabricatorPolicyFilter::hasCapability(
-        $viewer,
-        $repository,
-        DiffusionCapabilityPush::CAPABILITY);
-
     return $this->createActionView(
       $revision,
-      pht('Land to Hosted Repository'),
+      pht('Land to Hosted Repository'));
-      !$can_push);
   }
 }

src/applications/differential/landing/DifferentialLandingToHostedMercurial.php

@@ -93,7 +93,7 @@ final class DifferentialLandingToHostedMercurial
     $workspace->execxLocal("push -b default");
   }
 
-  public function createMenuItems(
+  public function createMenuItem(
     PhabricatorUser $viewer,
     DifferentialRevision $revision,
     PhabricatorRepository $repository) {
@@ -107,14 +107,8 @@ final class DifferentialLandingToHostedMercurial
       return;
     }
 
-    $can_push = PhabricatorPolicyFilter::hasCapability(
-        $viewer,
-        $repository,
-        DiffusionCapabilityPush::CAPABILITY);
-
     return $this->createActionView(
       $revision,
-      pht('Land to Hosted Repository'),
+      pht('Land to Hosted Repository'));
-      !$can_push);
   }
 }