Allow comment actions to be grouped; group Differential "Review" and "Revision" actions

Summary:
Ref T11114. Differential has more actions than it once did, and may have further actions in the future.

Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject".

(The action order still needs to be tweaked a bit.)

Test Plan: {F2274526}

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T11114

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

src/__phutil_library_map__.php

@@ -557,6 +557,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionRequiredActionResultBucket' => 'applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php',
     'DifferentialRevisionResignTransaction' => 'applications/differential/xaction/DifferentialRevisionResignTransaction.php',
     'DifferentialRevisionResultBucket' => 'applications/differential/query/DifferentialRevisionResultBucket.php',
+    'DifferentialRevisionReviewTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewTransaction.php',
     'DifferentialRevisionReviewersHeraldField' => 'applications/differential/herald/DifferentialRevisionReviewersHeraldField.php',
     'DifferentialRevisionReviewersTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewersTransaction.php',
     'DifferentialRevisionSearchConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionSearchConduitAPIMethod.php',
@@ -2554,6 +2555,7 @@ phutil_register_library_map(array(
     'PhabricatorEditEngineAPIMethod' => 'applications/transactions/editengine/PhabricatorEditEngineAPIMethod.php',
     'PhabricatorEditEngineColumnsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineColumnsCommentAction.php',
     'PhabricatorEditEngineCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php',
+    'PhabricatorEditEngineCommentActionGroup' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php',
     'PhabricatorEditEngineConfiguration' => 'applications/transactions/storage/PhabricatorEditEngineConfiguration.php',
     'PhabricatorEditEngineConfigurationDefaultCreateController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationDefaultCreateController.php',
     'PhabricatorEditEngineConfigurationDefaultsController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationDefaultsController.php',
@@ -5183,7 +5185,7 @@ phutil_register_library_map(array(
       'PhabricatorConduitResultInterface',
     ),
     'DifferentialRevisionAbandonTransaction' => 'DifferentialRevisionActionTransaction',
-    'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionActionTransaction',
+    'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionReviewTransaction',
     'DifferentialRevisionActionTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionAffectedFilesHeraldField' => 'DifferentialRevisionHeraldField',
     'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField',
@@ -5223,7 +5225,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionPlanChangesTransaction' => 'DifferentialRevisionActionTransaction',
     'DifferentialRevisionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DifferentialRevisionReclaimTransaction' => 'DifferentialRevisionActionTransaction',
-    'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionActionTransaction',
+    'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionReviewTransaction',
     'DifferentialRevisionRelationship' => 'PhabricatorObjectRelationship',
     'DifferentialRevisionRelationshipSource' => 'PhabricatorObjectRelationshipSource',
     'DifferentialRevisionReopenTransaction' => 'DifferentialRevisionActionTransaction',
@@ -5232,8 +5234,9 @@ phutil_register_library_map(array(
     'DifferentialRevisionRepositoryTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionRequestReviewTransaction' => 'DifferentialRevisionActionTransaction',
     'DifferentialRevisionRequiredActionResultBucket' => 'DifferentialRevisionResultBucket',
-    'DifferentialRevisionResignTransaction' => 'DifferentialRevisionActionTransaction',
+    'DifferentialRevisionResignTransaction' => 'DifferentialRevisionReviewTransaction',
     'DifferentialRevisionResultBucket' => 'PhabricatorSearchResultBucket',
+    'DifferentialRevisionReviewTransaction' => 'DifferentialRevisionActionTransaction',
     'DifferentialRevisionReviewersHeraldField' => 'DifferentialRevisionHeraldField',
     'DifferentialRevisionReviewersTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
@@ -7536,6 +7539,7 @@ phutil_register_library_map(array(
     'PhabricatorEditEngineAPIMethod' => 'ConduitAPIMethod',
     'PhabricatorEditEngineColumnsCommentAction' => 'PhabricatorEditEngineCommentAction',
     'PhabricatorEditEngineCommentAction' => 'Phobject',
+    'PhabricatorEditEngineCommentActionGroup' => 'Phobject',
     'PhabricatorEditEngineConfiguration' => array(
       'PhabricatorSearchDAO',
       'PhabricatorApplicationTransactionInterface',

src/applications/differential/editor/DifferentialRevisionEditEngine.php

@@ -9,6 +9,9 @@ final class DifferentialRevisionEditEngine
 
   const KEY_UPDATE = 'update';
 
+  const ACTIONGROUP_REVIEW = 'review';
+  const ACTIONGROUP_REVISION = 'revision';
+
   public function getEngineName() {
     return pht('Revisions');
   }
@@ -87,6 +90,17 @@ final class DifferentialRevisionEditEngine
     return $this->diff;
   }
 
+  protected function newCommentActionGroups() {
+    return array(
+      id(new PhabricatorEditEngineCommentActionGroup())
+        ->setKey(self::ACTIONGROUP_REVIEW)
+        ->setLabel(pht('Review Actions')),
+      id(new PhabricatorEditEngineCommentActionGroup())
+        ->setKey(self::ACTIONGROUP_REVISION)
+        ->setLabel(pht('Revision Actions')),
+    );
+  }
+
   protected function buildCustomEditFields($object) {
     $viewer = $this->getViewer();
 

src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php

@@ -1,7 +1,7 @@
 <?php
 
 final class DifferentialRevisionAcceptTransaction
-  extends DifferentialRevisionActionTransaction {
+  extends DifferentialRevisionReviewTransaction {
 
   const TRANSACTIONTYPE = 'differential.revision.accept';
   const ACTIONKEY = 'accept';

src/applications/differential/xaction/DifferentialRevisionActionTransaction.php

@@ -19,6 +19,10 @@ abstract class DifferentialRevisionActionTransaction
   abstract protected function validateAction($object, PhabricatorUser $viewer);
   abstract protected function getRevisionActionLabel();
 
+  protected function getRevisionActionGroupKey() {
+    return DifferentialRevisionEditEngine::ACTIONGROUP_REVISION;
+  }
+
   protected function getRevisionActionDescription() {
     return null;
   }
@@ -41,116 +45,6 @@ abstract class DifferentialRevisionActionTransaction
     return ($viewer->getPHID() === $revision->getAuthorPHID());
   }
 
-  protected function isViewerAnyReviewer(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer) {
-    return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
-  }
-
-  protected function isViewerAcceptingReviewer(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusAmong(
-      $revision,
-      $viewer,
-      array(
-        DifferentialReviewerStatus::STATUS_ACCEPTED,
-      ));
-  }
-
-  protected function isViewerRejectingReviewer(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusAmong(
-      $revision,
-      $viewer,
-      array(
-        DifferentialReviewerStatus::STATUS_REJECTED,
-      ));
-  }
-
-  protected function getViewerReviewerStatus(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer) {
-
-    if (!$viewer->getPHID()) {
-      return null;
-    }
-
-    foreach ($revision->getReviewerStatus() as $reviewer) {
-      if ($reviewer->getReviewerPHID() != $viewer->getPHID()) {
-        continue;
-      }
-
-      return $reviewer->getStatus();
-    }
-
-    return null;
-  }
-
-  protected function isViewerReviewerStatusAmong(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer,
-    array $status_list) {
-
-    $status = $this->getViewerReviewerStatus($revision, $viewer);
-    if ($status === null) {
-      return false;
-    }
-
-    $status_map = array_fuse($status_list);
-    return isset($status_map[$status]);
-  }
-
-  protected function applyReviewerEffect(
-    DifferentialRevision $revision,
-    PhabricatorUser $viewer,
-    $value,
-    $status) {
-
-    $map = array();
-
-    // When you accept or reject, you may accept or reject on behalf of all
-    // reviewers you have authority for. When you resign, you only affect
-    // yourself.
-    $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED);
-    if ($with_authority) {
-      foreach ($revision->getReviewerStatus() as $reviewer) {
-        if ($reviewer->hasAuthority($viewer)) {
-          $map[$reviewer->getReviewerPHID()] = $status;
-        }
-      }
-    }
-
-    // In all cases, you affect yourself.
-    $map[$viewer->getPHID()] = $status;
-
-    // Convert reviewer statuses into edge data.
-    foreach ($map as $reviewer_phid => $reviewer_status) {
-      $map[$reviewer_phid] = array(
-        'data' => array(
-          'status' => $reviewer_status,
-        ),
-      );
-    }
-
-    $src_phid = $revision->getPHID();
-    $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
-
-    $editor = new PhabricatorEdgeEditor();
-    foreach ($map as $dst_phid => $edge_data) {
-      if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
-        // TODO: For now, we just remove these reviewers. In the future, we will
-        // store resignations explicitly.
-        $editor->removeEdge($src_phid, $edge_type, $dst_phid);
-      } else {
-        $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data);
-      }
-    }
-
-    $editor->save();
-  }
-
   public function newEditField(
     DifferentialRevision $revision,
     PhabricatorUser $viewer) {
@@ -167,6 +61,9 @@ abstract class DifferentialRevisionActionTransaction
 
         $description = $this->getRevisionActionDescription();
         $field->setActionDescription($description);
+
+        $group_key = $this->getRevisionActionGroupKey();
+        $field->setCommentActionGroupKey($group_key);
       }
     }
 

src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php

@@ -1,7 +1,7 @@
 <?php
 
 final class DifferentialRevisionRejectTransaction
-  extends DifferentialRevisionActionTransaction {
+  extends DifferentialRevisionReviewTransaction {
 
   const TRANSACTIONTYPE = 'differential.revision.reject';
   const ACTIONKEY = 'reject';

src/applications/differential/xaction/DifferentialRevisionResignTransaction.php

@@ -1,7 +1,7 @@
 <?php
 
 final class DifferentialRevisionResignTransaction
-  extends DifferentialRevisionActionTransaction {
+  extends DifferentialRevisionReviewTransaction {
 
   const TRANSACTIONTYPE = 'differential.revision.resign';
   const ACTIONKEY = 'resign';

src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php

@@ -0,0 +1,120 @@
+<?php
+
+abstract class DifferentialRevisionReviewTransaction
+  extends DifferentialRevisionActionTransaction {
+
+  protected function getRevisionActionGroupKey() {
+    return DifferentialRevisionEditEngine::ACTIONGROUP_REVIEW;
+  }
+
+  protected function isViewerAnyReviewer(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer) {
+    return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
+  }
+
+  protected function isViewerAcceptingReviewer(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer) {
+    return $this->isViewerReviewerStatusAmong(
+      $revision,
+      $viewer,
+      array(
+        DifferentialReviewerStatus::STATUS_ACCEPTED,
+      ));
+  }
+
+  protected function isViewerRejectingReviewer(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer) {
+    return $this->isViewerReviewerStatusAmong(
+      $revision,
+      $viewer,
+      array(
+        DifferentialReviewerStatus::STATUS_REJECTED,
+      ));
+  }
+
+  protected function getViewerReviewerStatus(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer) {
+
+    if (!$viewer->getPHID()) {
+      return null;
+    }
+
+    foreach ($revision->getReviewerStatus() as $reviewer) {
+      if ($reviewer->getReviewerPHID() != $viewer->getPHID()) {
+        continue;
+      }
+
+      return $reviewer->getStatus();
+    }
+
+    return null;
+  }
+
+  protected function isViewerReviewerStatusAmong(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer,
+    array $status_list) {
+
+    $status = $this->getViewerReviewerStatus($revision, $viewer);
+    if ($status === null) {
+      return false;
+    }
+
+    $status_map = array_fuse($status_list);
+    return isset($status_map[$status]);
+  }
+
+  protected function applyReviewerEffect(
+    DifferentialRevision $revision,
+    PhabricatorUser $viewer,
+    $value,
+    $status) {
+
+    $map = array();
+
+    // When you accept or reject, you may accept or reject on behalf of all
+    // reviewers you have authority for. When you resign, you only affect
+    // yourself.
+    $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED);
+    if ($with_authority) {
+      foreach ($revision->getReviewerStatus() as $reviewer) {
+        if ($reviewer->hasAuthority($viewer)) {
+          $map[$reviewer->getReviewerPHID()] = $status;
+        }
+      }
+    }
+
+    // In all cases, you affect yourself.
+    $map[$viewer->getPHID()] = $status;
+
+    // Convert reviewer statuses into edge data.
+    foreach ($map as $reviewer_phid => $reviewer_status) {
+      $map[$reviewer_phid] = array(
+        'data' => array(
+          'status' => $reviewer_status,
+        ),
+      );
+    }
+
+    $src_phid = $revision->getPHID();
+    $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
+
+    $editor = new PhabricatorEdgeEditor();
+    foreach ($map as $dst_phid => $edge_data) {
+      if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
+        // TODO: For now, we just remove these reviewers. In the future, we will
+        // store resignations explicitly.
+        $editor->removeEdge($src_phid, $edge_type, $dst_phid);
+      } else {
+        $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data);
+      }
+    }
+
+    $editor->save();
+  }
+
+}

src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php

@@ -7,6 +7,7 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject {
   private $value;
   private $initialValue;
   private $order;
+  private $groupKey;
 
   abstract public function getPHUIXControlType();
   abstract public function getPHUIXControlSpecification();
@@ -20,6 +21,15 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject {
     return $this->key;
   }
 
+  public function setGroupKey($group_key) {
+    $this->groupKey = $group_key;
+    return $this;
+  }
+
+  public function getGroupKey() {
+    return $this->groupKey;
+  }
+
   public function setLabel($label) {
     $this->label = $label;
     return $this;

src/applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php

@@ -0,0 +1,27 @@
+<?php
+
+final class PhabricatorEditEngineCommentActionGroup
+  extends Phobject {
+
+  private $key;
+  private $label;
+
+  public function setKey($key) {
+    $this->key = $key;
+    return $this;
+  }
+
+  public function getKey() {
+    return $this->key;
+  }
+
+  public function setLabel($label) {
+    $this->label = $label;
+    return $this;
+  }
+
+  public function getLabel() {
+    return $this->label;
+  }
+
+}

src/applications/transactions/editengine/PhabricatorEditEngine.php

@@ -1549,6 +1549,9 @@ abstract class PhabricatorEditEngine
 
     $view->setCommentActions($comment_actions);
 
+    $comment_groups = $this->newCommentActionGroups();
+    $view->setCommentActionGroups($comment_groups);
+
     return $view;
   }
 
@@ -2157,6 +2160,10 @@ abstract class PhabricatorEditEngine
     return $request->getURIData('editAction');
   }
 
+  protected function newCommentActionGroups() {
+    return array();
+  }
+
 
 /* -(  Form Pages  )--------------------------------------------------------- */
 

src/applications/transactions/editfield/PhabricatorEditField.php

@@ -25,6 +25,7 @@ abstract class PhabricatorEditField extends Phobject {
 
   private $commentActionLabel;
   private $commentActionValue;
+  private $commentActionGroupKey;
   private $commentActionOrder = 1000;
   private $hasCommentActionValue;
 
@@ -245,6 +246,15 @@ abstract class PhabricatorEditField extends Phobject {
     return $this->commentActionLabel;
   }
 
+  public function setCommentActionGroupKey($key) {
+    $this->commentActionGroupKey = $key;
+    return $this;
+  }
+
+  public function getCommentActionGroupKey() {
+    return $this->commentActionGroupKey;
+  }
+
   public function setCommentActionOrder($order) {
     $this->commentActionOrder = $order;
     return $this;
@@ -719,7 +729,8 @@ abstract class PhabricatorEditField extends Phobject {
       ->setKey($this->getKey())
       ->setLabel($label)
       ->setValue($this->getValueForCommentAction($value))
-      ->setOrder($this->getCommentActionOrder());
+      ->setOrder($this->getCommentActionOrder())
+      ->setGroupKey($this->getCommentActionGroupKey());
 
     return $action;
   }

src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php

@@ -24,6 +24,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
   private $currentVersion;
   private $versionedDraft;
   private $commentActions;
+  private $commentActionGroups = array();
   private $transactionTimeline;
 
   public function setObjectPHID($object_phid) {
@@ -118,6 +119,16 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
     return $this->commentActions;
   }
 
+  public function setCommentActionGroups(array $groups) {
+    assert_instances_of($groups, 'PhabricatorEditEngineCommentActionGroup');
+    $this->commentActionGroups = $groups;
+    return $this;
+  }
+
+  public function getCommentActionGroups() {
+    return $this->commentActionGroups;
+  }
+
   public function setNoPermission($no_permission) {
     $this->noPermission = $no_permission;
     return $this;
@@ -279,16 +290,13 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
           'type' => $comment_action->getPHUIXControlType(),
           'spec' => $comment_action->getPHUIXControlSpecification(),
           'initialValue' => $comment_action->getInitialValue(),
+          'groupKey' => $comment_action->getGroupKey(),
         );
 
         $type_map[$key] = $comment_action;
       }
 
-      $options = array();
+      $options = $this->newCommentActionOptions($action_map);
-      $options['+'] = pht('Add Action...');
-      foreach ($action_map as $key => $item) {
-        $options[$key] = $item['label'];
-      }
 
       $action_id = celerity_generate_unique_node_id();
       $input_id = celerity_generate_unique_node_id();
@@ -424,4 +432,46 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView {
     return $this->commentID;
   }
 
+  private function newCommentActionOptions(array $action_map) {
+    $options = array();
+    $options['+'] = pht('Add Action...');
+
+    // Merge options into groups.
+    $groups = array();
+    foreach ($action_map as $key => $item) {
+      $group_key = $item['groupKey'];
+      if (!isset($groups[$group_key])) {
+        $groups[$group_key] = array();
+      }
+      $groups[$group_key][$key] = $item;
+    }
+
+    $group_specs = $this->getCommentActionGroups();
+    $group_labels = mpull($group_specs, 'getLabel', 'getKey');
+
+    // Reorder groups to put them in the same order as the recognized
+    // group definitions.
+    $groups = array_select_keys($groups, array_keys($group_labels)) + $groups;
+
+    // Move options with no group to the end.
+    $default_group = idx($groups, '');
+    if ($default_group) {
+      unset($groups['']);
+      $groups[''] = $default_group;
+    }
+
+    foreach ($groups as $group_key => $group_items) {
+      if (strlen($group_key)) {
+        $group_label = idx($group_labels, $group_key, $group_key);
+        $options[$group_label] = ipull($group_items, 'label');
+      } else {
+        foreach ($group_items as $key => $item) {
+          $options[$key] = $item['label'];
+        }
+      }
+    }
+
+    return $options;
+  }
+
 }