Integrate auxiliary fields into Maniphest transactions

Summary:
  - When changing auxiliary field values, use transactions.
  - Clean up some of the load/save logic for auxiliary fields so it's a little
more performant.

NOTE: The transaction display of auxiliary fields is incredibly hacky, I'll
follow up with a more nuanced approach but wanted to limit scope here.

Test Plan: Created and edited tasks with custom fields configured; created and
edited tasks without custom fields configured.

Reviewers: btrahan, jungejason, zeeg

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T418

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

src/applications/maniphest/constants/transactiontype/ManiphestTransactionType.php

@@ -32,6 +32,7 @@ final class ManiphestTransactionType extends ManiphestConstants {
 
   const TYPE_TITLE        = 'title';
   const TYPE_DESCRIPTION  = 'description';
+  const TYPE_AUXILIARY    = 'aux';
 
   public static function getTransactionTypeMap() {
     return array(

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

@@ -47,9 +47,6 @@ class ManiphestTaskDetailController extends ManiphestController {
       $parent_task = id(new ManiphestTask())->load($workflow);
     }
 
-    $extensions = ManiphestTaskExtensions::newExtensions();
-    $aux_fields = $extensions->getAuxiliaryFieldSpecifications();
-
     $transactions = id(new ManiphestTransaction())->loadAllWhere(
       'taskID = %d ORDER BY id ASC',
       $task->getID());
@@ -137,18 +134,14 @@ class ManiphestTaskDetailController extends ManiphestController {
       $dict['Projects'] = '<em>None</em>';
     }
 
+    $extensions = ManiphestTaskExtensions::newExtensions();
+    $aux_fields = $extensions->getAuxiliaryFieldSpecifications();
     if ($aux_fields) {
+      $task->loadAndAttachAuxiliaryAttributes();
       foreach ($aux_fields as $aux_field) {
-        $attribute = $task->loadAuxiliaryAttribute(
+        $aux_key = $aux_field->getAuxiliaryKey();
-          $aux_field->getAuxiliaryKey()
+        $aux_field->setValue($task->getAuxiliaryAttribute($aux_key));
-        );
-
-        if ($attribute) {
-          $aux_field->setValue($attribute->getValue());
-        }
-
         $value = $aux_field->renderForDetailView();
-
         if (strlen($value)) {
           $dict[$aux_field->getLabel()] = $value;
         }

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

@@ -200,8 +200,20 @@ class ManiphestTaskEditController extends ManiphestController {
           $transactions[] = $transaction;
         }
 
-        if ($transactions) {
+        if ($aux_fields) {
+          $task->loadAndAttachAuxiliaryAttributes();
+          foreach ($aux_fields as $aux_field) {
+            $transaction = clone $template;
+            $transaction->setTransactionType(
+              ManiphestTransactionType::TYPE_AUXILIARY);
+            $aux_key = $aux_field->getAuxiliaryKey();
+            $transaction->setMetadataValue('aux:key', $aux_key);
+            $transaction->setNewValue($aux_field->getValueForStorage());
+            $transactions[] = $transaction;
+          }
+        }
 
+        if ($transactions) {
           $event = new PhabricatorEvent(
             PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK,
             array(
@@ -220,14 +232,6 @@ class ManiphestTaskEditController extends ManiphestController {
           $editor->applyTransactions($task, $transactions);
         }
 
-        // TODO: Capture auxiliary field changes in a transaction
-        foreach ($aux_fields as $aux_field) {
-          $task->setAuxiliaryAttribute(
-            $aux_field->getAuxiliaryKey(),
-            $aux_field->getValueForStorage()
-          );
-        }
-
         if ($parent_task) {
           $type_task = PhabricatorPHIDConstants::PHID_TYPE_TASK;
 
@@ -413,28 +417,28 @@ class ManiphestTaskEditController extends ManiphestController {
               'Create New Project'))
           ->setDatasource('/typeahead/common/projects/'));
 
-    $attributes = $task->loadAuxiliaryAttributes();
+    if ($aux_fields) {
-    $attributes = mpull($attributes, 'getValue', 'getName');
+
+      if (!$request->isFormPost()) {
+        $task->loadAndAttachAuxiliaryAttributes();
+        foreach ($aux_fields as $aux_field) {
+          $aux_key = $aux_field->getAuxiliaryKey();
+          $value = $task->getAuxiliaryAttribute($aux_key);
+          $aux_field->setValueFromStorage($value);
+        }
+      }
 
       foreach ($aux_fields as $aux_field) {
-      if (!$request->isFormPost()) {
+        if ($aux_field->isRequired() &&
-        $attribute = null;
+            !$aux_field->getError() &&
-
+            !$aux_field->getValue()) {
-        if (isset($attributes[$aux_field->getAuxiliaryKey()])) {
-          $attribute = $attributes[$aux_field->getAuxiliaryKey()];
-          $aux_field->setValueFromStorage($attribute);
-        }
-      }
-
-      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');
 

src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php

@@ -72,6 +72,14 @@ class ManiphestTransactionEditor {
           $old = $task->getProjectPHIDs();
           $value_is_phid_set = true;
           break;
+        case ManiphestTransactionType::TYPE_AUXILIARY:
+          $aux_key = $transaction->getMetadataValue('aux:key');
+          if (!$aux_key) {
+            throw new Exception(
+              "Expected 'aux:key' metadata on TYPE_AUXILIARY transaction.");
+          }
+          $old = $task->getAuxiliaryAttribute($aux_key);
+          break;
         default:
           throw new Exception('Unknown action type.');
       }
@@ -150,6 +158,10 @@ class ManiphestTransactionEditor {
           case ManiphestTransactionType::TYPE_PROJECTS:
             $task->setProjectPHIDs($new);
             break;
+          case ManiphestTransactionType::TYPE_AUXILIARY:
+            $aux_key = $transaction->getMetadataValue('aux:key');
+            $task->setAuxiliaryAttribute($aux_key, $new);
+            break;
           default:
             throw new Exception('Unknown action type.');
         }

src/applications/maniphest/storage/task/ManiphestTask.php

@@ -41,6 +41,9 @@ class ManiphestTask extends ManiphestDAO {
 
   protected $ownerOrdering;
 
+  private $auxiliaryAttributes;
+  private $auxiliaryDirty = array();
+
   public function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,
@@ -87,44 +90,48 @@ class ManiphestTask extends ManiphestDAO {
     return $this;
   }
 
+  public function getAuxiliaryAttribute($key, $default = null) {
+    if ($this->auxiliaryAttributes === null) {
+      throw new Exception("Attach auxiliary attributes before getting them!");
+    }
+    return idx($this->auxiliaryAttributes, $key, $default);
+  }
+
   public function setAuxiliaryAttribute($key, $val) {
-    $this->removeAuxiliaryAttribute($key);
+    if ($this->auxiliaryAttributes === null) {
-
+      throw new Exception("Attach auxiliary attributes before setting them!");
-    $attribute = new ManiphestTaskAuxiliaryStorage();
+    }
-    $attribute->setTaskPHID($this->phid);
+    $this->auxiliaryAttributes[$key] = $val;
-    $attribute->setName($key);
+    $this->auxiliaryDirty[$key] = true;
-    $attribute->setValue($val);
+    return $this;
-    $attribute->save();
   }
 
-  public function loadAuxiliaryAttribute($key) {
+  public function attachAuxiliaryAttributes(array $attrs) {
-    $attribute = id(new ManiphestTaskAuxiliaryStorage())->loadOneWhere(
+    if ($this->auxiliaryDirty) {
-      'taskPHID = %s AND name = %s',
+      throw new Exception(
-      $this->getPHID(),
+        "This object has dirty attributes, you can not attach new attributes ".
-      $key);
+        "without writing or discarding the dirty attributes.");
-
+    }
-    return $attribute;
+    $this->auxiliaryAttributes = $attrs;
+    return $this;
   }
 
-  public function removeAuxiliaryAttribute($key) {
+  public function loadAndAttachAuxiliaryAttributes() {
-    $attribute = id(new ManiphestTaskAuxiliaryStorage())->loadOneWhere(
+    if (!$this->getPHID()) {
-      'taskPHID = %s AND name = %s',
+      $this->auxiliaryAttributes = array();
-      $this->getPHID(),
+      return;
-      $key);
-
-    if ($attribute) {
-      $attribute->delete();
-    }
     }
 
-  public function loadAuxiliaryAttributes() {
+    $storage = id(new ManiphestTaskAuxiliaryStorage())->loadAllWhere(
-    $attributes = id(new ManiphestTaskAuxiliaryStorage())->loadAllWhere(
       'taskPHID = %s',
       $this->getPHID());
 
-    return $attributes;
+    $this->auxiliaryAttributes = mpull($storage, 'getValue', 'getName');
+
+    return $this;
   }
 
+
   public function save() {
     if (!$this->mailKey) {
       $this->mailKey = Filesystem::readRandomCharacters(20);
@@ -146,7 +153,55 @@ class ManiphestTask extends ManiphestDAO {
       $this->subscribersNeedUpdate = false;
     }
 
+    if ($this->auxiliaryDirty) {
+      $this->writeAuxiliaryUpdates();
+      $this->auxiliaryDirty = array();
+    }
+
     return $result;
   }
 
+  private function writeAuxiliaryUpdates() {
+    $table = new ManiphestTaskAuxiliaryStorage();
+    $conn_w = $table->establishConnection('w');
+    $update = array();
+    $remove = array();
+
+    foreach ($this->auxiliaryDirty as $key => $dirty) {
+      $value = $this->getAuxiliaryAttribute($key);
+      if ($value === null) {
+        $remove[$key] = true;
+      } else {
+        $update[$key] = $value;
+      }
+    }
+
+    if ($remove) {
+      queryfx(
+        $conn_w,
+        'DELETE FROM %T WHERE taskPHID = %s AND name IN (%Ls)',
+        $table->getTableName(),
+        $this->getPHID(),
+        array_keys($remove));
+    }
+
+    if ($update) {
+      $sql = array();
+      foreach ($update as $key => $val) {
+        $sql[] = qsprintf(
+          $conn_w,
+          '(%s, %s, %s)',
+          $this->getPHID(),
+          $key,
+          $val);
+      }
+      queryfx(
+        $conn_w,
+        'INSERT INTO %T (taskPHID, name, value) VALUES %Q
+          ON DUPLICATE KEY UPDATE value = VALUES(value)',
+        $table->getTableName(),
+        implode(', ', $sql));
+    }
+  }
+
 }

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

@@ -12,6 +12,8 @@ phutil_require_module('phabricator', 'applications/maniphest/storage/subscriber'
 phutil_require_module('phabricator', 'applications/maniphest/storage/taskproject');
 phutil_require_module('phabricator', 'applications/phid/constants');
 phutil_require_module('phabricator', 'applications/phid/storage/phid');
+phutil_require_module('phabricator', 'storage/qsprintf');
+phutil_require_module('phabricator', 'storage/queryfx');
 
 phutil_require_module('phutil', 'filesystem');
 phutil_require_module('phutil', 'utils');

src/applications/maniphest/storage/transaction/ManiphestTransaction.php

@@ -81,6 +81,21 @@ class ManiphestTransaction extends ManiphestDAO {
     return $phids;
   }
 
+  public function getMetadataValue($key, $default = null) {
+    if (!is_array($this->metadata)) {
+      return $default;
+    }
+    return idx($this->metadata, $key, $default);
+  }
+
+  public function setMetadataValue($key, $value) {
+    if (!is_array($this->metadata)) {
+      $this->metadata = array();
+    }
+    $this->metadata[$key] = $value;
+    return $this;
+  }
+
   public function canGroupWith($target) {
     if ($target->getAuthorPHID() != $this->getAuthorPHID()) {
       return false;
@@ -95,8 +110,17 @@ class ManiphestTransaction extends ManiphestDAO {
     }
 
     if ($target->getTransactionType() == $this->getTransactionType()) {
+      $aux_type = ManiphestTransactionType::TYPE_AUXILIARY;
+      if ($this->getTransactionType() == $aux_type) {
+        $that_key = $target->getMetadataValue('aux:key');
+        $this_key = $this->getMetadataValue('aux:key');
+        if ($that_key == $this_key) {
           return false;
         }
+      } else {
+        return false;
+      }
+    }
 
     return true;
   }

src/applications/maniphest/storage/transaction/__init__.php

@@ -10,5 +10,7 @@ phutil_require_module('phabricator', 'applications/maniphest/constants/transacti
 phutil_require_module('phabricator', 'applications/maniphest/storage/base');
 phutil_require_module('phabricator', 'applications/metamta/contentsource/source');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('ManiphestTransaction.php');

src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php

@@ -528,6 +528,28 @@ class ManiphestTransactionDetailView extends ManiphestView {
           $desc = 'changed attached '.$plural.', added: '.$add_desc.'; '.
                                               'removed: '.$rem_desc;
         }
+        break;
+      case ManiphestTransactionType::TYPE_AUXILIARY:
+
+        // TODO: This is temporary and hacky! Allow auxiliary fields to
+        // customize this.
+
+        $old_esc = phutil_escape_html($old);
+        $new_esc = phutil_escape_html($new);
+
+        $aux_key = $transaction->getMetadataValue('aux:key');
+        if ($old === null) {
+          $verb = "Set Field";
+          $desc = "set field '{$aux_key}' to '{$new_esc}'";
+        } else if ($new === null) {
+          $verb = "Removed Field";
+          $desc = "removed field '{$aux_key}'";
+        } else {
+          $verb = "Updated Field";
+          $desc = "updated field '{$aux_key}' ".
+                  "from '{$old_esc}' to '{$new_esc}'";
+        }
+
         break;
       default:
         return array($type, ' brazenly '.$type."'d", $classes);