Show detailed ApplicationTransaction changes in a dialog

Summary: Fixes T4423. This works better and is simpler and more flexible.

Test Plan: Clicked "(Show Details)", got a popup with the details.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T4423

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

src/applications/maniphest/controller/ManiphestTransactionPreviewController.php

@@ -97,7 +97,7 @@ final class ManiphestTransactionPreviewController extends ManiphestController {
         break;
       case ManiphestTransaction::TYPE_STATUS:
         $phids = array();
-        $transaction->setOldvalue($task->getStatus());
+        $transaction->setOldValue($task->getStatus());
         $transaction->setNewValue($value);
         break;
       default:
@@ -126,8 +126,7 @@ final class ManiphestTransactionPreviewController extends ManiphestController {
     $view = id(new PhabricatorApplicationTransactionView())
       ->setUser($user)
       ->setTransactions($transactions)
-      ->setIsPreview(true)
+      ->setIsPreview(true);
-      ->setIsDetailView(true);
 
     return id(new AphrontAjaxResponse())
       ->setContent((string)phutil_implode_html('', $view->buildEvents()));

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -634,6 +634,8 @@ final class ManiphestTransaction
     $old = $this->getOldValue();
     $new = $this->getNewValue();
 
+    require_celerity_resource('differential-changeset-view-css');
+
     $view = id(new PhabricatorApplicationTransactionTextDiffDetailView())
       ->setUser($viewer)
       ->setOldText($old)

src/applications/transactions/controller/PhabricatorApplicationTransactionDetailController.php

@@ -11,23 +11,40 @@ final class PhabricatorApplicationTransactionDetailController
 
   public function processRequest() {
     $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();
 
     $xaction = id(new PhabricatorObjectQuery())
       ->withPHIDs(array($this->phid))
-      ->setViewer($user)
+      ->setViewer($viewer)
       ->executeOne();
-
     if (!$xaction) {
-      // future proofing for the day visibility of transactions can change
       return new Aphront404Response();
     }
 
-    return id(new PhabricatorApplicationTransactionResponse())
+    $details = $xaction->renderChangeDetails($viewer);
-      ->setViewer($user)
+
-      ->setTransactions(array($xaction))
+    // Take an educated guess at the URI where the transactions appear so we
-      ->setIsDetailView(true)
+    // can send the cancel button somewhere sensible. This won't always get the
-      ->setAnchorOffset($request->getStr('anchor'));
+    // best answer (for example, Diffusion's history is visible on a page other
+    // than the main object view page) but should always get a reasonable one.
+
+    $cancel_uri = '/';
+    $handle = id(new PhabricatorHandleQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($xaction->getObjectPHID()))
+      ->executeOne();
+    if ($handle) {
+      $cancel_uri = $handle->getURI();
+    }
+
+    $dialog = id(new AphrontDialogView())
+      ->setUser($viewer)
+      ->setTitle(pht('Change Details'))
+      ->setWidth(AphrontDialogView::WIDTH_FULL)
+      ->appendChild($details)
+      ->addCancelButton($cancel_uri);
+
+    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/applications/transactions/response/PhabricatorApplicationTransactionResponse.php

@@ -7,7 +7,6 @@ final class PhabricatorApplicationTransactionResponse
   private $transactions;
   private $anchorOffset;
   private $isPreview;
-  private $isDetailView;
 
   protected function buildProxy() {
     return new AphrontAjaxResponse();
@@ -47,11 +46,6 @@ final class PhabricatorApplicationTransactionResponse
     return $this;
   }
 
-  public function setIsDetailView($is_detail_view) {
-    $this->isDetailView = $is_detail_view;
-    return $this;
-  }
-
   public function reduceProxyResponse() {
     if ($this->getTransactions()) {
       $view = head($this->getTransactions())
@@ -63,8 +57,7 @@ final class PhabricatorApplicationTransactionResponse
     $view
       ->setUser($this->getViewer())
       ->setTransactions($this->getTransactions())
-      ->setIsPreview($this->isPreview)
+      ->setIsPreview($this->isPreview);
-      ->setIsDetailView($this->isDetailView);
 
     if ($this->getAnchorOffset()) {
       $view->setAnchorOffset($this->getAnchorOffset());

src/applications/transactions/view/PhabricatorApplicationTransactionView.php

@@ -11,12 +11,6 @@ class PhabricatorApplicationTransactionView extends AphrontView {
   private $showEditActions = true;
   private $isPreview;
   private $objectPHID;
-  private $isDetailView;
-
-  public function setIsDetailView($is_detail_view) {
-    $this->isDetailView = $is_detail_view;
-    return $this;
-  }
 
   public function setObjectPHID($object_phid) {
     $this->objectPHID = $object_phid;
@@ -134,59 +128,6 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     return $engine;
   }
 
-  private function buildChangeDetails(
-    PhabricatorApplicationTransaction $xaction) {
-
-    Javelin::initBehavior('phabricator-reveal-content');
-
-    $show_id = celerity_generate_unique_node_id();
-    $hide_id = celerity_generate_unique_node_id();
-    $content_id = celerity_generate_unique_node_id();
-
-    $show_more = javelin_tag(
-      'a',
-      array(
-        'href' => '#',
-        'sigil' => 'reveal-content',
-        'mustcapture' => true,
-        'id' => $show_id,
-        'style' => 'display: none',
-        'meta' => array(
-          'hideIDs' => array($show_id),
-          'showIDs' => array($hide_id, $content_id),
-        ),
-      ),
-      pht('(Show Details)'));
-
-    $hide_more = javelin_tag(
-      'a',
-      array(
-        'href' => '#',
-        'sigil' => 'reveal-content',
-        'mustcapture' => true,
-        'id' => $hide_id,
-        'meta' => array(
-          'hideIDs' => array($hide_id, $content_id),
-          'showIDs' => array($show_id),
-        ),
-      ),
-      pht('(Hide Details)'));
-
-    $content = phutil_tag(
-      'div',
-      array(
-        'id'    => $content_id,
-        'class' => 'phui-timeline-change-details',
-      ),
-      $xaction->renderChangeDetails($this->getUser()));
-
-    return array(
-      $show_more,
-      $hide_more,
-      $content,
-    );
-  }
-
   private function buildChangeDetailsLink(
     PhabricatorApplicationTransaction $xaction) {
 
@@ -303,16 +244,14 @@ class PhabricatorApplicationTransactionView extends AphrontView {
     if (!$this->shouldSuppressTitle($xaction, $group)) {
       $title = $xaction->getTitle();
       if ($xaction->hasChangeDetails()) {
-        if ($this->isPreview || $this->isDetailView) {
+        if (!$this->isPreview) {
-          $details = $this->buildChangeDetails($xaction);
-        } else {
           $details = $this->buildChangeDetailsLink($xaction);
+          $title = array(
+            $title,
+            ' ',
+            $details,
+          );
         }
-        $title = array(
-          $title,
-          ' ',
-          $details,
-        );
       }
       $event->setTitle($title);
     }