Modularize the Diffusion "Add Auditors" Herald action

Ref T8726.

resources/sql/autopatches/20150803.herald.1.sql

@@ -0,0 +1,13 @@
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'diffusion.auditors.add'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'audit';
+
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'diffusion.auditors.self.add'
+  WHERE r.ruleType = 'personal'
+  AND a.action = 'audit';

src/__phutil_library_map__.php

@@ -498,6 +498,9 @@ phutil_register_library_map(array(
     'DifferentialUpdateUnitResultsConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateUnitResultsConduitAPIMethod.php',
     'DifferentialViewPolicyField' => 'applications/differential/customfield/DifferentialViewPolicyField.php',
     'DiffusionAuditorDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorDatasource.php',
+    'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php',
+    'DiffusionAuditorsAddSelfHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php',
+    'DiffusionAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsHeraldAction.php',
     'DiffusionBranchQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php',
     'DiffusionBranchTableController' => 'applications/diffusion/controller/DiffusionBranchTableController.php',
     'DiffusionBranchTableView' => 'applications/diffusion/view/DiffusionBranchTableView.php',
@@ -4129,6 +4132,9 @@ phutil_register_library_map(array(
     'DifferentialUpdateUnitResultsConduitAPIMethod' => 'DifferentialConduitAPIMethod',
     'DifferentialViewPolicyField' => 'DifferentialCoreCustomField',
     'DiffusionAuditorDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
+    'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction',
+    'DiffusionAuditorsAddSelfHeraldAction' => 'DiffusionAuditorsHeraldAction',
+    'DiffusionAuditorsHeraldAction' => 'HeraldAction',
     'DiffusionBranchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionBranchTableController' => 'DiffusionController',
     'DiffusionBranchTableView' => 'DiffusionView',

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -877,30 +877,6 @@ final class PhabricatorAuditEditor
     HeraldAdapter $adapter,
     HeraldTranscript $transcript) {
 
-    $xactions = array();
-
-    $audit_phids = $adapter->getAuditMap();
-    foreach ($audit_phids as $phid => $rule_ids) {
-      foreach ($rule_ids as $rule_id) {
-        $this->addAuditReason(
-          $phid,
-          pht(
-            '%s Triggered Audit',
-            "H{$rule_id}"));
-      }
-    }
-
-    if ($audit_phids) {
-      $xactions[] = id(new PhabricatorAuditTransaction())
-        ->setTransactionType(PhabricatorAuditActionConstants::ADD_AUDITORS)
-        ->setNewValue(array_fuse(array_keys($audit_phids)))
-        ->setMetadataValue(
-          'auditStatus',
-          PhabricatorAuditStatusConstants::AUDIT_REQUIRED)
-        ->setMetadataValue(
-          'auditReasonMap', $this->auditReasonMap);
-    }
-
     $limit = self::MAX_FILES_SHOWN_IN_EMAIL;
     $files = $adapter->loadAffectedPaths();
     sort($files);
@@ -914,7 +890,7 @@ final class PhabricatorAuditEditor
     }
     $this->affectedFiles = implode("\n", $files);
 
-    return $xactions;
+    return array();
   }
 
   private function isCommitMostlyImported(PhabricatorLiskDAO $object) {

src/applications/differential/herald/DifferentialReviewersHeraldAction.php

@@ -200,7 +200,7 @@ abstract class DifferentialReviewersHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_NO_TARGETS:
         return pht('Rule lists no targets.');

src/applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php

@@ -0,0 +1,33 @@
+<?php
+
+final class DiffusionAuditorsAddAuditorsHeraldAction
+  extends DiffusionAuditorsHeraldAction {
+
+  const ACTIONCONST = 'diffusion.auditors.add';
+
+  public function getHeraldActionName() {
+    return pht('Add auditors');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $rule = $effect->getRule();
+    return $this->applyAuditors($effect->getTarget(), $rule);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new DiffusionAuditorDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add auditors: %s.', $this->renderHandleList($value));
+  }
+
+}

src/applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php

@@ -0,0 +1,30 @@
+<?php
+
+final class DiffusionAuditorsAddSelfHeraldAction
+  extends DiffusionAuditorsHeraldAction {
+
+  const ACTIONCONST = 'diffusion.auditors.self.add';
+
+  public function getHeraldActionName() {
+    return pht('Add me as an auditor');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $rule = $effect->getRule();
+    $phid = $rule->getAuthorPHID();
+    return $this->applyAuditors(array($phid), $rule);
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_NONE;
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Add rule author as auditor.');
+  }
+
+}

src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php

@@ -0,0 +1,76 @@
+<?php
+
+abstract class DiffusionAuditorsHeraldAction
+  extends HeraldAction {
+
+  const DO_ADD_AUDITORS = 'do.add-auditors';
+
+  public function getActionGroupKey() {
+    return HeraldApplicationActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorRepositoryCommit);
+  }
+
+  protected function applyAuditors(array $phids, HeraldRule $rule) {
+    $adapter = $this->getAdapter();
+    $object = $adapter->getObject();
+
+    $auditors = $object->getAudits();
+    $auditors = mpull($auditors, null, 'getAuditorPHID');
+    $current = array_keys($auditors);
+
+    $allowed_types = array(
+      PhabricatorPeopleUserPHIDType::TYPECONST,
+      PhabricatorProjectProjectPHIDType::TYPECONST,
+      PhabricatorOwnersPackagePHIDType::TYPECONST,
+    );
+
+    $targets = $this->loadStandardTargets($phids, $allowed_types, $current);
+    if (!$targets) {
+      return;
+    }
+
+    $phids = array_fuse(array_keys($targets));
+
+    // TODO: Convert this to be translatable, structured data eventually.
+    $reason_map = array();
+    foreach ($phids as $phid) {
+      $reason_map[$phid][] = pht('%s Triggered Audit', $rule->getMonogram());
+    }
+
+    $xaction = $adapter->newTransaction()
+      ->setTransactionType(PhabricatorAuditActionConstants::ADD_AUDITORS)
+      ->setNewValue($phids)
+      ->setMetadataValue(
+        'auditStatus',
+        PhabricatorAuditStatusConstants::AUDIT_REQUIRED)
+      ->setMetadataValue('auditReasonMap', $reason_map);
+
+    $adapter->queueTransaction($xaction);
+
+    $this->logEffect(self::DO_ADD_AUDITORS, $phids);
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_ADD_AUDITORS => array(
+        'icon' => 'fa-user',
+        'color' => 'green',
+        'name' => pht('Added Auditors'),
+      ),
+    );
+  }
+
+  protected function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_ADD_AUDITORS:
+        return pht(
+          'Added %s auditor(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+  }
+
+}

src/applications/diffusion/herald/HeraldCommitAdapter.php

@@ -12,8 +12,6 @@ final class HeraldCommitAdapter
   protected $commitData;
   private $commitDiff;
 
-  protected $auditMap = array();
-
   protected $affectedPaths;
   protected $affectedRevision;
   protected $affectedPackages;
@@ -86,24 +84,6 @@ final class HeraldCommitAdapter
     return pht('This rule can trigger for **repositories** and **projects**.');
   }
 
-  public function getActions($rule_type) {
-    switch ($rule_type) {
-      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
-      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
-        return array_merge(
-          array(
-            self::ACTION_AUDIT,
-          ),
-          parent::getActions($rule_type));
-      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return array_merge(
-          array(
-            self::ACTION_AUDIT,
-          ),
-          parent::getActions($rule_type));
-    }
-  }
-
   public static function newLegacyAdapter(
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit,
@@ -149,10 +129,6 @@ final class HeraldCommitAdapter
     return $this;
   }
 
-  public function getAuditMap() {
-    return $this->auditMap;
-  }
-
   public function getHeraldName() {
     return
       'r'.
@@ -322,33 +298,6 @@ final class HeraldCommitAdapter
     return $result;
   }
 
-  public function applyHeraldEffects(array $effects) {
-    assert_instances_of($effects, 'HeraldEffect');
-
-    $result = array();
-    foreach ($effects as $effect) {
-      $action = $effect->getAction();
-      switch ($action) {
-        case self::ACTION_AUDIT:
-          foreach ($effect->getTarget() as $phid) {
-            if (empty($this->auditMap[$phid])) {
-              $this->auditMap[$phid] = array();
-            }
-            $this->auditMap[$phid][] = $effect->getRule()->getID();
-          }
-          $result[] = new HeraldApplyTranscript(
-            $effect,
-            true,
-            pht('Triggered an audit.'));
-          break;
-        default:
-          $result[] = $this->applyStandardEffect($effect);
-          break;
-      }
-    }
-    return $result;
-  }
-
 
 /* -(  HarbormasterBuildableAdapterInterface  )------------------------------ */
 

src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php

@@ -72,7 +72,7 @@ final class PhabricatorFlagAddFlagHeraldAction extends HeraldAction {
     return pht('Mark with %s flag.', $color);
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_IGNORE:
         return pht(

src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php

@@ -68,7 +68,7 @@ final class HarbormasterRunBuildPlansHeraldAction
         'color' => 'red',
         'name' => pht('Invalid Targets'),
       ),
-      self::DO_ALREADY_REQUIRED => array(
+      self::DO_BUILD => array(
         'icon' => 'fa-play',
         'color' => 'green',
         'name' => pht('Building'),
@@ -76,7 +76,7 @@ final class HarbormasterRunBuildPlansHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_NO_TARGETS:
         return pht('Rule lists no targets.');
@@ -85,7 +85,7 @@ final class HarbormasterRunBuildPlansHeraldAction
           '%s build plan(s) are not valid: %s.',
           new PhutilNumber(count($data)),
           $this->renderHandleList($data));
-      case self::DO_REQUIRED:
+      case self::DO_BUILD:
         return pht(
           'Started %s build(s): %s.',
           new PhutilNumber(count($data)),

src/applications/herald/action/HeraldAction.php

@@ -9,11 +9,20 @@ abstract class HeraldAction extends Phobject {
   const STANDARD_NONE = 'standard.none';
   const STANDARD_PHID_LIST = 'standard.phid.list';
 
+  const DO_STANDARD_EMPTY = 'do.standard.empty';
+  const DO_STANDARD_NO_EFFECT = 'do.standard.no-effect';
+  const DO_STANDARD_INVALID = 'do.standard.invalid';
+  const DO_STANDARD_UNLOADABLE = 'do.standard.unloadable';
+  const DO_STANDARD_PERMISSION = 'do.standard.permission';
+
   abstract public function getHeraldActionName();
   abstract public function supportsObject($object);
   abstract public function supportsRuleType($rule_type);
   abstract public function applyEffect($object, HeraldEffect $effect);
-  abstract public function renderActionEffectDescription($type, $data);
+
+  protected function renderActionEffectDescription($type, $data) {
+    return null;
+  }
 
   public function getActionGroupKey() {
     return null;
@@ -154,21 +163,21 @@ abstract class HeraldAction extends Phobject {
   }
 
   private function getActionEffectSpec($type) {
-    $map = $this->getActionEffectMap();
+    $map = $this->getActionEffectMap() + $this->getStandardEffectMap();
     return idx($map, $type, array());
   }
 
-  public function renderActionEffectIcon($type, $data) {
+  final public function renderActionEffectIcon($type, $data) {
     $map = $this->getActionEffectSpec($type);
     return idx($map, 'icon');
   }
 
-  public function renderActionEffectColor($type, $data) {
+  final public function renderActionEffectColor($type, $data) {
     $map = $this->getActionEffectSpec($type);
     return idx($map, 'color');
   }
 
-  public function renderActionEffectName($type, $data) {
+  final public function renderActionEffectName($type, $data) {
     $map = $this->getActionEffectSpec($type);
     return idx($map, 'name');
   }
@@ -184,4 +193,173 @@ abstract class HeraldAction extends Phobject {
       ->render();
   }
 
+  protected function loadStandardTargets(
+    array $phids,
+    array $allowed_types,
+    array $current_value) {
+
+    $phids = array_fuse($phids);
+    if (!$phids) {
+      $this->logEffect(self::DO_STANDARD_EMPTY);
+    }
+
+    $current_value = array_fuse($current_value);
+    $no_effect = array();
+    foreach ($phids as $phid) {
+      if (isset($current_value[$phid])) {
+        $no_effect[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($no_effect) {
+      $this->logEffect(self::DO_STANDARD_NO_EFFECT, $no_effect);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $allowed_types = array_fuse($allowed_types);
+    $invalid = array();
+    foreach ($phids as $phid) {
+      $type = phid_get_type($phid);
+      if ($type == PhabricatorPHIDConstants::PHID_TYPE_UNKNOWN) {
+        $invalid[] = $phid;
+        unset($phids[$phid]);
+        continue;
+      }
+
+      if ($allowed_types && empty($allowed_types[$type])) {
+        $invalid[] = $phid;
+        unset($phids[$phid]);
+        continue;
+      }
+    }
+
+    if ($invalid) {
+      $this->logEffect(self::DO_STANDARD_INVALID, $invalid);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $targets = id(new PhabricatorObjectQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs($phids)
+      ->execute();
+    $targets = mpull($targets, null, 'getPHID');
+
+    $unloadable = array();
+    foreach ($phids as $phid) {
+      if (empty($targets[$phid])) {
+        $unloadable[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($unloadable) {
+      $this->logEffect(self::DO_STANDARD_UNLOADABLE, $unloadable);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $adapter = $this->getAdapter();
+    $object = $adapter->getObject();
+
+    if ($object instanceof PhabricatorPolicyInterface) {
+      $no_permission = 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_permission[] = $phid;
+        unset($targets[$phid]);
+      }
+    }
+
+    if ($no_permission) {
+      $this->logEffect(self::DO_STANDARD_PERMISSION, $no_permission);
+    }
+
+    return $targets;
+  }
+
+  protected function getStandardEffectMap() {
+    return array(
+      self::DO_STANDARD_EMPTY => array(
+        'icon' => 'fa-ban',
+        'color' => 'grey',
+        'name' => pht('No Targets'),
+      ),
+      self::DO_STANDARD_NO_EFFECT => array(
+        'icon' => 'fa-circle-o',
+        'color' => 'grey',
+        'name' => pht('No Effect'),
+      ),
+      self::DO_STANDARD_INVALID => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Targets'),
+      ),
+      self::DO_STANDARD_UNLOADABLE => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Unloadable Targets'),
+      ),
+      self::DO_STANDARD_PERMISSION => array(
+        'icon' => 'fa-lock',
+        'color' => 'red',
+        'name' => pht('No Permission'),
+      ),
+    );
+  }
+
+  final public function renderEffectDescription($type, $data) {
+    $result = $this->renderActionEffectDescription($type, $data);
+    if ($result !== null) {
+      return $result;
+    }
+
+    switch ($type) {
+      case self::DO_STANDARD_EMPTY:
+        return pht(
+          'This action specifies no targets.');
+      case self::DO_STANDARD_NO_EFFECT:
+        return pht(
+          'This action has no effect on %s target(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_STANDARD_INVALID:
+        return pht(
+          '%s target(s) are invalid or of the wrong type: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_STANDARD_UNLOADABLE:
+        return pht(
+          '%s target(s) could not be loaded: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_STANDARD_PERMISSION:
+        return pht(
+          '%s target(s) do not have permission to see this object: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+
+    return null;
+  }
+
 }

src/applications/herald/action/HeraldDoNothingAction.php

@@ -43,7 +43,7 @@ final class HeraldDoNothingAction extends HeraldAction {
     return pht('Do nothing.');
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     return pht('Did nothing.');
   }
 

src/applications/herald/adapter/HeraldAdapter.php

@@ -26,7 +26,6 @@ abstract class HeraldAdapter extends Phobject {
   const CONDITION_IS_TRUE         = 'true';
   const CONDITION_IS_FALSE        = 'false';
 
-  const ACTION_AUDIT        = 'audit';
   const ACTION_BLOCK = 'block';
 
   private $contentSource;
@@ -712,13 +711,11 @@ abstract class HeraldAdapter extends Phobject {
       case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
       case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
         $standard = array(
-          self::ACTION_AUDIT        => pht('Trigger an Audit by'),
           self::ACTION_BLOCK => pht('Block change with message'),
         );
         break;
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
         $standard = array(
-          self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
         );
         break;
       default:
@@ -757,8 +754,6 @@ abstract class HeraldAdapter extends Phobject {
     $rule_type = $rule->getRuleType();
     if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
       switch ($action->getAction()) {
-        case self::ACTION_AUDIT:
-          break;
         case self::ACTION_BLOCK:
           break;
         default:
@@ -790,14 +785,8 @@ abstract class HeraldAdapter extends Phobject {
 
     $is_personal = ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
 
-    if ($is_personal) {
+    if (!$is_personal) {
       switch ($action) {
-        case self::ACTION_AUDIT:
-          return new HeraldEmptyFieldValue();
-      }
-    } else {
-      switch ($action) {
-        case self::ACTION_AUDIT:
         case self::ACTION_BLOCK:
           return new HeraldTextFieldValue();
       }

src/applications/herald/controller/HeraldTranscriptController.php

@@ -376,7 +376,7 @@ final class HeraldTranscriptController extends HeraldController {
             $icon = $action->renderActionEffectIcon($type, $data);
             $color = $action->renderActionEffectColor($type, $data);
             $name = $action->renderActionEffectName($type, $data);
-            $note = $action->renderActionEffectDescription($type, $data);
+            $note = $action->renderEffectDescription($type, $data);
           } else {
             $icon = 'fa-question-circle';
             $color = 'indigo';

src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php

@@ -142,7 +142,7 @@ final class LegalpadRequireSignatureHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_NO_TARGETS:
         return pht('Rule lists no targets.');

src/applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php

@@ -90,7 +90,7 @@ abstract class ManiphestTaskAssignHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_EMPTY:
         return pht('Action lists no user to assign.');

src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php

@@ -50,7 +50,7 @@ abstract class PhabricatorMetaMTAEmailHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_SEND:
         return pht(

src/applications/project/herald/PhabricatorProjectHeraldAction.php

@@ -145,7 +145,7 @@ abstract class PhabricatorProjectHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_NO_TARGETS:
         return pht('Rule lists no projects.');

src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php

@@ -196,7 +196,7 @@ abstract class PhabricatorSubscriptionsHeraldAction
     );
   }
 
-  public function renderActionEffectDescription($type, $data) {
+  protected function renderActionEffectDescription($type, $data) {
     switch ($type) {
       case self::DO_NO_TARGETS:
         return pht('Rule lists no targets.');

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1338,6 +1338,16 @@ final class PhabricatorUSEnglishTranslation
         'Required signatures: %2$s.',
       ),
 
+      'Started %s build(s): %s.' => array(
+        'Started a build: %2$s.',
+        'Started builds: %2$s.',
+      ),
+
+      'Added %s auditor(s): %s.' => array(
+        'Added an auditor: %2$s.',
+        'Added auditors: %2$s.',
+      ),
+
     );
   }