From 4c0ec01ce5b3cf9cd6ba594aa737cd243f94ca6b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 5 Oct 2013 10:36:26 -0700 Subject: [PATCH] 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 | 2 +- .../editor/DifferentialRevisionEditor.php | 5 ++++- .../herald/adapter/HeraldAdapter.php | 17 ++++++++++++++--- .../HeraldDifferentialRevisionAdapter.php | 16 ++++++++++++++++ .../herald/controller/HeraldRuleController.php | 1 + .../js/application/herald/HeraldRuleEditor.js | 1 + 6 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 7db6c342d6..b67581b645 100644 --- a/src/__celerity_resource_map__.php +++ b/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( diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index cf09f76cd2..135aa88e19 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/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(), diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 3e867dedf8..9be426d48a 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/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:'); diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index d603796360..06a5bf0683 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/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}'."); } diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 75458a249d..1ddf62280b 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/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, ); diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index a09af6b994..35b7a01e90 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/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];