From ae54af32c145fec2c4628f360f169d2f6395939f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Feb 2019 13:41:34 -0800 Subject: [PATCH] When an Owners package accepts a revision, count that as an "involved owner" for the purposes of audit Summary: Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers. If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules. Test Plan: Used `bin/repository reparse --force --owners ` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13244 Differential Revision: https://secure.phabricator.com/D20130 --- .../PhabricatorRepositoryCommitOwnersWorker.php | 13 ++++++++++--- src/docs/user/userguide/owners.diviner | 3 ++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 65434658ab..b0c60667a4 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -200,6 +200,12 @@ final class PhabricatorRepositoryCommitOwnersWorker )); $owner_phids = array_fuse($owner_phids); + // For the purposes of deciding whether the owners were involved in the + // revision or not, consider a review by the package itself to count as + // involvement. This can happen when human reviewers force-accept on + // behalf of packages they don't own but have authority over. + $owner_phids[$package->getPHID()] = $package->getPHID(); + // If the commit author is identifiable and a package owner, they're // involved. if ($author_phid) { @@ -225,13 +231,14 @@ final class PhabricatorRepositoryCommitOwnersWorker foreach ($revision->getReviewers() as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); - // If this reviewer isn't a package owner, just ignore them. + // If this reviewer isn't a package owner or the package itself, + // just ignore them. if (empty($owner_phids[$reviewer_phid])) { continue; } - // If this reviewer accepted the revision and owns the package, we've - // found an involved owner. + // If this reviewer accepted the revision and owns the package (or is + // the package), we've found an involved owner. if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) { $found_accept = true; break; diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner index df74fc6e83..11dee8941a 100644 --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -129,7 +129,8 @@ audits for commits which: - affect code owned by the package; - were not authored by a package owner; and - - were not accepted (in Differential) by a package owner. + - were not accepted (in Differential) by a package owner or the package + itself. Audits do not trigger if the package has been archived.