From 9c24798e64c952ad5b350ff06e34c5c051cbc795 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 May 2016 10:55:40 -0700 Subject: [PATCH] Update Owners auditing rules for multiple reviewers Summary: Ref T10939. Fixes T10181. This slightly simplifies, then documents the auditing rules, which haven't been updated for a while. In particular: - If an owner authored the change, never audit. - Examine all reviewers to determine reviewer audit status, not just the first reviewer. - Simplify some of the loading code a bit. Test Plan: - Ran `bin/repository reparse --owners --force` to trigger this stuff. - Verified that the web UI did reasonable things with resulting audits. - Read documentation. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10181, T10939 Differential Revision: https://secure.phabricator.com/D15939 --- ...habricatorRepositoryCommitOwnersWorker.php | 188 ++++++++++-------- src/docs/user/userguide/owners.diviner | 24 +++ 2 files changed, 130 insertions(+), 82 deletions(-) diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 61a615950d..3f41226b9f 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -33,108 +33,132 @@ final class PhabricatorRepositoryCommitOwnersWorker $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 ($package->isArchived()) { - // Don't trigger audits if the package is archived. - continue; - } - - 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; - } - - $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(); + if (!$affected_packages) { + return; } - } - - private function checkAuditReasons( - PhabricatorRepositoryCommit $commit, - PhabricatorOwnersPackage $package) { $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( 'commitID = %d', $commit->getID()); + $commit->attachCommitData($data); - $reasons = array(); - - if ($data->getCommitDetail('vsDiff')) { - $reasons[] = pht('Changed After Revision Was Accepted'); - } - - $commit_author_phid = $data->getCommitDetail('authorPHID'); - if (!$commit_author_phid) { - $reasons[] = pht('Commit Author Not Recognized'); - } - + $author_phid = $data->getCommitDetail('authorPHID'); $revision_id = $data->getCommitDetail('differential.revisionID'); - - $revision_author_phid = null; - $commit_reviewedby_phid = null; - if ($revision_id) { $revision = id(new DifferentialRevisionQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withIDs(array($revision_id)) + ->needReviewerStatus(true) ->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'); - } - } else { - $reasons[] = pht('No Revision Specified'); + $revision = null; } - $owners_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( - array($package->getID())); + $requests = id(new PhabricatorRepositoryAuditRequest()) + ->loadAllWhere( + 'commitPHID = %s', + $commit->getPHID()); + $requests = mpull($requests, null, 'getAuditorPHID'); - if (!($commit_author_phid && in_array($commit_author_phid, $owners_phids) || - $commit_reviewedby_phid && in_array($commit_reviewedby_phid, - $owners_phids))) { + + foreach ($affected_packages as $package) { + $request = idx($requests, $package->getPHID()); + if ($request) { + // Don't update request if it exists already. + continue; + } + + 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, + $author_phid, + $revision) { + + $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( + array( + $package->getID(), + )); + $owner_phids = array_fuse($owner_phids); + + $reasons = array(); + + if (!$author_phid) { + $reasons[] = pht('Commit Author Not Recognized'); + } else if (isset($owner_phids[$author_phid])) { + return $reasons; + } + + if (!$revision) { + $reasons[] = pht('No Revision Specified'); + return $reasons; + } + + $accepted_statuses = array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER, + ); + $accepted_statuses = array_fuse($accepted_statuses); + + $found_accept = false; + foreach ($revision->getReviewerStatus() as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + + // If this reviewer isn't a package owner, just ignore them. + if (empty($owner_phids[$reviewer_phid])) { + continue; + } + + // 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; + } + } + + if (!$found_accept) { $reasons[] = pht('Owners Not Involved'); } diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index 4aa6a29f2d..a9a52bfab8 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -98,11 +98,35 @@ The available settings are: 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 behavior, you can use Herald to +write more sophisticated rules. + + Files in Multiple Packages ==========================