Index: src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php =================================================================== --- src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -44,14 +44,33 @@ 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 @@ 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 Index: src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php =================================================================== --- src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php +++ src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php @@ -47,14 +47,33 @@ 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 @@ 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 Index: src/applications/herald/adapter/HeraldAdapter.php =================================================================== --- src/applications/herald/adapter/HeraldAdapter.php +++ src/applications/herald/adapter/HeraldAdapter.php @@ -144,6 +144,18 @@ 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 @@ 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 @@ } } } + + if ($rule->isObjectRule()) { + $phids[] = $rule->getTriggerObjectPHID(); + } + return $phids; } Index: src/applications/herald/adapter/HeraldCommitAdapter.php =================================================================== --- src/applications/herald/adapter/HeraldCommitAdapter.php +++ src/applications/herald/adapter/HeraldCommitAdapter.php @@ -53,13 +53,32 @@ 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 @@ 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, Index: src/applications/herald/controller/HeraldNewController.php =================================================================== --- src/applications/herald/controller/HeraldNewController.php +++ src/applications/herald/controller/HeraldNewController.php @@ -4,19 +4,19 @@ 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 @@ } } - 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 @@ $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 @@ $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 @@ 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 @@ '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 @@ ); $radio = id(new AphrontFormRadioButtonControl()) - ->setLabel(pht('Type')) + ->setLabel(pht('Rule Type')) ->setName('rule_type') ->setValue($request->getStr('rule_type')) ->setError($e_rule); Index: src/applications/herald/controller/HeraldRuleController.php =================================================================== --- src/applications/herald/controller/HeraldRuleController.php +++ src/applications/herald/controller/HeraldRuleController.php @@ -46,6 +46,38 @@ } $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 @@ "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 @@ ->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 @@ "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 @@ $phids[] = $rule->getAuthorPHID(); + if ($rule->isObjectRule()) { + $phids[] = $rule->getTriggerObjectPHID(); + } + return $this->loadViewerHandles($phids); } Index: src/applications/herald/controller/HeraldRuleViewController.php =================================================================== --- src/applications/herald/controller/HeraldRuleViewController.php +++ src/applications/herald/controller/HeraldRuleViewController.php @@ -131,6 +131,7 @@ $this->getHandle($rule->getAuthorPHID())->renderLink()); } + $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType()); if ($adapter) { $view->addProperty( @@ -139,6 +140,12 @@ 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')); Index: src/applications/herald/controller/HeraldTranscriptController.php =================================================================== --- src/applications/herald/controller/HeraldTranscriptController.php +++ src/applications/herald/controller/HeraldTranscriptController.php @@ -16,7 +16,7 @@ $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 @@ 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'), ); } Index: src/applications/herald/engine/HeraldEngine.php =================================================================== --- src/applications/herald/engine/HeraldEngine.php +++ src/applications/herald/engine/HeraldEngine.php @@ -420,14 +420,14 @@ } $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; } Index: src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php =================================================================== --- src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -36,8 +36,10 @@ $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(