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 @@ -1189,8 +1189,6 @@ 'PhabricatorAuditAddCommentController' => 'applications/audit/controller/PhabricatorAuditAddCommentController.php', 'PhabricatorAuditComment' => 'applications/audit/storage/PhabricatorAuditComment.php', 'PhabricatorAuditCommentEditor' => 'applications/audit/editor/PhabricatorAuditCommentEditor.php', - 'PhabricatorAuditCommitListView' => 'applications/audit/view/PhabricatorAuditCommitListView.php', - 'PhabricatorAuditCommitQuery' => 'applications/audit/query/PhabricatorAuditCommitQuery.php', 'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/PhabricatorAuditCommitStatusConstants.php', 'PhabricatorAuditController' => 'applications/audit/controller/PhabricatorAuditController.php', 'PhabricatorAuditDAO' => 'applications/audit/storage/PhabricatorAuditDAO.php', @@ -1201,7 +1199,6 @@ 'PhabricatorAuditManagementDeleteWorkflow' => 'applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php', 'PhabricatorAuditManagementWorkflow' => 'applications/audit/management/PhabricatorAuditManagementWorkflow.php', 'PhabricatorAuditPreviewController' => 'applications/audit/controller/PhabricatorAuditPreviewController.php', - 'PhabricatorAuditQuery' => 'applications/audit/query/PhabricatorAuditQuery.php', 'PhabricatorAuditReplyHandler' => 'applications/audit/mail/PhabricatorAuditReplyHandler.php', 'PhabricatorAuditStatusConstants' => 'applications/audit/constants/PhabricatorAuditStatusConstants.php', 'PhabricatorAuthAccountView' => 'applications/auth/view/PhabricatorAuthAccountView.php', @@ -1312,6 +1309,7 @@ 'PhabricatorChatLogQuery' => 'applications/chatlog/query/PhabricatorChatLogQuery.php', 'PhabricatorCommitBranchesField' => 'applications/repository/customfield/PhabricatorCommitBranchesField.php', 'PhabricatorCommitCustomField' => 'applications/repository/customfield/PhabricatorCommitCustomField.php', + 'PhabricatorCommitSearchEngine' => 'applications/audit/query/PhabricatorCommitSearchEngine.php', 'PhabricatorCommitTagsField' => 'applications/repository/customfield/PhabricatorCommitTagsField.php', 'PhabricatorCommonPasswords' => 'applications/auth/constants/PhabricatorCommonPasswords.php', 'PhabricatorConduitAPIController' => 'applications/conduit/controller/PhabricatorConduitAPIController.php', @@ -3926,7 +3924,6 @@ 1 => 'PhabricatorMarkupInterface', ), 'PhabricatorAuditCommentEditor' => 'PhabricatorEditor', - 'PhabricatorAuditCommitListView' => 'AphrontView', 'PhabricatorAuditController' => 'PhabricatorController', 'PhabricatorAuditDAO' => 'PhabricatorLiskDAO', 'PhabricatorAuditInlineComment' => @@ -3934,7 +3931,11 @@ 0 => 'PhabricatorAuditDAO', 1 => 'PhabricatorInlineCommentInterface', ), - 'PhabricatorAuditListController' => 'PhabricatorAuditController', + 'PhabricatorAuditListController' => + array( + 0 => 'PhabricatorAuditController', + 1 => 'PhabricatorApplicationSearchResultsControllerInterface', + ), 'PhabricatorAuditListView' => 'AphrontView', 'PhabricatorAuditMailReceiver' => 'PhabricatorObjectMailReceiver', 'PhabricatorAuditManagementDeleteWorkflow' => 'PhabricatorAuditManagementWorkflow', @@ -4066,6 +4067,7 @@ 'PhabricatorChatLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorCommitBranchesField' => 'PhabricatorCommitCustomField', 'PhabricatorCommitCustomField' => 'PhabricatorCustomField', + 'PhabricatorCommitSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorCommitTagsField' => 'PhabricatorCommitCustomField', 'PhabricatorCommonPasswords' => 'Phobject', 'PhabricatorConduitAPIController' => 'PhabricatorConduitController', @@ -4795,7 +4797,11 @@ 'PhabricatorRepositoryArcanistProjectDeleteController' => 'PhabricatorRepositoryController', 'PhabricatorRepositoryArcanistProjectEditController' => 'PhabricatorRepositoryController', 'PhabricatorRepositoryArcanistProjectQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', - 'PhabricatorRepositoryAuditRequest' => 'PhabricatorRepositoryDAO', + 'PhabricatorRepositoryAuditRequest' => + array( + 0 => 'PhabricatorRepositoryDAO', + 1 => 'PhabricatorPolicyInterface', + ), 'PhabricatorRepositoryBranch' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryCommit' => array( diff --git a/src/applications/audit/application/PhabricatorApplicationAudit.php b/src/applications/audit/application/PhabricatorApplicationAudit.php --- a/src/applications/audit/application/PhabricatorApplicationAudit.php +++ b/src/applications/audit/application/PhabricatorApplicationAudit.php @@ -27,9 +27,7 @@ public function getRoutes() { return array( '/audit/' => array( - '' => 'PhabricatorAuditListController', - 'view/(?P[^/]+)/(?:(?P[^/]+)/)?' - => 'PhabricatorAuditListController', + '(?:query/(?P[^/]+)/)?' => 'PhabricatorAuditListController', 'addcomment/' => 'PhabricatorAuditAddCommentController', 'preview/(?P[1-9]\d*)/' => 'PhabricatorAuditPreviewController', ), @@ -49,10 +47,11 @@ $phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user); - $commits = id(new PhabricatorAuditCommitQuery()) - ->withAuthorPHIDs($phids) - ->withStatus(PhabricatorAuditCommitQuery::STATUS_CONCERN) - ->execute(); + $query = id(new DiffusionCommitQuery()) + ->setViewer($user) + ->withAuthorPHIDs(array($user->getPHID())) + ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_CONCERN); + $commits = $query->execute(); $count = count($commits); $type = PhabricatorApplicationStatusView::TYPE_NEEDS_ATTENTION; @@ -61,13 +60,14 @@ ->setText(pht('%d Problem Commit(s)', $count)) ->setCount($count); - $audits = id(new PhabricatorAuditQuery()) + $query = id(new DiffusionCommitQuery()) + ->setViewer($user) ->withAuditorPHIDs($phids) - ->withStatus(PhabricatorAuditQuery::STATUS_OPEN) - ->withAwaitingUser($user) - ->execute(); + ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_OPEN) + ->withAuditAwaitingUser($user); + $commits = $query->execute(); - $count = count($audits); + $count = count($commits); $type = PhabricatorApplicationStatusView::TYPE_WARNING; $status[] = id(new PhabricatorApplicationStatusView()) ->setType($type) diff --git a/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php b/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php --- a/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php +++ b/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php @@ -31,7 +31,8 @@ protected function execute(ConduitAPIRequest $request) { - $query = new PhabricatorAuditQuery(); + $query = id(new DiffusionCommitQuery()) + ->setViewer($request->getUser()); $auditor_phids = $request->getValue('auditorPHIDs', array()); if ($auditor_phids) { @@ -40,26 +41,31 @@ $commit_phids = $request->getValue('commitPHIDs', array()); if ($commit_phids) { - $query->withCommitPHIDs($commit_phids); + $query->withPHIDs($commit_phids); } - $status = $request->getValue('status', PhabricatorAuditQuery::STATUS_ANY); - $query->withStatus($status); + $status = $request->getValue( + 'status', + DiffusionCommitQuery::AUDIT_STATUS_ANY); + $query->withAuditStatus($status); $query->setOffset($request->getValue('offset', 0)); $query->setLimit($request->getValue('limit', 100)); - $requests = $query->execute(); + $commits = $query->execute(); $results = array(); - foreach ($requests as $request) { - $results[] = array( - 'id' => $request->getID(), - 'commitPHID' => $request->getCommitPHID(), - 'auditorPHID' => $request->getAuditorPHID(), - 'reasons' => $request->getAuditReasons(), - 'status' => $request->getAuditStatus(), - ); + foreach ($commits as $commit) { + $requests = $commit->getAudits(); + foreach ($requests as $request) { + $results[] = array( + 'id' => $request->getID(), + 'commitPHID' => $request->getCommitPHID(), + 'auditorPHID' => $request->getAuditorPHID(), + 'reasons' => $request->getAuditReasons(), + 'status' => $request->getAuditStatus(), + ); + } } return $results; diff --git a/src/applications/audit/controller/PhabricatorAuditController.php b/src/applications/audit/controller/PhabricatorAuditController.php --- a/src/applications/audit/controller/PhabricatorAuditController.php +++ b/src/applications/audit/controller/PhabricatorAuditController.php @@ -2,28 +2,18 @@ abstract class PhabricatorAuditController extends PhabricatorController { - public $filter; - public function buildSideNavView() { - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('/audit/view/')); - $nav->addLabel(pht('Active')); - $nav->addFilter('active', pht('Need Attention')); + $user = $this->getRequest()->getUser(); - $nav->addLabel(pht('Audits')); - $nav->addFilter('audits', pht('All')); - $nav->addFilter('user', pht('By User')); - $nav->addFilter('project', pht('By Project')); - $nav->addFilter('package', pht('By Package')); - $nav->addFilter('repository', pht('By Repository')); + $nav = new AphrontSideNavFilterView(); + $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); - $nav->addLabel(pht('Commits')); - $nav->addFilter('commits', pht('All')); - $nav->addFilter('author', pht('By Author')); - $nav->addFilter('packagecommits', pht('By Package')); + id(new PhabricatorCommitSearchEngine()) + ->setViewer($user) + ->addNavigationItems($nav->getMenu()); - $this->filter = $nav->selectFilter($this->filter, 'active'); + $nav->selectFilter(null); return $nav; } @@ -32,18 +22,4 @@ return $this->buildSideNavView()->getMenu(); } - public function buildStandardPageResponse($view, array $data) { - - $page = $this->buildStandardPageView(); - - $page->setApplicationName(pht('Audit')); - $page->setBaseURI('/audit/'); - $page->setTitle(idx($data, 'title')); - $page->setGlyph("\xE2\x9C\x8D"); - $page->appendChild($view); - - $response = new AphrontWebpageResponse(); - return $response->setContent($page->render()); - - } } diff --git a/src/applications/audit/controller/PhabricatorAuditListController.php b/src/applications/audit/controller/PhabricatorAuditListController.php --- a/src/applications/audit/controller/PhabricatorAuditListController.php +++ b/src/applications/audit/controller/PhabricatorAuditListController.php @@ -1,483 +1,48 @@ filter = idx($data, 'filter'); - $this->name = idx($data, 'name'); - } - - public function processRequest() { - $request = $this->getRequest(); - $nav = $this->buildSideNavView(); - - if ($request->isFormPost()) { - // If the list filter is POST'ed, redirect to GET so the page can be - // bookmarked. - $uri = $request->getRequestURI(); - $phid = head($request->getArr('set_phid')); - $user = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - $phid); - - $uri = $request->getRequestURI(); - if ($user) { - $username = phutil_escape_uri($user->getUsername()); - $uri = '/audit/view/'.$this->filter.'/'.$username.'/'; - } else if ($phid) { - $uri = $request->getRequestURI(); - $uri = $uri->alter('phid', $phid); - } - - return id(new AphrontRedirectResponse())->setURI($uri); - } - - $this->filterStatus = $request->getStr('status', 'all'); - - $handle = $this->loadHandle(); - - $nav->appendChild($this->buildListFilters($handle)); - - - $title = null; - $message = null; - - if (!$handle) { - switch ($this->filter) { - case 'project': - $title = pht('Choose A Project'); - $message = pht('Choose a project to view audits for.'); - break; - case 'repository': - $title = pht('Choose A Repository'); - $message = pht('Choose a repository to view audits for.'); - break; - case 'package': - case 'packagecommits': - $title = pht('Choose a Package'); - $message = pht('Choose a package to view audits for.'); - break; - } - } - - if (!$message) { - $nav->appendChild($this->buildViews($handle)); - } else { - $panel = id(new AphrontErrorView()) - ->setSeverity(AphrontErrorView::SEVERITY_NODATA) - ->setTitle($title) - ->appendChild($message); - $nav->appendChild($panel); - } - - return $this->buildApplicationPage( - $nav, - array( - 'title' => pht('Audits'), - 'device' => true, - )); - } - - private function buildListFilters(PhabricatorObjectHandle $handle = null) { - $request = $this->getRequest(); - $user = $request->getUser(); - - $form = new AphrontFormView(); - $form->setUser($user); - - $show_status = false; - $show_user = false; - $show_project = false; - $show_package = false; - $show_repository = false; - - switch ($this->filter) { - case 'audits': - case 'commits': - $show_status = true; - break; - case 'active': - $show_user = true; - break; - case 'author': - case 'user': - $show_user = true; - $show_status = true; - break; - case 'project': - $show_project = true; - $show_status = true; - break; - case 'repository': - $show_repository = true; - $show_status = true; - break; - case 'package': - case 'packagecommits': - $show_package = true; - $show_status = true; - break; - } - - if ($show_user || $show_project || $show_package || $show_repository) { - if ($show_user) { - $uri = '/typeahead/common/users/'; - $label = pht('User'); - } else if ($show_project) { - $uri = '/typeahead/common/projects/'; - $label = pht('Project'); - } else if ($show_package) { - $uri = '/typeahead/common/packages/'; - $label = pht('Package'); - } else if ($show_repository) { - $uri = '/typeahead/common/repositories/'; - $label = pht('Repository'); - } - - $tok_value = null; - if ($handle) { - $tok_value = array($handle); - } - - $form->appendChild( - id(new AphrontFormTokenizerControl()) - ->setName('set_phid') - ->setLabel($label) - ->setLimit(1) - ->setDatasource($uri) - ->setValue($tok_value)); - } - - if ($show_status) { - $form->appendChild( - id(new AphrontFormToggleButtonsControl()) - ->setName('status') - ->setLabel(pht('Status')) - ->setBaseURI($request->getRequestURI(), 'status') - ->setValue($this->filterStatus) - ->setButtons( - array( - 'all' => pht('All'), - 'open' => pht('Open'), - 'concern' => pht('Concern Raised'), - ))); - } - - $form->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Filter Audits'))); - - $view = new AphrontListFilterView(); - $view->appendChild($form); - return $view; - } - - private function loadHandle() { - $request = $this->getRequest(); - - $default = null; - switch ($this->filter) { - case 'user': - case 'active': - case 'author': - $default = $request->getUser()->getPHID(); - if ($this->name) { - $user = id(new PhabricatorUser())->loadOneWhere( - 'username = %s', - $this->name); - if ($user) { - $default = $user->getPHID(); - } - } - break; - } - - $phid = $request->getStr('phid', $default); - if (!$phid) { - return null; - } - - $phids = array($phid); - $handles = $this->loadViewerHandles($phids); - $handle = $handles[$phid]; - - $this->validateHandle($handle); - return $handle; - } - - private function validateHandle(PhabricatorObjectHandle $handle) { - $type = $handle->getType(); - - switch ($this->filter) { - case 'active': - case 'user': - case 'author': - if ($type !== PhabricatorPeoplePHIDTypeUser::TYPECONST) { - throw new Exception("PHID must be a user PHID!"); - } - break; - case 'package': - case 'packagecommits': - if ($type !== PhabricatorOwnersPHIDTypePackage::TYPECONST) { - throw new Exception("PHID must be a package PHID!"); - } - break; - case 'project': - if ($type !== PhabricatorProjectPHIDTypeProject::TYPECONST) { - throw new Exception("PHID must be a project PHID!"); - } - break; - case 'repository': - if ($type !== PhabricatorRepositoryPHIDTypeRepository::TYPECONST) { - throw new Exception("PHID must be a repository PHID!"); - } - break; - case 'audits': - case 'commits': - break; - default: - throw new Exception("Unknown filter '{$this->filter}'!"); - } + public function shouldAllowPublic() { + return true; } - private function buildViews(PhabricatorObjectHandle $handle = null) { - $views = array(); - switch ($this->filter) { - case 'active': - $views[] = $this->buildCommitView($handle); - $views[] = $this->buildAuditView($handle); - break; - case 'audits': - case 'user': - case 'package': - case 'project': - case 'repository': - $views[] = $this->buildAuditView($handle); - break; - case 'commits': - case 'packagecommits': - case 'author': - $views[] = $this->buildCommitView($handle); - break; - } - return $views; + public function willProcessRequest(array $data) { + $this->queryKey = idx($data, 'queryKey'); } - private function buildAuditView(PhabricatorObjectHandle $handle = null) { + public function processRequest() { $request = $this->getRequest(); + $controller = id(new PhabricatorApplicationSearchController($request)) + ->setQueryKey($this->queryKey) + ->setSearchEngine(new PhabricatorCommitSearchEngine()) + ->setNavigation($this->buildSideNavView()); - $query = new PhabricatorAuditQuery(); - - $pager = new AphrontPagerView(); - $pager->setURI($request->getRequestURI(), 'offset'); - $pager->setOffset($request->getInt('offset')); - - $query->setOffset($pager->getOffset()); - $query->setLimit($pager->getPageSize() + 1); - - $awaiting = null; - - $phids = null; - $repository_phids = null; - switch ($this->filter) { - case 'user': - case 'active': - $obj = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - $handle->getPHID()); - if (!$obj) { - throw new Exception("Invalid user!"); - } - $phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($obj); - $awaiting = $obj; - break; - case 'project': - case 'package': - $phids = array($handle->getPHID()); - break; - case 'repository': - $repository_phids = array($handle->getPHID()); - break; - case 'audits'; - break; - default: - throw new Exception("Unknown filter!"); - } - - if ($phids) { - $query->withAuditorPHIDs($phids); - } - - if ($repository_phids) { - $query->withRepositoryPHIDs($repository_phids); - } - - if ($awaiting) { - $query->withAwaitingUser($awaiting); - } - - switch ($this->filter) { - case 'active': - $query->withStatus(PhabricatorAuditQuery::STATUS_OPEN); - break; - default: - switch ($this->filterStatus) { - case 'open': - $query->withStatus(PhabricatorAuditQuery::STATUS_OPEN); - break; - case 'concern': - $query->withStatus(PhabricatorAuditQuery::STATUS_CONCERN); - break; - } - break; - } - - if ($handle) { - $handle_name = $handle->getFullName(); - } else { - $handle_name = null; - } - - switch ($this->filter) { - case 'active': - $header = pht('Required Audits'); - $nodata = pht('No commits require your audit.'); - break; - case 'user': - $header = pht("Audits for %s", $handle_name); - $nodata = pht("No matching audits by %s.", $handle_name); - break; - case 'audits': - $header = pht('Audits'); - $nodata = pht('No matching audits.'); - break; - case 'project': - $header = pht("Audits in Project %s", $handle_name); - $nodata = pht("No matching audits in project %s.", $handle_name); - break; - case 'package': - $header = pht("Audits for Package %s", $handle_name); - $nodata = pht("No matching audits in package %s.", $handle_name); - break; - case 'repository': - $header = pht("Audits in Repository %s", $handle_name); - $nodata = pht("No matching audits in repository %s.", $handle_name); - break; - } - - $query->needCommitData(true); - - $audits = $query->execute(); - $audits = $pager->sliceResults($audits); - - $view = new PhabricatorAuditListView(); - $view->setAudits($audits); - $view->setCommits($query->getCommits()); - $view->setUser($request->getUser()); - $view->setNoDataString($nodata); - - $phids = $view->getRequiredHandlePHIDs(); - $handles = $this->loadViewerHandles($phids); - $view->setHandles($handles); - - $panel = new AphrontPanelView(); - $panel->setHeader($header); - $panel->appendChild($view); - $panel->setNoBackground(); - - $panel->appendChild($pager); - - return $panel; + return $this->delegateToController($controller); } - private function buildCommitView(PhabricatorObjectHandle $handle = null) { - $request = $this->getRequest(); - - $query = new PhabricatorAuditCommitQuery(); - $query->needCommitData(true); - $query->needAudits(true); - - $pager = new AphrontPagerView(); - $pager->setURI($request->getRequestURI(), 'offset'); - $pager->setOffset($request->getInt('offset')); - - $query->setOffset($pager->getOffset()); - $query->setLimit($pager->getPageSize() + 1); + public function renderResultsList( + array $commits, + PhabricatorSavedQuery $query) { + assert_instances_of($commits, 'PhabricatorRepositoryCommit'); - switch ($this->filter) { - case 'active': - case 'author': - $query->withAuthorPHIDs(array($handle->getPHID())); - break; - case 'packagecommits': - $query->withPackagePHIDs(array($handle->getPHID())); - break; - } - - switch ($this->filter) { - case 'active': - $query->withStatus(PhabricatorAuditCommitQuery::STATUS_CONCERN); - break; - default: - switch ($this->filterStatus) { - case 'open': - $query->withStatus(PhabricatorAuditCommitQuery::STATUS_OPEN); - break; - case 'concern': - $query->withStatus(PhabricatorAuditCommitQuery::STATUS_CONCERN); - break; - } - break; - } - - if ($handle) { - $handle_name = $handle->getName(); - } else { - $handle_name = null; - } - - switch ($this->filter) { - case 'active': - $header = pht('Problem Commits'); - $nodata = pht('None of your commits have open concerns.'); - break; - case 'author': - $header = pht("Commits by %s", $handle_name); - $nodata = pht("No matching commits by %s.", $handle_name); - break; - case 'commits': - $header = pht("Commits"); - $nodata = pht("No matching commits."); - break; - case 'packagecommits': - $header = pht("Commits in Package %s", $handle_name); - $nodata = pht("No matching commits in package %s.", $handle_name); - break; - } - - $commits = $query->execute(); - $commits = $pager->sliceResults($commits); - - $view = new PhabricatorAuditCommitListView(); - $view->setUser($request->getUser()); - $view->setCommits($commits); - $view->setNoDataString($nodata); + $viewer = $this->getRequest()->getUser(); + $nodata = pht('No matching audits.'); + $view = id(new PhabricatorAuditListView()) + ->setUser($viewer) + ->setCommits($commits) + ->setAuthorityPHIDs( + PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($viewer)) + ->setNoDataString($nodata); $phids = $view->getRequiredHandlePHIDs(); $handles = $this->loadViewerHandles($phids); $view->setHandles($handles); - - $panel = new AphrontPanelView(); - $panel->setHeader($header); - $panel->appendChild($view); - $panel->setNoBackground(); - - $panel->appendChild($pager); - - return $panel; + return $view->buildList(); } - } 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 @@ -65,7 +65,7 @@ $status = $args->getArg('status'); if (!$status) { - $status = PhabricatorAuditQuery::STATUS_OPEN; + $status = DiffusionCommitQuery::AUDIT_STATUS_OPEN; } $min_date = $this->loadDate($args->getArg('min-commit-date')); @@ -77,19 +77,20 @@ $is_dry_run = $args->getArg('dry-run'); - $query = id(new PhabricatorAuditQuery()) - ->needCommits(true); + $query = id(new DiffusionCommitQuery()) + ->setViewer($this->getViewer()) + ->needAuditRequests(true); if ($status) { - $query->withStatus($status); + $query->withAuditStatus($status); } if ($ids) { - $query->withIDs($ids); + $query->withAuditIDs($ids); } if ($repos) { - $query->withRepositoryPHIDs(mpull($repos, 'getPHID')); + $query->withRepositoryIDs(mpull($repos, 'getID')); } if ($users) { @@ -97,40 +98,28 @@ } if ($commits) { - $query->withCommitPHIDs(mpull($commits, 'getPHID')); - } - - $audits = $query->execute(); - $commits = $query->getCommits(); - - if ($commits) { - // TODO: AuditQuery is currently not policy-aware and uses an old query - // to load commits. Load them in the modern way to get repositories. - // Remove this after modernizing PhabricatorAuditQuery. - $commits = id(new DiffusionCommitQuery()) - ->setViewer($viewer) - ->withPHIDs(mpull($commits, 'getPHID')) - ->execute(); - $commits = mpull($commits, null, 'getPHID'); + $query->withPHIDs(mpull($commits, 'getPHID')); } - foreach ($audits as $key => $audit) { - $commit = idx($commits, $audit->getCommitPHID()); - if (!$commit) { - unset($audits[$key]); - continue; - } - - if ($min_date && $commit->getEpoch() < $min_date) { - unset($audits[$key]); - continue; - } + $commits = $query->execute(); + $commits = mpull($commits, null, 'getPHID'); + $audits = array(); + foreach ($commits as $commit) { + $curr_audits = $commit->getAudits(); + foreach ($audits as $key => $audit) { + if ($min_date && $commit->getEpoch() < $min_date) { + unset($audits[$key]); + continue; + } - if ($max_date && $commit->getEpoch() > $max_date) { - unset($audits[$key]); - continue; + if ($max_date && $commit->getEpoch() > $max_date) { + unset($audits[$key]); + continue; + } } + $audits[] = $curr_audits; } + $audits = array_mergev($audits); $console = PhutilConsole::getConsole(); @@ -146,7 +135,7 @@ foreach ($audits as $audit) { - $commit = idx($commits, $audit->getCommitPHID()); + $commit = $commits[$audit->getCommitPHID()]; $console->writeOut( "%s\n", diff --git a/src/applications/audit/query/PhabricatorAuditCommitQuery.php b/src/applications/audit/query/PhabricatorAuditCommitQuery.php deleted file mode 100644 --- a/src/applications/audit/query/PhabricatorAuditCommitQuery.php +++ /dev/null @@ -1,220 +0,0 @@ -authorPHIDs = $author_phids; - return $this; - } - - public function withPackagePHIDs(array $phids) { - $this->packagePHIDs = $phids; - return $this; - } - - public function withCommitPHIDs(array $phids) { - $this->commitPHIDs = $phids; - return $this; - } - - public function withStatus($status) { - $this->status = $status; - return $this; - } - - public function withIdentifiers($repository_id, array $identifiers) { - $this->identifiers[] = array($repository_id, $identifiers); - return $this; - } - - public function needCommitData($need) { - $this->needCommitData = $need; - return $this; - } - - public function needAudits($need) { - $this->needAudits = $need; - return $this; - } - - public function setOffset($offset) { - $this->offset = $offset; - return $this; - } - - public function setLimit($limit) { - $this->limit = $limit; - return $this; - } - - public function execute() { - - $table = new PhabricatorRepositoryCommit(); - $conn_r = $table->establishConnection('r'); - - $join = $this->buildJoinClause($conn_r); - $where = $this->buildWhereClause($conn_r); - $order = $this->buildOrderClause($conn_r); - $limit = $this->buildLimitClause($conn_r); - - $data = queryfx_all( - $conn_r, - 'SELECT c.* FROM %T c %Q %Q %Q %Q', - $table->getTableName(), - $join, - $where, - $order, - $limit); - - $commits = $table->loadAllFromArray($data); - - if ($this->needCommitData && $commits) { - $data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( - 'commitID in (%Ld)', - mpull($commits, 'getID')); - $data = mpull($data, null, 'getCommitID'); - foreach ($commits as $commit) { - if (idx($data, $commit->getID())) { - $commit->attachCommitData($data[$commit->getID()]); - } else { - $commit->attachCommitData(new PhabricatorRepositoryCommitData()); - } - } - } - - if ($this->needAudits && $commits) { - $audits = id(new PhabricatorAuditComment())->loadAllWhere( - 'targetPHID in (%Ls)', - mpull($commits, 'getPHID')); - $audits = mgroup($audits, 'getTargetPHID'); - foreach ($commits as $commit) { - $commit->attachAudits(idx($audits, $commit->getPHID(), array())); - } - } - - return $commits; - - } - - private function buildOrderClause($conn_r) { - return 'ORDER BY c.epoch DESC'; - } - - private function buildJoinClause($conn_r) { - $join = array(); - - if ($this->packagePHIDs) { - $join[] = qsprintf( - $conn_r, - 'JOIN %T req ON c.phid = req.commitPHID', - id(new PhabricatorRepositoryAuditRequest())->getTableName()); - } - - if ($join) { - $join = implode(' ', $join); - } else { - $join = ''; - } - - return $join; - } - - private function buildWhereClause($conn_r) { - $where = array(); - - if ($this->commitPHIDs) { - $where[] = qsprintf( - $conn_r, - 'c.phid IN (%Ls)', - $this->commitPHIDs); - } - - if ($this->authorPHIDs) { - $where[] = qsprintf( - $conn_r, - 'c.authorPHID IN (%Ls)', - $this->authorPHIDs); - } - - if ($this->packagePHIDs) { - $where[] = qsprintf( - $conn_r, - 'req.auditorPHID in (%Ls)', - $this->packagePHIDs); - } - - if ($this->identifiers) { - $clauses = array(); - foreach ($this->identifiers as $spec) { - list($repository_id, $identifiers) = $spec; - if ($identifiers) { - $clauses[] = qsprintf( - $conn_r, - 'c.repositoryID = %d AND c.commitIdentifier IN (%Ls)', - $repository_id, - $identifiers); - } - } - if ($clauses) { - $where[] = '('.implode(') OR (', $clauses).')'; - } - } - - $status = $this->status; - switch ($status) { - case self::STATUS_CONCERN: - $where[] = qsprintf( - $conn_r, - 'c.auditStatus = %d', - PhabricatorAuditCommitStatusConstants::CONCERN_RAISED); - break; - case self::STATUS_OPEN: - $where[] = qsprintf( - $conn_r, - 'c.auditStatus IN (%Ld)', - PhabricatorAuditCommitStatusConstants::getOpenStatusConstants()); - break; - case self::STATUS_ANY: - break; - default: - throw new Exception("Unknown status '{$status}'!"); - } - - if ($where) { - $where = 'WHERE ('.implode(') AND (', $where).')'; - } else { - $where = ''; - } - - return $where; - } - - private function buildLimitClause($conn_r) { - if ($this->limit && $this->offset) { - return qsprintf($conn_r, 'LIMIT %d, %d', $this->offset, $this->limit); - } else if ($this->limit) { - return qsprintf($conn_r, 'LIMIT %d', $this->limit); - } else if ($this->offset) { - return qsprintf($conn_r, 'LIMIT %d, %d', $this->offset, PHP_INT_MAX); - } else { - return ''; - } - } - -} diff --git a/src/applications/audit/query/PhabricatorAuditQuery.php b/src/applications/audit/query/PhabricatorAuditQuery.php deleted file mode 100644 --- a/src/applications/audit/query/PhabricatorAuditQuery.php +++ /dev/null @@ -1,263 +0,0 @@ -ids = $ids; - return $this; - } - - public function withCommitPHIDs(array $commit_phids) { - $this->commitPHIDs = $commit_phids; - return $this; - } - - public function withAuditorPHIDs(array $auditor_phids) { - $this->auditorPHIDs = $auditor_phids; - return $this; - } - - public function withRepositoryPHIDs(array $repository_phids) { - $this->repositoryPHIDs = $repository_phids; - return $this; - } - - public function withAwaitingUser(PhabricatorUser $user) { - $this->awaitingUser = $user; - return $this; - } - - public function withStatus($status) { - $this->status = $status; - return $this; - } - - public function setOffset($offset) { - $this->offset = $offset; - return $this; - } - - public function setLimit($limit) { - $this->limit = $limit; - return $this; - } - - public function needCommits($need) { - $this->needCommits = $need; - return $this; - } - - public function needCommitData($need) { - $this->needCommitData = $need; - return $this; - } - - public function execute() { - $table = new PhabricatorRepositoryAuditRequest(); - $conn_r = $table->establishConnection('r'); - - $joins = $this->buildJoinClause($conn_r); - $where = $this->buildWhereClause($conn_r); - $order = $this->buildOrderClause($conn_r); - $limit = $this->buildLimitClause($conn_r); - - $data = queryfx_all( - $conn_r, - 'SELECT req.* FROM %T req %Q %Q %Q %Q', - $table->getTableName(), - $joins, - $where, - $order, - $limit); - - $audits = $table->loadAllFromArray($data); - - if ($this->needCommits || $this->needCommitData) { - $phids = mpull($audits, 'getCommitPHID', 'getCommitPHID'); - if ($phids) { - $cquery = new PhabricatorAuditCommitQuery(); - $cquery->needCommitData($this->needCommitData); - $cquery->withCommitPHIDs(array_keys($phids)); - $commits = $cquery->execute(); - } else { - $commits = array(); - } - $this->commits = $commits; - } - - return $audits; - } - - public function getCommits() { - if ($this->commits === null) { - throw new Exception( - "Call needCommits() or needCommitData() and then execute() the query ". - "before calling getCommits()!"); - } - - return $this->commits; - } - - private function buildJoinClause($conn_r) { - - $joins = array(); - - if ($this->awaitingUser) { - // Join the request table on the awaiting user's requests, so we can - // filter out package and project requests which the user has resigned - // from. - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T awaiting ON req.commitPHID = awaiting.commitPHID AND - awaiting.auditorPHID = %s', - id(new PhabricatorRepositoryAuditRequest())->getTableName(), - $this->awaitingUser->getPHID()); - } - - if ($this->awaitingUser || $this->repositoryPHIDs) { - // Join the commit table so we can get the commit author or repository id - // into the result row and filter by it later. - $joins[] = qsprintf( - $conn_r, - 'JOIN %T commit ON req.commitPHID = commit.phid', - id(new PhabricatorRepositoryCommit())->getTableName()); - } - - if ($this->repositoryPHIDs) { - // Join in the repository table so we can filter by repository PHID - $joins[] = qsprintf( - $conn_r, - 'JOIN %T repository ON repository.id = commit.repositoryID', - id(new PhabricatorRepository())->getTableName()); - } - - if ($joins) { - return implode(' ', $joins); - } else { - return ''; - } - } - - private function buildWhereClause($conn_r) { - $where = array(); - - if ($this->ids) { - $where[] = qsprintf( - $conn_r, - 'req.id IN (%Ld)', - $this->ids); - } - - if ($this->commitPHIDs) { - $where[] = qsprintf( - $conn_r, - 'req.commitPHID IN (%Ls)', - $this->commitPHIDs); - } - - if ($this->auditorPHIDs) { - $where[] = qsprintf( - $conn_r, - 'req.auditorPHID IN (%Ls)', - $this->auditorPHIDs); - } - - if ($this->awaitingUser) { - // Exclude package and project audits associated with commits where - // the user is the author. - $where[] = qsprintf( - $conn_r, - '(commit.authorPHID IS NULL OR commit.authorPHID != %s) - OR (req.auditorPHID = %s)', - $this->awaitingUser->getPHID(), - $this->awaitingUser->getPHID()); - } - - if ($this->repositoryPHIDs) { - // Filter only for a single repository - $where[] = qsprintf( - $conn_r, - 'repository.phid IN (%Ls)', - $this->repositoryPHIDs); - } - - $status = $this->status; - switch ($status) { - case self::STATUS_CONCERN: - $where[] = qsprintf( - $conn_r, - 'req.auditStatus = %s', - PhabricatorAuditStatusConstants::CONCERNED); - break; - case self::STATUS_OPEN: - $where[] = qsprintf( - $conn_r, - 'req.auditStatus in (%Ls)', - PhabricatorAuditStatusConstants::getOpenStatusConstants()); - if ($this->awaitingUser) { - $where[] = qsprintf( - $conn_r, - 'awaiting.auditStatus IS NULL OR awaiting.auditStatus != %s', - PhabricatorAuditStatusConstants::RESIGNED); - } - break; - case self::STATUS_ANY: - break; - default: - $valid = array( - self::STATUS_ANY, - self::STATUS_OPEN, - self::STATUS_CONCERN, - ); - throw new Exception( - "Unknown audit status '{$status}'! Valid statuses are: ". - implode(', ', $valid)); - } - - if ($where) { - $where = 'WHERE ('.implode(') AND (', $where).')'; - } else { - $where = ''; - } - - return $where; - } - - private function buildLimitClause($conn_r) { - if ($this->limit && $this->offset) { - return qsprintf($conn_r, 'LIMIT %d, %d', $this->offset, $this->limit); - } else if ($this->limit) { - return qsprintf($conn_r, 'LIMIT %d', $this->limit); - } else if ($this->offset) { - return qsprintf($conn_r, 'LIMIT %d, %d', $this->offset, PHP_INT_MAX); - } else { - return ''; - } - } - - private function buildOrderClause($conn_r) { - return 'ORDER BY req.id DESC'; - } - -} diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php new file mode 100644 --- /dev/null +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -0,0 +1,154 @@ +setParameter( + 'auditorPHIDs', + $this->readPHIDsFromRequest($request, 'auditorPHIDs')); + + $saved->setParameter( + 'commitAuthorPHIDs', + $this->readUsersFromRequest($request, 'commitAuthorPHIDs')); + + $saved->setParameter( + 'auditStatus', + $request->getStr('auditStatus')); + + $saved->setParameter( + 'repositoryPHIDs', + $this->readPHIDsFromRequest($request, 'repositoryPHIDs')); + + // -- TODO - T4173 - file location + + return $saved; + } + + public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { + $query = id(new DiffusionCommitQuery()) + ->needAuditRequests(true) + ->needCommitData(true); + + $auditor_phids = $saved->getParameter('auditorPHIDs', array()); + if ($auditor_phids) { + $query->withAuditorPHIDs($auditor_phids); + } + + $commit_author_phids = $saved->getParameter('commitAuthorPHIDs', array()); + if ($commit_author_phids) { + $query->withAuthorPHIDs($commit_author_phids); + } + + $audit_status = $saved->getParameter('auditStatus', null); + if ($audit_status) { + $query->withAuditStatus($audit_status); + } + + $awaiting_user_phid = $saved->getParameter('awaitingUserPHID', null); + if ($awaiting_user_phid) { + // This is used only for the built-in "needs attention" filter, + // so cheat and just use the already-loaded viewer rather than reloading + // it. + $query->withAuditAwaitingUser($this->requireViewer()); + } + + return $query; + } + + public function buildSearchForm( + AphrontFormView $form, + PhabricatorSavedQuery $saved) { + + $auditor_phids = $saved->getParameter('auditorPHIDs', array()); + $commit_author_phids = $saved->getParameter( + 'commitAuthorPHIDs', + array()); + $audit_status = $saved->getParameter('auditStatus', null); + + $phids = array_mergev( + array( + $auditor_phids, + $commit_author_phids)); + + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($this->requireViewer()) + ->withPHIDs($phids) + ->execute(); + + $form + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setDatasource('/typeahead/common/usersprojectsorpackages/') + ->setName('auditorPHIDs') + ->setLabel(pht('Auditors')) + ->setValue(array_select_keys($handles, $auditor_phids))) + ->appendChild( + id(new AphrontFormTokenizerControl()) + ->setDatasource('/typeahead/common/users/') + ->setName('commitAuthorPHIDs') + ->setLabel(pht('Commit Authors')) + ->setValue(array_select_keys($handles, $commit_author_phids))) + ->appendChild( + id(new AphrontFormSelectControl()) + ->setName('auditStatus') + ->setLabel(pht('Audit Status')) + ->setOptions($this->getAuditStatusOptions()) + ->setValue($audit_status)); + } + + protected function getURI($path) { + return '/audit/'.$path; + } + + public function getBuiltinQueryNames() { + $names = array(); + + if ($this->requireViewer()->isLoggedIn()) { + $names['need_attention'] = pht('Need Attention'); + } + $names['open'] = pht('Open Audits'); + + $names['all'] = pht('All Commits'); + + return $names; + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + $viewer = $this->requireViewer(); + + switch ($query_key) { + case 'all': + return $query; + case 'open': + $query->setParameter( + 'auditStatus', + DiffusionCommitQuery::AUDIT_STATUS_OPEN); + return $query; + case 'need_attention': + $query->setParameter('awaitingUserPHID', $viewer->getPHID()); + $query->setParameter( + 'auditStatus', + DiffusionCommitQuery::AUDIT_STATUS_OPEN); + $query->setParameter( + 'auditorPHIDs', + PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($viewer)); + 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'), + ); + } + +} diff --git a/src/applications/audit/view/PhabricatorAuditCommitListView.php b/src/applications/audit/view/PhabricatorAuditCommitListView.php deleted file mode 100644 --- a/src/applications/audit/view/PhabricatorAuditCommitListView.php +++ /dev/null @@ -1,123 +0,0 @@ -noDataString = $no_data_string; - return $this; - } - - public function setCommits(array $commits) { - assert_instances_of($commits, 'PhabricatorRepositoryCommit'); - $this->commits = mpull($commits, null, 'getPHID'); - return $this; - } - - 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; - } - - public function getRequiredHandlePHIDs() { - $phids = array(); - foreach ($this->commits as $commit) { - if ($commit->getAuthorPHID()) { - $phids[$commit->getAuthorPHID()] = true; - } - $phids[$commit->getPHID()] = true; - if ($commit->getAudits()) { - foreach ($commit->getAudits() as $audit) { - $phids[$audit->getActorPHID()] = true; - } - } - } - return array_keys($phids); - } - - private function getHandle($phid) { - $handle = idx($this->handles, $phid); - if (!$handle) { - throw new Exception("No handle for '{$phid}'!"); - } - return $handle; - } - - private function getCommitDescription($phid) { - if ($this->commits === null) { - return null; - } - - $commit = idx($this->commits, $phid); - if (!$commit) { - return null; - } - - return $commit->getCommitData()->getSummary(); - } - - public function render() { - $list = new PHUIObjectItemListView(); - $list->setCards(true); - $list->setFlush(true); - foreach ($this->commits as $commit) { - $commit_phid = $commit->getPHID(); - $commit_name = $this->getHandle($commit_phid)->getName(); - $commit_link = $this->getHandle($commit_phid)->getURI(); - $commit_desc = $this->getCommitDescription($commit_phid); - - $author_name = null; - if ($commit->getAuthorPHID()) { - $author_name = $this->getHandle($commit->getAuthorPHID())->renderLink(); - } - $auditors = array(); - if ($commit->getAudits()) { - foreach ($commit->getAudits() as $audit) { - $actor_phid = $audit->getActorPHID(); - $auditors[$actor_phid] = $this->getHandle($actor_phid)->renderLink(); - } - $auditors = phutil_implode_html(', ', $auditors); - } - $committed = phabricator_datetime($commit->getEpoch(), $this->user); - $audit_status = $commit->getAuditStatus(); - $commit_status = PhabricatorAuditCommitStatusConstants::getStatusName( - $audit_status); - $status_color = PhabricatorAuditCommitStatusConstants::getStatusColor( - $audit_status); - - $item = id(new PHUIObjectItemView()) - ->setBarColor($status_color) - ->setObjectName($commit_name) - ->setHeader($commit_desc) - ->setHref($commit_link) - ->addAttribute($commit_status) - ->addIcon('none', $committed); - - if (!empty($auditors)) { - $item->addAttribute(pht('Auditors: %s', $auditors)); - } - - if ($author_name) { - $item->addByline(pht('Author: %s', $author_name)); - } - - $list->addItem($item); - } - - if ($this->noDataString) { - $list->setNoDataString($this->noDataString); - } - - return $list->render(); - } - -} 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 @@ -2,21 +2,13 @@ final class PhabricatorAuditListView extends AphrontView { - private $audits; + private $commits; private $handles; private $authorityPHIDs = array(); private $noDataString; - private $commits; - private $showCommits = true; private $highlightedAudits; - public function setAudits(array $audits) { - assert_instances_of($audits, 'PhabricatorRepositoryAuditRequest'); - $this->audits = $audits; - return $this; - } - public function setHandles(array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); $this->handles = $handles; @@ -37,22 +29,29 @@ return $this->noDataString; } + /** + * These commits should have both commit data and audit requests attached. + */ public function setCommits(array $commits) { assert_instances_of($commits, 'PhabricatorRepositoryCommit'); $this->commits = mpull($commits, null, 'getPHID'); return $this; } - public function setShowCommits($show_commits) { - $this->showCommits = $show_commits; - return $this; + public function getCommits() { + return $this->commits; } public function getRequiredHandlePHIDs() { $phids = array(); - foreach ($this->audits as $audit) { - $phids[$audit->getCommitPHID()] = true; - $phids[$audit->getAuditorPHID()] = true; + $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); } @@ -78,65 +77,58 @@ return $commit->getCommitData()->getSummary(); } - public function getHighlightedAudits() { - if ($this->highlightedAudits === null) { - $this->highlightedAudits = array(); - - $user = $this->user; - $authority = array_fill_keys($this->authorityPHIDs, true); - - foreach ($this->audits as $audit) { - $has_authority = !empty($authority[$audit->getAuditorPHID()]); - if ($has_authority) { - $commit_phid = $audit->getCommitPHID(); - $commit_author = $this->commits[$commit_phid]->getAuthorPHID(); - - // You don't have authority over package and project audits on your - // own commits. - - $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); - $user_is_author = ($commit_author == $user->getPHID()); - - if ($auditor_is_user || !$user_is_author) { - $this->highlightedAudits[$audit->getID()] = $audit; - } - } - } - } - - return $this->highlightedAudits; + public function render() { + $list = $this->buildList(); + $list->setCards(true); + $list->setFlush(true); + return $list->render(); } - public function render() { + public function buildList() { + $user = $this->getUser(); + if (!$user) { + throw new Exception('you must setUser() before buildList()!'); + } $rowc = array(); $list = new PHUIObjectItemListView(); - $list->setCards(true); - $list->setFlush(true); - foreach ($this->audits as $audit) { - $commit_phid = $audit->getCommitPHID(); + $authority = array_fill_keys($this->authorityPHIDs, true); + foreach ($this->commits as $commit) { + $commit_phid = $commit->getPHID(); + $commit_handle = $this->getHandle($commit_phid); $committed = null; - $commit_name = $this->getHandle($commit_phid)->getName(); - $commit_link = $this->getHandle($commit_phid)->getURI(); + $commit_name = $commit_handle->getName(); + $commit_link = $commit_handle->getURI(); $commit_desc = $this->getCommitDescription($commit_phid); - $commit = idx($this->commits, $commit_phid); - if ($commit && $this->user) { - $committed = phabricator_datetime($commit->getEpoch(), $this->user); + $committed = phabricator_datetime($commit->getEpoch(), $user); + + $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 = phutil_implode_html(', ', $auditors); + + $audit = idx($audits, $user->getPHID()); + if ($audit) { + $reasons = $audit->getAuditReasons(); + $reasons = phutil_implode_html(', ', $reasons); + $status_code = $audit->getAuditStatus(); + $status_text = + PhabricatorAuditStatusConstants::getStatusName($status_code); + $status_color = + PhabricatorAuditStatusConstants::getStatusColor($status_code); + } else { + $reasons = null; + $status_text = null; + $status_color = null; } - - $reasons = $audit->getAuditReasons(); - $reasons = phutil_implode_html(', ', $reasons); - - $status_code = $audit->getAuditStatus(); - $status_text = - PhabricatorAuditStatusConstants::getStatusName($status_code); - $status_color = - PhabricatorAuditStatusConstants::getStatusColor($status_code); - $author_name = $commit->getCommitData()->getAuthorName(); - $auditor_handle = $this->getHandle($audit->getAuditorPHID()); $item = id(new PHUIObjectItemView()) ->setObjectName($commit_name) ->setHeader($commit_desc) @@ -144,11 +136,14 @@ ->setBarColor($status_color) ->addAttribute($status_text) ->addAttribute($reasons) - ->addAttribute(pht('Author: %s', $author_name)) ->addIcon('none', $committed) - ->addByline(pht('Auditor: %s', $auditor_handle->renderLink())); + ->addByline(pht('Author: %s', $author_name)); - if (array_key_exists($audit->getID(), $this->getHighlightedAudits())) { + if (!empty($auditors)) { + $item->addAttribute(pht('Auditors: %s', $auditors)); + } + + if ($commit->getAuthorityAudits($user, $this->authorityPHIDs)) { $item->setEffect('highlighted'); } @@ -159,7 +154,7 @@ $list->setNoDataString($this->noDataString); } - return $list->render(); + return $list; } } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -28,11 +28,18 @@ return $this->buildRawDiffResponse($drequest); } - $callsign = $drequest->getRepository()->getCallsign(); + $repository = $drequest->getRepository(); + $callsign = $repository->getCallsign(); $content = array(); - $repository = $drequest->getRepository(); - $commit = $drequest->loadCommit(); + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($request->getUser()) + ->withRepository($repository) + ->withIdentifiers(array($drequest->getCommit())) + ->needCommitData(true) + ->needAuditRequests(true) + ->executeOne(); $crumbs = $this->buildCrumbs(array( 'commit' => true, @@ -63,19 +70,16 @@ )); } - $commit_data = $drequest->loadCommitData(); - $commit->attachCommitData($commit_data); $top_anchor = id(new PhabricatorAnchorView()) ->setAnchorName('top') ->setNavigationMarker(true); - $audit_requests = id(new PhabricatorAuditQuery()) - ->withCommitPHIDs(array($commit->getPHID())) - ->execute(); + $audit_requests = $commit->getAudits(); $this->auditAuthorityPHIDs = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user); + $commit_data = $commit->getCommitData(); $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); $changesets = null; if ($is_foreign) { @@ -179,15 +183,9 @@ $content[] = $this->buildMergesTable($commit); - // TODO: This is silly, but the logic to figure out which audits are - // highlighted currently lives in PhabricatorAuditListView. Refactor this - // to be less goofy. - $highlighted_audits = id(new PhabricatorAuditListView()) - ->setAudits($audit_requests) - ->setAuthorityPHIDs($this->auditAuthorityPHIDs) - ->setUser($user) - ->setCommits(array($commit->getPHID() => $commit)) - ->getHighlightedAudits(); + $highlighted_audits = $commit->getAuthorityAudits( + $user, + $this->auditAuthorityPHIDs); $owners_paths = array(); if ($highlighted_audits) { diff --git a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php --- a/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php +++ b/src/applications/diffusion/doorkeeper/DiffusionDoorkeeperCommitFeedStoryPublisher.php @@ -46,14 +46,16 @@ } public function willPublishStory($commit) { - $requests = id(new PhabricatorAuditQuery()) - ->withCommitPHIDs(array($commit->getPHID())) - ->execute(); + $requests = id(new DiffusionCommitQuery()) + ->withPHIDs(array($commit->getPHID())) + ->needAuditRequests(true) + ->executeOne() + ->getAudits(); // TODO: This is messy and should be generalized, but we don't have a good // query for it yet. Since we run in the daemons, just do the easiest thing // we can for the moment. Figure out who all of the "active" (need to - // audit) and "passive" (no action necessary) user are. + // audit) and "passive" (no action necessary) users are. $auditor_phids = mpull($requests, 'getAuditorPHID'); $objects = id(new PhabricatorObjectQuery()) @@ -107,7 +109,6 @@ } - // Remove "Active" users from the "Passive" list. $passive = array_diff_key($passive, $active); 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 @@ -4,14 +4,40 @@ extends PhabricatorCursorPagedPolicyAwareQuery { private $ids; - private $identifiers; private $phids; + private $authorPHIDs; private $defaultRepository; - private $identifierMap; + private $identifiers; private $repositoryIDs; + private $identifierMap; + + private $needAuditRequests; + private $auditIDs; + private $auditorPHIDs; + private $auditAwaitingUser; + private $auditStatus; + const AUDIT_STATUS_ANY = 'audit-status-any'; + const AUDIT_STATUS_OPEN = 'audit-status-open'; + const AUDIT_STATUS_CONCERN = 'audit-status-concern'; + private $loadAuditIds; private $needCommitData; + public function withIDs(array $ids) { + $this->ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withAuthorPHIDs(array $phids) { + $this->authorPHIDs = $phids; + return $this; + } + /** * Load commits by partial or full identifiers, e.g. "rXab82393", "rX1234", * or "a9caf12". When an identifier matches multiple commits, they will all @@ -24,6 +50,16 @@ } /** + * Look up commits in a specific repository. This is a shorthand for calling + * @{method:withDefaultRepository} and @{method:withRepositoryIDs}. + */ + public function withRepository(PhabricatorRepository $repository) { + $this->withDefaultRepository($repository); + $this->withRepositoryIDs(array($repository->getID())); + return $this; + } + + /** * If a default repository is provided, ambiguous commit identifiers will * be assumed to belong to the default repository. * @@ -42,29 +78,42 @@ return $this; } - public function withIDs(array $ids) { - $this->ids = $ids; + public function needCommitData($need) { + $this->needCommitData = $need; return $this; } - public function withPHIDs(array $phids) { - $this->phids = $phids; + public function needAuditRequests($need) { + $this->needAuditRequests = $need; return $this; } + public function getAuditRequests() { + return + $this->needAuditRequests || + $this->auditIDs || + $this->auditorPHIDs || + $this->auditAwaitingUser || + $this->auditStatus; + } - /** - * Look up commits in a specific repository. This is a shorthand for calling - * @{method:withDefaultRepository} and @{method:withRepositoryIDs}. - */ - public function withRepository(PhabricatorRepository $repository) { - $this->withDefaultRepository($repository); - $this->withRepositoryIDs(array($repository->getID())); + public function withAuditIDs(array $ids) { + $this->auditIDs = $ids; return $this; } - public function needCommitData($need) { - $this->needCommitData = $need; + public function withAuditorPHIDs(array $auditor_phids) { + $this->auditorPHIDs = $auditor_phids; + return $this; + } + + public function withAuditAwaitingUser(PhabricatorUser $user) { + $this->auditAwaitingUser = $user; + return $this; + } + + public function withAuditStatus($status) { + $this->auditStatus = $status; return $this; } @@ -76,6 +125,10 @@ return $this->identifierMap; } + protected function getPagingColumn() { + return 'commit.id'; + } + protected function willExecute() { if ($this->identifierMap === null) { $this->identifierMap = array(); @@ -88,15 +141,31 @@ $data = queryfx_all( $conn_r, - 'SELECT * FROM %T %Q %Q %Q', + 'SELECT commit.* %Q FROM %T commit %Q %Q %Q %Q', + $this->buildAuditSelect($conn_r), $table->getTableName(), + $this->buildJoinClause($conn_r), $this->buildWhereClause($conn_r), $this->buildOrderClause($conn_r), $this->buildLimitClause($conn_r)); + if ($this->getAuditRequests()) { + $this->loadAuditIds = ipull($data, 'audit_id'); + } + return $table->loadAllFromArray($data); } + private function buildAuditSelect($conn_r) { + if ($this->getAuditRequests()) { + return qsprintf( + $conn_r, + ', audit.id as audit_id'); + } + + return ''; + } + protected function willFilterPage(array $commits) { $repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID'); $repos = id(new PhabricatorRepositoryQuery()) @@ -170,12 +239,54 @@ } } + if ($this->getAuditRequests()) { + $requests = id(new PhabricatorRepositoryAuditRequest()) + ->loadAllWhere('id IN (%Ld)', $this->loadAuditIds); + + $requests = mgroup($requests, 'getCommitPHID'); + foreach ($commits as $commit) { + $audit_requests = idx($requests, $commit->getPHID(), array()); + $commit->attachAudits($audit_requests); + foreach ($audit_requests as $audit_request) { + $audit_request->attachCommit($commit); + } + } + } + return $commits; } private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); + if ($this->ids) { + $where[] = qsprintf( + $conn_r, + 'commit.id IN (%Ld)', + $this->ids); + } + + if ($this->phids) { + $where[] = qsprintf( + $conn_r, + 'commit.phid IN (%Ls)', + $this->phids); + } + + if ($this->repositoryIDs) { + $where[] = qsprintf( + $conn_r, + 'commit.repositoryID IN (%Ld)', + $this->repositoryIDs); + } + + if ($this->authorPHIDs) { + $where[] = qsprintf( + $conn_r, + 'commit.authorPHID IN (%Ls)', + $this->authorPHIDs); + } + if ($this->identifiers) { $min_unqualified = PhabricatorRepository::MINIMUM_UNQUALIFIED_HASH; $min_qualified = PhabricatorRepository::MINIMUM_QUALIFIED_HASH; @@ -212,7 +323,8 @@ foreach ($bare as $identifier) { $sql[] = qsprintf( $conn_r, - '(commitIdentifier LIKE %> AND LENGTH(commitIdentifier) = 40)', + '(commit.commitIdentifier LIKE %> AND '. + 'LENGTH(commit.commitIdentifier) = 40)', $identifier); } @@ -236,7 +348,7 @@ } $sql[] = qsprintf( $conn_r, - '(repositoryID = %d AND commitIdentifier = %s)', + '(commit.repositoryID = %d AND commit.commitIdentifier = %s)', $repo->getID(), // NOTE: Because the 'commitIdentifier' column is a string, MySQL // ignores the index if we hand it an integer. Hand it a string. @@ -248,7 +360,7 @@ } $sql[] = qsprintf( $conn_r, - '(repositoryID = %d AND commitIdentifier LIKE %>)', + '(commit.repositoryID = %d AND commit.commitIdentifier LIKE %>)', $repo->getID(), $ref['identifier']); } @@ -266,25 +378,65 @@ $where[] = '('.implode(' OR ', $sql).')'; } - if ($this->ids) { + if ($this->auditIDs) { $where[] = qsprintf( $conn_r, - 'id IN (%Ld)', - $this->ids); + 'audit.id IN (%Ld)', + $this->auditIDs); } - if ($this->phids) { + if ($this->auditorPHIDs) { $where[] = qsprintf( $conn_r, - 'phid IN (%Ls)', - $this->phids); + 'audit.auditorPHID IN (%Ls)', + $this->auditorPHIDs); } - if ($this->repositoryIDs) { + if ($this->auditAwaitingUser) { + $awaiting_user_phid = $this->auditAwaitingUser->getPHID(); + // Exclude package and project audits associated with commits where + // the user is the author. $where[] = qsprintf( $conn_r, - 'repositoryID IN (%Ld)', - $this->repositoryIDs); + '(commit.authorPHID IS NULL OR commit.authorPHID != %s) + OR (audit.auditorPHID = %s)', + $awaiting_user_phid, + $awaiting_user_phid); + } + + $status = $this->auditStatus; + if ($status !== null) { + switch ($status) { + case self::AUDIT_STATUS_CONCERN: + $where[] = qsprintf( + $conn_r, + 'audit.auditStatus = %s', + PhabricatorAuditStatusConstants::CONCERNED); + break; + case self::AUDIT_STATUS_OPEN: + $where[] = qsprintf( + $conn_r, + 'audit.auditStatus in (%Ls)', + PhabricatorAuditStatusConstants::getOpenStatusConstants()); + if ($this->auditAwaitingUser) { + $where[] = qsprintf( + $conn_r, + 'awaiting.auditStatus IS NULL OR awaiting.auditStatus != %s', + PhabricatorAuditStatusConstants::RESIGNED); + } + break; + case self::AUDIT_STATUS_ANY: + break; + default: + $valid = array( + self::AUDIT_STATUS_ANY, + self::AUDIT_STATUS_OPEN, + self::AUDIT_STATUS_CONCERN, + ); + throw new Exception( + "Unknown audit status '{$status}'! Valid statuses are: ". + implode(', ', $valid)); + } } $where[] = $this->buildPagingClause($conn_r); @@ -302,6 +454,36 @@ } } + private function buildJoinClause($conn_r) { + $joins = array(); + $audit_request = new PhabricatorRepositoryAuditRequest(); + + if ($this->getAuditRequests()) { + $joins[] = qsprintf( + $conn_r, + 'JOIN %T audit ON commit.phid = audit.commitPHID', + $audit_request->getTableName()); + } + + if ($this->auditAwaitingUser) { + // Join the request table on the awaiting user's requests, so we can + // filter out package and project requests which the user has resigned + // from. + $joins[] = qsprintf( + $conn_r, + 'LEFT JOIN %T awaiting ON audit.commitPHID = awaiting.commitPHID AND + awaiting.auditorPHID = %s', + $audit_request->getTableName(), + $this->auditAwaitingUser->getPHID()); + } + + if ($joins) { + return implode(' ', $joins); + } else { + return ''; + } + } + public function getQueryApplicationClass() { return 'PhabricatorApplicationDiffusion'; } diff --git a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php --- a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php @@ -97,10 +97,10 @@ $repository = $this->getRequest()->getRepository(); - $commits = id(new PhabricatorAuditCommitQuery()) - ->withIdentifiers( - $repository->getID(), - array_unique($line_rev_dict)) + $commits = id(new DiffusionCommitQuery()) + ->setViewer($this->getViewer()) + ->withDefaultRepository($repository) + ->withIdentifiers(array_unique($line_rev_dict)) ->execute(); foreach ($commits as $commit) { @@ -145,6 +145,10 @@ return $this; } + public function getViewer() { + return $this->viewer; + } + protected function processRevList(array $rev_list) { return $rev_list; } diff --git a/src/applications/home/controller/PhabricatorHomeMainController.php b/src/applications/home/controller/PhabricatorHomeMainController.php --- a/src/applications/home/controller/PhabricatorHomeMainController.php +++ b/src/applications/home/controller/PhabricatorHomeMainController.php @@ -413,26 +413,25 @@ $phids = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user); - $query = new PhabricatorAuditQuery(); - $query->withAuditorPHIDs($phids); - $query->withStatus(PhabricatorAuditQuery::STATUS_OPEN); - $query->withAwaitingUser($user); - $query->needCommitData(true); - $query->setLimit(10); + $query = id(new DiffusionCommitQuery()) + ->setViewer($user) + ->withAuditorPHIDs($phids) + ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_OPEN) + ->withAuditAwaitingUser($user) + ->needCommitData(true) + ->setLimit(10); - $audits = $query->execute(); - $commits = $query->getCommits(); + $commits = $query->execute(); - if (!$audits) { + if (!$commits) { return $this->renderMinipanel( 'No Audits', 'No commits are waiting for you to audit them.'); } - $view = new PhabricatorAuditListView(); - $view->setAudits($audits); - $view->setCommits($commits); - $view->setUser($user); + $view = id(new PhabricatorAuditListView()) + ->setCommits($commits) + ->setUser($user); $phids = $view->getRequiredHandlePHIDs(); $handles = $this->loadViewerHandles($phids); @@ -454,11 +453,12 @@ $phids = array($user->getPHID()); - $query = new PhabricatorAuditCommitQuery(); - $query->withAuthorPHIDs($phids); - $query->withStatus(PhabricatorAuditCommitQuery::STATUS_CONCERN); - $query->needCommitData(true); - $query->setLimit(10); + $query = id(new DiffusionCommitQuery()) + ->setViewer($user) + ->withAuthorPHIDs($phids) + ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_CONCERN) + ->needCommitData(true) + ->setLimit(10); $commits = $query->execute(); @@ -468,9 +468,9 @@ 'No one has raised concerns with your commits.'); } - $view = new PhabricatorAuditCommitListView(); - $view->setCommits($commits); - $view->setUser($user); + $view = id(new PhabricatorAuditListView()) + ->setCommits($commits) + ->setUser($user); $phids = $view->getRequiredHandlePHIDs(); $handles = $this->loadViewerHandles($phids); 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 @@ -145,17 +145,17 @@ 'phid' => $package->getPHID(), )); - $attention_query = id(new PhabricatorAuditCommitQuery()) - ->withPackagePHIDs(array($package->getPHID())) - ->withStatus(PhabricatorAuditCommitQuery::STATUS_CONCERN) + $attention_commits = id(new DiffusionCommitQuery()) + ->setViewer($request->getUser()) + ->withAuditorPHIDs(array($package->getPHID())) + ->withAuditStatus(DiffusionCommitQuery::AUDIT_STATUS_CONCERN) ->needCommitData(true) - ->needAudits(true) - ->setLimit(10); - $attention_commits = $attention_query->execute(); + ->setLimit(10) + ->execute(); if ($attention_commits) { - $view = new PhabricatorAuditCommitListView(); - $view->setUser($user); - $view->setCommits($attention_commits); + $view = id(new PhabricatorAuditListView()) + ->setUser($user) + ->setCommits($attention_commits); $commit_views[] = array( 'view' => $view, @@ -170,17 +170,17 @@ ); } - $all_query = id(new PhabricatorAuditCommitQuery()) - ->withPackagePHIDs(array($package->getPHID())) + $all_commits = id(new DiffusionCommitQuery()) + ->setViewer($request->getUser()) + ->withAuditorPHIDs(array($package->getPHID())) ->needCommitData(true) - ->needAudits(true) - ->setLimit(100); - $all_commits = $all_query->execute(); - - $view = new PhabricatorAuditCommitListView(); - $view->setUser($user); - $view->setCommits($all_commits); - $view->setNoDataString(pht('No commits in this package.')); + ->setLimit(100) + ->execute(); + + $view = id(new PhabricatorAuditListView()) + ->setUser($user) + ->setCommits($all_commits) + ->setNoDataString(pht('No commits in this package.')); $commit_views[] = array( 'view' => $view, diff --git a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php --- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php +++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php @@ -1,12 +1,16 @@ false, @@ -21,4 +25,35 @@ return (phid_get_type($this->getAuditorPHID()) == $user_type); } + public function attachCommit(PhabricatorRepositoryCommit $commit) { + $this->commit = $commit; + return $this; + } + + public function getCommit() { + return $this->assertAttached($this->commit); + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return $this->getCommit()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getCommit()->hasAutomaticCapability($capability, $viewer); + } + + public function describeAutomaticCapability($capability) { + return pht( + 'This audit is attached to a commit, and inherits its policies.'); + } } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -28,7 +28,7 @@ const IMPORTED_CLOSEABLE = 1024; private $commitData = self::ATTACHABLE; - private $audits; + private $audits = self::ATTACHABLE; private $repository = self::ATTACHABLE; private $customFields = self::ATTACHABLE; @@ -91,13 +91,39 @@ } public function attachAudits(array $audits) { - assert_instances_of($audits, 'PhabricatorAuditComment'); + assert_instances_of($audits, 'PhabricatorRepositoryAuditRequest'); $this->audits = $audits; return $this; } public function getAudits() { - return $this->audits; + return $this->assertAttached($this->audits); + } + + public function getAuthorityAudits( + PhabricatorUser $user, + array $authority_phids) { + + $authority = array_fill_keys($authority_phids, true); + $audits = $this->getAudits(); + $authority_audits = array(); + foreach ($audits as $audit) { + $has_authority = !empty($authority[$audit->getAuditorPHID()]); + if ($has_authority) { + $commit_author = $this->getAuthorPHID(); + + // You don't have authority over package and project audits on your + // own commits. + + $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); + $user_is_author = ($commit_author == $user->getPHID()); + + if ($auditor_is_user || !$user_is_author) { + $authority_audits[$audit->getID()] = $audit; + } + } + } + return $authority_audits; } public function save() {