Convert maniphest to use standard fields

Summary: Ref T3794. Drop auxiliary field, use standard field.

Test Plan: Performed migration, field seemed to survive it intact. Edited and viewed tasks.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3794

Differential Revision: https://secure.phabricator.com/D7036

resources/sql/patches/20130919.mfieldconf.php

@@ -0,0 +1,66 @@
+<?php
+
+echo "Migrating Maniphest custom field configuration...\n";
+
+$old_key = 'maniphest.custom-fields';
+$new_key = 'maniphest.custom-field-definitions';
+
+if (PhabricatorEnv::getEnvConfig($new_key)) {
+  echo "Skipping migration, new data is already set.\n";
+  return;
+}
+
+$old = PhabricatorEnv::getEnvConfigIfExists($old_key);
+if (!$old) {
+  echo "Skipping migration, old data does not exist.\n";
+  return;
+}
+
+$new = array();
+foreach ($old as $field_key => $spec) {
+  $new_spec = array();
+
+  foreach ($spec as $key => $value) {
+    switch ($key) {
+      case 'label':
+        $new_spec['name'] = $value;
+        break;
+      case 'required':
+      case 'default':
+      case 'caption':
+      case 'options':
+        $new_spec[$key] = $value;
+        break;
+      case 'checkbox-label':
+        $new_spec['strings']['edit.checkbox'] = $value;
+        break;
+      case 'checkbox-value':
+        $new_spec['strings']['view.yes'] = $value;
+        break;
+      case 'type':
+        switch ($value) {
+          case 'string':
+            $value = 'text';
+            break;
+          case 'user':
+            $value = 'users';
+            $new_spec['limit'] = 1;
+            break;
+        }
+        $new_spec['type'] = $value;
+        break;
+      case 'copy':
+        $new_spec['copy'] = $value;
+        break;
+    }
+  }
+
+  $new[$field_key] = $new_spec;
+}
+
+PhabricatorConfigEntry::loadConfigEntry($new_key)
+  ->setIsDeleted(0)
+  ->setValue($new)
+  ->save();
+
+echo "Done.\n";

src/__phutil_library_map__.php

@@ -688,6 +688,7 @@ phutil_register_library_map(array(
     'ManiphestAuxiliaryFieldDefaultSpecification' => 'applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php',
     'ManiphestAuxiliaryFieldSpecification' => 'applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php',
     'ManiphestBatchEditController' => 'applications/maniphest/controller/ManiphestBatchEditController.php',
+    'ManiphestConfiguredCustomField' => 'applications/maniphest/field/ManiphestConfiguredCustomField.php',
     'ManiphestConstants' => 'applications/maniphest/constants/ManiphestConstants.php',
     'ManiphestController' => 'applications/maniphest/controller/ManiphestController.php',
     'ManiphestCreateMailReceiver' => 'applications/maniphest/mail/ManiphestCreateMailReceiver.php',
@@ -2765,6 +2766,11 @@ phutil_register_library_map(array(
       1 => 'PhabricatorMarkupInterface',
     ),
     'ManiphestBatchEditController' => 'ManiphestController',
+    'ManiphestConfiguredCustomField' =>
+    array(
+      0 => 'ManiphestCustomField',
+      1 => 'PhabricatorStandardCustomFieldInterface',
+    ),
     'ManiphestController' => 'PhabricatorController',
     'ManiphestCreateMailReceiver' => 'PhabricatorMailReceiver',
     'ManiphestCustomField' => 'PhabricatorCustomField',

src/applications/config/check/PhabricatorSetupCheckExtraConfig.php

@@ -150,6 +150,11 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck {
         pht(
           'Maniphest fields are now loaded automatically. You can configure '.
           'them with `maniphest.fields`.'),
+      'maniphest.custom-fields' =>
+        pht(
+          'Maniphest fields are now defined in '.
+          '`maniphest.custom-field-definitions`. Existing definitions have '.
+          'been migrated.'),
     );
 
     return $ancient_config;

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php

@@ -26,32 +26,7 @@ class ManiphestAuxiliaryFieldDefaultSpecification
   const TYPE_HEADER   = 'header';
 
   public function createFields() {
-    $fields = PhabricatorEnv::getEnvConfig('maniphest.custom-fields');
+    return array();
-    $specs = array();
-    foreach ($fields as $aux => $info) {
-      $spec = new ManiphestAuxiliaryFieldDefaultSpecification();
-      $spec->setAuxiliaryKey($aux);
-      $spec->setLabel(idx($info, 'label'));
-      $spec->setCaption(idx($info, 'caption'));
-      $spec->setFieldType(idx($info, 'type'));
-      $spec->setRequired(idx($info, 'required'));
-
-      $spec->setCheckboxLabel(idx($info, 'checkbox-label'));
-      $spec->setCheckboxValue(idx($info, 'checkbox-value', 1));
-
-      if ($spec->getFieldType() ==
-        ManiphestAuxiliaryFieldDefaultSpecification::TYPE_SELECT) {
-        $spec->setSelectOptions(idx($info, 'options'));
-      }
-
-      $spec->setShouldCopyWhenCreatingSimilarTask(idx($info, 'copy'));
-
-      $spec->setDefaultValue(idx($info, 'default'));
-
-      $specs[] = $spec;
-    }
-
-    return $specs;
   }
 
   public function getFieldType() {

src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php

@@ -59,7 +59,7 @@ final class PhabricatorManiphestConfigOptions
     $custom_field_type = 'custom:PhabricatorCustomFieldConfigOptionType';
 
     return array(
-      $this->newOption('maniphest.custom-fields', 'wild', array())
+      $this->newOption('maniphest.custom-field-definitions', 'wild', array())
         ->setSummary(pht("Custom Maniphest fields."))
         ->setDescription(
           pht(
@@ -67,9 +67,9 @@ final class PhabricatorManiphestConfigOptions
             "adding custom fields to Maniphest, see 'Maniphest User Guide: ".
             "Adding Custom Fields'."))
         ->addExample(
-          '{"mycompany:estimated-hours": {"label": "Estimated Hours", '.
+          '{"mycompany:estimated-hours": {"name": "Estimated Hours", '.
           '"type": "int", "caption": "Estimated number of hours this will '.
-          'take.", "required": false}}',
+          'take."}}',
           pht('Valid Setting')),
       $this->newOption('maniphest.fields', $custom_field_type, $default_fields)
         ->setCustomData(id(new ManiphestTask())->getCustomFieldBaseClass())

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -229,9 +229,21 @@ final class ManiphestTaskEditController extends ManiphestController {
               ManiphestTransactionType::TYPE_AUXILIARY);
             $aux_key = $aux_field->getFieldKey();
             $transaction->setMetadataValue('aux:key', $aux_key);
-            $transaction->setOldValue(idx($old_values, $aux_key));
+            $old = idx($old_values, $aux_key);
-            $transaction->setNewValue(
+            $new = $aux_field->getNewValueForApplicationTransactions();
-              $aux_field->getNewValueForApplicationTransactions());
+
+            // TODO: This is a ghetto check for transactions with no effect.
+            if (!is_array($old) && !is_array($new)) {
+              if ((string)$old === (string)$new) {
+                continue;
+              }
+            } else if ($old == $new) {
+              continue;
+            }
+
+            $transaction->setOldValue($old);
+            $transaction->setNewValue($new);
+
             $transactions[] = $transaction;
           }
         }

src/applications/maniphest/field/ManiphestConfiguredCustomField.php

@@ -0,0 +1,22 @@
+<?php
+
+final class ManiphestConfiguredCustomField
+  extends ManiphestCustomField
+  implements PhabricatorStandardCustomFieldInterface {
+
+  public function getStandardCustomFieldNamespace() {
+    return 'maniphest';
+  }
+
+  public function createFields() {
+    $config = PhabricatorEnv::getEnvConfig(
+      'maniphest.custom-field-definitions',
+      array());
+    $fields = PhabricatorStandardCustomField::buildStandardFields(
+      $this,
+      $config);
+
+    return $fields;
+  }
+
+}

src/applications/maniphest/field/ManiphestCustomField.php

@@ -81,12 +81,17 @@ abstract class ManiphestCustomField
   public function renderTransactionDescription(
     ManiphestTransaction $transaction,
     $target) {
-    return 'updated a custom field';
+    $old = $transaction->getOldValue();
+    $new = $transaction->getNewValue();
+    return pht(
+      'updated field %s from %s to %s',
+      $this->getFieldName(),
+      $old,
+      $new);
   }
 
   public function getMarkupFields() {
     return array();
   }
 
-
 }

src/docs/user/userguide/maniphest_custom.diviner

@@ -22,36 +22,34 @@ 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
+Customize Maniphest fields by setting `maniphest.custom-field-definitions` in
-configuration. For example, suppose you want to add "Estimated Hours" and
+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',
+      'name'     => 'Estimated Hours',
       'type'      => 'int',
       'caption'   => 'Estimated number of hours this will take.',
-      'required'  => false,
+      'required'  => true,
     ),
     'mycompany:actual-hours' => array(
-      'label'     => 'Actual Hours',
+      'name'     => '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.
+  - **name**: Display label for the field on the edit and detail interfaces.
   - **type**: Field type. The supported field types are:
     - **int**: An integer, rendered as a text field.
-    - **string**: A string, rendered as a text field.
+    - **text**: A string, rendered as a text field.
     - **bool**: A boolean value, rendered as a checkbox.
     - **select**: Allows the user to select from several options, rendered
       as a dropdown.
     - **remarkup**: A text area which allows the user to enter markup.
-    - **user**: A single user typeahead.
     - **users**: A typeahead which allows multiple users to be input.
     - **date**: A date/time picker.
     - **header**: Renders a visual divider which you can use to group fields.
@@ -59,14 +57,7 @@ the field. These options are available:
   - **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.
-  - **checkbox-label**: If type is set to **bool**, an optional string to
-    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()`.
-    - 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.

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1604,6 +1604,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type' => 'php',
         'name' => $this->getPatchPath('20130915.maniphestmigrate.php'),
       ),
+      '20130919.mfieldconf.php' => array(
+        'type' => 'php',
+        'name' => $this->getPatchPath('20130919.mfieldconf.php'),
+      ),
     );
   }
 }