From bfa2abed84289f3663342ca8747a17d3d883223f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Sep 2013 12:55:47 -0700 Subject: [PATCH] Mostly modernize lint views and delete dead code Summary: This is a mostly-faithful modernization of the Diffusion lint interfaces. It: - Makes them policy aware; - removes the last callsites for old/dead code (crumbs, nav). It's a little rough, but should be perfectly usable. At some point this should get another pass, but probably after we make it easier to populate the lint data. Test Plan: See screenshots. Reviewers: btrahan Reviewed By: btrahan CC: aran, FacebookPOC Differential Revision: https://secure.phabricator.com/D7065 --- .../controller/DiffusionController.php | 124 -------------- .../controller/DiffusionLintController.php | 158 +++++++++++++++--- .../DiffusionLintDetailsController.php | 22 +-- 3 files changed, 140 insertions(+), 164 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index f75a75f389..a0baac4180 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -25,53 +25,6 @@ abstract class DiffusionController extends PhabricatorController { return $this->diffusionRequest; } - final protected function buildSideNav($selected, $has_change_view) { - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI('')); - - $navs = array( - 'history' => pht('History View'), - 'browse' => pht('Browse View'), - 'change' => pht('Change View'), - ); - - if (!$has_change_view) { - unset($navs['change']); - } - - $drequest = $this->getDiffusionRequest(); - $branch = $drequest->loadBranch(); - - if ($branch && $branch->getLintCommit()) { - $navs['lint'] = pht('Lint View'); - } - - $selected_href = null; - foreach ($navs as $action => $name) { - $href = $drequest->generateURI( - array( - 'action' => $action, - )); - if ($action == $selected) { - $selected_href = $href; - } - - $nav->addFilter($href, $name, $href); - } - $nav->selectFilter($selected_href, null); - - // TODO: URI encoding might need to be sorted out for this link. - - $nav->addFilter( - '', - pht("Search Owners \xE2\x86\x97"), - '/owners/view/search/'. - '?repository='.phutil_escape_uri($drequest->getCallsign()). - '&path='.phutil_escape_uri('/'.$drequest->getPath())); - - return $nav; - } - public function buildCrumbs(array $spec = array()) { $crumbs = $this->buildApplicationCrumbs(); $crumb_list = $this->buildCrumbList($spec); @@ -167,19 +120,6 @@ abstract class DiffusionController extends PhabricatorController { $crumb = new PhabricatorCrumbView(); $view = $spec['view']; - $path = null; - if (isset($spec['path'])) { - $path = $drequest->getPath(); - } - - if ($raw_commit) { - $commit_link = DiffusionView::linkCommit( - $repository, - $raw_commit); - } else { - $commit_link = ''; - } - switch ($view) { case 'history': $view_name = pht('History'); @@ -195,74 +135,10 @@ abstract class DiffusionController extends PhabricatorController { break; } - $uri_params = array( - 'action' => $view, - ); - $crumb = id(new PhabricatorCrumbView()) ->setName($view_name); - if ($view == 'browse' || $view == 'change' || $view == 'history') { - $crumb_list[] = $crumb; - return $crumb_list; - } - - $crumb->setHref($drequest->generateURI( - array( - 'path' => '', - ) + $uri_params)); $crumb_list[] = $crumb; - - $path_parts = explode('/', $path); - do { - $last = array_pop($path_parts); - } while (count($path_parts) && $last == ''); - - $path_sections = array(); - $thus_far = ''; - foreach ($path_parts as $path_part) { - $thus_far .= $path_part.'/'; - $path_sections[] = '/'; - $path_sections[] = phutil_tag( - 'a', - array( - 'href' => $drequest->generateURI( - array( - 'path' => $thus_far, - ) + $uri_params), - ), - $path_part); - } - - $path_sections[] = '/'.$last; - - $crumb_list[] = id(new PhabricatorCrumbView()) - ->setName($path_sections); - - $last_crumb = array_pop($crumb_list); - - if ($raw_commit) { - $jump_link = phutil_tag( - 'a', - array( - 'href' => $drequest->generateURI( - array( - 'commit' => '', - ) + $uri_params), - ), - pht('Jump to HEAD')); - - $name = $last_crumb->getName(); - $name = hsprintf('%s @ %s (%s)', $name, $commit_link, $jump_link); - $last_crumb->setName($name); - } else if ($spec['view'] != 'lint') { - $name = $last_crumb->getName(); - $name = hsprintf('%s @ HEAD', $name); - $last_crumb->setName($name); - } - - $crumb_list[] = $last_crumb; - return $crumb_list; } diff --git a/src/applications/diffusion/controller/DiffusionLintController.php b/src/applications/diffusion/controller/DiffusionLintController.php index c97f71c49d..ff4ca65700 100644 --- a/src/applications/diffusion/controller/DiffusionLintController.php +++ b/src/applications/diffusion/controller/DiffusionLintController.php @@ -2,6 +2,10 @@ final class DiffusionLintController extends DiffusionController { + public function shouldAllowPublic() { + return true; + } + public function processRequest() { $request = $this->getRequest(); $user = $this->getRequest()->getUser(); @@ -17,7 +21,9 @@ final class DiffusionLintController extends DiffusionController { $owners = array(); if (!$drequest) { if (!$request->getArr('owner')) { - $owners[$user->getPHID()] = $user->getFullName(); + if ($user->isLoggedIn()) { + $owners[$user->getPHID()] = $user->getFullName(); + } } else { $phids = $request->getArr('owner'); $phid = reset($phids); @@ -29,16 +35,23 @@ final class DiffusionLintController extends DiffusionController { $codes = $this->loadLintCodes(array_keys($owners)); if ($codes && !$drequest) { + // TODO: Build some real Query classes for this stuff. + $branches = id(new PhabricatorRepositoryBranch())->loadAllWhere( 'id IN (%Ld)', array_unique(ipull($codes, 'branchID'))); - $repositories = id(new PhabricatorRepository())->loadAllWhere( - 'id IN (%Ld)', - array_unique(mpull($branches, 'getRepositoryID'))); + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($user) + ->withIDs(mpull($branches, 'getRepositoryID')) + ->execute(); $drequests = array(); foreach ($branches as $id => $branch) { + if (empty($repositories[$branch->getRepositoryID()])) { + continue; + } + $drequests[$id] = DiffusionRequest::newFromDictionary(array( 'user' => $user, 'repository' => $repositories[$branch->getRepositoryID()], @@ -48,11 +61,18 @@ final class DiffusionLintController extends DiffusionController { } $rows = array(); + $total = 0; foreach ($codes as $code) { if (!$this->diffusionRequest) { - $drequest = $drequests[$code['branchID']]; + $drequest = idx($drequests, $code['branchID']); } + if (!$drequest) { + continue; + } + + $total += $code['n']; + $rows[] = array( hsprintf( '%s', @@ -95,16 +115,7 @@ final class DiffusionLintController extends DiffusionController { $content = array(); $link = null; - if ($this->diffusionRequest) { - $link = hsprintf( - '%s', - $drequest->generateURI(array( - 'action' => 'lint', - 'lint' => '', - )), - pht('Switch to List View')); - - } else { + if (!$this->diffusionRequest) { $form = id(new AphrontFormView()) ->setUser($user) ->setMethod('GET') @@ -122,7 +133,7 @@ final class DiffusionLintController extends DiffusionController { } $content[] = id(new AphrontPanelView()) - ->setHeader(pht('%d Lint Message(s)', array_sum(ipull($codes, 'n')))) + ->setNoBackground(true) ->setCaption($link) ->appendChild($table); @@ -133,21 +144,45 @@ final class DiffusionLintController extends DiffusionController { 'path' => true, 'view' => 'lint', )); + if ($this->diffusionRequest) { $title[] = $drequest->getCallsign(); - $content = $this->buildSideNav('lint', false) - ->setCrumbs($crumbs) - ->appendChild($content); } else { - array_unshift($content, $crumbs); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('All Lint'))); } + if ($this->diffusionRequest) { + $branch = $drequest->loadBranch(); + + $header = id(new PHUIHeaderView()) + ->setHeader($this->renderPathLinks($drequest, 'lint')) + ->setUser($user) + ->setPolicyObject($drequest->getRepository()); + $actions = $this->buildActionView($drequest); + $properties = $this->buildPropertyView( + $drequest, + $branch, + $total); + } else { + $header = null; + $actions = null; + $properties = null; + } + + return $this->buildApplicationPage( - $content, + array( + $crumbs, + $header, + $actions, + $properties, + $content, + ), array( 'title' => $title, - 'device' => true, - )); + )); } private function loadLintCodes(array $owner_phids) { @@ -233,4 +268,81 @@ final class DiffusionLintController extends DiffusionController { implode(' AND ', $where)); } + protected function buildActionView(DiffusionRequest $drequest) { + $viewer = $this->getRequest()->getUser(); + + $view = id(new PhabricatorActionListView()) + ->setUser($viewer); + + $list_uri = $drequest->generateURI( + array( + 'action' => 'lint', + 'lint' => '', + )); + + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('View As List')) + ->setHref($list_uri) + ->setIcon('transcript')); + + $history_uri = $drequest->generateURI( + array( + 'action' => 'history', + )); + + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('View History')) + ->setHref($history_uri) + ->setIcon('history')); + + $browse_uri = $drequest->generateURI( + array( + 'action' => 'browse', + )); + + $view->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Browse Content')) + ->setHref($browse_uri) + ->setIcon('file')); + + return $view; + } + + protected function buildPropertyView( + DiffusionRequest $drequest, + PhabricatorRepositoryBranch $branch, + $total) { + + $viewer = $this->getRequest()->getUser(); + + $view = id(new PhabricatorPropertyListView()) + ->setUser($viewer); + + $callsign = $drequest->getRepository()->getCallsign(); + $lint_commit = $branch->getLintCommit(); + + $view->addProperty( + pht('Lint Commit'), + phutil_tag( + 'a', + array( + 'href' => $drequest->generateURI( + array( + 'action' => 'commit', + 'commit' => $lint_commit, + )), + ), + $drequest->getRepository()->formatCommitName($lint_commit))); + + $view->addProperty( + pht('Total Messages'), + pht('%s', new PhutilNumber($total))); + + return $view; + } + + } diff --git a/src/applications/diffusion/controller/DiffusionLintDetailsController.php b/src/applications/diffusion/controller/DiffusionLintDetailsController.php index 2e3c729b23..d9437e6f12 100644 --- a/src/applications/diffusion/controller/DiffusionLintDetailsController.php +++ b/src/applications/diffusion/controller/DiffusionLintDetailsController.php @@ -68,35 +68,23 @@ final class DiffusionLintDetailsController extends DiffusionController { ->setHasMorePages(count($messages) >= $limit) ->setURI($this->getRequest()->getRequestURI(), 'offset'); - $lint = $drequest->getLint(); - $link = hsprintf( - '%s', - $drequest->generateURI(array( - 'action' => 'lint', - 'lint' => null, - )), - pht('Switch to Grouped View')); - $content[] = id(new AphrontPanelView()) - ->setHeader( - ($lint != '' ? $lint." \xC2\xB7 " : ''). - pht('%d Lint Message(s)', count($messages))) - ->setCaption($link) + ->setNoBackground(true) ->appendChild($table) ->appendChild($pager); - $nav = $this->buildSideNav('lint', false); - $nav->appendChild($content); $crumbs = $this->buildCrumbs( array( 'branch' => true, 'path' => true, 'view' => 'lint', )); - $nav->setCrumbs($crumbs); return $this->buildApplicationPage( - $nav, + array( + $crumbs, + $content, + ), array( 'device' => true, 'title' =>