From a335004a91a903766f89505912967c83bcf9e8ff Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Jul 2015 11:39:35 -0700 Subject: [PATCH] Modularize Differential Reviewer actions in Herald Ref T8726. Modularizes the "Add Reviewers" and "Add Blocking Reviewers" Herald actions. --- ...724.herald.1.sql => 20150730.herald.1.sql} | 8 +- ...724.herald.2.sql => 20150730.herald.2.sql} | 4 +- ...724.herald.3.sql => 20150730.herald.3.sql} | 4 +- ...724.herald.4.sql => 20150730.herald.4.sql} | 4 +- .../sql/autopatches/20150730.herald.5.sql | 27 ++ src/__phutil_library_map__.php | 10 + .../editor/DifferentialTransactionEditor.php | 59 ----- ...iewersAddBlockingReviewersHeraldAction.php | 33 +++ ...alReviewersAddBlockingSelfHeraldAction.php | 30 +++ ...ntialReviewersAddReviewersHeraldAction.php | 33 +++ ...fferentialReviewersAddSelfHeraldAction.php | 30 +++ .../DifferentialReviewersHeraldAction.php | 240 ++++++++++++++++++ .../HeraldDifferentialRevisionAdapter.php | 53 +--- .../herald/adapter/HeraldAdapter.php | 17 -- .../PhabricatorUSEnglishTranslation.php | 10 + 15 files changed, 426 insertions(+), 136 deletions(-) rename resources/sql/autopatches/{20150724.herald.1.sql => 20150730.herald.1.sql} (76%) rename resources/sql/autopatches/{20150724.herald.2.sql => 20150730.herald.2.sql} (75%) rename resources/sql/autopatches/{20150724.herald.3.sql => 20150730.herald.3.sql} (72%) rename resources/sql/autopatches/{20150724.herald.4.sql => 20150730.herald.4.sql} (77%) create mode 100644 resources/sql/autopatches/20150730.herald.5.sql create mode 100644 src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php create mode 100644 src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php create mode 100644 src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php create mode 100644 src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php create mode 100644 src/applications/differential/herald/DifferentialReviewersHeraldAction.php diff --git a/resources/sql/autopatches/20150724.herald.1.sql b/resources/sql/autopatches/20150730.herald.1.sql similarity index 76% rename from resources/sql/autopatches/20150724.herald.1.sql rename to resources/sql/autopatches/20150730.herald.1.sql index aaf9b69196..e6e097f82c 100644 --- a/resources/sql/autopatches/20150724.herald.1.sql +++ b/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' diff --git a/resources/sql/autopatches/20150724.herald.2.sql b/resources/sql/autopatches/20150730.herald.2.sql similarity index 75% rename from resources/sql/autopatches/20150724.herald.2.sql rename to resources/sql/autopatches/20150730.herald.2.sql index eef860a24c..3d0b5a0a05 100644 --- a/resources/sql/autopatches/20150724.herald.2.sql +++ b/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' diff --git a/resources/sql/autopatches/20150724.herald.3.sql b/resources/sql/autopatches/20150730.herald.3.sql similarity index 72% rename from resources/sql/autopatches/20150724.herald.3.sql rename to resources/sql/autopatches/20150730.herald.3.sql index 58caa58407..e9fcbff59b 100644 --- a/resources/sql/autopatches/20150724.herald.3.sql +++ b/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' diff --git a/resources/sql/autopatches/20150724.herald.4.sql b/resources/sql/autopatches/20150730.herald.4.sql similarity index 77% rename from resources/sql/autopatches/20150724.herald.4.sql rename to resources/sql/autopatches/20150730.herald.4.sql index 193aca9a91..1f5c35a851 100644 --- a/resources/sql/autopatches/20150724.herald.4.sql +++ b/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' diff --git a/resources/sql/autopatches/20150730.herald.5.sql b/resources/sql/autopatches/20150730.herald.5.sql new file mode 100644 index 0000000000..84c9eea3c5 --- /dev/null +++ b/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'; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9a37356065..090dbc06e4 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index c73bb53956..0e8548c0a6 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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) { diff --git a/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php new file mode 100644 index 0000000000..0ad16ea83b --- /dev/null +++ b/src/applications/differential/herald/DifferentialReviewersAddBlockingReviewersHeraldAction.php @@ -0,0 +1,33 @@ +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)); + } + +} + diff --git a/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php new file mode 100644 index 0000000000..d173e4b814 --- /dev/null +++ b/src/applications/differential/herald/DifferentialReviewersAddBlockingSelfHeraldAction.php @@ -0,0 +1,30 @@ +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.'); + } + +} + diff --git a/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php new file mode 100644 index 0000000000..9530f903bf --- /dev/null +++ b/src/applications/differential/herald/DifferentialReviewersAddReviewersHeraldAction.php @@ -0,0 +1,33 @@ +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)); + } + +} + diff --git a/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php new file mode 100644 index 0000000000..de22adbd27 --- /dev/null +++ b/src/applications/differential/herald/DifferentialReviewersAddSelfHeraldAction.php @@ -0,0 +1,30 @@ +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.'); + } + +} + diff --git a/src/applications/differential/herald/DifferentialReviewersHeraldAction.php b/src/applications/differential/herald/DifferentialReviewersHeraldAction.php new file mode 100644 index 0000000000..3694bdb087 --- /dev/null +++ b/src/applications/differential/herald/DifferentialReviewersHeraldAction.php @@ -0,0 +1,240 @@ +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)); + } + } + + +} diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index a2d7f002d3..9166e0e060 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/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; diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 36a47dc75b..c4759a937e 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/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()); diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index d4343b2ec9..6284ed7a97 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/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.', + ), + ); }