Consolidate feed query code

Summary: Simplify FeedQuery by making it extend from PhabricatorIDPagedPolicyQuery Test Plan: Looked at feed on home, projects, user profile, and called `feed.query`. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2905
avish · Jul 2, 2012 · 310cf00 · 310cf00
1 parent 3a453f2
commit 310cf00
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 130 deletions.
diff --git a/conf/default.conf.php b/conf/default.conf.php
@@ -1010,6 +1010,9 @@
   // pages using iframes. These feeds are completely public, and a login is not
   // required to view them! This is intended for things like open source
   // projects that want to expose an activity feed on the project homepage.
+  //
+  // NOTE: You must also set `policy.allow-public` to true for this setting
+  // to work properly.
   'feed.public' => false,
 
 
@@ -1020,6 +1023,7 @@
   'amazon-ec2.access-key'   => null,
   'amazon-ec2.secret-key'   => null,
 
+
 // -- Customization --------------------------------------------------------- //
 
   // Paths to additional phutil libraries to load.

diff --git a/src/applications/conduit/method/feed/ConduitAPI_feed_query_Method.php b/src/applications/conduit/method/feed/ConduitAPI_feed_query_Method.php
@@ -88,7 +88,8 @@ protected function execute(ConduitAPIRequest $request) {
     $query = id(new PhabricatorFeedQuery())
       ->setLimit($limit)
       ->setFilterPHIDs($filter_phids)
-      ->setAfter($after);
+      ->setViewer($user)
+      ->setAfterID($after);
     $stories = $query->execute();
 
     if ($stories) {

diff --git a/src/applications/directory/controller/PhabricatorDirectoryMainController.php b/src/applications/directory/controller/PhabricatorDirectoryMainController.php
@@ -401,55 +401,16 @@ private function buildFeedView(array $phids) {
     $user_phid = $user->getPHID();
 
     $feed_query = new PhabricatorFeedQuery();
+    $feed_query->setViewer($user);
     if ($phids) {
       $feed_query->setFilterPHIDs($phids);
     }
 
-    // TODO: All this limit stuff should probably be consolidated into the
-    // feed query?
+    $pager = new AphrontIDPagerView();
+    $pager->readFromRequest($request);
+    $pager->setPageSize(200);
 
-    $old_link = null;
-    $new_link = null;
-
-    $feed_query->setAfter($request->getStr('after'));
-    $feed_query->setBefore($request->getStr('before'));
-    $limit = 500;
-
-    // Grab one more story than we intend to display so we can figure out
-    // if we need to render an "Older Posts" link or not (with reasonable
-    // accuracy, at least).
-    $feed_query->setLimit($limit + 1);
-    $feed = $feed_query->execute();
-    $extra_row = (count($feed) == $limit + 1);
-
-    $have_new = ($request->getStr('before')) ||
-                ($request->getStr('after') && $extra_row);
-
-    $have_old = ($request->getStr('after')) ||
-                ($request->getStr('before') && $extra_row) ||
-                (!$request->getStr('before') &&
-                 !$request->getStr('after') &&
-                 $extra_row);
-    $feed = array_slice($feed, 0, $limit, $preserve_keys = true);
-
-    if ($have_old) {
-      $old_link = phutil_render_tag(
-        'a',
-        array(
-          'href' => '?before='.end($feed)->getChronologicalKey(),
-          'class' => 'phabricator-feed-older-link',
-        ),
-        "Older Stories \xC2\xBB");
-    }
-    if ($have_new) {
-      $new_link = phutil_render_tag(
-        'a',
-        array(
-          'href' => '?after='.reset($feed)->getChronologicalKey(),
-          'class' => 'phabricator-feed-newer-link',
-        ),
-        "\xC2\xAB Newer Stories");
-    }
+    $feed = $feed_query->executeWithPager($pager);
 
     $builder = new PhabricatorFeedBuilder($feed);
     $builder->setUser($user);
@@ -464,8 +425,7 @@ private function buildFeedView(array $phids) {
         '</div>'.
         $feed_view->render().
         '<div class="phabricator-feed-frame">'.
-          $new_link.
-          $old_link.
+          $pager->render().
         '</div>'.
       '</div>';
   }

diff --git a/src/applications/feed/PhabricatorFeedQuery.php b/src/applications/feed/PhabricatorFeedQuery.php
@@ -16,98 +16,74 @@
  * limitations under the License.
  */
 
-final class PhabricatorFeedQuery {
+final class PhabricatorFeedQuery extends PhabricatorIDPagedPolicyQuery {
 
   private $filterPHIDs;
-  private $limit = 100;
-  private $after;
-  private $before;
 
   public function setFilterPHIDs(array $phids) {
     $this->filterPHIDs = $phids;
     return $this;
   }
 
-  public function setLimit($limit) {
-    $this->limit = $limit;
-    return $this;
-  }
+  public function loadPage() {
 
-  public function setAfter($after) {
-    $this->after = $after;
-    return $this;
-  }
+    $story_table = new PhabricatorFeedStoryData();
+    $conn = $story_table->establishConnection('r');
 
-  public function setBefore($before) {
-    $this->before = $before;
-    return $this;
+    $data = queryfx_all(
+      $conn,
+      'SELECT story.* FROM %T story %Q %Q %Q %Q %Q',
+      $story_table->getTableName(),
+      $this->buildJoinClause($conn),
+      $this->buildWhereClause($conn),
+      $this->buildGroupClause($conn),
+      $this->buildOrderClause($conn),
+      $this->buildLimitClause($conn));
+
+    $results = PhabricatorFeedStory::loadAllFromRows($data);
+
+    return $this->processResults($results);
   }
 
-  public function execute() {
+  private function buildJoinClause(AphrontDatabaseConnection $conn_r) {
+    // NOTE: We perform this join unconditionally (even if we have no filter
+    // PHIDs) to omit rows which have no story references. These story data
+    // rows are notifications or realtime alerts.
 
     $ref_table = new PhabricatorFeedStoryReference();
-    $story_table = new PhabricatorFeedStoryData();
-
-    $conn = $story_table->establishConnection('r');
+    return qsprintf(
+      $conn_r,
+      'JOIN %T ref ON ref.chronologicalKey = story.chronologicalKey',
+      $ref_table->getTableName());
+  }
 
+  private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
     $where = array();
+
     if ($this->filterPHIDs) {
       $where[] = qsprintf(
-        $conn,
+        $conn_r,
         'ref.objectPHID IN (%Ls)',
         $this->filterPHIDs);
     }
 
-    // For "before" queries, we can just add a constraint to the WHERE clause.
-    // For "after" queries, we must also reverse the result ordering, since
-    // otherwise we'll always grab the first page of results if there's a limit.
-    // After MySQL applies the limit, we reverse the page in PHP (below) to
-    // ensure consistent ordering.
-
-    $order = 'DESC';
-
-    if ($this->after) {
-      $where[] = qsprintf(
-        $conn,
-        'ref.chronologicalKey > %s',
-        $this->after);
-      $order = 'ASC';
-    }
+    $where[] = $this->buildPagingClause($conn_r);
 
-    if ($this->before) {
-      $where[] = qsprintf(
-        $conn,
-        'ref.chronologicalKey < %s',
-        $this->before);
-    }
+    return $this->formatWhereClause($where);
+  }
 
-    if ($where) {
-      $where = 'WHERE ('.implode(') AND (', $where).')';
-    } else {
-      $where = '';
-    }
+  private function buildGroupClause(AphrontDatabaseConnection $conn_r) {
+    return qsprintf(
+      $conn_r,
+      'GROUP BY ref.chronologicalKey');
+  }
 
-    $data = queryfx_all(
-      $conn,
-      'SELECT story.* FROM %T ref
-        JOIN %T story ON ref.chronologicalKey = story.chronologicalKey
-        %Q
-        GROUP BY ref.chronologicalKey
-        ORDER BY ref.chronologicalKey %Q
-        LIMIT %d',
-      $ref_table->getTableName(),
-      $story_table->getTableName(),
-      $where,
-      $order,
-      $this->limit);
-
-    if ($order != 'DESC') {
-      // If we did order ASC to pull 'after' data, reverse the result set so
-      // that stories are returned in a consistent (descending) order.
-      $data = array_reverse($data);
-    }
+  protected function getPagingColumn() {
+    return 'ref.chronologicalKey';
+  }
 
-    return PhabricatorFeedStory::loadAllFromRows($data);
+  protected function getPagingValue($item) {
+    return $item->getChronologicalKey();
   }
 
 }
diff --git a/src/applications/feed/controller/PhabricatorFeedPublicStreamController.php b/src/applications/feed/controller/PhabricatorFeedPublicStreamController.php
@@ -24,22 +24,21 @@ public function shouldRequireLogin() {
   }
 
   public function processRequest() {
-
     if (!PhabricatorEnv::getEnvConfig('feed.public')) {
       return new Aphront404Response();
     }
 
-    // TODO: Profile images won't render correctly for logged-out users.
-
     $request = $this->getRequest();
+    $viewer = $request->getUser();
 
     $query = new PhabricatorFeedQuery();
+    $query->setViewer($viewer);
     $stories = $query->execute();
 
     $builder = new PhabricatorFeedBuilder($stories);
     $builder
       ->setFramed(true)
-      ->setUser($request->getUser());
+      ->setUser($viewer);
 
     $view = $builder->buildView();
 

diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php
@@ -23,7 +23,7 @@
  *
  * @task load Loading Stories
  */
-abstract class PhabricatorFeedStory {
+abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface {
 
   private $data;
   private $hasViewed;
@@ -74,6 +74,21 @@ public static function loadAllFromRows(array $rows) {
     return $stories;
   }
 
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    return PhabricatorEnv::getEnvConfig('feed.public')
+      ? PhabricatorPolicies::POLICY_PUBLIC
+      : PhabricatorPolicies::POLICY_USER;
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return false;
+  }
 
   public function setPrimaryObjectPHID($primary_object_phid) {
     $this->primaryObjectPHID = $primary_object_phid;

diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php
@@ -204,15 +204,18 @@ private function renderBasicInformation($user, $profile) {
   }
 
   private function renderUserFeed(PhabricatorUser $user) {
+    $viewer = $this->getRequest()->getUser();
+
     $query = new PhabricatorFeedQuery();
     $query->setFilterPHIDs(
       array(
         $user->getPHID(),
       ));
+    $query->setViewer($viewer);
     $stories = $query->execute();
 
     $builder = new PhabricatorFeedBuilder($stories);
-    $builder->setUser($this->getRequest()->getUser());
+    $builder->setUser($viewer);
     $view = $builder->buildView();
 
     return

diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php
@@ -78,6 +78,7 @@ public function processRequest() {
           array(
             $project->getPHID(),
           ));
+        $query->setViewer($this->getRequest()->getUser());
         $stories = $query->execute();
 
         $content .= $this->renderStories($stories);
@@ -243,19 +244,13 @@ private function renderFeedPage(
 
     $query = new PhabricatorFeedQuery();
     $query->setFilterPHIDs(array($project->getPHID()));
+    $query->setViewer($this->getRequest()->getUser());
     $stories = $query->execute();
 
     if (!$stories) {
       return 'There are no stories about this project.';
     }
 
-    $query = new PhabricatorFeedQuery();
-    $query->setFilterPHIDs(
-      array(
-        $project->getPHID(),
-      ));
-    $stories = $query->execute();
-
     return $this->renderStories($stories);
   }
 

diff --git a/src/infrastructure/query/policy/PhabricatorIDPagedPolicyQuery.php b/src/infrastructure/query/policy/PhabricatorIDPagedPolicyQuery.php
@@ -65,13 +65,13 @@ final protected function buildPagingClause(
     if ($this->beforeID) {
       return qsprintf(
         $conn_r,
-        '%C > %s',
+        '%Q > %s',
         $this->getPagingColumn(),
         $this->beforeID);
     } else if ($this->afterID) {
       return qsprintf(
         $conn_r,
-        '%C < %s',
+        '%Q < %s',
         $this->getPagingColumn(),
         $this->afterID);
     }
@@ -83,12 +83,12 @@ final protected function buildOrderClause(AphrontDatabaseConnection $conn_r) {
     if ($this->beforeID) {
       return qsprintf(
         $conn_r,
-        'ORDER BY %C ASC',
+        'ORDER BY %Q ASC',
         $this->getPagingColumn());
     } else {
       return qsprintf(
         $conn_r,
-        'ORDER BY %C DESC',
+        'ORDER BY %Q DESC',
         $this->getPagingColumn());
     }
   }