From 7e1743a959e01e63d306dcc670685f678effe540 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Thu, 4 Apr 2019 16:30:42 -0700 Subject: [PATCH] Add a trigger rule to reassign a task Summary: Ref T13269. Workboard triggers can now reassign tasks on column drop. Also sprinkles some `setViewer()` calls in places that needed them. This mostly works, but a few issues: * To set the owner to unassigned, you must explicitly put the "No Owner" token in the typeahead. Maybe this should just figure out you've put nothing in that field and set it for you? * I'm pretty sure this was already broken, but if you change the rule type from a tokenizer to a different type, the default for the field doesn't populate correctly: {F6312227} Also adds a new hook for trigger rules: `getValueForField($value)` which allows you to transform a value stored in the DB into a form suitable for setting on a form control. Test Plan: Dragged tasks between columns and observed new owners as expected. Didn't try to get fancy to assign tasks to deleted users, users that the viewer can't see, bot users, etc etc. I'm relying on the underlying transaction to hopefully do the right thing. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T13269 Differential Revision: https://secure.phabricator.com/D20329 --- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectBoardViewController.php | 5 + ...habricatorProjectTriggerEditController.php | 6 + ...habricatorProjectTriggerViewController.php | 1 + .../storage/PhabricatorProjectTrigger.php | 30 ++++- ...icatorProjectTriggerManiphestOwnerRule.php | 126 ++++++++++++++++++ .../trigger/PhabricatorProjectTriggerRule.php | 6 +- ...icatorProjectTriggerRulesetTransaction.php | 3 +- 8 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6c810f6d32..7c144dbb61 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4202,6 +4202,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTriggerEditor' => 'applications/project/editor/PhabricatorProjectTriggerEditor.php', 'PhabricatorProjectTriggerInvalidRule' => 'applications/project/trigger/PhabricatorProjectTriggerInvalidRule.php', 'PhabricatorProjectTriggerListController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerListController.php', + 'PhabricatorProjectTriggerManiphestOwnerRule' => 'applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php', 'PhabricatorProjectTriggerManiphestPriorityRule' => 'applications/project/trigger/PhabricatorProjectTriggerManiphestPriorityRule.php', 'PhabricatorProjectTriggerManiphestStatusRule' => 'applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php', 'PhabricatorProjectTriggerNameTransaction' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerNameTransaction.php', @@ -10368,6 +10369,7 @@ phutil_register_library_map(array( 'PhabricatorProjectTriggerEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorProjectTriggerInvalidRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerListController' => 'PhabricatorProjectTriggerController', + 'PhabricatorProjectTriggerManiphestOwnerRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerManiphestPriorityRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerManiphestStatusRule' => 'PhabricatorProjectTriggerRule', 'PhabricatorProjectTriggerNameTransaction' => 'PhabricatorProjectTriggerTransactionType', diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 3ee0213daa..8ceb576b6f 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -577,6 +577,11 @@ final class PhabricatorProjectBoardViewController $panel->addHeaderAction($column_menu); if ($column->canHaveTrigger()) { + $trigger = $column->getTrigger(); + if ($trigger) { + $trigger->setViewer($viewer); + } + $trigger_menu = $this->buildTriggerMenu($column); $panel->addHeaderAction($trigger_menu); } diff --git a/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php b/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php index df362efb61..f8fbbf7356 100644 --- a/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php +++ b/src/applications/project/controller/trigger/PhabricatorProjectTriggerEditController.php @@ -25,6 +25,8 @@ final class PhabricatorProjectTriggerEditController $trigger = PhabricatorProjectTrigger::initializeNewTrigger(); } + $trigger->setViewer($viewer); + $column_phid = $request->getStr('columnPHID'); if ($column_phid) { $column = id(new PhabricatorProjectColumnQuery()) @@ -272,6 +274,10 @@ final class PhabricatorProjectTriggerEditController $rule_list = array_values($rule_list); $type_list = PhabricatorProjectTriggerRule::getAllTriggerRules(); + + foreach ($type_list as $rule) { + $rule->setViewer($this->getViewer()); + } $type_list = mpull($type_list, 'newTemplate'); $type_list = array_values($type_list); diff --git a/src/applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php b/src/applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php index d148c0a421..b5d22f4d72 100644 --- a/src/applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php +++ b/src/applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php @@ -20,6 +20,7 @@ final class PhabricatorProjectTriggerViewController if (!$trigger) { return new Aphront404Response(); } + $trigger->setViewer($viewer); $rules_view = $this->newRulesView($trigger); $columns_view = $this->newColumnsView($trigger); diff --git a/src/applications/project/storage/PhabricatorProjectTrigger.php b/src/applications/project/storage/PhabricatorProjectTrigger.php index 625dc7ffd8..2fd4a73788 100644 --- a/src/applications/project/storage/PhabricatorProjectTrigger.php +++ b/src/applications/project/storage/PhabricatorProjectTrigger.php @@ -13,6 +13,7 @@ final class PhabricatorProjectTrigger protected $editPolicy; private $triggerRules; + private $viewer; private $usage = self::ATTACHABLE; public static function initializeNewTrigger() { @@ -41,6 +42,15 @@ final class PhabricatorProjectTrigger return PhabricatorProjectTriggerPHIDType::TYPECONST; } + public function getViewer() { + return $this->viewer; + } + + public function setViewer(PhabricatorUser $user) { + $this->viewer = $user; + return $this; + } + public function getDisplayName() { $name = $this->getName(); if (strlen($name)) { @@ -72,11 +82,16 @@ final class PhabricatorProjectTrigger parent::setRuleset($ruleset); } - public function getTriggerRules() { + public function getTriggerRules($viewer = null) { if ($this->triggerRules === null) { + if (!$viewer) { + $viewer = $this->getViewer(); + } + $trigger_rules = self::newTriggerRulesFromRuleSpecifications( $this->getRuleset(), - $allow_invalid = true); + $allow_invalid = true, + $viewer); $this->triggerRules = $trigger_rules; } @@ -86,7 +101,8 @@ final class PhabricatorProjectTrigger public static function newTriggerRulesFromRuleSpecifications( array $list, - $allow_invalid) { + $allow_invalid, + PhabricatorUser $viewer) { // 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 @@ -124,7 +140,7 @@ final class PhabricatorProjectTrigger if (!is_array($rule)) { throw new PhabricatorProjectTriggerCorruptionException( pht( - 'Trigger ruleset is corrupt: rule (with key "%s") should be a '. + 'Trigger ruleset is corrupt: rule (at index "%s") should be a '. 'rule specification, but is actually "%s".', $key, phutil_describe_type($rule))); @@ -140,7 +156,7 @@ final class PhabricatorProjectTrigger } catch (PhutilTypeCheckException $ex) { throw new PhabricatorProjectTriggerCorruptionException( pht( - 'Trigger ruleset is corrupt: rule (with key "%s") is not a '. + 'Trigger ruleset is corrupt: rule (at index "%s") is not a '. 'valid rule specification: %s', $key, $ex->getMessage())); @@ -179,6 +195,7 @@ final class PhabricatorProjectTrigger ->setRecord($record) ->setException($ex); } + $rule->setViewer($viewer); $trigger_rules[] = $rule; } @@ -206,9 +223,8 @@ final class PhabricatorProjectTrigger $object) { $trigger_xactions = array(); - foreach ($this->getTriggerRules() as $rule) { + foreach ($this->getTriggerRules($viewer) as $rule) { $rule - ->setViewer($viewer) ->setTrigger($this) ->setColumn($column) ->setObject($object); diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php new file mode 100644 index 0000000000..e59a69cdf5 --- /dev/null +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php @@ -0,0 +1,126 @@ +getDatasource()->getWireTokens($this->getValue()); + } + + private function convertTokenizerValueToOwner($value) { + $value = head($value); + if ($value === PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN) { + $value = null; + } + return $value; + } + + protected function assertValidRuleValue($value) { + if (!is_array($value)) { + throw new Exception( + pht( + 'Owner rule value should be a list, but is not (value is "%s").', + phutil_describe_type($value))); + } + + if (count($value) != 1) { + throw new Exception( + pht( + 'Owner rule value should be a list of exactly one user PHID, or the '. + 'token "none()" (value is "%s").', + implode(', ', $value))); + } + } + + protected function newDropTransactions($object, $value) { + $value = $this->convertTokenizerValueToOwner($value); + return array( + $this->newTransaction() + ->setTransactionType(ManiphestTaskOwnerTransaction::TRANSACTIONTYPE) + ->setNewValue($value), + ); + } + + protected function newDropEffects($value) { + $owner_value = $this->convertTokenizerValueToOwner($value); + + return array( + $this->newEffect() + ->setIcon('fa-user') + ->setContent($this->getRuleViewDescription($value)) + ->addCondition('owner', '!=', $owner_value), + ); + } + + protected function getDefaultValue() { + return null; + } + + protected function getPHUIXControlType() { + return 'tokenizer'; + } + + protected function getDatasource() { + $datasource = id(new ManiphestAssigneeDatasource()) + ->setLimit(1); + + if ($this->getViewer()) { + $datasource->setViewer($this->getViewer()); + } + + return $datasource; + } + + protected function getPHUIXControlSpecification() { + $template = id(new AphrontTokenizerTemplateView()) + ->setViewer($this->getViewer()); + + $template_markup = $template->render(); + $datasource = $this->getDatasource(); + + return array( + 'markup' => (string)hsprintf('%s', $template_markup), + 'config' => array( + 'src' => $datasource->getDatasourceURI(), + 'browseURI' => $datasource->getBrowseURI(), + 'placeholder' => $datasource->getPlaceholderText(), + 'limit' => $datasource->getLimit(), + ), + 'value' => null, + ); + } + + public function getRuleViewLabel() { + return pht('Change Owner'); + } + + public function getRuleViewDescription($value) { + $value = $this->convertTokenizerValueToOwner($value); + + if (!$value) { + return pht('Unassign task.'); + } else { + return pht( + 'Assign task to %s.', + phutil_tag( + 'strong', + array(), + $this->getViewer() + ->renderHandle($value) + ->render())); + } + } + + public function getRuleViewIcon($value) { + return id(new PHUIIconView()) + ->setIcon('fa-user', 'green'); + } + + +} diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php index ae2b3ee092..b150ee466f 100644 --- a/src/applications/project/trigger/PhabricatorProjectTriggerRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerRule.php @@ -37,6 +37,10 @@ abstract class PhabricatorProjectTriggerRule return $this->getRecord()->getValue(); } + protected function getValueForEditorField() { + return $this->getValue(); + } + abstract public function getSelectControlName(); abstract public function getRuleViewLabel(); abstract public function getRuleViewDescription($value); @@ -130,7 +134,7 @@ abstract class PhabricatorProjectTriggerRule return array( 'type' => $record->getType(), - 'value' => $record->getValue(), + 'value' => $this->getValueForEditorField(), 'isValidRule' => $is_valid, 'invalidView' => $invalid_view, ); diff --git a/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php index 59c846becf..3aefb5cfbd 100644 --- a/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php +++ b/src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php @@ -28,7 +28,8 @@ final class PhabricatorProjectTriggerRulesetTransaction try { PhabricatorProjectTrigger::newTriggerRulesFromRuleSpecifications( $ruleset, - $allow_invalid = false); + $allow_invalid = false, + $xaction->getViewer()); } catch (PhabricatorProjectTriggerCorruptionException $ex) { $errors[] = $this->newInvalidError( pht(