Modularize Differential Reviewer actions in Herald

Ref T8726. Modularizes the "Add Reviewers" and "Add Blocking Reviewers" Herald actions.

resources/sql/autopatches/20150730.herald.1.sql

@@ -1,25 +1,25 @@
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'subscribers.add'
   WHERE r.ruleType != 'personal'
   AND a.action = 'addcc';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'subscribers.self.add'
   WHERE r.ruleType = 'personal'
   AND a.action = 'addcc';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'subscribers.remove'
   WHERE r.ruleType != 'personal'
   AND a.action = 'remcc';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'subscribers.self.remove'

resources/sql/autopatches/20150730.herald.2.sql

@@ -1,11 +1,11 @@
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'email.other'
   WHERE r.ruleType != 'personal'
   AND a.action = 'email';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'email.self'

resources/sql/autopatches/20150730.herald.3.sql

@@ -1,10 +1,10 @@
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'projects.add'
   WHERE a.action = 'addprojects';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'projects.remove'

resources/sql/autopatches/20150730.herald.4.sql

@@ -1,11 +1,11 @@
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'maniphest.assign.other'
   WHERE r.ruleType != 'personal'
   AND a.action = 'assigntask';
 
-UPDATE {$NAMESPACE}_herald.herald_actionrecord a
+UPDATE {$NAMESPACE}_herald.herald_action a
   JOIN {$NAMESPACE}_herald.herald_rule r
   ON a.ruleID = r.id
   SET a.action = 'maniphest.assign.self'

resources/sql/autopatches/20150730.herald.5.sql

@@ -0,0 +1,27 @@
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'differential.reviewers.blocking'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'addreviewers';
+
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'differential.reviewers.self.blocking'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'addreviewers';
+
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'differential.reviewers.add'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'addblockingreviewers';
+
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'differential.reviewers.self.add'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'addblockingreviewers';

src/__phutil_library_map__.php

@@ -435,7 +435,12 @@ phutil_register_library_map(array(
     'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php',
     'DifferentialReviewerForRevisionEdgeType' => 'applications/differential/edge/DifferentialReviewerForRevisionEdgeType.php',
     'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php',
+    'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php',
+    'DifferentialReviewersAddBlockingSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php',
+    'DifferentialReviewersAddReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php',
+    'DifferentialReviewersAddSelfHeraldAction' => 'applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php',
     'DifferentialReviewersField' => 'applications/differential/customfield/DifferentialReviewersField.php',
+    'DifferentialReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersHeraldAction.php',
     'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php',
     'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
     'DifferentialRevisionAffectedFilesHeraldField' => 'applications/differential/herald/DifferentialRevisionAffectedFilesHeraldField.php',
@@ -4044,7 +4049,12 @@ phutil_register_library_map(array(
     'DifferentialReviewer' => 'Phobject',
     'DifferentialReviewerForRevisionEdgeType' => 'PhabricatorEdgeType',
     'DifferentialReviewerStatus' => 'Phobject',
+    'DifferentialReviewersAddBlockingReviewersHeraldAction' => 'DifferentialReviewersHeraldAction',
+    'DifferentialReviewersAddBlockingSelfHeraldAction' => 'DifferentialReviewersHeraldAction',
+    'DifferentialReviewersAddReviewersHeraldAction' => 'DifferentialReviewersHeraldAction',
+    'DifferentialReviewersAddSelfHeraldAction' => 'DifferentialReviewersHeraldAction',
     'DifferentialReviewersField' => 'DifferentialCoreCustomField',
+    'DifferentialReviewersHeraldAction' => 'HeraldAction',
     'DifferentialReviewersView' => 'AphrontView',
     'DifferentialRevision' => array(
       'DifferentialDAO',

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -1588,11 +1588,6 @@ final class DifferentialTransactionEditor
       $revision,
       $revision->getActiveDiff());
 
-    $reviewers = $revision->getReviewerStatus();
-    $reviewer_phids = mpull($reviewers, 'getReviewerPHID');
-
-    $adapter->setExplicitReviewers($reviewer_phids);
-
     return $adapter;
   }
 
@@ -1603,60 +1598,6 @@ final class DifferentialTransactionEditor
 
     $xactions = array();
 
-    // Build a transaction to adjust reviewers.
-    $reviewers = array(
-      DifferentialReviewerStatus::STATUS_ADDED =>
-        array_keys($adapter->getReviewersAddedByHerald()),
-      DifferentialReviewerStatus::STATUS_BLOCKING =>
-        array_keys($adapter->getBlockingReviewersAddedByHerald()),
-    );
-
-    $old_reviewers = $object->getReviewerStatus();
-    $old_reviewers = mpull($old_reviewers, null, 'getReviewerPHID');
-
-    $value = array();
-    foreach ($reviewers as $status => $phids) {
-      foreach ($phids as $phid) {
-        if ($phid == $object->getAuthorPHID()) {
-          // Don't try to add the revision's author as a reviewer, since this
-          // isn't valid and doesn't make sense.
-          continue;
-        }
-
-        // If the target is already a reviewer, don't try to change anything
-        // if their current status is at least as strong as the new status.
-        // For example, don't downgrade an "Accepted" to a "Blocking Reviewer".
-        $old_reviewer = idx($old_reviewers, $phid);
-        if ($old_reviewer) {
-          $old_status = $old_reviewer->getStatus();
-
-          $old_strength = DifferentialReviewerStatus::getStatusStrength(
-            $old_status);
-          $new_strength = DifferentialReviewerStatus::getStatusStrength(
-            $status);
-
-          if ($new_strength <= $old_strength) {
-            continue;
-          }
-        }
-
-        $value['+'][$phid] = array(
-          'data' => array(
-            'status' => $status,
-          ),
-        );
-      }
-    }
-
-    if ($value) {
-      $edge_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
-
-      $xactions[] = id(new DifferentialTransaction())
-        ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-        ->setMetadataValue('edge:type', $edge_reviewer)
-        ->setNewValue($value);
-    }
-
     // Require legalpad document signatures.
     $legal_phids = $adapter->getRequiredSignatureDocumentPHIDs();
     if ($legal_phids) {

src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php

@@ -0,0 +1,33 @@
+<?php
+
+final class DifferentialReviewersAddBlockingReviewersHeraldAction
+  extends DifferentialReviewersHeraldAction {
+
+  const ACTIONCONST = 'differential.reviewers.blocking';
+
+  public function getHeraldActionName() {
+    return pht('Add blocking reviewers');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applyReviewers($effect->getTarget(), $is_blocking = true);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new PhabricatorMetaMTAMailableDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add blocking reviewers: %s.', $this->renderHandleList($value));
+  }
+
+}
+

src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php

@@ -0,0 +1,30 @@
+<?php
+
+final class DifferentialReviewersAddBlockingSelfHeraldAction
+  extends DifferentialReviewersHeraldAction {
+
+  const ACTIONCONST = 'differential.reviewers.self.blocking';
+
+  public function getHeraldActionName() {
+    return pht('Add me as a blocking reviewer');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+    return $this->applyReviewers(array($phid), $is_blocking = true);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add rule author as blocking reviewer.');
+  }
+
+}
+

src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php

@@ -0,0 +1,33 @@
+<?php
+
+final class DifferentialReviewersAddReviewersHeraldAction
+  extends DifferentialReviewersHeraldAction {
+
+  const ACTIONCONST = 'differential.reviewers.add';
+
+  public function getHeraldActionName() {
+    return pht('Add reviewers');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applyReviewers($effect->getTarget(), $is_blocking = false);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new PhabricatorMetaMTAMailableDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add reviewers: %s.', $this->renderHandleList($value));
+  }
+
+}
+

src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php

@@ -0,0 +1,30 @@
+<?php
+
+final class DifferentialReviewersAddSelfHeraldAction
+  extends DifferentialReviewersHeraldAction {
+
+  const ACTIONCONST = 'differential.reviewers.self.add';
+
+  public function getHeraldActionName() {
+    return pht('Add me as a reviewer');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $effect->getRule()->getAuthorPHID();
+    return $this->applyReviewers(array($phid), $is_blocking = false);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add rule author as reviewer.');
+  }
+
+}
+

src/applications/differential/herald/DifferentialReviewersHeraldAction.php

@@ -0,0 +1,240 @@
+<?php
+
+abstract class DifferentialReviewersHeraldAction
+  extends HeraldAction {
+
+  const DO_NO_TARGETS = 'do.no-targets';
+  const DO_AUTHORS = 'do.authors';
+  const DO_INVALID = 'do.invalid';
+  const DO_ALREADY_REVIEWERS = 'do.already-reviewers';
+  const DO_PERMISSION = 'do.permission';
+  const DO_ADD_REVIEWERS = 'do.add-reviewers';
+  const DO_ADD_BLOCKING_REVIEWERS = 'do.add-blocking-reviewers';
+
+  public function getActionGroupKey() {
+    return HeraldApplicationActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof DifferentialRevision);
+  }
+
+  protected function applyReviewers(array $phids, $is_blocking) {
+    $adapter = $this->getAdapter();
+    $object = $adapter->getObject();
+
+    $phids = array_fuse($phids);
+    if (!$phids) {
+      $this->logEffect(self::DO_NO_TARGETS);
+      return;
+    }
+
+    // Don't try to add revision authors as reviewers.
+    $authors = array();
+    foreach ($phids as $phid) {
+      if ($phid == $object->getAuthorPHID()) {
+        $authors[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($authors) {
+      $this->logEffect(self::DO_AUTHORS, $authors);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $reviewers = $object->getReviewerStatus();
+    $reviewers = mpull($reviewers, null, 'getReviewerPHID');
+
+    if ($is_blocking) {
+      $new_status = DifferentialReviewerStatus::STATUS_BLOCKING;
+    } else {
+      $new_status = DifferentialReviewerStatus::STATUS_ADDED;
+    }
+
+    $new_strength = DifferentialReviewerStatus::getStatusStrength(
+      $new_status);
+
+    $already = array();
+    foreach ($phids as $phid) {
+      if (!isset($reviewers[$phid])) {
+        continue;
+      }
+
+      // If we're applying a stronger status (usually, upgrading a reviewer
+      // into a blocking reviewer), skip this check so we apply the change.
+      $old_strength = DifferentialReviewerStatus::getStatusStrength(
+        $reviewers[$phid]->getStatus());
+      if ($old_strength <= $new_strength) {
+        continue;
+      }
+
+      $already[] = $phid;
+      unset($phids[$phid]);
+    }
+
+    if ($already) {
+      $this->logEffect(self::DO_ALREADY_REVIEWERS, $already);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $targets = id(new PhabricatorObjectQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs($phids)
+      ->execute();
+    $targets = mpull($targets, null, 'getPHID');
+
+    $invalid = array();
+    foreach ($phids as $phid) {
+      if (empty($targets[$phid])) {
+        $invalid[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($invalid) {
+      $this->logEffect(self::DO_INVALID, $invalid);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $no_access = array();
+    foreach ($targets as $phid => $target) {
+      if (!($target instanceof PhabricatorUser)) {
+        continue;
+      }
+
+      $can_view = PhabricatorPolicyFilter::hasCapability(
+        $target,
+        $object,
+        PhabricatorPolicyCapability::CAN_VIEW);
+      if ($can_view) {
+        continue;
+      }
+
+      $no_access[] = $phid;
+      unset($phids[$phid]);
+    }
+
+    if ($no_access) {
+      $this->logEffect(self::DO_PERMISSION, $no_access);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $value = array();
+    foreach ($phids as $phid) {
+      $value[$phid] = array(
+        'data' => array(
+          'status' => $new_status,
+        ),
+      );
+    }
+
+    $edgetype_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
+
+    $xaction = $adapter->newTransaction()
+      ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+      ->setMetadataValue('edge:type', $edgetype_reviewer)
+      ->setNewValue(
+        array(
+          '+' => $value,
+        ));
+
+    $adapter->queueTransaction($xaction);
+
+    if ($is_blocking) {
+      $this->logEffect(self::DO_ADD_BLOCKING_REVIEWERS, $phids);
+    } else {
+      $this->logEffect(self::DO_ADD_REVIEWERS, $phids);
+    }
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_NO_TARGETS => array(
+        'icon' => 'fa-ban',
+        'color' => 'grey',
+        'name' => pht('No Targets'),
+      ),
+      self::DO_AUTHORS => array(
+        'icon' => 'fa-user',
+        'color' => 'grey',
+        'name' => pht('Revision Author'),
+      ),
+      self::DO_INVALID => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Targets'),
+      ),
+      self::DO_ALREADY_REVIEWERS => array(
+        'icon' => 'fa-user',
+        'color' => 'grey',
+        'name' => pht('Already Reviewers'),
+      ),
+      self::DO_PERMISSION => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('No Permission'),
+      ),
+      self::DO_ADD_REVIEWERS => array(
+        'icon' => 'fa-user',
+        'color' => 'green',
+        'name' => pht('Added Reviewers'),
+      ),
+      self::DO_ADD_BLOCKING_REVIEWERS => array(
+        'icon' => 'fa-user',
+        'color' => 'green',
+        'name' => pht('Added Blocking Reviewers'),
+      ),
+    );
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_NO_TARGETS:
+        return pht('Rule lists no targets.');
+      case self::DO_AUTHORS:
+        return pht(
+          'Declined to add revision author as reviewer: %s.',
+          $this->renderHandleList($data));
+      case self::DO_INVALID:
+        return pht(
+          'Declined to act on %s invalid target(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ALREADY_REVIEWERS:
+        return pht(
+          '%s target(s) were already reviewers: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_PERMISSION:
+        return pht(
+          '%s target(s) do not have permission to see the revision: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ADD_REVIEWERS:
+        return pht(
+          'Added %s reviewer(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ADD_BLOCKING_REVIEWERS:
+        return pht(
+          'Added %s blocking reviewer(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+  }
+
+
+}

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -5,9 +5,6 @@ final class HeraldDifferentialRevisionAdapter
 
   protected $revision;
 
-  protected $explicitReviewers;
-  protected $addReviewerPHIDs = array();
-  protected $blockingReviewerPHIDs = array();
   protected $buildPlans = array();
   protected $requiredSignatureDocumentPHIDs = array();
 
@@ -83,19 +80,6 @@ final class HeraldDifferentialRevisionAdapter
     return $object;
   }
 
-  public function setExplicitReviewers($explicit_reviewers) {
-    $this->explicitReviewers = $explicit_reviewers;
-    return $this;
-  }
-
-  public function getReviewersAddedByHerald() {
-    return $this->addReviewerPHIDs;
-  }
-
-  public function getBlockingReviewersAddedByHerald() {
-    return $this->blockingReviewerPHIDs;
-  }
-
   public function getRequiredSignatureDocumentPHIDs() {
     return $this->requiredSignatureDocumentPHIDs;
   }
@@ -147,14 +131,8 @@ final class HeraldDifferentialRevisionAdapter
   }
 
   public function loadReviewers() {
-    // TODO: This can probably go away as I believe it's just a performance
-    // optimization, just retaining it while modularizing fields to limit the
-    // scope of that change.
-    if (isset($this->explicitReviewers)) {
-      return array_keys($this->explicitReviewers);
-    } else {
-      return $this->revision->getReviewers();
-    }
+    $reviewers = $this->getObject()->getReviewerStatus();
+    return mpull($reviewers, 'getReviewerPHID');
   }
 
   public function getActions($rule_type) {
@@ -162,18 +140,13 @@ final class HeraldDifferentialRevisionAdapter
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
         return array_merge(
           array(
-            self::ACTION_ADD_REVIEWERS,
-            self::ACTION_ADD_BLOCKING_REVIEWERS,
             self::ACTION_APPLY_BUILD_PLANS,
             self::ACTION_REQUIRE_SIGNATURE,
           ),
           parent::getActions($rule_type));
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array_merge(
-          array(
-            self::ACTION_ADD_REVIEWERS,
-            self::ACTION_ADD_BLOCKING_REVIEWERS,
-          ),
+          array(),
           parent::getActions($rule_type));
     }
   }
@@ -186,26 +159,6 @@ final class HeraldDifferentialRevisionAdapter
     foreach ($effects as $effect) {
       $action = $effect->getAction();
       switch ($action) {
-        case self::ACTION_ADD_REVIEWERS:
-          foreach ($effect->getTarget() as $phid) {
-            $this->addReviewerPHIDs[$phid] = true;
-          }
-          $result[] = new HeraldApplyTranscript(
-            $effect,
-            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;
         case self::ACTION_APPLY_BUILD_PLANS:
           foreach ($effect->getTarget() as $phid) {
             $this->buildPlans[] = $phid;

src/applications/herald/adapter/HeraldAdapter.php

@@ -27,8 +27,6 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_FALSE        = 'false';
 
   const ACTION_AUDIT        = 'audit';
-  const ACTION_ADD_REVIEWERS = 'addreviewers';
-  const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers';
   const ACTION_APPLY_BUILD_PLANS = 'applybuildplans';
   const ACTION_BLOCK = 'block';
   const ACTION_REQUIRE_SIGNATURE = 'signature';
@@ -717,8 +715,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         $standard = array(
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
-          self::ACTION_ADD_REVIEWERS => pht('Add reviewers'),
-          self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'),
           self::ACTION_APPLY_BUILD_PLANS => pht('Run build plans'),
           self::ACTION_REQUIRE_SIGNATURE => pht('Require legal signatures'),
           self::ACTION_BLOCK => pht('Block change with message'),
@@ -727,9 +723,6 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         $standard = array(
           self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
-          self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'),
-          self::ACTION_ADD_BLOCKING_REVIEWERS =>
-            pht('Add me as a blocking reviewer'),
         );
         break;
       default:
@@ -769,10 +762,6 @@ abstract class HeraldAdapter extends Phobject {
     if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
       switch ($action->getAction()) {
         case self::ACTION_AUDIT:
-        case self::ACTION_ADD_REVIEWERS:
-        case self::ACTION_ADD_BLOCKING_REVIEWERS:
-          // For personal rules, force these actions to target the rule owner.
-          $target = array($author_phid);
           break;
         case self::ACTION_BLOCK:
           break;
@@ -808,17 +797,11 @@ abstract class HeraldAdapter extends Phobject {
     if ($is_personal) {
       switch ($action) {
         case self::ACTION_AUDIT:
-        case self::ACTION_ADD_REVIEWERS:
-        case self::ACTION_ADD_BLOCKING_REVIEWERS:
           return new HeraldEmptyFieldValue();
       }
     } else {
       switch ($action) {
         case self::ACTION_AUDIT:
-        case self::ACTION_ADD_REVIEWERS:
-        case self::ACTION_ADD_BLOCKING_REVIEWERS:
-          return $this->buildTokenizerFieldValue(
-            new PhabricatorProjectOrUserDatasource());
         case self::ACTION_APPLY_BUILD_PLANS:
           return $this->buildTokenizerFieldValue(
             new HarbormasterBuildPlanDatasource());

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1323,6 +1323,16 @@ final class PhabricatorUSEnglishTranslation
         'Removed projects: %2$s.',
       ),
 
+      'Added %s reviewer(s): %s.' => array(
+        'Added a reviewer: %2$s.',
+        'Added reviewers: %2$s.',
+      ),
+
+      'Added %s blocking reviewer(s): %s.' => array(
+        'Added a blocking reviewer: %2$s.',
+        'Added blocking reviewers: %2$s.',
+      ),
+
     );
   }