From 149f8cc9595a286854d3bb22624ed6c4da871ce6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Mar 2019 16:40:18 -0700 Subject: [PATCH] Hard code a "close task" action on every column Trigger Summary: Depends on D20287. Ref T5474. This hard-codes a storage value for every trigger, with a "Change status to " rule and two bogus rules. Rules may now apply transactions when cards are dropped. Test Plan: Dragged cards to a column with a trigger, saw them close. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T5474 Differential Revision: https://secure.phabricator.com/D20288 --- src/__phutil_library_map__.php | 12 ++ .../PhabricatorProjectBoardViewController.php | 1 + .../PhabricatorProjectMoveController.php | 14 ++ ...catorProjectTriggerCorruptionException.php | 4 + .../storage/PhabricatorProjectTrigger.php | 179 +++++++++++++++++- .../PhabricatorProjectTriggerInvalidRule.php | 22 +++ ...catorProjectTriggerManiphestStatusRule.php | 41 ++++ .../trigger/PhabricatorProjectTriggerRule.php | 89 +++++++++ .../PhabricatorProjectTriggerRuleRecord.php | 27 +++ .../PhabricatorProjectTriggerUnknownRule.php | 22 +++ 10 files changed, 408 insertions(+), 3 deletions(-) create mode 100644 src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerRule.php create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 50df6a7201..b042b017e2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4174,16 +4174,22 @@ phutil_register_library_map(array( 'PhabricatorProjectTransactionType' => 'applications/project/xaction/PhabricatorProjectTransactionType.php', 'PhabricatorProjectTrigger' => 'applications/project/storage/PhabricatorProjectTrigger.php', 'PhabricatorProjectTriggerController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerController.php', + 'PhabricatorProjectTriggerCorruptionException' => 'applications/project/exception/PhabricatorProjectTriggerCorruptionException.php', 'PhabricatorProjectTriggerEditController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php', 'PhabricatorProjectTriggerEditor' => 'applications/project/editor/PhabricatorProjectTriggerEditor.php', + 'PhabricatorProjectTriggerInvalidRule' => 'applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php', 'PhabricatorProjectTriggerListController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerListController.php', + 'PhabricatorProjectTriggerManiphestStatusRule' => 'applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php', 'PhabricatorProjectTriggerNameTransaction' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerNameTransaction.php', 'PhabricatorProjectTriggerPHIDType' => 'applications/project/phid/PhabricatorProjectTriggerPHIDType.php', 'PhabricatorProjectTriggerQuery' => 'applications/project/query/PhabricatorProjectTriggerQuery.php', + 'PhabricatorProjectTriggerRule' => 'applications/project/trigger/PhabricatorProjectTriggerRule.php', + 'PhabricatorProjectTriggerRuleRecord' => 'applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php', 'PhabricatorProjectTriggerSearchEngine' => 'applications/project/query/PhabricatorProjectTriggerSearchEngine.php', 'PhabricatorProjectTriggerTransaction' => 'applications/project/storage/PhabricatorProjectTriggerTransaction.php', 'PhabricatorProjectTriggerTransactionQuery' => 'applications/project/query/PhabricatorProjectTriggerTransactionQuery.php', 'PhabricatorProjectTriggerTransactionType' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerTransactionType.php', + 'PhabricatorProjectTriggerUnknownRule' => 'applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php', 'PhabricatorProjectTriggerViewController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php', 'PhabricatorProjectTypeTransaction' => 'applications/project/xaction/PhabricatorProjectTypeTransaction.php', 'PhabricatorProjectUIEventListener' => 'applications/project/events/PhabricatorProjectUIEventListener.php', @@ -10300,16 +10306,22 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', ), 'PhabricatorProjectTriggerController' => 'PhabricatorProjectController', + 'PhabricatorProjectTriggerCorruptionException' => 'Exception', 'PhabricatorProjectTriggerEditController' => 'PhabricatorProjectTriggerController', 'PhabricatorProjectTriggerEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorProjectTriggerInvalidRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerListController' => 'PhabricatorProjectTriggerController', + 'PhabricatorProjectTriggerManiphestStatusRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerNameTransaction' => 'PhabricatorProjectTriggerTransactionType', 'PhabricatorProjectTriggerPHIDType' => 'PhabricatorPHIDType', 'PhabricatorProjectTriggerQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorProjectTriggerRule' => 'Phobject', + 'PhabricatorProjectTriggerRuleRecord' => 'Phobject', 'PhabricatorProjectTriggerSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectTriggerTransaction' => 'PhabricatorModularTransaction', 'PhabricatorProjectTriggerTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorProjectTriggerTransactionType' => 'PhabricatorModularTransactionType', + 'PhabricatorProjectTriggerUnknownRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerViewController' => 'PhabricatorProjectTriggerController', 'PhabricatorProjectTypeTransaction' => 'PhabricatorProjectTransactionType', 'PhabricatorProjectUIEventListener' => 'PhabricatorEventListener', diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index dda21c0c43..3e702b981b 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -1270,6 +1270,7 @@ final class PhabricatorProjectBoardViewController array( 'items' => hsprintf('%s', $trigger_menu), 'tip' => $trigger_tip, + 'size' => 300, )); return $trigger_button; diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 3cfd94894b..71588754c0 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -70,6 +70,7 @@ final class PhabricatorProjectMoveController $columns = id(new PhabricatorProjectColumnQuery()) ->setViewer($viewer) ->withProjectPHIDs(array($project->getPHID())) + ->needTriggers(true) ->execute(); $columns = mpull($columns, null, 'getPHID'); @@ -110,6 +111,19 @@ final class PhabricatorProjectMoveController $xactions[] = $header_xaction; } + if ($column->canHaveTrigger()) { + $trigger = $column->getTrigger(); + if ($trigger) { + $trigger_xactions = $trigger->newDropTransactions( + $viewer, + $column, + $object); + foreach ($trigger_xactions as $trigger_xaction) { + $xactions[] = $trigger_xaction; + } + } + } + $editor = id(new ManiphestTransactionEditor()) ->setActor($viewer) ->setContinueOnMissingFields(true) diff --git a/src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php b/src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php new file mode 100644 index 0000000000..c235fe7357 --- /dev/null +++ b/src/applications/project/exception/PhabricatorProjectTriggerCorruptionException.php @@ -0,0 +1,4 @@ +getID()); } - public function getRulesDescription() { - // TODO: Summarize the trigger rules in human-readable text. - return pht('Does things.'); + 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; + } + + $this->triggerRules = $trigger_rules; + } + + return $this->triggerRules; } + public function getRulesDescription() { + $rules = $this->getTriggerRules(); + if (!$rules) { + return pht('Does nothing.'); + } + + $things = array(); + + $count = count($rules); + $limit = 3; + + if ($count > $limit) { + $show_rules = array_slice($rules, 0, ($limit - 1)); + } else { + $show_rules = $rules; + } + + foreach ($show_rules as $rule) { + $things[] = $rule->getDescription(); + } + + if ($count > $limit) { + $things[] = pht( + '(Applies %s more actions.)', + new PhutilNumber($count - $limit)); + } + + return implode("\n", $things); + } + + public function newDropTransactions( + PhabricatorUser $viewer, + PhabricatorProjectColumn $column, + $object) { + + $trigger_xactions = array(); + foreach ($this->getTriggerRules() as $rule) { + $rule + ->setViewer($viewer) + ->setTrigger($this) + ->setColumn($column) + ->setObject($object); + + $xactions = $rule->getDropTransactions( + $object, + $rule->getRecord()->getValue()); + + if (!is_array($xactions)) { + throw new Exception( + pht( + 'Expected trigger rule (of class "%s") to return a list of '. + 'transactions from "newDropTransactions()", but got "%s".', + get_class($rule), + phutil_describe_type($xactions))); + } + + $expect_type = get_class($object->getApplicationTransactionTemplate()); + assert_instances_of($xactions, $expect_type); + + foreach ($xactions as $xaction) { + $trigger_xactions[] = $xaction; + } + } + + return $trigger_xactions; + } + + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php new file mode 100644 index 0000000000..55e91e9136 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php @@ -0,0 +1,22 @@ +getRecord()->getType()); + } + + protected function assertValidRuleValue($value) { + return; + } + + protected function newDropTransactions($object, $value) { + return array(); + } + +} diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php new file mode 100644 index 0000000000..ef630f89d9 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php @@ -0,0 +1,41 @@ +getValue(); + + return pht( + 'Changes status to "%s".', + ManiphestTaskStatus::getTaskStatusName($value)); + } + + protected function assertValidRuleValue($value) { + if (!is_string($value)) { + throw new Exception( + pht( + 'Status rule value should be a string, but is not (value is "%s").', + phutil_describe_type($value))); + } + + $map = ManiphestTaskStatus::getTaskStatusMap(); + if (!isset($map[$value])) { + throw new Exception( + pht( + 'Rule value ("%s") is not a valid task status.', + $value)); + } + } + + protected function newDropTransactions($object, $value) { + return array( + $this->newTransaction() + ->setTransactionType(ManiphestTaskStatusTransaction::TRANSACTIONTYPE) + ->setNewValue($value), + ); + } + +} diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php new file mode 100644 index 0000000000..b311c05ff8 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php @@ -0,0 +1,89 @@ +getPhobjectClassConstant('TRIGGERTYPE', 64); + } + + final public static function getAllTriggerRules() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getTriggerType') + ->execute(); + } + + final public function setRecord(PhabricatorProjectTriggerRuleRecord $record) { + $value = $record->getValue(); + + $this->assertValidRuleValue($value); + + $this->record = $record; + return $this; + } + + final public function getRecord() { + return $this->record; + } + + final protected function getValue() { + return $this->getRecord()->getValue(); + } + + abstract public function getDescription(); + abstract protected function assertValidRuleValue($value); + abstract protected function newDropTransactions($object, $value); + + final public function getDropTransactions($object, $value) { + return $this->newDropTransactions($object, $value); + } + + final public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + final public function setColumn(PhabricatorProjectColumn $column) { + $this->column = $column; + return $this; + } + + final public function getColumn() { + return $this->column; + } + + final public function setTrigger(PhabricatorProjectTrigger $trigger) { + $this->trigger = $trigger; + return $this; + } + + final public function getTrigger() { + return $this->trigger; + } + + final public function setObject( + PhabricatorApplicationTransactionInterface $object) { + $this->object = $object; + return $this; + } + + final public function getObject() { + return $this->object; + } + + final protected function newTransaction() { + return $this->getObject()->getApplicationTransactionTemplate(); + } + +} diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php b/src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php new file mode 100644 index 0000000000..da36d9a4d8 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerRuleRecord.php @@ -0,0 +1,27 @@ +type = $type; + return $this; + } + + public function getType() { + return $this->type; + } + + public function setValue($value) { + $this->value = $value; + return $this; + } + + public function getValue() { + return $this->value; + } + +} diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php new file mode 100644 index 0000000000..881a6652f4 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php @@ -0,0 +1,22 @@ +getRecord()->getType()); + } + + protected function assertValidRuleValue($value) { + return; + } + + protected function newDropTransactions($object, $value) { + return array(); + } + +}