Improve rendering for Maniphest custom fields

Summary: We render these in a realtively unreadable way right now; allow customization and provide reasonable defaults.

Test Plan: Looked at some tasks with custom fields on them.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T930

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

src/applications/maniphest/auxiliaryfield/base/ManiphestAuxiliaryFieldSpecification.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,9 @@
  */
 abstract class ManiphestAuxiliaryFieldSpecification {
 
+  const RENDER_TARGET_HTML  = 'html';
+  const RENDER_TARGET_TEXT  = 'text';
+
   private $label;
   private $auxiliaryKey;
   private $caption;
@@ -59,7 +62,7 @@ abstract class ManiphestAuxiliaryFieldSpecification {
   }
 
   public function validate() {
-    return TRUE;
+    return true;
   }
 
   public function isRequired() {
@@ -83,4 +86,54 @@ abstract class ManiphestAuxiliaryFieldSpecification {
     return phutil_escape_html($this->getValue());
   }
 
+
+  /**
+   * Render a verb to appear in email titles when a transaction involving this
+   * field occurs. Specifically, Maniphest emails are formatted like this:
+   *
+   *   [Maniphest] [Verb Here] TNNN: Task title here
+   *                ^^^^^^^^^
+   *
+   * You should optionally return a title-case verb or short phrase like
+   * "Created", "Retitled", "Closed", "Resolved", "Commented On",
+   * "Lowered Priority", etc., which describes the transaction.
+   *
+   * @param ManiphestTransaction The transaction which needs description.
+   * @return string|null A short description of the transaction.
+   */
+  public function renderTransactionEmailVerb(
+    ManiphestTransaction $transaction) {
+    return null;
+  }
+
+
+  /**
+   * Render a short description of the transaction, to appear above comments
+   * in the Maniphest transaction log. The string will be rendered after the
+   * acting user's name. Examples are:
+   *
+   *    added a comment
+   *    added alincoln to CC
+   *    claimed this task
+   *    created this task
+   *    closed this task out of spite
+   *
+   * You should return a similar string, describing the transaction.
+   *
+   * Note the ##$target## parameter -- Maniphest needs to render transaction
+   * descriptions for different targets, like web and email. This method will
+   * be called with a ##ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_*##
+   * constant describing the intended target.
+   *
+   * @param ManiphestTransaction The transaction which needs description.
+   * @param const Constant describing the rendering target (e.g., html or text).
+   * @return string|null Description of the transaction.
+   */
+  public function renderTransactionDescription(
+    ManiphestTransaction $transaction,
+    $target) {
+    return 'updated a custom field';
+  }
+
+
 }

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

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -178,4 +178,50 @@ class ManiphestAuxiliaryFieldDefaultSpecification
     return parent::renderForDetailView();
   }
 
+  public function renderTransactionDescription(
+    ManiphestTransaction $transaction,
+    $target) {
+
+    $label = $this->getLabel();
+    $old = $transaction->getOldValue();
+    $new = $transaction->getNewValue();
+
+    switch ($this->getFieldType()) {
+      case self::TYPE_BOOL:
+        if ($new) {
+          $desc = "set field '{$label}' true";
+        } else {
+          $desc = "set field '{$label}' false";
+        }
+        break;
+      case self::TYPE_SELECT:
+        $old_display = idx($this->getSelectOptions(), $old);
+        $new_display = idx($this->getSelectOptions(), $new);
+        if ($old === null) {
+          $desc = "set field '{$label}' to '{$new_display}'";
+        } else {
+          $desc = "changed field '{$label}' ".
+                  "from '{$old_display}' to '{$new_display}'";
+        }
+        break;
+      default:
+        if (!strlen($old)) {
+          if (!strlen($new)) {
+            return null;
+          }
+          $desc = "set field '{$label}' to '{$new}'";
+        } else {
+          $desc = "updated '{$label}' ".
+                  "from '{$old}' to '{$new}'";
+        }
+        break;
+    }
+
+    if ($target == self::RENDER_TARGET_HTML) {
+      $desc = phutil_escape_html($desc);
+    }
+
+    return $desc;
+  }
+
 }

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

@@ -466,7 +466,6 @@ class ManiphestTaskDetailController extends ManiphestController {
       ),
     ));
 
-
     Javelin::initBehavior('maniphest-transaction-preview', array(
       'uri'     => '/maniphest/transaction/preview/'.$task->getID().'/',
       'preview' => 'transaction-preview',
@@ -494,6 +493,7 @@ class ManiphestTaskDetailController extends ManiphestController {
     $transaction_view->setTransactions($transactions);
     $transaction_view->setHandles($handles);
     $transaction_view->setUser($user);
+    $transaction_view->setAuxiliaryFields($aux_fields);
     $transaction_view->setMarkupEngine($engine);
 
     return $this->buildStandardPageResponse(

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

@@ -229,6 +229,7 @@ class ManiphestTaskEditController extends ManiphestController {
           $transactions = $event->getValue('transactions');
 
           $editor = new ManiphestTransactionEditor();
+          $editor->setAuxiliaryFields($aux_fields);
           $editor->applyTransactions($task, $transactions);
         }
 
@@ -246,6 +247,7 @@ class ManiphestTaskEditController extends ManiphestController {
           $parent_xaction->setNewValue($new_value);
 
           $editor = new ManiphestTransactionEditor();
+          $editor->setAuxiliaryFields($aux_fields);
           $editor->applyTransactions($parent_task, array($parent_xaction));
 
           $workflow = $parent_task->getID();

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

@@ -22,6 +22,12 @@
 class ManiphestTransactionEditor {
 
   private $parentMessageID;
+  private $auxiliaryFields = array();
+
+  public function setAuxiliaryFields(array $fields) {
+    $this->auxiliaryFields = $fields;
+    return $this;
+  }
 
   public function setParentMessageID($parent_message_id) {
     $this->parentMessageID = $parent_message_id;
@@ -219,6 +225,7 @@ class ManiphestTransactionEditor {
     $view = new ManiphestTransactionDetailView();
     $view->setTransactionGroup($transactions);
     $view->setHandles($handles);
+    $view->setAuxiliaryFields($this->auxiliaryFields);
     list($action, $body) = $view->renderForEmail($with_date = false);
 
     $is_create = $this->isCreate($transactions);

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

@@ -33,6 +33,17 @@ class ManiphestTransactionDetailView extends ManiphestView {
   private $renderFullSummary;
   private $user;
 
+  private $auxiliaryFields;
+
+  public function setAuxiliaryFields(array $fields) {
+    $this->auxiliaryFields = mpull($fields, null, 'getAuxiliaryKey');
+    return $this;
+  }
+
+  public function getAuxiliaryField($key) {
+    return idx($this->auxiliaryFields, $key);
+  }
+
   public function setTransactionGroup(array $transactions) {
     $this->transactions = $transactions;
     return $this;
@@ -101,6 +112,9 @@ class ManiphestTransactionDetailView extends ManiphestView {
     $comments = null;
     foreach ($this->transactions as $transaction) {
       list($verb, $desc, $classes) = $this->describeAction($transaction);
+      if ($desc === null) {
+        continue;
+      }
       if ($action === null) {
         $action = $verb;
       }
@@ -161,6 +175,9 @@ class ManiphestTransactionDetailView extends ManiphestView {
     $descs = array();
     foreach ($transactions as $transaction) {
       list($verb, $desc, $classes) = $this->describeAction($transaction);
+      if ($desc === null) {
+        continue;
+      }
       $more_classes = array_merge($more_classes, $classes);
       $full_summary = null;
       if ($this->getRenderFullSummary()) {
@@ -503,25 +520,37 @@ class ManiphestTransactionDetailView extends ManiphestView {
         }
         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}'";
+        $aux_field = $this->getAuxiliaryField($aux_key);
+
+        $verb = null;
+        if ($aux_field) {
+          $verb = $aux_field->renderTransactionEmailVerb($transaction);
         }
+        if ($verb === null) {
+          if ($old === null) {
+            $verb = "Set Field";
+          } else if ($new === null) {
+            $verb = "Removed Field";
+          } else {
+            $verb = "Updated Field";
+          }
+        }
+
+        $desc = null;
+        if ($aux_field) {
+          $use_field = $aux_field;
+        } else {
+          $use_field = id(new ManiphestAuxiliaryFieldDefaultSpecification())
+            ->setFieldType(
+              ManiphestAuxiliaryFieldDefaultSpecification::TYPE_STRING);
+        }
+
+        $desc = $use_field->renderTransactionDescription(
+          $transaction,
+          $this->forEmail
+            ? ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_TEXT
+            : ManiphestAuxiliaryFieldSpecification::RENDER_TARGET_HTML);
 
         break;
       default:

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

@@ -8,6 +8,8 @@
 
 phutil_require_module('phabricator', 'aphront/writeguard');
 phutil_require_module('phabricator', 'applications/differential/parser/changeset');
+phutil_require_module('phabricator', 'applications/maniphest/auxiliaryfield/base');
+phutil_require_module('phabricator', 'applications/maniphest/auxiliaryfield/default');
 phutil_require_module('phabricator', 'applications/maniphest/constants/priority');
 phutil_require_module('phabricator', 'applications/maniphest/constants/status');
 phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');

src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php

@@ -26,6 +26,7 @@ class ManiphestTransactionListView extends ManiphestView {
   private $user;
   private $markupEngine;
   private $preview;
+  private $auxiliaryFields;
 
   public function setTransactions(array $transactions) {
     $this->transactions = $transactions;
@@ -52,6 +53,11 @@ class ManiphestTransactionListView extends ManiphestView {
     return $this;
   }
 
+  public function setAuxiliaryFields(array $fields) {
+    $this->auxiliaryFields = $fields;
+    return $this;
+  }
+
   public function render() {
 
     $views = array();
@@ -99,6 +105,7 @@ class ManiphestTransactionListView extends ManiphestView {
     foreach ($groups as $group) {
       $view = new ManiphestTransactionDetailView();
       $view->setUser($this->user);
+      $view->setAuxiliaryFields($this->auxiliaryFields);
       $view->setTransactionGroup($group);
       $view->setHandles($this->handles);
       $view->setMarkupEngine($this->markupEngine);