From a95c9873aa1ecf11c6412210833f504fc8061863 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Mar 2012 15:10:35 -0800 Subject: [PATCH] Add an "Auditors" field to commit messages which pushes audit requests when present Summary: Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests. This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential. It is then parsed by the daemons if present, and audit requests are pushed to valid users. Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered. Reviewers: btrahan Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T904, T880 Differential Revision: https://secure.phabricator.com/D1793 --- src/__phutil_library_map__.php | 2 + .../PhabricatorAuditStatusConstants.php | 12 +-- .../query/audit/PhabricatorAuditQuery.php | 1 + .../DifferentialDefaultFieldSelector.php | 1 + .../field/selector/default/__init__.php | 1 + ...DifferentialAuditorsFieldSpecification.php | 79 +++++++++++++++++++ .../field/specification/auditors/__init__.php | 14 ++++ .../base/DifferentialFieldSpecification.php | 15 ++-- ...ifferentialReviewersFieldSpecification.php | 2 +- ...habricatorRepositoryCommitHeraldWorker.php | 64 ++++++++++++++- .../repository/worker/herald/__init__.php | 1 + 11 files changed, 179 insertions(+), 13 deletions(-) create mode 100644 src/applications/differential/field/specification/auditors/DifferentialAuditorsFieldSpecification.php create mode 100644 src/applications/differential/field/specification/auditors/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6263b4f642..ecf46e1da2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -185,6 +185,7 @@ phutil_register_library_map(array( 'DifferentialAffectedPath' => 'applications/differential/storage/affectedpath', 'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch', 'DifferentialArcanistProjectFieldSpecification' => 'applications/differential/field/specification/arcanistproject', + 'DifferentialAuditorsFieldSpecification' => 'applications/differential/field/specification/auditors', 'DifferentialAuthorFieldSpecification' => 'applications/differential/field/specification/author', 'DifferentialAuxiliaryField' => 'applications/differential/storage/auxiliaryfield', 'DifferentialBlameRevisionFieldSpecification' => 'applications/differential/field/specification/blamerev', @@ -1050,6 +1051,7 @@ phutil_register_library_map(array( 'DifferentialAffectedPath' => 'DifferentialDAO', 'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialArcanistProjectFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialAuditorsFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialAuthorFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialAuxiliaryField' => 'DifferentialDAO', 'DifferentialBlameRevisionFieldSpecification' => 'DifferentialFieldSpecification', diff --git a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php index 74aa302e6c..517a7c8199 100644 --- a/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php @@ -18,11 +18,12 @@ final class PhabricatorAuditStatusConstants { - const NONE = ''; - const AUDIT_NOT_REQUIRED = 'audit-not-required'; - const AUDIT_REQUIRED = 'audit-required'; - const CONCERNED = 'concerned'; - const ACCEPTED = 'accepted'; + const NONE = ''; + const AUDIT_NOT_REQUIRED = 'audit-not-required'; + const AUDIT_REQUIRED = 'audit-required'; + const CONCERNED = 'concerned'; + const ACCEPTED = 'accepted'; + const AUDIT_REQUESTED = 'requested'; public static function getStatusNameMap() { static $map = array( @@ -31,6 +32,7 @@ final class PhabricatorAuditStatusConstants { self::AUDIT_REQUIRED => 'Audit Required', self::CONCERNED => 'Concern Raised', self::ACCEPTED => 'Accepted', + self::AUDIT_REQUESTED => 'Audit Requested', ); return $map; diff --git a/src/applications/audit/query/audit/PhabricatorAuditQuery.php b/src/applications/audit/query/audit/PhabricatorAuditQuery.php index c390ff3c50..7987d9926d 100644 --- a/src/applications/audit/query/audit/PhabricatorAuditQuery.php +++ b/src/applications/audit/query/audit/PhabricatorAuditQuery.php @@ -98,6 +98,7 @@ final class PhabricatorAuditQuery { array( PhabricatorAuditStatusConstants::AUDIT_REQUIRED, PhabricatorAuditStatusConstants::CONCERNED, + PhabricatorAuditStatusConstants::AUDIT_REQUESTED, )); break; case self::STATUS_ANY: diff --git a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php index 9faf9319ee..1a2baecbf1 100644 --- a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php +++ b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php @@ -55,6 +55,7 @@ final class DifferentialDefaultFieldSelector new DifferentialGitSVNIDFieldSpecification(), new DifferentialDateModifiedFieldSpecification(), new DifferentialDateCreatedFieldSpecification(), + new DifferentialAuditorsFieldSpecification(), )); return $fields; diff --git a/src/applications/differential/field/selector/default/__init__.php b/src/applications/differential/field/selector/default/__init__.php index 5d86554148..c047bea250 100644 --- a/src/applications/differential/field/selector/default/__init__.php +++ b/src/applications/differential/field/selector/default/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'applications/differential/field/selector/base'); phutil_require_module('phabricator', 'applications/differential/field/specification/applypatch'); phutil_require_module('phabricator', 'applications/differential/field/specification/arcanistproject'); +phutil_require_module('phabricator', 'applications/differential/field/specification/auditors'); phutil_require_module('phabricator', 'applications/differential/field/specification/author'); phutil_require_module('phabricator', 'applications/differential/field/specification/branch'); phutil_require_module('phabricator', 'applications/differential/field/specification/ccs'); diff --git a/src/applications/differential/field/specification/auditors/DifferentialAuditorsFieldSpecification.php b/src/applications/differential/field/specification/auditors/DifferentialAuditorsFieldSpecification.php new file mode 100644 index 0000000000..bafa3811b0 --- /dev/null +++ b/src/applications/differential/field/specification/auditors/DifferentialAuditorsFieldSpecification.php @@ -0,0 +1,79 @@ +auditors = nonempty($value, array()); + return $this; + } + + public function renderLabelForCommitMessage() { + return 'Auditors'; + } + + public function getRequiredHandlePHIDsForCommitMessage() { + return $this->auditors; + } + + public function renderValueForCommitMessage($is_edit) { + if (!$this->auditors) { + return null; + } + + $names = array(); + foreach ($this->auditors as $phid) { + $names[] = $this->getHandle($phid)->getName(); + } + + return implode(', ', $names); + } + + public function parseValueFromCommitMessage($value) { + return $this->parseCommitMessageUserList($value); + } + + public function getStorageKey() { + return 'phabricator:auditors'; + } + + public function getValueForStorage() { + return json_encode($this->auditors); + } + + public function setValueFromStorage($value) { + $this->auditors = json_decode($value, true); + return $this; + } + +} diff --git a/src/applications/differential/field/specification/auditors/__init__.php b/src/applications/differential/field/specification/auditors/__init__.php new file mode 100644 index 0000000000..4595d028e5 --- /dev/null +++ b/src/applications/differential/field/specification/auditors/__init__.php @@ -0,0 +1,14 @@ +loadAllWhere( '((username IN (%Ls)) OR (email IN (%Ls))) - AND isDisabled = 0 AND isSystemAgent = 0', $value, $value); @@ -691,14 +697,13 @@ abstract class DifferentialFieldSpecification { } } - if ($invalid) { + if ($invalid && !$allow_partial) { $invalid = implode(', ', $invalid); $what = $include_mailables ? "users and mailing lists" : "users"; throw new DifferentialFieldParseException( - "Commit message references disabled or nonexistent {$what}: ". - "{$invalid}."); + "Commit message references nonexistent {$what}: {$invalid}."); } return array_unique($results); diff --git a/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php index 6934bf1dd9..91d3908477 100644 --- a/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php +++ b/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php @@ -19,7 +19,7 @@ final class DifferentialReviewersFieldSpecification extends DifferentialFieldSpecification { - private $reviewers; + private $reviewers = array(); private $error; public function shouldAppearOnRevisionView() { diff --git a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php index d5c9fd5057..ccfb8bfaa3 100644 --- a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php @@ -27,6 +27,11 @@ class PhabricatorRepositoryCommitHeraldWorker 'commitID = %d', $commit->getID()); + if (!$data) { + // TODO: Permanent failure. + return; + } + $rules = HeraldRule::loadAllByContentTypeWithFullData( HeraldContentTypeConfig::CONTENT_TYPE_COMMIT, $commit->getPHID()); @@ -45,6 +50,8 @@ class PhabricatorRepositoryCommitHeraldWorker $this->createAudits($commit, $audit_phids, $rules); } + $this->createAuditsFromCommitMessage($commit, $data); + $email_phids = $adapter->getEmailPHIDs(); if (!$email_phids) { return; @@ -173,8 +180,7 @@ EOBODY; array $map, array $rules) { - $table = new PhabricatorRepositoryAuditRequest(); - $requests = $table->loadAllWhere( + $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere( 'commitPHID = %s', $commit->getPHID()); $requests = mpull($requests, null, 'getAuditorPHID'); @@ -205,4 +211,58 @@ EOBODY; $commit->updateAuditStatus($requests); $commit->save(); } + + + /** + * Find audit requests in the "Auditors" field if it is present and trigger + * explicit audit requests. + */ + private function createAuditsFromCommitMessage( + PhabricatorRepositoryCommit $commit, + PhabricatorRepositoryCommitData $data) { + + $message = $data->getCommitMessage(); + + $matches = null; + if (!preg_match('/^Auditors:\s*(.*)$/im', $message, $matches)) { + return; + } + + $phids = DifferentialFieldSpecification::parseCommitMessageObjectList( + $matches[1], + $include_mailables = false, + $allow_partial = true); + + if (!$phids) { + return; + } + + $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere( + 'commitPHID = %s', + $commit->getPHID()); + $requests = mpull($requests, null, 'getAuditorPHID'); + + foreach ($phids as $phid) { + if (isset($requests[$phid])) { + continue; + } + + $request = new PhabricatorRepositoryAuditRequest(); + $request->setCommitPHID($commit->getPHID()); + $request->setAuditorPHID($phid); + $request->setAuditStatus( + PhabricatorAuditStatusConstants::AUDIT_REQUESTED); + $request->setAuditReasons( + array( + 'Requested by Author', + )); + $request->save(); + + $requests[$phid] = $request; + } + + $commit->updateAuditStatus($requests); + $commit->save(); + } + } diff --git a/src/applications/repository/worker/herald/__init__.php b/src/applications/repository/worker/herald/__init__.php index d9d803eb70..c2d2e60aa8 100644 --- a/src/applications/repository/worker/herald/__init__.php +++ b/src/applications/repository/worker/herald/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/audit/constants/status'); phutil_require_module('phabricator', 'applications/audit/editor/comment'); +phutil_require_module('phabricator', 'applications/differential/field/specification/base'); phutil_require_module('phabricator', 'applications/herald/adapter/commit'); phutil_require_module('phabricator', 'applications/herald/config/contenttype'); phutil_require_module('phabricator', 'applications/herald/engine/engine');