From a635da68d41ecd5b07011f30a558164ab0786204 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Jan 2017 08:49:36 -0800 Subject: [PATCH] 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 --- src/__phutil_library_map__.php | 4 + .../PhabricatorAuditApplication.php | 53 ------ .../conduit/AuditQueryConduitAPIMethod.php | 50 ++++-- .../PhabricatorAuditCommitStatusConstants.php | 1 + .../PhabricatorAuditStatusConstants.php | 7 + ...abricatorAuditManagementDeleteWorkflow.php | 5 +- .../query/PhabricatorCommitSearchEngine.php | 150 ++++++++--------- .../audit/view/PhabricatorAuditListView.php | 64 +++----- .../diffusion/query/DiffusionCommitQuery.php | 130 +++++---------- ...fusionCommitRequiredActionResultBucket.php | 151 ++++++++++++++++++ .../query/DiffusionCommitResultBucket.php | 33 ++++ .../PhabricatorOwnersDetailController.php | 19 +-- .../policy/filter/PhabricatorPolicyFilter.php | 22 ++- ...PhabricatorApplicationSearchController.php | 25 +++ .../PhabricatorApplicationSearchEngine.php | 2 +- 15 files changed, 422 insertions(+), 294 deletions(-) create mode 100644 src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php create mode 100644 src/applications/diffusion/query/DiffusionCommitResultBucket.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a644231520..e704ece353 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -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', diff --git a/src/applications/audit/application/PhabricatorAuditApplication.php b/src/applications/audit/application/PhabricatorAuditApplication.php index 9eb9cd0e09..7d8ad94aa7 100644 --- a/src/applications/audit/application/PhabricatorAuditApplication.php +++ b/src/applications/audit/application/PhabricatorAuditApplication.php @@ -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; - } - } diff --git a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php index 97dbec3d68..0f2fe473f5 100644 --- a/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php +++ b/src/applications/audit/conduit/AuditQueryConduitAPIMethod.php @@ -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 diff --git a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php index ca959d4aef..7c015efc43 100644 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -28,6 +28,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject { return array( self::CONCERN_RAISED, self::NEEDS_AUDIT, + self::PARTIALLY_AUDITED, ); } diff --git a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php index 5920b7f357..9685e152e9 100644 --- a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php @@ -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')); } diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php index 95fbdb430f..23583422fb 100644 --- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php @@ -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(); diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 40bab34264..c6c37208f8 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -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.')', - ); + case 'active': + $bucket_key = DiffusionCommitRequiredActionResultBucket::BUCKETKEY; - $query->setParameter('needsAuditByPHIDs', $needs_tokens); - $query->setParameter('auditStatus', $status_open); + $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); diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php index 593172006c..c359833729 100644 --- a/src/applications/audit/view/PhabricatorAuditListView.php +++ b/src/applications/audit/view/PhabricatorAuditListView.php @@ -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; } diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 4aeca4c38e..a24f8038e0 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -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,14 +113,22 @@ 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 withStatuses(array $statuses) { + $this->statuses = $statuses; return $this; } public function withAuditStatus($status) { - $this->auditStatus = $status; - return $this; + // TODO: Replace callers with `withStatuses()`. + return $this->withStatuses( + array( + $status, + )); } public function withEpochRange($min, $max) { @@ -251,10 +253,7 @@ final class DiffusionCommitQuery } } - // TODO: This should just be `needAuditRequests`, not `shouldJoinAudits()`, - // but leave that for a future diff. - - if ($this->needAuditRequests || $this->shouldJoinAudits()) { + if ($this->needAuditRequests) { $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere( 'commitPHID IN (%Ls)', mpull($commits, 'getPHID')); @@ -459,67 +458,30 @@ final class DiffusionCommitQuery if ($this->auditIDs !== null) { $where[] = qsprintf( $conn, - 'audit.id IN (%Ld)', + 'auditor.id IN (%Ld)', $this->auditIDs); } if ($this->auditorPHIDs !== null) { $where[] = qsprintf( $conn, - 'audit.auditorPHID IN (%Ls)', + 'auditor.auditorPHID IN (%Ls)', $this->auditorPHIDs); } - if ($this->needsAuditByPHIDs !== null) { + if ($this->responsiblePHIDs !== null) { $where[] = qsprintf( $conn, - 'needs.auditorPHID IN (%Ls)', - $this->needsAuditByPHIDs); + '(audit.auditorPHID IN (%Ls) OR commit.authorPHID IN (%Ls))', + $this->responsiblePHIDs, + $this->responsiblePHIDs); } - $status = $this->auditStatus; - if ($status !== null) { - switch ($status) { - case self::AUDIT_STATUS_PARTIAL: - $where[] = qsprintf( - $conn, - 'commit.auditStatus = %d', - PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED); - break; - case self::AUDIT_STATUS_ACCEPTED: - $where[] = qsprintf( - $conn, - 'commit.auditStatus = %d', - PhabricatorAuditCommitStatusConstants::FULLY_AUDITED); - break; - case self::AUDIT_STATUS_CONCERN: - $where[] = qsprintf( - $conn, - 'status.auditStatus = %s', - PhabricatorAuditStatusConstants::CONCERNED); - break; - case self::AUDIT_STATUS_OPEN: - $where[] = qsprintf( - $conn, - 'status.auditStatus in (%Ls)', - PhabricatorAuditStatusConstants::getOpenStatusConstants()); - break; - case self::AUDIT_STATUS_ANY: - break; - default: - $valid = array( - self::AUDIT_STATUS_ANY, - self::AUDIT_STATUS_OPEN, - self::AUDIT_STATUS_CONCERN, - self::AUDIT_STATUS_ACCEPTED, - self::AUDIT_STATUS_PARTIAL, - ); - throw new Exception( - pht( - "Unknown audit status '%s'! Valid statuses are: %s.", - $status, - implode(', ', $valid))); - } + if ($this->statuses !== null) { + $where[] = qsprintf( + $conn, + 'commit.auditStatus IN (%Ls)', + $this->statuses); } return $where; @@ -535,61 +497,41 @@ final class DiffusionCommitQuery } } - private function shouldJoinStatus() { - return $this->auditStatus; + private function shouldJoinAuditor() { + return ($this->auditIDs || $this->auditorPHIDs); } - private function shouldJoinAudits() { - return $this->auditIDs || $this->auditorPHIDs; - } - - private function shouldJoinNeeds() { - return $this->needsAuditByPHIDs; + private function shouldJoinAudit() { + return (bool)$this->responsiblePHIDs; } protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { $join = parent::buildJoinClauseParts($conn); $audit_request = new PhabricatorRepositoryAuditRequest(); - if ($this->shouldJoinStatus()) { + if ($this->shouldJoinAuditor()) { $join[] = qsprintf( $conn, - 'LEFT JOIN %T status ON commit.phid = status.commitPHID', + 'JOIN %T auditor ON commit.phid = auditor.commitPHID', $audit_request->getTableName()); } - if ($this->shouldJoinAudits()) { + if ($this->shouldJoinAudit()) { $join[] = qsprintf( $conn, - 'JOIN %T audit ON commit.phid = audit.commitPHID', + 'LEFT JOIN %T audit ON commit.phid = audit.commitPHID', $audit_request->getTableName()); } - if ($this->shouldJoinNeeds()) { - $join[] = qsprintf( - $conn, - 'JOIN %T needs ON commit.phid = needs.commitPHID - AND needs.auditStatus IN (%Ls)', - $audit_request->getTableName(), - array( - PhabricatorAuditStatusConstants::AUDIT_REQUESTED, - PhabricatorAuditStatusConstants::AUDIT_REQUIRED, - )); - } - return $join; } protected function shouldGroupQueryResultRows() { - if ($this->shouldJoinStatus()) { + if ($this->shouldJoinAuditor()) { return true; } - if ($this->shouldJoinAudits()) { - return true; - } - - if ($this->shouldJoinNeeds()) { + if ($this->shouldJoinAudit()) { return true; } diff --git a/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php new file mode 100644 index 0000000000..ad80cddf39 --- /dev/null +++ b/src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php @@ -0,0 +1,151 @@ +objects = $objects; + + $phids = $query->getEvaluatedParameter('responsiblePHIDs', array()); + if (!$phids) { + throw new Exception( + pht( + 'You can not bucket results by required action without '. + 'specifying "Responsible Users".')); + } + $phids = array_fuse($phids); + + $groups = array(); + + $groups[] = $this->newGroup() + ->setName(pht('Needs Attention')) + ->setNoDataString(pht('None of your commits have active concerns.')) + ->setObjects($this->filterConcernRaised($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Ready to Audit')) + ->setNoDataString(pht('No commits are waiting for you to audit them.')) + ->setObjects($this->filterShouldAudit($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Waiting on Authors')) + ->setNoDataString(pht('None of your audits are waiting on authors.')) + ->setObjects($this->filterWaitingOnAuthors($phids)); + + $groups[] = $this->newGroup() + ->setName(pht('Waiting on Auditors')) + ->setNoDataString(pht('None of your commits are waiting on audit.')) + ->setObjects($this->filterWaitingOnAuditors($phids)); + + // Because you can apply these buckets to queries which include revisions + // that have been closed, add an "Other" bucket if we still have stuff + // that didn't get filtered into any of the previous buckets. + if ($this->objects) { + $groups[] = $this->newGroup() + ->setName(pht('Other Commits')) + ->setObjects($this->objects); + } + + return $groups; + } + + private function filterConcernRaised(array $phids) { + $results = array(); + $objects = $this->objects; + + $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; + + foreach ($objects as $key => $object) { + if (empty($phids[$object->getAuthorPHID()])) { + continue; + } + + if ($object->getAuditStatus() != $status_concern) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterShouldAudit(array $phids) { + $results = array(); + $objects = $this->objects; + + $should_audit = array( + PhabricatorAuditStatusConstants::AUDIT_REQUIRED, + PhabricatorAuditStatusConstants::AUDIT_REQUESTED, + ); + $should_audit = array_fuse($should_audit); + + foreach ($objects as $key => $object) { + if (!$this->hasAuditorsWithStatus($object, $phids, $should_audit)) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterWaitingOnAuthors(array $phids) { + $results = array(); + $objects = $this->objects; + + $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; + + foreach ($objects as $key => $object) { + if ($object->getAuditStatus() != $status_concern) { + continue; + } + + if (isset($phids[$object->getAuthorPHID()])) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + + private function filterWaitingOnAuditors(array $phids) { + $results = array(); + $objects = $this->objects; + + $status_waiting = array( + PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT, + PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED, + ); + $status_waiting = array_fuse($status_waiting); + + foreach ($objects as $key => $object) { + if (empty($status_waiting[$object->getAuditStatus()])) { + continue; + } + + $results[$key] = $object; + unset($this->objects[$key]); + } + + return $results; + } + +} diff --git a/src/applications/diffusion/query/DiffusionCommitResultBucket.php b/src/applications/diffusion/query/DiffusionCommitResultBucket.php new file mode 100644 index 0000000000..28d80aa56c --- /dev/null +++ b/src/applications/diffusion/query/DiffusionCommitResultBucket.php @@ -0,0 +1,33 @@ +setAncestorClass(__CLASS__) + ->setUniqueMethod('getResultBucketKey') + ->execute(); + } + + protected function hasAuditorsWithStatus( + PhabricatorRepositoryCommit $commit, + array $phids, + array $statuses) { + + foreach ($commit->getAudits() as $audit) { + if (!isset($phids[$audit->getAuditorPHID()])) { + continue; + } + + if (!isset($statuses[$audit->getAuditStatus()])) { + continue; + } + + return true; + } + + return false; + } + +} diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index d7d249d8a5..b641290f56 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -71,13 +71,17 @@ final class PhabricatorOwnersDetailController 'auditorPHIDs' => $package->getPHID(), )); - $status_concern = DiffusionCommitQuery::AUDIT_STATUS_CONCERN; + $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; $attention_commits = id(new DiffusionCommitQuery()) ->setViewer($request->getUser()) ->withAuditorPHIDs(array($package->getPHID())) - ->withAuditStatus($status_concern) + ->withStatuses( + array( + $status_concern, + )) ->needCommitData(true) + ->needAuditRequests(true) ->setLimit(10) ->execute(); $view = id(new PhabricatorAuditListView()) @@ -100,7 +104,8 @@ final class PhabricatorOwnersDetailController ->setViewer($request->getUser()) ->withAuditorPHIDs(array($package->getPHID())) ->needCommitData(true) - ->setLimit(100) + ->needAuditRequests(true) + ->setLimit(25) ->execute(); $view = id(new PhabricatorAuditListView()) @@ -119,13 +124,6 @@ final class PhabricatorOwnersDetailController ->setText(pht('View All')), ); - $phids = array(); - foreach ($commit_views as $commit_view) { - $phids[] = $commit_view['view']->getRequiredHandlePHIDs(); - } - $phids = array_mergev($phids); - $handles = $this->loadViewerHandles($phids); - $commit_panels = array(); foreach ($commit_views as $commit_view) { $commit_panel = id(new PHUIObjectBoxView()) @@ -136,7 +134,6 @@ final class PhabricatorOwnersDetailController if (isset($commit_view['button'])) { $commit_header->addActionLink($commit_view['button']); } - $commit_view['view']->setHandles($handles); $commit_panel->setHeader($commit_header); $commit_panel->appendChild($commit_view['view']); diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index e0c223cd2c..44931085fc 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -917,15 +917,27 @@ final class PhabricatorPolicyFilter extends Phobject { } private function getApplicationForPHID($phid) { - $phid_type = phid_get_type($phid); + static $class_map = array(); - $type_objects = PhabricatorPHIDType::getTypes(array($phid_type)); - $type_object = idx($type_objects, $phid_type); - if (!$type_object) { + $phid_type = phid_get_type($phid); + if (!isset($class_map[$phid_type])) { + $type_objects = PhabricatorPHIDType::getTypes(array($phid_type)); + $type_object = idx($type_objects, $phid_type); + if (!$type_object) { + $class = false; + } else { + $class = $type_object->getPHIDTypeApplicationClass(); + } + + $class_map[$phid_type] = $class; + } + + $class = $class_map[$phid_type]; + if ($class === false) { return null; } - return $type_object->getPHIDTypeApplicationClass(); + return $class; } } diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 8937c124ea..cdd6b9335a 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -239,6 +239,10 @@ final class PhabricatorApplicationSearchController $nux_view = null; } + $is_overflowing = + $pager->willShowPagingControls() && + $engine->getResultBucket($saved_query); + $force_overheated = $request->getBool('overheated'); $is_overheated = $query->getIsOverheated() || $force_overheated; @@ -265,6 +269,11 @@ final class PhabricatorApplicationSearchController if ($list->getInfoView()) { $box->setInfoView($list->getInfoView()); } + + if ($is_overflowing) { + $box->appendChild($this->newOverflowingView()); + } + if ($list->getContent()) { $box->appendChild($list->getContent()); } @@ -545,6 +554,22 @@ final class PhabricatorApplicationSearchController ->setDropdownMenu($action_list); } + private function newOverflowingView() { + $message = pht( + 'The query matched more than one page of results. Results are '. + 'paginated before bucketing, so later pages may contain additional '. + 'results in any bucket.'); + + return id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->setFlush(true) + ->setTitle(pht('Buckets Overflowing')) + ->setErrors( + array( + $message, + )); + } + private function newOverheatedView(array $results) { if ($results) { $message = pht( diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 26a9e0f8e4..d5ca8b7a5e 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -908,7 +908,7 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { return array(); } - protected function getResultBucket(PhabricatorSavedQuery $saved) { + public function getResultBucket(PhabricatorSavedQuery $saved) { $key = $saved->getParameter('bucket'); if ($key == self::BUCKET_NONE) { return null;