diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -657,7 +657,9 @@ '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 @@ '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 --- a/src/applications/audit/application/PhabricatorAuditApplication.php +++ b/src/applications/audit/application/PhabricatorAuditApplication.php @@ -34,57 +34,4 @@ 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 --- 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 @@ 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 @@ $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 --- a/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php @@ -28,6 +28,7 @@ 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 --- a/src/applications/audit/constants/PhabricatorAuditStatusConstants.php +++ b/src/applications/audit/constants/PhabricatorAuditStatusConstants.php @@ -28,6 +28,13 @@ 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 --- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php +++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php @@ -67,9 +67,6 @@ $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 @@ ->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 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -17,23 +17,27 @@ ->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 @@ 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 @@ $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 @@ $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); diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php --- 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 @@ 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 @@ 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 @@ } 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 @@ } $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 @@ $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 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -11,22 +11,16 @@ 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 @@ 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; } + public function withAuditStatus($status) { + // TODO: Replace callers with `withStatuses()`. + return $this->withStatuses( + array( + $status, + )); + } + public function withEpochRange($min, $max) { $this->epochMin = $min; $this->epochMax = $max; @@ -251,10 +253,7 @@ } } - // 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 @@ 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 @@ } } - 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()) { - return true; - } - - if ($this->shouldJoinAudits()) { + if ($this->shouldJoinAuditor()) { 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 --- /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 --- /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 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -71,13 +71,17 @@ '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 @@ ->setViewer($request->getUser()) ->withAuditorPHIDs(array($package->getPHID())) ->needCommitData(true) - ->setLimit(100) + ->needAuditRequests(true) + ->setLimit(25) ->execute(); $view = id(new PhabricatorAuditListView()) @@ -119,13 +124,6 @@ ->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 @@ 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 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -917,15 +917,27 @@ } private function getApplicationForPHID($phid) { + static $class_map = array(); + $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; + } - $type_objects = PhabricatorPHIDType::getTypes(array($phid_type)); - $type_object = idx($type_objects, $phid_type); - if (!$type_object) { + $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 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -239,6 +239,10 @@ $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 @@ 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 @@ ->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 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -908,7 +908,7 @@ return array(); } - protected function getResultBucket(PhabricatorSavedQuery $saved) { + public function getResultBucket(PhabricatorSavedQuery $saved) { $key = $saved->getParameter('bucket'); if ($key == self::BUCKET_NONE) { return null;