diff --git a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php --- a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php +++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php @@ -4,7 +4,10 @@ extends Phobject { const AUDITING_NONE = 'none'; - const AUDITING_AUDIT = 'audit'; + const AUDITING_NO_OWNER = 'audit'; + const AUDITING_UNREVIEWED = 'unreviewed'; + const AUDITING_NO_OWNER_AND_UNREVIEWED = 'uninvolved-unreviewed'; + const AUDITING_ALL = 'all'; private $key; private $spec; @@ -86,13 +89,26 @@ '0' => '"0"', ), ), - self::AUDITING_AUDIT => array( - 'name' => pht('Audit Commits'), + self::AUDITING_UNREVIEWED => array( + 'name' => pht('Audit Unreviewed Commits'), + 'icon.icon' => 'fa-check', + ), + self::AUDITING_NO_OWNER => array( + 'name' => pht('Audit Commits With No Owner Involvement'), 'icon.icon' => 'fa-check', 'deprecated' => array( '1' => '"1"', ), ), + self::AUDITING_NO_OWNER_AND_UNREVIEWED => array( + 'name' => pht( + 'Audit Unreviewed Commits and Commits With No Owner Involvement'), + 'icon.icon' => 'fa-check', + ), + self::AUDITING_ALL => array( + 'name' => pht('Audit All Commits'), + 'icon.icon' => 'fa-check', + ), ); } 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 @@ -132,29 +132,85 @@ $author_phid, $revision) { - // Don't trigger an audit if auditing isn't enabled for the package. + $audit_uninvolved = false; + $audit_unreviewed = false; + $rule = $package->newAuditingRule(); - if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) { - return false; + switch ($rule->getKey()) { + case PhabricatorOwnersAuditRule::AUDITING_NONE: + return false; + case PhabricatorOwnersAuditRule::AUDITING_ALL: + return true; + case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER: + $audit_uninvolved = true; + break; + case PhabricatorOwnersAuditRule::AUDITING_UNREVIEWED: + $audit_unreviewed = true; + break; + case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER_AND_UNREVIEWED: + $audit_uninvolved = true; + $audit_unreviewed = true; + break; } - // Trigger an audit if we don't recognize the commit's author. - if (!$author_phid) { - return true; + // If auditing is configured to trigger on unreviewed changes, check if + // the revision was "Accepted" when it landed. If not, trigger an audit. + if ($audit_unreviewed) { + $commit_unreviewed = true; + if ($revision) { + $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED; + if ($revision->isPublished()) { + if ($revision->getProperty($was_accepted)) { + $commit_unreviewed = false; + } + } + } + + if ($commit_unreviewed) { + return true; + } } + // If auditing is configured to trigger on changes with no involved owner, + // check for an owner. If we don't find one, trigger an audit. + if ($audit_uninvolved) { + $commit_uninvolved = $this->isOwnerInvolved( + $commit, + $package, + $author_phid, + $revision); + if ($commit_uninvolved) { + return true; + } + } + + // We can't find any reason to trigger an audit for this commit. + return false; + } + + private function isOwnerInvolved( + PhabricatorRepositoryCommit $commit, + PhabricatorOwnersPackage $package, + $author_phid, + $revision) { + $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs( array( $package->getID(), )); $owner_phids = array_fuse($owner_phids); - // Don't trigger an audit if the author is a package owner. - if (isset($owner_phids[$author_phid])) { - return false; + // If the commit author is identifiable and a package owner, they're + // involved. + if ($author_phid) { + if (isset($owner_phids[$author_phid])) { + return true; + } } - // Trigger an audit of there is no corresponding revision. + // Otherwise, we need to find an owner as a reviewer. + + // If we don't have a revision, this is hopeless: no owners are involved. if (!$revision) { return true; } @@ -174,21 +230,19 @@ continue; } - // If this reviewer accepted the revision and owns the package, we're - // all clear and do not need to trigger an audit. + // If this reviewer accepted the revision and owns the package, we've + // found an involved owner. if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) { $found_accept = true; break; } } - // Don't trigger an audit if a package owner already reviewed the - // revision. if ($found_accept) { - return false; + return true; } - return true; + return false; } } 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 @@ -114,16 +114,22 @@ ======== You can automatically trigger audits on unreviewed code by configuring -**Auditing**. The available settings are: +**Auditing**. The available settings allow you to select behavior based on +these conditions: - - **Disabled**: Do not trigger audits. - - **Enabled**: Trigger audits. + - **No Owner Involvement**: Triggers an audit when the commit author is not + a package owner, and no package owner reviewed an associated revision in + Differential. + - **Unreviewed Commits**: Triggers an audit when a commit has no associated + revision in Differential, or the associated revision in Differential landed + without being "Accepted". -When enabled, audits are triggered for commits which: +For example, the **Audit Commits With No Owner Involvement** option triggers +audits for commits which: - affect code owned by the package; - were not authored by a package owner; and - - were not accepted by a package owner. + - were not accepted (in Differential) by a package owner. Audits do not trigger if the package has been archived.