herald: add the ability to execute a rule the first time only

Summary:
- added a new config class for representing the kind of repetition a rule has
(once, every time, first time only)

- added an email action to herald rules for differential to allow someone to get
an email but only the first one

- changed the herald rule ui to allow a user to pick the amount of repetition

Test Plan:
created a test rule and ran it over and over

Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: aran, epriestley, gc3
Revert Plan:
Tags:

- begin *PUBLIC* platform impact section -
Bugzilla: #
- end platform impact -

Differential Revision: 357

resources/sql/patches/041.heraldrepetition.sql

@@ -0,0 +1,7 @@
+CREATE TABLE phabricator_herald.herald_ruleapplied (
+  ruleID int unsigned not null,
+  phid varchar(64) binary not null,
+  PRIMARY KEY(ruleID, phid)
+) ENGINE=InnoDB;
+
+ALTER TABLE phabricator_herald.herald_rule add repetitionPolicy int unsigned;

src/__phutil_library_map__.php

@@ -237,6 +237,7 @@ phutil_register_library_map(array(
     'HeraldObjectAdapter' => 'applications/herald/adapter/base',
     'HeraldObjectTranscript' => 'applications/herald/storage/transcript/object',
     'HeraldRecursiveConditionsException' => 'applications/herald/engine/engine/exception',
+    'HeraldRepetitionPolicyConfig' => 'applications/herald/config/repetitionpolicy',
     'HeraldRule' => 'applications/herald/storage/rule',
     'HeraldRuleController' => 'applications/herald/controller/rule',
     'HeraldRuleTranscript' => 'applications/herald/storage/transcript/rule',

src/applications/differential/editor/revision/DifferentialRevisionEditor.php

@@ -289,13 +289,19 @@ class DifferentialRevisionEditor {
       array_keys($add['ccs']),
       $this->actorPHID);
 
-    // Add the author to the relevant set of users so they get a copy of the
-    // email.
+    // Add the author and users included from Herald rules to the relevant set
+    // of users so they get a copy of the email.
     if (!$this->silentUpdate) {
       if ($is_new) {
         $add['rev'][$this->getActorPHID()] = true;
+        if ($diff) {
+          $add['rev'] += $adapter->getEmailPHIDsAddedByHerald();
+        }
       } else {
         $stable['rev'][$this->getActorPHID()] = true;
+        if ($diff) {
+          $stable['rev'] += $adapter->getEmailPHIDsAddedByHerald();
+        }
       }
     }
 
@@ -356,13 +362,6 @@ class DifferentialRevisionEditor {
 // TODO
 //    $revision->saveTransaction();
 
-    $event = array(
-      'revision_id' => $revision->getID(),
-      'PHID'        => $revision->getPHID(),
-      'action'      => $is_new ? 'create' : 'update',
-      'actor'       => $this->getActorPHID(),
-    );
-
 //  TODO: Move this into a worker task thing.
     PhabricatorSearchDifferentialIndexer::indexRevision($revision);
 

src/applications/herald/adapter/differential/HeraldDifferentialRevisionAdapter.php

@@ -27,6 +27,7 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter {
 
   protected $newCCs = array();
   protected $remCCs = array();
+  protected $emailPHIDs = array();
 
   protected $repository;
   protected $affectedPackages;
@@ -64,6 +65,10 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter {
     return $this->remCCs;
   }
 
+  public function getEmailPHIDsAddedByHerald() {
+    return $this->emailPHIDs;
+  }
+
   public function getPHID() {
     return $this->revision->getPHID();
   }
@@ -247,7 +252,9 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter {
             true,
             'OK, did nothing.');
           break;
+        case HeraldActionConfig::ACTION_EMAIL:
         case HeraldActionConfig::ACTION_ADD_CC:
+          $op = ($action == HeraldActionConfig::ACTION_EMAIL) ? 'email' : 'CC';
           $base_target = $effect->getTarget();
           $forbidden = array();
           foreach ($base_target as $key => $fbid) {
@@ -255,7 +262,11 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter {
               $forbidden[] = $fbid;
               unset($base_target[$key]);
             } else {
-              $this->newCCs[$fbid] = true;
+              if ($action == HeraldActionConfig::ACTION_EMAIL) {
+                $this->emailPHIDs[$fbid] = true;
+              } else {
+                $this->newCCs[$fbid] = true;
+              }
             }
           }
 
@@ -267,17 +278,18 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter {
               $result[] = new HeraldApplyTranscript(
                 $effect,
                 true,
-                'Added these addresses to CC list. Others could not be added.');
+                'Added these addresses to '.$op.' list. '.
+                'Others could not be added.');
             }
             $result[] = new HeraldApplyTranscript(
               $failed,
               false,
-              'CC forbidden, these addresses have unsubscribed.');
+              $op.' forbidden, these addresses have unsubscribed.');
           } else {
             $result[] = new HeraldApplyTranscript(
               $effect,
               true,
-              'Added addresses to CC list.');
+              'Added addresses to '.$op.' list.');
           }
           break;
         case HeraldActionConfig::ACTION_REMOVE_CC:

src/applications/herald/config/action/HeraldActionConfig.php

@@ -41,6 +41,7 @@ class HeraldActionConfig {
           array(
             self::ACTION_ADD_CC,
             self::ACTION_REMOVE_CC,
+            self::ACTION_EMAIL,
             self::ACTION_NOTHING,
           ));
       case HeraldContentTypeConfig::CONTENT_TYPE_COMMIT:

src/applications/herald/config/repetitionpolicy/HeraldRepetitionPolicyConfig.php

@@ -0,0 +1,64 @@
+<?php
+
+/*
+ * Copyright 2011 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 HeraldRepetitionPolicyConfig {
+  const FIRST   = 'first';  // only execute the first time (no repeating)
+  const EVERY   = 'every';  // repeat every time
+
+  private static $policyIntMap = array(
+    self::FIRST   => 0,
+    self::EVERY   => 1,
+  );
+
+  private static $policyMap = array(
+    self::FIRST   => 'only the first time',
+    self::EVERY   => 'every time',
+  );
+
+  public static function getMap() {
+    return self::$policyMap;
+  }
+
+  public static function getMapForContentType($type) {
+    switch ($type) {
+      case HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL:
+        return array_select_keys(
+          self::$policyMap,
+          array(
+            self::EVERY,
+            self::FIRST,
+        ));
+
+      case HeraldContentTypeConfig::CONTENT_TYPE_COMMIT:
+      case HeraldContentTypeConfig::CONTENT_TYPE_MERGE:
+      case HeraldContentTypeConfig::CONTENT_TYPE_OWNERS:
+        return array();
+
+      default:
+        throw new Exception("Unknown content type '{$type}'.");
+    }
+  }
+
+  public static function toInt($str) {
+    return idx(self::$policyIntMap, $str, self::$policyIntMap[self::EVERY]);
+  }
+
+  public static function toString($int) {
+    return idx(array_flip(self::$policyIntMap), $int, self::EVERY);
+  }
+}

src/applications/herald/config/repetitionpolicy/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/herald/config/contenttype');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('HeraldRepetitionPolicyConfig.php');

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

@@ -74,6 +74,11 @@ class HeraldRuleController extends HeraldController {
       $rule->setName($request->getStr('name'));
       $rule->setMustMatchAll(($request->getStr('must_match') == 'all'));
 
+      $repetition_policy_param = $request->getStr('repetition_policy');
+      $rule->setRepetitionPolicy(
+        HeraldRepetitionPolicyConfig::toInt($repetition_policy_param)
+      );
+
       if (!strlen($rule->getName())) {
         $e_name = "Required";
         $errors[] = "Rule must have a name.";
@@ -250,6 +255,45 @@ class HeraldRuleController extends HeraldController {
       $action = '/herald/rule/'.$rule->getID().'/';
     }
 
+    // Make the selector for choosing how often this rule should be repeated
+    $repetition_selector = "";
+    $repetition_policy = HeraldRepetitionPolicyConfig::toString(
+      $rule->getRepetitionPolicy()
+    );
+    $repetition_options = HeraldRepetitionPolicyConfig::getMapForContentType(
+      $rule->getContentType()
+    );
+
+    if (empty($repetition_options)) {
+      // default option is 'every time'
+      $repetition_selector = idx(
+        HeraldRepetitionPolicyConfig::getMap(),
+        HeraldRepetitionPolicyConfig::EVERY
+      );
+    } else if (count($repetition_options) == 1) {
+      // if there's only 1 option, just pick it for the user
+      $repetition_selector = reset($repetition_options);
+    } else {
+      // give the user all the options for this rule type
+      $tags = array();
+
+      foreach ($repetition_options as $name => $option) {
+        $tags[] = phutil_render_tag(
+          'option',
+          array (
+            'selected'  => ($repetition_policy == $name) ? 'selected' : null,
+            'value'     => $name,
+          ),
+          phutil_escape_html($option)
+        );
+      }
+
+      $repetition_selector =
+        '<select name="repetition_policy">'.
+          implode("\n", $tags).
+        '</select>';
+    }
+
     require_celerity_resource('herald-css');
 
     $type_name = $content_type_map[$rule->getContentType()];
@@ -320,7 +364,9 @@ class HeraldRuleController extends HeraldController {
             ),
             'Create New Action').
           '</div>'.
-          '<p>Take these actions:</p>'.
+          '<p>'.
+            'Take these actions '.$repetition_selector.' this rule matches:'.
+          '</p>'.
           '<div style="clear: both;"></div>'.
           javelin_render_tag(
             'table',

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

@@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'applications/herald/config/action');
 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/valuetype');
 phutil_require_module('phabricator', 'applications/herald/controller/base');
 phutil_require_module('phabricator', 'applications/herald/storage/action');

src/applications/herald/engine/engine/HeraldEngine.php

@@ -51,11 +51,27 @@ class HeraldEngine {
 
     $effects = array();
     foreach ($rules as $id => $rule) {
-
-
       $this->stack = array();
       try {
-        $rule_matches = $this->doesRuleMatch($rule, $object);
+        if (($rule->getRepetitionPolicy() ==
+             HeraldRepetitionPolicyConfig::FIRST) &&
+            $rule->getRuleApplied($object->getPHID())) {
+          // This rule is only supposed to be applied a single time, and it's
+          // aleady been applied, so this is an automatic failure.
+          $xscript = id(new HeraldRuleTranscript())
+            ->setRuleID($id)
+            ->setResult(false)
+            ->setRuleName($rule->getName())
+            ->setRuleOwner($rule->getAuthorPHID())
+            ->setReason(
+              "This rule is only supposed to be repeated a single time, ".
+              "and it has already been applied."
+            );
+          $this->transcript->addRuleTranscript($xscript);
+          $rule_matches = false;
+        } else {
+          $rule_matches = $this->doesRuleMatch($rule, $object);
+        }
       } catch (HeraldRecursiveConditionsException $ex) {
         $names = array();
         foreach ($this->stack as $rule_id => $ignored) {
@@ -101,12 +117,10 @@ class HeraldEngine {
   }
 
   public function applyEffects(array $effects, HeraldObjectAdapter $object) {
-    if ($object instanceof DryRunHeraldable) {
-      $this->transcript->setDryRun(true);
-    } else {
-      $this->transcript->setDryRun(false);
-    }
-    foreach ($object->applyHeraldEffects($effects) as $apply_xscript) {
+    $this->transcript->setDryRun($object instanceof HeraldDryRunAdapter);
+
+    $xscripts = $object->applyHeraldEffects($effects);
+    foreach ($xscripts as $apply_xscript) {
       if (!($apply_xscript instanceof HeraldApplyTranscript)) {
         throw new Exception(
           "Heraldable must return HeraldApplyTranscripts from ".
@@ -114,6 +128,17 @@ class HeraldEngine {
       }
       $this->transcript->addApplyTranscript($apply_xscript);
     }
+
+    if (!$this->transcript->getDryRun()) {
+      // Mark all the rules that have had their effects applied as having been
+      // executed for the current object.
+      $ruleIDs = mpull($xscripts, 'getRuleID');
+      foreach ($ruleIDs as $ruleID) {
+        id(new HeraldRule())
+          ->setID($ruleID)
+          ->saveRuleApplied($object->getPHID());
+      }
+    }
   }
 
   public function getTranscript() {

src/applications/herald/engine/engine/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/herald/config/condition');
 phutil_require_module('phabricator', 'applications/herald/config/field');
+phutil_require_module('phabricator', 'applications/herald/config/repetitionpolicy');
 phutil_require_module('phabricator', 'applications/herald/engine/effect');
 phutil_require_module('phabricator', 'applications/herald/engine/engine/exception');
 phutil_require_module('phabricator', 'applications/herald/storage/rule');

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

@@ -18,13 +18,18 @@
 
 class HeraldRule extends HeraldDAO {
 
+  const TABLE_RULE_APPLIED = 'herald_ruleapplied';
+
   protected $name;
   protected $authorPHID;
 
   protected $contentType;
   protected $mustMatchAll;
+  protected $repetitionPolicy;
 
-  protected $configVersion = 7;
+  protected $configVersion = 8;
+
+  private $ruleApplied = array(); // phids for which this rule has been applied
 
   public static function loadAllByContentTypeWithFullData($content_type) {
     $rules = id(new HeraldRule())->loadAllWhere(
@@ -45,10 +50,19 @@ class HeraldRule extends HeraldDAO {
       'ruleID in (%Ld)',
       $rule_ids);
 
+    $applied = queryfx_all(
+      id(new HeraldRule())->establishConnection('r'),
+      'SELECT * FROM %T WHERE ruleID in (%Ld)',
+      self::TABLE_RULE_APPLIED, $rule_ids
+    );
+
     $conditions = mgroup($conditions, 'getRuleID');
     $actions = mgroup($actions, 'getRuleID');
+    $applied = igroup($applied, 'ruleID');
+
 
     foreach ($rules as $rule) {
+      $rule->attachAllRuleApplied(idx($applied, $rule->getID(), array()));
       $rule->attachConditions(idx($conditions, $rule->getID(), array()));
       $rule->attachActions(idx($actions, $rule->getID(), array()));
     }
@@ -56,6 +70,35 @@ class HeraldRule extends HeraldDAO {
     return $rules;
   }
 
+  public function getRuleApplied($phid) {
+    // defaults to false because (ruleID, phid) pairs not in the db imply
+    // a rule that's not been applied before
+    return idx($this->ruleApplied, $phid, false);
+  }
+
+  public function setRuleApplied($phid) {
+    $this->ruleApplied[$phid] = true;
+  }
+
+  public function attachAllRuleApplied(array $applied) {
+    // turn array of array(ruleID, phid) into array of ruleID => true
+    $this->ruleApplied = array_fill_keys(ipull($applied, 'phid'), true);
+ }
+
+  public function saveRuleApplied($phid) {
+    if (!$this->getID()) {
+      throw new Exception("Save rule before saving children.");
+    }
+
+    queryfx(
+      $this->establishConnection('w'),
+      'INSERT IGNORE INTO %T (phid, ruleID) VALUES (%s, %d)',
+      self::TABLE_RULE_APPLIED, $phid, $this->getID()
+    );
+
+    $this->setRuleApplied($phid);
+  }
+
   public function loadConditions() {
     if (!$this->getID()) {
       return array();