From 06b3f21b61bcb517958ddb143ae42af8d94b5e9a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2013 17:24:58 -0800 Subject: [PATCH] Implement "user" and "users" Maniphest custom fields Summary: Ref T2575. Implements "user" (zero or one users) and "users" (zero or more users) field types. Also allows custom fields to participate in the handle pipeline. Test Plan: {F35071} Reviewers: hach-que, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2575 Differential Revision: https://secure.phabricator.com/D5287 --- ...hestAuxiliaryFieldDefaultSpecification.php | 97 ++++++++++++++++++- .../ManiphestAuxiliaryFieldSpecification.php | 20 ++++ .../ManiphestTaskDetailController.php | 16 ++- .../ManiphestTaskEditController.php | 39 ++++---- src/docs/userguide/maniphest_custom.diviner | 10 +- 5 files changed, 152 insertions(+), 30 deletions(-) diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php index b0818a22e6..41d3009618 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php @@ -21,6 +21,8 @@ class ManiphestAuxiliaryFieldDefaultSpecification const TYPE_BOOL = 'bool'; const TYPE_DATE = 'date'; const TYPE_REMARKUP = 'remarkup'; + const TYPE_USER = 'user'; + const TYPE_USERS = 'users'; public function getFieldType() { return $this->fieldType; @@ -102,6 +104,14 @@ class ManiphestAuxiliaryFieldDefaultSpecification $control = new PhabricatorRemarkupControl(); $control->setUser($this->getUser()); break; + case self::TYPE_USER: + case self::TYPE_USERS: + $control = new AphrontFormTokenizerControl(); + $control->setDatasource('/typeahead/common/users/'); + if ($type == self::TYPE_USER) { + $control->setLimit(1); + } + break; default: $label = $this->getLabel(); throw new ManiphestAuxiliaryFieldTypeException( @@ -121,6 +131,15 @@ class ManiphestAuxiliaryFieldDefaultSpecification $control->setValue($this->getValue()); $control->setName('auxiliary_date_'.$this->getAuxiliaryKey()); break; + case self::TYPE_USER: + case self::TYPE_USERS: + $control->setName('auxiliary_tokenizer_'.$this->getAuxiliaryKey()); + $value = array(); + foreach ($this->getValue() as $phid) { + $value[$phid] = $this->getHandle($phid)->getFullName(); + } + $control->setValue($value); + break; default: $control->setValue($this->getValue()); $control->setName('auxiliary['.$this->getAuxiliaryKey().']'); @@ -135,11 +154,20 @@ class ManiphestAuxiliaryFieldDefaultSpecification } public function setValueFromRequest(AphrontRequest $request) { - switch ($this->getFieldType()) { + $type = $this->getFieldType(); + switch ($type) { case self::TYPE_DATE: $control = $this->renderControl(); $value = $control->readValueFromRequest($request); break; + case self::TYPE_USER: + case self::TYPE_USERS: + $name = 'auxiliary_tokenizer_'.$this->getAuxiliaryKey(); + $value = $request->getArr($name); + if ($type == self::TYPE_USER) { + $value = array_slice($value, 0, 1); + } + break; default: $aux_post_values = $request->getArr('auxiliary'); $value = idx($aux_post_values, $this->getAuxiliaryKey(), ''); @@ -149,17 +177,34 @@ class ManiphestAuxiliaryFieldDefaultSpecification } public function getValueForStorage() { - return $this->getValue(); + switch ($this->getFieldType()) { + case self::TYPE_USER: + case self::TYPE_USERS: + return json_encode($this->getValue()); + default: + return $this->getValue(); + } } public function setValueFromStorage($value) { + switch ($this->getFieldType()) { + case self::TYPE_USER: + case self::TYPE_USERS: + $value = json_decode($value, true); + if (!is_array($value)) { + $value = array(); + } + break; + default: + break; + } return $this->setValue($value); } public function validate() { switch ($this->getFieldType()) { case self::TYPE_INT: - if (!is_numeric($this->getValue())) { + if ($this->getValue() && !is_numeric($this->getValue())) { throw new ManiphestAuxiliaryFieldValidationException( pht( '%s must be an integer value.', @@ -173,13 +218,22 @@ class ManiphestAuxiliaryFieldDefaultSpecification case self::TYPE_SELECT: return true; case self::TYPE_DATE: - if ($this->getValue() <= 0) { + if ((int)$this->getValue() <= 0) { throw new ManiphestAuxiliaryFieldValidationException( pht( '%s must be a valid date.', $this->getLabel())); } break; + case self::TYPE_USER: + case self::TYPE_USERS: + if (!is_array($this->getValue())) { + throw new ManiphestAuxiliaryFieldValidationException( + pht( + '%s is not a valid list of user PHIDs.', + $this->getLabel())); + } + break; } } @@ -192,6 +246,15 @@ class ManiphestAuxiliaryFieldDefaultSpecification } $this->setValue($value); break; + case self::TYPE_USER: + case self::TYPE_USERS: + if (!is_array($value)) { + $value = array(); + } else { + $value = array_values($value); + } + $this->setValue($value); + break; default: $this->setValue((string)$value); break; @@ -222,10 +285,30 @@ class ManiphestAuxiliaryFieldDefaultSpecification return $this->getMarkupEngine()->getOutput( $this, 'default'); + case self::TYPE_USER: + case self::TYPE_USERS: + return $this->renderHandleList($this->getValue()); } return parent::renderForDetailView(); } + public function getRequiredHandlePHIDs() { + switch ($this->getFieldType()) { + case self::TYPE_USER; + case self::TYPE_USERS: + return $this->getValue(); + } + return parent::getRequiredHandlePHIDs(); + } + + protected function renderHandleList(array $phids) { + $links = array(); + foreach ($phids as $phid) { + $links[] = $this->getHandle($phid)->renderLink(); + } + return phutil_implode_html(', ', $links); + } + public function renderTransactionDescription( ManiphestTransaction $transaction, $target) { @@ -266,6 +349,12 @@ class ManiphestAuxiliaryFieldDefaultSpecification // TODO: After we get ApplicationTransactions, straighten this out. $desc = "updated field '{$label}'"; break; + case self::TYPE_USER: + case self::TYPE_USERS: + // TODO: As above, this is a mess that should get straightened out, + // but it will be easier after T2217. + $desc = "updated field '{$label}'"; + break; default: if (!strlen($old)) { if (!strlen($new)) { diff --git a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php index 6d1f8d89ad..7cc76ea8e5 100644 --- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php +++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php @@ -16,6 +16,7 @@ abstract class ManiphestAuxiliaryFieldSpecification private $user; private $task; private $markupEngine; + private $handles; public function setTask(ManiphestTask $task) { $this->task = $task; @@ -159,6 +160,25 @@ abstract class ManiphestAuxiliaryFieldSpecification return 'updated a custom field'; } + public function getRequiredHandlePHIDs() { + return array(); + } + + public function setHandles(array $handles) { + assert_instances_of($handles, 'PhabricatorObjectHandle'); + $this->handles = array_select_keys( + $handles, + $this->getRequiredHandlePHIDs()); + return $this; + } + + public function getHandle($phid) { + if (empty($this->handles[$phid])) { + throw new Exception( + "Field is requesting a handle ('{$phid}') it did not require."); + } + return $this->handles[$phid]; + } public function getMarkupFields() { return array(); diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 0533c75b47..d90643cc0c 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -35,6 +35,9 @@ final class ManiphestTaskDetailController extends ManiphestController { 'taskID = %d ORDER BY id ASC', $task->getID()); + $extensions = ManiphestTaskExtensions::newExtensions(); + $aux_fields = $extensions->loadFields($task, $user); + $e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT; $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK; $e_dep_by = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK; @@ -82,8 +85,18 @@ final class ManiphestTaskDetailController extends ManiphestController { } $phids = array_keys($phids); + + $phids = array_merge( + $phids, + array_mergev(mpull($aux_fields, 'getRequiredHandlePHIDs'))); + $this->loadHandles($phids); + $handles = $this->getLoadedHandles(); + foreach ($aux_fields as $aux_field) { + $aux_field->setHandles($handles); + } + $context_bar = null; if ($parent_task) { @@ -127,9 +140,6 @@ final class ManiphestTaskDetailController extends ManiphestController { } } - $extensions = ManiphestTaskExtensions::newExtensions(); - $aux_fields = $extensions->loadFields($task, $user); - foreach ($aux_fields as $aux_field) { foreach ($aux_field->getMarkupFields() as $markup_field) { $engine->addObject($aux_field, $markup_field); diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index ed96eb7453..10e1e1a20b 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -111,18 +111,16 @@ final class ManiphestTaskEditController extends ManiphestController { foreach ($aux_fields as $aux_field) { $aux_field->setValueFromRequest($request); - if ($aux_field->isRequired() && !strlen($aux_field->getValue())) { + if ($aux_field->isRequired() && !$aux_field->getValue()) { $errors[] = $aux_field->getLabel() . ' is required.'; $aux_field->setError('Required'); } - if (strlen($aux_field->getValue())) { - try { - $aux_field->validate(); - } catch (Exception $e) { - $errors[] = $e->getMessage(); - $aux_field->setError('Invalid'); - } + try { + $aux_field->validate(); + } catch (Exception $e) { + $errors[] = $e->getMessage(); + $aux_field->setError('Invalid'); } } @@ -283,7 +281,8 @@ final class ManiphestTaskEditController extends ManiphestController { $phids = array_merge( array($task->getOwnerPHID()), $task->getCCPHIDs(), - $task->getProjectPHIDs()); + $task->getProjectPHIDs(), + array_mergev(mpull($aux_fields, 'getRequiredHandlePHIDs'))); if ($parent_task) { $phids[] = $parent_task->getPHID(); @@ -294,6 +293,10 @@ final class ManiphestTaskEditController extends ManiphestController { $handles = $this->loadViewerHandles($phids); + foreach ($aux_fields as $aux_field) { + $aux_field->setHandles($handles); + } + $tvalues = mpull($handles, 'getFullName', 'getPHID'); $error_view = null; @@ -424,17 +427,15 @@ final class ManiphestTaskEditController extends ManiphestController { pht('Create New Project'))) ->setDatasource('/typeahead/common/projects/')); - if ($aux_fields) { - foreach ($aux_fields as $aux_field) { - if ($aux_field->isRequired() && - !$aux_field->getError() && - !$aux_field->getValue()) { - $aux_field->setError(true); - } - - $aux_control = $aux_field->renderControl(); - $form->appendChild($aux_control); + foreach ($aux_fields as $aux_field) { + if ($aux_field->isRequired() && + !$aux_field->getError() && + !$aux_field->getValue()) { + $aux_field->setError(true); } + + $aux_control = $aux_field->renderControl(); + $form->appendChild($aux_control); } require_celerity_resource('aphront-error-view-css'); diff --git a/src/docs/userguide/maniphest_custom.diviner b/src/docs/userguide/maniphest_custom.diviner index 45c079aaac..47dd7b32ab 100644 --- a/src/docs/userguide/maniphest_custom.diviner +++ b/src/docs/userguide/maniphest_custom.diviner @@ -44,8 +44,8 @@ Each array key must be unique, and is used to organize the internal storage of the field. These options are available: - **label**: Display label for the field on the edit and detail interfaces. - - **type**: Field type, one of **int**, **string**, **bool**, **select**, - **remarkup**, or **date**. + - **type**: Field type: one of **int**, **string**, **bool**, **select**, + **remarkup**, **user**, **users**, or **date**. - **caption**: A caption to display underneath the field (optional). - **required**: True if the user should be required to provide a value. - **options**: If type is set to **select**, provide options for the dropdown @@ -54,8 +54,10 @@ the field. These options are available: show next to the checkbox. - **checkbox-value**: If type is set to **bool**, the value to show on the detail view when the checkbox is selected. - - **default**: Default field value. For **date**, you can use a string like - `"July 4, 1990"`, `"5PM today"`, or any other valid input to `strtotime()`. + - **default**: Default field value. + - For **date**, you can use a string like `"July 4, 1990"`, `"5PM today"`, + or any other valid input to `strtotime()`. + - For **user** and **users**, you can use an array of user PHIDs. - **copy**: When a user creates a task, the UI gives them an option to "Create Another Similar Task". Some fields from the original task are copied into the new task, while others are not; by default, fields are not copied.