Commit a635da68 authored by epriestley's avatar epriestley
Browse files

Provide bucketing for commits in Audit

Summary:
Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.

This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:

  - The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
  - I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
  - When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.

Test Plan: {F2351123}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9430, T9362, T9544

Differential Revision: https://secure.phabricator.com/D17192
parent 7d3d0224
......@@ -657,7 +657,9 @@ phutil_register_library_map(array(
'DiffusionCommitRemarkupRuleTestCase' => 'applications/diffusion/remarkup/__tests__/DiffusionCommitRemarkupRuleTestCase.php',
'DiffusionCommitRepositoryHeraldField' => 'applications/diffusion/herald/DiffusionCommitRepositoryHeraldField.php',
'DiffusionCommitRepositoryProjectsHeraldField' => 'applications/diffusion/herald/DiffusionCommitRepositoryProjectsHeraldField.php',
'DiffusionCommitRequiredActionResultBucket' => 'applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php',
'DiffusionCommitResignTransaction' => 'applications/diffusion/xaction/DiffusionCommitResignTransaction.php',
'DiffusionCommitResultBucket' => 'applications/diffusion/query/DiffusionCommitResultBucket.php',
'DiffusionCommitRevertedByCommitEdgeType' => 'applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php',
'DiffusionCommitRevertsCommitEdgeType' => 'applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php',
'DiffusionCommitReviewerHeraldField' => 'applications/diffusion/herald/DiffusionCommitReviewerHeraldField.php',
......@@ -5361,7 +5363,9 @@ phutil_register_library_map(array(
'DiffusionCommitRemarkupRuleTestCase' => 'PhabricatorTestCase',
'DiffusionCommitRepositoryHeraldField' => 'DiffusionCommitHeraldField',
'DiffusionCommitRepositoryProjectsHeraldField' => 'DiffusionCommitHeraldField',
'DiffusionCommitRequiredActionResultBucket' => 'DiffusionCommitResultBucket',
'DiffusionCommitResignTransaction' => 'DiffusionCommitAuditTransaction',
'DiffusionCommitResultBucket' => 'PhabricatorSearchResultBucket',
'DiffusionCommitRevertedByCommitEdgeType' => 'PhabricatorEdgeType',
'DiffusionCommitRevertsCommitEdgeType' => 'PhabricatorEdgeType',
'DiffusionCommitReviewerHeraldField' => 'DiffusionCommitHeraldField',
......
......@@ -34,57 +34,4 @@ final class PhabricatorAuditApplication extends PhabricatorApplication {
return 0.130;
}
public function loadStatus(PhabricatorUser $user) {
$status = array();
$limit = self::MAX_STATUS_ITEMS;
$phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user);
$query = id(new DiffusionCommitQuery())
->setViewer($user)
->withAuthorPHIDs(array($user->getPHID()))
->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_CONCERN)
->setLimit($limit);
$commits = $query->execute();
$count = count($commits);
if ($count >= $limit) {
$count_str = pht('%s+ Problem Commits', new PhutilNumber($limit - 1));
} else {
$count_str = pht('%s Problem Commit(s)', new PhutilNumber($count));
}
$type = PhabricatorApplicationStatusView::TYPE_NEEDS_ATTENTION;
$status[] = id(new PhabricatorApplicationStatusView())
->setType($type)
->setText($count_str)
->setCount($count);
$query = id(new DiffusionCommitQuery())
->setViewer($user)
->withNeedsAuditByPHIDs($phids)
->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_OPEN)
->setLimit($limit);
$commits = $query->execute();
$count = count($commits);
if ($count >= $limit) {
$count_str = pht(
'%s+ Commits Awaiting Audit',
new PhutilNumber($limit - 1));
} else {
$count_str = pht(
'%s Commit(s) Awaiting Audit',
new PhutilNumber($count));
}
$type = PhabricatorApplicationStatusView::TYPE_WARNING;
$status[] = id(new PhabricatorApplicationStatusView())
->setType($type)
->setText($count_str)
->setCount($count);
return $status;
}
}
......@@ -2,6 +2,12 @@
final class AuditQueryConduitAPIMethod extends AuditConduitAPIMethod {
const AUDIT_LEGACYSTATUS_ANY = 'audit-status-any';
const AUDIT_LEGACYSTATUS_OPEN = 'audit-status-open';
const AUDIT_LEGACYSTATUS_CONCERN = 'audit-status-concern';
const AUDIT_LEGACYSTATUS_ACCEPTED = 'audit-status-accepted';
const AUDIT_LEGACYSTATUS_PARTIAL = 'audit-status-partial';
public function getAPIMethodName() {
return 'audit.query';
}
......@@ -10,13 +16,23 @@ final class AuditQueryConduitAPIMethod extends AuditConduitAPIMethod {
return pht('Query audit requests.');
}
public function getMethodStatus() {
return self::METHOD_STATUS_FROZEN;
}
public function getMethodStatusDescription() {
return pht(
'This method is frozen and will eventually be deprecated. New code '.
'should use "diffusion.commit.search" instead.');
}
protected function defineParamTypes() {
$statuses = array(
DiffusionCommitQuery::AUDIT_STATUS_ANY,
DiffusionCommitQuery::AUDIT_STATUS_OPEN,
DiffusionCommitQuery::AUDIT_STATUS_CONCERN,
DiffusionCommitQuery::AUDIT_STATUS_ACCEPTED,
DiffusionCommitQuery::AUDIT_STATUS_PARTIAL,
self::AUDIT_LEGACYSTATUS_ANY,
self::AUDIT_LEGACYSTATUS_OPEN,
self::AUDIT_LEGACYSTATUS_CONCERN,
self::AUDIT_LEGACYSTATUS_ACCEPTED,
self::AUDIT_LEGACYSTATUS_PARTIAL,
);
$status_const = $this->formatStringConstants($statuses);
......@@ -50,10 +66,26 @@ final class AuditQueryConduitAPIMethod extends AuditConduitAPIMethod {
$query->withPHIDs($commit_phids);
}
$status = $request->getValue(
'status',
DiffusionCommitQuery::AUDIT_STATUS_ANY);
$query->withAuditStatus($status);
$status_map = array(
self::AUDIT_LEGACYSTATUS_OPEN => array(
PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT,
PhabricatorAuditCommitStatusConstants::CONCERN_RAISED,
),
self::AUDIT_LEGACYSTATUS_CONCERN => array(
PhabricatorAuditCommitStatusConstants::CONCERN_RAISED,
),
self::AUDIT_LEGACYSTATUS_ACCEPTED => array(
PhabricatorAuditCommitStatusConstants::CONCERN_ACCEPTED,
),
self::AUDIT_LEGACYSTATUS_PARTIAL => array(
PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED,
),
);
$status = $request->getValue('status');
if (isset($status_map[$status])) {
$query->withStatuses($status_map[$status]);
}
// NOTE: These affect the number of commits identified, which is sort of
// reasonable but means the method may return an arbitrary number of
......
......@@ -28,6 +28,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
return array(
self::CONCERN_RAISED,
self::NEEDS_AUDIT,
self::PARTIALLY_AUDITED,
);
}
......
......@@ -28,6 +28,13 @@ final class PhabricatorAuditStatusConstants extends Phobject {
return $map;
}
public static function getActionRequiredStatusConstants() {
return array(
self::AUDIT_REQUIRED,
self::AUDIT_REQUESTED,
);
}
public static function getStatusName($code) {
return idx(self::getStatusNameMap(), $code, pht('Unknown'));
}
......
......@@ -67,9 +67,6 @@ final class PhabricatorAuditManagementDeleteWorkflow
$ids = $this->parseList($args->getArg('ids'));
$status = $args->getArg('status');
if (!$status) {
$status = DiffusionCommitQuery::AUDIT_STATUS_OPEN;
}
$min_date = $this->loadDate($args->getArg('min-commit-date'));
$max_date = $this->loadDate($args->getArg('max-commit-date'));
......@@ -85,7 +82,7 @@ final class PhabricatorAuditManagementDeleteWorkflow
->needAuditRequests(true);
if ($status) {
$query->withAuditStatus($status);
$query->withStatuses(array($status));
}
$id_map = array();
......
......@@ -17,23 +17,27 @@ final class PhabricatorCommitSearchEngine
->needCommitData(true);
}
protected function newResultBuckets() {
return DiffusionCommitResultBucket::getAllResultBuckets();
}
protected function buildQueryFromParameters(array $map) {
$query = $this->newQuery();
if ($map['needsAuditByPHIDs']) {
$query->withNeedsAuditByPHIDs($map['needsAuditByPHIDs']);
if ($map['responsiblePHIDs']) {
$query->withResponsiblePHIDs($map['responsiblePHIDs']);
}
if ($map['auditorPHIDs']) {
$query->withAuditorPHIDs($map['auditorPHIDs']);
}
if ($map['commitAuthorPHIDs']) {
$query->withAuthorPHIDs($map['commitAuthorPHIDs']);
if ($map['authorPHIDs']) {
$query->withAuthorPHIDs($map['authorPHIDs']);
}
if ($map['auditStatus']) {
$query->withAuditStatus($map['auditStatus']);
if ($map['statuses']) {
$query->withStatuses($map['statuses']);
}
if ($map['repositoryPHIDs']) {
......@@ -46,28 +50,32 @@ final class PhabricatorCommitSearchEngine
protected function buildCustomSearchFields() {
return array(
id(new PhabricatorSearchDatasourceField())
->setLabel(pht('Needs Audit By'))
->setKey('needsAuditByPHIDs')
->setAliases(array('needs', 'need'))
->setDatasource(new DiffusionAuditorFunctionDatasource()),
->setLabel(pht('Responsible Users'))
->setKey('responsiblePHIDs')
->setConduitKey('responsible')
->setAliases(array('responsible', 'responsibles', 'responsiblePHID'))
->setDatasource(new DifferentialResponsibleDatasource()),
id(new PhabricatorUsersSearchField())
->setLabel(pht('Authors'))
->setKey('authorPHIDs')
->setConduitKey('authors')
->setAliases(array('author', 'authors', 'authorPHID')),
id(new PhabricatorSearchDatasourceField())
->setLabel(pht('Auditors'))
->setKey('auditorPHIDs')
->setAliases(array('auditor', 'auditors'))
->setConduitKey('auditors')
->setAliases(array('auditor', 'auditors', 'auditorPHID'))
->setDatasource(new DiffusionAuditorFunctionDatasource()),
id(new PhabricatorUsersSearchField())
->setLabel(pht('Authors'))
->setKey('commitAuthorPHIDs')
->setAliases(array('author', 'authors')),
id(new PhabricatorSearchSelectField())
id(new PhabricatorSearchCheckboxesField())
->setLabel(pht('Audit Status'))
->setKey('auditStatus')
->setKey('statuses')
->setAliases(array('status'))
->setOptions($this->getAuditStatusOptions()),
->setOptions(PhabricatorAuditCommitStatusConstants::getStatusNameMap()),
id(new PhabricatorSearchDatasourceField())
->setLabel(pht('Repositories'))
->setKey('repositoryPHIDs')
->setAliases(array('repository', 'repositories'))
->setConduitKey('repositories')
->setAliases(array('repository', 'repositories', 'repositoryPHID'))
->setDatasource(new DiffusionRepositoryDatasource()),
);
}
......@@ -80,14 +88,9 @@ final class PhabricatorCommitSearchEngine
$names = array();
if ($this->requireViewer()->isLoggedIn()) {
$names['need'] = pht('Needs Audit');
$names['problem'] = pht('Problem Commits');
}
$names['open'] = pht('Open Audits');
if ($this->requireViewer()->isLoggedIn()) {
$names['authored'] = pht('Authored Commits');
$names['active'] = pht('Active Audits');
$names['authored'] = pht('Authored');
$names['audited'] = pht('Audited');
}
$names['all'] = pht('All Commits');
......@@ -101,76 +104,75 @@ final class PhabricatorCommitSearchEngine
$viewer = $this->requireViewer();
$viewer_phid = $viewer->getPHID();
$status_open = DiffusionCommitQuery::AUDIT_STATUS_OPEN;
switch ($query_key) {
case 'all':
return $query;
case 'open':
$query->setParameter('auditStatus', $status_open);
return $query;
case 'need':
$needs_tokens = array(
$viewer_phid,
'projects('.$viewer_phid.')',
'packages('.$viewer_phid.')',
);
$query->setParameter('needsAuditByPHIDs', $needs_tokens);
$query->setParameter('auditStatus', $status_open);
case 'active':
$bucket_key = DiffusionCommitRequiredActionResultBucket::BUCKETKEY;
$open = PhabricatorAuditCommitStatusConstants::getOpenStatusConstants();
$query
->setParameter('responsiblePHIDs', array($viewer_phid))
->setParameter('statuses', $open)
->setParameter('bucket', $bucket_key);
return $query;
case 'authored':
$query->setParameter('commitAuthorPHIDs', array($viewer->getPHID()));
$query
->setParameter('authorPHIDs', array($viewer_phid));
return $query;
case 'problem':
$query->setParameter('commitAuthorPHIDs', array($viewer->getPHID()));
$query->setParameter(
'auditStatus',
DiffusionCommitQuery::AUDIT_STATUS_CONCERN);
case 'audited':
$query
->setParameter('auditorPHIDs', array($viewer_phid));
return $query;
}
return parent::buildSavedQueryFromBuiltin($query_key);
}
private function getAuditStatusOptions() {
return array(
DiffusionCommitQuery::AUDIT_STATUS_ANY => pht('Any'),
DiffusionCommitQuery::AUDIT_STATUS_OPEN => pht('Open'),
DiffusionCommitQuery::AUDIT_STATUS_CONCERN => pht('Concern Raised'),
DiffusionCommitQuery::AUDIT_STATUS_ACCEPTED => pht('Accepted'),
DiffusionCommitQuery::AUDIT_STATUS_PARTIAL => pht('Partially Audited'),
);
}
protected function renderResultList(
array $commits,
PhabricatorSavedQuery $query,
array $handles) {
assert_instances_of($commits, 'PhabricatorRepositoryCommit');
$viewer = $this->requireViewer();
$nodata = pht('No matching audits.');
$view = id(new PhabricatorAuditListView())
->setUser($viewer)
->setCommits($commits)
->setAuthorityPHIDs(
PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($viewer))
->setNoDataString($nodata);
$phids = $view->getRequiredHandlePHIDs();
if ($phids) {
$handles = id(new PhabricatorHandleQuery())
->setViewer($viewer)
->withPHIDs($phids)
->execute();
$bucket = $this->getResultBucket($query);
$authority_phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser(
$viewer);
$template = id(new PhabricatorAuditListView())
->setViewer($viewer)
->setAuthorityPHIDs($authority_phids);
$views = array();
if ($bucket) {
$bucket->setViewer($viewer);
try {
$groups = $bucket->newResultGroups($query, $commits);
foreach ($groups as $group) {
$views[] = id(clone $template)
->setHeader($group->getName())
->setNoDataString($group->getNoDataString())
->setCommits($group->getObjects());
}
} catch (Exception $ex) {
$this->addError($ex->getMessage());
}
} else {
$handles = array();
$views[] = id(clone $template)
->setCommits($commits)
->setNoDataString(pht('No matching commits.'));
}
$view->setHandles($handles);
$list = $view->buildList();
if (count($views) == 1) {
$list = head($views)->buildList();
} else {
$list = $views;
}
$result = new PhabricatorApplicationSearchResultView();
$result->setContent($list);
......
......@@ -3,18 +3,11 @@
final class PhabricatorAuditListView extends AphrontView {
private $commits;
private $handles;
private $authorityPHIDs = array();
private $header;
private $noDataString;
private $highlightedAudits;
public function setHandles(array $handles) {
assert_instances_of($handles, 'PhabricatorObjectHandle');
$this->handles = $handles;
return $this;
}
public function setAuthorityPHIDs(array $phids) {
$this->authorityPHIDs = $phids;
return $this;
......@@ -29,6 +22,15 @@ final class PhabricatorAuditListView extends AphrontView {
return $this->noDataString;
}
public function setHeader($header) {
$this->header = $header;
return $this;
}
public function getHeader() {
return $this->header;
}
/**
* These commits should have both commit data and audit requests attached.
*/
......@@ -42,28 +44,6 @@ final class PhabricatorAuditListView extends AphrontView {
return $this->commits;
}
public function getRequiredHandlePHIDs() {
$phids = array();
$commits = $this->getCommits();
foreach ($commits as $commit) {
$phids[$commit->getPHID()] = true;
$phids[$commit->getAuthorPHID()] = true;
$audits = $commit->getAudits();
foreach ($audits as $audit) {
$phids[$audit->getAuditorPHID()] = true;
}
}
return array_keys($phids);
}
private function getHandle($phid) {
$handle = idx($this->handles, $phid);
if (!$handle) {
throw new Exception(pht("No handle for '%s'!", $phid));
}
return $handle;
}
private function getCommitDescription($phid) {
if ($this->commits === null) {
return pht('(Unknown Commit)');
......@@ -96,34 +76,28 @@ final class PhabricatorAuditListView extends AphrontView {
}
public function buildList() {
$user = $this->getUser();
if (!$user) {
throw new Exception(
pht(
'You must %s before %s!',
'setUser()',
__FUNCTION__.'()'));
}
$viewer = $this->getViewer();
$rowc = array();
$handles = $viewer->loadHandles(mpull($this->commits, 'getPHID'));
$list = new PHUIObjectItemListView();
foreach ($this->commits as $commit) {
$commit_phid = $commit->getPHID();
$commit_handle = $this->getHandle($commit_phid);
$commit_handle = $handles[$commit_phid];
$committed = null;
$commit_name = $commit_handle->getName();
$commit_link = $commit_handle->getURI();
$commit_desc = $this->getCommitDescription($commit_phid);
$committed = phabricator_datetime($commit->getEpoch(), $user);
$committed = phabricator_datetime($commit->getEpoch(), $viewer);
$audits = mpull($commit->getAudits(), null, 'getAuditorPHID');
$auditors = array();
$reasons = array();
foreach ($audits as $audit) {
$auditor_phid = $audit->getAuditorPHID();
$auditors[$auditor_phid] =
$this->getHandle($auditor_phid)->renderLink();
$auditors[$auditor_phid] = $viewer->renderHandle($auditor_phid);
}
$auditors = phutil_implode_html(', ', $auditors);
......@@ -151,7 +125,7 @@ final class PhabricatorAuditListView extends AphrontView {
}
$author_phid = $commit->getAuthorPHID();
if ($author_phid) {
$author_name = $this->getHandle($author_phid)->renderLink();
$author_name = $viewer->renderHandle($author_phid);
} else {
$author_name = $commit->getCommitData()->getAuthorName();
}
......@@ -180,6 +154,10 @@ final class PhabricatorAuditListView extends AphrontView {
$list->setNoDataString($this->noDataString);
}
if ($this->header) {
$list->setHeader($this->header);
}
return $list;
}
......
......@@ -11,22 +11,16 @@ final class DiffusionCommitQuery
private $repositoryIDs;
private $repositoryPHIDs;
private $identifierMap;
private $responsiblePHIDs;
private $statuses;
private $needAuditRequests;
private $auditIDs;
private $auditorPHIDs;
private $needsAuditByPHIDs;
private $auditStatus;
private $epochMin;
private $epochMax;
private $importing;
const AUDIT_STATUS_ANY = 'audit-status-any';
const AUDIT_STATUS_OPEN = 'audit-status-open';
const AUDIT_STATUS_CONCERN = 'audit-status-concern';
const AUDIT_STATUS_ACCEPTED = 'audit-status-accepted';
const AUDIT_STATUS_PARTIAL = 'audit-status-partial';
private $needCommitData;
public function withIDs(array $ids) {
......@@ -119,16 +113,24 @@ final class DiffusionCommitQuery
return $this;
}
public function withNeedsAuditByPHIDs(array $needs_phids) {
$this->needsAuditByPHIDs = $needs_phids;
public function withResponsiblePHIDs(array $responsible_phids) {
$this->responsiblePHIDs = $responsible_phids;
return $this;
}
public function withAuditStatus($status) {
$this->auditStatus = $status;
public function withStatuses(array $statuses) {
$this->statuses = $statuses;
return $this;
}