Allow CustomField to provide ApplicationTransaction change details

Summary: Ref T3886. Ref T418. For fields like "Summary" and "Test Plan" where changes can't be summarized in one line, allow CustomField to provide a "(Show Details)" link and render a diff. Also consolidate some of the existing copy/paste, and simplify this featuer slightly now that we've move to dialogs. Test Plan: {F115918} - Viewed "description"-style field changes in phlux, pholio, legalpad, maniphest, differential, ponder (questions), ponder (answers), and repositories. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3886, T418 Differential Revision: https://secure.phabricator.com/D8284
2025-01-27 15:08:20 +01:00 · 2014-02-21 11:53:04 -08:00 · 2014-02-21 11:53:04 -08:00 · 88227d26bc
commit 88227d26bc
parent be262f4b0c
16 changed files with 140 additions and 128 deletions
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@ -407,7 +407,7 @@ return array(
    'rsrc/js/application/search/behavior-reorder-queries.js' => '34397f68',
    'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => 'a51fdb2e',
    'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => '9084a36f',
-    'rsrc/js/application/transactions/behavior-transaction-list.js' => 'bfb45968',
+    'rsrc/js/application/transactions/behavior-transaction-list.js' => '3c918aa8',
    'rsrc/js/application/uiexample/JavelinViewExample.js' => 'd4a14807',
    'rsrc/js/application/uiexample/ReactorButtonExample.js' => '44524435',
    'rsrc/js/application/uiexample/ReactorCheckboxExample.js' => '7ba325ee',
@ -598,7 +598,7 @@ return array(
    'javelin-behavior-phabricator-show-all-transactions' => '7c273581',
    'javelin-behavior-phabricator-tooltips' => 'e5dd1c6d',
    'javelin-behavior-phabricator-transaction-comment-form' => '9084a36f',
-    'javelin-behavior-phabricator-transaction-list' => 'bfb45968',
+    'javelin-behavior-phabricator-transaction-list' => '3c918aa8',
    'javelin-behavior-phabricator-watch-anchor' => '06e05112',
    'javelin-behavior-phame-post-preview' => '61d927ec',
    'javelin-behavior-pholio-mock-edit' => '1e1e8bb0',
@ -1057,6 +1057,14 @@ return array(
      2 => 'javelin-util',
      3 => 'javelin-request',
    ),
+    '3c918aa8' =>
+    array(
+      0 => 'javelin-behavior',
+      1 => 'javelin-stratcom',
+      2 => 'javelin-workflow',
+      3 => 'javelin-dom',
+      4 => 'javelin-fx',
+    ),
    '403a3dce' =>
    array(
      0 => 'javelin-install',
@ -1592,14 +1600,6 @@ return array(
      1 => 'javelin-dom',
      2 => 'javelin-reactor-dom',
    ),
-    'bfb45968' =>
-    array(
-      0 => 'javelin-behavior',
-      1 => 'javelin-stratcom',
-      2 => 'javelin-workflow',
-      3 => 'javelin-dom',
-      4 => 'javelin-fx',
-    ),
    'c01153ea' =>
    array(
      0 => 'javelin-behavior',
--- a/src/applications/config/storage/PhabricatorConfigTransaction.php
+++ b/src/applications/config/storage/PhabricatorConfigTransaction.php
@ -89,12 +89,10 @@ final class PhabricatorConfigTransaction
      $new_text = PhabricatorConfigJSON::prettyPrintJSON($new['value']);
    }

-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old_text)
-      ->setNewText($new_text);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $old_text,
+      $new_text);
  }

  public function getColor() {
--- a/src/applications/differential/customfield/DifferentialSummaryField.php
+++ b/src/applications/differential/customfield/DifferentialSummaryField.php
@ -64,6 +64,18 @@ final class DifferentialSummaryField
      $xaction->renderHandleLink($object_phid));
  }

-  // TODO: Support hasChangeDetails() in CustomFields.
+  public function getApplicationTransactionHasChangeDetails(
+    PhabricatorApplicationTransaction $xaction) {
+    return true;
+  }
+
+  public function getApplicationTransactionChangeDetails(
+    PhabricatorApplicationTransaction $xaction,
+    PhabricatorUser $viewer) {
+    return $xaction->renderTextCorpusChangeDetails(
+      $viewer,
+      $xaction->getOldValue(),
+      $xaction->getNewValue());
+  }

 }
--- a/src/applications/legalpad/storage/LegalpadTransaction.php
+++ b/src/applications/legalpad/storage/LegalpadTransaction.php
@ -68,15 +68,10 @@ final class LegalpadTransaction extends PhabricatorApplicationTransaction {
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
  }

 }
--- a/src/applications/maniphest/storage/ManiphestTransaction.php
+++ b/src/applications/maniphest/storage/ManiphestTransaction.php
@ -631,17 +631,10 @@ final class ManiphestTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    require_celerity_resource('differential-changeset-view-css');
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
  }

  public function getMailTags() {
--- a/src/applications/passphrase/storage/PassphraseCredentialTransaction.php
+++ b/src/applications/passphrase/storage/PassphraseCredentialTransaction.php
@ -91,15 +91,10 @@ final class PassphraseCredentialTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText(json_encode($old))
-      ->setNewText(json_encode($new));
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      json_encode($this->getOldValue()),
+      json_encode($this->getNewValue()));
  }


--- a/src/applications/phlux/storage/PhluxTransaction.php
+++ b/src/applications/phlux/storage/PhluxTransaction.php
@ -43,15 +43,10 @@ final class PhluxTransaction extends PhabricatorApplicationTransaction {
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText(json_encode($old))
-      ->setNewText(json_encode($new));
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      json_encode($this->getOldValue()),
+      json_encode($this->getNewValue()));
  }


--- a/src/applications/pholio/storage/PholioTransaction.php
+++ b/src/applications/pholio/storage/PholioTransaction.php
@ -283,12 +283,10 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
      $new = reset($new);
    }

-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $old,
+      $new);
  }

  public function getColor() {
--- a/src/applications/ponder/storage/PonderAnswerTransaction.php
+++ b/src/applications/ponder/storage/PonderAnswerTransaction.php
@ -95,15 +95,10 @@ final class PonderAnswerTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
  }

 }
--- a/src/applications/ponder/storage/PonderQuestionTransaction.php
+++ b/src/applications/ponder/storage/PonderQuestionTransaction.php
@ -135,15 +135,10 @@ final class PonderQuestionTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
  }

  public function getActionStrength() {
--- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php
@ -381,15 +381,10 @@ final class PhabricatorRepositoryTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails(
+      $viewer,
+      $this->getOldValue(),
+      $this->getNewValue());
  }

 }
--- a/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php
+++ b/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php
@ -119,15 +119,7 @@ final class PhabricatorSlowvoteTransaction
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
-      ->setUser($viewer)
-      ->setOldText($old)
-      ->setNewText($new);
-
-    return $view->render();
+    return $this->renderTextCorpusChangeDetails($viewer);
  }


--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@ -250,6 +250,29 @@ abstract class PhabricatorApplicationTransaction
    return null;
  }

+  protected function getTransactionCustomField() {
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+        $key = $this->getMetadataValue('customfield:key');
+        if (!$key) {
+          return null;
+        }
+
+        $field = PhabricatorCustomField::getObjectField(
+          $this->getObject(),
+          PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS,
+          $key);
+        if (!$field) {
+          return null;
+        }
+
+        $field->setViewer($this->getViewer());
+        return $field;
+    }
+
+    return null;
+  }
+
  public function shouldHide() {
    switch ($this->getTransactionType()) {
      case PhabricatorTransactions::TYPE_VIEW_POLICY:
@ -396,13 +419,8 @@ abstract class PhabricatorApplicationTransaction
        }

      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
-        $key = $this->getMetadataValue('customfield:key');
-        $field = PhabricatorCustomField::getObjectField(
-          $this->getObject(),
-          PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS,
-          $key);
+        $field = $this->getTransactionCustomField();
        if ($field) {
-          $field->setViewer($this->getViewer());
          return $field->getApplicationTransactionTitle($this);
        } else {
          return pht(
@ -460,13 +478,8 @@ abstract class PhabricatorApplicationTransaction
          $this->renderHandleLink($author_phid),
          $this->renderHandleLink($object_phid));
      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
-        $key = $this->getMetadataValue('customfield:key');
-        $field = PhabricatorCustomField::getObjectField(
-          $this->getObject(),
-          PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS,
-          $key);
+        $field = $this->getTransactionCustomField();
        if ($field) {
-          $field->setViewer($this->getViewer());
          return $field->getApplicationTransactionTitleForFeed($this, $story);
        } else {
          return pht(
@ -524,11 +537,43 @@ abstract class PhabricatorApplicationTransaction
  }

  public function hasChangeDetails() {
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+        $field = $this->getTransactionCustomField();
+        if ($field) {
+          return $field->getApplicationTransactionHasChangeDetails($this);
+        }
+        break;
+    }
    return false;
  }

  public function renderChangeDetails(PhabricatorUser $viewer) {
-    return null;
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_CUSTOMFIELD:
+        $field = $this->getTransactionCustomField();
+        if ($field) {
+          return $field->getApplicationTransactionChangeDetails($this, $viewer);
+        }
+        break;
+    }
+
+    return $this->renderTextCorpusChangeDetails();
+  }
+
+  public function renderTextCorpusChangeDetails(
+    PhabricatorUser $viewer,
+    $old,
+    $new) {
+
+    require_celerity_resource('differential-changeset-view-css');
+
+    $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
+      ->setUser($viewer)
+      ->setOldText($old)
+      ->setNewText($new);
+
+    return $view->render();
  }

  public function attachTransactionGroup(array $group) {
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@ -175,11 +175,7 @@ class PhabricatorApplicationTransactionView extends AphrontView {
      'a',
      array(
        'href' => '/transactions/detail/'.$xaction->getPHID().'/',
-        'sigil' => 'transaction-detail',
-        'mustcapture' => true,
-        'meta' => array(
-          'anchor' => $this->anchorOffset,
-        ),
+        'sigil' => 'workflow',
      ),
      pht('(Show Details)'));
  }
--- a/src/infrastructure/customfield/field/PhabricatorCustomField.php
+++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php
@ -904,6 +904,27 @@ abstract class PhabricatorCustomField {
  }


+  public function getApplicationTransactionHasChangeDetails(
+    PhabricatorApplicationTransaction $xaction) {
+    if ($this->proxy) {
+      return $this->proxy->getApplicationTransactionHasChangeDetails(
+        $xaction);
+    }
+    return false;
+  }
+
+  public function getApplicationTransactionChangeDetails(
+    PhabricatorApplicationTransaction $xaction,
+    PhabricatorUser $viewer) {
+    if ($this->proxy) {
+      return $this->proxy->getApplicationTransactionChangeDetails(
+        $xaction,
+        $viewer);
+    }
+    return null;
+  }
+
+
 /* -(  Edit View  )---------------------------------------------------------- */


--- a/webroot/rsrc/js/application/transactions/behavior-transaction-list.js
+++ b/webroot/rsrc/js/application/transactions/behavior-transaction-list.js
@ -75,19 +75,6 @@ JX.behavior('phabricator-transaction-list', function(config) {
    e.kill();
  });

-  JX.DOM.listen(list, 'click', 'transaction-detail', function(e) {
-    if (!e.isNormalClick()) {
-      return;
-    }
-
-    JX.Workflow.newFromLink(e.getTarget())
-      .setData({anchor: e.getData('anchor')})
-      .setHandler(ontransactions)
-      .start();
-
-    e.kill();
-  });
-
  JX.Stratcom.listen(
    ['submit', 'didSyntheticSubmit'],
    'transaction-append',