From 5e715c1acac0811a7efd8036eae4ada45366a5ba Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 27 Dec 2015 02:04:37 -0800 Subject: [PATCH] Simplify some logic in project controllers Summary: Ref T10010. Several controlers currently have similar logic for handling tags and slugs, loading projects, and canonicalizing URIs. Clean it up a bit. Test Plan: - Visited profile, boards, feed. - Visited by ID and by tag. - Visited by non-normal tag (redircted). - Visited by alternate tag (redirected). - Visited non-policy project by non-normal tag (redirected into policy error). Reviewers: chad Reviewed By: chad Maniphest Tasks: T10010 Differential Revision: https://secure.phabricator.com/D14890 --- .../PhabricatorProjectController.php | 66 +++++++++++++++++++ .../PhabricatorProjectFeedController.php | 61 +++++------------ ...habricatorProjectMembersEditController.php | 4 -- .../PhabricatorProjectProfileController.php | 43 ++++-------- .../PhabricatorProjectViewController.php | 55 ++-------------- 5 files changed, 99 insertions(+), 130 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectController.php b/src/applications/project/controller/PhabricatorProjectController.php index ac406412ad..2fcb276576 100644 --- a/src/applications/project/controller/PhabricatorProjectController.php +++ b/src/applications/project/controller/PhabricatorProjectController.php @@ -13,6 +13,72 @@ abstract class PhabricatorProjectController extends PhabricatorController { return $this->project; } + protected function loadProject() { + $viewer = $this->getViewer(); + $request = $this->getRequest(); + + $id = $request->getURIData('id'); + $slug = $request->getURIData('slug'); + + if ($slug) { + $normal_slug = PhabricatorSlug::normalizeProjectSlug($slug); + $is_abnormal = ($slug !== $normal_slug); + $normal_uri = "/tag/{$normal_slug}/"; + } else { + $is_abnormal = false; + } + + $query = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->needMembers(true) + ->needWatchers(true) + ->needImages(true) + ->needSlugs(true); + + if ($slug) { + $query->withSlugs(array($slug)); + } else { + $query->withIDs(array($id)); + } + + $policy_exception = null; + try { + $project = $query->executeOne(); + } catch (PhabricatorPolicyException $ex) { + $policy_exception = $ex; + $project = null; + } + + if (!$project) { + // This project legitimately does not exist, so just 404 the user. + if (!$policy_exception) { + return new Aphront404Response(); + } + + // Here, the project exists but the user can't see it. If they are + // using a non-canonical slug to view the project, redirect to the + // canonical slug. If they're already using the canonical slug, rethrow + // the exception to give them the policy error. + if ($is_abnormal) { + return id(new AphrontRedirectResponse())->setURI($normal_uri); + } else { + throw $policy_exception; + } + } + + // The user can view the project, but is using a noncanonical slug. + // Redirect to the canonical slug. + $primary_slug = $project->getPrimarySlug(); + if ($slug && ($slug !== $primary_slug)) { + $primary_uri = "/tag/{$primary_slug}/"; + return id(new AphrontRedirectResponse())->setURI($primary_uri); + } + + $this->setProject($project); + + return null; + } + public function buildApplicationMenu() { return $this->buildSideNavView(true)->getMenu(); } diff --git a/src/applications/project/controller/PhabricatorProjectFeedController.php b/src/applications/project/controller/PhabricatorProjectFeedController.php index efd82d31d0..5cd74094fc 100644 --- a/src/applications/project/controller/PhabricatorProjectFeedController.php +++ b/src/applications/project/controller/PhabricatorProjectFeedController.php @@ -8,38 +8,25 @@ final class PhabricatorProjectFeedController } public function handleRequest(AphrontRequest $request) { - $user = $request->getUser(); + $viewer = $request->getUser(); - $query = id(new PhabricatorProjectQuery()) - ->setViewer($user) - ->needMembers(true) - ->needWatchers(true) - ->needImages(true) - ->needSlugs(true); - $id = $request->getURIData('id'); - $slug = $request->getURIData('slug'); - if ($slug) { - $query->withSlugs(array($slug)); - } else { - $query->withIDs(array($id)); - } - $project = $query->executeOne(); - if (!$project) { - return new Aphront404Response(); - } - if ($slug && $slug != $project->getPrimarySlug()) { - return id(new AphrontRedirectResponse()) - ->setURI('/tag/'.$project->getPrimarySlug().'/'); + $response = $this->loadProject(); + if ($response) { + return $response; } - $query = new PhabricatorFeedQuery(); - $query->setFilterPHIDs( - array( - $project->getPHID(), - )); - $query->setLimit(50); - $query->setViewer($request->getUser()); - $stories = $query->execute(); + $project = $this->getProject(); + $id = $project->getID(); + + $stories = id(new PhabricatorFeedQuery()) + ->setViewer($viewer) + ->setFilterPHIDs( + array( + $project->getPHID(), + )) + ->setLimit(50) + ->execute(); + $feed = $this->renderStories($stories); $box = id(new PHUIObjectBoxView()) @@ -57,21 +44,6 @@ final class PhabricatorProjectFeedController )); } - private function renderFeedPage(PhabricatorProject $project) { - - $query = new PhabricatorFeedQuery(); - $query->setFilterPHIDs(array($project->getPHID())); - $query->setViewer($this->getRequest()->getUser()); - $query->setLimit(100); - $stories = $query->execute(); - - if (!$stories) { - return pht('There are no stories about this project.'); - } - - return $this->renderStories($stories); - } - private function renderStories(array $stories) { assert_instances_of($stories, 'PhabricatorFeedStory'); @@ -85,5 +57,4 @@ final class PhabricatorProjectFeedController $view->render()); } - } diff --git a/src/applications/project/controller/PhabricatorProjectMembersEditController.php b/src/applications/project/controller/PhabricatorProjectMembersEditController.php index c7a9144188..f007650e4a 100644 --- a/src/applications/project/controller/PhabricatorProjectMembersEditController.php +++ b/src/applications/project/controller/PhabricatorProjectMembersEditController.php @@ -12,10 +12,6 @@ final class PhabricatorProjectMembersEditController ->withIDs(array($id)) ->needMembers(true) ->needImages(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - )) ->executeOne(); if (!$project) { return new Aphront404Response(); diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index 89880bdd1a..cc246b5a2d 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -8,35 +8,20 @@ final class PhabricatorProjectProfileController } public function handleRequest(AphrontRequest $request) { - $user = $request->getUser(); + $viewer = $request->getUser(); - $query = id(new PhabricatorProjectQuery()) - ->setViewer($user) - ->needMembers(true) - ->needWatchers(true) - ->needImages(true) - ->needSlugs(true); - $id = $request->getURIData('id'); - $slug = $request->getURIData('slug'); - if ($slug) { - $query->withSlugs(array($slug)); - } else { - $query->withIDs(array($id)); - } - $project = $query->executeOne(); - if (!$project) { - return new Aphront404Response(); - } - if ($slug && $slug != $project->getPrimarySlug()) { - return id(new AphrontRedirectResponse()) - ->setURI('/tag/'.$project->getPrimarySlug().'/'); + $response = $this->loadProject(); + if ($response) { + return $response; } + $project = $this->getProject(); + $id = $project->getID(); $picture = $project->getProfileImageURI(); $header = id(new PHUIHeaderView()) ->setHeader($project->getName()) - ->setUser($user) + ->setUser($viewer) ->setPolicyObject($project) ->setImage($picture); @@ -60,15 +45,13 @@ final class PhabricatorProjectProfileController $nav = $this->buildIconNavView($project); $nav->selectFilter("profile/{$id}/"); - $nav->appendChild($object_box); - $nav->appendChild($timeline); - return $this->buildApplicationPage( - $nav, - array( - 'title' => $project->getName(), - 'pageObjects' => array($project->getPHID()), - )); + return $this->newPage() + ->setNavigation($nav) + ->setTitle($project->getName()) + ->setPageObjectPHIDs(array($project->getPHID())) + ->appendChild($object_box) + ->appendChild($timeline); } private function buildActionListView(PhabricatorProject $project) { diff --git a/src/applications/project/controller/PhabricatorProjectViewController.php b/src/applications/project/controller/PhabricatorProjectViewController.php index bfc9827ab5..087971878b 100644 --- a/src/applications/project/controller/PhabricatorProjectViewController.php +++ b/src/applications/project/controller/PhabricatorProjectViewController.php @@ -11,31 +11,11 @@ final class PhabricatorProjectViewController $request = $this->getRequest(); $viewer = $request->getViewer(); - $query = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->needMembers(true) - ->needWatchers(true) - ->needImages(true) - ->needSlugs(true); - $id = $request->getURIData('id'); - $slug = $request->getURIData('slug'); - if ($slug) { - $query->withSlugs(array($slug)); - } else { - $query->withIDs(array($id)); - } - $project = $query->executeOne(); - if (!$project) { - - // If this request corresponds to a project but just doesn't have the - // slug quite right, redirect to the proper URI. - $uri = $this->getNormalizedURI($slug); - if ($uri !== null) { - return id(new AphrontRedirectResponse())->setURI($uri); - } - - return new Aphront404Response(); + $response = $this->loadProject(); + if ($response) { + return $response; } + $project = $this->getProject(); $columns = id(new PhabricatorProjectColumnQuery()) ->setViewer($viewer) @@ -60,31 +40,4 @@ final class PhabricatorProjectViewController return $this->delegateToController($controller_object); } - private function getNormalizedURI($slug) { - if (!strlen($slug)) { - return null; - } - - $normal = PhabricatorSlug::normalizeProjectSlug($slug); - if ($normal === $slug) { - return null; - } - - $viewer = $this->getViewer(); - - // Do execute() instead of executeOne() here so we canonicalize before - // raising a policy exception. This is a little more polished than letting - // the user hit the error on any variant of the slug. - - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->withSlugs(array($normal)) - ->execute(); - if (!$projects) { - return null; - } - - return "/tag/{$normal}/"; - } - }