Route Maniphest editing and transactions primarily through shared code

Summary: Ref T418. Although Maniphest does not use ApplicationTransactions, we can fake a lot of it and provide a more uniform API. Deletes as much custom code from Maniphest as possible along the edit workflows, using core code instead.

Test Plan:
With custom fields:

  - Edited a task.
  - Created a task.
  - Queried a task with Maniphest.
  - Updated a task with Maniphest.
  - Used `?template=nnn` to create a similar task.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418

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

src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php

@@ -89,20 +89,6 @@ abstract class ManiphestAuxiliaryFieldSpecification
   }
 
 
-  /**
-   * When the user creates a task, the UI prompts them to "Create another
-   * similar task". This copies some fields (e.g., Owner and CCs) but not other
-   * fields (e.g., description). If this custom field should also be copied,
-   * return true from this method.
-   *
-   * @return bool True to copy the default value from the template task when
-   *              creating a new similar task.
-   */
-  public function shouldCopyWhenCreatingSimilarTask() {
-    return false;
-  }
-
-
   /**
    * Render a verb to appear in email titles when a transaction involving this
    * field occurs. Specifically, Maniphest emails are formatted like this:
@@ -247,23 +233,56 @@ abstract class ManiphestAuxiliaryFieldSpecification
     return $this->getLabel();
   }
 
+  public function readValueFromRequest(AphrontRequest $request) {
+    return $this->setValueFromRequest($request);
+  }
 
-/* -(  Legacy Migration Support  )------------------------------------------- */
+  public static function writeLegacyAuxiliaryUpdates(
-
-
-  // TODO: Migrate to common storage and remove this.
-  public static function loadLegacyDataFromStorage(
     ManiphestTask $task,
-    PhabricatorCustomFieldList $list) {
+    array $map) {
 
-    $task->loadAndAttachAuxiliaryAttributes();
+    $table = new ManiphestCustomFieldStorage();
+    $conn_w = $table->establishConnection('w');
+    $update = array();
+    $remove = array();
 
-    foreach ($list->getFields() as $field) {
+    foreach ($map as $key => $value) {
-      if ($task->getID()) {
+      $index = PhabricatorHash::digestForIndex($key);
-        $key = $field->getAuxiliaryKey();
+      if ($value === null) {
-        $field->setValueFromStorage($task->getAuxiliaryAttribute($key));
+        $remove[$index] = true;
+      } else {
+        $update[$index] = $value;
       }
     }
+
+    if ($remove) {
+      queryfx(
+        $conn_w,
+        'DELETE FROM %T WHERE objectPHID = %s AND fieldIndex IN (%Ls)',
+        $table->getTableName(),
+        $task->getPHID(),
+        array_keys($remove));
+    }
+
+    if ($update) {
+      $sql = array();
+      foreach ($update as $index => $val) {
+        $sql[] = qsprintf(
+          $conn_w,
+          '(%s, %s, %s)',
+          $task->getPHID(),
+          $index,
+          $val);
+      }
+      queryfx(
+        $conn_w,
+        'INSERT INTO %T (objectPHID, fieldIndex, fieldValue)
+          VALUES %Q ON DUPLICATE KEY
+          UPDATE fieldValue = VALUES(fieldValue)',
+        $table->getTableName(),
+        implode(', ', $sql));
+    }
+
   }
 
 }

src/applications/maniphest/conduit/ConduitAPI_maniphest_Method.php

@@ -157,19 +157,31 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod {
       $transactions[] = $transaction;
     }
 
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $task,
+      PhabricatorCustomField::ROLE_EDIT);
+
     $auxiliary = $request->getValue('auxiliary');
     if ($auxiliary) {
-      $task->loadAndAttachAuxiliaryAttributes();
+      foreach ($field_list->getFields() as $key => $field) {
-      foreach ($auxiliary as $aux_key => $aux_value) {
+        if (!array_key_exists($key, $auxiliary)) {
+          continue;
+        }
         $transaction = clone $template;
         $transaction->setTransactionType(
           ManiphestTransactionType::TYPE_AUXILIARY);
-        $transaction->setMetadataValue('aux:key', $aux_key);
+        $transaction->setMetadataValue('aux:key', $key);
-        $transaction->setNewValue($aux_value);
+        $transaction->setOldValue(
+          $field->getOldValueForApplicationTransactions());
+        $transaction->setNewValue($auxiliary[$key]);
         $transactions[] = $transaction;
       }
     }
 
+    if (!$transactions) {
+      return;
+    }
+
     $event = new PhabricatorEvent(
       PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK,
       array(
@@ -186,6 +198,7 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod {
 
     $editor = new ManiphestTransactionEditor();
     $editor->setActor($request->getUser());
+    $editor->setAuxiliaryFields($field_list->getFields());
     $editor->applyTransactions($task, $transactions);
 
     $event = new PhabricatorEvent(
@@ -217,7 +230,15 @@ abstract class ConduitAPI_maniphest_Method extends ConduitAPIMethod {
     $result = array();
     foreach ($tasks as $task) {
       // TODO: Batch this get as CustomField gets cleaned up.
-      $auxiliary = $task->loadLegacyAuxiliaryFieldMap();
+      $field_list = PhabricatorCustomField::getObjectFields(
+        $task,
+        PhabricatorCustomField::ROLE_EDIT);
+      $field_list->readFieldsFromStorage($task);
+
+      $auxiliary = mpull(
+        $field_list->getFields(),
+        'getValueForStorage',
+        'getFieldKey');
 
       $task_deps = $all_deps->getDestinationPHIDs(
         array($task->getPHID()),

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -93,9 +93,7 @@ final class ManiphestTaskEditController extends ManiphestController {
       $field->setViewer($user);
     }
 
-    ManiphestAuxiliaryFieldSpecification::loadLegacyDataFromStorage(
+    $field_list->readFieldsFromStorage($task);
-      $task,
-      $field_list);
 
     $aux_fields = $field_list->getFields();
 
@@ -135,12 +133,17 @@ final class ManiphestTaskEditController extends ManiphestController {
         $errors[] = pht('Title is required.');
       }
 
+      $old_values = array();
       foreach ($aux_fields as $aux_arr_key => $aux_field) {
-        $aux_field->setValueFromRequest($request);
+        // TODO: This should be buildFieldTransactionsFromRequest() once we
-        $aux_key = $aux_field->getAuxiliaryKey();
+        // switch to ApplicationTransactions properly.
-        $aux_old_value = $task->getAuxiliaryAttribute($aux_key);
 
-        if ((int)$aux_old_value === $aux_field->getValueForStorage()) {
+        $aux_old_value = $aux_field->getOldValueForApplicationTransactions();
+        $aux_field->readValueFromRequest($request);
+        $aux_new_value = $aux_field->getNewValueForApplicationTransactions();
+
+        // TODO: What's going on here?
+        if ((int)$aux_old_value === $aux_new_value) {
           unset($aux_fields[$aux_arr_key]);
           continue;
         }
@@ -156,6 +159,8 @@ final class ManiphestTaskEditController extends ManiphestController {
           $errors[] = $e->getMessage();
           $aux_field->setError(pht('Invalid'));
         }
+
+        $old_values[$aux_field->getFieldKey()] = $aux_old_value;
       }
 
       if ($errors) {
@@ -220,9 +225,11 @@ final class ManiphestTaskEditController extends ManiphestController {
             $transaction = clone $template;
             $transaction->setTransactionType(
               ManiphestTransactionType::TYPE_AUXILIARY);
-            $aux_key = $aux_field->getAuxiliaryKey();
+            $aux_key = $aux_field->getFieldKey();
             $transaction->setMetadataValue('aux:key', $aux_key);
-            $transaction->setNewValue($aux_field->getValueForStorage());
+            $transaction->setOldValue(idx($old_values, $aux_key));
+            $transaction->setNewValue(
+              $aux_field->getNewValueForApplicationTransactions());
             $transactions[] = $transaction;
           }
         }
@@ -302,16 +309,27 @@ final class ManiphestTaskEditController extends ManiphestController {
             $task->setOwnerPHID($template_task->getOwnerPHID());
             $task->setPriority($template_task->getPriority());
 
-            if ($aux_fields) {
+            $template_fields = PhabricatorCustomField::getObjectFields(
-              $template_task->loadAndAttachAuxiliaryAttributes();
+              $template_task,
-              foreach ($aux_fields as $aux_field) {
+              PhabricatorCustomField::ROLE_EDIT);
-                if (!$aux_field->shouldCopyWhenCreatingSimilarTask()) {
+
-                  continue;
+            $fields = $template_fields->getFields();
+            foreach ($fields as $key => $field) {
+              if (!$field->shouldCopyWhenCreatingSimilarTask()) {
+                unset($fields[$key]);
+              }
+              if (empty($aux_fields[$key])) {
+                unset($fields[$key]);
+              }
             }
 
-                $aux_key = $aux_field->getAuxiliaryKey();
+            if ($fields) {
-                $value = $template_task->getAuxiliaryAttribute($aux_key);
+              id(new PhabricatorCustomFieldList($fields))
-                $aux_field->setValueFromStorage($value);
+                ->readFieldsFromStorage($template_task);
+
+              foreach ($fields as $key => $field) {
+                $aux_fields[$key]->setValueFromStorage(
+                  $field->getValueForStorage());
               }
             }
           }

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -9,7 +9,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
   private $auxiliaryFields = array();
 
   public function setAuxiliaryFields(array $fields) {
-    assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification');
+    assert_instances_of($fields, 'ManiphestCustomField');
     $this->auxiliaryFields = $fields;
     return $this;
   }
@@ -28,6 +28,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
     $email_to[] = $task->getOwnerPHID();
 
     $pri_changed = $this->isCreate($transactions);
+    $aux_writes = array();
 
     foreach ($transactions as $key => $transaction) {
       $type = $transaction->getTransactionType();
@@ -75,7 +76,8 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
             throw new Exception(
               "Expected 'aux:key' metadata on TYPE_AUXILIARY transaction.");
           }
-          $old = $task->getAuxiliaryAttribute($aux_key);
+          // This has already been populated.
+          $old = $transaction->getOldValue();
           break;
         default:
           throw new Exception('Unknown action type.');
@@ -160,7 +162,7 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
             break;
           case ManiphestTransactionType::TYPE_AUXILIARY:
             $aux_key = $transaction->getMetadataValue('aux:key');
-            $task->setAuxiliaryAttribute($aux_key, $new);
+            $aux_writes[$aux_key] = $new;
             break;
           case ManiphestTransactionType::TYPE_EDGE:
             // Edge edits are accomplished through PhabricatorEdgeEditor, which
@@ -184,6 +186,13 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
     }
 
     $task->save();
+
+    if ($aux_writes) {
+      ManiphestAuxiliaryFieldSpecification::writeLegacyAuxiliaryUpdates(
+        $task,
+        $aux_writes);
+    }
+
     foreach ($transactions as $transaction) {
       $transaction->setTaskID($task->getID());
       $transaction->save();

src/applications/maniphest/field/ManiphestCustomField.php

@@ -15,4 +15,18 @@ abstract class ManiphestCustomField
     return new ManiphestCustomFieldNumericIndex();
   }
 
+  /**
+   * When the user creates a task, the UI prompts them to "Create another
+   * similar task". This copies some fields (e.g., Owner and CCs) but not other
+   * fields (e.g., description). If this custom field should also be copied,
+   * return true from this method.
+   *
+   * @return bool True to copy the default value from the template task when
+   *              creating a new similar task.
+   */
+  public function shouldCopyWhenCreatingSimilarTask() {
+    return false;
+  }
+
+
 }

src/applications/maniphest/storage/ManiphestTask.php

@@ -35,8 +35,6 @@ final class ManiphestTask extends ManiphestDAO
 
   protected $ownerOrdering;
 
-  private $auxiliaryAttributes = self::ATTACHABLE;
-  private $auxiliaryDirty = array();
   private $groupByProjectPHID = self::ATTACHABLE;
   private $customFields = self::ATTACHABLE;
 
@@ -97,19 +95,6 @@ final class ManiphestTask extends ManiphestDAO
     return $this;
   }
 
-  public function getAuxiliaryAttribute($key, $default = null) {
-    $this->assertAttached($this->auxiliaryAttributes);
-    return idx($this->auxiliaryAttributes, $key, $default);
-  }
-
-  public function setAuxiliaryAttribute($key, $val) {
-    $this->assertAttached($this->auxiliaryAttributes);
-
-    $this->auxiliaryAttributes[$key] = $val;
-    $this->auxiliaryDirty[$key] = true;
-    return $this;
-  }
-
   public function setTitle($title) {
     $this->title = $title;
     if (!$this->getID()) {
@@ -127,41 +112,6 @@ final class ManiphestTask extends ManiphestDAO
     return $this->assertAttached($this->groupByProjectPHID);
   }
 
-  public function attachAuxiliaryAttributes(array $attrs) {
-    if ($this->auxiliaryDirty) {
-      throw new Exception(
-        "This object has dirty attributes, you can not attach new attributes ".
-        "without writing or discarding the dirty attributes.");
-    }
-    $this->auxiliaryAttributes = $attrs;
-    return $this;
-  }
-
-  public function loadLegacyAuxiliaryFieldMap() {
-    $field_list = PhabricatorCustomField::getObjectFields(
-      $this,
-      PhabricatorCustomField::ROLE_EDIT);
-    $field_list->readFieldsFromStorage($this);
-
-    $map = array();
-    foreach ($field_list->getFields() as $field) {
-      $map[$field->getFieldKey()] = $field->getValueForStorage();
-    }
-
-    return $map;
-  }
-
-  public function loadAndAttachAuxiliaryAttributes() {
-    if (!$this->getPHID()) {
-      $this->auxiliaryAttributes = array();
-      return $this;
-    }
-
-    $this->auxiliaryAttributes = $this->loadLegacyAuxiliaryFieldMap();
-
-    return $this;
-  }
-
   public function save() {
     if (!$this->mailKey) {
       $this->mailKey = Filesystem::readRandomCharacters(20);
@@ -183,59 +133,9 @@ final class ManiphestTask extends ManiphestDAO
       $this->subscribersNeedUpdate = false;
     }
 
-    if ($this->auxiliaryDirty) {
-      $this->writeAuxiliaryUpdates();
-      $this->auxiliaryDirty = array();
-    }
-
     return $result;
   }
 
-  private function writeAuxiliaryUpdates() {
-    $table = new ManiphestCustomFieldStorage();
-    $conn_w = $table->establishConnection('w');
-    $update = array();
-    $remove = array();
-
-    foreach ($this->auxiliaryDirty as $key => $dirty) {
-      $value = $this->getAuxiliaryAttribute($key);
-
-      $index = PhabricatorHash::digestForIndex($key);
-      if ($value === null) {
-        $remove[$index] = true;
-      } else {
-        $update[$index] = $value;
-      }
-    }
-
-    if ($remove) {
-      queryfx(
-        $conn_w,
-        'DELETE FROM %T WHERE objectPHID = %s AND fieldIndex IN (%Ls)',
-        $table->getTableName(),
-        $this->getPHID(),
-        array_keys($remove));
-    }
-
-    if ($update) {
-      $sql = array();
-      foreach ($update as $index => $val) {
-        $sql[] = qsprintf(
-          $conn_w,
-          '(%s, %s, %s)',
-          $this->getPHID(),
-          $index,
-          $val);
-      }
-      queryfx(
-        $conn_w,
-        'INSERT INTO %T (objectPHID, fieldIndex, fieldValue)
-          VALUES %Q ON DUPLICATE KEY
-          UPDATE fieldValue = VALUES(fieldValue)',
-        $table->getTableName(),
-        implode(', ', $sql));
-    }
-  }
 
 
 /* -(  Markup Interface  )--------------------------------------------------- */

src/applications/maniphest/view/ManiphestTransactionDetailView.php

@@ -19,7 +19,7 @@ final class ManiphestTransactionDetailView extends ManiphestView {
   private $auxiliaryFields;
 
   public function setAuxiliaryFields(array $fields) {
-    assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification');
+    assert_instances_of($fields, 'ManiphestCustomField');
     $this->auxiliaryFields = mpull($fields, null, 'getAuxiliaryKey');
     return $this;
   }

src/applications/maniphest/view/ManiphestTransactionListView.php

@@ -34,7 +34,7 @@ final class ManiphestTransactionListView extends ManiphestView {
   }
 
   public function setAuxiliaryFields(array $fields) {
-    assert_instances_of($fields, 'ManiphestAuxiliaryFieldSpecification');
+    assert_instances_of($fields, 'ManiphestCustomField');
     $this->auxiliaryFields = $fields;
     return $this;
   }