Page MenuHomePhabricator
Files F15385388

D15939.id38376.diff
No OneTemporary
Actions

D15939.id38376.diff
View Options

diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -33,108 +33,132 @@
$repository,
$commit,
PhabricatorUser::getOmnipotentUser());
+
$affected_packages = PhabricatorOwnersPackage::loadAffectedPackages(
$repository,
$affected_paths);
- if ($affected_packages) {
- $requests = id(new PhabricatorRepositoryAuditRequest())
- ->loadAllWhere(
- 'commitPHID = %s',
- $commit->getPHID());
- $requests = mpull($requests, null, 'getAuditorPHID');
-
- foreach ($affected_packages as $package) {
- $request = idx($requests, $package->getPHID());
- if ($request) {
- // Don't update request if it exists already.
- continue;
- }
+ if (!$affected_packages) {
+ return;
+ }
- if ($package->isArchived()) {
- // Don't trigger audits if the package is archived.
- continue;
- }
+ $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+ 'commitID = %d',
+ $commit->getID());
+ $commit->attachCommitData($data);
- if ($package->getAuditingEnabled()) {
- $reasons = $this->checkAuditReasons($commit, $package);
- if ($reasons) {
- $audit_status =
- PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
- } else {
- $audit_status =
- PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
- }
- } else {
- $reasons = array();
- $audit_status = PhabricatorAuditStatusConstants::NONE;
- }
+ $author_phid = $data->getCommitDetail('authorPHID');
+ $revision_id = $data->getCommitDetail('differential.revisionID');
+ if ($revision_id) {
+ $revision = id(new DifferentialRevisionQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withIDs(array($revision_id))
+ ->needReviewerStatus(true)
+ ->executeOne();
+ } else {
+ $revision = null;
+ }
- $relationship = new PhabricatorRepositoryAuditRequest();
- $relationship->setAuditorPHID($package->getPHID());
- $relationship->setCommitPHID($commit->getPHID());
- $relationship->setAuditReasons($reasons);
- $relationship->setAuditStatus($audit_status);
+ $requests = id(new PhabricatorRepositoryAuditRequest())
+ ->loadAllWhere(
+ 'commitPHID = %s',
+ $commit->getPHID());
+ $requests = mpull($requests, null, 'getAuditorPHID');
- $relationship->save();
- $requests[$package->getPHID()] = $relationship;
+ foreach ($affected_packages as $package) {
+ $request = idx($requests, $package->getPHID());
+ if ($request) {
+ // Don't update request if it exists already.
+ continue;
}
- $commit->updateAuditStatus($requests);
- $commit->save();
+ if ($package->isArchived()) {
+ // Don't trigger audits if the package is archived.
+ continue;
+ }
+
+ if ($package->getAuditingEnabled()) {
+ $reasons = $this->checkAuditReasons(
+ $commit,
+ $package,
+ $author_phid,
+ $revision);
+
+ if ($reasons) {
+ $audit_status = PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
+ } else {
+ $audit_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
+ }
+ } else {
+ $reasons = array();
+ $audit_status = PhabricatorAuditStatusConstants::NONE;
+ }
+
+ $relationship = new PhabricatorRepositoryAuditRequest();
+ $relationship->setAuditorPHID($package->getPHID());
+ $relationship->setCommitPHID($commit->getPHID());
+ $relationship->setAuditReasons($reasons);
+ $relationship->setAuditStatus($audit_status);
+
+ $relationship->save();
+
+ $requests[$package->getPHID()] = $relationship;
}
+
+ $commit->updateAuditStatus($requests);
+ $commit->save();
}
private function checkAuditReasons(
PhabricatorRepositoryCommit $commit,
- PhabricatorOwnersPackage $package) {
+ PhabricatorOwnersPackage $package,
+ $author_phid,
+ $revision) {
- $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
- 'commitID = %d',
- $commit->getID());
+ $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
+ array(
+ $package->getID(),
+ ));
+ $owner_phids = array_fuse($owner_phids);
$reasons = array();
- if ($data->getCommitDetail('vsDiff')) {
- $reasons[] = pht('Changed After Revision Was Accepted');
+ if (!$author_phid) {
+ $reasons[] = pht('Commit Author Not Recognized');
+ } else if (isset($owner_phids[$author_phid])) {
+ return $reasons;
}
- $commit_author_phid = $data->getCommitDetail('authorPHID');
- if (!$commit_author_phid) {
- $reasons[] = pht('Commit Author Not Recognized');
+ if (!$revision) {
+ $reasons[] = pht('No Revision Specified');
+ return $reasons;
}
- $revision_id = $data->getCommitDetail('differential.revisionID');
+ $accepted_statuses = array(
+ DifferentialReviewerStatus::STATUS_ACCEPTED,
+ DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER,
+ );
+ $accepted_statuses = array_fuse($accepted_statuses);
- $revision_author_phid = null;
- $commit_reviewedby_phid = null;
+ $found_accept = false;
+ foreach ($revision->getReviewerStatus() as $reviewer) {
+ $reviewer_phid = $reviewer->getReviewerPHID();
- if ($revision_id) {
- $revision = id(new DifferentialRevisionQuery())
- ->setViewer(PhabricatorUser::getOmnipotentUser())
- ->withIDs(array($revision_id))
- ->executeOne();
- if ($revision) {
- $revision_author_phid = $revision->getAuthorPHID();
- $commit_reviewedby_phid = $data->getCommitDetail('reviewerPHID');
- if ($revision_author_phid !== $commit_author_phid) {
- $reasons[] = pht('Author Not Matching with Revision');
- }
- } else {
- $reasons[] = pht('Revision Not Found');
+ // If this reviewer isn't a package owner, just ignore them.
+ if (empty($owner_phids[$reviewer_phid])) {
+ continue;
}
- } else {
- $reasons[] = pht('No Revision Specified');
+ // If this reviewer accepted the revision and owns the package, we're
+ // all clear and do not need to trigger an audit.
+ if (isset($accepted_statuses[$reviewer->getStatus()])) {
+ $found_accept = true;
+ break;
+ }
}
- $owners_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
- array($package->getID()));
-
- if (!($commit_author_phid && in_array($commit_author_phid, $owners_phids) ||
- $commit_reviewedby_phid && in_array($commit_reviewedby_phid,
- $owners_phids))) {
+ if (!$found_accept) {
$reasons[] = pht('Owners Not Involved');
}
diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner
--- a/src/docs/user/userguide/owners.diviner
+++ b/src/docs/user/userguide/owners.diviner
@@ -98,11 +98,35 @@
NOTE: These rules **do not trigger** if the change author is a package owner.
They only apply to changes made by users who aren't already owners.
+These rules also do not trigger if the package has been archived.
+
The intent of this feature is to make it easy to configure simple, reasonable
behaviors. If you want more tailored or specific triggers, you can write more
powerful rules by using Herald.
+Auditing
+========
+
+You can automatically trigger audits on unreviewed code by configuring
+**Auditing**. The available settings are:
+
+ - **Disabled**: Do not trigger audits.
+ - **Enabled**: Trigger audits.
+
+When enabled, audits are triggered for commits which:
+
+ - affect code owned by the package;
+ - were not authored by a package owner; and
+ - were not accepted by a package owner.
+
+Audits do not trigger if the package has been archived.
+
+The intent of this feature is to make it easy to configure simple auditing
+behavior. If you want more powerful auditing rules, you can use Herald to
+write more sophisticated rules.
+
+
Files in Multiple Packages
==========================

File Metadata

Mime Type
text/plain
Expires
Sat, Mar 15, 10:27 PM (4 w, 16 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7704191
Default Alt Text
D15939.id38376.diff (8 KB)

Event Timeline

Phabricator · Built by Phacility