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

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();
   }

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())
+    $response = $this->loadProject();
-      ->setViewer($user)
+    if ($response) {
-      ->needMembers(true)
+      return $response;
-      ->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().'/');
     }
 
-    $query = new PhabricatorFeedQuery();
+    $project = $this->getProject();
-    $query->setFilterPHIDs(
+    $id = $project->getID();
-      array(
+
-        $project->getPHID(),
+    $stories = id(new PhabricatorFeedQuery())
-      ));
+      ->setViewer($viewer)
-    $query->setLimit(50);
+      ->setFilterPHIDs(
-    $query->setViewer($request->getUser());
+        array(
-    $stories = $query->execute();
+          $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());
   }
 
-
 }

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();

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())
+    $response = $this->loadProject();
-      ->setViewer($user)
+    if ($response) {
-      ->needMembers(true)
+      return $response;
-      ->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().'/');
     }
 
+    $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(
+    return $this->newPage()
-      $nav,
+      ->setNavigation($nav)
-      array(
+      ->setTitle($project->getName())
-        'title' => $project->getName(),
+      ->setPageObjectPHIDs(array($project->getPHID()))
-        'pageObjects' => array($project->getPHID()),
+      ->appendChild($object_box)
-      ));
+      ->appendChild($timeline);
   }
 
   private function buildActionListView(PhabricatorProject $project) {

src/applications/project/controller/PhabricatorProjectViewController.php

@@ -11,31 +11,11 @@ final class PhabricatorProjectViewController
     $request = $this->getRequest();
     $viewer = $request->getViewer();
 
-    $query = id(new PhabricatorProjectQuery())
+    $response = $this->loadProject();
-      ->setViewer($viewer)
+    if ($response) {
-      ->needMembers(true)
+      return $response;
-      ->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();
     }
+    $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}/";
-  }
-
 }