From 140c88e97136a0f9f571b8e2b94ddd0971ef2710 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 Dec 2013 16:48:14 -0800 Subject: [PATCH] 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 --- .../herald/HeraldPreCommitContentAdapter.php | 22 ++- .../herald/HeraldPreCommitRefAdapter.php | 22 ++- .../herald/adapter/HeraldAdapter.php | 18 +++ .../herald/adapter/HeraldCommitAdapter.php | 22 ++- .../herald/controller/HeraldNewController.php | 141 +++++++++++++++--- .../controller/HeraldRuleController.php | 53 ++++++- .../controller/HeraldRuleViewController.php | 7 + .../controller/HeraldTranscriptController.php | 6 +- .../herald/engine/HeraldEngine.php | 10 +- .../herald/storage/HeraldRule.php | 2 +- ...habricatorRepositoryCommitHeraldWorker.php | 6 +- 11 files changed, 269 insertions(+), 40 deletions(-) diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index aeff27ad11..d0472bdaa8 100644 --- 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 diff --git a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php index 90f6748571..dbbf6f6596 100644 --- 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 diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 5735321b41..1a2fc9a378 100644 --- 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; } diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 5206860bae..85811ebfa3 100644 --- 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, diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php index 610c74d8ca..bfc37d9edc 100644 --- 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); diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index e3ddabae0e..a5f11edfdb 100644 --- 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); } diff --git a/src/applications/herald/controller/HeraldRuleViewController.php b/src/applications/herald/controller/HeraldRuleViewController.php index 115f85b718..ea5c6b44e2 100644 --- 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')); diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php index 7e320b235d..c08f54d3e8 100644 --- 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'), ); } diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index d3d3899b98..f2915d703e 100644 --- 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; } diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index a441dab770..2eb442a8c7 100644 --- 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; diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 8c4201aafb..4d1d702e4f 100644 --- 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(