Implement basic object rules for Herald

Summary: Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it. Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs. Test Plan: - Created non-object rules. - Created object rules. - Triggered object rules against matching and unmatching objects. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3506, T4264 Differential Revision: https://secure.phabricator.com/D7853
2024-11-22 14:52:41 +01:00 · 2013-12-30 16:48:14 -08:00 · 2013-12-30 16:48:14 -08:00 · 140c88e971
commit 140c88e971
parent 472b0f983e
11 changed files with 269 additions and 40 deletions
--- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php
+++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php
@ -44,14 +44,33 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter {
  public function supportsRuleType($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return true;
      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
      default:
        return false;
    }
  }

+  public function canTriggerOnObject($object) {
+    if ($object instanceof PhabricatorRepository) {
+      return true;
+    }
+    return false;
+  }
+
+  public function explainValidTriggerObjects() {
+    return pht(
+      'This rule can trigger for **repositories**.');
+  }
+
+  public function getTriggerObjectPHIDs() {
+    return array(
+      $this->hookEngine->getRepository()->getPHID(),
+      $this->getPHID(),
+    );
+  }
+
  public function getFieldNameMap() {
    return array(
    ) + parent::getFieldNameMap();
@ -92,6 +111,7 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter {
  public function getActions($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return array(
          self::ACTION_BLOCK,
          self::ACTION_NOTHING
--- a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php
+++ b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php
@ -47,14 +47,33 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter {
  public function supportsRuleType($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return true;
      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
      default:
        return false;
    }
  }

+  public function canTriggerOnObject($object) {
+    if ($object instanceof PhabricatorRepository) {
+      return true;
+    }
+    return false;
+  }
+
+  public function explainValidTriggerObjects() {
+    return pht(
+      'This rule can trigger for **repositories**.');
+  }
+
+  public function getTriggerObjectPHIDs() {
+    return array(
+      $this->hookEngine->getRepository()->getPHID(),
+      $this->getPHID(),
+    );
+  }
+
  public function getFieldNameMap() {
    return array(
      self::FIELD_REF_TYPE => pht('Ref type'),
@ -103,6 +122,7 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter {
  public function getActions($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return array(
          self::ACTION_BLOCK,
          self::ACTION_NOTHING
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@ -144,6 +144,18 @@ abstract class HeraldAdapter {
    return false;
  }

+  public function canTriggerOnObject($object) {
+    return false;
+  }
+
+  public function explainValidTriggerObjects() {
+    return pht('This adapter can not trigger on objects.');
+  }
+
+  public function getTriggerObjectPHIDs() {
+    return array($this->getPHID());
+  }
+
  public function getAdapterSortKey() {
    return sprintf(
      '%08d%s',
@ -577,6 +589,7 @@ abstract class HeraldAdapter {
  public function getActionNameMap($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return array(
          self::ACTION_NOTHING      => pht('Do nothing'),
          self::ACTION_ADD_CC       => pht('Add emails to CC'),
@ -1002,6 +1015,11 @@ abstract class HeraldAdapter {
        }
      }
    }
+
+    if ($rule->isObjectRule()) {
+      $phids[] = $rule->getTriggerObjectPHID();
+    }
+
    return $phids;
  }

--- a/src/applications/herald/adapter/HeraldCommitAdapter.php
+++ b/src/applications/herald/adapter/HeraldCommitAdapter.php
@ -53,13 +53,32 @@ final class HeraldCommitAdapter extends HeraldAdapter {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
      case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
-        return true;
      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
+        return true;
      default:
        return false;
    }
  }

+  public function canTriggerOnObject($object) {
+    if ($object instanceof PhabricatorRepository) {
+      return true;
+    }
+    return false;
+  }
+
+  public function getTriggerObjectPHIDs() {
+    return array(
+      $this->repository->getPHID(),
+      $this->getPHID(),
+    );
+  }
+
+  public function explainValidTriggerObjects() {
+    return pht(
+      'This rule can trigger for **repositories**.');
+  }
+
  public function getFieldNameMap() {
    return array(
      self::FIELD_NEED_AUDIT_FOR_PACKAGE =>
@ -111,6 +130,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
  public function getActions($rule_type) {
    switch ($rule_type) {
      case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
+      case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
        return array(
          self::ACTION_ADD_CC,
          self::ACTION_EMAIL,
--- a/src/applications/herald/controller/HeraldNewController.php
+++ b/src/applications/herald/controller/HeraldNewController.php
@ -4,19 +4,19 @@ final class HeraldNewController extends HeraldController {

  public function processRequest() {
    $request = $this->getRequest();
-    $user = $request->getUser();
+    $viewer = $request->getUser();

-    $content_type_map = HeraldAdapter::getEnabledAdapterMap($user);
+    $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer);
    $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap();

    $errors = array();

    $e_type = null;
    $e_rule = null;
+    $e_object = null;

-    $step = 0;
+    $step = $request->getInt('step');
    if ($request->isFormPost()) {
-      $step = $request->getInt('step');
      $content_type = $request->getStr('content_type');
      if (empty($content_type_map[$content_type])) {
        $errors[] = pht('You must choose a content type for this rule.');
@ -33,24 +33,77 @@ final class HeraldNewController extends HeraldController {
        }
      }

-      if (!$errors && $step == 2) {
-        $uri = id(new PhutilURI('edit/'))
-          ->setQueryParams(
-            array(
-              'content_type' => $content_type,
-              'rule_type' => $rule_type,
-            ));
-        $uri = $this->getApplicationURI($uri);
-        return id(new AphrontRedirectResponse())->setURI($uri);
+      if (!$errors && $step >= 2) {
+        $target_phid = null;
+        $object_name = $request->getStr('objectName');
+        $done = false;
+        if ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_OBJECT) {
+          $done = true;
+        } else if (strlen($object_name)) {
+          $target_object = id(new PhabricatorObjectQuery())
+            ->setViewer($viewer)
+            ->withNames(array($object_name))
+            ->executeOne();
+          if ($target_object) {
+            $can_edit = PhabricatorPolicyFilter::hasCapability(
+              $viewer,
+              $target_object,
+              PhabricatorPolicyCapability::CAN_EDIT);
+            if (!$can_edit) {
+              $errors[] = pht(
+                'You can not create a rule for that object, because you do '.
+                'not have permission to edit it. You can only create rules '.
+                'for objects you can edit.');
+              $e_object = pht('Not Editable');
+              $step = 2;
+            } else {
+              $adapter = HeraldAdapter::getAdapterForContentType($content_type);
+              if (!$adapter->canTriggerOnObject($target_object)) {
+                $errors[] = pht(
+                  'This object is not of an allowed type for the rule. '.
+                  'Rules can only trigger on certain objects.');
+                $e_object = pht('Invalid');
+                $step = 2;
+              } else {
+                $target_phid = $target_object->getPHID();
+                $done = true;
+              }
+            }
+          } else {
+            $errors[] = pht('No object exists by that name.');
+            $e_object = pht('Invalid');
+            $step = 2;
+          }
+        } else if ($step > 2) {
+          $errors[] = pht(
+            'You must choose an object to associate this rule with.');
+          $e_object = pht('Required');
+          $step = 2;
+        }
+
+        if (!$errors && $done) {
+          $uri = id(new PhutilURI('edit/'))
+            ->setQueryParams(
+              array(
+                'content_type' => $content_type,
+                'rule_type' => $rule_type,
+                'targetPHID' => $target_phid,
+              ));
+          $uri = $this->getApplicationURI($uri);
+          return id(new AphrontRedirectResponse())->setURI($uri);
+        }
      }
    }

+    $content_type = $request->getStr('content_type');
+    $rule_type = $request->getStr('rule_type');
+
    if ($errors) {
      $errors = id(new AphrontErrorView())->setErrors($errors);
    }

    $form = id(new AphrontFormView())
-      ->setUser($user)
+      ->setUser($viewer)
      ->setAction($this->getApplicationURI('new/'));

    switch ($step) {
@ -73,7 +126,7 @@ final class HeraldNewController extends HeraldController {
          $e_rule);

        $form
-          ->addHiddenInput('content_type', $request->getStr('content_type'))
+          ->addHiddenInput('content_type', $content_type)
          ->addHiddenInput('step', 2)
          ->appendChild(
            id(new AphrontFormStaticControl())
@ -89,7 +142,52 @@ final class HeraldNewController extends HeraldController {
        $cancel_uri = id(new PhutilURI('new/'))
          ->setQueryParams(
            array(
-              'content_type' => $request->getStr('content_type'),
+              'content_type' => $content_type,
+              'step' => 0,
+            ));
+        $cancel_uri = $this->getApplicationURI($cancel_uri);
+        break;
+      case 2:
+        $adapter = HeraldAdapter::getAdapterForContentType($content_type);
+        $form
+          ->addHiddenInput('content_type', $content_type)
+          ->addHiddenInput('rule_type', $rule_type)
+          ->addHiddenInput('step', 3)
+          ->appendChild(
+            id(new AphrontFormStaticControl())
+              ->setLabel(pht('Rule for'))
+              ->setValue(
+                phutil_tag(
+                  'strong',
+                  array(),
+                  idx($content_type_map, $content_type))))
+          ->appendChild(
+            id(new AphrontFormStaticControl())
+              ->setLabel(pht('Rule Type'))
+              ->setValue(
+                phutil_tag(
+                  'strong',
+                  array(),
+                  idx($rule_type_map, $rule_type))))
+          ->appendRemarkupInstructions(
+            pht(
+              'Choose the object this rule will act on (for example, enter '.
+              '`rX` to act on the `rX` repository).'))
+          ->appendRemarkupInstructions(
+            $adapter->explainValidTriggerObjects())
+          ->appendChild(
+            id(new AphrontFormTextControl())
+              ->setName('objectName')
+              ->setError($e_object)
+              ->setValue($request->getStr('objectName'))
+              ->setLabel(pht('Object')));
+
+        $cancel_text = pht('Back');
+        $cancel_uri = id(new PhutilURI('new/'))
+          ->setQueryParams(
+            array(
+              'content_type' => $content_type,
+              'rule_type' => $rule_type,
              'step' => 1,
            ));
        $cancel_uri = $this->getApplicationURI($cancel_uri);
@ -156,9 +254,6 @@ final class HeraldNewController extends HeraldController {
        HeraldRuleTypeConfig::RULE_TYPE_GLOBAL,
      )) + $rule_type_map;

-    // TODO: Enable this.
-    unset($rule_type_map[HeraldRuleTypeConfig::RULE_TYPE_OBJECT]);
-
    list($can_global, $global_link) = $this->explainApplicationCapability(
      HeraldCapabilityManageGlobalRules::CAPABILITY,
      pht('You have permission to create and manage global rules.'),
@ -170,6 +265,12 @@ final class HeraldNewController extends HeraldController {
          'Personal rules notify you about events. You own them, but they can '.
          'only affect you. Personal rules only trigger for objects you have '.
          'permission to see.'),
+      HeraldRuleTypeConfig::RULE_TYPE_OBJECT =>
+        pht(
+          'Object rules notify anyone about events. They are bound to an '.
+          'object (like a repository) and can only act on that object. You '.
+          'must be able to edit an object to create object rules for it. '.
+          'Other users who an edit the object can edit its rules.'),
      HeraldRuleTypeConfig::RULE_TYPE_GLOBAL =>
        array(
          pht(
@ -180,7 +281,7 @@ final class HeraldNewController extends HeraldController {
    );

    $radio = id(new AphrontFormRadioButtonControl())
-      ->setLabel(pht('Type'))
+      ->setLabel(pht('Rule Type'))
      ->setName('rule_type')
      ->setValue($request->getStr('rule_type'))
      ->setError($e_rule);
--- a/src/applications/herald/controller/HeraldRuleController.php
+++ b/src/applications/herald/controller/HeraldRuleController.php
@ -46,6 +46,38 @@ final class HeraldRuleController extends HeraldController {
      }
      $rule->setRuleType($rule_type);

+      $adapter = HeraldAdapter::getAdapterForContentType(
+        $rule->getContentType());
+
+      if (!$adapter->supportsRuleType($rule->getRuleType())) {
+        throw new Exception(
+          pht(
+            "This rule's content type does not support the selected rule ".
+            "type."));
+      }
+
+      if ($rule->isObjectRule()) {
+        $rule->setTriggerObjectPHID($request->getStr('targetPHID'));
+        $object = id(new PhabricatorObjectQuery())
+          ->setViewer($user)
+          ->withPHIDs(array($rule->getTriggerObjectPHID()))
+          ->requireCapabilities(
+            array(
+              PhabricatorPolicyCapability::CAN_VIEW,
+              PhabricatorPolicyCapability::CAN_EDIT,
+            ))
+          ->executeOne();
+        if (!$object) {
+          throw new Exception(
+            pht('No valid object provided for object rule!'));
+        }
+
+        if (!$adapter->canTriggerOnObject($object)) {
+          throw new Exception(
+            pht('Object is of wrong type for adapter!'));
+        }
+      }
+
      $cancel_uri = $this->getApplicationURI();
    }

@ -65,12 +97,6 @@ final class HeraldRuleController extends HeraldController {
          "deployment."));
    }

-    if (!$adapter->supportsRuleType($rule->getRuleType())) {
-      throw new Exception(
-        pht(
-          "This rule's content type does not support the selected rule type."));
-    }
-
    // Upgrade rule version to our version, since we might add newly-defined
    // conditions, etc.
    $rule->setConfigVersion($local_version);
@ -133,6 +159,16 @@ final class HeraldRuleController extends HeraldController {
          ->setError($e_name)
          ->setValue($rule->getName()));

+    $trigger_object_control = false;
+    if ($rule->isObjectRule()) {
+      $trigger_object_control = id(new AphrontFormStaticControl())
+        ->setValue(
+          pht(
+            'This rule triggers for %s.',
+            $handles[$rule->getTriggerObjectPHID()]->renderLink()));
+    }
+
+
    $form
      ->appendChild(
        id(new AphrontFormMarkupControl())
@ -140,6 +176,7 @@ final class HeraldRuleController extends HeraldController {
            "This %s rule triggers for %s.",
            phutil_tag('strong', array(), $rule_type_name),
            phutil_tag('strong', array(), $content_type_name))))
+      ->appendChild($trigger_object_control)
      ->appendChild(
        id(new AphrontFormInsetView())
          ->setTitle(pht('Conditions'))
@ -489,6 +526,10 @@ final class HeraldRuleController extends HeraldController {

    $phids[] = $rule->getAuthorPHID();

+    if ($rule->isObjectRule()) {
+      $phids[] = $rule->getTriggerObjectPHID();
+    }
+
    return $this->loadViewerHandles($phids);
  }

--- a/src/applications/herald/controller/HeraldRuleViewController.php
+++ b/src/applications/herald/controller/HeraldRuleViewController.php
@ -131,6 +131,7 @@ final class HeraldRuleViewController extends HeraldController {
        $this->getHandle($rule->getAuthorPHID())->renderLink());
    }

+
    $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType());
    if ($adapter) {
      $view->addProperty(
@ -139,6 +140,12 @@ final class HeraldRuleViewController extends HeraldController {
          HeraldAdapter::getEnabledAdapterMap($viewer),
          $rule->getContentType()));

+      if ($rule->isObjectRule()) {
+        $view->addProperty(
+          pht('Trigger Object'),
+          $this->getHandle($rule->getTriggerObjectPHID())->renderLink());
+      }
+
      $view->invokeWillRenderEvent();

      $view->addSectionHeader(pht('Rule Description'));
--- a/src/applications/herald/controller/HeraldTranscriptController.php
+++ b/src/applications/herald/controller/HeraldTranscriptController.php
@ -16,7 +16,7 @@ final class HeraldTranscriptController extends HeraldController {
    $map = $this->getFilterMap();
    $this->filter = idx($data, 'filter');
    if (empty($map[$this->filter])) {
-      $this->filter = self::FILTER_AFFECTED;
+      $this->filter = self::FILTER_ALL;
    }
  }

@ -149,9 +149,9 @@ final class HeraldTranscriptController extends HeraldController {

  protected function getFilterMap() {
    return array(
-      self::FILTER_AFFECTED => pht('Rules that Affected Me'),
-      self::FILTER_OWNED    => pht('Rules I Own'),
      self::FILTER_ALL      => pht('All Rules'),
+      self::FILTER_OWNED    => pht('Rules I Own'),
+      self::FILTER_AFFECTED => pht('Rules that Affected Me'),
    );
  }

--- a/src/applications/herald/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/HeraldEngine.php
@ -420,14 +420,14 @@ final class HeraldEngine {
    }

    $trigger_phid = $rule->getTriggerObjectPHID();
-    $object_phid = $adapter->getPHID();
+    $object_phids = $adapter->getTriggerObjectPHIDs();

-    if ($trigger_phid == $object_phid) {
-      return true;
+    if ($object_phids) {
+      if (in_array($trigger_phid, $object_phids)) {
+        return true;
+      }
    }

-    // TODO: We should also handle projects.
-
    return false;
  }

--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@ -17,7 +17,7 @@ final class HeraldRule extends HeraldDAO
  protected $isDisabled = 0;
  protected $triggerObjectPHID;

-  protected $configVersion = 22;
+  protected $configVersion = 23;

  // phids for which this rule has been applied
  private $ruleApplied = self::ATTACHABLE;
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php
@ -36,8 +36,10 @@ final class PhabricatorRepositoryCommitHeraldWorker
      $commit->getID());

    if (!$data) {
-      // TODO: Permanent failure.
-      return;
+      throw new PhabricatorWorkerPermanentFailureException(
+        pht(
+          'Unable to load commit data. The data for this task is invalid '.
+          'or no longer exists.'));
    }

    $adapter = HeraldCommitAdapter::newLegacyAdapter(