Separate Herald transcripts into several different views

Summary: Ref T13586. The Herald transcript page has become more and more complicated over time, and recently added "Transactions" and "Profiler" sections. Split these across separate navigation tabs to limit the maximum complexity of any single view and make it easier to navigate to particular sections, like the profiler section. Test Plan: Viewed various transcripts, saw nice digestible sections. Maniphest Tasks: T13586 Differential Revision: https://secure.phabricator.com/D21493
2024-09-20 01:08:50 +02:00 · 2020-11-06 16:06:04 -08:00 · 2020-11-06 16:06:04 -08:00 · 5408429466
commit 5408429466
parent be0bb68f65
2 changed files with 166 additions and 57 deletions
--- a/src/applications/herald/application/PhabricatorHeraldApplication.php
+++ b/src/applications/herald/application/PhabricatorHeraldApplication.php
@ -57,13 +57,13 @@ final class PhabricatorHeraldApplication extends PhabricatorApplication {
        'new/' => 'HeraldNewController',
        'create/' => 'HeraldNewController',
        'edit/(?:(?P<id>[1-9]\d*)/)?' => 'HeraldRuleController',
-        'disable/(?P<id>[1-9]\d*)/(?P<action>\w+)/'
+        'disable/(?P<id>[1-9]\d*)/(?P<action>[^/]+)/'
          => 'HeraldDisableController',
        'test/' => 'HeraldTestConsoleController',
        'transcript/' => array(
          '' => 'HeraldTranscriptListController',
          '(?:query/(?P<queryKey>[^/]+)/)?' => 'HeraldTranscriptListController',
-          '(?P<id>[1-9]\d*)/'
+          '(?P<id>[1-9]\d*)/(?:(?P<view>[^/]+)/)?'
            => 'HeraldTranscriptController',
        ),
        'webhook/' => array(
--- a/src/applications/herald/controller/HeraldTranscriptController.php
+++ b/src/applications/herald/controller/HeraldTranscriptController.php
@ -9,6 +9,12 @@ final class HeraldTranscriptController extends HeraldController {
    return $this->adapter;
  }

+  public function buildApplicationMenu() {
+    // Use the menu we build in this controller, not the default menu for
+    // Herald.
+    return null;
+  }
+
  public function handleRequest(AphrontRequest $request) {
    $viewer = $this->getViewer();

@ -20,6 +26,13 @@ final class HeraldTranscriptController extends HeraldController {
      return new Aphront404Response();
    }

+    $view_key = $this->getViewKey($request);
+    if (!$view_key) {
+      return new Aphront404Response();
+    }
+
+    $navigation = $this->newSideNavView($xscript, $view_key);
+
    $object = $xscript->getObject();

    require_celerity_resource('herald-test-css');
@ -57,42 +70,21 @@ final class HeraldTranscriptController extends HeraldController {
      $handles = $this->loadViewerHandles($phids);
      $this->handles = $handles;

-      if ($xscript->getDryRun()) {
-        $notice = new PHUIInfoView();
-        $notice->setSeverity(PHUIInfoView::SEVERITY_NOTICE);
-        $notice->setTitle(pht('Dry Run'));
-        $notice->appendChild(
-          pht(
-            'This was a dry run to test Herald rules, '.
-            'no actions were executed.'));
-        $content[] = $notice;
-      }
-
      $warning_panel = $this->buildWarningPanel($xscript);
      $content[] = $warning_panel;

-      $content[] = array(
-        $this->buildActionTranscriptPanel($xscript),
-        $this->buildObjectTranscriptPanel($xscript),
-        $this->buildTransactionsTranscriptPanel(
-          $object,
-          $xscript),
-        $this->buildProfilerTranscriptPanel($xscript),
-      );
+      $content[] = $this->newContentView($xscript, $view_key);
    }

    $crumbs = id($this->buildApplicationCrumbs())
      ->addTextCrumb(
        pht('Transcripts'),
        $this->getApplicationURI('/transcript/'))
-      ->addTextCrumb($xscript->getID())
+      ->addTextCrumb(pht('Transcript %d', $xscript->getID()))
      ->setBorder(true);

-    $title = pht('Transcript: %s', $xscript->getID());
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title)
-      ->setHeaderIcon('fa-file');
+    $title = pht('Herald Transcript %s', $xscript->getID());
+    $header = $this->newHeaderView($xscript, $title);

    $view = id(new PHUITwoColumnView())
      ->setHeader($header)
@ -101,10 +93,8 @@ final class HeraldTranscriptController extends HeraldController {
    return $this->newPage()
      ->setTitle($title)
      ->setCrumbs($crumbs)
-      ->appendChild(
-        array(
-          $view,
-      ));
+      ->setNavigation($navigation)
+      ->appendChild($view);
  }

  protected function renderConditionTestValue($condition, $handles) {
@ -443,7 +433,22 @@ final class HeraldTranscriptController extends HeraldController {
      ->setHeaderText(pht('Rule Transcript'))
      ->appendChild($rule_list);

-    return $box;
+    $content = array();
+
+    if ($xscript->getDryRun()) {
+      $notice = new PHUIInfoView();
+      $notice->setSeverity(PHUIInfoView::SEVERITY_NOTICE);
+      $notice->setTitle(pht('Dry Run'));
+      $notice->appendChild(
+        pht(
+          'This was a dry run to test Herald rules, '.
+          'no actions were executed.'));
+      $content[] = $notice;
+    }
+
+    $content[] = $box;
+
+    return $content;
  }

  private function buildObjectTranscriptPanel(HeraldTranscript $xscript) {
@ -520,35 +525,15 @@ final class HeraldTranscriptController extends HeraldController {
    return $box;
  }

-  private function buildTransactionsTranscriptPanel(
-    $object,
-    HeraldTranscript $xscript) {
+  private function buildTransactionsTranscriptPanel(HeraldTranscript $xscript) {
    $viewer = $this->getViewer();

-    $object_xscript = $xscript->getObjectTranscript();
-
-    $xaction_phids = $object_xscript->getAppliedTransactionPHIDs();
-
-    // If the value is "null", this is an older transcript or this adapter
-    // does not use transactions. We render nothing.
-    //
-    // If the value is "array()", this is a modern transcript which uses
-    // transactions, there just weren't any applied. Below, we'll render a
-    // "No Transactions Applied" state.
-    if ($xaction_phids === null) {
-      return null;
-    }
-
-    // If this object doesn't implement the right interface, we won't be
-    // able to load the transactions. Just bail.
-    if (!($object instanceof PhabricatorApplicationTransactionInterface)) {
-      return null;
-    }
-
-    $query = PhabricatorApplicationTransactionQuery::newQueryForObject(
-      $object);
+    $xaction_phids = $this->getTranscriptTransactionPHIDs($xscript);

    if ($xaction_phids) {
+      $object = $xscript->getObject();
+      $query = PhabricatorApplicationTransactionQuery::newQueryForObject(
+        $object);
      $xactions = $query
        ->setViewer($viewer)
        ->withPHIDs($xaction_phids)
@ -704,4 +689,128 @@ final class HeraldTranscriptController extends HeraldController {
    return $box_view;
  }

+  private function getViewKey(AphrontRequest $request) {
+    $view_key = $request->getURIData('view');
+
+    if ($view_key === null) {
+      return 'rules';
+    }
+
+    switch ($view_key) {
+      case 'fields':
+      case 'xactions':
+      case 'profile':
+        return $view_key;
+      default:
+        return null;
+    }
+  }
+
+  private function newSideNavView(
+    HeraldTranscript $xscript,
+    $view_key) {
+
+    $base_uri = urisprintf(
+      'transcript/%d/',
+      $xscript->getID());
+
+    $base_uri = $this->getApplicationURI($base_uri);
+    $base_uri = new PhutilURI($base_uri);
+
+    $nav = id(new AphrontSideNavFilterView())
+      ->setBaseURI($base_uri);
+
+    $nav->newLink('rules')
+      ->setHref($base_uri)
+      ->setName(pht('Rules'))
+      ->setIcon('fa-list-ul');
+
+    $nav->newLink('fields')
+      ->setName(pht('Field Values'))
+      ->setIcon('fa-file-text-o');
+
+    $xaction_phids = $this->getTranscriptTransactionPHIDs($xscript);
+    $has_xactions = (bool)$xaction_phids;
+
+    $nav->newLink('xactions')
+      ->setName(pht('Transactions'))
+      ->setIcon('fa-forward')
+      ->setDisabled(!$has_xactions);
+
+    $nav->newLink('profile')
+      ->setName(pht('Profiler'))
+      ->setIcon('fa-tachometer');
+
+    $nav->selectFilter($view_key);
+
+    return $nav;
+  }
+
+  private function newContentView(
+    HeraldTranscript $xscript,
+    $view_key) {
+
+    switch ($view_key) {
+      case 'rules':
+        $content = $this->buildActionTranscriptPanel($xscript);
+        break;
+      case 'fields':
+        $content = $this->buildObjectTranscriptPanel($xscript);
+        break;
+      case 'xactions':
+        $content = $this->buildTransactionsTranscriptPanel($xscript);
+        break;
+      case 'profile':
+        $content = $this->buildProfilerTranscriptPanel($xscript);
+        break;
+      default:
+        throw new Exception(pht('Unknown view key "%s".', $view_key));
+    }
+
+    return $content;
+  }
+
+  private function getTranscriptTransactionPHIDs(HeraldTranscript $xscript) {
+
+    $object_xscript = $xscript->getObjectTranscript();
+    $xaction_phids = $object_xscript->getAppliedTransactionPHIDs();
+
+    // If the value is "null", this is an older transcript or this adapter
+    // does not use transactions.
+    //
+    // (If the value is "array()", this is a modern transcript which uses
+    // transactions, there just weren't any applied.)
+    if ($xaction_phids === null) {
+      return array();
+    }
+
+    $object = $xscript->getObject();
+
+    // If this object doesn't implement the right interface, we won't be
+    // able to load the transactions.
+    if (!($object instanceof PhabricatorApplicationTransactionInterface)) {
+      return array();
+    }
+
+    return $xaction_phids;
+  }
+
+  private function newHeaderView(HeraldTranscript $xscript, $title) {
+    $header = id(new PHUIHeaderView())
+      ->setHeader($title)
+      ->setHeaderIcon('fa-list-ul');
+
+    if ($xscript->getDryRun()) {
+      $dry_run_tag = id(new PHUITagView())
+        ->setType(PHUITagView::TYPE_SHADE)
+        ->setColor(PHUITagView::COLOR_VIOLET)
+        ->setName(pht('Dry Run'))
+        ->setIcon('fa-exclamation-triangle');
+
+      $header->addTag($dry_run_tag);
+    }
+
+    return $header;
+  }
+
 }