From 5f8280506862f8301995b5f9d13493bc9850e8b6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 2 Oct 2014 09:49:32 -0700
Subject: [PATCH] Make execution order of Herald rules explicit

Summary: Fixes T6211. This gives Herald rules an explicit execution order, which seems generally good. See some discussion on T6211 and inline.

Test Plan:
  - Added unit test.
  - Dry ran rules and saw rules appear in the expected order in the transcript.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6211

Differential Revision: https://secure.phabricator.com/D10624
---
 src/__phutil_library_map__.php                |  2 +
 .../herald/engine/HeraldEngine.php            |  2 +
 .../herald/storage/HeraldRule.php             | 36 ++++++++++++++++
 .../storage/__tests__/HeraldRuleTestCase.php  | 41 +++++++++++++++++++
 4 files changed, 81 insertions(+)
 create mode 100644 src/applications/herald/storage/__tests__/HeraldRuleTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 0a0eb45969..f04493ab8b 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -815,6 +815,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php',
     'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php',
     'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php',
+    'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php',
     'HeraldRuleTransaction' => 'applications/herald/storage/HeraldRuleTransaction.php',
     'HeraldRuleTransactionComment' => 'applications/herald/storage/HeraldRuleTransactionComment.php',
     'HeraldRuleTranscript' => 'applications/herald/storage/transcript/HeraldRuleTranscript.php',
@@ -3712,6 +3713,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'PhabricatorPHIDType',
     'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine',
+    'HeraldRuleTestCase' => 'PhabricatorTestCase',
     'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction',
     'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'HeraldRuleViewController' => 'HeraldController',
diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php
index d4fedf1e94..e15cfc24fb 100644
--- a/src/applications/herald/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/HeraldEngine.php
@@ -49,6 +49,8 @@ final class HeraldEngine {
     assert_instances_of($rules, 'HeraldRule');
     $t_start = microtime(true);
 
+    // Rules execute in a well-defined order: sort them into execution order.
+    $rules = msort($rules, 'getRuleExecutionOrderSortKey');
     $rules = mpull($rules, null, 'getPHID');
 
     $this->transcript = new HeraldTranscript();
diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php
index 7dda0b0729..bc85bdd203 100644
--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@@ -228,6 +228,42 @@ final class HeraldRule extends HeraldDAO
     return $this->assertAttached($this->triggerObject);
   }
 
+  /**
+   * Get a sortable key for rule execution order.
+   *
+   * Rules execute in a well-defined order: personal rules first, then object
+   * rules, then global rules. Within each rule type, rules execute from lowest
+   * ID to highest ID.
+   *
+   * This ordering allows more powerful rules (like global rules) to override
+   * weaker rules (like personal rules) when multiple rules exist which try to
+   * affect the same field. Executing from low IDs to high IDs makes
+   * interactions easier to understand when adding new rules, because the newest
+   * rules always happen last.
+   *
+   * @return string A sortable key for this rule.
+   */
+  public function getRuleExecutionOrderSortKey() {
+
+    $rule_type = $this->getRuleType();
+
+    switch ($rule_type) {
+      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
+        $type_order = 1;
+        break;
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
+        $type_order = 2;
+        break;
+      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+        $type_order = 3;
+        break;
+      default:
+        throw new Exception(pht('Unknown rule type "%s"!', $rule_type));
+    }
+
+    return sprintf('~%d%010d', $type_order, $this->getID());
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
diff --git a/src/applications/herald/storage/__tests__/HeraldRuleTestCase.php b/src/applications/herald/storage/__tests__/HeraldRuleTestCase.php
new file mode 100644
index 0000000000..7b44247966
--- /dev/null
+++ b/src/applications/herald/storage/__tests__/HeraldRuleTestCase.php
@@ -0,0 +1,41 @@
+<?php
+
+final class HeraldRuleTestCase extends PhabricatorTestCase {
+
+  public function testHeraldRuleExecutionOrder() {
+    $rules = array(
+      1 => HeraldRuleTypeConfig::RULE_TYPE_GLOBAL,
+      2 => HeraldRuleTypeConfig::RULE_TYPE_GLOBAL,
+      3 => HeraldRuleTypeConfig::RULE_TYPE_OBJECT,
+      4 => HeraldRuleTypeConfig::RULE_TYPE_PERSONAL,
+      5 => HeraldRuleTypeConfig::RULE_TYPE_GLOBAL,
+      6 => HeraldRuleTypeConfig::RULE_TYPE_PERSONAL,
+    );
+
+    foreach ($rules as $id => $type) {
+      $rules[$id] = id(new HeraldRule())
+        ->setID($id)
+        ->setRuleType($type);
+    }
+
+    shuffle($rules);
+    $rules = msort($rules, 'getRuleExecutionOrderSortKey');
+    $this->assertEqual(
+      array(
+        // Personal
+        4,
+        6,
+
+        // Object
+        3,
+
+        // Global
+        1,
+        2,
+        5
+      ),
+      array_values(mpull($rules, 'getID')));
+  }
+
+
+}