From 6080d7411294e558e20ecc77992e2083045d9268 Mon Sep 17 00:00:00 2001
From: awyler <awyler@fb.com>
Date: Fri, 13 Jan 2012 15:24:56 -0800
Subject: [PATCH] Created personal vs. global herald rule distingtion

Summary:
A personal rule only has actions targeting the owner.  Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.

There are no new action types.  Instead, type of the rule modifies the available
targets and the messaging in the ui.  This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.

This diff sets up the logic and ui for creating personal/global rules.  All
existing rules have been defaulted to global.

TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.

Test Plan:
1. Created a personal rule to email myself.  Created a dumby revision satisfying
the conditions of that rule.  Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, zizzy

Differential Revision: https://secure.phabricator.com/D1449
---
 resources/sql/patches/097.heraldruletypes.sql |  2 +
 .../patches/098.heraldruletypemigration.php   | 62 ++++++++++++++
 src/__phutil_library_map__.php                |  1 +
 ...AphrontDefaultApplicationConfiguration.php |  5 +-
 .../config/action/HeraldActionConfig.php      | 83 ++++++++++++++++---
 .../herald/config/action/__init__.php         |  2 +
 .../config/ruletype/HeraldRuleTypeConfig.php  | 31 +++++++
 .../herald/config/ruletype/__init__.php       | 10 +++
 .../valuetype/HeraldValueTypeConfig.php       |  7 +-
 .../herald/config/valuetype/__init__.php      |  1 +
 .../controller/base/HeraldController.php      | 10 ++-
 .../controller/home/HeraldHomeController.php  | 25 ++++--
 .../controller/new/HeraldNewController.php    | 29 ++++---
 .../herald/controller/new/__init__.php        |  1 +
 .../controller/rule/HeraldRuleController.php  | 64 ++++++++------
 .../herald/controller/rule/__init__.php       |  2 +-
 .../herald/storage/rule/HeraldRule.php        |  3 +-
 17 files changed, 279 insertions(+), 59 deletions(-)
 create mode 100644 resources/sql/patches/097.heraldruletypes.sql
 create mode 100644 resources/sql/patches/098.heraldruletypemigration.php
 create mode 100644 src/applications/herald/config/ruletype/HeraldRuleTypeConfig.php
 create mode 100644 src/applications/herald/config/ruletype/__init__.php

diff --git a/resources/sql/patches/097.heraldruletypes.sql b/resources/sql/patches/097.heraldruletypes.sql
new file mode 100644
index 0000000000..cfc63e9d4a
--- /dev/null
+++ b/resources/sql/patches/097.heraldruletypes.sql
@@ -0,0 +1,2 @@
+ALTER TABLE phabricator_herald.herald_rule ADD ruleType varchar(255) not null DEFAULT 'global';
+CREATE INDEX IDX_RULE_TYPE on phabricator_herald.herald_rule (ruleType);
\ No newline at end of file
diff --git a/resources/sql/patches/098.heraldruletypemigration.php b/resources/sql/patches/098.heraldruletypemigration.php
new file mode 100644
index 0000000000..1ce2bc1cb4
--- /dev/null
+++ b/resources/sql/patches/098.heraldruletypemigration.php
@@ -0,0 +1,62 @@
+<?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.
+ */
+
+echo "Checking for rules that can be converted to 'personal'. ";
+
+$rules = id(new HeraldRule())->loadAll();
+
+foreach ($rules as $rule) {
+  if ($rule->getRuleType() !== HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+    $actions = $rule->loadActions();
+    $can_be_personal = true;
+    foreach ($actions as $action) {
+      $target = $action->getTarget();
+      if (is_array($target)) {
+        if (count($target) > 1) {
+          $can_be_personal = false;
+          break;
+        } else {
+          $targetPHID = head($target);
+          if ($targetPHID !== $rule->getAuthorPHID()) {
+            $can_be_personal = false;
+            break;
+          }
+        }
+      } else if ($target) {
+        if ($target !== $rule->getAuthorPHID()) {
+          $can_be_personal = false;
+          break;
+        }
+      }
+    }
+
+    if ($can_be_personal) {
+      $rule->setRuleType(HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+      queryfx(
+        $rule->establishConnection('w'),
+        'UPDATE %T SET ruleType = %s WHERE id = %d',
+        $rule->getTableName(),
+        $rule->getRuleType(),
+        $rule->getID());
+
+      echo "Setting rule '" . $rule->getName() . "' to personal. ";
+    }
+  }
+}
+
+echo "Done. ";
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 65cfbd8ae0..fbbd7e1a01 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -335,6 +335,7 @@ phutil_register_library_map(array(
     'HeraldRuleController' => 'applications/herald/controller/rule',
     'HeraldRuleListView' => 'applications/herald/view/rulelist',
     'HeraldRuleTranscript' => 'applications/herald/storage/transcript/rule',
+    'HeraldRuleTypeConfig' => 'applications/herald/config/ruletype',
     'HeraldTestConsoleController' => 'applications/herald/controller/test',
     'HeraldTranscript' => 'applications/herald/storage/transcript/base',
     'HeraldTranscriptController' => 'applications/herald/controller/transcript',
diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php
index 89c648ddd3..f420b3bfa7 100644
--- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php
+++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php
@@ -270,7 +270,10 @@ class AphrontDefaultApplicationConfiguration
 
       '/herald/' => array(
         '$' => 'HeraldHomeController',
-        'view/(?P<view>[^/]+)/$' => 'HeraldHomeController',
+        'view/(?P<view>[^/]+)/' => array(
+          '$' => 'HeraldHomeController',
+          '(?P<global>global)/$' => 'HeraldHomeController'
+        ),
         'new/(?:(?P<type>[^/]+)/)?$' => 'HeraldNewController',
         'rule/(?:(?P<id>\d+)/)?$' => 'HeraldRuleController',
         'delete/(?P<id>\d+)/$' => 'HeraldDeleteController',
diff --git a/src/applications/herald/config/action/HeraldActionConfig.php b/src/applications/herald/config/action/HeraldActionConfig.php
index 3974c6f59c..a1ceb6c840 100644
--- a/src/applications/herald/config/action/HeraldActionConfig.php
+++ b/src/applications/herald/config/action/HeraldActionConfig.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -23,18 +23,39 @@ class HeraldActionConfig {
   const ACTION_EMAIL        = 'email';
   const ACTION_NOTHING      = 'nothing';
 
-  public static function getActionMap() {
-    return array(
-      self::ACTION_ADD_CC       => 'Add emails to CC',
-      self::ACTION_REMOVE_CC    => 'Remove emails from CC',
-      self::ACTION_EMAIL        => 'Send an email to',
-      self::ACTION_NOTHING      => 'Do nothing',
-    );
+  public static function getActionMessageMapForRuleType($rule_type) {
+    $generic_mappings =
+      array(
+        self::ACTION_NOTHING      => 'Do nothing',
+      );
+
+    switch ($rule_type) {
+      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+        $specific_mappings =
+          array(
+            self::ACTION_ADD_CC       => 'Add emails to CC',
+            self::ACTION_REMOVE_CC    => 'Remove emails from CC',
+            self::ACTION_EMAIL        => 'Send an email to',
+          );
+        break;
+      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
+        $specific_mappings =
+          array(
+            self::ACTION_ADD_CC       => 'CC me',
+            self::ACTION_REMOVE_CC    => 'Remove me from CC',
+            self::ACTION_EMAIL        => 'Email me',
+          );
+        break;
+      default:
+        throw new Exception("Unknown rule type '${rule_type}'");
+    }
+    return $generic_mappings + $specific_mappings;
   }
 
-  public static function getActionMapForContentType($type) {
-    $map = self::getActionMap();
-    switch ($type) {
+  public static function getActionMessageMap($content_type,
+                                             $rule_type) {
+    $map = self::getActionMessageMapForRuleType($rule_type);
+    switch ($content_type) {
       case HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL:
         return array_select_keys(
           $map,
@@ -70,4 +91,44 @@ class HeraldActionConfig {
     }
   }
 
+  /**
+   * Create a HeraldAction to save from data.
+   *
+   * $data is of the form:
+   *   array(
+   *     0 => <action type>
+   *     1 => array(<targets>)
+   *   )
+   */
+  public static function willSaveAction($rule_type,
+                                        $author_phid,
+                                        $data) {
+    $obj = new HeraldAction();
+    $obj->setAction($data[0]);
+
+    // for personal rule types, set the target to be the owner of the rule
+    if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      switch ($obj->getAction()) {
+        case HeraldActionConfig::ACTION_EMAIL:
+        case HeraldActionConfig::ACTION_ADD_CC:
+        case HeraldActionConfig::ACTION_REMOVE_CC:
+          $data[1] = array($author_phid => $author_phid);
+          break;
+        case HeraldActionConfig::ACTION_NOTHING:
+          break;
+        default:
+          throw new Exception('Unrecognized action type: ' .
+                              $obj->getAction());
+      }
+    }
+
+    if (is_array($data[1])) {
+      $obj->setTarget(array_keys($data[1]));
+    } else {
+      $obj->setTarget($data[1]);
+    }
+
+    return $obj;
+  }
+
 }
diff --git a/src/applications/herald/config/action/__init__.php b/src/applications/herald/config/action/__init__.php
index 67737f7071..09df5cee40 100644
--- a/src/applications/herald/config/action/__init__.php
+++ b/src/applications/herald/config/action/__init__.php
@@ -7,6 +7,8 @@
 
 
 phutil_require_module('phabricator', 'applications/herald/config/contenttype');
+phutil_require_module('phabricator', 'applications/herald/config/ruletype');
+phutil_require_module('phabricator', 'applications/herald/storage/action');
 
 phutil_require_module('phutil', 'utils');
 
diff --git a/src/applications/herald/config/ruletype/HeraldRuleTypeConfig.php b/src/applications/herald/config/ruletype/HeraldRuleTypeConfig.php
new file mode 100644
index 0000000000..143f29c031
--- /dev/null
+++ b/src/applications/herald/config/ruletype/HeraldRuleTypeConfig.php
@@ -0,0 +1,31 @@
+<?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.
+ */
+
+class HeraldRuleTypeConfig {
+
+  const RULE_TYPE_GLOBAL = 'global';
+  const RULE_TYPE_PERSONAL = 'personal';
+
+  public static function getRuleTypeMap() {
+    static $map = array(
+      self::RULE_TYPE_GLOBAL     => 'Global',
+      self::RULE_TYPE_PERSONAL   => 'Personal',
+    );
+    return $map;
+  }
+}
diff --git a/src/applications/herald/config/ruletype/__init__.php b/src/applications/herald/config/ruletype/__init__.php
new file mode 100644
index 0000000000..5c1e01b5b5
--- /dev/null
+++ b/src/applications/herald/config/ruletype/__init__.php
@@ -0,0 +1,10 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+
+phutil_require_source('HeraldRuleTypeConfig.php');
diff --git a/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php b/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
index cef37858e8..8846c2566d 100644
--- a/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
+++ b/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php
@@ -76,7 +76,12 @@ class HeraldValueTypeConfig {
     }
   }
 
-  public static function getValueTypeForAction($action) {
+  public static function getValueTypeForAction($action, $rule_type) {
+    // users can't change targets for personal rule actions
+    if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
+      return self::VALUE_NONE;
+    }
+
     switch ($action) {
       case HeraldActionConfig::ACTION_ADD_CC:
       case HeraldActionConfig::ACTION_REMOVE_CC:
diff --git a/src/applications/herald/config/valuetype/__init__.php b/src/applications/herald/config/valuetype/__init__.php
index fa2853276e..83396f36ad 100644
--- a/src/applications/herald/config/valuetype/__init__.php
+++ b/src/applications/herald/config/valuetype/__init__.php
@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'applications/herald/config/action');
 phutil_require_module('phabricator', 'applications/herald/config/condition');
 phutil_require_module('phabricator', 'applications/herald/config/field');
+phutil_require_module('phabricator', 'applications/herald/config/ruletype');
 
 
 phutil_require_source('HeraldValueTypeConfig.php');
diff --git a/src/applications/herald/controller/base/HeraldController.php b/src/applications/herald/controller/base/HeraldController.php
index d6be181000..06e2927181 100644
--- a/src/applications/herald/controller/base/HeraldController.php
+++ b/src/applications/herald/controller/base/HeraldController.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -31,7 +31,7 @@ abstract class HeraldController extends PhabricatorController {
     $nav = new AphrontSideNavFilterView();
     $nav
       ->setBaseURI(new PhutilURI('/herald/'))
-      ->addLabel('Rules')
+      ->addLabel('My Rules')
       ->addFilter('new', 'Create Rule');
     $rules_map = HeraldContentTypeConfig::getContentTypeMap();
     $first_filter = null;
@@ -41,6 +41,12 @@ abstract class HeraldController extends PhabricatorController {
         $first_filter = 'view/'.$key;
       }
     }
+    $nav
+      ->addSpacer()
+      ->addLabel('Global Rules');
+    foreach ($rules_map as $key => $value) {
+      $nav->addFilter("view/${key}/global", $value);
+    }
 
     $nav
       ->addSpacer()
diff --git a/src/applications/herald/controller/home/HeraldHomeController.php b/src/applications/herald/controller/home/HeraldHomeController.php
index 9771c632a9..cb258b7ca8 100644
--- a/src/applications/herald/controller/home/HeraldHomeController.php
+++ b/src/applications/herald/controller/home/HeraldHomeController.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -20,10 +20,16 @@ class HeraldHomeController extends HeraldController {
 
   private $view;
   private $filter;
+  private $global;
 
   public function willProcessRequest(array $data) {
     $this->view = idx($data, 'view');
-    $this->setFilter($this->view);
+    $this->global = idx($data, 'global');
+    if ($this->global) {
+      $this->setFilter($this->view . '/global');
+    } else {
+      $this->setFilter($this->view);
+    }
   }
 
   public function getFilter() {
@@ -45,10 +51,17 @@ class HeraldHomeController extends HeraldController {
       $this->view = key($map);
     }
 
-    $rules = id(new HeraldRule())->loadAllWhere(
-      'contentType = %s AND authorPHID = %s',
-      $this->view,
-      $user->getPHID());
+    if ($this->global) {
+      $rules = id(new HeraldRule())->loadAllWhere(
+        'contentType = %s AND ruleType = %s',
+        $this->view,
+        'global');
+    } else {
+      $rules = id(new HeraldRule())->loadAllWhere(
+        'contentType = %s AND authorPHID = %s',
+        $this->view,
+        $user->getPHID());
+    }
 
     $need_phids = mpull($rules, 'getAuthorPHID');
     $handles = id(new PhabricatorObjectHandleData($need_phids))
diff --git a/src/applications/herald/controller/new/HeraldNewController.php b/src/applications/herald/controller/new/HeraldNewController.php
index 71e34da1cc..b8df026849 100644
--- a/src/applications/herald/controller/new/HeraldNewController.php
+++ b/src/applications/herald/controller/new/HeraldNewController.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -18,14 +18,14 @@
 
 class HeraldNewController extends HeraldController {
 
-  private $type;
+  private $contentType;
 
   public function getFilter() {
     return 'new';
   }
 
   public function willProcessRequest(array $data) {
-    $this->type = idx($data, 'type');
+    $this->contentType = idx($data, 'type');
   }
 
   public function processRequest() {
@@ -33,25 +33,32 @@ class HeraldNewController extends HeraldController {
     $request = $this->getRequest();
     $user = $request->getUser();
 
-    $map = HeraldContentTypeConfig::getContentTypeMap();
-    if (empty($map[$this->type])) {
-      reset($map);
-      $this->type = key($map);
+    $content_type_map = HeraldContentTypeConfig::getContentTypeMap();
+    if (empty($content_type_map[$this->contentType])) {
+      reset($content_type_map);
+      $this->contentType = key($content_type_map);
     }
 
+    $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();
+
     $form = id(new AphrontFormView())
       ->setUser($user)
       ->setAction('/herald/rule/')
       ->appendChild(
         id(new AphrontFormSelectControl())
           ->setLabel('New rule for')
-          ->setName('type')
-          ->setValue($this->type)
-          ->setOptions($map))
+          ->setName('content_type')
+          ->setValue($this->contentType)
+          ->setOptions($content_type_map))
+      ->appendChild(
+        id(new AphrontFormSelectControl())
+          ->setLabel('Type')
+          ->setName('rule_type')
+          ->setOptions($rule_type_map))
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->setValue('Create Rule')
-          ->addCancelButton('/herald/view/'.$this->type.'/'));
+          ->addCancelButton('/herald/view/'.$this->contentType.'/'));
 
     $panel = new AphrontPanelView();
     $panel->setHeader('Create New Herald Rule');
diff --git a/src/applications/herald/controller/new/__init__.php b/src/applications/herald/controller/new/__init__.php
index a90d20cf3e..8724556b3d 100644
--- a/src/applications/herald/controller/new/__init__.php
+++ b/src/applications/herald/controller/new/__init__.php
@@ -7,6 +7,7 @@
 
 
 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', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/select');
diff --git a/src/applications/herald/controller/rule/HeraldRuleController.php b/src/applications/herald/controller/rule/HeraldRuleController.php
index 63cd1fed42..2368c369c9 100644
--- a/src/applications/herald/controller/rule/HeraldRuleController.php
+++ b/src/applications/herald/controller/rule/HeraldRuleController.php
@@ -38,13 +38,14 @@ class HeraldRuleController extends HeraldController {
     $user = $request->getUser();
 
     $content_type_map = HeraldContentTypeConfig::getContentTypeMap();
+    $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();
 
     if ($this->id) {
       $rule = id(new HeraldRule())->load($this->id);
       if (!$rule) {
         return new Aphront404Response();
       }
-      if ($rule->getAuthorPHID() != $user->getPHID() && !$user->getIsAdmin()) {
+      if (!$this->canEditRule($rule, $user)) {
         throw new Exception("You don't own this rule and can't edit it.");
       }
     } else {
@@ -52,11 +53,17 @@ class HeraldRuleController extends HeraldController {
       $rule->setAuthorPHID($user->getPHID());
       $rule->setMustMatchAll(true);
 
-      $type = $request->getStr('type');
-      if (!isset($content_type_map[$type])) {
-        $type = HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL;
+      $content_type = $request->getStr('content_type');
+      if (!isset($content_type_map[$content_type])) {
+        $content_type = HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL;
       }
-      $rule->setContentType($type);
+      $rule->setContentType($content_type);
+
+      $rule_type = $request->getStr('rule_type');
+      if (!isset($rule_type_map[$rule_type])) {
+        $rule_type = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL;
+      }
+      $rule->setRuleType($rule_type);
     }
     $this->setFilter($rule->getContentType());
 
@@ -104,12 +111,14 @@ class HeraldRuleController extends HeraldController {
 
     require_celerity_resource('herald-css');
 
-    $type_name = $content_type_map[$rule->getContentType()];
+    $content_type_name = $content_type_map[$rule->getContentType()];
+    $rule_type_name = $rule_type_map[$rule->getRuleType()];
 
     $form = id(new AphrontFormView())
       ->setUser($user)
       ->setID('herald-rule-edit-form')
-      ->addHiddenInput('type', $rule->getContentType())
+      ->addHiddenInput('content_type', $rule->getContentType())
+      ->addHiddenInput('rule_type', $rule->getRuleType())
       ->addHiddenInput('save', 1)
       ->appendChild(
         // Build this explicitly so we can add a sigil to it.
@@ -142,7 +151,8 @@ class HeraldRuleController extends HeraldController {
       ->appendChild(
         id(new AphrontFormMarkupControl())
           ->setValue(
-            "This rule triggers for <strong>{$type_name}</strong>."))
+            "This <strong>${rule_type_name}</strong> rule triggers for " .
+            "<strong>${content_type_name}</strong>."))
       ->appendChild(
         '<h1>Conditions</h1>'.
         '<div class="aphront-form-inset">'.
@@ -215,6 +225,13 @@ class HeraldRuleController extends HeraldController {
       ));
   }
 
+  private function canEditRule($rule, $user) {
+    return
+      $user->getIsAdmin() ||
+      $rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL ||
+      $rule->getAuthorPHID() == $user->getPHID();
+  }
+
   private function saveRule($rule, $request) {
     $rule->setName($request->getStr('name'));
     $rule->setMustMatchAll(($request->getStr('must_match') == 'all'));
@@ -301,6 +318,11 @@ 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) {
@@ -308,27 +330,15 @@ class HeraldRuleController extends HeraldController {
         continue;
       }
 
-      $obj = new HeraldAction();
-      $obj->setAction($action[0]);
-
       if (!isset($action[1])) {
         // Legitimate for any action which doesn't need a target, like
         // "Do nothing".
         $action[1] = null;
       }
 
-      if (is_array($action[1])) {
-        $obj->setTarget(array_keys($action[1]));
-      } else {
-        $obj->setTarget($action[1]);
-      }
-
-      $actions[] = $obj;
-    }
-
-    $author = $request->getStr('author');
-    if ($author) {
-      $rule->setAuthorPHID($author);
+      $actions[] = HeraldActionConfig::willSaveAction($rule->getRuleType(),
+                                                      $rule->getAuthorPHID(),
+                                                      $action);
     }
 
     $rule->attachConditions($conditions);
@@ -425,11 +435,15 @@ class HeraldRuleController extends HeraldController {
     }
 
     $config_info['actions'] =
-      HeraldActionConfig::getActionMapForContentType($rule->getContentType());
+      HeraldActionConfig::getActionMessageMap($rule->getContentType(),
+                                              $rule->getRuleType());
+
+    $config_info['rule_type'] = $rule->getRuleType();
 
     foreach ($config_info['actions'] as $action => $name) {
       $config_info['targets'][$action] =
-        HeraldValueTypeConfig::getValueTypeForAction($action);
+        HeraldValueTypeConfig::getValueTypeForAction($action,
+                                                     $rule->getRuleType());
     }
 
     Javelin::initBehavior(
diff --git a/src/applications/herald/controller/rule/__init__.php b/src/applications/herald/controller/rule/__init__.php
index 9202bab04c..cc92ef1ec7 100644
--- a/src/applications/herald/controller/rule/__init__.php
+++ b/src/applications/herald/controller/rule/__init__.php
@@ -13,9 +13,9 @@ phutil_require_module('phabricator', 'applications/herald/config/condition');
 phutil_require_module('phabricator', 'applications/herald/config/contenttype');
 phutil_require_module('phabricator', 'applications/herald/config/field');
 phutil_require_module('phabricator', 'applications/herald/config/repetitionpolicy');
+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/storage/action');
 phutil_require_module('phabricator', 'applications/herald/storage/condition');
 phutil_require_module('phabricator', 'applications/herald/storage/rule');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
diff --git a/src/applications/herald/storage/rule/HeraldRule.php b/src/applications/herald/storage/rule/HeraldRule.php
index 79acda53be..bc1702a15f 100644
--- a/src/applications/herald/storage/rule/HeraldRule.php
+++ b/src/applications/herald/storage/rule/HeraldRule.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * 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.
@@ -26,6 +26,7 @@ class HeraldRule extends HeraldDAO {
   protected $contentType;
   protected $mustMatchAll;
   protected $repetitionPolicy;
+  protected $ruleType;
 
   protected $configVersion = 8;