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 behavior, you can use Herald to +write more sophisticated rules. + + Files in Multiple Packages ==========================