fixed/moved specs; made Person.all_from_aspects scope (move direct AR querying...

fixed/moved specs; made Person.all_from_aspects scope (move direct AR querying from AspectStream; added more documentation in AspectStream

app/controllers/aspects_controller.rb

@@ -19,7 +19,7 @@ class AspectsController < ApplicationController
     aspect_ids = (params[:a_ids] ? params[:a_ids] : [])
     @stream = AspectStream.new(current_user, aspect_ids,
                                :order => session[:sort_order],
-                               :max_time => params[:max_time])
+                               :max_time => params[:max_time].to_i)
 
     if params[:only_posts]
       render :partial => 'shared/stream', :locals => {:posts => @stream.posts}

app/models/person.rb

@@ -48,6 +48,14 @@ class Person < ActiveRecord::Base
   scope :local, where('people.owner_id IS NOT NULL')
   scope :for_json, select('DISTINCT people.id, people.diaspora_handle').includes(:profile)
 
+  # @note user is passed in here defensively
+  scope :all_from_aspects, lambda { |aspect_ids, user|
+    joins(:contacts => :aspect_memberships).
+         where(:contacts => {:user_id => user.id},
+               :aspect_memberships => {:aspect_id => aspect_ids}).
+         select("DISTINCT people.*").includes(:profile)
+  }
+
   def self.featured_users
     AppConfig[:featured_users].present? ? Person.where(:diaspora_handle => AppConfig[:featured_users]) : []
   end
@@ -68,13 +76,13 @@ class Person < ActiveRecord::Base
   
   
   def self.find_from_id_or_username(params)
-    p =   if params[:id].present?
-            Person.where(:id => params[:id]).first
-          elsif params[:username].present? && u = User.find_by_username(params[:username])
-            u.person
-          else
-            nil
-          end
+    p = if params[:id].present?
+          Person.where(:id => params[:id]).first
+        elsif params[:username].present? && u = User.find_by_username(params[:username])
+          u.person
+        else
+          nil
+        end
     raise ActiveRecord::RecordNotFound unless p.present?
     p
   end
@@ -126,6 +134,8 @@ class Person < ActiveRecord::Base
     Person.searchable.where(sql, *tokens)
   end
 
+
+
   def name(opts = {})
     if self.profile.nil?
       fix_profile

lib/aspect_stream.rb

@@ -7,12 +7,15 @@ class AspectStream
   attr_reader :max_time, :order
 
   # @param user [User]
+  # @param inputted_aspect_ids [Array<Integer>] Ids of aspects for given stream
   # @param aspect_ids [Array<Integer>] Aspects this stream is responsible for
+  # @opt max_time [Integer] Unix timestamp of stream's post ceiling
+  # @opt order [String] Order of posts (i.e. 'created_at', 'updated_at')
+  # @return [void]
   def initialize(user, inputted_aspect_ids, opts={})
     @user = user
     @inputted_aspect_ids = inputted_aspect_ids
-
-    @max_time = opts[:max_time].to_i
+    @max_time = opts[:max_time]
     @order = opts[:order]
   end
 
@@ -27,7 +30,6 @@ class AspectStream
       a = a.where(:id => @inputted_aspect_ids) if @inputted_aspect_ids.length > 0
       a
     end.call
-    @aspects
   end
 
   # Maps ids into an array from #aspects
@@ -39,9 +41,9 @@ class AspectStream
 
   # @return [ActiveRecord::Association<Post>] AR association of posts
   def posts
-    # NOTE(this should be something like User.aspect_post(@aspect_ids, {}) that calls visible_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,
-                                   :type => ['StatusMessage','Reshare', 'ActivityStreams::Photo'],
+                                   :type => ['StatusMessage', 'Reshare', 'ActivityStreams::Photo'],
                                    :order => "#{@order} DESC",
                                    :max_time => @max_time
                    ).includes(:mentions => {:person => :profile}, :author => :profile)
@@ -49,11 +51,7 @@ class AspectStream
 
   # @return [ActiveRecord::Association<Person>] AR association of people within stream's given aspects
   def people
-    # NOTE(this should call a method in Person
-    @people ||= Person.joins(:contacts => :aspect_memberships).
-                                   where(:contacts => {:user_id => @user.id},
-                                         :aspect_memberships => {:aspect_id => @aspect_ids}).
-                                   select("DISTINCT people.*").includes(:profile)
+    @people ||= Person.all_from_aspects(aspect_ids, @user)
   end
 
   # @note aspects.first is used for mobile. NOTE(this is a hack and should be fixed)
@@ -71,5 +69,4 @@ class AspectStream
   def for_all_aspects?
     @all_aspects ||= aspect_ids.length == @user.aspects.size
   end
-
 end

spec/controllers/aspects_controller_spec.rb

@@ -125,111 +125,44 @@ describe AspectsController do
       end
     end
 
-    context "mobile" do
-      it "renders a share button when you don't pass aspect IDs" do
-        get :index, :format => :mobile
-        response.body.should =~ /#{Regexp.escape('id="status_message_submit"')}/
-      end
-      
-      it "renders a share button when you pass aspect IDs" do
-        get :index, :a_ids => [@alices_aspect_1], :format => :mobile
-        response.body.should =~ /#{Regexp.escape('id="status_message_submit"')}/
-      end
+    it 'renders just the stream with the infinite scroll param set' do
+      get :index, :only_posts => true
+      response.should render_template('shared/_stream')
     end
 
-    context 'with posts in multiple aspects' do
-      before do
-        pending 'this spec needs to be moved into AspectStream spec or visible_posts spec'
-        @posts = []
-        2.times do |n|
-          user = Factory(:user)
-          aspect = user.aspects.create(:name => 'people')
-          connect_users(alice, @alices_aspect_1, user, aspect)
-          target_aspect = n.even? ? @alices_aspect_1 : @alices_aspect_2
-          post = alice.post(:status_message, :text=> "hello#{n}", :to => target_aspect)
-          post.created_at = Time.now - (2 - n).seconds
-          post.save!
-          @posts << post
-        end
-        alice.build_comment(:text => 'lalala', :post => @posts.first).save
-      end
-
-      describe "post visibilities" do
-        before do
-          aspect_to_post = bob.aspects.where(:name => "generic").first
-          @status = bob.post(:status_message, :text=> "hello", :to => aspect_to_post)
-          @vis = @status.post_visibilities.first
-        end
-
-        it "pulls back non hidden posts" do
-          get :index
-          assigns[:posts].include?(@status).should be_true
-        end
-
-        it "does not pull back hidden posts" do
-          @vis.update_attributes(:hidden => true)
-          get :index
-          assigns[:posts].include?(@status).should be_false
-        end
-      end
-
-      describe 'infinite scroll' do
-        it 'renders with the infinite scroll param' do
-          get :index, :only_posts => true
-          assigns[:posts].include?(@posts.first).should be_true
-          response.should be_success
-        end
+    it 'assigns an AspectStream' do
+      get :index
+      assigns(:stream).class.should == AspectStream
+    end
 
+    describe 'filtering by aspect' do
+      before do
+        @aspect1 = alice.aspects.create(:name => "test aspect")
+        @stream = AspectStream.new(alice, [])
+        @stream.stub(:posts).and_return([])
       end
 
-      describe "ordering" do
-        it "orders posts by updated_at by default" do
-          get :index
-          assigns(:posts).should == @posts
-        end
-
-        it "orders posts by created_at on request" do
-          get :index, :sort_order => 'created_at'
-          assigns(:posts).should == @posts.reverse
-        end
-
-        it "remembers your sort order and lets you override the memory" do
-          get :index, :sort_order => "created_at"
-          assigns(:posts).should == @posts.reverse
-          get :index
-          assigns(:posts).should == @posts.reverse
-          get :index, :sort_order => "updated_at"
-          assigns(:posts).should == @posts
-        end
-
-        it "doesn't allow SQL injection" do
-          get :index, :sort_order => "\"; DROP TABLE users;"
-          assigns(:posts).should == @posts
-          get :index, :sort_order => "created_at"
-          assigns(:posts).should == @posts.reverse
-        end
+      it 'respects a single aspect' do
+        AspectStream.should_receive(:new).with(alice, [@aspect1.id], anything).and_return(@stream)
+        get :index, :a_ids => [@aspect1.id]
       end
 
-      it "returns all posts by default" do
-        alice.aspects.reload
-        get :index
-        assigns(:posts).length.should == 2
-      end
-
-      it "posts include reshares" do
-        reshare = alice.post(:reshare, :public => true, :root_guid => Factory(:status_message, :public => true).guid, :to => alice.aspects)
-        get :index
-        assigns[:posts].map { |x| x.id }.should include(reshare.id)
+      it 'respects multiple aspects' do
+        aspect2 = alice.aspects.create(:name => "test aspect two")
+        AspectStream.should_receive(:new).with(alice, [@aspect1.id, aspect2.id], anything).and_return(@stream)
+        get :index, :a_ids => [@aspect1.id, aspect2.id]
       end
+    end
 
-      it "can filter to a single aspect" do
-        get :index, :a_ids => [@alices_aspect_2.id.to_s]
-        assigns(:posts).length.should == 1
+    context "mobile" do
+      it "renders a share button when you don't pass aspect IDs" do
+        get :index, :format => :mobile
+        response.body.should =~ /#{Regexp.escape('id="status_message_submit"')}/
       end
-
-      it "can filter to multiple aspects" do
-        get :index, :a_ids => [@alices_aspect_1.id.to_s, @alices_aspect_2.id.to_s]
-        assigns(:posts).length.should == 2
+      
+      it "renders a share button when you pass aspect IDs" do
+        get :index, :a_ids => [@alices_aspect_1], :format => :mobile
+        response.body.should =~ /#{Regexp.escape('id="status_message_submit"')}/
       end
     end
 

spec/lib/aspect_stream_spec.rb

@@ -52,6 +52,12 @@ describe AspectStream do
       stream.posts
     end
 
+    it 'is called with 3 types' do
+      stream = AspectStream.new(@alice, [1,2], :order => 'created_at')
+      @alice.should_receive(:visible_posts).with(hash_including(:type=> ['StatusMessage', 'Reshare', 'ActivityStreams::Photo'])).and_return(stub.as_null_object)
+      stream.posts
+    end
+
     it 'respects ordering' do 
       stream = AspectStream.new(@alice, [1,2], :order => 'created_at')
       @alice.should_receive(:visible_posts).with(hash_including(:order => 'created_at DESC')).and_return(stub.as_null_object)
@@ -66,7 +72,17 @@ describe AspectStream do
   end
 
   describe '#people' do
-    it 'should call a method on person that doesnt exist yet'
+    it 'should call Person.all_from_aspects' do
+      class Person ; end
+
+      alice = stub.as_null_object
+      aspect_ids = [1,2,3]
+      stream = AspectStream.new(alice, [])
+
+      stream.stub(:aspect_ids).and_return(aspect_ids)
+      Person.should_receive(:all_from_aspects).with(stream.aspect_ids, alice)
+      stream.people
+    end
   end
 
   describe '#aspect' do

spec/models/person_spec.rb

@@ -40,6 +40,7 @@ describe Person do
         person_ids.uniq.should == person_ids
       end
     end
+
     describe '.local' do
       it 'returns only local people' do
         Person.local =~ [@person]
@@ -73,7 +74,25 @@ describe Person do
         }.to raise_error ActiveRecord::RecordNotFound
       end
     end
+
+    describe '.all_from_aspects' do
+      it "pulls back the right people given all a user's aspects" do
+        aspect_ids = bob.aspects.map(&:id)
+        Person.all_from_aspects(aspect_ids, bob).map(&:id).should =~ bob.contacts.includes(:person).map{|c| c.person.id}
+      end
+
+      it "pulls back the right people given a subset of aspects" do
+        aspect_ids = bob.aspects.first.id
+        Person.all_from_aspects(aspect_ids, bob).map(&:id).should =~ bob.aspects.first.contacts.includes(:person).map{|c| c.person.id}
+      end
+
+      it "respects aspects given a user" do
+        aspect_ids = alice.aspects.map(&:id)
+        Person.all_from_aspects(aspect_ids, bob).map(&:id).should == []
+      end
+    end
   end
+
   describe "delegating" do
     it "delegates last_name to the profile" do
       @person.last_name.should == @person.profile.last_name

spec/models/user/querying_spec.rb

@@ -17,38 +17,46 @@ describe User do
       public_post = alice.post(:status_message, :text => "hi", :to => @alices_aspect.id, :public => true)
       alice.visible_posts.should include(public_post)
     end
+
     it "contains your non-public posts" do
       private_post = alice.post(:status_message, :text => "hi", :to => @alices_aspect.id, :public => false)
       alice.visible_posts.should include(private_post)
     end
+
     it "contains public posts from people you're following" do
       dogs = bob.aspects.create(:name => "dogs")
       bobs_public_post = bob.post(:status_message, :text => "hello", :public => true, :to => dogs.id)
       alice.visible_posts.should include(bobs_public_post)
     end
+
     it "contains non-public posts from people who are following you" do
       bobs_post = bob.post(:status_message, :text => "hello", :to => @bobs_aspect.id)
       alice.visible_posts.should include(bobs_post)
     end
+
     it "does not contain non-public posts from aspects you're not in" do
       dogs = bob.aspects.create(:name => "dogs")
       invisible_post = bob.post(:status_message, :text => "foobar", :to => dogs.id)
       alice.visible_posts.should_not include(invisible_post)
     end
+
     it "does not contain pending posts" do
       pending_post = bob.post(:status_message, :text => "hey", :public => true, :to => @bobs_aspect.id, :pending => true)
       pending_post.should be_pending
       alice.visible_posts.should_not include pending_post
     end
+
     it "does not contain pending photos" do
       pending_photo = bob.post(:photo, :pending => true, :user_file=> File.open(photo_fixture_name), :to => @bobs_aspect)
       alice.visible_posts.should_not include pending_photo
     end
+
     it "respects the :type option" do
       photo = bob.post(:photo, :pending => false, :user_file=> File.open(photo_fixture_name), :to => @bobs_aspect)
       alice.visible_posts.should include(photo)
       alice.visible_posts(:type => 'StatusMessage').should_not include(photo)
     end
+
     it "does not contain duplicate posts" do
       bobs_other_aspect = bob.aspects.create(:name => "cat people")
       bob.add_contact_to_aspect(bob.contact_for(alice.person), bobs_other_aspect)
@@ -59,16 +67,35 @@ describe User do
       alice.visible_posts.length.should == 1
       alice.visible_posts.should include(bobs_post)
     end
+
+    describe 'hidden posts' do
+      before do
+        aspect_to_post = bob.aspects.where(:name => "generic").first
+        @status = bob.post(:status_message, :text=> "hello", :to => aspect_to_post)
+        @vis = @status.post_visibilities.first
+      end
+
+      it "pulls back non hidden posts" do
+        alice.visible_posts.include?(@status).should be_true
+      end
+
+      it "does not pull back hidden posts" do
+        @vis.update_attributes(:hidden => true)
+        alice.visible_posts.include?(@status).should be_false
+      end
+    end
+
     context 'with many posts' do
       before do
         bob.move_contact(eve.person, @bobs_aspect, bob.aspects.create(:name => 'new aspect'))
         time_interval = 1000
+        time_past = 1000000
         (1..25).each do |n|
           [alice, bob, eve].each do |u|
             aspect_to_post = u.aspects.where(:name => "generic").first
             post = u.post :status_message, :text => "#{u.username} - #{n}", :to => aspect_to_post.id
-            post.created_at = post.created_at - time_interval
-            post.updated_at = post.updated_at - time_interval
+            post.created_at = (post.created_at-time_past) - time_interval
+            post.updated_at = (post.updated_at-time_past) + time_interval
             post.save
             time_interval += 1000
           end
@@ -79,6 +106,14 @@ describe User do
         bob.visible_posts.should == bob.visible_posts(:by_members_of => bob.aspects.map { |a| a.id }) # it is the same when joining through aspects
         bob.visible_posts.sort_by { |p| p.updated_at }.map { |p| p.id }.should == bob.visible_posts.map { |p| p.id }.reverse #it is sorted updated_at desc by default
 
+        # It should respect the order option
+        opts = {:order => 'created_at DESC'}
+        bob.visible_posts(opts).first.created_at.should > bob.visible_posts(opts).last.created_at
+
+        # It should respect the order option
+        opts = {:order => 'updated_at DESC'}
+        bob.visible_posts(opts).first.updated_at.should > bob.visible_posts(opts).last.updated_at
+        
         # It should respect the limit option
         opts = {:limit => 40}
         bob.visible_posts(opts).length.should == 40