General Herald refactoring pass

Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:

  - Personal rules can be deleted only by their owners.
  - Global rules can be deleted by any user.
  - All deletes are logged.
  - Logs are more detailed.
  - All logged actions can be viewed in aggregate.

**Minor Cleanup**

  - Merged `HomeController` and `AllController`.
  - Moved most queries to Query classes.
  - Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
  - Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
  - Reenable some transaction code (this works again now).
  - Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
  - Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
  - Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
  - Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.

Test Plan:
  - Browsed, created, edited, deleted personal and gules.
  - Verified generated logs.
  - Did some dry runs.
  - Verified transcript list and transcript details.
  - Created/edited all/any rules; created/edited once/every time rules.
  - Filtered admin views by users.

Reviewers: jungejason, btrahan

Reviewed By: btrahan

CC: aran, epriestley

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

resources/sql/patches/123.heraldrulelog.sql

@@ -0,0 +1,5 @@
+ALTER TABLE phabricator_herald.herald_ruleedit
+  ADD ruleName VARCHAR(255) NOT NULL COLLATE utf8_general_ci;
+
+ALTER TABLE phabricator_herald.herald_ruleedit
+  ADD action VARCHAR(32) NOT NULL COLLATE utf8_general_ci;

src/__celerity_resource_map__.php

@@ -304,7 +304,7 @@ celerity_register_resource_map(array(
   ),
   'herald-css' =>
   array(
-    'uri' => '/res/ed5556e6/rsrc/css/application/herald/herald.css',
+    'uri' => '/res/5051f3ab/rsrc/css/application/herald/herald.css',
     'type' => 'css',
     'requires' =>
     array(
@@ -313,7 +313,7 @@ celerity_register_resource_map(array(
   ),
   'herald-rule-editor' =>
   array(
-    'uri' => '/res/745c9d43/rsrc/js/application/herald/HeraldRuleEditor.js',
+    'uri' => '/res/209ed8c4/rsrc/js/application/herald/HeraldRuleEditor.js',
     'type' => 'js',
     'requires' =>
     array(

src/__phutil_library_map__.php

@@ -395,6 +395,7 @@ phutil_register_library_map(array(
     'HeraldDeleteController' => 'applications/herald/controller/delete',
     'HeraldDifferentialRevisionAdapter' => 'applications/herald/adapter/differential',
     'HeraldDryRunAdapter' => 'applications/herald/adapter/dryrun',
+    'HeraldEditLogQuery' => 'applications/herald/query/log',
     'HeraldEffect' => 'applications/herald/engine/effect',
     'HeraldEngine' => 'applications/herald/engine/engine',
     'HeraldFieldConfig' => 'applications/herald/config/field',
@@ -412,6 +413,7 @@ phutil_register_library_map(array(
     'HeraldRuleEditHistoryController' => 'applications/herald/controller/edithistory',
     'HeraldRuleEditHistoryView' => 'applications/herald/view/edithistory',
     'HeraldRuleListView' => 'applications/herald/view/rulelist',
+    'HeraldRuleQuery' => 'applications/herald/query/rule',
     'HeraldRuleTranscript' => 'applications/herald/storage/transcript/rule',
     'HeraldRuleTypeConfig' => 'applications/herald/config/ruletype',
     'HeraldTestConsoleController' => 'applications/herald/controller/test',
@@ -1188,6 +1190,7 @@ phutil_register_library_map(array(
     'DiffusionContainsQuery' => 'DiffusionQuery',
     'DiffusionController' => 'PhabricatorController',
     'DiffusionDiffController' => 'DiffusionController',
+    'DiffusionDiffQuery' => 'DiffusionQuery',
     'DiffusionEmptyResultView' => 'DiffusionView',
     'DiffusionExternalController' => 'DiffusionController',
     'DiffusionFileContentQuery' => 'DiffusionQuery',
@@ -1208,6 +1211,7 @@ phutil_register_library_map(array(
     'DiffusionHomeController' => 'DiffusionController',
     'DiffusionInlineCommentController' => 'PhabricatorInlineCommentController',
     'DiffusionLastModifiedController' => 'DiffusionController',
+    'DiffusionLastModifiedQuery' => 'DiffusionQuery',
     'DiffusionMercurialBranchQuery' => 'DiffusionBranchQuery',
     'DiffusionMercurialBrowseQuery' => 'DiffusionBrowseQuery',
     'DiffusionMercurialCommitParentsQuery' => 'DiffusionCommitParentsQuery',
@@ -1268,6 +1272,7 @@ phutil_register_library_map(array(
     'HeraldDeleteController' => 'HeraldController',
     'HeraldDifferentialRevisionAdapter' => 'HeraldObjectAdapter',
     'HeraldDryRunAdapter' => 'HeraldObjectAdapter',
+    'HeraldEditLogQuery' => 'PhabricatorOffsetPagedQuery',
     'HeraldHomeController' => 'HeraldController',
     'HeraldNewController' => 'HeraldController',
     'HeraldRule' => 'HeraldDAO',
@@ -1276,6 +1281,7 @@ phutil_register_library_map(array(
     'HeraldRuleEditHistoryController' => 'HeraldController',
     'HeraldRuleEditHistoryView' => 'AphrontView',
     'HeraldRuleListView' => 'AphrontView',
+    'HeraldRuleQuery' => 'PhabricatorOffsetPagedQuery',
     'HeraldTestConsoleController' => 'HeraldController',
     'HeraldTranscript' => 'HeraldDAO',
     'HeraldTranscriptController' => 'HeraldController',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -282,19 +282,14 @@ class AphrontDefaultApplicationConfiguration
 
       '/herald/' => array(
         '' => 'HeraldHomeController',
-        'view/(?P<view>[^/]+)/' => array(
+        'view/(?P<content_type>[^/]+)/(?:(?P<rule_type>[^/]+)/)?'
-          '' => 'HeraldHomeController',
+          => 'HeraldHomeController',
-          '(?P<global>global)/' => 'HeraldHomeController'
+        'new/(?:(?P<type>[^/]+)/(?:(?P<rule_type>[^/]+)/)?)?'
-        ),
+          => 'HeraldNewController',
-        'new/(?:(?P<type>[^/]+)/)?' => 'HeraldNewController',
         'rule/(?:(?P<id>\d+)/)?' => 'HeraldRuleController',
-        'history/(?P<id>\d+)/' => 'HeraldRuleEditHistoryController',
+        'history/(?:(?P<id>\d+)/)?' => 'HeraldRuleEditHistoryController',
         'delete/(?P<id>\d+)/' => 'HeraldDeleteController',
         'test/' => 'HeraldTestConsoleController',
-        'all/' => array(
-          '' => 'HeraldAllRulesController',
-          'view/(?P<view>[^/]+)/' => 'HeraldAllRulesController',
-        ),
         'transcript/' => 'HeraldTranscriptListController',
         'transcript/(?P<id>\d+)/(?:(?P<filter>\w+)/)?'
           => 'HeraldTranscriptController',

src/applications/herald/controller/all/HeraldAllRulesController.php

@@ -1,173 +0,0 @@
-<?php
-
-/*
- * Copyright 2012 Facebook, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-final class HeraldAllRulesController extends HeraldController {
-
-  private $view;
-  private $viewPHID;
-  private $filter;
-
-  public function getFilter() {
-    return $this->filter;
-  }
-  public function setFilter($filter) {
-    $this->filter = 'all/view/'.$filter;
-    return $this;
-  }
-
-  public function shouldRequireAdmin() {
-    return true;
-  }
-
-  public function willProcessRequest(array $data) {
-    $this->view = idx($data, 'view');
-    $this->setFilter($this->view);
-  }
-
-  public function processRequest() {
-
-    $request = $this->getRequest();
-    $user = $request->getUser();
-    $this->viewPHID = nonempty($request->getStr('phid'), null);
-
-    if ($request->isFormPost()) {
-      $phid_arr = $request->getArr('view_user');
-      $view_target = head($phid_arr);
-      return id(new AphrontRedirectResponse())
-        ->setURI($request->getRequestURI()->alter('phid', $view_target));
-    }
-
-    $map = HeraldContentTypeConfig::getContentTypeMap();
-    if (empty($map[$this->view])) {
-      reset($map);
-      $this->view = key($map);
-    }
-
-    $offset = $request->getInt('offset', 0);
-    $pager = new AphrontPagerView();
-    $pager->setPageSize(50);
-    $pager->setOffset($offset);
-    $pager->setURI($request->getRequestURI(), 'offset');
-
-    list($rules, $handles) = $this->queryRules($pager);
-
-    if (!$this->viewPHID) {
-      $view_users = array();
-    } else {
-      $view_users = array(
-        $this->viewPHID => $handles[$this->viewPHID]->getFullName(),
-      );
-    }
-
-    $filter_form = id(new AphrontFormView())
-      ->setUser($user)
-      ->appendChild(
-        id(new AphrontFormTokenizerControl())
-          ->setDatasource('/typeahead/common/users/')
-          ->setLabel('View User')
-          ->setName('view_user')
-          ->setValue($view_users)
-          ->setLimit(1));
-    $filter_view = new AphrontListFilterView();
-    $filter_view->appendChild($filter_form);
-
-    $list_view = id(new HeraldRuleListView())
-      ->setRules($rules)
-      ->setHandles($handles)
-      ->setMap($map)
-      ->setShowType(true)
-      ->setAllowCreation(false)
-      ->setUser($user)
-      ->setView($this->view);
-    $panel = $list_view->render();
-    $panel->appendChild($pager);
-
-    $sidenav = new AphrontSideNavView();
-    $sidenav->appendChild($filter_view);
-    $sidenav->appendChild($panel);
-
-    $query = '';
-    if ($this->viewPHID) {
-      $query = '?phid='.$this->viewPHID;
-    }
-
-    foreach ($map as $key => $value) {
-      $sidenav->addNavItem(
-        phutil_render_tag(
-          'a',
-          array(
-            'href' => '/herald/all/view/'.$key.'/'.$query,
-            'class' => ($key == $this->view)
-              ? 'aphront-side-nav-selected'
-              : null,
-          ),
-          phutil_escape_html($value)));
-    }
-
-    return $this->buildStandardPageResponse(
-      array(
-        $filter_view,
-        $panel
-      ),
-      array(
-        'title' => 'Herald',
-        'tab' => 'all',
-      ));
-  }
-
-  private function queryRules(AphrontPagerView $pager) {
-    $rule = new HeraldRule();
-    $conn_r = $rule->establishConnection('r');
-
-    $where_clause = qsprintf(
-      $conn_r,
-      'WHERE contentType = %s',
-      $this->view);
-
-    if ($this->viewPHID) {
-      $where_clause .= qsprintf(
-        $conn_r,
-        ' AND authorPHID = %s',
-        $this->viewPHID);
-    }
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T
-        %Q
-        ORDER BY id DESC
-        LIMIT %d, %d',
-      $rule->getTableName(),
-      $where_clause,
-      $pager->getOffset(),
-      $pager->getPageSize() + 1);
-    $data = $pager->sliceResults($data);
-    $rules = $rule->loadAllFromArray($data);
-
-    $need_phids = mpull($rules, 'getAuthorPHID');
-    if ($this->viewPHID) {
-      $need_phids[] = $this->viewPHID;
-    }
-
-    $handles = id(new PhabricatorObjectHandleData($need_phids))
-      ->loadHandles();
-
-    return array($rules, $handles);
-  }
-}
-

src/applications/herald/controller/all/__init__.php

@@ -1,27 +0,0 @@
-<?php
-/**
- * This file is automatically generated. Lint this module to rebuild it.
- * @generated
- */
-
-
-
-phutil_require_module('phabricator', 'aphront/response/redirect');
-phutil_require_module('phabricator', 'applications/herald/config/contenttype');
-phutil_require_module('phabricator', 'applications/herald/controller/base');
-phutil_require_module('phabricator', 'applications/herald/storage/rule');
-phutil_require_module('phabricator', 'applications/herald/view/rulelist');
-phutil_require_module('phabricator', 'applications/phid/handle/data');
-phutil_require_module('phabricator', 'storage/qsprintf');
-phutil_require_module('phabricator', 'storage/queryfx');
-phutil_require_module('phabricator', 'view/control/pager');
-phutil_require_module('phabricator', 'view/form/base');
-phutil_require_module('phabricator', 'view/form/control/tokenizer');
-phutil_require_module('phabricator', 'view/layout/listfilter');
-phutil_require_module('phabricator', 'view/layout/sidenav');
-
-phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'utils');
-
-
-phutil_require_source('HeraldAllRulesController.php');

src/applications/herald/controller/base/HeraldController.php

@@ -28,48 +28,6 @@ abstract class HeraldController extends PhabricatorController {
 
     $doclink = PhabricatorEnv::getDoclink('article/Herald_User_Guide.html');
 
-    $nav = new AphrontSideNavFilterView();
-    $nav
-      ->setBaseURI(new PhutilURI('/herald/'))
-      ->addLabel('My Rules')
-      ->addFilter('new', 'Create Rule');
-    $rules_map = HeraldContentTypeConfig::getContentTypeMap();
-    $first_filter = null;
-    foreach ($rules_map as $key => $value) {
-      $nav->addFilter('view/'.$key, $value);
-      if (!$first_filter) {
-        $first_filter = 'view/'.$key;
-      }
-    }
-    $nav
-      ->addSpacer()
-      ->addLabel('Global Rules');
-    foreach ($rules_map as $key => $value) {
-      $nav->addFilter("view/${key}/global", $value);
-    }
-
-    $nav
-      ->addSpacer()
-      ->addLabel('Utilities')
-      ->addFilter('test',       'Test Console')
-      ->addFilter('transcript', 'Transcripts');
-
-    $user = $this->getRequest()->getUser();
-    if ($user->getIsAdmin()) {
-      $nav
-        ->addSpacer()
-        ->addLabel('Admin');
-      $view_PHID = nonempty($this->getRequest()->getStr('phid'), null);
-      foreach ($rules_map as $key => $value) {
-        $nav
-          ->addFilter('all/view/'.$key, $value);
-      }
-    }
-
-    $nav->selectFilter($this->getFilter(), $first_filter);
-    $nav->appendChild($view);
-    $page->appendChild($nav);
-
     $tabs = array(
       'help' => array(
         'href' => $doclink,
@@ -78,10 +36,49 @@ abstract class HeraldController extends PhabricatorController {
     );
     $page->setTabs($tabs, null);
 
+    $page->appendChild($view);
+    $page->setIsAdminInterface(idx($data, 'admin'));
+
     $response = new AphrontWebpageResponse();
     return $response->setContent($page->render());
-
   }
 
-  abstract function getFilter();
+  protected function renderNav() {
+    $nav = id(new AphrontSideNavFilterView())
+      ->setBaseURI(new PhutilURI('/herald/'))
+      ->addLabel('My Rules')
+      ->addFilter('new', 'Create Rule');
+
+    $rules_map = HeraldContentTypeConfig::getContentTypeMap();
+    foreach ($rules_map as $key => $value) {
+      $nav->addFilter("view/{$key}/personal", $value);
+    }
+
+    $nav
+      ->addSpacer()
+      ->addLabel('Global Rules');
+
+    foreach ($rules_map as $key => $value) {
+      $nav->addFilter("view/{$key}/global", $value);
+    }
+
+    $nav
+      ->addSpacer()
+      ->addLabel('Utilities')
+      ->addFilter('test',       'Test Console')
+      ->addFilter('transcript', 'Transcripts')
+      ->addFilter('history',    'Edit Log');
+
+    if ($this->getRequest()->getUser()->getIsAdmin()) {
+      $nav
+        ->addSpacer()
+        ->addLabel('Admin');
+      foreach ($rules_map as $key => $value) {
+        $nav->addFilter("view/{$key}/all", $value);
+      }
+    }
+
+    return $nav;
+  }
+
 }

src/applications/herald/controller/delete/HeraldDeleteController.php

@@ -40,17 +40,18 @@ final class HeraldDeleteController extends HeraldController {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    if ($user->getPHID() != $rule->getAuthorPHID() && !$user->getIsAdmin()) {
+    // Anyone can delete a global rule, but only the rule owner can delete a
+    // personal one.
+    if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      if ($user->getPHID() != $rule->getAuthorPHID()) {
         return new Aphront400Response();
       }
+    }
 
     if ($request->isFormPost()) {
+      $rule->logEdit($user->getPHID(), 'delete');
       $rule->delete();
-      if ($request->isAjax()) {
+      return id(new AphrontReloadResponse())->setURI('/herald/');
-        return new AphrontRedirectResponse();
-      } else {
-        return id(new AphrontRedirectResponse())->setURI('/herald/');
-      }
     }
 
     $dialog = new AphrontDialogView();

src/applications/herald/controller/delete/__init__.php

@@ -9,7 +9,8 @@
 phutil_require_module('phabricator', 'aphront/response/400');
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/dialog');
-phutil_require_module('phabricator', 'aphront/response/redirect');
+phutil_require_module('phabricator', 'aphront/response/reload');
+phutil_require_module('phabricator', 'applications/herald/config/ruletype');
 phutil_require_module('phabricator', 'applications/herald/controller/base');
 phutil_require_module('phabricator', 'applications/herald/storage/rule');
 phutil_require_module('phabricator', 'view/dialog');

src/applications/herald/controller/edithistory/HeraldRuleEditHistoryController.php

@@ -21,35 +21,45 @@ final class HeraldRuleEditHistoryController extends HeraldController {
   private $id;
 
   public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
+    $this->id = idx($data, 'id');
   }
 
   public function processRequest() {
-    $rule = id(new HeraldRule())->load($this->id);
+    $request = $this->getRequest();
-    if ($rule === null) {
+
-      return new Aphront404Response();
+    $edit_query = new HeraldEditLogQuery();
+    if ($this->id) {
+      $edit_query->withRuleIDs(array($this->id));
     }
 
-    $edits = $rule->loadEdits();
+    $pager = new AphrontPagerView();
-    $rule->attachEdits($edits);
+    $pager->setURI($request->getRequestURI(), 'offset');
+    $pager->setOffset($request->getStr('offset'));
+
+    $edits = $edit_query->executeWithPager($pager);
 
     $need_phids = mpull($edits, 'getEditorPHID');
     $handles = id(new PhabricatorObjectHandleData($need_phids))
       ->loadHandles();
 
     $list_view = id(new HeraldRuleEditHistoryView())
-      ->setRule($rule)
+      ->setEdits($edits)
       ->setHandles($handles)
       ->setUser($this->getRequest()->getUser());
 
+    $panel = new AphrontPanelView();
+    $panel->setHeader('Edit History');
+    $panel->appendChild($list_view);
+
+    $nav = $this->renderNav();
+    $nav->selectFilter('history');
+    $nav->appendChild($panel);
+
     return $this->buildStandardPageResponse(
-      $list_view->render(),
+      $nav,
       array(
         'title' => 'Rule Edit History',
       ));
   }
 
-  public function getFilter() {
-    return;
-  }
 }

src/applications/herald/controller/edithistory/__init__.php

@@ -6,11 +6,12 @@
 
 
 
-phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'applications/herald/controller/base');
-phutil_require_module('phabricator', 'applications/herald/storage/rule');
+phutil_require_module('phabricator', 'applications/herald/query/log');
 phutil_require_module('phabricator', 'applications/herald/view/edithistory');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
+phutil_require_module('phabricator', 'view/control/pager');
+phutil_require_module('phabricator', 'view/layout/panel');
 
 phutil_require_module('phutil', 'utils');
 

src/applications/herald/controller/home/HeraldHomeController.php

@@ -18,56 +18,82 @@
 
 final class HeraldHomeController extends HeraldController {
 
-  private $view;
+  private $contentType;
-  private $filter;
+  private $ruleType;
-  private $global;
 
   public function willProcessRequest(array $data) {
-    $this->view = idx($data, 'view');
+    $this->contentType = idx($data, 'content_type');
-    $this->global = idx($data, 'global');
+    $this->ruleType = idx($data, 'rule_type');
-    if ($this->global) {
-      $this->setFilter($this->view.'/global');
-    } else {
-      $this->setFilter($this->view);
-    }
-  }
-
-  public function getFilter() {
-    return $this->filter;
-  }
-  public function setFilter($filter) {
-    $this->filter = 'view/'.$filter;
-    return $this;
   }
 
   public function processRequest() {
-
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $map = HeraldContentTypeConfig::getContentTypeMap();
+    if ($request->isFormPost()) {
-    if (empty($map[$this->view])) {
+      $phids = $request->getArr('set_phid');
-      reset($map);
+      $phid = head($phids);
-      $this->view = key($map);
+
+      $uri = $request->getRequestURI();
+      if ($phid) {
+        $uri = $uri->alter('phid', nonempty($phid, null));
       }
 
-    if ($this->global) {
+      return id(new AphrontRedirectResponse())->setURI($uri);
-      $rules = id(new HeraldRule())->loadAllWhere(
-        'contentType = %s AND ruleType = %s',
-        $this->view,
-        HeraldRuleTypeConfig::RULE_TYPE_GLOBAL);
-    } else {
-      $rules = id(new HeraldRule())->loadAllWhere(
-        'contentType = %s AND authorPHID = %s AND ruleType = %s',
-        $this->view,
-        $user->getPHID(),
-        HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
     }
 
-    foreach ($rules as $rule) {
+    $query = new HeraldRuleQuery();
-      $edits = $rule->loadEdits();
+
-      $rule->attachEdits($edits);
+    $content_type_map = HeraldContentTypeConfig::getContentTypeMap();
+    if (empty($content_type_map[$this->contentType])) {
+      $this->contentType = key($content_type_map);
     }
+    $content_desc = $content_type_map[$this->contentType];
+
+    $query->withContentTypes(array($this->contentType));
+
+    $is_admin_page = false;
+    $show_author = false;
+    $show_rule_type = false;
+    $can_create = false;
+    $has_author_filter = false;
+    $author_filter_phid = null;
+
+    switch ($this->ruleType) {
+      case 'all':
+        if (!$user->getIsAdmin()) {
+          return new Aphront400Response();
+        }
+        $is_admin_page = true;
+        $show_rule_type = true;
+        $show_author = true;
+        $has_author_filter = true;
+        $author_filter_phid = $request->getStr('phid');
+        if ($author_filter_phid) {
+          $query->withAuthorPHIDs(array($author_filter_phid));
+        }
+        $rule_desc = 'All';
+        break;
+      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+        $query->withRuleTypes(array(HeraldRuleTypeConfig::RULE_TYPE_GLOBAL));
+        $can_create = true;
+        $rule_desc = 'Global';
+        break;
+      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
+      default:
+        $this->ruleType = HeraldRuleTypeConfig::RULE_TYPE_PERSONAL;
+        $query->withRuleTypes(array(HeraldRuleTypeConfig::RULE_TYPE_PERSONAL));
+        $query->withAuthorPHIDs(array($user->getPHID()));
+        $can_create = true;
+        $rule_desc = 'Personal';
+        break;
+    }
+
+    $pager = new AphrontPagerView();
+    $pager->setURI($request->getRequestURI(), 'offset');
+    $pager->setOffset($request->getStr('offset'));
+
+    $rules = $query->executeWithPager($pager);
 
     $need_phids = mpull($rules, 'getAuthorPHID');
     $handles = id(new PhabricatorObjectHandleData($need_phids))
@@ -75,19 +101,73 @@ final class HeraldHomeController extends HeraldController {
 
     $list_view = id(new HeraldRuleListView())
       ->setRules($rules)
-      ->setShowOwner(!$this->global)
+      ->setShowAuthor($show_author)
+      ->setShowRuleType($show_rule_type)
       ->setHandles($handles)
-      ->setMap($map)
-      ->setAllowCreation(true)
-      ->setView($this->view)
       ->setUser($user);
-    $panel = $list_view->render();
 
+    $panel = new AphrontPanelView();
+    $panel->appendChild($list_view);
+    $panel->appendChild($pager);
+
+    $panel->setHeader("Herald: {$rule_desc} Rules for {$content_desc}");
+
+    if ($can_create) {
+      $panel->addButton(
+        phutil_render_tag(
+          'a',
+          array(
+            'href' => '/herald/new/'.$this->contentType.'/'.$this->ruleType.'/',
+            'class' => 'green button',
+          ),
+          'Create New Herald Rule'));
+    }
+
+
+    $nav = $this->renderNav();
+    $nav->selectFilter('view/'.$this->contentType.'/'.$this->ruleType);
+
+    if ($has_author_filter) {
+      $nav->appendChild($this->renderAuthorFilter($author_filter_phid));
+    }
+
+    $nav->appendChild($panel);
 
     return $this->buildStandardPageResponse(
-      $panel,
+      $nav,
       array(
         'title' => 'Herald',
+        'admin' => $is_admin_page,
       ));
   }
+
+  private function renderAuthorFilter($phid) {
+    if ($phid) {
+      $handle = PhabricatorObjectHandleData::loadOneHandle($phid);
+      $tokens = array(
+        $phid => $handle->getFullName(),
+      );
+    } else {
+      $tokens = array();
+    }
+
+    $form = id(new AphrontFormView())
+      ->setUser($this->getRequest()->getUser())
+      ->appendChild(
+        id(new AphrontFormTokenizerControl())
+          ->setName('set_phid')
+          ->setValue($tokens)
+          ->setLimit(1)
+          ->setLabel('Filter Author')
+          ->setDataSource('/typeahead/common/accounts/'))
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+          ->setValue('Apply Filter'));
+
+    $filter = new AphrontListFilterView();
+    $filter->appendChild($form);
+    return $filter;
+  }
+
+
 }

src/applications/herald/controller/home/__init__.php

@@ -6,13 +6,22 @@
 
 
 
+phutil_require_module('phabricator', 'aphront/response/400');
+phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/herald/config/contenttype');
 phutil_require_module('phabricator', 'applications/herald/config/ruletype');
 phutil_require_module('phabricator', 'applications/herald/controller/base');
-phutil_require_module('phabricator', 'applications/herald/storage/rule');
+phutil_require_module('phabricator', 'applications/herald/query/rule');
 phutil_require_module('phabricator', 'applications/herald/view/rulelist');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
+phutil_require_module('phabricator', 'view/control/pager');
+phutil_require_module('phabricator', 'view/form/base');
+phutil_require_module('phabricator', 'view/form/control/submit');
+phutil_require_module('phabricator', 'view/form/control/tokenizer');
+phutil_require_module('phabricator', 'view/layout/listfilter');
+phutil_require_module('phabricator', 'view/layout/panel');
 
+phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/herald/controller/new/HeraldNewController.php

@@ -19,13 +19,11 @@
 final class HeraldNewController extends HeraldController {
 
   private $contentType;
-
+  private $ruleType;
-  public function getFilter() {
-    return 'new';
-  }
 
   public function willProcessRequest(array $data) {
     $this->contentType = idx($data, 'type');
+    $this->ruleType = idx($data, 'rule_type');
   }
 
   public function processRequest() {
@@ -40,6 +38,9 @@ final class HeraldNewController extends HeraldController {
     }
 
     $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();
+    if (empty($rule_type_map[$this->ruleType])) {
+      $this->ruleType = HeraldRuleTypeConfig::RULE_TYPE_PERSONAL;
+    }
 
     // Reorder array to put "personal" first.
     $rule_type_map = array_select_keys(
@@ -61,7 +62,7 @@ final class HeraldNewController extends HeraldController {
     $radio = id(new AphrontFormRadioButtonControl())
       ->setLabel('Type')
       ->setName('rule_type')
-      ->setValue(HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+      ->setValue($this->ruleType);
 
     foreach ($rule_type_map as $value => $name) {
       $radio->addButton(
@@ -90,8 +91,12 @@ final class HeraldNewController extends HeraldController {
     $panel->setWidth(AphrontPanelView::WIDTH_FULL);
     $panel->appendChild($form);
 
+    $nav = $this->renderNav();
+    $nav->selectFilter('new');
+    $nav->appendChild($panel);
+
     return $this->buildStandardPageResponse(
-      $panel,
+      $nav,
       array(
         'title' => 'Create Herald Rule',
       ));

src/applications/herald/controller/rule/HeraldRuleController.php

@@ -21,13 +21,6 @@ final class HeraldRuleController extends HeraldController {
   private $id;
   private $filter;
 
-  public function getFilter() {
-    return $this->filter;
-  }
-  public function setFilter($filter) {
-    $this->filter = 'view/'.$filter;
-  }
-
   public function willProcessRequest(array $data) {
     $this->id = (int)idx($data, 'id');
   }
@@ -65,13 +58,13 @@ final class HeraldRuleController extends HeraldController {
       }
       $rule->setRuleType($rule_type);
     }
-    $this->setFilter($rule->getContentType());
 
     $local_version = id(new HeraldRule())->getConfigVersion();
     if ($rule->getConfigVersion() > $local_version) {
       throw new Exception(
         "This rule was created with a newer version of Herald. You can not ".
-        "view or edit it in this older version. Try dev or wait for a push.");
+        "view or edit it in this older version. Upgrade your Phabricator ".
+        "deployment.");
     }
 
     // Upgrade rule version to our version, since we might add newly-defined
@@ -88,16 +81,11 @@ final class HeraldRuleController extends HeraldController {
     $errors = array();
     if ($request->isFormPost() && $request->getStr('save')) {
       list($e_name, $errors) = $this->saveRule($rule, $request);
-
       if (!$errors) {
-        $uri = '/herald/view/'.$rule->getContentType().'/';
+        $uri = '/herald/view/'.
-
+          $rule->getContentType().'/'.
-        if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) {
+          $rule->getRuleType().'/';
-          $uri .= 'global/';
+        return id(new AphrontRedirectResponse())->setURI($uri);
-        }
-
-        return id(new AphrontRedirectResponse())
-          ->setURI($uri);
       }
     }
 
@@ -109,8 +97,8 @@ final class HeraldRuleController extends HeraldController {
       $error_view = null;
     }
 
-    $must_match_selector = $this->getMustMatchSelector($rule);
+    $must_match_selector = $this->renderMustMatchSelector($rule);
-    $repetition_selector = $this->getRepetitionSelector($rule);
+    $repetition_selector = $this->renderRepetitionSelector($rule);
 
     $handles = $this->loadHandles($rule);
 
@@ -126,7 +114,8 @@ final class HeraldRuleController extends HeraldController {
       ->addHiddenInput('rule_type', $rule->getRuleType())
       ->addHiddenInput('save', 1)
       ->appendChild(
-        // Build this explicitly so we can add a sigil to it.
+        // Build this explicitly (instead of using addHiddenInput())
+        // so we can add a sigil to it.
         javelin_render_tag(
           'input',
           array(
@@ -141,23 +130,6 @@ final class HeraldRuleController extends HeraldController {
           ->setError($e_name)
           ->setValue($rule->getName()));
 
-    if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
-      $form
-        ->appendChild(
-          id(new AphrontFormMarkupControl())
-            ->setLabel('Owner')
-            ->setValue('<div id="author-input"/>'))
-      ->appendChild(
-        // Build this explicitly so we can add a sigil to it.
-        javelin_render_tag(
-          'input',
-          array(
-            'type'  => 'hidden',
-            'name'  => 'author',
-            'sigil' => 'author',
-          )));
-    }
-
     $form
       ->appendChild(
         id(new AphrontFormMarkupControl())
@@ -215,15 +187,24 @@ final class HeraldRuleController extends HeraldController {
     $this->setupEditorBehavior($rule, $handles);
 
     $panel = new AphrontPanelView();
-    $panel->setHeader('Edit Herald Rule');
+    $panel->setHeader(
+      $rule->getID()
+        ? 'Edit Herald Rule'
+        : 'Create Herald Rule');
     $panel->setWidth(AphrontPanelView::WIDTH_WIDE);
     $panel->appendChild($form);
 
-    return $this->buildStandardPageResponse(
+    $nav = $this->renderNav();
+    $nav->selectFilter(
+      'view/'.$rule->getContentType().'/'.$rule->getRuleType());
+    $nav->appendChild(
       array(
         $error_view,
         $panel,
-      ),
+      ));
+
+    return $this->buildStandardPageResponse(
+      $nav,
       array(
         'title' => 'Edit Rule',
       ));
@@ -231,9 +212,9 @@ final class HeraldRuleController extends HeraldController {
 
   private function canEditRule($rule, $user) {
     return
-      $user->getIsAdmin() ||
+      ($user->getIsAdmin()) ||
-      $rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL ||
+      ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) ||
-      $rule->getAuthorPHID() == $user->getPHID();
+      ($rule->getAuthorPHID() == $user->getPHID());
   }
 
   private function saveRule($rule, $request) {
@@ -322,11 +303,6 @@ final class HeraldRuleController extends HeraldController {
       $conditions[] = $obj;
     }
 
-    $author = $request->getStr('author');
-    if ($author) {
-      $rule->setAuthorPHID($author);
-    }
-
     $actions = array();
     foreach ($data['actions'] as $action) {
       if ($action === null) {
@@ -351,13 +327,14 @@ final class HeraldRuleController extends HeraldController {
     if (!$errors) {
       try {
 
-// TODO
+        $edit_action = $rule->getID() ? 'edit' : 'create';
-//          $rule->openTransaction();
+
+        $rule->openTransaction();
           $rule->save();
           $rule->saveConditions($conditions);
           $rule->saveActions($actions);
-          $rule->saveEdit($request->getUser()->getPHID());
+          $rule->logEdit($request->getUser()->getPHID(), $edit_action);
-//          $rule->saveTransaction();
+        $rule->saveTransaction();
 
       } catch (AphrontQueryDuplicateKeyException $ex) {
         $e_name = "Not Unique";
@@ -413,14 +390,9 @@ final class HeraldRuleController extends HeraldController {
       }
     }
 
-    $all_rules = id(new HeraldRule())->loadAllWhere(
+    $all_rules = $this->loadRulesThisRuleMayDependUpon($rule);
-      'authorPHID = %s AND contentType = %s',
-      $rule->getAuthorPHID(),
-      $rule->getContentType());
     $all_rules = mpull($all_rules, 'getName', 'getID');
     asort($all_rules);
-    unset($all_rules[$rule->getID()]);
-
 
     $config_info = array();
     $config_info['fields']
@@ -468,7 +440,6 @@ final class HeraldRuleController extends HeraldController {
 
   private function loadHandles($rule) {
     $phids = array();
-    $phids[] = $rule->getAuthorPHID();
 
     foreach ($rule->getActions() as $action) {
       if (!is_array($action->getTarget())) {
@@ -491,79 +462,60 @@ final class HeraldRuleController extends HeraldController {
       }
     }
 
-    $phids += array($rule->getAuthorPHID());
+    $phids[] = $rule->getAuthorPHID();
-    $handles = id(new PhabricatorObjectHandleData($phids))
+
-      ->loadHandles();
+    return id(new PhabricatorObjectHandleData($phids))->loadHandles();
-    return $handles;
   }
 
-  private function getMustMatchSelector($rule) {
+
-    $options = array(
+  /**
+   * Render the selector for the "When (all of | any of) these conditions are
+   * met:" element.
+   */
+  private function renderMustMatchSelector($rule) {
+    return AphrontFormSelectControl::renderSelectTag(
+      $rule->getMustMatchAll() ? 'all' : 'any',
+      array(
         'all' => 'all of',
         'any' => 'any of',
-    );
-
-    $selected = $rule->getMustMatchAll() ? 'all' : 'any';
-
-    $must_match = array();
-    foreach ($options as $key => $option) {
-      $must_match[] = phutil_render_tag(
-        'option',
-        array(
-          'selected' => ($selected == $key) ? 'selected' : null,
-          'value' => $key,
       ),
-        phutil_escape_html($option));
+      array(
-    }
+        'name' => 'must_match',
-    $must_match =
+      ));
-      '<select name="must_match">' .
-      implode("\n", $must_match) .
-      '</select>';
-    return $must_match;
   }
 
-  private function getRepetitionSelector($rule) {
+
+  /**
+   * Render the selector for "Take these actions (every time | only the first
+   * time) this rule matches..." element.
+   */
+  private function renderRepetitionSelector($rule) {
     // Make the selector for choosing how often this rule should be repeated
     $repetition_policy = HeraldRepetitionPolicyConfig::toString(
-      $rule->getRepetitionPolicy()
+      $rule->getRepetitionPolicy());
-    );
     $repetition_options = HeraldRepetitionPolicyConfig::getMapForContentType(
-      $rule->getContentType()
+      $rule->getContentType());
-    );
 
     if (empty($repetition_options)) {
       // default option is 'every time'
       $repetition_selector = idx(
         HeraldRepetitionPolicyConfig::getMap(),
-        HeraldRepetitionPolicyConfig::EVERY
+        HeraldRepetitionPolicyConfig::EVERY);
-      );
       return $repetition_selector;
     } else if (count($repetition_options) == 1) {
       // if there's only 1 option, just pick it for the user
       $repetition_selector = reset($repetition_options);
       return $repetition_selector;
     } else {
-      // give the user all the options for this rule type
+      return AphrontFormSelectControl::renderSelectTag(
-      $tags = array();
+        $repetition_policy,
-
+        $repetition_options,
-      foreach ($repetition_options as $name => $option) {
-        $tags[] = phutil_render_tag(
-          'option',
         array(
-            'selected' => ($repetition_policy == $name) ? 'selected' : null,
+          'name' => 'repetition_policy',
-            'value' => $name,
+        ));
-          ),
+    }
-          phutil_escape_html($option)
-        );
   }
 
-      $repetition_selector =
-        '<select name="repetition_policy">' .
-        implode("\n", $tags) .
-        '</select>';
-      return $repetition_selector;
-    }
-  }
 
   protected function buildTokenizerTemplates() {
     $template = new AphrontTokenizerTemplateView();
@@ -580,4 +532,33 @@ final class HeraldRuleController extends HeraldController {
       'markup' => $template,
     );
   }
+
+
+  /**
+   * Load rules for the "Another Herald rule..." condition dropdown, which
+   * allows one rule to depend upon the success or failure of another rule.
+   */
+  private function loadRulesThisRuleMayDependUpon(HeraldRule $rule) {
+    // Any rule can depend on a global rule.
+    $all_rules = id(new HeraldRuleQuery())
+      ->withRuleTypes(array(HeraldRuleTypeConfig::RULE_TYPE_GLOBAL))
+      ->withContentTypes(array($rule->getContentType()))
+      ->execute();
+
+    if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      // Personal rules may depend upon your other personal rules.
+      $all_rules += id(new HeraldRuleQuery())
+        ->withRuleTypes(array(HeraldRuleTypeConfig::RULE_TYPE_PERSONAL))
+        ->withContentTypes(array($rule->getContentType()))
+        ->withAuthorPHIDs(array($rule->getAuthorPHID()))
+        ->execute();
+    }
+
+    // A rule can not depend upon itself.
+    unset($all_rules[$rule->getID()]);
+
+    return $all_rules;
+  }
+
+
 }

src/applications/herald/controller/rule/__init__.php

@@ -16,6 +16,7 @@ phutil_require_module('phabricator', 'applications/herald/config/repetitionpolic
 phutil_require_module('phabricator', 'applications/herald/config/ruletype');
 phutil_require_module('phabricator', 'applications/herald/config/valuetype');
 phutil_require_module('phabricator', 'applications/herald/controller/base');
+phutil_require_module('phabricator', 'applications/herald/query/rule');
 phutil_require_module('phabricator', 'applications/herald/storage/condition');
 phutil_require_module('phabricator', 'applications/herald/storage/rule');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
@@ -25,13 +26,13 @@ phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 phutil_require_module('phabricator', 'view/control/tokenizer');
 phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/markup');
+phutil_require_module('phabricator', 'view/form/control/select');
 phutil_require_module('phabricator', 'view/form/control/submit');
 phutil_require_module('phabricator', 'view/form/control/text');
 phutil_require_module('phabricator', 'view/form/error');
 phutil_require_module('phabricator', 'view/form/inset');
 phutil_require_module('phabricator', 'view/layout/panel');
 
-phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/herald/controller/test/HeraldTestConsoleController.php

@@ -18,10 +18,6 @@
 
 final class HeraldTestConsoleController extends HeraldController {
 
-  public function getFilter() {
-    return 'test';
-  }
-
   public function processRequest() {
 
     $request = $this->getRequest();
@@ -136,11 +132,16 @@ final class HeraldTestConsoleController extends HeraldController {
     $panel->setWidth(AphrontPanelView::WIDTH_FULL);
     $panel->appendChild($form);
 
-    return $this->buildStandardPageResponse(
+    $nav = $this->renderNav();
+    $nav->selectFilter('test');
+    $nav->appendChild(
       array(
         $error_view,
         $panel,
-      ),
+      ));
+
+    return $this->buildStandardPageResponse(
+      $nav,
       array(
         'title' => 'Test Console',
         'tab' => 'test',

src/applications/herald/controller/transcript/HeraldTranscriptController.php

@@ -26,10 +26,6 @@ final class HeraldTranscriptController extends HeraldController {
   private $filter;
   private $handles;
 
-  public function getFilter() {
-    return 'transcript';
-  }
-
   public function willProcessRequest(array $data) {
     $this->id = $data['id'];
     $map = $this->getFilterMap();
@@ -69,6 +65,15 @@ final class HeraldTranscriptController extends HeraldController {
         ->loadHandles();
       $this->handles = $handles;
 
+      if ($xscript->getDryRun()) {
+        $notice = new AphrontErrorView();
+        $notice->setSeverity(AphrontErrorView::SEVERITY_NOTICE);
+        $notice->setTitle('Dry Run');
+        $notice->appendChild(
+          'This was a dry run to test Herald rules, no actions were executed.');
+        $nav->appendChild($notice);
+      }
+
       $apply_xscript_panel = $this->buildApplyTranscriptPanel(
         $xscript);
       $nav->appendChild($apply_xscript_panel);
@@ -82,21 +87,12 @@ final class HeraldTranscriptController extends HeraldController {
       $nav->appendChild($object_xscript_panel);
     }
 
-/*
+    $main_nav = $this->renderNav();
-
+    $main_nav->selectFilter('transcript');
-  TODO
+    $main_nav->appendChild($nav);
-
-    $notice = null;
-    if ($xscript->getDryRun()) {
-      $notice =
-        <tools:notice title="Dry Run">
-          This was a dry run to test Herald rules, no actions were executed.
-        </tools:notice>;
-    }
-*/
 
     return $this->buildStandardPageResponse(
-      $nav,
+      $main_nav,
       array(
         'title' => 'Transcript',
       ));

src/applications/herald/controller/transcriptlist/HeraldTranscriptListController.php

@@ -18,10 +18,6 @@
 
 final class HeraldTranscriptListController extends HeraldController {
 
-  public function getFilter() {
-    return 'transcript';
-  }
-
   public function processRequest() {
 
     $request = $this->getRequest();
@@ -41,12 +37,15 @@ final class HeraldTranscriptListController extends HeraldController {
         $phid);
     }
 
-    $offset = $request->getInt('offset', 0);
+    $pager = new AphrontPagerView();
-    $page_size = 100;
+    $pager->setOffset($request->getInt('offset'));
+    $pager->setURI($request->getRequestURI(), 'offset');
+
     $limit_clause = qsprintf(
       $conn_r,
       'LIMIT %d, %d',
-      $offset, $page_size + 1);
+      $pager->getOffset(),
+      $pager->getPageSize() + 1);
 
     $data = queryfx_all(
       $conn_r,
@@ -58,39 +57,7 @@ final class HeraldTranscriptListController extends HeraldController {
       $where_clause,
       $limit_clause);
 
-    $pager = new AphrontPagerView();
+    $data = $pager->sliceResults($data);
-    $pager->getPageSize($page_size);
-    $pager->setHasMorePages(count($data) == $page_size + 1);
-    $pager->setOffset($offset);
-    $pager->setURI($request->getRequestURI(), 'offset');
-
-    /*
-
-    $conn_r = smc_get_db('cdb.herald', 'r');
-
-    $page_size = 100;
-
-    $pager = new SimplePager();
-    $pager->setPageSize($page_size);
-    $pager->setOffset((((int)$request->getInt('page')) - 1) * $page_size);
-    $pager->order('id', array('id'));
-
-
-    $fbid = $request->getInt('fbid');
-    if ($fbid) {
-      $filter = qsprintf(
-        $conn_r,
-        'WHERE objectID = %d',
-        $fbid);
-    } else {
-      $filter = '';
-    }
-
-    $data = $pager->select(
-      $conn_r,
-      'id, objectID, time, duration, dryRun FROM transcript %Q',
-      $filter);
-*/
 
     // Render the table.
     $handles = array();
@@ -144,8 +111,12 @@ final class HeraldTranscriptListController extends HeraldController {
     $panel->appendChild($table);
     $panel->appendChild($pager);
 
+    $nav = $this->renderNav();
+    $nav->selectFilter('transcript');
+    $nav->appendChild($panel);
+
     return $this->buildStandardPageResponse(
-      $panel,
+      $nav,
       array(
         'title' => 'Herald Transcripts',
         'tab' => 'transcripts',

src/applications/herald/query/log/HeraldEditLogQuery.php

@@ -0,0 +1,64 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class HeraldEditLogQuery extends PhabricatorOffsetPagedQuery {
+
+  private $ruleIDs;
+
+  public function withRuleIDs(array $rule_ids) {
+    $this->ruleIDs = $rule_ids;
+    return $this;
+  }
+
+  public function execute() {
+    $table = new HeraldRuleEdit();
+    $conn_r = $table->establishConnection('r');
+
+    $where = $this->buildWhereClause($conn_r);
+    $order = $this->buildOrderClause($conn_r);
+    $limit = $this->buildLimitClause($conn_r);
+
+    $data = queryfx_all(
+      $conn_r,
+      'SELECT log.* FROM %T log %Q %Q %Q',
+      $table->getTableName(),
+      $where,
+      $order,
+      $limit);
+
+    return $table->loadAllFromArray($data);
+  }
+
+  private function buildWhereClause($conn_r) {
+    $where = array();
+
+    if ($this->ruleIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'ruleID IN (%Ld)',
+        $this->ruleIDs);
+    }
+
+    return $this->formatWhereClause($where);
+  }
+
+  private function buildOrderClause($conn_r) {
+    return 'ORDER BY id DESC';
+  }
+
+}

src/applications/herald/query/log/__init__.php

@@ -0,0 +1,15 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/herald/storage/edithistory');
+phutil_require_module('phabricator', 'infrastructure/query/offsetpaged');
+phutil_require_module('phabricator', 'storage/qsprintf');
+phutil_require_module('phabricator', 'storage/queryfx');
+
+
+phutil_require_source('HeraldEditLogQuery.php');

src/applications/herald/query/rule/HeraldRuleQuery.php

@@ -0,0 +1,90 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class HeraldRuleQuery extends PhabricatorOffsetPagedQuery {
+
+  private $authorPHIDs;
+  private $ruleTypes;
+  private $contentTypes;
+
+  public function withAuthorPHIDs(array $author_phids) {
+    $this->authorPHIDs = $author_phids;
+    return $this;
+  }
+
+  public function withRuleTypes(array $types) {
+    $this->ruleTypes = $types;
+    return $this;
+  }
+
+  public function withContentTypes(array $types) {
+    $this->contentTypes = $types;
+    return $this;
+  }
+
+  public function execute() {
+    $table = new HeraldRule();
+    $conn_r = $table->establishConnection('r');
+
+    $where = $this->buildWhereClause($conn_r);
+    $order = $this->buildOrderClause($conn_r);
+    $limit = $this->buildLimitClause($conn_r);
+
+    $data = queryfx_all(
+      $conn_r,
+      'SELECT rule.* FROM %T rule %Q %Q %Q',
+      $table->getTableName(),
+      $where,
+      $order,
+      $limit);
+
+    return $table->loadAllFromArray($data);
+  }
+
+  private function buildWhereClause($conn_r) {
+    $where = array();
+
+    if ($this->authorPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'rule.authorPHID IN (%Ls)',
+        $this->authorPHIDs);
+    }
+
+    if ($this->ruleTypes) {
+      $where[] = qsprintf(
+        $conn_r,
+        'rule.ruleType IN (%Ls)',
+        $this->ruleTypes);
+    }
+
+    if ($this->contentTypes) {
+      $where[] = qsprintf(
+        $conn_r,
+        'rule.contentType IN (%Ls)',
+        $this->contentTypes);
+    }
+
+    return $this->formatWhereClause($where);
+  }
+
+  private function buildOrderClause($conn_r) {
+    return 'ORDER BY id DESC';
+  }
+
+}

src/applications/herald/query/rule/__init__.php

@@ -0,0 +1,15 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/herald/storage/rule');
+phutil_require_module('phabricator', 'infrastructure/query/offsetpaged');
+phutil_require_module('phabricator', 'storage/qsprintf');
+phutil_require_module('phabricator', 'storage/queryfx');
+
+
+phutil_require_source('HeraldRuleQuery.php');

src/applications/herald/storage/edithistory/HeraldRuleEdit.php

@@ -20,5 +20,7 @@ final class HeraldRuleEdit extends HeraldDAO {
 
   protected $editorPHID;
   protected $ruleID;
+  protected $ruleName;
+  protected $action;
 
 }

src/applications/herald/storage/rule/HeraldRule.php

@@ -163,23 +163,13 @@ final class HeraldRule extends HeraldDAO {
     return $edits;
   }
 
-  public function attachEdits(array $edits) {
+  public function logEdit($editor_phid, $action) {
-    $this->edits = $edits;
+    id(new HeraldRuleEdit())
-    return $this;
+      ->setRuleID($this->getID())
-  }
+      ->setRuleName($this->getName())
-
+      ->setEditorPHID($editor_phid)
-  public function getEdits() {
+      ->setAction($action)
-    if ($this->edits === null) {
+      ->save();
-      throw new Exception("Attach edits before accessing them!");
-    }
-    return $this->edits;
-  }
-
-  public function saveEdit($editor_phid) {
-    $edit = new HeraldRuleEdit();
-    $edit->setRuleID($this->getID());
-    $edit->setEditorPHID($editor_phid);
-    $edit->save();
   }
 
   public function saveConditions(array $conditions) {

src/applications/herald/view/edithistory/HeraldRuleEditHistoryView.php

@@ -17,14 +17,19 @@
  */
 
 final class HeraldRuleEditHistoryView extends AphrontView {
-  private $rule;
+
+  private $edits;
   private $handles;
 
-  public function setRule($rule) {
+  public function setEdits(array $edits) {
-    $this->rule = $rule;
+    $this->edits = $edits;
     return $this;
   }
 
+  public function getEdits() {
+    return $this->edits;
+  }
+
   public function setHandles(array $handles) {
     $this->handles = $handles;
     return $this;
@@ -38,36 +43,51 @@ final class HeraldRuleEditHistoryView extends AphrontView {
   public function render() {
     $rows = array();
 
-    foreach ($this->rule->getEdits() as $edit) {
+    foreach ($this->edits as $edit) {
-      $editor = $this->handles[$edit->getEditorPHID()]->renderLink();
+      $name = nonempty($edit->getRuleName(), 'Unknown Rule');
+      $rule_name = phutil_render_tag(
+        'strong',
+        array(),
+        phutil_escape_html($name));
 
-      $edit_date = phabricator_datetime($edit->getDateCreated(), $this->user);
+      switch ($edit->getAction()) {
-
+        case 'create':
-      // Something useful could totally go here
+          $details = "Created rule '{$rule_name}'.";
-      $details = "";
+          break;
+        case 'delete':
+          $details = "Deleted rule '{$rule_name}'.";
+          break;
+        case 'edit':
+        default:
+          $details = "Edited rule '{$rule_name}'.";
+          break;
+      }
 
       $rows[] = array(
-        $editor,
+        $edit->getRuleID(),
-        $edit_date,
+        $this->handles[$edit->getEditorPHID()]->renderLink(),
         $details,
+        phabricator_datetime($edit->getDateCreated(), $this->user),
       );
     }
 
-    $rule_name = phutil_escape_html($this->rule->getName());
     $table = new AphrontTableView($rows);
-    $table->setNoDataString(
+    $table->setNoDataString("No edits for rule.");
-      "No edits for \"${rule_name}\"");
     $table->setHeaders(
       array(
+        'Rule ID',
         'Editor',
-        'Edit Date',
         'Details',
+        'Edit Date',
+      ));
+    $table->setColumnClasses(
+      array(
+        '',
+        '',
+        'wide',
+        '',
       ));
 
-    $panel = new AphrontPanelView();
+    return $table->render();
-    $panel->setHeader("Edit History for \"${rule_name}\"");
-    $panel->appendChild($table);
-
-    return $panel;
   }
 }

src/applications/herald/view/edithistory/__init__.php

@@ -8,10 +8,10 @@
 
 phutil_require_module('phabricator', 'view/base');
 phutil_require_module('phabricator', 'view/control/table');
-phutil_require_module('phabricator', 'view/layout/panel');
 phutil_require_module('phabricator', 'view/utils');
 
 phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('HeraldRuleEditHistoryView.php');

src/applications/herald/view/rulelist/HeraldRuleListView.php

@@ -17,13 +17,12 @@
  */
 
 final class HeraldRuleListView extends AphrontView {
+
   private $rules;
   private $handles;
-  private $map;
+
-  private $view;
+  private $showAuthor;
-  private $allowCreation;
+  private $showRuleType;
-  private $showOwner = true;
-  private $showType = false;
   private $user;
 
   public function setRules(array $rules) {
@@ -36,28 +35,13 @@ final class HeraldRuleListView extends AphrontView {
     return $this;
   }
 
-  public function setMap($map) {
+  public function setShowAuthor($show_author) {
-    $this->map = $map;
+    $this->showAuthor = $show_author;
     return $this;
   }
 
-  public function setView($view) {
+  public function setShowRuleType($show_rule_type) {
-    $this->view = $view;
+    $this->showRuleType = $show_rule_type;
-    return $this;
-  }
-
-  public function setAllowCreation($allow_creation) {
-    $this->allowCreation = $allow_creation;
-    return $this;
-  }
-
-  public function setShowOwner($show_owner) {
-    $this->showOwner = $show_owner;
-    return $this;
-  }
-
-  public function setShowType($show_type) {
-    $this->showType = $show_type;
     return $this;
   }
 
@@ -73,7 +57,12 @@ final class HeraldRuleListView extends AphrontView {
     $rows = array();
 
     foreach ($this->rules as $rule) {
-      $owner = $this->handles[$rule->getAuthorPHID()]->renderLink();
+
+      if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) {
+        $author = null;
+      } else {
+        $author = $this->handles[$rule->getAuthorPHID()]->renderLink();
+      }
 
       $name = phutil_render_tag(
         'a',
@@ -82,21 +71,12 @@ final class HeraldRuleListView extends AphrontView {
         ),
         phutil_escape_html($rule->getName()));
 
-      $last_edit_date = phabricator_datetime($rule->getDateModified(),
+      $edit_log = phutil_render_tag(
-                                             $this->user);
-
-      $view_edits = phutil_render_tag(
         'a',
         array(
           'href' => '/herald/history/'.$rule->getID().'/',
         ),
-        '(View Edits)');
+        'View Edit Log');
-
-      $last_edited = phutil_render_tag(
-        'span',
-        array(),
-        "Last edited on $last_edit_date ${view_edits}");
-
 
       $delete = javelin_render_tag(
         'a',
@@ -108,60 +88,42 @@ final class HeraldRuleListView extends AphrontView {
         'Delete');
 
       $rows[] = array(
-        $this->map[$rule->getContentType()],
         $type_map[$rule->getRuleType()],
-        $owner,
+        $author,
         $name,
-        $last_edited,
+        $edit_log,
         $delete,
       );
     }
 
-    $rules_for = phutil_escape_html($this->map[$this->view]);
-
     $table = new AphrontTableView($rows);
-    $table->setNoDataString(
+    $table->setNoDataString("No matching rules.");
-      "No matching subscription rules for {$rules_for}.");
 
     $table->setHeaders(
       array(
-        'Content Type',
         'Rule Type',
-        'Owner',
+        'Author',
         'Rule Name',
-        'Last Edited',
+        'Edit Log',
         '',
       ));
     $table->setColumnClasses(
       array(
         '',
         '',
-        '',
+        'wide pri',
-        'wide wrap pri',
         '',
         'action'
       ));
     $table->setColumnVisibility(
       array(
+        $this->showRuleType,
+        $this->showAuthor,
         true,
-        $this->showType,
-        $this->showOwner,
         true,
         true,
       ));
 
-    $panel = new AphrontPanelView();
+    return $table->render();
-    $panel->setHeader("Herald Rules for {$rules_for}");
-
-    if ($this->allowCreation) {
-      $panel->setCreateButton(
-        'Create New Herald Rule',
-        '/herald/new/'.$this->view.'/');
-    }
-
-    $panel->appendChild($table);
-
-    return $panel;
-
   }
 }

src/applications/herald/view/rulelist/__init__.php

@@ -10,8 +10,6 @@ phutil_require_module('phabricator', 'applications/herald/config/ruletype');
 phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 phutil_require_module('phabricator', 'view/base');
 phutil_require_module('phabricator', 'view/control/table');
-phutil_require_module('phabricator', 'view/layout/panel');
-phutil_require_module('phabricator', 'view/utils');
 
 phutil_require_module('phutil', 'markup');
 

src/applications/phid/handle/data/PhabricatorObjectHandleData.php

@@ -24,6 +24,11 @@ final class PhabricatorObjectHandleData {
     $this->phids = array_unique($phids);
   }
 
+  public static function loadOneHandle($phid) {
+    $handles = id(new PhabricatorObjectHandleData(array($phid)))->loadHandles();
+    return $handles[$phid];
+  }
+
   public function loadObjects() {
     $types = array();
     foreach ($this->phids as $phid) {

src/view/control/table/AphrontTableView.php

@@ -237,7 +237,7 @@ final class AphrontTableView extends AphrontView {
         ++$row_num;
       }
     } else {
-      $colspan = max(count($headers), 1);
+      $colspan = max(count(array_filter($visibility)), 1);
       $table[] =
         '<tr class="no-data"><td colspan="'.$colspan.'">'.
           coalesce($this->noDataString, 'No data available.').

src/view/layout/sidenavfilter/AphrontSideNavFilterView.php

@@ -78,7 +78,7 @@ final class AphrontSideNavFilterView extends AphrontView {
     return $this->baseURI;
   }
 
-  public function selectFilter($key, $default) {
+  public function selectFilter($key, $default = null) {
     $this->selectedFilter = $default;
     if ($key !== null) {
       foreach ($this->items as $item) {

webroot/rsrc/css/application/herald/herald.css

@@ -29,7 +29,3 @@
 .herald-action-table td.target {
   width: 100%;
 }
-
-#herald-rule-edit-form #author-input {
-  width: 300px;
-}

webroot/rsrc/js/application/herald/HeraldRuleEditor.js

@@ -65,7 +65,6 @@ JX.install('HeraldRuleEditor', {
 
     this._renderConditions(conditions);
     this._renderActions(actions);
-    this._renderAuthorInput();
   },
 
   members : {
@@ -120,12 +119,6 @@ JX.install('HeraldRuleEditor', {
         conditions: this._config.conditions,
         actions: this._config.actions
       });
-
-      var authorInput = JX.DOM.find(this._root, 'input', 'author');
-      var authorID = JX.keys(this._config.authorGetter())[0];
-      if (authorID) {
-        authorInput.value = authorID;
-      }
     },
 
     _getConditionValue : function(id) {
@@ -243,22 +236,6 @@ JX.install('HeraldRuleEditor', {
       return [input, get_fn, set_fn];
     },
 
-    _renderAuthorInput : function() {
-      var tokenizer = this._newTokenizer('email', 1);
-      input = tokenizer[0];
-      set_fn = tokenizer[2];
-      set_fn(this._config.author);
-      this._config.authorGetter = tokenizer[1];
-      try {
-        var author_cell = JX.$('author-input');
-        JX.DOM.setContent(author_cell, input);
-      } catch (ex) {
-        // TODO: Get rid of the ownership changing stuff or something?
-        // On global rules, there's no "author" input. Not fixing this
-        // properly because I think I'm going to nuke it soon.
-      }
-   },
-
     _renderValueInputForRow : function(row_id) {
       var cond = this._config.conditions[row_id];
       var type = this._config.info.values[cond[0]][cond[1]];