Write an explicit edge for commit membership in packages

Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.

This auditor is used to power the "Commits in this package" query in Owners.

This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.

Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!

I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.

Test Plan:
  - Ran `bin/audit update-owners` with various arguments.
  - Viewed packages in web UI, saw them load the proper commits.
  - Queried by packages in Diffusion explicitly.
  - Clicked the "View All" link in Owners and got to the right search UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17264
This commit is contained in:
epriestley
2017-01-30 10:39:35 -08:00
parent 4b248e3545
commit 5e7a091737
8 changed files with 197 additions and 3 deletions

View File

@@ -0,0 +1,8 @@
<?php
final class DiffusionCommitHasPackageEdgeType
extends PhabricatorEdgeType {
const EDGECONST = 65;
}

View File

@@ -13,6 +13,7 @@ final class DiffusionCommitQuery
private $identifierMap;
private $responsiblePHIDs;
private $statuses;
private $packagePHIDs;
private $needAuditRequests;
private $auditIDs;
@@ -124,6 +125,11 @@ final class DiffusionCommitQuery
return $this;
}
public function withPackagePHIDs(array $package_phids) {
$this->packagePHIDs = $package_phids;
return $this;
}
public function withStatuses(array $statuses) {
$this->statuses = $statuses;
return $this;
@@ -498,6 +504,13 @@ final class DiffusionCommitQuery
$this->statuses);
}
if ($this->packagePHIDs !== null) {
$where[] = qsprintf(
$conn,
'package.dst IN (%Ls)',
$this->packagePHIDs);
}
return $where;
}
@@ -519,6 +532,10 @@ final class DiffusionCommitQuery
return (bool)$this->responsiblePHIDs;
}
private function shouldJoinOwners() {
return (bool)$this->packagePHIDs;
}
protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
$join = parent::buildJoinClauseParts($conn);
$audit_request = new PhabricatorRepositoryAuditRequest();
@@ -537,6 +554,15 @@ final class DiffusionCommitQuery
$audit_request->getTableName());
}
if ($this->shouldJoinOwners()) {
$join[] = qsprintf(
$conn,
'JOIN %T package ON commit.phid = package.src
AND package.type = %s',
PhabricatorEdgeConfig::TABLE_NAME_EDGE,
DiffusionCommitHasPackageEdgeType::EDGECONST);
}
return $join;
}
@@ -549,6 +575,10 @@ final class DiffusionCommitQuery
return true;
}
if ($this->shouldJoinOwners()) {
return true;
}
return parent::shouldGroupQueryResultRows();
}