From 1bd37038cc8de16c153ede73bc1d5055e1ece43f Mon Sep 17 00:00:00 2001
From: danielgrippi <danielgrippi@gmail.com>
Date: Sun, 11 Sep 2011 14:16:24 -0700
Subject: [PATCH] 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 | 2 +-
app/models/person.rb | 24 ++--
lib/aspect_stream.rb | 19 ++-
spec/controllers/aspects_controller_spec.rb | 123 +++++---------------
spec/lib/aspect_stream_spec.rb | 18 ++-
spec/models/person_spec.rb | 19 +++
spec/models/user/querying_spec.rb | 39 ++++++-
7 files changed, 127 insertions(+), 117 deletions(-)
diff --git a/app/controllers/aspects_controller.rb b/app/controllers/aspects_controller.rb
index 9757df1a81..93cdd8b8e0 100644
--- a/app/controllers/aspects_controller.rb
+++ b/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}
diff --git a/app/models/person.rb b/app/models/person.rb
index eabe66b9e2..e859c688b5 100644
--- a/app/models/person.rb
+++ b/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
diff --git a/lib/aspect_stream.rb b/lib/aspect_stream.rb
index df8b50dc7b..74e4b87c63 100644
--- a/lib/aspect_stream.rb
+++ b/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
diff --git a/spec/controllers/aspects_controller_spec.rb b/spec/controllers/aspects_controller_spec.rb
index 584ffcf02d..09af7263c7 100644
--- a/spec/controllers/aspects_controller_spec.rb
+++ b/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
diff --git a/spec/lib/aspect_stream_spec.rb b/spec/lib/aspect_stream_spec.rb
index 8753962790..44dc6ae05b 100644
--- a/spec/lib/aspect_stream_spec.rb
+++ b/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
diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb
index 49237d6389..ad9271f3cf 100644
--- a/spec/models/person_spec.rb
+++ b/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
diff --git a/spec/models/user/querying_spec.rb b/spec/models/user/querying_spec.rb
index 9228d72e4c..d682d5d09b 100644
--- a/spec/models/user/querying_spec.rb
+++ b/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
--
GitLab