From 63f4e66b1122dad0f09051af8ed876a6c63b81b3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Apr 2015 10:00:45 -0700 Subject: [PATCH] Attach HeraldRules to HeraldEffects Summary: Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects. Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them. Just attach the `Rule` objects. Test Plan: Will test thoroughly after next-ish changeset. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7731 Differential Revision: https://secure.phabricator.com/D12269 --- .../editor/DifferentialDiffEditor.php | 11 +++------- .../engine/DiffusionCommitHookEngine.php | 16 +++++--------- .../herald/adapter/HeraldAdapter.php | 7 ++---- .../herald/adapter/HeraldCommitAdapter.php | 4 ++-- .../herald/engine/HeraldEffect.php | 22 +++++-------------- .../herald/engine/HeraldEngine.php | 14 +++++------- .../herald/storage/HeraldRule.php | 4 ++++ .../transcript/HeraldApplyTranscript.php | 2 +- 8 files changed, 28 insertions(+), 52 deletions(-) diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php index 69125da1e1..9d4fa0767c 100644 --- a/src/applications/differential/editor/DifferentialDiffEditor.php +++ b/src/applications/differential/editor/DifferentialDiffEditor.php @@ -145,12 +145,7 @@ final class DifferentialDiffEditor } if ($blocking_effect) { - $rule = idx($rules, $effect->getRuleID()); - if ($rule && strlen($rule->getName())) { - $rule_name = $rule->getName(); - } else { - $rule_name = pht('Unnamed Herald Rule'); - } + $rule = $blocking_effect->getRule(); $message = $effect->getTarget(); if (!strlen($message)) { @@ -164,8 +159,8 @@ final class DifferentialDiffEditor "Creation of this diff was rejected by Herald rule %s.\n". " Rule: %s\n". "Reason: %s", - 'H'.$effect->getRuleID(), - $rule_name, + $rule->getMonogram(), + $rule->getName(), $message)); } break; diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index fe1a5e150f..13c7127e08 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -337,22 +337,16 @@ final class DiffusionCommitHookEngine extends Phobject { } if ($blocking_effect) { + $rule = $blocking_effect->getRule(); + $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_HERALD; - $this->rejectDetails = $blocking_effect->getRulePHID(); + $this->rejectDetails = $rule->getPHID(); $message = $blocking_effect->getTarget(); if (!strlen($message)) { $message = pht('(None.)'); } - $rules = mpull($rules, null, 'getID'); - $rule = idx($rules, $effect->getRuleID()); - if ($rule && strlen($rule->getName())) { - $rule_name = $rule->getName(); - } else { - $rule_name = pht('Unnamed Herald Rule'); - } - $blocked_ref_name = coalesce( $blocked_update->getRefName(), $blocked_update->getRefNewShort()); @@ -364,9 +358,9 @@ final class DiffusionCommitHookEngine extends Phobject { "Change: %s\n". " Rule: %s\n". "Reason: %s", - 'H'.$blocking_effect->getRuleID(), + $rule->getMonogram(), $blocked_name, - $rule_name, + $rule->getName(), $message)); } } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 8440eb303c..7a66ab4f18 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -999,11 +999,8 @@ abstract class HeraldAdapter { public static function applyFlagEffect(HeraldEffect $effect, $phid) { $color = $effect->getTarget(); - // TODO: Silly that we need to load this again here. - $rule = id(new HeraldRule())->load($effect->getRuleID()); - $user = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - $rule->getAuthorPHID()); + $rule = $effect->getRule(); + $user = $rule->getAuthor(); $flag = PhabricatorFlagQuery::loadUserFlag($user, $phid); if ($flag) { diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 9580101640..9bdf4a1463 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -501,7 +501,7 @@ final class HeraldCommitAdapter extends HeraldAdapter { if (empty($this->addCCPHIDs[$phid])) { $this->addCCPHIDs[$phid] = array(); } - $this->addCCPHIDs[$phid][] = $effect->getRuleID(); + $this->addCCPHIDs[$phid][] = $effect->getRule()->getID(); } $result[] = new HeraldApplyTranscript( $effect, @@ -513,7 +513,7 @@ final class HeraldCommitAdapter extends HeraldAdapter { if (empty($this->auditMap[$phid])) { $this->auditMap[$phid] = array(); } - $this->auditMap[$phid][] = $effect->getRuleID(); + $this->auditMap[$phid][] = $effect->getRule()->getID(); } $result[] = new HeraldApplyTranscript( $effect, diff --git a/src/applications/herald/engine/HeraldEffect.php b/src/applications/herald/engine/HeraldEffect.php index 7d6c71b20e..d9a536be37 100644 --- a/src/applications/herald/engine/HeraldEffect.php +++ b/src/applications/herald/engine/HeraldEffect.php @@ -5,10 +5,7 @@ final class HeraldEffect { private $objectPHID; private $action; private $target; - - private $ruleID; - private $rulePHID; - + private $rule; private $reason; public function setObjectPHID($object_phid) { @@ -38,22 +35,13 @@ final class HeraldEffect { return $this->target; } - public function setRuleID($rule_id) { - $this->ruleID = $rule_id; + public function setRule(HeraldRule $rule) { + $this->rule = $rule; return $this; } - public function getRuleID() { - return $this->ruleID; - } - - public function setRulePHID($rule_phid) { - $this->rulePHID = $rule_phid; - return $this; - } - - public function getRulePHID() { - return $this->rulePHID; + public function getRule() { + return $this->rule; } public function setReason($reason) { diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index e15cfc24fb..9774cb866a 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/src/applications/herald/engine/HeraldEngine.php @@ -368,16 +368,14 @@ final class HeraldEngine { $effects = array(); foreach ($rule->getActions() as $action) { - $effect = new HeraldEffect(); - $effect->setObjectPHID($object->getPHID()); - $effect->setAction($action->getAction()); - $effect->setTarget($action->getTarget()); - - $effect->setRuleID($rule->getID()); - $effect->setRulePHID($rule->getPHID()); + $effect = id(new HeraldEffect()) + ->setObjectPHID($object->getPHID()) + ->setAction($action->getAction()) + ->setTarget($action->getTarget()) + ->setRule($rule); $name = $rule->getName(); - $id = $rule->getID(); + $id = $rule->getID(); $effect->setReason( pht( 'Conditions were met for %s', diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index fb9f5197bb..1c1623c5bf 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -265,6 +265,10 @@ final class HeraldRule extends HeraldDAO return sprintf('~%d%010d', $type_order, $this->getID()); } + public function getMonogram() { + return 'H'.$this->getID(); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/herald/storage/transcript/HeraldApplyTranscript.php b/src/applications/herald/storage/transcript/HeraldApplyTranscript.php index 81464760c9..10e6c3bc0d 100644 --- a/src/applications/herald/storage/transcript/HeraldApplyTranscript.php +++ b/src/applications/herald/storage/transcript/HeraldApplyTranscript.php @@ -16,7 +16,7 @@ final class HeraldApplyTranscript extends Phobject { $this->setAction($effect->getAction()); $this->setTarget($effect->getTarget()); - $this->setRuleID($effect->getRuleID()); + $this->setRuleID($effect->getRule()->getID()); $this->setReason($effect->getReason()); $this->setApplied($applied); $this->setAppliedReason($reason);