From 97342630c4449fd5daa51100c1c966989d7f266b Mon Sep 17 00:00:00 2001
From: Ilya Zhitomirskiy <iz268@nyu.edu>
Date: Tue, 4 Oct 2011 14:39:56 -0700
Subject: [PATCH] dg iz added some more documentation and only caching on all
 aspects

---
 lib/diaspora/user/querying.rb         |  2 +-
 lib/stream/aspect_stream.rb           | 25 ++++++++++++++++++++++---
 spec/lib/stream/aspect_stream_spec.rb |  8 ++++++++
 spec/models/user/querying_spec.rb     |  8 +++++++-
 4 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/lib/diaspora/user/querying.rb b/lib/diaspora/user/querying.rb
index 3c53b030d..432a929c8 100644
--- a/lib/diaspora/user/querying.rb
+++ b/lib/diaspora/user/querying.rb
@@ -24,7 +24,7 @@ module Diaspora
       def visible_post_ids(opts={})
         opts = prep_opts(opts)
 
-        if AppConfig[:redis_cache] && RedisCache.supported_order?(opts[:order_field])
+        if AppConfig[:redis_cache] && RedisCache.supported_order?(opts[:order_field]) && opts[:all_aspects?].present?
           cache = RedisCache.new(self, opts[:order_field])
 
           cache.ensure_populated!
diff --git a/lib/stream/aspect_stream.rb b/lib/stream/aspect_stream.rb
index 3edb4d0af..69ab65092 100644
--- a/lib/stream/aspect_stream.rb
+++ b/lib/stream/aspect_stream.rb
@@ -39,7 +39,8 @@ class AspectStream < BaseStream
   # @return [ActiveRecord::Association<Post>] AR association of posts
   def posts
     # NOTE(this should be something like Post.all_for_stream(@user, aspect_ids, {}) that calls visible_posts
-    @posts ||= user.visible_posts(:by_members_of => aspect_ids,
+    @posts ||= user.visible_posts(:all_aspects? => for_all_aspects?,
+                                   :by_members_of => aspect_ids,
                                    :type => TYPES_OF_POST_IN_STREAM,
                                    :order => "#{order} DESC",
                                    :max_time => max_time
@@ -51,6 +52,7 @@ class AspectStream < BaseStream
     @people ||= Person.all_from_aspects(aspect_ids, user).includes(:profile)
   end
 
+  # @return [String] URL
   def link(opts={})
     Rails.application.routes.url_helpers.aspects_path(opts.merge(:a_ids => aspect_ids))
   end
@@ -64,16 +66,26 @@ class AspectStream < BaseStream
     end
   end
 
+  # Only ajax in the stream if all aspects are present.
+  # In this case, we know we're on the first page of the stream,
+  # as the default view for aspects/index is showing posts from
+  # all a user's aspects.
+  #
+  # @return [Boolean] see #for_all_aspects?
   def ajax_stream?
     for_all_aspects?
   end
 
+  # The title that will display at the top of the stream's
+  # publisher box.
+  #
+  # @return [String]
   def title
     if self.for_all_aspects?
       I18n.t('aspects.aspect_stream.stream')
-    else 
+    else
       self.aspects.to_sentence
-    end    
+    end
   end
 
   # Determine whether or not the stream is displaying across
@@ -84,6 +96,9 @@ class AspectStream < BaseStream
     @all_aspects ||= aspect_ids.length == user.aspects.size
   end
 
+  # Provides a translated title for contacts box on the right pane.
+  #
+  # @return [String]
   def contacts_title
     if self.for_all_aspects? || self.aspect_ids.size > 1
       I18n.t('_contacts')
@@ -92,6 +107,10 @@ class AspectStream < BaseStream
     end
   end
 
+  # Provides a link to the user to the contacts page that corresponds with
+  # the stream's active aspects.
+  #
+  # @return [String] Link to contacts
   def contacts_link
     if for_all_aspects? || aspect_ids.size > 1
       Rails.application.routes.url_helpers.contacts_path
diff --git a/spec/lib/stream/aspect_stream_spec.rb b/spec/lib/stream/aspect_stream_spec.rb
index 381af67f0..6d9c486ad 100644
--- a/spec/lib/stream/aspect_stream_spec.rb
+++ b/spec/lib/stream/aspect_stream_spec.rb
@@ -73,6 +73,14 @@ describe AspectStream do
       @alice.should_receive(:visible_posts).with(hash_including(:max_time => instance_of(Time))).and_return(stub.as_null_object)
       stream.posts
     end
+
+    it 'passes for_all_aspects to visible posts' do
+      stream = AspectStream.new(@alice, [1,2], :max_time => 123)
+      all_aspects = mock
+      stream.stub(:for_all_aspects?).and_return(all_aspects)
+      @alice.should_receive(:visible_posts).with(hash_including(:all_aspects? => all_aspects)).and_return(stub.as_null_object)
+      stream.posts
+    end
   end
 
   describe '#people' do
diff --git a/spec/models/user/querying_spec.rb b/spec/models/user/querying_spec.rb
index 0cdc999b4..43e4a5eda 100644
--- a/spec/models/user/querying_spec.rb
+++ b/spec/models/user/querying_spec.rb
@@ -88,7 +88,7 @@ describe User do
     context "RedisCache" do
       before do
         AppConfig[:redis_cache] = true
-        @opts = {:order => "created_at DESC"}
+        @opts = {:order => "created_at DESC", :all_aspects? => true}
       end
 
       after do
@@ -103,6 +103,12 @@ describe User do
         alice.visible_post_ids(@opts)
       end
 
+      it 'does not get used if if all_aspects? option is not present' do
+        RedisCache.should_not_receive(:new)
+
+        alice.visible_post_ids(@opts.merge({:all_aspects? => false}))
+      end
+
       describe "#ensure_populated_cache" do
         it 'does nothing if the cache is already populated'
         it 're-populates the cache with the latest posts (in hashes)'