Page MenuHomePhabricator
Files F13184594

D20126.id48066.diff
No OneTemporary
Actions

D20126.id48066.diff
View Options

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.

File Metadata

Mime Type
text/plain
Expires
Sat, May 11, 8:52 PM (1 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6286775
Default Alt Text
D20126.id48066.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility