Provide an "Add blocking reviewer..." Herald action

Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.

Test Plan: Added Herald rules and updated revisions; see screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

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

src/applications/differential/constants/DifferentialReviewerStatus.php

@@ -2,6 +2,7 @@
 
 final class DifferentialReviewerStatus {
 
+  const STATUS_BLOCKING = 'blocking';
   const STATUS_ADDED = 'added';
   const STATUS_ACCEPTED = 'accepted';
   const STATUS_REJECTED = 'rejected';

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -276,11 +276,14 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
         'ccs' => $adapter->getCCsAddedByHerald(),
       );
       $rem_ccs = $adapter->getCCsRemovedByHerald();
+      $blocking_reviewers = array_keys(
+        $adapter->getBlockingReviewersAddedByHerald());
     } else {
       $sub = array(
         'rev' => array(),
         'ccs' => array(),
       );
+      $blocking_reviewers = array();
     }
 
     // Remove any CCs which are prevented by Herald rules.
@@ -314,7 +317,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
       $this->getActor(),
       array_keys($add['rev']),
       array_keys($rem['rev']),
-      $this->getActorPHID());
+      $blocking_reviewers);
 
     // We want to attribute new CCs to a "reasonPHID", representing the reason
     // they were added. This is either a user (if some user explicitly CCs
@@ -602,7 +605,8 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     DifferentialRevision $revision,
     PhabricatorUser $actor,
     array $add_phids,
-    array $remove_phids) {
+    array $remove_phids,
+    array $blocking_phids = array()) {
 
     $reviewers = $revision->getReviewers();
 
@@ -618,22 +622,32 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     $editor = id(new PhabricatorEdgeEditor())
       ->setActor($actor);
 
-    $options = array(
-      'data' => array(
-        'status' => DifferentialReviewerStatus::STATUS_ADDED
-      )
-    );
-
     $reviewer_phids_map = array_fill_keys($reviewers, true);
 
+    $blocking_phids = array_fuse($blocking_phids);
     foreach ($add_phids as $phid) {
 
       // Adding an already existing edge again would have cause memory loss
       // That is, the previous state for that reviewer would be lost
       if (isset($reviewer_phids_map[$phid])) {
+        // TODO: If we're writing a blocking edge, we should overwrite an
+        // existing weaker edge (like "added" or "commented"), just not a
+        // stronger existing edge.
         continue;
       }
 
+      if (isset($blocking_phids[$phid])) {
+        $status = DifferentialReviewerStatus::STATUS_BLOCKING;
+      } else {
+        $status = DifferentialReviewerStatus::STATUS_ADDED;
+      }
+
+      $options = array(
+        'data' => array(
+          'status' => $status,
+        )
+      );
+
       $editor->addEdge(
         $revision->getPHID(),
         PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER,

src/applications/differential/view/DifferentialReviewersView.php

@@ -82,6 +82,10 @@ final class DifferentialReviewersView extends AphrontView {
           }
           break;
 
+        case DifferentialReviewerStatus::STATUS_BLOCKING:
+          $item->setIcon('minus-red', pht('Blocking Review'));
+          break;
+
         default:
           $item->setIcon('question-dark', pht('%s?', $reviewer->getStatus()));
           break;

src/applications/herald/adapter/HeraldAdapter.php

@@ -53,6 +53,7 @@ abstract class HeraldAdapter {
   const ACTION_ASSIGN_TASK  = 'assigntask';
   const ACTION_ADD_PROJECTS = 'addprojects';
   const ACTION_ADD_REVIEWERS = 'addreviewers';
+  const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers';
 
   const VALUE_TEXT            = 'text';
   const VALUE_NONE            = 'none';
@@ -488,6 +489,7 @@ abstract class HeraldAdapter {
           self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
           self::ACTION_ADD_PROJECTS => pht('Add projects'),
           self::ACTION_ADD_REVIEWERS => pht('Add reviewers'),
+          self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'),
         );
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array(
@@ -500,6 +502,8 @@ abstract class HeraldAdapter {
           self::ACTION_ASSIGN_TASK  => pht('Assign task to me'),
           self::ACTION_ADD_PROJECTS => pht('Add projects'),
           self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'),
+          self::ACTION_ADD_BLOCKING_REVIEWERS =>
+            pht('Add me as a blocking reviewer'),
         );
       default:
         throw new Exception("Unknown rule type '{$rule_type}'!");
@@ -525,6 +529,7 @@ abstract class HeraldAdapter {
         case self::ACTION_REMOVE_CC:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
+        case self::ACTION_ADD_REVIEWERS:
         case self::ACTION_ADD_REVIEWERS:
           // For personal rules, force these actions to target the rule owner.
           $target = array($author_phid);
@@ -623,6 +628,7 @@ abstract class HeraldAdapter {
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
         case self::ACTION_ADD_REVIEWERS:
+        case self::ACTION_ADD_BLOCKING_REVIEWERS:
           return self::VALUE_NONE;
         case self::ACTION_FLAG:
           return self::VALUE_FLAG_COLOR;
@@ -647,6 +653,7 @@ abstract class HeraldAdapter {
         case self::ACTION_ASSIGN_TASK:
           return self::VALUE_USER;
         case self::ACTION_ADD_REVIEWERS:
+        case self::ACTION_ADD_BLOCKING_REVIEWERS:
           return self::VALUE_USER_OR_PROJECT;
         default:
           throw new Exception("Unknown or invalid action '{$action}'.");

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -115,6 +115,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $this->addReviewerPHIDs;
   }
 
+  public function getBlockingReviewersAddedByHerald() {
+    return $this->blockingReviewerPHIDs;
+  }
+
   public function getPHID() {
     return $this->revision->getPHID();
   }
@@ -346,6 +350,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
           self::ACTION_REMOVE_CC,
           self::ACTION_EMAIL,
           self::ACTION_ADD_REVIEWERS,
+          self::ACTION_ADD_BLOCKING_REVIEWERS,
           self::ACTION_NOTHING,
         );
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
@@ -355,6 +360,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
           self::ACTION_EMAIL,
           self::ACTION_FLAG,
           self::ACTION_ADD_REVIEWERS,
+          self::ACTION_ADD_BLOCKING_REVIEWERS,
           self::ACTION_NOTHING,
         );
     }
@@ -453,6 +459,17 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
             true,
             pht('Added reviewers.'));
           break;
+        case self::ACTION_ADD_BLOCKING_REVIEWERS:
+          // This adds reviewers normally, it just also marks them blocking.
+          foreach ($effect->getTarget() as $phid) {
+            $this->addReviewerPHIDs[$phid] = true;
+            $this->blockingReviewerPHIDs[$phid] = true;
+          }
+          $result[] = new HeraldApplyTranscript(
+            $effect,
+            true,
+            pht('Added blocking reviewers.'));
+          break;
         default:
           throw new Exception("No rules to handle action '{$action}'.");
       }

src/applications/herald/storage/HeraldRule.php

@@ -13,7 +13,7 @@ final class HeraldRule extends HeraldDAO
   protected $repetitionPolicy;
   protected $ruleType;
 
-  protected $configVersion = 13;
+  protected $configVersion = 14;
 
   private $ruleApplied = self::ATTACHABLE; // phids for which this rule has been applied
   private $validAuthor = self::ATTACHABLE;