From 950a7bbb19e3405698f2e92abf6601e1e7649f8a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 06:10:11 -0800 Subject: [PATCH] On Harbormaster build plans, show which Herald rules trigger builds Summary: Ref T13258. Provide an easy way to find rules which trigger a particular build plan from the build plan page. The implementation here ends up a little messy: we can't just search for `actionType = 'build' AND targetPHID = ''` since the field is a blob of JSON. Instead, make rules indexable and write a "build plan is affected by rule actions" edge when indexing rules, then search on that edge. For now, only "Run Build Plan: ..." rules actually write this edge, since I think (?) that it doesn't really have meaningful values for other edge types today. Maybe "Call Webhooks", and you could get a link from a hook to rules that trigger it? Reasonable to do in the future. Things end up a little bit rough overall, but I think this panel is pretty useful to add to the Build Plan page. This index needs to be rebuilt with `bin/search index --type HeraldRule`. I'll call this out in the changelog but I'm not planning to explicitly migrate or add an activity, since this is only really important for larger installs and they probably (?) read the changelog. As rules are edited over time, this will converge to the right behavior even if you don't rebuild the index. Test Plan: {F6260095} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13258 Differential Revision: https://secure.phabricator.com/D20259 --- src/__phutil_library_map__.php | 7 ++ .../HarbormasterPlanViewController.php | 38 ++++++++ .../HarbormasterRunBuildPlansHeraldAction.php | 5 + .../herald/action/HeraldAction.php | 4 + .../HeraldRuleActionAffectsObjectEdgeType.php | 8 ++ .../herald/editor/HeraldRuleEditor.php | 4 + .../HeraldRuleIndexEngineExtension.php | 92 +++++++++++++++++++ .../herald/query/HeraldRuleQuery.php | 28 ++++++ .../herald/query/HeraldRuleSearchEngine.php | 52 +++-------- .../herald/storage/HeraldRule.php | 1 + .../herald/view/HeraldRuleListView.php | 65 +++++++++++++ 11 files changed, 264 insertions(+), 40 deletions(-) create mode 100644 src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php create mode 100644 src/applications/herald/engineextension/HeraldRuleIndexEngineExtension.php create mode 100644 src/applications/herald/view/HeraldRuleListView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index def6e0de7d..1a5ab46743 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1531,6 +1531,7 @@ phutil_register_library_map(array( 'HeraldRemarkupFieldValue' => 'applications/herald/value/HeraldRemarkupFieldValue.php', 'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', + 'HeraldRuleActionAffectsObjectEdgeType' => 'applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php', 'HeraldRuleAdapter' => 'applications/herald/adapter/HeraldRuleAdapter.php', 'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', @@ -1540,7 +1541,9 @@ phutil_register_library_map(array( 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', + 'HeraldRuleIndexEngineExtension' => 'applications/herald/engineextension/HeraldRuleIndexEngineExtension.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', + 'HeraldRuleListView' => 'applications/herald/view/HeraldRuleListView.php', 'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', @@ -7189,8 +7192,10 @@ phutil_register_library_map(array( 'PhabricatorFlaggableInterface', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorIndexableInterface', 'PhabricatorSubscribableInterface', ), + 'HeraldRuleActionAffectsObjectEdgeType' => 'PhabricatorEdgeType', 'HeraldRuleAdapter' => 'HeraldAdapter', 'HeraldRuleAdapterField' => 'HeraldRuleField', 'HeraldRuleController' => 'HeraldController', @@ -7200,7 +7205,9 @@ phutil_register_library_map(array( 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleField' => 'HeraldField', 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', + 'HeraldRuleIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'HeraldRuleListController' => 'HeraldController', + 'HeraldRuleListView' => 'AphrontView', 'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 141f321caa..731c4fa784 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -62,6 +62,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $builds_view = $this->newBuildsView($plan); $options_view = $this->newOptionsView($plan); + $rules_view = $this->newRulesView($plan); $timeline = $this->buildTransactionTimeline( $plan, @@ -76,6 +77,7 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { $error, $step_list, $options_view, + $rules_view, $builds_view, $timeline, )); @@ -486,6 +488,42 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { ->appendChild($list); } + private function newRulesView(HarbormasterBuildPlan $plan) { + $viewer = $this->getViewer(); + + $rules = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withDisabled(false) + ->withAffectedObjectPHIDs(array($plan->getPHID())) + ->needValidateAuthors(true) + ->execute(); + + $list = id(new HeraldRuleListView()) + ->setViewer($viewer) + ->setRules($rules) + ->newObjectList(); + + $list->setNoDataString(pht('No active Herald rules trigger this build.')); + + $more_href = new PhutilURI( + '/herald/', + array('affectedPHID' => $plan->getPHID())); + + $more_link = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-list-ul') + ->setText(pht('View All Rules')) + ->setHref($more_href); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Run By Herald Rules')) + ->addActionLink($more_link); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($list); + } private function newOptionsView(HarbormasterBuildPlan $plan) { $viewer = $this->getViewer(); diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 8c718e5f5d..9fc053e8ae 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -91,4 +91,9 @@ final class HarbormasterRunBuildPlansHeraldAction 'Run build plans: %s.', $this->renderHandleList($value)); } + + public function getPHIDsAffectedByAction(HeraldActionRecord $record) { + return $record->getTarget(); + } + } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index 04884a94d4..a9740d1736 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -401,4 +401,8 @@ abstract class HeraldAction extends Phobject { return null; } + public function getPHIDsAffectedByAction(HeraldActionRecord $record) { + return array(); + } + } diff --git a/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php b/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php new file mode 100644 index 0000000000..35a30773ac --- /dev/null +++ b/src/applications/herald/edge/HeraldRuleActionAffectsObjectEdgeType.php @@ -0,0 +1,8 @@ +getPHID(), + $edge_type); + $old_edges = array_fuse($old_edges); + + $new_edges = $this->getPHIDsAffectedByActions($object); + $new_edges = array_fuse($new_edges); + + $add_edges = array_diff_key($new_edges, $old_edges); + $rem_edges = array_diff_key($old_edges, $new_edges); + + if (!$add_edges && !$rem_edges) { + return; + } + + $editor = new PhabricatorEdgeEditor(); + + foreach ($add_edges as $phid) { + $editor->addEdge($object->getPHID(), $edge_type, $phid); + } + + foreach ($rem_edges as $phid) { + $editor->removeEdge($object->getPHID(), $edge_type, $phid); + } + + $editor->save(); + } + + public function getIndexVersion($object) { + $phids = $this->getPHIDsAffectedByActions($object); + sort($phids); + $phids = implode(':', $phids); + return PhabricatorHash::digestForIndex($phids); + } + + private function getPHIDsAffectedByActions(HeraldRule $rule) { + $viewer = $this->getViewer(); + + $rule = id(new HeraldRuleQuery()) + ->setViewer($viewer) + ->withIDs(array($rule->getID())) + ->needConditionsAndActions(true) + ->executeOne(); + if (!$rule) { + return array(); + } + + $phids = array(); + + $actions = HeraldAction::getAllActions(); + foreach ($rule->getActions() as $action_record) { + $action = idx($actions, $action_record->getAction()); + + if (!$action) { + continue; + } + + foreach ($action->getPHIDsAffectedByAction($action_record) as $phid) { + $phids[] = $phid; + } + } + + $phids = array_fuse($phids); + return array_keys($phids); + } + +} diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index e6dba43c7a..e346a998d4 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -11,6 +11,7 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $active; private $datasourceQuery; private $triggerObjectPHIDs; + private $affectedObjectPHIDs; private $needConditionsAndActions; private $needAppliedToPHIDs; @@ -61,6 +62,11 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $this; } + public function withAffectedObjectPHIDs(array $phids) { + $this->affectedObjectPHIDs = $phids; + return $this; + } + public function needConditionsAndActions($need) { $this->needConditionsAndActions = $need; return $this; @@ -261,9 +267,31 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { $this->triggerObjectPHIDs); } + if ($this->affectedObjectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'edge_affects.dst IN (%Ls)', + $this->affectedObjectPHIDs); + } + return $where; } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->affectedObjectPHIDs !== null) { + $joins[] = qsprintf( + $conn, + 'JOIN %T edge_affects ON rule.phid = edge_affects.src + AND edge_affects.type = %d', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + HeraldRuleActionAffectsObjectEdgeType::EDGECONST); + } + + return $joins; + } + private function validateRuleAuthors(array $rules) { // "Global" and "Object" rules always have valid authors. foreach ($rules as $key => $rule) { diff --git a/src/applications/herald/query/HeraldRuleSearchEngine.php b/src/applications/herald/query/HeraldRuleSearchEngine.php index 47a6832731..95e3079717 100644 --- a/src/applications/herald/query/HeraldRuleSearchEngine.php +++ b/src/applications/herald/query/HeraldRuleSearchEngine.php @@ -55,6 +55,10 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { pht('(Show All)'), pht('Show Only Disabled Rules'), pht('Show Only Enabled Rules')), + id(new PhabricatorPHIDsSearchField()) + ->setLabel(pht('Affected Objects')) + ->setKey('affectedPHIDs') + ->setAliases(array('affectedPHID')), ); } @@ -81,6 +85,10 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { $query->withActive($map['active']); } + if ($map['affectedPHIDs']) { + $query->withAffectedObjectPHIDs($map['affectedPHIDs']); + } + return $query; } @@ -127,54 +135,18 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { PhabricatorSavedQuery $query, array $handles) { assert_instances_of($rules, 'HeraldRule'); - $viewer = $this->requireViewer(); - $handles = $viewer->loadHandles(mpull($rules, 'getAuthorPHID')); - $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer); - - $list = id(new PHUIObjectItemListView()) - ->setUser($viewer); - foreach ($rules as $rule) { - $monogram = $rule->getMonogram(); - - $item = id(new PHUIObjectItemView()) - ->setObjectName($monogram) - ->setHeader($rule->getName()) - ->setHref("/{$monogram}"); - - if ($rule->isPersonalRule()) { - $item->addIcon('fa-user', pht('Personal Rule')); - $item->addByline( - pht( - 'Authored by %s', - $handles[$rule->getAuthorPHID()]->renderLink())); - } else if ($rule->isObjectRule()) { - $item->addIcon('fa-briefcase', pht('Object Rule')); - } else { - $item->addIcon('fa-globe', pht('Global Rule')); - } - - if ($rule->getIsDisabled()) { - $item->setDisabled(true); - $item->addIcon('fa-lock grey', pht('Disabled')); - } else if (!$rule->hasValidAuthor()) { - $item->setDisabled(true); - $item->addIcon('fa-user grey', pht('Author Not Active')); - } - - $content_type_name = idx($content_type_map, $rule->getContentType()); - $item->addAttribute(pht('Affects: %s', $content_type_name)); - - $list->addItem($item); - } + $list = id(new HeraldRuleListView()) + ->setViewer($viewer) + ->setRules($rules) + ->newObjectList(); $result = new PhabricatorApplicationSearchResultView(); $result->setObjectList($list); $result->setNoDataString(pht('No rules found.')); return $result; - } protected function getNewUserBody() { diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 1b005898cb..a9c131e717 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -6,6 +6,7 @@ final class HeraldRule extends HeraldDAO PhabricatorFlaggableInterface, PhabricatorPolicyInterface, PhabricatorDestructibleInterface, + PhabricatorIndexableInterface, PhabricatorSubscribableInterface { const TABLE_RULE_APPLIED = 'herald_ruleapplied'; diff --git a/src/applications/herald/view/HeraldRuleListView.php b/src/applications/herald/view/HeraldRuleListView.php new file mode 100644 index 0000000000..150499ce87 --- /dev/null +++ b/src/applications/herald/view/HeraldRuleListView.php @@ -0,0 +1,65 @@ +rules = $rules; + return $this; + } + + public function render() { + return $this->newObjectList(); + } + + public function newObjectList() { + $viewer = $this->getViewer(); + $rules = $this->rules; + + $handles = $viewer->loadHandles(mpull($rules, 'getAuthorPHID')); + + $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer); + + $list = id(new PHUIObjectItemListView()) + ->setViewer($viewer); + foreach ($rules as $rule) { + $monogram = $rule->getMonogram(); + + $item = id(new PHUIObjectItemView()) + ->setObjectName($monogram) + ->setHeader($rule->getName()) + ->setHref($rule->getURI()); + + if ($rule->isPersonalRule()) { + $item->addIcon('fa-user', pht('Personal Rule')); + $item->addByline( + pht( + 'Authored by %s', + $handles[$rule->getAuthorPHID()]->renderLink())); + } else if ($rule->isObjectRule()) { + $item->addIcon('fa-briefcase', pht('Object Rule')); + } else { + $item->addIcon('fa-globe', pht('Global Rule')); + } + + if ($rule->getIsDisabled()) { + $item->setDisabled(true); + $item->addIcon('fa-lock grey', pht('Disabled')); + } else if (!$rule->hasValidAuthor()) { + $item->setDisabled(true); + $item->addIcon('fa-user grey', pht('Author Not Active')); + } + + $content_type_name = idx($content_type_map, $rule->getContentType()); + $item->addAttribute(pht('Affects: %s', $content_type_name)); + + $list->addItem($item); + } + + return $list; + } + +}