Improve UX and messaging for certain errors when landing revisions

Summary:
Ref T9994.

  - Allow errors to be dismissed.
  - Tailor messaging for closed/abandoned revisions.
  - Reduce scare messaging on land dialog, since it's not really that scary anymore.

Test Plan:
  - Dismissed errors.
  - Hit new warnings.
  - Wasn't as scared when landing.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9994

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

Author: epriestley

resources/sql/autopatches/20151226.reop.1.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_drydock.drydock_repositoryoperation
+  ADD isDismissed BOOL NOT NULL;

src/__phutil_library_map__.php

@@ -911,6 +911,7 @@
     'DrydockObjectAuthorizationView' => 'applications/drydock/view/DrydockObjectAuthorizationView.php',
     'DrydockQuery' => 'applications/drydock/query/DrydockQuery.php',
     'DrydockRepositoryOperation' => 'applications/drydock/storage/DrydockRepositoryOperation.php',
+    'DrydockRepositoryOperationDismissController' => 'applications/drydock/controller/DrydockRepositoryOperationDismissController.php',
     'DrydockRepositoryOperationListController' => 'applications/drydock/controller/DrydockRepositoryOperationListController.php',
     'DrydockRepositoryOperationPHIDType' => 'applications/drydock/phid/DrydockRepositoryOperationPHIDType.php',
     'DrydockRepositoryOperationQuery' => 'applications/drydock/query/DrydockRepositoryOperationQuery.php',
@@ -4900,6 +4901,7 @@
       'DrydockDAO',
       'PhabricatorPolicyInterface',
     ),
+    'DrydockRepositoryOperationDismissController' => 'DrydockController',
     'DrydockRepositoryOperationListController' => 'DrydockController',
     'DrydockRepositoryOperationPHIDType' => 'PhabricatorPHIDType',
     'DrydockRepositoryOperationQuery' => 'DrydockQuery',

src/applications/differential/controller/DifferentialRevisionOperationController.php

@@ -94,28 +94,23 @@ public function handleRequest(AphrontRequest $request) {
       ->setUser($viewer)
       ->appendRemarkupInstructions(
         pht(
-          'In theory, this will do approximately what `arc land` would do. '.
-          'In practice, you will have a riveting adventure instead.'))
+          '(NOTE) This feature is new and experimental.'))
       ->appendControl(
         id(new AphrontFormTokenizerControl())
           ->setLabel(pht('Onto Branch'))
           ->setName('refPHIDs')
           ->setLimit(1)
           ->setError($e_ref)
           ->setValue($v_ref)
-          ->setDatasource($ref_datasource))
-      ->appendRemarkupInstructions(
-        pht(
-          '(WARNING) THIS FEATURE IS EXPERIMENTAL AND DANGEROUS! USE IT AT '.
-          'YOUR OWN RISK!'));
+          ->setDatasource($ref_datasource));
 
     return $this->newDialog()
       ->setWidth(AphrontDialogView::WIDTH_FORM)
       ->setTitle(pht('Land Revision'))
       ->setErrors($errors)
       ->appendForm($form)
       ->addCancelButton($detail_uri)
-      ->addSubmitButton(pht('Mutate Repository Unpredictably'));
+      ->addSubmitButton(pht('Land Revision'));
   }
 
   private function newRefQuery(PhabricatorRepository $repository) {

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -1047,6 +1047,7 @@ private function buildOperationsBox(DifferentialRevision $revision) {
     $operations = id(new DrydockRepositoryOperationQuery())
       ->setViewer($viewer)
       ->withObjectPHIDs(array($revision->getPHID()))
+      ->withIsDismissed(false)
       ->withOperationTypes(
         array(
           DrydockLandRepositoryOperation::OPCONST,

src/applications/drydock/application/PhabricatorDrydockApplication.php

@@ -96,6 +96,7 @@ public function getRoutes() {
           '(?P<id>[1-9]\d*)/' => array(
             '' => 'DrydockRepositoryOperationViewController',
             'status/' => 'DrydockRepositoryOperationStatusController',
+            'dismiss/' => 'DrydockRepositoryOperationDismissController',
           ),
         ),
       ),

src/applications/drydock/controller/DrydockRepositoryOperationDismissController.php

@@ -0,0 +1,56 @@
+<?php
+
+final class DrydockRepositoryOperationDismissController
+  extends DrydockController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $request->getViewer();
+    $id = $request->getURIData('id');
+
+    $operation = id(new DrydockRepositoryOperationQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+    if (!$operation) {
+      return new Aphront404Response();
+    }
+
+    $object_phid = $operation->getObjectPHID();
+    $handles = $viewer->loadHandles(array($object_phid));
+    $done_uri = $handles[$object_phid]->getURI();
+
+    if ($operation->getIsDismissed()) {
+      return $this->newDialog()
+        ->setTitle(pht('Already Dismissed'))
+        ->appendParagraph(
+          pht(
+            'This operation has already been dismissed, and can not be '.
+            'dismissed any further.'))
+        ->addCancelButton($done_uri);
+    }
+
+
+    if ($request->isFormPost()) {
+      $operation
+        ->setIsDismissed(1)
+        ->save();
+
+      return id(new AphrontRedirectResponse())->setURI($done_uri);
+    }
+
+    return $this->newDialog()
+      ->setTitle(pht('Dismiss Operation'))
+      ->appendParagraph(
+        pht(
+          'Dismiss this operation? It will no longer be shown, but logs '.
+          'can be found in Drydock.'))
+      ->addSubmitButton(pht('Dismiss'))
+      ->addCancelButton($done_uri);
+  }
+
+}

src/applications/drydock/operation/DrydockLandRepositoryOperation.php

@@ -235,12 +235,29 @@ public function getBarrierToLanding(
 
     $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.'),
-      );
+      switch ($revision->getStatus()) {
+        case ArcanistDifferentialRevisionStatus::CLOSED:
+          return array(
+            'title' => pht('Revision Closed'),
+            'body' => pht(
+              'This revision has already been closed. Only open, accepted '.
+              'revisions may land.'),
+          );
+        case ArcanistDifferentialRevisionStatus::ABANDONED:
+          return array(
+            'title' => pht('Revision Abandoned'),
+            'body' => pht(
+              'This revision has been abandoned. Only accepted revisions '.
+              'may land.'),
+          );
+        default:
+          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

src/applications/drydock/query/DrydockRepositoryOperationQuery.php

@@ -8,6 +8,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery {
   private $repositoryPHIDs;
   private $operationStates;
   private $operationTypes;
+  private $isDismissed;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -39,6 +40,11 @@ public function withOperationTypes(array $types) {
     return $this;
   }
 
+  public function withIsDismissed($dismissed) {
+    $this->isDismissed = $dismissed;
+    return $this;
+  }
+
   public function newResultObject() {
     return new DrydockRepositoryOperation();
   }
@@ -152,6 +158,13 @@ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
         $this->operationTypes);
     }
 
+    if ($this->isDismissed !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'isDismissed = %d',
+        (int)$this->isDismissed);
+    }
+
     return $where;
   }
 

src/applications/drydock/storage/DrydockRepositoryOperation.php

@@ -20,6 +20,7 @@ final class DrydockRepositoryOperation extends DrydockDAO
   protected $operationType;
   protected $operationState;
   protected $properties = array();
+  protected $isDismissed;
 
   private $repository = self::ATTACHABLE;
   private $object = self::ATTACHABLE;
@@ -30,7 +31,8 @@ public static function initializeNewOperation(
 
     return id(new DrydockRepositoryOperation())
       ->setOperationState(self::STATE_WAIT)
-      ->setOperationType($op->getOperationConstant());
+      ->setOperationType($op->getOperationConstant())
+      ->setIsDismissed(0);
   }
 
   protected function getConfiguration() {
@@ -43,6 +45,7 @@ protected function getConfiguration() {
         'repositoryTarget' => 'bytes',
         'operationType' => 'text32',
         'operationState' => 'text32',
+        'isDismissed' => 'bool',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'key_object' => array(
@@ -196,11 +199,17 @@ public function getCapabilities() {
   }
 
   public function getPolicy($capability) {
-    return $this->getRepository()->getPolicy($capability);
+    $need_capability = $this->getRequiredRepositoryCapability($capability);
+
+    return $this->getRepository()
+      ->getPolicy($need_capability);
   }
 
   public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return $this->getRepository()->hasAutomaticCapability($capability, $viewer);
+    $need_capability = $this->getRequiredRepositoryCapability($capability);
+
+    return $this->getRepository()
+      ->hasAutomaticCapability($need_capability, $viewer);
   }
 
   public function describeAutomaticCapability($capability) {
@@ -209,4 +218,17 @@ public function describeAutomaticCapability($capability) {
       'affects.');
   }
 
+  private function getRequiredRepositoryCapability($capability) {
+    // To edit a RepositoryOperation, require that the user be able to push
+    // to the repository.
+
+    $map = array(
+      PhabricatorPolicyCapability::CAN_EDIT =>
+        DiffusionPushCapability::CAPABILITY,
+    );
+
+    return idx($map, $capability, $capability);
+  }
+
+
 }

src/applications/drydock/view/DrydockRepositoryOperationStatusView.php

@@ -67,6 +67,7 @@ public function renderUnderwayState() {
 
     $item = id(new PHUIObjectItemView())
       ->setHref("/drydock/operation/{$id}/")
+      ->setObjectName(pht('Operation %d', $id))
       ->setHeader($operation->getOperationDescription($viewer))
       ->setStatusIcon($icon, $name);
 
@@ -98,6 +99,16 @@ public function renderUnderwayState() {
       } else {
         $item->addAttribute(pht('Operation encountered an error.'));
       }
+
+      $is_dismissed = $operation->getIsDismissed();
+
+      $item->addAction(
+        id(new PHUIListItemView())
+          ->setName('Dismiss')
+          ->setIcon('fa-times')
+          ->setDisabled($is_dismissed)
+          ->setWorkflow(true)
+          ->setHref("/drydock/operation/{$id}/dismiss/"));
     }
 
     return id(new PHUIObjectItemListView())