Allow Herald rules to add reviewers

Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.

I will probably write some lengthy treatise on how you shouldn't use this rule later.

Implementation is straightforward because Differential previously supported this rule.

This rule can also be used to add project reviewers.

Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

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

src/__celerity_resource_map__.php

@@ -1176,7 +1176,7 @@ celerity_register_resource_map(array(
   ),
   'herald-rule-editor' =>
   array(
-    'uri' => '/res/c42c0444/rsrc/js/application/herald/HeraldRuleEditor.js',
+    'uri' => '/res/a561eb19/rsrc/js/application/herald/HeraldRuleEditor.js',
     'type' => 'js',
     'requires' =>
     array(

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -272,7 +272,7 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
         $xscript_header);
 
       $sub = array(
-        'rev' => array(),
+        'rev' => $adapter->getReviewersAddedByHerald(),
         'ccs' => $adapter->getCCsAddedByHerald(),
       );
       $rem_ccs = $adapter->getCCsRemovedByHerald();
@@ -306,6 +306,9 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
       $stable[$key] = array_diff_key($old[$key], $add[$key] + $rem[$key]);
     }
 
+    // Prevent Herald rules from adding a revision's owner as a reviewer.
+    unset($add['rev'][$revision->getAuthorPHID()]);
+
     self::updateReviewers(
       $revision,
       $this->getActor(),

src/applications/herald/adapter/HeraldAdapter.php

@@ -51,6 +51,7 @@ abstract class HeraldAdapter {
   const ACTION_FLAG         = 'flag';
   const ACTION_ASSIGN_TASK  = 'assigntask';
   const ACTION_ADD_PROJECTS = 'addprojects';
+  const ACTION_ADD_REVIEWERS = 'addreviewers';
 
   const VALUE_TEXT            = 'text';
   const VALUE_NONE            = 'none';
@@ -63,6 +64,7 @@ abstract class HeraldAdapter {
   const VALUE_PROJECT         = 'project';
   const VALUE_FLAG_COLOR      = 'flagcolor';
   const VALUE_CONTENT_SOURCE  = 'contentsource';
+  const VALUE_USER_OR_PROJECT = 'userorproject';
 
   private $contentSource;
 
@@ -293,7 +295,7 @@ abstract class HeraldAdapter {
         }
         if (!is_array($condition_value)) {
           throw new HeraldInvalidConditionException(
-            "Expected conditionv value to be an array.");
+            "Expected condition value to be an array.");
         }
 
         $have = array_select_keys(array_fuse($field_value), $condition_value);
@@ -482,6 +484,7 @@ abstract class HeraldAdapter {
           self::ACTION_FLAG         => pht('Mark with flag'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
           self::ACTION_ADD_PROJECTS => pht('Add projects'),
+          self::ACTION_ADD_REVIEWERS => pht('Add reviewers'),
         );
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         return array(
@@ -491,8 +494,9 @@ abstract class HeraldAdapter {
           self::ACTION_EMAIL        => pht('Send me an email'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
           self::ACTION_FLAG         => pht('Mark with flag'),
-          self::ACTION_ASSIGN_TASK  => pht('Assign task to me.'),
+          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'),
         );
       default:
         throw new Exception("Unknown rule type '{$rule_type}'!");
@@ -518,6 +522,7 @@ abstract class HeraldAdapter {
         case self::ACTION_REMOVE_CC:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
+        case self::ACTION_ADD_REVIEWERS:
           // For personal rules, force these actions to target the rule owner.
           $target = array($author_phid);
           break;
@@ -612,6 +617,7 @@ abstract class HeraldAdapter {
         case self::ACTION_NOTHING:
         case self::ACTION_AUDIT:
         case self::ACTION_ASSIGN_TASK:
+        case self::ACTION_ADD_REVIEWERS:
           return self::VALUE_NONE;
         case self::ACTION_FLAG:
           return self::VALUE_FLAG_COLOR;
@@ -635,6 +641,8 @@ abstract class HeraldAdapter {
           return self::VALUE_FLAG_COLOR;
         case self::ACTION_ASSIGN_TASK:
           return self::VALUE_USER;
+        case self::ACTION_ADD_REVIEWERS:
+          return self::VALUE_USER_OR_PROJECT;
         default:
           throw new Exception("Unknown or invalid action '{$action}'.");
       }
@@ -752,7 +760,10 @@ abstract class HeraldAdapter {
     }
     $out[] = null;
 
-    if ($rule->getRepetitionPolicy() == HeraldRepetitionPolicyConfig::EVERY) {
+    $integer_code_for_every = HeraldRepetitionPolicyConfig::toInt(
+      HeraldRepetitionPolicyConfig::EVERY);
+
+    if ($rule->getRepetitionPolicy() == $integer_code_for_every) {
       $out[] = pht('Take these actions every time this rule matches:');
     } else {
       $out[] = pht('Take these actions the first time this rule matches:');

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -15,6 +15,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
   protected $newCCs = array();
   protected $remCCs = array();
   protected $emailPHIDs = array();
+  protected $addReviewerPHIDs = array();
 
   protected $repository;
   protected $affectedPackages;
@@ -109,6 +110,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
     return $this->emailPHIDs;
   }
 
+  public function getReviewersAddedByHerald() {
+    return $this->addReviewerPHIDs;
+  }
+
   public function getPHID() {
     return $this->revision->getPHID();
   }
@@ -327,6 +332,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
           self::ACTION_ADD_CC,
           self::ACTION_REMOVE_CC,
           self::ACTION_EMAIL,
+          self::ACTION_ADD_REVIEWERS,
           self::ACTION_NOTHING,
         );
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
@@ -335,6 +341,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
           self::ACTION_REMOVE_CC,
           self::ACTION_EMAIL,
           self::ACTION_FLAG,
+          self::ACTION_ADD_REVIEWERS,
           self::ACTION_NOTHING,
         );
     }
@@ -424,6 +431,15 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
             true,
             pht('Removed addresses from CC list.'));
           break;
+        case self::ACTION_ADD_REVIEWERS:
+          foreach ($effect->getTarget() as $phid) {
+            $this->addReviewerPHIDs[$phid] = true;
+          }
+          $result[] = new HeraldApplyTranscript(
+            $effect,
+            true,
+            pht('Added reviewers.'));
+          break;
         default:
           throw new Exception("No rules to handle action '{$action}'.");
       }

src/applications/herald/controller/HeraldRuleController.php

@@ -513,6 +513,7 @@ final class HeraldRuleController extends HeraldController {
         'repository'  => '/typeahead/common/repositories/',
         'package'     => '/typeahead/common/packages/',
         'project'     => '/typeahead/common/projects/',
+        'userorproject' => '/typeahead/common/usersorprojects/',
       ),
       'markup' => $template,
     );

webroot/rsrc/js/application/herald/HeraldRuleEditor.js

@@ -220,6 +220,7 @@ JX.install('HeraldRuleEditor', {
         case 'tag':
         case 'package':
         case 'project':
+        case 'userorproject':
           var tokenizer = this._newTokenizer(type);
           input = tokenizer[0];
           get_fn = tokenizer[1];