From 4faab06c3c76188f7c68652c240ae832fc432498 Mon Sep 17 00:00:00 2001 From: jungejason Date: Fri, 13 Jan 2012 23:08:19 -0800 Subject: [PATCH] Enable herald rule for commits that need auditing Summary: enable herald commit rules to have access to auditing info. Note that the new herald condition I added contains info for the packages. I thought about using a simpler herald condition like "Requires audit is true or false" and let it work together with the existing "Affected package contains any of the package". It doesn't work because we need the info about the package to decide if the commit requires audit, but the herald conditions work separately. Test Plan: - A commit requiring auditing was detected by a herald rule that checks the auditing status - A commit not requiring auditing was not detected by a herald rule which checks auditing status, but was detected by a rule which doesn't check the auditing status Reviewers: epriestley, nh Reviewed By: epriestley CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1399 --- .../adapter/commit/HeraldCommitAdapter.php | 23 ++++++- .../herald/adapter/commit/__init__.php | 2 + .../condition/HeraldConditionConfig.php | 3 +- .../herald/config/field/HeraldFieldConfig.php | 6 +- .../valuetype/HeraldValueTypeConfig.php | 3 +- .../herald/engine/engine/HeraldEngine.php | 3 +- ...atorRepositoryCommitChangeParserWorker.php | 10 +-- ...habricatorRepositoryCommitOwnersWorker.php | 66 ++++++++++--------- .../repository/worker/owner/__init__.php | 1 + 9 files changed, 73 insertions(+), 44 deletions(-) diff --git a/src/applications/herald/adapter/commit/HeraldCommitAdapter.php b/src/applications/herald/adapter/commit/HeraldCommitAdapter.php index 31c6537a6c..c39c7946f4 100644 --- a/src/applications/herald/adapter/commit/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/commit/HeraldCommitAdapter.php @@ -1,7 +1,7 @@ affectedPackages; } + public function loadAuditNeededPackage() { + if ($this->auditNeededPackages === null) { + $status_arr = array ( + PhabricatorAuditStatusConstants::AUDIT_REQUIRED, + PhabricatorAuditStatusConstants::CONCERNED, + ); + $relationships = id(new PhabricatorOwnersPackageCommitRelationship()) + ->loadAllWhere( + "commitPHID = %s AND auditStatus IN (%Ls)", + $this->commit->getPHID(), + $status_arr); + + $packages = mpull($relationships, 'getPackagePHID'); + $this->auditNeededPackages = $packages; + } + return $this->auditNeededPackages; + } + public function loadDifferentialRevision() { if ($this->affectedRevision === null) { $this->affectedRevision = false; @@ -140,6 +159,8 @@ class HeraldCommitAdapter extends HeraldObjectAdapter { $packages = $this->loadAffectedPackages(); $owners = PhabricatorOwnersOwner::loadAllForPackages($packages); return mpull($owners, 'getUserPHID'); + case HeraldFieldConfig::FIELD_NEED_AUDIT_FOR_PACKAGE: + return $this->loadAuditNeededPackage(); case HeraldFieldConfig::FIELD_DIFFERENTIAL_REVISION: $revision = $this->loadDifferentialRevision(); if (!$revision) { diff --git a/src/applications/herald/adapter/commit/__init__.php b/src/applications/herald/adapter/commit/__init__.php index 24262a55f0..a436638308 100644 --- a/src/applications/herald/adapter/commit/__init__.php +++ b/src/applications/herald/adapter/commit/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/audit/constants/status'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phabricator', 'applications/herald/adapter/base'); phutil_require_module('phabricator', 'applications/herald/config/action'); @@ -15,6 +16,7 @@ phutil_require_module('phabricator', 'applications/herald/storage/transcript/app phutil_require_module('phabricator', 'applications/owners/query/path'); phutil_require_module('phabricator', 'applications/owners/storage/owner'); phutil_require_module('phabricator', 'applications/owners/storage/package'); +phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/herald/config/condition/HeraldConditionConfig.php b/src/applications/herald/config/condition/HeraldConditionConfig.php index 1716b640a0..60d683b126 100644 --- a/src/applications/herald/config/condition/HeraldConditionConfig.php +++ b/src/applications/herald/config/condition/HeraldConditionConfig.php @@ -1,7 +1,7 @@ 'Another Herald rule', self::FIELD_AFFECTED_PACKAGE => 'Any affected package', self::FIELD_AFFECTED_PACKAGE_OWNER => "Any affected package's owner", + self::FIELD_NEED_AUDIT_FOR_PACKAGE => + 'Affected packages that need audit', self::FIELD_DIFFERENTIAL_REVISION => 'Differential revision', self::FIELD_DIFFERENTIAL_REVIEWERS => 'Differential reviewers', self::FIELD_DIFFERENTIAL_CCS => 'Differential CCs', @@ -93,6 +96,7 @@ class HeraldFieldConfig { self::FIELD_RULE, self::FIELD_AFFECTED_PACKAGE, self::FIELD_AFFECTED_PACKAGE_OWNER, + self::FIELD_NEED_AUDIT_FOR_PACKAGE, self::FIELD_DIFFERENTIAL_REVISION, self::FIELD_DIFFERENTIAL_REVIEWERS, self::FIELD_DIFFERENTIAL_CCS, diff --git a/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php b/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php index 98d2731d34..cef37858e8 100644 --- a/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php +++ b/src/applications/herald/config/valuetype/HeraldValueTypeConfig.php @@ -1,7 +1,7 @@ object->getHeraldField($field); if (!is_array($result)) { throw new HeraldInvalidFieldException( diff --git a/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php index d264db1175..b18590747e 100644 --- a/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php @@ -1,7 +1,7 @@ shouldQueueFollowupTasks()) { - $herald_task = new PhabricatorWorkerTask(); - $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker'); - $herald_task->setData( - array( - 'commitID' => $commit->getID(), - )); - $herald_task->save(); - $owner_task = new PhabricatorWorkerTask(); $owner_task->setTaskClass('PhabricatorRepositoryCommitOwnersWorker'); $owner_task->setData( diff --git a/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php index c4c5510aad..08c5bb7eaa 100644 --- a/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php @@ -1,7 +1,7 @@ loadOneWhere('packagePHID=%s AND commitPHID=%s', + $package->getPHID(), + $commit->getPHID()); - foreach ($affected_packages as $package) { - $relationship = id(new PhabricatorOwnersPackageCommitRelationship()) - ->loadOneWhere('packagePHID=%s AND commitPHID=%s', - $package->getPHID(), - $commit->getPHID()); - - // Don't update relationship if it exists already - if (!$relationship) { - if ($package->getAuditingEnabled()) { - $reasons = $this->checkAuditReasons($commit, $package); - if ($reasons) { - $audit_status = - PhabricatorAuditStatusConstants::AUDIT_REQUIRED; + // Don't update relationship if it exists already + if (!$relationship) { + if ($package->getAuditingEnabled()) { + $reasons = $this->checkAuditReasons($commit, $package); + if ($reasons) { + $audit_status = + PhabricatorAuditStatusConstants::AUDIT_REQUIRED; + } else { + $audit_status = + PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; + } } else { - $audit_status = - PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED; + $reasons = array(); + $audit_status = PhabricatorAuditStatusConstants::NONE; } - } else { - $reasons = array(); - $audit_status = PhabricatorAuditStatusConstants::NONE; + + $relationship = new PhabricatorOwnersPackageCommitRelationship(); + $relationship->setPackagePHID($package->getPHID()); + $relationship->setCommitPHID($commit->getPHID()); + $relationship->setAuditReasons($reasons); + $relationship->setAuditStatus($audit_status); + + $relationship->save(); } - - $relationship = new PhabricatorOwnersPackageCommitRelationship(); - $relationship->setPackagePHID($package->getPHID()); - $relationship->setCommitPHID($commit->getPHID()); - $relationship->setAuditReasons($reasons); - $relationship->setAuditStatus($audit_status); - - $relationship->save(); } } + + $herald_task = new PhabricatorWorkerTask(); + $herald_task->setTaskClass('PhabricatorRepositoryCommitHeraldWorker'); + $herald_task->setData( + array( + 'commitID' => $commit->getID(), + )); + $herald_task->save(); } private function checkAuditReasons( diff --git a/src/applications/repository/worker/owner/__init__.php b/src/applications/repository/worker/owner/__init__.php index 5b27967247..89e144e125 100644 --- a/src/applications/repository/worker/owner/__init__.php +++ b/src/applications/repository/worker/owner/__init__.php @@ -14,6 +14,7 @@ phutil_require_module('phabricator', 'applications/owners/storage/package'); phutil_require_module('phabricator', 'applications/owners/storage/packagecommitrelationship'); phutil_require_module('phabricator', 'applications/repository/storage/commitdata'); phutil_require_module('phabricator', 'applications/repository/worker/base'); +phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task'); phutil_require_module('phutil', 'utils');