Tweak Maniphest custom fields

Summary:
  - Fix a bug where 'caption' didn't do anything.
  - Provide an abstract base implementation for extensions.
  - Add some documentation.
  - Expose aux fields via conduit.

Test Plan: Added some fields like "Dinosaur", "Kilograms" and "derp" on my local
install. Read documentation.
Reviewed By: jungejason
Reviewers: hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, philc, jungejason
Differential Revision: 785

conf/default.conf.php

@@ -453,12 +453,13 @@ return array(
 
   'maniphest.enabled' => true,
 
-  // Array of custom fields for Maniphest tasks. The array should contain
+  // Array of custom fields for Maniphest tasks. For details on adding custom
-  // arrays of field specifications keyed with 'type', 'label', 'caption',
+  // fields to Maniphest, see "Maniphest User Guide: Adding Custom Fields".
-  // 'required' and whatever specific options exist for the given field
-  // type.
   'maniphest.custom-fields' => array(),
 
+  // Class which drives custom field construction. See "Maniphest User Guide:
+  // Adding Custom Fields" in the documentation for more information.
+  'maniphest.custom-task-extensions-class' => 'ManiphestDefaultTaskExtensions',
 
 // -- Remarkup -------------------------------------------------------------- //
 

src/__phutil_library_map__.php

@@ -291,6 +291,7 @@ phutil_register_library_map(array(
     'ManiphestTaskDescriptionChangeController' => 'applications/maniphest/controller/descriptionchange',
     'ManiphestTaskDetailController' => 'applications/maniphest/controller/taskdetail',
     'ManiphestTaskEditController' => 'applications/maniphest/controller/taskedit',
+    'ManiphestTaskExtensions' => 'applications/maniphest/extensions/base',
     'ManiphestTaskListController' => 'applications/maniphest/controller/tasklist',
     'ManiphestTaskListView' => 'applications/maniphest/view/tasklist',
     'ManiphestTaskOwner' => 'applications/maniphest/constants/owner',
@@ -868,6 +869,7 @@ phutil_register_library_map(array(
     'ManiphestAuxiliaryFieldDefaultSpecification' => 'ManiphestAuxiliaryFieldSpecification',
     'ManiphestController' => 'PhabricatorController',
     'ManiphestDAO' => 'PhabricatorLiskDAO',
+    'ManiphestDefaultTaskExtensions' => 'ManiphestTaskExtensions',
     'ManiphestReplyHandler' => 'PhabricatorMailReplyHandler',
     'ManiphestTask' => 'ManiphestDAO',
     'ManiphestTaskAuxiliaryStorage' => 'ManiphestDAO',

src/applications/conduit/method/maniphest/info/ConduitAPI_maniphest_info_Method.php

@@ -49,6 +49,9 @@ class ConduitAPI_maniphest_info_Method extends ConduitAPIMethod {
       throw new ConduitException('ERR_BAD_TASK');
     }
 
+    $auxiliary = $task->loadAuxiliaryAttributes();
+    $auxiliary = mpull($auxiliary, 'getValue', 'getName');
+
     $result = array(
       'id'           => $task->getID(),
       'phid'         => $task->getPHID(),
@@ -63,6 +66,8 @@ class ConduitAPI_maniphest_info_Method extends ConduitAPIMethod {
       'projectPHIDs' => $task->getProjectPHIDs(),
       'uri'          => PhabricatorEnv::getProductionURI('/T'.$task->getID()),
 
+      'auxiliary'    => $auxiliary,
+
       // Not sure what this is yet.
       // 'attached' => array($task->getAttached()),
     );

src/applications/maniphest/auxiliaryfield/default/ManiphestAuxiliaryFieldDefaultSpecification.php

@@ -38,6 +38,7 @@ class ManiphestAuxiliaryFieldDefaultSpecification
 
   public function setFieldType($val) {
     $this->fieldType = $val;
+    return $this;
   }
 
   public function getError() {
@@ -46,6 +47,7 @@ class ManiphestAuxiliaryFieldDefaultSpecification
 
   public function setError($val) {
     $this->error = $val;
+    return $this;
   }
 
   public function getSelectOptions() {
@@ -54,10 +56,12 @@ class ManiphestAuxiliaryFieldDefaultSpecification
 
   public function setSelectOptions($array) {
     $this->selectOptions = $array;
+    return $this;
   }
 
   public function setRequired($bool) {
     $this->required = $bool;
+    return $this;
   }
 
   public function isRequired() {
@@ -91,6 +95,7 @@ class ManiphestAuxiliaryFieldDefaultSpecification
 
     $control->setValue($this->getValue());
     $control->setLabel($this->getLabel());
+    $control->setCaption($this->getCaption());
     $control->setName('auxiliary['.$this->getAuxiliaryKey().']');
     $control->setError($this->getError());
 
@@ -99,10 +104,7 @@ class ManiphestAuxiliaryFieldDefaultSpecification
 
   public function setValueFromRequest($request) {
     $aux_post_values = $request->getArr('auxiliary');
-
+    $this->setValue(idx($aux_post_values, $this->getAuxiliaryKey()));
-    $this->setValue(
-      $aux_post_values[$this->getAuxiliaryKey()]
-    );
   }
 
   public function getValueForStorage() {

src/applications/maniphest/auxiliaryfield/default/__init__.php

@@ -12,5 +12,7 @@ phutil_require_module('phabricator', 'applications/maniphest/auxiliaryfield/vali
 phutil_require_module('phabricator', 'view/form/control/select');
 phutil_require_module('phabricator', 'view/form/control/text');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('ManiphestAuxiliaryFieldDefaultSpecification.php');

src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php

@@ -47,8 +47,8 @@ class ManiphestTaskDetailController extends ManiphestController {
       $parent_task = id(new ManiphestTask())->load($workflow);
     }
 
-    $aux_fields = id(new ManiphestDefaultTaskExtensions())
+    $extensions = ManiphestTaskExtensions::newExtensions();
-      ->getAuxiliaryFieldSpecifications();
+    $aux_fields = $extensions->getAuxiliaryFieldSpecifications();
 
     $transactions = id(new ManiphestTransaction())->loadAllWhere(
       'taskID = %d ORDER BY id ASC',

src/applications/maniphest/controller/taskdetail/__init__.php

@@ -13,7 +13,7 @@ phutil_require_module('phabricator', 'applications/maniphest/constants/priority'
 phutil_require_module('phabricator', 'applications/maniphest/constants/status');
 phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
 phutil_require_module('phabricator', 'applications/maniphest/controller/base');
-phutil_require_module('phabricator', 'applications/maniphest/extensions/task');
+phutil_require_module('phabricator', 'applications/maniphest/extensions/base');
 phutil_require_module('phabricator', 'applications/maniphest/storage/task');
 phutil_require_module('phabricator', 'applications/maniphest/storage/transaction');
 phutil_require_module('phabricator', 'applications/maniphest/view/transactionlist');

src/applications/maniphest/controller/taskedit/ManiphestTaskEditController.php

@@ -85,8 +85,8 @@ class ManiphestTaskEditController extends ManiphestController {
     $errors = array();
     $e_title = true;
 
-    $aux_fields = id(new ManiphestDefaultTaskExtensions())
+    $extensions = ManiphestTaskExtensions::newExtensions();
-      ->getAuxiliaryFieldSpecifications();
+    $aux_fields = $extensions->getAuxiliaryFieldSpecifications();
 
     if ($request->isFormPost()) {
 
@@ -394,8 +394,7 @@ class ManiphestTaskEditController extends ManiphestController {
     $attributes = $task->loadAuxiliaryAttributes();
     $attributes = mpull($attributes, 'getValue', 'getName');
 
-    foreach ($aux_fields as $aux_field)
+    foreach ($aux_fields as $aux_field) {
-    {
       if (!$request->isFormPost()) {
         $attribute = null;
 

src/applications/maniphest/controller/taskedit/__init__.php

@@ -14,7 +14,7 @@ phutil_require_module('phabricator', 'applications/maniphest/constants/status');
 phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
 phutil_require_module('phabricator', 'applications/maniphest/controller/base');
 phutil_require_module('phabricator', 'applications/maniphest/editor/transaction');
-phutil_require_module('phabricator', 'applications/maniphest/extensions/task');
+phutil_require_module('phabricator', 'applications/maniphest/extensions/base');
 phutil_require_module('phabricator', 'applications/maniphest/storage/task');
 phutil_require_module('phabricator', 'applications/maniphest/storage/transaction');
 phutil_require_module('phabricator', 'applications/phid/constants');

src/applications/maniphest/extensions/base/ManiphestTaskExtensions.php

@@ -0,0 +1,37 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @group maniphest
+ */
+abstract class ManiphestTaskExtensions {
+
+  final public function __construct() {
+    // <empty>
+  }
+
+  abstract public function getAuxiliaryFieldSpecifications();
+
+
+  final public static function newExtensions() {
+    $key = 'maniphest.custom-task-extensions-class';
+    $class = PhabricatorEnv::getEnvConfig($key);
+    return newv($class, array());
+  }
+
+}

src/applications/maniphest/extensions/base/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'infrastructure/env');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('ManiphestTaskExtensions.php');

src/applications/maniphest/extensions/task/ManiphestDefaultTaskExtensions.php

@@ -19,7 +19,8 @@
 /**
  * @group maniphest
  */
-final class ManiphestDefaultTaskExtensions {
+final class ManiphestDefaultTaskExtensions
+  extends ManiphestTaskExtensions {
 
   public function getAuxiliaryFieldSpecifications() {
     $fields = PhabricatorEnv::getEnvConfig('maniphest.custom-fields');

src/applications/maniphest/extensions/task/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'applications/maniphest/auxiliaryfield/default');
+phutil_require_module('phabricator', 'applications/maniphest/extensions/base');
 phutil_require_module('phabricator', 'infrastructure/env');
 
 phutil_require_module('phutil', 'utils');

src/docs/userguide/maniphest_custom.diviner

@@ -0,0 +1,66 @@
+@title Maniphest User Guide: Adding Custom Fields
+@group userguide
+
+How to add custom fields to Maniphest.
+
+= Overview =
+
+Maniphest provides some support for adding new fields to tasks, like an
+"cost" field, a "milestone" field, etc.
+
+NOTE: Currently, these fields are somewhat limited. They primarily give you a
+structured way to record data on tasks, but there isn't much support for
+bringing them into other interfaces (e.g., querying by them, aggregating them,
+drawing graphs, etc.). If you have a use case, let us know what you want to do
+and maybe we can figure something out. This data is also exposed via the Conduit
+API, so you might be able to write your own interface if you want to do
+something very custom.
+
+= Simple Field Customization =
+
+If you don't need complicated display controls or sophisticated validation, you
+can add simple fields. These allow you to attach things like strings, numbers,
+and dropdown menus to the task template.
+
+Customize Maniphest fields by setting ##maniphest.custom-fields## in your
+configuration. For example, suppose you want to add "Estimated Hours" and
+"Actual Hours" fields. To do this, set your configuration like this:
+
+  'maniphest.custom-fields' => array(
+    'mycompany:estimated-hours' => array(
+      'label'     => 'Estimated Hours',
+      'type'      => 'int',
+      'caption'   => 'Estimated number of hours this will take.',
+      'required'  => false,
+    ),
+    'mycompany:actual-hours' => array(
+      'label'     => 'Actual Hours',
+      'type'      => 'int',
+      'required'  => false,
+    ),
+  )
+
+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** or **select**.
+  - **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
+    as a dictionary.
+
+= Advanced Field Customization =
+
+If you want to add fields with more specialized validation, storage, or
+rendering logic, you can do so with a little work:
+
+  - Extend @{class:ManiphestAuxiliaryFieldSpecification} and implement
+    your specialized rendering, validation, storage, etc., logic.
+  - Extend @{class:ManiphestTaskExtensions} and return a list of fields which
+    includes your custom field objects.
+  - Set 'maniphest.custom-extensions' to the name of your new extensions
+    class.
+
+This is relatively advanced but should give you significant flexibility in
+defining custom fields.