From 5bfd6bda773321d44a94b4fecf31537c910b38db Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Nov 2020 13:55:15 -0800 Subject: [PATCH] Provide a more structured result log for Herald rules Summary: Ref T13586. In the footsteps of D21563, make Herald rule results more formal and structured to support meaningful exception reporting. Test Plan: Ran various Herald rules and viewed transcripts, including rules with recursive dependencies and condition exceptions. {F8447894} Maniphest Tasks: T13586 Differential Revision: https://secure.phabricator.com/D21565 --- src/__phutil_library_map__.php | 4 + .../controller/HeraldTranscriptController.php | 48 ++-- .../herald/engine/HeraldEngine.php | 270 ++++++++++++------ .../HeraldRuleEvaluationException.php | 3 + .../transcript/HeraldConditionResult.php | 11 +- .../storage/transcript/HeraldRuleResult.php | 238 +++++++++++++++ .../transcript/HeraldRuleTranscript.php | 69 +++-- .../transcript/HeraldTranscriptResult.php | 13 +- 8 files changed, 508 insertions(+), 148 deletions(-) create mode 100644 src/applications/herald/engine/exception/HeraldRuleEvaluationException.php create mode 100644 src/applications/herald/storage/transcript/HeraldRuleResult.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9c7c1a85a4..f3304ce202 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1621,6 +1621,7 @@ phutil_register_library_map(array( 'HeraldRuleDisableTransaction' => 'applications/herald/xaction/HeraldRuleDisableTransaction.php', 'HeraldRuleEditTransaction' => 'applications/herald/xaction/HeraldRuleEditTransaction.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', + 'HeraldRuleEvaluationException' => 'applications/herald/engine/exception/HeraldRuleEvaluationException.php', 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', 'HeraldRuleIndexEngineExtension' => 'applications/herald/engineextension/HeraldRuleIndexEngineExtension.php', @@ -1631,6 +1632,7 @@ phutil_register_library_map(array( 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', 'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php', + 'HeraldRuleResult' => 'applications/herald/storage/transcript/HeraldRuleResult.php', 'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php', 'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php', 'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php', @@ -7861,6 +7863,7 @@ phutil_register_library_map(array( 'HeraldRuleDisableTransaction' => 'HeraldRuleTransactionType', 'HeraldRuleEditTransaction' => 'HeraldRuleTransactionType', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', + 'HeraldRuleEvaluationException' => 'Exception', 'HeraldRuleField' => 'HeraldField', 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', 'HeraldRuleIndexEngineExtension' => 'PhabricatorEdgeIndexEngineExtension', @@ -7871,6 +7874,7 @@ phutil_register_library_map(array( 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', + 'HeraldRuleResult' => 'HeraldTranscriptResult', 'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HeraldRuleSerializer' => 'Phobject', 'HeraldRuleTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index a7498abdb7..dfc0dfc0b1 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -224,6 +224,7 @@ final class HeraldTranscriptController extends HeraldController { } private function buildActionTranscriptPanel(HeraldTranscript $xscript) { + $viewer = $this->getViewer(); $action_xscript = mgroup($xscript->getApplyTranscripts(), 'getRuleID'); $adapter = $this->getAdapter(); @@ -253,7 +254,9 @@ final class HeraldTranscriptController extends HeraldController { ->setHeader($rule_xscript->getRuleName()) ->setHref($rule_uri); - if (!$rule_xscript->getResult()) { + $rule_result = $rule_xscript->getRuleResult(); + + if (!$rule_result->getShouldApplyActions()) { $rule_item->setDisabled(true); } @@ -275,7 +278,7 @@ final class HeraldTranscriptController extends HeraldController { $color = $result->getIconColor(); $name = $result->getName(); - $result_details = $result->newDetailsView(); + $result_details = $result->newDetailsView($viewer); if ($result_details !== null) { $result_details = phutil_tag( 'div', @@ -301,27 +304,27 @@ final class HeraldTranscriptController extends HeraldController { $cond_list->addItem($cond_item); } - if ($rule_xscript->isForbidden()) { - $last_icon = 'fa-ban'; - $last_color = 'indigo'; - $last_result = pht('Forbidden'); - $last_note = pht('Object state prevented rule evaluation.'); - } else if ($rule_xscript->getResult()) { - $last_icon = 'fa-check-circle'; - $last_color = 'green'; - $last_result = pht('Passed'); - $last_note = pht('Rule passed.'); - } else { - $last_icon = 'fa-times-circle'; - $last_color = 'red'; - $last_result = pht('Failed'); - $last_note = pht('Rule failed.'); + $rule_result = $rule_xscript->getRuleResult(); + + $last_icon = $rule_result->getIconIcon(); + $last_color = $rule_result->getIconColor(); + $last_result = $rule_result->getName(); + $last_note = $rule_result->getDescription(); + + $last_details = $rule_result->newDetailsView($viewer); + if ($last_details !== null) { + $last_details = phutil_tag( + 'div', + array( + 'class' => 'herald-condition-note', + ), + $last_details); } $cond_last = id(new PHUIStatusItemView()) ->setIcon($last_icon, $last_color) ->setTarget(phutil_tag('strong', array(), $last_result)) - ->setNote($last_note); + ->setNote(array($last_note, $last_details)); $cond_list->addItem($cond_last); $cond_box = id(new PHUIBoxView()) @@ -330,11 +333,10 @@ final class HeraldTranscriptController extends HeraldController { $rule_item->appendChild($cond_box); - if (!$rule_xscript->getResult()) { - // If the rule didn't pass, don't generate an action transcript since - // actions didn't apply. - continue; - } + // Not all rules will have any action transcripts, but we show them + // in general because they may have relevant information even when + // rules did not take actions. In particular, state-based actions may + // forbid rules from matching. $cond_box->addMargin(PHUI::MARGIN_MEDIUM_BOTTOM); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index 78a3c6c695..91a2050779 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -3,8 +3,6 @@ final class HeraldEngine extends Phobject { protected $rules = array(); - protected $results = array(); - protected $stack = array(); protected $activeRule; protected $transcript; @@ -20,6 +18,9 @@ final class HeraldEngine extends Phobject { private $profilerStack = array(); private $profilerFrames = array(); + private $ruleResults; + private $ruleStack; + public function setDryRun($dry_run) { $this->dryRun = $dry_run; return $this; @@ -54,6 +55,74 @@ final class HeraldEngine extends Phobject { return $engine->getTranscript(); } +/* -( Rule Stack )--------------------------------------------------------- */ + + private function resetRuleStack() { + $this->ruleStack = array(); + return $this; + } + + private function hasRuleOnStack(HeraldRule $rule) { + $phid = $rule->getPHID(); + return isset($this->ruleStack[$phid]); + } + + private function pushRuleStack(HeraldRule $rule) { + $phid = $rule->getPHID(); + $this->ruleStack[$phid] = $rule; + return $this; + } + + private function getRuleStack() { + return array_values($this->ruleStack); + } + +/* -( Rule Results )------------------------------------------------------- */ + + private function resetRuleResults() { + $this->ruleResults = array(); + return $this; + } + + private function setRuleResult( + HeraldRule $rule, + HeraldRuleResult $result) { + + $phid = $rule->getPHID(); + + if ($this->hasRuleResult($rule)) { + throw new Exception( + pht( + 'Herald rule "%s" already has an evaluation result.', + $phid)); + } + + $this->ruleResults[$phid] = $result; + + $this->newRuleTranscript($rule) + ->setRuleResult($result); + + return $this; + } + + private function hasRuleResult(HeraldRule $rule) { + $phid = $rule->getPHID(); + return isset($this->ruleResults[$phid]); + } + + private function getRuleResult(HeraldRule $rule) { + $phid = $rule->getPHID(); + + if (!$this->hasRuleResult($rule)) { + throw new Exception( + pht( + 'Herald rule "%s" does not have an evaluation result.', + $phid)); + } + + return $this->ruleResults[$phid]; + } + public function applyRules(array $rules, HeraldAdapter $object) { assert_instances_of($rules, 'HeraldRule'); $t_start = microtime(true); @@ -66,62 +135,70 @@ final class HeraldEngine extends Phobject { $this->transcript->setObjectPHID((string)$object->getPHID()); $this->fieldCache = array(); $this->fieldExceptions = array(); - $this->results = array(); $this->rules = $rules; $this->object = $object; + $this->resetRuleResults(); + $effects = array(); foreach ($rules as $phid => $rule) { - $this->stack = array(); - - $is_first_only = $rule->isRepeatFirst(); + $this->resetRuleStack(); $caught = null; + $result = null; try { + $is_first_only = $rule->isRepeatFirst(); + if (!$this->getDryRun() && $is_first_only && $rule->getRuleApplied($object->getPHID())) { + // This is not a dry run, and this rule is only supposed to be - // applied a single time, and it's already been applied... + // applied a single time, and it has already been applied. // That means automatic failure. - $this->newRuleTranscript($rule) - ->setResult(false) - ->setReason( - pht( - 'This rule is only supposed to be repeated a single time, '. - 'and it has already been applied.')); - $rule_matches = false; + $result_code = HeraldRuleResult::RESULT_ALREADY_APPLIED; + $result = HeraldRuleResult::newFromResultCode($result_code); + } else if ($this->isForbidden($rule, $object)) { + $result_code = HeraldRuleResult::RESULT_OBJECT_STATE; + $result = HeraldRuleResult::newFromResultCode($result_code); } else { - if ($this->isForbidden($rule, $object)) { - $this->newRuleTranscript($rule) - ->setResult(HeraldRuleTranscript::RESULT_FORBIDDEN) - ->setReason( - pht( - 'Object state is not compatible with rule.')); - - $rule_matches = false; - } else { - $rule_matches = $this->doesRuleMatch($rule, $object); - } + $result = $this->getRuleMatchResult($rule, $object); } } catch (HeraldRecursiveConditionsException $ex) { - $names = array(); - foreach ($this->stack as $rule_phid => $ignored) { - $names[] = '"'.$rules[$rule_phid]->getName().'"'; + $cycle_phids = array(); + + $stack = $this->getRuleStack(); + foreach ($stack as $stack_rule) { + $cycle_phids[] = $stack_rule->getPHID(); } - $names = implode(', ', $names); - foreach ($this->stack as $rule_phid => $ignored) { - $this->newRuleTranscript($rules[$rule_phid]) - ->setResult(false) - ->setReason( - pht( - "Rules %s are recursively dependent upon one another! ". - "Don't do this! You have formed an unresolvable cycle in the ". - "dependency graph!", - $names)); + // Add the rule which actually cycled to the list to make the + // result more clear when we show it to the user. + $cycle_phids[] = $phid; + + foreach ($stack as $stack_rule) { + if ($this->hasRuleResult($stack_rule)) { + continue; + } + + $result_code = HeraldRuleResult::RESULT_RECURSION; + $result_data = array( + 'cyclePHIDs' => $cycle_phids, + ); + + $result = HeraldRuleResult::newFromResultCode($result_code) + ->setResultData($result_data); + $this->setRuleResult($stack_rule, $result); } - $rule_matches = false; + + $result = $this->getRuleResult($rule); + } catch (HeraldRuleEvaluationException $ex) { + // When we encounter an evaluation exception, the condition which + // failed to evaluate is responsible for logging the details of the + // error. + + $result_code = HeraldRuleResult::RESULT_EVALUATION_EXCEPTION; + $result = HeraldRuleResult::newFromResultCode($result_code); } catch (Exception $ex) { $caught = $ex; } catch (Throwable $ex) { @@ -129,17 +206,26 @@ final class HeraldEngine extends Phobject { } if ($caught) { - $this->newRuleTranscript($rules[$phid]) - ->setResult(false) - ->setReason( - pht( - 'Rule encountered an exception while evaluting.')); - $rule_matches = false; + // These exceptions are unexpected, and did not arise during rule + // evaluation, so we're responsible for handling the details. + + $result_code = HeraldRuleResult::RESULT_EXCEPTION; + + $result_data = array( + 'exception.class' => get_class($caught), + 'exception.message' => $ex->getMessage(), + ); + + $result = HeraldRuleResult::newFromResultCode($result_code) + ->setResultData($result_data); } - $this->results[$phid] = $rule_matches; + if (!$this->hasRuleResult($rule)) { + $this->setRuleResult($rule, $result); + } + $result = $this->getRuleResult($rule); - if ($rule_matches) { + if ($result->getShouldApplyActions()) { foreach ($this->getRuleEffects($rule, $object) as $effect) { $effects[] = $effect; } @@ -286,58 +372,46 @@ final class HeraldEngine extends Phobject { public function doesRuleMatch( HeraldRule $rule, HeraldAdapter $object) { + $result = $this->getRuleMatchResult($rule, $object); + return $result->getShouldApplyActions(); + } - $phid = $rule->getPHID(); + private function getRuleMatchResult( + HeraldRule $rule, + HeraldAdapter $object) { - if (isset($this->results[$phid])) { + if ($this->hasRuleResult($rule)) { // If we've already evaluated this rule because another rule depends // on it, we don't need to reevaluate it. - return $this->results[$phid]; + return $this->getRuleResult($rule); } - if (isset($this->stack[$phid])) { + if ($this->hasRuleOnStack($rule)) { // We've recursed, fail all of the rules on the stack. This happens when // there's a dependency cycle with "Rule conditions match for rule ..." // conditions. - foreach ($this->stack as $rule_phid => $ignored) { - $this->results[$rule_phid] = false; - } throw new HeraldRecursiveConditionsException(); } - - $this->stack[$phid] = true; + $this->pushRuleStack($rule); $all = $rule->getMustMatchAll(); $conditions = $rule->getConditions(); - $result = null; + $result_code = null; + $result_data = array(); $local_version = id(new HeraldRule())->getConfigVersion(); if ($rule->getConfigVersion() > $local_version) { - $reason = pht( - 'Rule could not be processed, it was created with a newer version '. - 'of Herald.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_VERSION; } else if (!$conditions) { - $reason = pht( - 'Rule failed automatically because it has no conditions.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_EMPTY; } else if (!$rule->hasValidAuthor()) { - $reason = pht( - 'Rule failed automatically because its owner is invalid '. - 'or disabled.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_OWNER; } else if (!$this->canAuthorViewObject($rule, $object)) { - $reason = pht( - 'Rule failed automatically because it is a personal rule and its '. - 'owner can not see the object.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_VIEW_POLICY; } else if (!$this->canRuleApplyToObject($rule, $object)) { - $reason = pht( - 'Rule failed automatically because it is an object rule which is '. - 'not relevant for this object.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_OBJECT_RULE; } else { foreach ($conditions as $condition) { $caught = null; @@ -347,6 +421,10 @@ final class HeraldEngine extends Phobject { $rule, $condition, $object); + } catch (HeraldRuleEvaluationException $ex) { + throw $ex; + } catch (HeraldRecursiveConditionsException $ex) { + throw $ex; } catch (Exception $ex) { $caught = $ex; } catch (Throwable $ex) { @@ -354,60 +432,59 @@ final class HeraldEngine extends Phobject { } if ($caught) { - throw $ex; + throw new HeraldRuleEvaluationException(); } if (!$all && $match) { - $reason = pht('Any condition matched.'); - $result = true; + $result_code = HeraldRuleResult::RESULT_ANY_MATCHED; break; } if ($all && !$match) { - $reason = pht('Not all conditions matched.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_ANY_FAILED; break; } } - if ($result === null) { + if ($result_code === null) { if ($all) { - $reason = pht('All conditions matched.'); - $result = true; + $result_code = HeraldRuleResult::RESULT_ALL_MATCHED; } else { - $reason = pht('No conditions matched.'); - $result = false; + $result_code = HeraldRuleResult::RESULT_ALL_FAILED; } } } // If this rule matched, and is set to run "if it did not match the last - // time", and we matched the last time, we're going to return a match in - // the transcript but set a flag so we don't actually apply any effects. + // time", and we matched the last time, we're going to return a special + // result code which records a match but doesn't actually apply effects. // We need the rule to match so that storage gets updated properly. If we // just pretend the rule didn't match it won't cause any effects (which // is correct), but it also won't set the "it matched" flag in storage, // so the next run after this one would incorrectly trigger again. + $result = HeraldRuleResult::newFromResultCode($result_code) + ->setResultData($result_data); + + $should_apply = $result->getShouldApplyActions(); + $is_dry_run = $this->getDryRun(); - if ($result && !$is_dry_run) { + if ($should_apply && !$is_dry_run) { $is_on_change = $rule->isRepeatOnChange(); if ($is_on_change) { $did_apply = $rule->getRuleApplied($object->getPHID()); if ($did_apply) { - $reason = pht( - 'This rule matched, but did not take any actions because it '. - 'is configured to act only if it did not match the last time.'); + // Replace the result with our modified result. + $result_code = HeraldRuleResult::RESULT_LAST_MATCHED; + $result = HeraldRuleResult::newFromResultCode($result_code); $this->skipEffects[$rule->getID()] = true; } } } - $this->newRuleTranscript($rule) - ->setResult($result) - ->setReason($reason); + $this->setRuleResult($rule, $result); return $result; } @@ -439,6 +516,9 @@ final class HeraldEngine extends Phobject { } else { $result_code = HeraldConditionResult::RESULT_FAILED; } + } catch (HeraldRecursiveConditionsException $ex) { + $result_code = HeraldConditionResult::RESULT_RECURSION; + $caught = $ex; } catch (HeraldInvalidConditionException $ex) { $result_code = HeraldConditionResult::RESULT_INVALID; $caught = $ex; diff --git a/src/applications/herald/engine/exception/HeraldRuleEvaluationException.php b/src/applications/herald/engine/exception/HeraldRuleEvaluationException.php new file mode 100644 index 0000000000..5335cb967c --- /dev/null +++ b/src/applications/herald/engine/exception/HeraldRuleEvaluationException.php @@ -0,0 +1,3 @@ +getSpecificationProperty('match') === true); } - public function newDetailsView() { + public function newDetailsView(PhabricatorUser $viewer) { switch ($this->getResultCode()) { case self::RESULT_OBJECT_STATE: $reason = $this->getDataProperty('reason'); @@ -54,8 +55,6 @@ final class HeraldConditionResult phutil_tag('strong', array(), $error_class), phutil_escape_html_newlines($error_message)); break; - $details = 'exception'; - break; default: $details = null; break; @@ -90,6 +89,12 @@ final class HeraldConditionResult 'color.icon' => 'yellow', 'name' => pht('Invalid'), ), + self::RESULT_RECURSION => array( + 'match' => null, + 'icon' => 'fa-exclamation-triangle', + 'color.icon' => 'red', + 'name' => pht('Recursion'), + ), self::RESULT_EXCEPTION => array( 'match' => null, 'icon' => 'fa-exclamation-triangle', diff --git a/src/applications/herald/storage/transcript/HeraldRuleResult.php b/src/applications/herald/storage/transcript/HeraldRuleResult.php new file mode 100644 index 0000000000..e7fc39bfd0 --- /dev/null +++ b/src/applications/herald/storage/transcript/HeraldRuleResult.php @@ -0,0 +1,238 @@ +setResultCode($result_code); + } + + public static function newFromResultMap(array $map) { + return id(new self())->loadFromResultMap($map); + } + + public function getShouldRecordMatch() { + return ($this->getSpecificationProperty('match') === true); + } + + public function getShouldApplyActions() { + return ($this->getSpecificationProperty('apply') === true); + } + + public function getDescription() { + return $this->getSpecificationProperty('description'); + } + + public function newDetailsView(PhabricatorUser $viewer) { + switch ($this->getResultCode()) { + case self::RESULT_EXCEPTION: + $error_class = $this->getDataProperty('exception.class'); + $error_message = $this->getDataProperty('exception.message'); + + if (!strlen($error_class)) { + $error_class = pht('Unknown Error'); + } + + if (!strlen($error_message)) { + $error_message = pht( + 'An unknown error occurred while evaluating this condition. No '. + 'additional information is available.'); + } + + $details = $this->newErrorView($error_class, $error_message); + break; + case self::RESULT_RECURSION: + $rule_phids = $this->getDataProperty('cyclePHIDs', array()); + $handles = $viewer->loadHandles($rule_phids); + + $links = array(); + foreach ($rule_phids as $rule_phid) { + $links[] = $handles[$rule_phid]->renderLink(); + } + + $links = phutil_implode_html(' > ', $links); + + $details = array( + pht('This rule has a dependency cycle and can not be evaluated:'), + ' ', + $links, + ); + break; + default: + $details = null; + break; + } + + return $details; + } + + protected function newResultSpecificationMap() { + return array( + self::RESULT_ANY_MATCHED => array( + 'match' => true, + 'apply' => true, + 'name' => pht('Matched'), + 'description' => pht('Any condition matched.'), + 'icon' => 'fa-check-circle', + 'color.icon' => 'green', + ), + self::RESULT_ALL_MATCHED => array( + 'match' => true, + 'apply' => true, + 'name' => pht('Matched'), + 'description' => pht('All conditions matched.'), + 'icon' => 'fa-check-circle', + 'color.icon' => 'green', + ), + self::RESULT_ANY_FAILED => array( + 'match' => false, + 'apply' => false, + 'name' => pht('Failed'), + 'description' => pht('Not all conditions matched.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_ALL_FAILED => array( + 'match' => false, + 'apply' => false, + 'name' => pht('Failed'), + 'description' => pht('No conditions matched.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_LAST_MATCHED => array( + 'match' => true, + 'apply' => false, + 'name' => pht('Failed'), + 'description' => pht( + 'This rule matched, but did not take any actions because it '. + 'is configured to act only if it did not match the last time.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_VERSION => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Version Issue'), + 'description' => pht( + 'Rule could not be processed because it was created with a newer '. + 'version of Herald.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_EMPTY => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Empty'), + 'description' => pht( + 'Rule failed automatically because it has no conditions.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_OWNER => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Rule Owner'), + 'description' => pht( + 'Rule failed automatically because it is a personal rule and '. + 'its owner is invalid or disabled.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_VIEW_POLICY => array( + 'match' => null, + 'apply' => false, + 'name' => pht('View Policy'), + 'description' => pht( + 'Rule failed automatically because it is a personal rule and '. + 'its owner does not have permission to view the object.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_OBJECT_RULE => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Object Rule'), + 'description' => pht( + 'Rule failed automatically because it is an object rule which is '. + 'not relevant for this object.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_EXCEPTION => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Exception'), + 'description' => pht( + 'Rule failed because an exception occurred.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_EVALUATION_EXCEPTION => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Exception'), + 'description' => pht( + 'Rule failed because an exception occurred while evaluating it.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_UNKNOWN => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Unknown'), + 'description' => pht( + 'Rule evaluation result is unknown.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_ALREADY_APPLIED => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Already Applied'), + 'description' => pht( + 'This rule is only supposed to be repeated a single time, '. + 'and it has already been applied.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + self::RESULT_OBJECT_STATE => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Forbidden'), + 'description' => pht( + 'Object state prevented rule evaluation.'), + 'icon' => 'fa-ban', + 'color.icon' => 'indigo', + ), + self::RESULT_RECURSION => array( + 'match' => null, + 'apply' => false, + 'name' => pht('Recursion'), + 'description' => pht( + 'This rule has a recursive dependency on itself and can not '. + 'be evaluated.'), + 'icon' => 'fa-times-circle', + 'color.icon' => 'red', + ), + ); + } + +} diff --git a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php index 8928a74cf6..7410d4bd7e 100644 --- a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php @@ -3,35 +3,18 @@ final class HeraldRuleTranscript extends Phobject { protected $ruleID; - protected $result; - protected $reason; - + protected $ruleResultMap; protected $ruleName; protected $ruleOwner; - const RESULT_FORBIDDEN = 'forbidden'; + // See T13586. This no longer has readers, but was written by older versions + // of Herald. It contained a human readable English-language description of + // the outcome of rule evaluation and was superseded by "HeraldRuleResult". + protected $reason; - public function isForbidden() { - return ($this->getResult() === self::RESULT_FORBIDDEN); - } - - public function setResult($result) { - $this->result = $result; - return $this; - } - - public function getResult() { - return $this->result; - } - - public function setReason($reason) { - $this->reason = $reason; - return $this; - } - - public function getReason() { - return $this->reason; - } + // See T13586. Older transcripts store a boolean "true", a boolean "false", + // or the string "forbidden" here. + protected $result; public function setRuleID($rule_id) { $this->ruleID = $rule_id; @@ -59,4 +42,40 @@ final class HeraldRuleTranscript extends Phobject { public function getRuleOwner() { return $this->ruleOwner; } + + public function setRuleResult(HeraldRuleResult $result) { + $this->ruleResultMap = $result->newResultMap(); + return $this; + } + + public function getRuleResult() { + $map = $this->ruleResultMap; + + if (is_array($map)) { + $result = HeraldRuleResult::newFromResultMap($map); + } else { + $legacy_result = $this->result; + + $result_data = array(); + + if ($legacy_result === 'forbidden') { + $result_code = HeraldRuleResult::RESULT_OBJECT_STATE; + $result_data = array( + 'reason' => $this->reason, + ); + } else if ($legacy_result === true) { + $result_code = HeraldRuleResult::RESULT_ANY_MATCHED; + } else if ($legacy_result === false) { + $result_code = HeraldRuleResult::RESULT_ANY_FAILED; + } else { + $result_code = HeraldRuleResult::RESULT_UNKNOWN; + } + + $result = HeraldRuleResult::newFromResultCode($result_code) + ->setResultData($result_data); + } + + return $result; + } + } diff --git a/src/applications/herald/storage/transcript/HeraldTranscriptResult.php b/src/applications/herald/storage/transcript/HeraldTranscriptResult.php index 1e5887c533..f53bc574e0 100644 --- a/src/applications/herald/storage/transcript/HeraldTranscriptResult.php +++ b/src/applications/herald/storage/transcript/HeraldTranscriptResult.php @@ -47,9 +47,11 @@ abstract class HeraldTranscriptResult return $this->getSpecificationProperty('name'); } - final protected function getDataProperty($key) { + abstract public function newDetailsView(PhabricatorUser $viewer); + + final protected function getDataProperty($key, $default = null) { $data = $this->getResultData(); - return idx($data, $key); + return idx($data, $key, $default); } final public function newResultMap() { @@ -79,4 +81,11 @@ abstract class HeraldTranscriptResult abstract protected function newResultSpecificationMap(); + final protected function newErrorView($error_class, $error_message) { + return pht( + '%s: %s', + phutil_tag('strong', array(), $error_class), + phutil_escape_html_newlines($error_message)); + } + }