Page MenuHomePhabricator
Differential D20126 Diff 48066 src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

Changeset View

Standalone View

View Options

src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

Show First 20 Lines • Show All 126 Lines • ▼ Show 20 Linesfinal class PhabricatorRepositoryCommitOwnersWorker
} }
private function shouldTriggerAudit( private function shouldTriggerAudit(
PhabricatorRepositoryCommit $commit, PhabricatorRepositoryCommit $commit,
PhabricatorOwnersPackage $package, PhabricatorOwnersPackage $package,
$author_phid, $author_phid,
$revision) { $revision) {
// Don't trigger an audit if auditing isn't enabled for the package. $audit_uninvolved = false;
$audit_unreviewed = false;
$rule = $package->newAuditingRule(); $rule = $package->newAuditingRule();
if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) { switch ($rule->getKey()) {
case PhabricatorOwnersAuditRule::AUDITING_NONE:
return false; 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;
}
// 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;
}
}
} }
// Trigger an audit if we don't recognize the commit's author. if ($commit_unreviewed) {
if (!$author_phid) { 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(
joshuaspenceUnsubmitted
Not Done
Inline Actions

I think this is wrong? The way this is written, if the owners are involved (i.e. $this->isOwnerInvolved() returns true) then shouldTriggerAudit will return true. I ran the following script (yes it's gross because it uses reflection, I know) and it returns false before this change and true after.

<?php

require_once '/usr/local/src/phabricator/scripts/__init_script__.php';

$commit_identifier = 'rPPdd9e3b4e5f2f4e5105cf4d1b983f41c75242fa9f';
$owners_package_id = 43;

$viewer = PhabricatorUser::getOmnipotentUser();

$commit = (new DiffusionCommitQuery())
  ->setViewer($viewer)
  ->withIdentifiers([$commit_identifier])
  ->needCommitData(true)
  ->executeOne();

$owners_package = (new PhabricatorOwnersPackageQuery())
  ->setViewer($viewer)
  ->withIDs([$owners_package_id])
  ->executeOne();

$author_phid = $commit->getCommitData()->getCommitDetail('authorPHID');
$revision_id = $commit->getCommitData()->getCommitDetail('differential.revisionID');

$revision = (new DifferentialRevisionQuery())
  ->setViewer($viewer)
  ->withIDs([$revision_id])
  ->needReviewers(true)
  ->executeOne();

$class = new PhabricatorRepositoryCommitOwnersWorker([]);
$method = new ReflectionMethod($class, 'shouldTriggerAudit');
$method->setAccessible(true);
$result = $method->invoke($class, $commit, $owners_package, $author_phid, $revision);
var_dump($result);
joshuaspence: I think this is wrong? The way this is written, if the owners are involved (i.e. `$this…
$commit,
$package,
$author_phid,
$revision);
if ($commit_uninvolved) {
return true; 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( $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
array( array(
$package->getID(), $package->getID(),
)); ));
$owner_phids = array_fuse($owner_phids); $owner_phids = array_fuse($owner_phids);
// Don't trigger an audit if the author is a package owner. // If the commit author is identifiable and a package owner, they're
// involved.
if ($author_phid) {
if (isset($owner_phids[$author_phid])) { if (isset($owner_phids[$author_phid])) {
return false; return true;
} }
}
// Otherwise, we need to find an owner as a reviewer.
// Trigger an audit of there is no corresponding revision. // If we don't have a revision, this is hopeless: no owners are involved.
if (!$revision) { if (!$revision) {
return true; return true;
} }
$accepted_statuses = array( $accepted_statuses = array(
DifferentialReviewerStatus::STATUS_ACCEPTED, DifferentialReviewerStatus::STATUS_ACCEPTED,
DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER, DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER,
); );
$accepted_statuses = array_fuse($accepted_statuses); $accepted_statuses = array_fuse($accepted_statuses);
$found_accept = false; $found_accept = false;
foreach ($revision->getReviewers() as $reviewer) { foreach ($revision->getReviewers() as $reviewer) {
$reviewer_phid = $reviewer->getReviewerPHID(); $reviewer_phid = $reviewer->getReviewerPHID();
// If this reviewer isn't a package owner, just ignore them. // If this reviewer isn't a package owner, just ignore them.
if (empty($owner_phids[$reviewer_phid])) { if (empty($owner_phids[$reviewer_phid])) {
continue; continue;
} }
// If this reviewer accepted the revision and owns the package, we're // If this reviewer accepted the revision and owns the package, we've
// all clear and do not need to trigger an audit. // found an involved owner.
if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) { if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) {
$found_accept = true; $found_accept = true;
break; break;
} }
} }
// Don't trigger an audit if a package owner already reviewed the
// revision.
if ($found_accept) { if ($found_accept) {
return false; return true;
} }
return true; return false;
} }
} }
Phabricator · Built by Phacility