From 37e26f1b45315872a2483cc76a5037f0524ac03a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 19 Jun 2019 13:17:00 -0700
Subject: [PATCH] Improve rendering of "default value changed" custom form
 transactions to at least have all the information

Summary:
Ref T13319. Currently, transactions about changes to a default form value use a raw internal key for the affected field and don't show the actual value change.

An ideal implementation will likely require us to specialize a great deal of rendering, but we can do much better than we currently do without too much work:

  - Try to pull the actual `EditField` object for the key so we can `getLabel()` it and get a human-readable label (like `Visible To` instead of `policy.view`).
  - Add a "(Show Changes)" action that dumps the raw values as more-or-less JSON, so you can at least figure out what happened if you're sophisticated enough.

Test Plan:
Before:

{F6516640}

After:

{F6516642}

The quality of "Show Details" varies a lot. For some fields, like "Description", it's pretty good:

{F6516645}

For others, like "Assigned To", it's better than nothing but pretty technical:

{F6516647}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13319

Differential Revision: https://secure.phabricator.com/D20594
---
 ...atorEditEngineConfigurationTransaction.php | 60 ++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php b/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
index 71048e96c5..bf23bd3b4a 100644
--- a/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
@@ -58,10 +58,23 @@ final class PhabricatorEditEngineConfigurationTransaction
           $this->renderHandleLink($author_phid));
       case self::TYPE_DEFAULT:
         $key = $this->getMetadataValue('field.key');
+
+        $object = $this->getObject();
+        $engine = $object->getEngine();
+        $fields = $engine->getFieldsForConfig($object);
+
+        $field = idx($fields, $key);
+        if (!$field) {
+          return pht(
+            '%s changed the default value for field "%s".',
+            $this->renderHandleLink($author_phid),
+            $key);
+        }
+
         return pht(
           '%s changed the default value for field "%s".',
           $this->renderHandleLink($author_phid),
-          $key);
+          $field->getLabel());
       case self::TYPE_LOCKS:
         return pht(
           '%s changed locked and hidden fields.',
@@ -164,4 +177,49 @@ final class PhabricatorEditEngineConfigurationTransaction
     return $changes;
   }
 
+  public function hasChangeDetails() {
+    switch ($this->getTransactionType()) {
+      case self::TYPE_DEFAULT:
+        return true;
+    }
+
+    return parent::hasChangeDetails();
+  }
+
+  public function renderChangeDetails(PhabricatorUser $viewer) {
+    switch ($this->getTransactionType()) {
+      case self::TYPE_DEFAULT:
+        $old_value = $this->getOldValue();
+        $new_value = $this->getNewValue();
+
+        $old_value = $this->renderDefaultValueAsFallbackText($old_value);
+        $new_value = $this->renderDefaultValueAsFallbackText($new_value);
+
+        return $this->renderTextCorpusChangeDetails(
+          $viewer,
+          $old_value,
+          $new_value);
+    }
+
+    return parent::renderChangeDetails($viewer);
+  }
+
+  private function renderDefaultValueAsFallbackText($default_value) {
+    // See T13319. When rendering an "alice changed the default value for
+    // field X." story on custom forms, we may fall back to a generic
+    // rendering. Try to present the value change in a comprehensible way
+    // even if it isn't especially human readable (for example, it may
+    // contain PHIDs or other internal identifiers).
+
+    if (is_scalar($default_value) || is_null($default_value)) {
+      return $default_value;
+    }
+
+    if (phutil_is_natural_list($default_value)) {
+      return id(new PhutilJSON())->encodeAsList($default_value);
+    }
+
+    return id(new PhutilJSON())->encodeAsObject($default_value);
+  }
+
 }