From ff128e1b3244dc717a002de76c25f70d72d0a1ee Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Mar 2019 11:48:59 -0700 Subject: [PATCH] Write workboard trigger rules to the database Summary: Ref T5474. Read and write trigger rules so users can actually edit them. Test Plan: Added, modified, and removed trigger rules. Saved changes, used "Show Details" to review edits. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T5474 Differential Revision: https://secure.phabricator.com/D20302 --- src/__phutil_library_map__.php | 2 + ...habricatorProjectTriggerEditController.php | 20 +- .../storage/PhabricatorProjectTrigger.php | 212 ++++++++++-------- ...icatorProjectTriggerRulesetTransaction.php | 65 ++++++ 4 files changed, 196 insertions(+), 103 deletions(-) create mode 100644 src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ebe7c2b9c3..a4edf3d246 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4186,6 +4186,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTriggerQuery' => 'applications/project/query/PhabricatorProjectTriggerQuery.php', 'PhabricatorProjectTriggerRule' => 'applications/project/trigger/PhabricatorProjectTriggerRule.php', 'PhabricatorProjectTriggerRuleRecord' => 'applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php', + 'PhabricatorProjectTriggerRulesetTransaction' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php', 'PhabricatorProjectTriggerSearchEngine' => 'applications/project/query/PhabricatorProjectTriggerSearchEngine.php', 'PhabricatorProjectTriggerTransaction' => 'applications/project/storage/PhabricatorProjectTriggerTransaction.php', 'PhabricatorProjectTriggerTransactionQuery' => 'applications/project/query/PhabricatorProjectTriggerTransactionQuery.php', @@ -10319,6 +10320,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTriggerQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectTriggerRule' => 'Phobject', 'PhabricatorProjectTriggerRuleRecord' => 'Phobject', + 'PhabricatorProjectTriggerRulesetTransaction' => 'PhabricatorProjectTriggerTransactionType', 'PhabricatorProjectTriggerSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectTriggerTransaction' => 'PhabricatorModularTransaction', 'PhabricatorProjectTriggerTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php b/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php index 95d1e9f021..7189df70ec 100644 --- a/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php +++ b/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php @@ -55,6 +55,7 @@ final class PhabricatorProjectTriggerEditController $v_name = $trigger->getName(); $v_edit = $trigger->getEditPolicy(); + $v_rules = $trigger->getTriggerRules(); $e_name = null; $e_edit = null; @@ -65,8 +66,15 @@ final class PhabricatorProjectTriggerEditController $v_name = $request->getStr('name'); $v_edit = $request->getStr('editPolicy'); - $v_rules = $request->getStr('rules'); - $v_rules = phutil_json_decode($v_rules); + // Read the JSON rules from the request and convert them back into + // "TriggerRule" objects so we can render the correct form state + // if the user is modifying the rules + $raw_rules = $request->getStr('rules'); + $raw_rules = phutil_json_decode($raw_rules); + + $copy = clone $trigger; + $copy->setRuleset($raw_rules); + $v_rules = $copy->getTriggerRules(); $xactions = array(); if (!$trigger->getID()) { @@ -84,7 +92,10 @@ final class PhabricatorProjectTriggerEditController ->setTransactionType(PhabricatorTransactions::TYPE_EDIT_POLICY) ->setNewValue($v_edit); - // TODO: Actually write the new rules to the database. + $xactions[] = $trigger->getApplicationTransactionTemplate() + ->setTransactionType( + PhabricatorProjectTriggerRulesetTransaction::TRANSACTIONTYPE) + ->setNewValue($raw_rules); $editor = $trigger->getApplicationTransactionEditor() ->setActor($viewer) @@ -207,6 +218,7 @@ final class PhabricatorProjectTriggerEditController $this->setupEditorBehavior( $trigger, + $v_rules, $form_id, $table_id, $create_id, @@ -250,12 +262,12 @@ final class PhabricatorProjectTriggerEditController private function setupEditorBehavior( PhabricatorProjectTrigger $trigger, + array $rule_list, $form_id, $table_id, $create_id, $input_id) { - $rule_list = $trigger->getTriggerRules(); $rule_list = mpull($rule_list, 'toDictionary'); $rule_list = array_values($rule_list); diff --git a/src/applications/project/storage/PhabricatorProjectTrigger.php b/src/applications/project/storage/PhabricatorProjectTrigger.php index b02e1ce107..a195e9fc44 100644 --- a/src/applications/project/storage/PhabricatorProjectTrigger.php +++ b/src/applications/project/storage/PhabricatorProjectTrigger.php @@ -62,107 +62,19 @@ final class PhabricatorProjectTrigger return pht('Trigger %d', $this->getID()); } + public function setRuleset(array $ruleset) { + // Clear any cached trigger rules, since we're changing the ruleset + // for the trigger. + $this->triggerRules = null; + + parent::setRuleset($ruleset); + } + public function getTriggerRules() { if ($this->triggerRules === null) { - - // TODO: Temporary hard-coded rule specification. - $rule_specifications = array( - array( - 'type' => 'status', - 'value' => ManiphestTaskStatus::getDefaultClosedStatus(), - ), - // This is an intentionally unknown rule. - array( - 'type' => 'quack', - 'value' => 'aaa', - ), - // This is an intentionally invalid rule. - array( - 'type' => 'status', - 'value' => 'quack', - ), - ); - - // NOTE: We're trying to preserve the database state in the rule - // structure, even if it includes rule types we don't have implementations - // for, or rules with invalid rule values. - - // If an administrator adds or removes extensions which add rules, or - // an upgrade affects rule validity, existing rules may become invalid. - // When they do, we still want the UI to reflect the ruleset state - // accurately and "Edit" + "Save" shouldn't destroy data unless the - // user explicitly modifies the ruleset. - - // When we run into rules which are structured correctly but which have - // types we don't know about, we replace them with "Unknown Rules". If - // we know about the type of a rule but the value doesn't validate, we - // replace it with "Invalid Rules". These two rule types don't take any - // actions when a card is dropped into the column, but they show the user - // what's wrong with the ruleset and can be saved without causing any - // collateral damage. - - $rule_map = PhabricatorProjectTriggerRule::getAllTriggerRules(); - - // If the stored rule data isn't a list of rules (or we encounter other - // fundamental structural problems, below), there isn't much we can do - // to try to represent the state. - if (!is_array($rule_specifications)) { - throw new PhabricatorProjectTriggerCorruptionException( - pht( - 'Trigger ("%s") has a corrupt ruleset: expected a list of '. - 'rule specifications, found "%s".', - $this->getPHID(), - phutil_describe_type($rule_specifications))); - } - - $trigger_rules = array(); - foreach ($rule_specifications as $key => $rule) { - if (!is_array($rule)) { - throw new PhabricatorProjectTriggerCorruptionException( - pht( - 'Trigger ("%s") has a corrupt ruleset: rule (with key "%s") '. - 'should be a rule specification, but is actually "%s".', - $this->getPHID(), - $key, - phutil_describe_type($rule))); - } - - try { - PhutilTypeSpec::checkMap( - $rule, - array( - 'type' => 'string', - 'value' => 'wild', - )); - } catch (PhutilTypeCheckException $ex) { - throw new PhabricatorProjectTriggerCorruptionException( - pht( - 'Trigger ("%s") has a corrupt ruleset: rule (with key "%s") '. - 'is not a valid rule specification: %s', - $this->getPHID(), - $key, - $ex->getMessage())); - } - - $record = id(new PhabricatorProjectTriggerRuleRecord()) - ->setType(idx($rule, 'type')) - ->setValue(idx($rule, 'value')); - - if (!isset($rule_map[$record->getType()])) { - $rule = new PhabricatorProjectTriggerUnknownRule(); - } else { - $rule = clone $rule_map[$record->getType()]; - } - - try { - $rule->setRecord($record); - } catch (Exception $ex) { - $rule = id(new PhabricatorProjectTriggerInvalidRule()) - ->setRecord($record); - } - - $trigger_rules[] = $rule; - } + $trigger_rules = self::newTriggerRulesFromRuleSpecifications( + $this->getRuleset(), + $allow_invalid = true); $this->triggerRules = $trigger_rules; } @@ -170,6 +82,108 @@ final class PhabricatorProjectTrigger return $this->triggerRules; } + public static function newTriggerRulesFromRuleSpecifications( + array $list, + $allow_invalid) { + + // NOTE: With "$allow_invalid" set, we're trying to preserve the database + // state in the rule structure, even if it includes rule types we don't + // ha ve implementations for, or rules with invalid rule values. + + // If an administrator adds or removes extensions which add rules, or + // an upgrade affects rule validity, existing rules may become invalid. + // When they do, we still want the UI to reflect the ruleset state + // accurately and "Edit" + "Save" shouldn't destroy data unless the + // user explicitly modifies the ruleset. + + // In this mode, when we run into rules which are structured correctly but + // which have types we don't know about, we replace them with "Unknown + // Rules". If we know about the type of a rule but the value doesn't + // validate, we replace it with "Invalid Rules". These two rule types don't + // take any actions when a card is dropped into the column, but they show + // the user what's wrong with the ruleset and can be saved without causing + // any collateral damage. + + $rule_map = PhabricatorProjectTriggerRule::getAllTriggerRules(); + + // If the stored rule data isn't a list of rules (or we encounter other + // fundamental structural problems, below), there isn't much we can do + // to try to represent the state. + if (!is_array($list)) { + throw new PhabricatorProjectTriggerCorruptionException( + pht( + 'Trigger ruleset is corrupt: expected a list of rule '. + 'specifications, found "%s".', + phutil_describe_type($list))); + } + + $trigger_rules = array(); + foreach ($list as $key => $rule) { + if (!is_array($rule)) { + throw new PhabricatorProjectTriggerCorruptionException( + pht( + 'Trigger ruleset is corrupt: rule (with key "%s") should be a '. + 'rule specification, but is actually "%s".', + $key, + phutil_describe_type($rule))); + } + + try { + PhutilTypeSpec::checkMap( + $rule, + array( + 'type' => 'string', + 'value' => 'wild', + )); + } catch (PhutilTypeCheckException $ex) { + throw new PhabricatorProjectTriggerCorruptionException( + pht( + 'Trigger ruleset is corrupt: rule (with key "%s") is not a '. + 'valid rule specification: %s', + $key, + $ex->getMessage())); + } + + $record = id(new PhabricatorProjectTriggerRuleRecord()) + ->setType(idx($rule, 'type')) + ->setValue(idx($rule, 'value')); + + if (!isset($rule_map[$record->getType()])) { + if (!$allow_invalid) { + throw new PhabricatorProjectTriggerCorruptionException( + pht( + 'Trigger ruleset is corrupt: rule type "%s" is unknown.', + $record->getType())); + } + + $rule = new PhabricatorProjectTriggerUnknownRule(); + } else { + $rule = clone $rule_map[$record->getType()]; + } + + try { + $rule->setRecord($record); + } catch (Exception $ex) { + if (!$allow_invalid) { + throw new PhabricatorProjectTriggerCorruptionException( + pht( + 'Trigger ruleset is corrupt, rule (of type "%s") does not '. + 'validate: %s', + $record->getType(), + $ex->getMessage())); + } + + $rule = id(new PhabricatorProjectTriggerInvalidRule()) + ->setRecord($record); + } + + $trigger_rules[] = $rule; + } + + return $trigger_rules; + } + + public function getDropEffects() { $effects = array(); diff --git a/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php new file mode 100644 index 0000000000..59c846becf --- /dev/null +++ b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php @@ -0,0 +1,65 @@ +getRuleset(); + } + + public function applyInternalEffects($object, $value) { + $object->setRuleset($value); + } + + public function getTitle() { + return pht( + '%s updated the ruleset for this trigger.', + $this->renderAuthor()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $ruleset = $xaction->getNewValue(); + + try { + PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications( + $ruleset, + $allow_invalid = false); + } catch (PhabricatorProjectTriggerCorruptionException $ex) { + $errors[] = $this->newInvalidError( + pht( + 'Ruleset specification is not valid. %s', + $ex->getMessage()), + $xaction); + continue; + } + } + + return $errors; + } + + public function hasChangeDetailView() { + return true; + } + + public function newChangeDetailView() { + $viewer = $this->getViewer(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $json = new PhutilJSON(); + $old_json = $json->encodeAsList($old); + $new_json = $json->encodeAsList($new); + + return id(new PhabricatorApplicationTransactionTextDiffDetailView()) + ->setViewer($viewer) + ->setOldText($old_json) + ->setNewText($new_json); + } + +}