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

@@ -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);

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. ";

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',

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',

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() {
+  public static function getActionMessageMapForRuleType($rule_type) {
-    return array(
+    $generic_mappings =
-      self::ACTION_ADD_CC       => 'Add emails to CC',
+      array(
-      self::ACTION_REMOVE_CC    => 'Remove emails from CC',
+        self::ACTION_NOTHING      => 'Do nothing',
-      self::ACTION_EMAIL        => 'Send an email to',
+      );
-      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) {
+  public static function getActionMessageMap($content_type,
-    $map = self::getActionMap();
+                                             $rule_type) {
-    switch ($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;
+  }
+
 }

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');
 

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;
+  }
+}

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');

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:

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');

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()

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(
+    if ($this->global) {
-      'contentType = %s AND authorPHID = %s',
+      $rules = id(new HeraldRule())->loadAllWhere(
-      $this->view,
+        'contentType = %s AND ruleType = %s',
-      $user->getPHID());
+        $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))

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();
+    $content_type_map = HeraldContentTypeConfig::getContentTypeMap();
-    if (empty($map[$this->type])) {
+    if (empty($content_type_map[$this->contentType])) {
-      reset($map);
+      reset($content_type_map);
-      $this->type = key($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')
+          ->setName('content_type')
-          ->setValue($this->type)
+          ->setValue($this->contentType)
-          ->setOptions($map))
+          ->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');

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');

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');
+      $content_type = $request->getStr('content_type');
-      if (!isset($content_type_map[$type])) {
+      if (!isset($content_type_map[$content_type])) {
-        $type = HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL;
+        $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])) {
+      $actions[] = HeraldActionConfig::willSaveAction($rule->getRuleType(),
-        $obj->setTarget(array_keys($action[1]));
+                                                      $rule->getAuthorPHID(),
-      } else {
+                                                      $action);
-        $obj->setTarget($action[1]);
-      }
-
-      $actions[] = $obj;
-    }
-
-    $author = $request->getStr('author');
-    if ($author) {
-      $rule->setAuthorPHID($author);
     }
 
     $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(

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');

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;