diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 91d5a3cc7b..2040568496 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -458,6 +458,7 @@ phutil_register_library_map(array( 'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php', 'DifferentialGitSVNIDCommitMessageField' => 'applications/differential/field/DifferentialGitSVNIDCommitMessageField.php', 'DifferentialHarbormasterField' => 'applications/differential/customfield/DifferentialHarbormasterField.php', + 'DifferentialHeraldStateReasons' => 'applications/differential/herald/DifferentialHeraldStateReasons.php', 'DifferentialHiddenComment' => 'applications/differential/storage/DifferentialHiddenComment.php', 'DifferentialHostField' => 'applications/differential/customfield/DifferentialHostField.php', 'DifferentialHovercardEngineExtension' => 'applications/differential/engineextension/DifferentialHovercardEngineExtension.php', @@ -1328,6 +1329,7 @@ phutil_register_library_map(array( 'HeraldApplicationActionGroup' => 'applications/herald/action/HeraldApplicationActionGroup.php', 'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php', 'HeraldBasicFieldGroup' => 'applications/herald/field/HeraldBasicFieldGroup.php', + 'HeraldBuildableState' => 'applications/herald/state/HeraldBuildableState.php', 'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php', 'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php', 'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php', @@ -1351,6 +1353,7 @@ phutil_register_library_map(array( 'HeraldGroup' => 'applications/herald/group/HeraldGroup.php', 'HeraldInvalidActionException' => 'applications/herald/engine/exception/HeraldInvalidActionException.php', 'HeraldInvalidConditionException' => 'applications/herald/engine/exception/HeraldInvalidConditionException.php', + 'HeraldMailableState' => 'applications/herald/state/HeraldMailableState.php', 'HeraldManageGlobalRulesCapability' => 'applications/herald/capability/HeraldManageGlobalRulesCapability.php', 'HeraldManiphestTaskAdapter' => 'applications/maniphest/herald/HeraldManiphestTaskAdapter.php', 'HeraldNewController' => 'applications/herald/controller/HeraldNewController.php', @@ -1388,6 +1391,8 @@ phutil_register_library_map(array( 'HeraldSchemaSpec' => 'applications/herald/storage/HeraldSchemaSpec.php', 'HeraldSelectFieldValue' => 'applications/herald/value/HeraldSelectFieldValue.php', 'HeraldSpaceField' => 'applications/spaces/herald/HeraldSpaceField.php', + 'HeraldState' => 'applications/herald/state/HeraldState.php', + 'HeraldStateReasons' => 'applications/herald/state/HeraldStateReasons.php', 'HeraldSubscribersField' => 'applications/subscriptions/herald/HeraldSubscribersField.php', 'HeraldSupportActionGroup' => 'applications/herald/action/HeraldSupportActionGroup.php', 'HeraldSupportFieldGroup' => 'applications/herald/field/HeraldSupportFieldGroup.php', @@ -5467,6 +5472,7 @@ phutil_register_library_map(array( 'DifferentialGetWorkingCopy' => 'Phobject', 'DifferentialGitSVNIDCommitMessageField' => 'DifferentialCommitMessageField', 'DifferentialHarbormasterField' => 'DifferentialCustomField', + 'DifferentialHeraldStateReasons' => 'HeraldStateReasons', 'DifferentialHiddenComment' => 'DifferentialDAO', 'DifferentialHostField' => 'DifferentialCustomField', 'DifferentialHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', @@ -6458,6 +6464,7 @@ phutil_register_library_map(array( 'HeraldApplicationActionGroup' => 'HeraldActionGroup', 'HeraldApplyTranscript' => 'Phobject', 'HeraldBasicFieldGroup' => 'HeraldFieldGroup', + 'HeraldBuildableState' => 'HeraldState', 'HeraldCommitAdapter' => array( 'HeraldAdapter', 'HarbormasterBuildableAdapterInterface', @@ -6487,6 +6494,7 @@ phutil_register_library_map(array( 'HeraldGroup' => 'Phobject', 'HeraldInvalidActionException' => 'Exception', 'HeraldInvalidConditionException' => 'Exception', + 'HeraldMailableState' => 'HeraldState', 'HeraldManageGlobalRulesCapability' => 'PhabricatorPolicyCapability', 'HeraldManiphestTaskAdapter' => 'HeraldAdapter', 'HeraldNewController' => 'HeraldController', @@ -6531,6 +6539,8 @@ phutil_register_library_map(array( 'HeraldSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'HeraldSelectFieldValue' => 'HeraldFieldValue', 'HeraldSpaceField' => 'HeraldField', + 'HeraldState' => 'Phobject', + 'HeraldStateReasons' => 'Phobject', 'HeraldSubscribersField' => 'HeraldField', 'HeraldSupportActionGroup' => 'HeraldActionGroup', 'HeraldSupportFieldGroup' => 'HeraldFieldGroup', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 5e013eff43..9ab30c3f06 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1003,26 +1003,7 @@ final class DifferentialTransactionEditor protected function shouldApplyHeraldRules( PhabricatorLiskDAO $object, array $xactions) { - - if ($this->getIsNewObject()) { - return true; - } - - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: - if (!$this->getIsCloseByCommit()) { - return true; - } - break; - case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: - // When users commandeer revisions, we may need to trigger - // signatures or author-based rules. - return true; - } - } - - return parent::shouldApplyHeraldRules($object, $xactions); + return true; } protected function didApplyHeraldRules( @@ -1211,6 +1192,33 @@ final class DifferentialTransactionEditor $revision, $revision->getActiveDiff()); + // If the object is still a draft, prevent "Send me an email" and other + // similar rules from acting yet. + if (!$object->shouldBroadcast()) { + $adapter->setForbiddenAction( + HeraldMailableState::STATECONST, + DifferentialHeraldStateReasons::REASON_DRAFT); + } + + // If this edit didn't actually change the diff (for example, a user + // edited the title or changed subscribers), prevent "Run build plan" + // and other similar rules from acting yet, since the build results will + // not (or, at least, should not) change unless the actual source changes. + $has_update = false; + $type_update = DifferentialTransaction::TYPE_UPDATE; + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() == $type_update) { + $has_update = true; + break; + } + } + + if (!$has_update) { + $adapter->setForbiddenAction( + HeraldBuildableState::STATECONST, + DifferentialHeraldStateReasons::REASON_UNCHANGED); + } + return $adapter; } diff --git a/src/applications/differential/herald/DifferentialHeraldStateReasons.php b/src/applications/differential/herald/DifferentialHeraldStateReasons.php new file mode 100644 index 0000000000..d3259560d5 --- /dev/null +++ b/src/applications/differential/herald/DifferentialHeraldStateReasons.php @@ -0,0 +1,22 @@ + pht( + 'This revision is still an unsubmitted draft, so mail will not '. + 'be sent yet.'), + self::REASON_UNCHANGED => pht( + 'The update which triggered Herald did not update the diff for '. + 'this revision, so builds will not run.'), + ); + + return idx($reasons, $reason); + } + +} diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 76bdf9ccc3..8c718e5f5d 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -7,6 +7,12 @@ final class HarbormasterRunBuildPlansHeraldAction const ACTIONCONST = 'harbormaster.build'; + public function getRequiredAdapterStates() { + return array( + HeraldBuildableState::STATECONST, + ); + } + public function getActionGroupKey() { return HeraldSupportActionGroup::ACTIONGROUPKEY; } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index 091f3e9bc2..a2d589f9f2 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -17,6 +17,7 @@ abstract class HeraldAction extends Phobject { const DO_STANDARD_PERMISSION = 'do.standard.permission'; const DO_STANDARD_INVALID_ACTION = 'do.standard.invalid-action'; const DO_STANDARD_WRONG_RULE_TYPE = 'do.standard.wrong-rule-type'; + const DO_STANDARD_FORBIDDEN = 'do.standard.forbidden'; abstract public function getHeraldActionName(); abstract public function supportsObject($object); @@ -25,6 +26,10 @@ abstract class HeraldAction extends Phobject { abstract public function renderActionDescription($value); + public function getRequiredAdapterStates() { + return array(); + } + protected function renderActionEffectDescription($type, $data) { return null; } @@ -336,6 +341,11 @@ abstract class HeraldAction extends Phobject { 'color' => 'red', 'name' => pht('Wrong Rule Type'), ), + self::DO_STANDARD_FORBIDDEN => array( + 'icon' => 'fa-ban', + 'color' => 'violet', + 'name' => pht('Forbidden'), + ), ); } @@ -381,6 +391,8 @@ abstract class HeraldAction extends Phobject { return pht( 'This action does not support rules of type "%s".', $data); + case self::DO_STANDARD_FORBIDDEN: + return HeraldStateReasons::getExplanation($data); } return null; diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 78ce862945..1c50c0b5ee 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -37,6 +37,7 @@ abstract class HeraldAdapter extends Phobject { private $fieldMap; private $actionMap; private $edgeCache = array(); + private $forbiddenActions = array(); public function getEmailPHIDs() { return array_values($this->emailPHIDs); @@ -1116,4 +1117,38 @@ abstract class HeraldAdapter extends Phobject { return $this->edgeCache[$type]; } + +/* -( Forbidden Actions )-------------------------------------------------- */ + + + final public function getForbiddenActions() { + return array_keys($this->forbiddenActions); + } + + final public function setForbiddenAction($action, $reason) { + $this->forbiddenActions[$action] = $reason; + return $this; + } + + final public function getRequiredFieldStates($field_key) { + return $this->requireFieldImplementation($field_key) + ->getRequiredAdapterStates(); + } + + final public function getRequiredActionStates($action_key) { + return $this->requireActionImplementation($action_key) + ->getRequiredAdapterStates(); + } + + final public function getForbiddenReason($action) { + if (!isset($this->forbiddenActions[$action])) { + throw new Exception( + pht( + 'Action "%s" is not forbidden!', + $action)); + } + + return $this->forbiddenActions[$action]; + } + } diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index a9509beb33..dc7ca676e3 100644 --- a/src/applications/herald/controller/HeraldTranscriptController.php +++ b/src/applications/herald/controller/HeraldTranscriptController.php @@ -273,7 +273,11 @@ final class HeraldTranscriptController extends HeraldController { ->setTarget(phutil_tag('strong', array(), pht('Conditions')))); foreach ($cond_xscripts as $cond_xscript) { - if ($cond_xscript->getResult()) { + if ($cond_xscript->isForbidden()) { + $icon = 'fa-ban'; + $color = 'indigo'; + $result = pht('Forbidden'); + } else if ($cond_xscript->getResult()) { $icon = 'fa-check'; $color = 'green'; $result = pht('Passed'); @@ -284,12 +288,17 @@ final class HeraldTranscriptController extends HeraldController { } if ($cond_xscript->getNote()) { + $note_text = $cond_xscript->getNote(); + if ($cond_xscript->isForbidden()) { + $note_text = HeraldStateReasons::getExplanation($note_text); + } + $note = phutil_tag( 'div', array( 'class' => 'herald-condition-note', ), - $cond_xscript->getNote()); + $note_text); } else { $note = null; } @@ -310,7 +319,12 @@ final class HeraldTranscriptController extends HeraldController { $cond_list->addItem($cond_item); } - if ($rule_xscript->getResult()) { + 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'); diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index df81117b80..b8e1db5626 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -12,6 +12,9 @@ final class HeraldEngine extends Phobject { protected $object; private $dryRun; + private $forbiddenFields = array(); + private $forbiddenActions = array(); + public function setDryRun($dry_run) { $this->dryRun = $dry_run; return $this; @@ -76,39 +79,42 @@ final class HeraldEngine extends Phobject { // This is not a dry run, and this rule is only supposed to be // applied a single time, and it's already been applied... // That means automatic failure. - $xscript = id(new HeraldRuleTranscript()) - ->setRuleID($rule->getID()) + $this->newRuleTranscript($rule) ->setResult(false) - ->setRuleName($rule->getName()) - ->setRuleOwner($rule->getAuthorPHID()) ->setReason( pht( 'This rule is only supposed to be repeated a single time, '. 'and it has already been applied.')); - $this->transcript->addRuleTranscript($xscript); + $rule_matches = false; } else { - $rule_matches = $this->doesRuleMatch($rule, $object); + 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); + } } } catch (HeraldRecursiveConditionsException $ex) { $names = array(); - foreach ($this->stack as $rule_id => $ignored) { - $names[] = '"'.$rules[$rule_id]->getName().'"'; + foreach ($this->stack as $rule_phid => $ignored) { + $names[] = '"'.$rules[$rule_phid]->getName().'"'; } $names = implode(', ', $names); - foreach ($this->stack as $rule_id => $ignored) { - $xscript = new HeraldRuleTranscript(); - $xscript->setRuleID($rule_id); - $xscript->setResult(false); - $xscript->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)); - $xscript->setRuleName($rules[$rule_id]->getName()); - $xscript->setRuleOwner($rules[$rule_id]->getAuthorPHID()); - $this->transcript->addRuleTranscript($xscript); + 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)); } $rule_matches = false; } @@ -309,14 +315,9 @@ final class HeraldEngine extends Phobject { } } - $rule_transcript = new HeraldRuleTranscript(); - $rule_transcript->setRuleID($rule->getID()); - $rule_transcript->setResult($result); - $rule_transcript->setReason($reason); - $rule_transcript->setRuleName($rule->getName()); - $rule_transcript->setRuleOwner($rule->getAuthorPHID()); - - $this->transcript->addRuleTranscript($rule_transcript); + $this->newRuleTranscript($rule) + ->setResult($result) + ->setReason($reason); return $result; } @@ -327,16 +328,7 @@ final class HeraldEngine extends Phobject { HeraldAdapter $object) { $object_value = $this->getConditionObjectValue($condition, $object); - $test_value = $condition->getValue(); - - $cond = $condition->getFieldCondition(); - - $transcript = new HeraldConditionTranscript(); - $transcript->setRuleID($rule->getID()); - $transcript->setConditionID($condition->getID()); - $transcript->setFieldName($condition->getFieldName()); - $transcript->setCondition($cond); - $transcript->setTestValue($test_value); + $transcript = $this->newConditionTranscript($rule, $condition); try { $result = $object->doesConditionMatch( @@ -351,8 +343,6 @@ final class HeraldEngine extends Phobject { $transcript->setResult($result); - $this->transcript->addConditionTranscript($transcript); - return $result; } @@ -446,4 +436,136 @@ final class HeraldEngine extends Phobject { return false; } + private function newRuleTranscript(HeraldRule $rule) { + $xscript = id(new HeraldRuleTranscript()) + ->setRuleID($rule->getID()) + ->setRuleName($rule->getName()) + ->setRuleOwner($rule->getAuthorPHID()); + + $this->transcript->addRuleTranscript($xscript); + + return $xscript; + } + + private function newConditionTranscript( + HeraldRule $rule, + HeraldCondition $condition) { + + $xscript = id(new HeraldConditionTranscript()) + ->setRuleID($rule->getID()) + ->setConditionID($condition->getID()) + ->setFieldName($condition->getFieldName()) + ->setCondition($condition->getFieldCondition()) + ->setTestValue($condition->getValue()); + + $this->transcript->addConditionTranscript($xscript); + + return $xscript; + } + + private function newApplyTranscript( + HeraldAdapter $adapter, + HeraldRule $rule, + HeraldActionRecord $action) { + + $effect = id(new HeraldEffect()) + ->setObjectPHID($adapter->getPHID()) + ->setAction($action->getAction()) + ->setTarget($action->getTarget()) + ->setRule($rule); + + $xscript = new HeraldApplyTranscript($effect, false); + + $this->transcript->addApplyTranscript($xscript); + + return $xscript; + } + + private function isForbidden( + HeraldRule $rule, + HeraldAdapter $adapter) { + + $forbidden = $adapter->getForbiddenActions(); + if (!$forbidden) { + return false; + } + + $forbidden = array_fuse($forbidden); + + $is_forbidden = false; + + foreach ($rule->getConditions() as $condition) { + $field_key = $condition->getFieldName(); + + if (!isset($this->forbiddenFields[$field_key])) { + $reason = null; + + try { + $states = $adapter->getRequiredFieldStates($field_key); + } catch (Exception $ex) { + $states = array(); + } + + foreach ($states as $state) { + if (!isset($forbidden[$state])) { + continue; + } + $reason = $adapter->getForbiddenReason($state); + break; + } + + $this->forbiddenFields[$field_key] = $reason; + } + + $forbidden_reason = $this->forbiddenFields[$field_key]; + if ($forbidden_reason !== null) { + $this->newConditionTranscript($rule, $condition) + ->setResult(HeraldConditionTranscript::RESULT_FORBIDDEN) + ->setNote($forbidden_reason); + + $is_forbidden = true; + } + } + + foreach ($rule->getActions() as $action_record) { + $action_key = $action_record->getAction(); + + if (!isset($this->forbiddenActions[$action_key])) { + $reason = null; + + try { + $states = $adapter->getRequiredActionStates($action_key); + } catch (Exception $ex) { + $states = array(); + } + + foreach ($states as $state) { + if (!isset($forbidden[$state])) { + continue; + } + $reason = $adapter->getForbiddenReason($state); + break; + } + + $this->forbiddenActions[$action_key] = $reason; + } + + $forbidden_reason = $this->forbiddenActions[$action_key]; + if ($forbidden_reason !== null) { + $this->newApplyTranscript($adapter, $rule, $action_record) + ->setAppliedReason( + array( + array( + 'type' => HeraldAction::DO_STANDARD_FORBIDDEN, + 'data' => $forbidden_reason, + ), + )); + + $is_forbidden = true; + } + } + + return $is_forbidden; + } + } diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 2abed0ff1d..408a76f7fb 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -20,6 +20,10 @@ abstract class HeraldField extends Phobject { return null; } + public function getRequiredAdapterStates() { + return array(); + } + protected function getHeraldFieldStandardType() { throw new PhutilMethodNotImplementedException(); } diff --git a/src/applications/herald/state/HeraldBuildableState.php b/src/applications/herald/state/HeraldBuildableState.php new file mode 100644 index 0000000000..d19ae87c36 --- /dev/null +++ b/src/applications/herald/state/HeraldBuildableState.php @@ -0,0 +1,7 @@ +setAncestorClass(__CLASS__) + ->execute(); + } + + final public static function getExplanation($reason) { + $reasons = self::getAllReasons(); + + foreach ($reasons as $reason_implementation) { + $explanation = $reason_implementation->explainReason($reason); + if ($explanation !== null) { + return $explanation; + } + } + + return pht('Unknown reason ("%s").', $reason); + } + +} diff --git a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php index 36a534e6b0..a7096b6880 100644 --- a/src/applications/herald/storage/transcript/HeraldConditionTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldConditionTranscript.php @@ -10,6 +10,8 @@ final class HeraldConditionTranscript extends Phobject { protected $note; protected $result; + const RESULT_FORBIDDEN = 'forbidden'; + public function setRuleID($rule_id) { $this->ruleID = $rule_id; return $this; @@ -72,4 +74,9 @@ final class HeraldConditionTranscript extends Phobject { public function getResult() { return $this->result; } + + public function isForbidden() { + return ($this->getResult() === self::RESULT_FORBIDDEN); + } + } diff --git a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php index d12f1f9dd9..8928a74cf6 100644 --- a/src/applications/herald/storage/transcript/HeraldRuleTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldRuleTranscript.php @@ -9,6 +9,12 @@ final class HeraldRuleTranscript extends Phobject { protected $ruleName; protected $ruleOwner; + const RESULT_FORBIDDEN = 'forbidden'; + + public function isForbidden() { + return ($this->getResult() === self::RESULT_FORBIDDEN); + } + public function setResult($result) { $this->result = $result; return $this; diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php index 2b29fe2443..74fb879fe7 100644 --- a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php +++ b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php @@ -6,6 +6,12 @@ abstract class PhabricatorMetaMTAEmailHeraldAction const DO_SEND = 'do.send'; const DO_FORCE = 'do.force'; + public function getRequiredAdapterStates() { + return array( + HeraldMailableState::STATECONST, + ); + } + public function supportsObject($object) { // NOTE: This implementation lacks generality, but there's no great way to // figure out if something generates email right now.