From 1e5fd3a3864f78c8ef27ed777c6fa4f5355c00dd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 9 Jun 2011 13:48:39 -0700
Subject: [PATCH] Fix Herald exception when updating a diff that has carryover
 CCs

Summary:
This is pretty subtle and tricky, but some apply transcripts don't have a rule
ID because they're purely informational. We currently get an exception, which
prevnets diff updates.

jason/tuomas: don't update phabricator.fb.com until this lands :P

Test Plan:
Applied this patch live to secure.phabricator.com and was able to update D420.

Reviewed By: gc3
Reviewers: gc3, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, gc3
Differential Revision: 421
---
 .../herald/engine/engine/HeraldEngine.php           | 13 ++++++++-----
 src/applications/herald/storage/rule/HeraldRule.php | 12 +++---------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/applications/herald/engine/engine/HeraldEngine.php b/src/applications/herald/engine/engine/HeraldEngine.php
index 14339f0e32..6baa2ca8ef 100644
--- a/src/applications/herald/engine/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/engine/HeraldEngine.php
@@ -132,11 +132,14 @@ class HeraldEngine {
     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());
+      $rule_ids = mpull($xscripts, 'getRuleID');
+      foreach ($rule_ids as $rule_id) {
+        if (!$rule_id) {
+          // Some apply transcripts are purely informational and not associated
+          // with a rule, e.g. carryover emails from earlier revisions.
+          continue;
+        }
+        HeraldRule::saveRuleApplied($rule_id, $object->getPHID());
       }
     }
   }
diff --git a/src/applications/herald/storage/rule/HeraldRule.php b/src/applications/herald/storage/rule/HeraldRule.php
index fd43ec9bbe..79acda53be 100644
--- a/src/applications/herald/storage/rule/HeraldRule.php
+++ b/src/applications/herald/storage/rule/HeraldRule.php
@@ -85,18 +85,12 @@ class HeraldRule extends HeraldDAO {
     $this->ruleApplied = array_fill_keys(ipull($applied, 'phid'), true);
  }
 
-  public function saveRuleApplied($phid) {
-    if (!$this->getID()) {
-      throw new Exception("Save rule before saving children.");
-    }
-
+  public static function saveRuleApplied($rule_id, $phid) {
     queryfx(
-      $this->establishConnection('w'),
+      id(new HeraldRule())->establishConnection('w'),
       'INSERT IGNORE INTO %T (phid, ruleID) VALUES (%s, %d)',
-      self::TABLE_RULE_APPLIED, $phid, $this->getID()
+      self::TABLE_RULE_APPLIED, $phid, $rule_id
     );
-
-    $this->setRuleApplied($phid);
   }
 
   public function loadConditions() {