From 2e87f9f53cda1d20308fdd59ca94d87b915f395e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 2 Aug 2013 10:54:19 -0700 Subject: [PATCH] Move most Herald condition config into dynamic adapters Summary: Ref T2769. Pushes most condition configuration into Adapters, out of the hard-coded class. Test Plan: Looked at, edited, and dry-run'd Herald rules. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2769 Differential Revision: https://secure.phabricator.com/D6658 --- .../herald/adapter/HeraldAdapter.php | 95 +++++++++++++++++++ .../herald/adapter/HeraldCommitAdapter.php | 47 ++++++--- .../HeraldDifferentialRevisionAdapter.php | 20 ++-- .../herald/config/HeraldConditionConfig.php | 80 +--------------- .../controller/HeraldRuleController.php | 14 ++- 5 files changed, 151 insertions(+), 105 deletions(-) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 570cf3188c..871abce5a3 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -16,6 +16,24 @@ abstract class HeraldAdapter { const FIELD_AFFECTED_PACKAGE = 'affected-package'; const FIELD_AFFECTED_PACKAGE_OWNER = 'affected-package-owner'; + const CONDITION_CONTAINS = 'contains'; + const CONDITION_NOT_CONTAINS = '!contains'; + const CONDITION_IS = 'is'; + const CONDITION_IS_NOT = '!is'; + const CONDITION_IS_ANY = 'isany'; + const CONDITION_IS_NOT_ANY = '!isany'; + const CONDITION_INCLUDE_ALL = 'all'; + const CONDITION_INCLUDE_ANY = 'any'; + const CONDITION_INCLUDE_NONE = 'none'; + const CONDITION_IS_ME = 'me'; + const CONDITION_IS_NOT_ME = '!me'; + const CONDITION_REGEXP = 'regexp'; + const CONDITION_RULE = 'conditions'; + const CONDITION_NOT_RULE = '!conditions'; + const CONDITION_EXISTS = 'exists'; + const CONDITION_NOT_EXISTS = '!exists'; + const CONDITION_REGEXP_PAIR = 'regexp-pair'; + abstract public function getPHID(); abstract public function getHeraldName(); abstract public function getHeraldTypeName(); @@ -56,6 +74,83 @@ abstract class HeraldAdapter { ); } + public function getConditionNameMap() { + return array( + self::CONDITION_CONTAINS => pht('contains'), + self::CONDITION_NOT_CONTAINS => pht('does not contain'), + self::CONDITION_IS => pht('is'), + self::CONDITION_IS_NOT => pht('is not'), + self::CONDITION_IS_ANY => pht('is any of'), + self::CONDITION_IS_NOT_ANY => pht('is not any of'), + self::CONDITION_INCLUDE_ALL => pht('include all of'), + self::CONDITION_INCLUDE_ANY => pht('include any of'), + self::CONDITION_INCLUDE_NONE => pht('include none of'), + self::CONDITION_IS_ME => pht('is myself'), + self::CONDITION_IS_NOT_ME => pht('is not myself'), + self::CONDITION_REGEXP => pht('matches regexp'), + self::CONDITION_RULE => pht('matches:'), + self::CONDITION_NOT_RULE => pht('does not match:'), + self::CONDITION_EXISTS => pht('exists'), + self::CONDITION_NOT_EXISTS => pht('does not exist'), + self::CONDITION_REGEXP_PAIR => pht('matches regexp pair'), + ); + } + + public function getConditionsForField($field) { + switch ($field) { + case self::FIELD_TITLE: + case self::FIELD_BODY: + return array( + self::CONDITION_CONTAINS, + self::CONDITION_NOT_CONTAINS, + self::CONDITION_IS, + self::CONDITION_IS_NOT, + self::CONDITION_REGEXP, + ); + case self::FIELD_AUTHOR: + case self::FIELD_REPOSITORY: + case self::FIELD_REVIEWER: + return array( + self::CONDITION_IS_ANY, + self::CONDITION_IS_NOT_ANY, + ); + case self::FIELD_TAGS: + case self::FIELD_REVIEWERS: + case self::FIELD_CC: + return array( + self::CONDITION_INCLUDE_ALL, + self::CONDITION_INCLUDE_ANY, + self::CONDITION_INCLUDE_NONE, + ); + case self::FIELD_DIFF_FILE: + return array( + self::CONDITION_CONTAINS, + self::CONDITION_REGEXP, + ); + case self::FIELD_DIFF_CONTENT: + return array( + self::CONDITION_CONTAINS, + self::CONDITION_REGEXP, + self::CONDITION_REGEXP_PAIR, + ); + case self::FIELD_RULE: + return array( + self::CONDITION_RULE, + self::CONDITION_NOT_RULE, + ); + case self::FIELD_AFFECTED_PACKAGE: + case self::FIELD_AFFECTED_PACKAGE_OWNER: + return array( + self::CONDITION_INCLUDE_ANY, + self::CONDITION_INCLUDE_NONE, + ); + default: + throw new Exception( + "This adapter does not define conditions for field '{$field}'!"); + } + } + + public static function applyFlagEffect(HeraldEffect $effect, $phid) { $color = $effect->getTarget(); diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index f39ddf4fc8..68e0650409 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -63,6 +63,29 @@ final class HeraldCommitAdapter extends HeraldAdapter { ); } + public function getConditionsForField($field) { + switch ($field) { + case self::FIELD_DIFFERENTIAL_REVIEWERS: + case self::FIELD_DIFFERENTIAL_CCS: + return array( + self::CONDITION_INCLUDE_ALL, + self::CONDITION_INCLUDE_ANY, + self::CONDITION_INCLUDE_NONE, + ); + case self::FIELD_DIFFERENTIAL_REVISION: + return array( + self::CONDITION_EXISTS, + self::CONDITION_NOT_EXISTS, + ); + case self::FIELD_NEED_AUDIT_FOR_PACKAGE: + return array( + self::CONDITION_INCLUDE_ANY, + self::CONDITION_INCLUDE_NONE, + ); + } + return parent::getConditionsForField($field); + } + public static function newLegacyAdapter( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit, @@ -182,17 +205,17 @@ final class HeraldCommitAdapter extends HeraldAdapter { public function getHeraldField($field) { $data = $this->commitData; switch ($field) { - case HeraldFieldConfig::FIELD_BODY: + case self::FIELD_BODY: return $data->getCommitMessage(); - case HeraldFieldConfig::FIELD_AUTHOR: + case self::FIELD_AUTHOR: return $data->getCommitDetail('authorPHID'); - case HeraldFieldConfig::FIELD_REVIEWER: + case self::FIELD_REVIEWER: return $data->getCommitDetail('reviewerPHID'); - case HeraldFieldConfig::FIELD_DIFF_FILE: + case self::FIELD_DIFF_FILE: return $this->loadAffectedPaths(); - case HeraldFieldConfig::FIELD_REPOSITORY: + case self::FIELD_REPOSITORY: return $this->repository->getPHID(); - case HeraldFieldConfig::FIELD_DIFF_CONTENT: + case self::FIELD_DIFF_CONTENT: try { $diff = $this->loadCommitDiff(); } catch (Exception $ex) { @@ -211,28 +234,28 @@ final class HeraldCommitAdapter extends HeraldAdapter { $dict[$change->getFilename()] = implode("\n", $lines); } return $dict; - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE: + case self::FIELD_AFFECTED_PACKAGE: $packages = $this->loadAffectedPackages(); return mpull($packages, 'getPHID'); - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER: + case self::FIELD_AFFECTED_PACKAGE_OWNER: $packages = $this->loadAffectedPackages(); $owners = PhabricatorOwnersOwner::loadAllForPackages($packages); return mpull($owners, 'getUserPHID'); - case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE: + case self::FIELD_NEED_AUDIT_FOR_PACKAGE: return $this->loadAuditNeededPackage(); - case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVISION: + case self::FIELD_DIFFERENTIAL_REVISION: $revision = $this->loadDifferentialRevision(); if (!$revision) { return null; } return $revision->getID(); - case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVIEWERS: + case self::FIELD_DIFFERENTIAL_REVIEWERS: $revision = $this->loadDifferentialRevision(); if (!$revision) { return array(); } return $revision->getReviewers(); - case HeraldFieldConfig::FIELD_DIFFERENTIAL_CCS: + case self::FIELD_DIFFERENTIAL_CCS: $revision = $this->loadDifferentialRevision(); if (!$revision) { return array(); diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 7bc4253e2c..8483da7a38 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -198,42 +198,42 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { public function getHeraldField($field) { switch ($field) { - case HeraldFieldConfig::FIELD_TITLE: + case self::FIELD_TITLE: return $this->revision->getTitle(); break; - case HeraldFieldConfig::FIELD_BODY: + case self::FIELD_BODY: return $this->revision->getSummary()."\n". $this->revision->getTestPlan(); break; - case HeraldFieldConfig::FIELD_AUTHOR: + case self::FIELD_AUTHOR: return $this->revision->getAuthorPHID(); break; - case HeraldFieldConfig::FIELD_DIFF_FILE: + case self::FIELD_DIFF_FILE: return $this->loadAffectedPaths(); - case HeraldFieldConfig::FIELD_CC: + case self::FIELD_CC: if (isset($this->explicitCCs)) { return array_keys($this->explicitCCs); } else { return $this->revision->getCCPHIDs(); } - case HeraldFieldConfig::FIELD_REVIEWERS: + case self::FIELD_REVIEWERS: if (isset($this->explicitReviewers)) { return array_keys($this->explicitReviewers); } else { return $this->revision->getReviewers(); } - case HeraldFieldConfig::FIELD_REPOSITORY: + case self::FIELD_REPOSITORY: $repository = $this->loadRepository(); if (!$repository) { return null; } return $repository->getPHID(); - case HeraldFieldConfig::FIELD_DIFF_CONTENT: + case self::FIELD_DIFF_CONTENT: return $this->loadContentDictionary(); - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE: + case self::FIELD_AFFECTED_PACKAGE: $packages = $this->loadAffectedPackages(); return mpull($packages, 'getPHID'); - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER: + case self::FIELD_AFFECTED_PACKAGE_OWNER: $packages = $this->loadAffectedPackages(); return PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( mpull($packages, 'getID')); diff --git a/src/applications/herald/config/HeraldConditionConfig.php b/src/applications/herald/config/HeraldConditionConfig.php index 240ff3e3c1..4772e51f88 100644 --- a/src/applications/herald/config/HeraldConditionConfig.php +++ b/src/applications/herald/config/HeraldConditionConfig.php @@ -2,6 +2,7 @@ final class HeraldConditionConfig { + // TODO: Remove; still used by Engine/etc. const CONDITION_CONTAINS = 'contains'; const CONDITION_NOT_CONTAINS = '!contains'; const CONDITION_IS = 'is'; @@ -20,6 +21,7 @@ final class HeraldConditionConfig { const CONDITION_NOT_EXISTS = '!exists'; const CONDITION_REGEXP_PAIR = 'regexp-pair'; + // TODO: Remove; still used by Transcripts. public static function getConditionMap() { $map = array( self::CONDITION_CONTAINS => pht('contains'), @@ -44,82 +46,4 @@ final class HeraldConditionConfig { return $map; } - public static function getConditionMapForField($field) { - $map = self::getConditionMap(); - switch ($field) { - case HeraldFieldConfig::FIELD_TITLE: - case HeraldFieldConfig::FIELD_BODY: - return array_select_keys( - $map, - array( - self::CONDITION_CONTAINS, - self::CONDITION_NOT_CONTAINS, - self::CONDITION_IS, - self::CONDITION_IS_NOT, - self::CONDITION_REGEXP, - )); - case HeraldFieldConfig::FIELD_AUTHOR: - case HeraldFieldConfig::FIELD_REPOSITORY: - case HeraldFieldConfig::FIELD_REVIEWER: - return array_select_keys( - $map, - array( - self::CONDITION_IS_ANY, - self::CONDITION_IS_NOT_ANY, - )); - case HeraldFieldConfig::FIELD_TAGS: - case HeraldFieldConfig::FIELD_REVIEWERS: - case HeraldFieldConfig::FIELD_CC: - case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVIEWERS: - case HeraldFieldConfig::FIELD_DIFFERENTIAL_CCS: - return array_select_keys( - $map, - array( - self::CONDITION_INCLUDE_ALL, - self::CONDITION_INCLUDE_ANY, - self::CONDITION_INCLUDE_NONE, - )); - case HeraldFieldConfig::FIELD_DIFF_FILE: - return array_select_keys( - $map, - array( - self::CONDITION_CONTAINS, - self::CONDITION_REGEXP, - )); - case HeraldFieldConfig::FIELD_DIFF_CONTENT: - return array_select_keys( - $map, - array( - self::CONDITION_CONTAINS, - self::CONDITION_REGEXP, - self::CONDITION_REGEXP_PAIR, - )); - case HeraldFieldConfig::FIELD_RULE: - return array_select_keys( - $map, - array( - self::CONDITION_RULE, - self::CONDITION_NOT_RULE, - )); - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE: - case HeraldFieldConfig::FIELD_AFFECTED_PACKAGE_OWNER: - case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE: - return array_select_keys( - $map, - array( - self::CONDITION_INCLUDE_ANY, - self::CONDITION_INCLUDE_NONE, - )); - case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVISION: - return array_select_keys( - $map, - array( - self::CONDITION_EXISTS, - self::CONDITION_NOT_EXISTS, - )); - default: - throw new Exception("Unknown field type '{$field}'."); - } - } - } diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index e29fd3c710..0c160f555e 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -387,18 +387,22 @@ final class HeraldRuleController extends HeraldController { $all_rules = mpull($all_rules, 'getName', 'getID'); asort($all_rules); + $all_fields = $adapter->getFieldNameMap(); + $all_conditions = $adapter->getConditionNameMap(); + $fields = $adapter->getFields(); - $field_map = array_select_keys($adapter->getFieldNameMap(), $fields); + $field_map = array_select_keys($all_fields, $fields); $config_info = array(); $config_info['fields'] = $field_map; - $config_info['conditions'] = HeraldConditionConfig::getConditionMap(); + $config_info['conditions'] = $all_conditions; foreach ($config_info['fields'] as $field => $name) { - $config_info['conditionMap'][$field] = array_keys( - HeraldConditionConfig::getConditionMapForField($field)); + $field_conditions = $adapter->getConditionsForField($field); + $config_info['conditionMap'][$field] = $field_conditions; } + foreach ($config_info['fields'] as $field => $fname) { - foreach ($config_info['conditions'] as $condition => $cname) { + foreach ($config_info['conditionMap'][$field] as $condition) { $config_info['values'][$field][$condition] = HeraldValueTypeConfig::getValueTypeForFieldAndCondition( $field,