Modularize Herald "flag" action, plus update transcripts

Summary:
Ref T8726. This modularizes "Mark with flag", plus rebuilds transcripts in a more modern/flexible way. The big transcript stuff is:

  - Transcripts are now translatable.
  - Transcripts can now show multiple outputs from a single action. For example, an action like "add A, B, C to subscribers" can now say "added A; B is invalid; C was already subscribed".

Test Plan: {F637784}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, eadler, epriestley

Maniphest Tasks: T8726

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

resources/celerity/map.php

@@ -73,7 +73,7 @@ return array(
     'rsrc/css/application/files/global-drag-and-drop.css' => '697324ad',
     'rsrc/css/application/flag/flag.css' => '5337623f',
     'rsrc/css/application/harbormaster/harbormaster.css' => '49d64eb4',
-    'rsrc/css/application/herald/herald-test.css' => '778b008e',
+    'rsrc/css/application/herald/herald-test.css' => 'a52e323e',
     'rsrc/css/application/herald/herald.css' => '826075fa',
     'rsrc/css/application/maniphest/batch-editor.css' => 'b0f0b6d5',
     'rsrc/css/application/maniphest/report.css' => 'f6931fdf',
@@ -539,7 +539,7 @@ return array(
     'harbormaster-css' => '49d64eb4',
     'herald-css' => '826075fa',
     'herald-rule-editor' => '91a6031b',
-    'herald-test-css' => '778b008e',
+    'herald-test-css' => 'a52e323e',
     'inline-comment-summary-css' => '51efda3a',
     'javelin-aphlict' => '5359e785',
     'javelin-behavior' => '61cbc29a',

src/__phutil_library_map__.php

@@ -2110,6 +2110,7 @@ phutil_register_library_map(array(
     'PhabricatorFilesManagementWorkflow' => 'applications/files/management/PhabricatorFilesManagementWorkflow.php',
     'PhabricatorFilesOutboundRequestAction' => 'applications/files/action/PhabricatorFilesOutboundRequestAction.php',
     'PhabricatorFlag' => 'applications/flag/storage/PhabricatorFlag.php',
+    'PhabricatorFlagAddFlagHeraldAction' => 'applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php',
     'PhabricatorFlagColor' => 'applications/flag/constants/PhabricatorFlagColor.php',
     'PhabricatorFlagConstants' => 'applications/flag/constants/PhabricatorFlagConstants.php',
     'PhabricatorFlagController' => 'applications/flag/controller/PhabricatorFlagController.php',
@@ -5988,6 +5989,7 @@ phutil_register_library_map(array(
       'PhabricatorFlagDAO',
       'PhabricatorPolicyInterface',
     ),
+    'PhabricatorFlagAddFlagHeraldAction' => 'HeraldAction',
     'PhabricatorFlagColor' => 'PhabricatorFlagConstants',
     'PhabricatorFlagConstants' => 'Phobject',
     'PhabricatorFlagController' => 'PhabricatorController',

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -177,7 +177,6 @@ final class HeraldDifferentialRevisionAdapter
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
-            self::ACTION_FLAG,
             self::ACTION_ADD_REVIEWERS,
             self::ACTION_ADD_BLOCKING_REVIEWERS,
           ),

src/applications/diffusion/herald/HeraldCommitAdapter.php

@@ -102,7 +102,6 @@ final class HeraldCommitAdapter extends HeraldAdapter {
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
-            self::ACTION_FLAG,
             self::ACTION_AUDIT,
           ),
           parent::getActions($rule_type));

src/applications/flag/herald/PhabricatorFlagAddFlagHeraldAction.php

@@ -0,0 +1,88 @@
+<?php
+
+final class PhabricatorFlagAddFlagHeraldAction extends HeraldAction {
+
+  const ACTIONCONST = 'flag';
+
+  const DO_FLAG = 'do.flag';
+  const DO_IGNORE = 'do.flagged';
+
+  public function getHeraldActionName() {
+    return pht('Mark with flag');
+  }
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    return ($object instanceof PhabricatorFlaggableInterface);
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    $phid = $this->getAdapter()->getPHID();
+    $rule = $effect->getRule();
+    $author = $rule->getAuthor();
+
+    $flag = PhabricatorFlagQuery::loadUserFlag($author, $phid);
+    if ($flag) {
+      $this->logEffect(self::DO_IGNORE, $flag->getColor());
+      return;
+    }
+
+    $flag = id(new PhabricatorFlag())
+      ->setOwnerPHID($author->getPHID())
+      ->setType(phid_get_type($phid))
+      ->setObjectPHID($phid)
+      ->setReasonPHID($rule->getPHID())
+      ->setColor($effect->getTarget())
+      ->setNote('')
+      ->save();
+
+    $this->logEffect(self::DO_FLAG, $flag->getColor());
+  }
+
+  public function getHeraldActionValueType() {
+    return id(new HeraldSelectFieldValue())
+      ->setKey('flag.color')
+      ->setOptions(PhabricatorFlagColor::getColorNameMap())
+      ->setDefault(PhabricatorFlagColor::COLOR_BLUE);
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_IGNORE => array(
+        'icon' => 'fa-times',
+        'color' => 'grey',
+        'name' => pht('Already Marked'),
+      ),
+      self::DO_FLAG => array(
+        'icon' => 'fa-flag',
+        'name' => pht('Flagged'),
+      ),
+    );
+  }
+
+  public function renderActionDescription($value) {
+    $color = PhabricatorFlagColor::getColorName($value);
+    return pht('Mark with %s flag.', $color);
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_IGNORE:
+        return pht(
+          'Already marked with %s flag.',
+          PhabricatorFlagColor::getColorName($data));
+      case self::DO_FLAG:
+        return pht(
+          'Marked with "%s" flag.',
+          PhabricatorFlagColor::getColorName($data));
+    }
+  }
+
+}

src/applications/herald/action/HeraldAction.php

@@ -3,6 +3,7 @@
 abstract class HeraldAction extends Phobject {
 
   private $adapter;
+  private $viewer;
   private $applyLog = array();
 
   const STANDARD_NONE = 'standard.none';
@@ -12,6 +13,7 @@ abstract class HeraldAction extends Phobject {
   abstract public function supportsObject($object);
   abstract public function supportsRuleType($rule_type);
   abstract public function applyEffect($object, HeraldEffect $effect);
+  abstract public function renderActionEffectDescription($type, $data);
 
   public function getActionGroupKey() {
     return null;
@@ -66,6 +68,15 @@ abstract class HeraldAction extends Phobject {
     return $this->adapter;
   }
 
+  final public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  final public function getViewer() {
+    return $this->viewer;
+  }
+
   final public function getActionConstant() {
     $class = new ReflectionClass($this);
 
@@ -106,13 +117,49 @@ abstract class HeraldAction extends Phobject {
   }
 
   protected function logEffect($type, $data = null) {
-    return;
+    if (!is_string($type)) {
+      throw new Exception(
+        pht(
+          'Effect type passed to "%s" must be a scalar string.',
+          'logEffect()'));
+    }
+
+    $this->applyLog[] = array(
+      'type' => $type,
+      'data' => $data,
+    );
+
+    return $this;
   }
 
   final public function getApplyTranscript(HeraldEffect $effect) {
-    $context = 'v2/'.phutil_json_encode($this->applyLog);
+    $context = $this->applyLog;
     $this->applyLog = array();
     return new HeraldApplyTranscript($effect, true, $context);
   }
 
+  protected function getActionEffectMap() {
+    throw new PhutilMethodNotImplementedException();
+  }
+
+  private function getActionEffectSpec($type) {
+    $map = $this->getActionEffectMap();
+    return idx($map, $type, array());
+  }
+
+  public function renderActionEffectIcon($type, $data) {
+    $map = $this->getActionEffectSpec($type);
+    return idx($map, 'icon');
+  }
+
+  public function renderActionEffectColor($type, $data) {
+    $map = $this->getActionEffectSpec($type);
+    return idx($map, 'color');
+  }
+
+  public function renderActionEffectName($type, $data) {
+    $map = $this->getActionEffectSpec($type);
+    return idx($map, 'name');
+  }
+
 }

src/applications/herald/action/HeraldDoNothingAction.php

@@ -22,11 +22,29 @@ final class HeraldDoNothingAction extends HeraldAction {
   }
 
   public function applyEffect($object, HeraldEffect $effect) {
-    $this->logEffect($effect, self::DO_NOTHING);
+    $this->logEffect(self::DO_NOTHING);
   }
 
   public function getHeraldActionStandardType() {
     return self::STANDARD_NONE;
   }
 
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_NOTHING => array(
+        'icon' => 'fa-check',
+        'color' => 'grey',
+        'name' => pht('Did Nothing'),
+      ),
+    );
+  }
+
+  public function renderActionDescription($value) {
+    return pht('Do nothing.');
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    return pht('Did nothing.');
+  }
+
 }

src/applications/herald/adapter/HeraldAdapter.php

@@ -30,7 +30,6 @@ abstract class HeraldAdapter extends Phobject {
   const ACTION_REMOVE_CC    = 'remcc';
   const ACTION_EMAIL        = 'email';
   const ACTION_AUDIT        = 'audit';
-  const ACTION_FLAG         = 'flag';
   const ACTION_ASSIGN_TASK  = 'assigntask';
   const ACTION_ADD_PROJECTS = 'addprojects';
   const ACTION_REMOVE_PROJECTS = 'removeprojects';
@@ -670,7 +669,7 @@ abstract class HeraldAdapter extends Phobject {
     return $actions;
   }
 
-  private function getActionImplementation($key) {
+  public function getActionImplementation($key) {
     return idx($this->getActionImplementationMap(), $key);
   }
 
@@ -728,7 +727,6 @@ abstract class HeraldAdapter extends Phobject {
           self::ACTION_REMOVE_CC    => pht('Remove Subscribers'),
           self::ACTION_EMAIL        => pht('Send an email to'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
-          self::ACTION_FLAG         => pht('Mark with flag'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to'),
           self::ACTION_ADD_PROJECTS => pht('Add projects'),
           self::ACTION_REMOVE_PROJECTS => pht('Remove projects'),
@@ -745,7 +743,6 @@ abstract class HeraldAdapter extends Phobject {
           self::ACTION_REMOVE_CC    => pht('Remove me as a subscriber'),
           self::ACTION_EMAIL        => pht('Send me an email'),
           self::ACTION_AUDIT        => pht('Trigger an Audit by me'),
-          self::ACTION_FLAG         => pht('Mark with flag'),
           self::ACTION_ASSIGN_TASK  => pht('Assign task to me'),
           self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'),
           self::ACTION_ADD_BLOCKING_REVIEWERS =>
@@ -798,13 +795,6 @@ abstract class HeraldAdapter extends Phobject {
           // For personal rules, force these actions to target the rule owner.
           $target = array($author_phid);
           break;
-        case self::ACTION_FLAG:
-          // Make sure flag color is valid; set to blue if not.
-          $color_map = PhabricatorFlagColor::getColorNameMap();
-          if (empty($color_map[$target])) {
-            $target = PhabricatorFlagColor::COLOR_BLUE;
-          }
-          break;
         case self::ACTION_BLOCK:
           break;
         default:
@@ -846,8 +836,6 @@ abstract class HeraldAdapter extends Phobject {
         case self::ACTION_ADD_REVIEWERS:
         case self::ACTION_ADD_BLOCKING_REVIEWERS:
           return new HeraldEmptyFieldValue();
-        case self::ACTION_FLAG:
-          return $this->buildFlagColorFieldValue();
         case self::ACTION_ADD_PROJECTS:
         case self::ACTION_REMOVE_PROJECTS:
           return $this->buildTokenizerFieldValue(
@@ -864,8 +852,6 @@ abstract class HeraldAdapter extends Phobject {
         case self::ACTION_REMOVE_PROJECTS:
           return $this->buildTokenizerFieldValue(
             new PhabricatorProjectDatasource());
-        case self::ACTION_FLAG:
-          return $this->buildFlagColorFieldValue();
         case self::ACTION_ASSIGN_TASK:
           return $this->buildTokenizerFieldValue(
             new PhabricatorPeopleDatasource());
@@ -893,13 +879,6 @@ abstract class HeraldAdapter extends Phobject {
     throw new Exception(pht("Unknown or invalid action '%s'.", $action));
   }
 
-  private function buildFlagColorFieldValue() {
-    return id(new HeraldSelectFieldValue())
-      ->setKey('flag.color')
-      ->setOptions(PhabricatorFlagColor::getColorNameMap())
-      ->setDefault(PhabricatorFlagColor::COLOR_BLUE);
-  }
-
   private function buildTokenizerFieldValue(
     PhabricatorTypeaheadDatasource $datasource) {
 
@@ -1051,7 +1030,7 @@ abstract class HeraldAdapter extends Phobject {
         ),
         array(
           $icon,
-          $this->renderActionAsText($action, $handles),
+          $this->renderActionAsText($viewer, $action, $handles),
         ));
     }
 
@@ -1083,8 +1062,16 @@ abstract class HeraldAdapter extends Phobject {
   }
 
   private function renderActionAsText(
+    PhabricatorUser $viewer,
     HeraldActionRecord $action,
     PhabricatorHandleList $handles) {
+
+    $impl = $this->getActionImplementation($action->getAction());
+    if ($impl) {
+      $value = $action->getTarget();
+      return $impl->renderActionDescription($viewer, $value);
+    }
+
     $rule_global = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
 
     $action_type = $action->getAction();
@@ -1118,15 +1105,14 @@ abstract class HeraldAdapter extends Phobject {
     HeraldActionRecord $action,
     PhabricatorHandleList $handles) {
 
+    // TODO: This should be driven through HeraldAction.
+
     $target = $action->getTarget();
     if (!is_array($target)) {
       $target = array($target);
     }
     foreach ($target as $index => $val) {
       switch ($action->getAction()) {
-        case self::ACTION_FLAG:
-          $target[$index] = PhabricatorFlagColor::getColorName($val);
-          break;
         default:
           $handle = $handles->getHandleIfExists($val);
           if ($handle) {
@@ -1222,8 +1208,6 @@ abstract class HeraldAdapter extends Phobject {
       case self::ACTION_ADD_CC:
       case self::ACTION_REMOVE_CC:
         return $this->applySubscribersEffect($effect);
-      case self::ACTION_FLAG:
-        return $this->applyFlagEffect($effect);
       case self::ACTION_EMAIL:
         return $this->applyEmailEffect($effect);
       default:
@@ -1364,50 +1348,6 @@ abstract class HeraldAdapter extends Phobject {
     return new HeraldApplyTranscript($effect, true, $message);
   }
 
-
-  /**
-   * @task apply
-   */
-  private function applyFlagEffect(HeraldEffect $effect) {
-    $phid = $this->getPHID();
-    $color = $effect->getTarget();
-
-    $rule = $effect->getRule();
-    $user = $rule->getAuthor();
-
-    $flag = PhabricatorFlagQuery::loadUserFlag($user, $phid);
-    if ($flag) {
-      return new HeraldApplyTranscript(
-        $effect,
-        false,
-        pht('Object already flagged.'));
-    }
-
-    $handle = id(new PhabricatorHandleQuery())
-      ->setViewer($user)
-      ->withPHIDs(array($phid))
-      ->executeOne();
-
-    $flag = new PhabricatorFlag();
-    $flag->setOwnerPHID($user->getPHID());
-    $flag->setType($handle->getType());
-    $flag->setObjectPHID($handle->getPHID());
-
-    // TOOD: Should really be transcript PHID, but it doesn't exist yet.
-    $flag->setReasonPHID($user->getPHID());
-
-    $flag->setColor($color);
-    $flag->setNote(
-      pht('Flagged by Herald Rule "%s".', $rule->getName()));
-    $flag->save();
-
-    return new HeraldApplyTranscript(
-      $effect,
-      true,
-      pht('Added flag.'));
-  }
-
-
   /**
    * @task apply
    */

src/applications/herald/application/PhabricatorHeraldApplication.php

@@ -58,7 +58,7 @@ final class PhabricatorHeraldApplication extends PhabricatorApplication {
         'transcript/' => array(
           '' => 'HeraldTranscriptListController',
           '(?:query/(?P<queryKey>[^/]+)/)?' => 'HeraldTranscriptListController',
-          '(?P<id>[1-9]\d*)/(?:(?P<filter>\w+)/)?'
+          '(?P<id>[1-9]\d*)/'
             => 'HeraldTranscriptController',
         ),
       ),

src/applications/herald/controller/HeraldRuleController.php

@@ -367,7 +367,6 @@ final class HeraldRuleController extends HeraldController {
       $serial_actions = array();
       foreach ($rule->getActions() as $action) {
         switch ($action->getAction()) {
-          case HeraldAdapter::ACTION_FLAG:
           case HeraldAdapter::ACTION_BLOCK:
             $current_value = $action->getTarget();
             break;

src/applications/herald/controller/HeraldTranscriptController.php

@@ -2,43 +2,26 @@
 
 final class HeraldTranscriptController extends HeraldController {
 
-  const FILTER_AFFECTED = 'affected';
-  const FILTER_OWNED    = 'owned';
-  const FILTER_ALL      = 'all';
-
-  private $id;
-  private $filter;
   private $handles;
   private $adapter;
 
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-    $map = $this->getFilterMap();
-    $this->filter = idx($data, 'filter');
-    if (empty($map[$this->filter])) {
-      $this->filter = self::FILTER_ALL;
-    }
-  }
-
   private function getAdapter() {
     return $this->adapter;
   }
 
-  public function processRequest() {
+  public function handleRequest(AphrontRequest $request) {
-    $request = $this->getRequest();
+    $viewer = $this->getViewer();
-    $viewer = $request->getUser();
 
     $xscript = id(new HeraldTranscriptQuery())
       ->setViewer($viewer)
-      ->withIDs(array($this->id))
+      ->withIDs(array($request->getURIData('id')))
       ->executeOne();
     if (!$xscript) {
       return new Aphront404Response();
     }
 
     require_celerity_resource('herald-test-css');
-
+    $content = array();
-    $nav = $this->buildSideNav();
 
     $object_xscript = $xscript->getObjectTranscript();
     if (!$object_xscript) {
@@ -49,7 +32,7 @@ final class HeraldTranscriptController extends HeraldController {
           'p',
           array(),
           pht('Details of this transcript have been garbage collected.')));
-      $nav->appendChild($notice);
+      $content[] = $notice;
     } else {
       $map = HeraldAdapter::getEnabledAdapterMap($viewer);
       $object_type = $object_xscript->getType();
@@ -65,9 +48,7 @@ final class HeraldTranscriptController extends HeraldController {
 
       $this->adapter = HeraldAdapter::getAdapterForContentType($object_type);
 
-      $filter = $this->getFilterPHIDs();
+      $phids = $this->getTranscriptPHIDs($xscript);
-      $this->filterTranscript($xscript, $filter);
-      $phids = array_merge($filter, $this->getTranscriptPHIDs($xscript));
       $phids = array_unique($phids);
       $phids = array_filter($phids);
 
@@ -82,23 +63,16 @@ final class HeraldTranscriptController extends HeraldController {
           pht(
             'This was a dry run to test Herald rules, '.
             'no actions were executed.'));
-        $nav->appendChild($notice);
+        $content[] = $notice;
       }
 
       $warning_panel = $this->buildWarningPanel($xscript);
-      $nav->appendChild($warning_panel);
+      $content[] = $warning_panel;
 
-      $apply_xscript_panel = $this->buildApplyTranscriptPanel(
+      $content[] = array(
-        $xscript);
+        $this->buildActionTranscriptPanel($xscript),
-      $nav->appendChild($apply_xscript_panel);
+        $this->buildObjectTranscriptPanel($xscript),
-
+      );
-      $action_xscript_panel = $this->buildActionTranscriptPanel(
-        $xscript);
-      $nav->appendChild($action_xscript_panel);
-
-      $object_xscript_panel = $this->buildObjectTranscriptPanel(
-        $xscript);
-      $nav->appendChild($object_xscript_panel);
     }
 
     $crumbs = id($this->buildApplicationCrumbs())
@@ -106,10 +80,12 @@ final class HeraldTranscriptController extends HeraldController {
         pht('Transcripts'),
         $this->getApplicationURI('/transcript/'))
       ->addTextCrumb($xscript->getID());
-    $nav->setCrumbs($crumbs);
 
     return $this->buildApplicationPage(
-      $nav,
+      array(
+        $crumbs,
+        $content,
+      ),
       array(
         'title' => pht('Transcript'),
       ));
@@ -146,33 +122,6 @@ final class HeraldTranscriptController extends HeraldController {
     return phutil_tag('span', array('class' => 'condition-test-value'), $value);
   }
 
-  private function buildSideNav() {
-    $nav = new AphrontSideNavFilterView();
-    $nav->setBaseURI(new PhutilURI('/herald/transcript/'.$this->id.'/'));
-
-    $items = array();
-    $filters = $this->getFilterMap();
-    foreach ($filters as $key => $name) {
-      $nav->addFilter($key, $name);
-    }
-    $nav->selectFilter($this->filter, null);
-
-    return $nav;
-  }
-
-  protected function getFilterMap() {
-    return array(
-      self::FILTER_ALL      => pht('All Rules'),
-      self::FILTER_OWNED    => pht('Rules I Own'),
-      self::FILTER_AFFECTED => pht('Rules that Affected Me'),
-    );
-  }
-
-
-  protected function getFilterPHIDs() {
-    return array($this->getRequest()->getUser()->getPHID());
-  }
-
   protected function getTranscriptPHIDs($xscript) {
     $phids = array();
 
@@ -228,71 +177,6 @@ final class HeraldTranscriptController extends HeraldController {
     return $phids;
   }
 
-  protected function filterTranscript($xscript, $filter_phids) {
-    $filter_owned = ($this->filter == self::FILTER_OWNED);
-    $filter_affected = ($this->filter == self::FILTER_AFFECTED);
-
-    if (!$filter_owned && !$filter_affected) {
-      // No filtering to be done.
-      return;
-    }
-
-    if (!$xscript->getObjectTranscript()) {
-      return;
-    }
-
-    $user_phid = $this->getRequest()->getUser()->getPHID();
-
-    $keep_apply_xscripts = array();
-    $keep_rule_xscripts  = array();
-
-    $filter_phids = array_fill_keys($filter_phids, true);
-
-    $rule_xscripts = $xscript->getRuleTranscripts();
-    foreach ($xscript->getApplyTranscripts() as $id => $apply_xscript) {
-      $rule_id = $apply_xscript->getRuleID();
-      if ($filter_owned) {
-        if (empty($rule_xscripts[$rule_id])) {
-          // No associated rule so you can't own this effect.
-          continue;
-        }
-        if ($rule_xscripts[$rule_id]->getRuleOwner() != $user_phid) {
-          continue;
-        }
-      } else if ($filter_affected) {
-        $targets = (array)$apply_xscript->getTarget();
-        if (!array_select_keys($filter_phids, $targets)) {
-          continue;
-        }
-      }
-      $keep_apply_xscripts[$id] = true;
-      if ($rule_id) {
-        $keep_rule_xscripts[$rule_id] = true;
-      }
-    }
-
-    foreach ($rule_xscripts as $rule_id => $rule_xscript) {
-      if ($filter_owned && $rule_xscript->getRuleOwner() == $user_phid) {
-        $keep_rule_xscripts[$rule_id] = true;
-      }
-    }
-
-    $xscript->setRuleTranscripts(
-      array_intersect_key(
-        $xscript->getRuleTranscripts(),
-        $keep_rule_xscripts));
-
-    $xscript->setApplyTranscripts(
-      array_intersect_key(
-        $xscript->getApplyTranscripts(),
-        $keep_apply_xscripts));
-
-    $xscript->setConditionTranscripts(
-      array_intersect_key(
-        $xscript->getConditionTranscripts(),
-        $keep_rule_xscripts));
-  }
-
   private function buildWarningPanel(HeraldTranscript $xscript) {
     $request = $this->getRequest();
     $panel = null;
@@ -333,165 +217,185 @@ final class HeraldTranscriptController extends HeraldController {
     return $panel;
   }
 
-  private function buildApplyTranscriptPanel(HeraldTranscript $xscript) {
-    $handles = $this->handles;
-    $adapter = $this->getAdapter();
-
-    $rule_type_global = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
-    $action_names = $adapter->getActionNameMap($rule_type_global);
-
-    $list = new PHUIObjectItemListView();
-    $list->setStates(true);
-    $list->setNoDataString(pht('No actions were taken.'));
-    foreach ($xscript->getApplyTranscripts() as $apply_xscript) {
-
-      $target = $apply_xscript->getTarget();
-      switch ($apply_xscript->getAction()) {
-        case HeraldAdapter::ACTION_FLAG:
-          $target = PhabricatorFlagColor::getColorName($target);
-          break;
-        case HeraldAdapter::ACTION_BLOCK:
-          // Target is a text string.
-          $target = $target;
-          break;
-        default:
-          // TODO: This should be driven by HeraldActions.
-
-          if (is_array($target) && $target) {
-            foreach ($target as $k => $phid) {
-              if (isset($handles[$phid])) {
-                $target[$k] = $handles[$phid]->getName();
-              }
-            }
-            $target = implode(', ', $target);
-          } else if (is_string($target)) {
-            $target = $target;
-          } else {
-            $target = '<empty>';
-          }
-          break;
-      }
-
-      $item = new PHUIObjectItemView();
-
-      if ($apply_xscript->getApplied()) {
-        $item->setState(PHUIObjectItemView::STATE_SUCCESS);
-      } else {
-        $item->setState(PHUIObjectItemView::STATE_FAIL);
-      }
-
-      $rule = idx(
-        $action_names,
-        $apply_xscript->getAction(),
-        pht('Unknown Action "%s"', $apply_xscript->getAction()));
-
-      $item->setHeader(pht('%s: %s', $rule, $target));
-      $item->addAttribute($apply_xscript->getReason());
-
-      // TODO: This is a bit of a mess while actions convert.
-
-      $item->addAttribute(
-        pht('Outcome: %s', $apply_xscript->getAppliedReason()));
-
-      $list->addItem($item);
-    }
-
-    $box = new PHUIObjectBoxView();
-    $box->setHeaderText(pht('Actions Taken'));
-    $box->appendChild($list);
-
-    return $box;
-  }
-
   private function buildActionTranscriptPanel(HeraldTranscript $xscript) {
     $action_xscript = mgroup($xscript->getApplyTranscripts(), 'getRuleID');
 
     $adapter = $this->getAdapter();
 
-
     $field_names = $adapter->getFieldNameMap();
     $condition_names = $adapter->getConditionNameMap();
 
     $handles = $this->handles;
 
-    $rule_markup = array();
+    $action_map = $xscript->getApplyTranscripts();
-    foreach ($xscript->getRuleTranscripts() as $rule_id => $rule) {
+    $action_map = mgroup($action_map, 'getRuleID');
-      $cond_markup = array();
+
-      foreach ($xscript->getConditionTranscriptsForRule($rule_id) as $cond) {
+    $rule_list = id(new PHUIObjectItemListView())
-        if ($cond->getNote()) {
+      ->setNoDataString(pht('No Herald rules applied to this object.'));
-          $note = phutil_tag_div('herald-condition-note', $cond->getNote());
+
+    foreach ($xscript->getRuleTranscripts() as $rule_xscript) {
+      $rule_id = $rule_xscript->getRuleID();
+
+      $rule_item = id(new PHUIObjectItemView())
+        ->setObjectName(pht('H%d', $rule_id))
+        ->setHeader($rule_xscript->getRuleName());
+
+      if (!$rule_xscript->getResult()) {
+        $rule_item->setDisabled(true);
+      }
+
+      $rule_list->addItem($rule_item);
+
+      // Build the field/condition transcript.
+
+      $cond_xscripts = $xscript->getConditionTranscriptsForRule($rule_id);
+
+      $cond_list = id(new PHUIStatusListView());
+      $cond_list->addItem(
+        id(new PHUIStatusItemView())
+          ->setTarget(phutil_tag('strong', array(), pht('Conditions'))));
+
+      foreach ($cond_xscripts as $cond_xscript) {
+        if ($cond_xscript->getResult()) {
+          $icon = 'fa-check';
+          $color = 'green';
+          $result = pht('Passed');
+        } else {
+          $icon = 'fa-times';
+          $color = 'red';
+          $result = pht('Failed');
+        }
+
+        if ($cond_xscript->getNote()) {
+          $note = phutil_tag(
+            'div',
+            array(
+              'class' => 'herald-condition-note',
+            ),
+            $cond_xscript->getNote());
         } else {
           $note = null;
         }
 
-        if ($cond->getResult()) {
+        // TODO: This is not really translatable and should be driven through
-          $result = phutil_tag(
+        // HeraldField.
-            'span',
+        $explanation = pht(
-            array('class' => 'herald-outcome condition-pass'),
+          '%s %s %s',
-            "\xE2\x9C\x93");
+          idx($field_names, $cond_xscript->getFieldName(), pht('Unknown')),
-        } else {
+          idx($condition_names, $cond_xscript->getCondition(), pht('Unknown')),
-          $result = phutil_tag(
+          $this->renderConditionTestValue($cond_xscript, $handles));
-            'span',
+
-            array('class' => 'herald-outcome condition-fail'),
+        $cond_item = id(new PHUIStatusItemView())
-            "\xE2\x9C\x98");
+          ->setIcon($icon, $color)
+          ->setTarget($result)
+          ->setNote(array($explanation, $note));
+
+        $cond_list->addItem($cond_item);
       }
 
-        $cond_markup[] = phutil_tag(
+      if ($rule_xscript->getResult()) {
-          'li',
+        $last_icon = 'fa-check-circle';
-          array(),
+        $last_color = 'green';
+        $last_result = pht('Passed');
+        $last_note = pht('Rule passed.');
+      } else {
+        $last_icon = 'fa-times-circle';
+        $last_color = 'red';
+        $last_result = pht('Failed');
+        $last_note = pht('Rule failed.');
+      }
+
+      $cond_last = id(new PHUIStatusItemView())
+        ->setIcon($last_icon, $last_color)
+        ->setTarget(phutil_tag('strong', array(), $last_result))
+        ->setNote($last_note);
+      $cond_list->addItem($cond_last);
+
+      $cond_box = id(new PHUIBoxView())
+        ->appendChild($cond_list)
+        ->addMargin(PHUI::MARGIN_LARGE_LEFT);
+
+      $rule_item->appendChild($cond_box);
+
+      if (!$rule_xscript->getResult()) {
+        // If the rule didn't pass, don't generate an action transcript since
+        // actions didn't apply.
+        continue;
+      }
+
+      $cond_box->addMargin(PHUI::MARGIN_MEDIUM_BOTTOM);
+
+      $action_xscripts = idx($action_map, $rule_id, array());
+      foreach ($action_xscripts as $action_xscript) {
+        $action_key = $action_xscript->getAction();
+        $action = $adapter->getActionImplementation($action_key);
+
+        if ($action) {
+          $name = $action->getHeraldActionName();
+          $action->setViewer($this->getViewer());
+        } else {
+          $name = pht('Unknown Action ("%s")', $action_key);
+        }
+
+        $name = pht('Action: %s', $name);
+
+        $action_list = id(new PHUIStatusListView());
+        $action_list->addItem(
+          id(new PHUIStatusItemView())
+            ->setTarget(phutil_tag('strong', array(), $name)));
+
+        $action_box = id(new PHUIBoxView())
+          ->appendChild($action_list)
+          ->addMargin(PHUI::MARGIN_LARGE_LEFT);
+
+        $rule_item->appendChild($action_box);
+
+        $log = $action_xscript->getAppliedReason();
+
+        // Handle older transcripts which used a static string to record
+        // action results.
+        if (!is_array($log)) {
+          $action_list->addItem(
+            id(new PHUIStatusItemView())
+              ->setIcon('fa-clock-o', 'grey')
+              ->setTarget(pht('Old Transcript'))
+              ->setNote(
                 pht(
-            '%s Condition: %s %s %s%s',
+                  'This is an old transcript which uses an obsolete log '.
-            $result,
+                  'format. Detailed action information is not available.')));
-            idx($field_names, $cond->getFieldName(), pht('Unknown')),
+          continue;
-            idx($condition_names, $cond->getCondition(), pht('Unknown')),
-            $this->renderConditionTestValue($cond, $handles),
-            $note));
         }
 
-      if ($rule->getResult()) {
+        foreach ($log as $entry) {
-        $result = phutil_tag(
+          $type = idx($entry, 'type');
-          'span',
+          $data = idx($entry, 'data');
-          array('class' => 'herald-outcome rule-pass'),
+
-          pht('PASS'));
+          if ($action) {
-        $class = 'herald-rule-pass';
+            $icon = $action->renderActionEffectIcon($type, $data);
+            $color = $action->renderActionEffectColor($type, $data);
+            $name = $action->renderActionEffectName($type, $data);
+            $note = $action->renderActionEffectDescription($type, $data);
           } else {
-        $result = phutil_tag(
+            $icon = 'fa-question-circle';
-          'span',
+            $color = 'indigo';
-          array('class' => 'herald-outcome rule-fail'),
+            $name = pht('Unknown Effect ("%s")', $type);
-          pht('FAIL'));
+            $note = null;
-        $class = 'herald-rule-fail';
           }
 
-      $cond_markup[] = phutil_tag(
+          $action_item = id(new PHUIStatusItemView())
-        'li',
+            ->setIcon($icon, $color)
-        array(),
+            ->setTarget($name)
-        array($result, $rule->getReason()));
+            ->setNote($note);
-      $user_phid = $this->getRequest()->getUser()->getPHID();
 
-      $name = $rule->getRuleName();
+          $action_list->addItem($action_item);
-
+        }
-      $rule_markup[] =
+      }
-        phutil_tag(
-          'li',
-          array(
-            'class' => $class,
-          ),
-          phutil_tag_div('rule-name', array(
-            phutil_tag('strong', array(), $name),
-            ' ',
-            phutil_tag('ul', array(), $cond_markup),
-          )));
     }
 
-    $box = null;
+    $box = id(new PHUIObjectBoxView())
-    if ($rule_markup) {
+      ->setHeaderText(pht('Rule Transcript'))
-      $box = new PHUIObjectBoxView();
+      ->appendChild($rule_list);
-      $box->setHeaderText(pht('Rule Details'));
+
-      $box->appendChild(phutil_tag(
-        'ul',
-        array('class' => 'herald-explain-list'),
-        $rule_markup));
-    }
     return $box;
   }
 

src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php

@@ -84,7 +84,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter {
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
-            self::ACTION_FLAG,
             self::ACTION_ASSIGN_TASK,
           ),
           parent::getActions($rule_type));

src/applications/pholio/herald/HeraldPholioMockAdapter.php

@@ -61,7 +61,6 @@ final class HeraldPholioMockAdapter extends HeraldAdapter {
           array(
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
-            self::ACTION_FLAG,
           ),
           parent::getActions($rule_type));
     }

src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php

@@ -64,7 +64,6 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter {
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
             self::ACTION_EMAIL,
-            self::ACTION_FLAG,
           ),
           parent::getActions($rule_type));
     }

webroot/rsrc/css/application/herald/herald-test.css

@@ -2,79 +2,10 @@
  * @provides herald-test-css
  */
 
-ul.herald-explain-list {
+.herald-condition-note {
-  margin: 12px;
-}
-
-div.herald-condition-note {
-  clear: both;
-  margin: .5em 0em .53em 86px;
-  padding: .5em 1em;
-  background: #FFFF00;
-  font-weight: bold;
-}
-
-ul.herald-explain-list .herald-outcome {
-  margin-right: 6px;
-  width: 50px;
-  text-align: center;
-  position: relative;
-  float: left;
-  font-weight: bold;
-  padding: 1px 2px;
-}
-
-ul.herald-explain-list .condition-fail,
-ul.herald-explain-list .rule-fail {
   color: {$red};
 }
 
-ul.herald-explain-list .condition-pass,
-ul.herald-explain-list .rule-pass {
-  color: {$green};
-}
-
-ul.herald-explain-list li.herald-rule-pass,
-ul.herald-explain-list li.herald-rule-fail {
-  margin: 0 0 8px;
-}
-
-ul.herald-explain-list div.rule-name {
-  padding: 4px 0 8px 0;
-}
-
-ul.herald-explain-list li.herald-rule-fail,
-ul.herald-explain-list li.herald-rule-pass {
-  background: #ffffff;
-}
-
-ul.herald-explain-list ul {
-  margin: 4px;
-}
-
-ul.herald-explain-list ul li {
-  padding: 2px 0;
-}
-
-.outcome-failure,
-.outcome-success {
-  font-weight: bold;
-}
-
-.outcome-failure {
-  color: {$red};
-}
-
-.outcome-success {
-  color: {$green};
-}
-
-.action-header {
-  font-weight: bold;
-  padding-top: 12px;
-  border-bottom: 1px solid {$thinblueborder};
-}
-
 textarea.herald-field-value-transcript {
   width: 100%;
   height: 10em;