From c49c76e2a62a5aee991f01d57a4d5c1d1e214aea Mon Sep 17 00:00:00 2001
From: danielgrippi <danielgrippi@gmail.com>
Date: Wed, 19 Oct 2011 15:47:11 -0700
Subject: [PATCH] DG MS; some refactoring
---
app/models/post.rb | 9 +--
app/models/status_message.rb | 10 ----
lib/diaspora/shareable.rb | 22 +++++++
lib/diaspora/user/querying.rb | 35 ++++++++---
spec/models/post_spec.rb | 94 +++++++++++++++++++++++++-----
spec/models/status_message_spec.rb | 32 +---------
spec/models/user/querying_spec.rb | 12 +++-
7 files changed, 138 insertions(+), 76 deletions(-)
diff --git a/app/models/post.rb b/app/models/post.rb
index 9dd5c8a869..559564e695 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -24,15 +24,10 @@ class Post < ActiveRecord::Base
scope :includes_for_a_stream, includes(:o_embed_cache, {:author => :profile}, :mentions => {:person => :profile}) #note should include root and photos, but i think those are both on status_message
def self.for_a_stream(max_time, order)
- by_max_time(max_time, order).
- includes_for_a_stream.
- where(:type => Stream::Base::TYPES_OF_POST_IN_STREAM).
- limit(15)
+ self.for_visible_shareable_sql(max_time, order).
+ includes_for_a_stream
end
- def self.by_max_time(max_time, order='created_at')
- where("posts.#{order} < ?", max_time).order("posts.#{order} desc")
- end
#############
def self.diaspora_initialize params
diff --git a/app/models/status_message.rb b/app/models/status_message.rb
index 9601479aeb..2860afab9f 100644
--- a/app/models/status_message.rb
+++ b/app/models/status_message.rb
@@ -35,16 +35,6 @@ class StatusMessage < Post
#scopes
scope :where_person_is_mentioned, lambda{|person| joins(:mentions).where(:mentions => {:person_id => person.id})}
- def self.owned_or_visible_by_user(user)
- joins("LEFT OUTER JOIN share_visibilities ON share_visibilities.shareable_id = posts.id AND share_visibilities.shareable_type = 'Post'").
- joins("LEFT OUTER JOIN contacts ON contacts.id = share_visibilities.contact_id").
- where(Contact.arel_table[:user_id].eq(user.id).or(
- StatusMessage.arel_table[:public].eq(true).or(
- StatusMessage.arel_table[:author_id].eq(user.person.id)
- )
- )).select('DISTINCT posts.*')
- end
-
def self.tag_stream(user, tag_array, max_time, order)
owned_or_visible_by_user(user).
joins(:tags).where(:tags => {:name => tag_array}).
diff --git a/lib/diaspora/shareable.rb b/lib/diaspora/shareable.rb
index e91658b20a..1ba5746ef8 100644
--- a/lib/diaspora/shareable.rb
+++ b/lib/diaspora/shareable.rb
@@ -25,6 +25,28 @@ module Diaspora
#scopes
scope :all_public, where(:public => true, :pending => false)
+ def self.owned_or_visible_by_user(user)
+ self.joins("LEFT OUTER JOIN share_visibilities ON share_visibilities.shareable_id = posts.id AND share_visibilities.shareable_type = 'Post'").
+ joins("LEFT OUTER JOIN contacts ON contacts.id = share_visibilities.contact_id").
+ where(Contact.arel_table[:user_id].eq(user.id).or(
+ self.arel_table[:public].eq(true).or(
+ self.arel_table[:author_id].eq(user.person.id)
+ )
+ )
+ ).
+ select("DISTINCT #{self.table_name}.*")
+ end
+
+ def self.for_visible_shareable_sql(max_time, order, limit = 15, types = Stream::Base::TYPES_OF_POST_IN_STREAM)
+ by_max_time(max_time, order).
+ where(:type => types).
+ limit(limit)
+ end
+
+ def self.by_max_time(max_time, order='created_at')
+ where("#{self.table_name}.#{order} < ?", max_time).order("#{self.table_name}.#{order} desc")
+ end
+
xml_attr :diaspora_handle
xml_attr :public
xml_attr :created_at
diff --git a/lib/diaspora/user/querying.rb b/lib/diaspora/user/querying.rb
index 18b23cc871..d6c3b7274c 100644
--- a/lib/diaspora/user/querying.rb
+++ b/lib/diaspora/user/querying.rb
@@ -48,26 +48,43 @@ module Diaspora
def visible_shareable_sql(klass, opts={})
table = klass.table_name
opts = prep_opts(klass, opts)
+ opts[:klass] = klass
+
+ shareable_from_others = construct_shareable_from_others_query(opts)
+ shareable_from_self = construct_shareable_from_self_query(opts)
+
+ "(#{shareable_from_others.to_sql} LIMIT #{opts[:limit]}) UNION ALL (#{shareable_from_self.to_sql} LIMIT #{opts[:limit]}) ORDER BY #{opts[:order]} LIMIT #{opts[:limit]}"
+ end
+
+ def ugly_select_clause(query, opts)
+ klass = opts[:klass]
select_clause ='DISTINCT %s.id, %s.updated_at AS updated_at, %s.created_at AS created_at' % [klass.table_name, klass.table_name, klass.table_name]
+ query.select(select_clause).order(opts[:order_with_table]).where(klass.arel_table[opts[:order_field]].lt(opts[:max_time]))
+ end
+ def construct_shareable_from_others_query(opts)
conditions = {:pending => false, :share_visibilities => {:hidden => opts[:hidden]}, :contacts => {:user_id => self.id} }
conditions[:type] = opts[:type] if opts.has_key?(:type)
- shareable_from_others = klass.joins(:contacts).where(conditions)
+ query = opts[:klass].joins(:contacts).where(conditions)
+ if opts[:by_members_of]
+ query = query.joins(:contacts => :aspect_memberships).where(
+ :aspect_memberships => {:aspect_id => opts[:by_members_of]})
+ end
+
+ ugly_select_clause(query, opts)
+ end
+
+ def construct_shareable_from_self_query(opts)
conditions = {:pending => false }
conditions[:type] = opts[:type] if opts.has_key?(:type)
- shareable_from_self = self.person.send(klass.to_s.tableize).where(conditions)
+ query = self.person.send(opts[:klass].to_s.tableize).where(conditions)
if opts[:by_members_of]
- shareable_from_others = shareable_from_others.joins(:contacts => :aspect_memberships).where(
- :aspect_memberships => {:aspect_id => opts[:by_members_of]})
- shareable_from_self = shareable_from_self.joins(:aspect_visibilities).where(:aspect_visibilities => {:aspect_id => opts[:by_members_of]})
+ query = query.joins(:aspect_visibilities).where(:aspect_visibilities => {:aspect_id => opts[:by_members_of]})
end
- shareable_from_others = shareable_from_others.select(select_clause).order(opts[:order_with_table]).where(klass.arel_table[opts[:order_field]].lt(opts[:max_time]))
- shareable_from_self = shareable_from_self.select(select_clause).order(opts[:order_with_table]).where(klass.arel_table[opts[:order_field]].lt(opts[:max_time]))
-
- "(#{shareable_from_others.to_sql} LIMIT #{opts[:limit]}) UNION ALL (#{shareable_from_self.to_sql} LIMIT #{opts[:limit]}) ORDER BY #{opts[:order]} LIMIT #{opts[:limit]}"
+ ugly_select_clause(query, opts)
end
def contact_for(person)
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
index 43da5b8e39..648a0c24b6 100644
--- a/spec/models/post_spec.rb
+++ b/spec/models/post_spec.rb
@@ -11,7 +11,56 @@ describe Post do
end
describe 'scopes' do
+ describe '.owned_or_visible_by_user' do
+ before do
+ @you = bob
+ @public_post = Factory(:status_message, :public => true)
+ @your_post = Factory(:status_message, :author => @you.person)
+ @post_from_contact = eve.post(:status_message, :text => 'wooo', :to => eve.aspects.where(:name => 'generic').first)
+ @post_from_stranger = Factory(:status_message, :public => false)
+ end
+
+ it 'returns post from your contacts' do
+ StatusMessage.owned_or_visible_by_user(@you).should include(@post_from_contact)
+ end
+
+ it 'returns your posts' do
+ StatusMessage.owned_or_visible_by_user(@you).should include(@your_post)
+ end
+
+ it 'returns public posts' do
+ StatusMessage.owned_or_visible_by_user(@you).should include(@public_post)
+ end
+
+ it 'returns public post from your contact' do
+ sm = Factory(:status_message, :author => eve.person, :public => true)
+
+ StatusMessage.owned_or_visible_by_user(@you).should include(sm)
+ end
+
+ it 'does not return non contacts, non-public post' do
+ StatusMessage.owned_or_visible_by_user(@you).should_not include(@post_from_stranger)
+ end
+
+ it 'should return the three visible posts' do
+ StatusMessage.owned_or_visible_by_user(@you).count.should == 3
+ end
+ end
+
describe '.for_a_stream' do
+ it 'calls #for_visible_shareable_sql' do
+ time, order = stub, stub
+ Post.should_receive(:for_visible_shareable_sql).with(time, order).and_return(Post)
+ Post.for_a_stream(time, order)
+ end
+
+ it 'calls includes_for_a_stream' do
+ Post.should_receive(:includes_for_a_stream)
+ Post.for_a_stream(stub, stub)
+ end
+ end
+
+ context 'having some posts' do
before do
time_interval = 1000
time_past = 1000000
@@ -26,28 +75,41 @@ describe Post do
end
end
- it 'returns the posts ordered and limited by unix time' do
- Post.for_a_stream(Time.now + 1, "created_at").should == @posts
- Post.for_a_stream(Time.now + 1, "updated_at").should == @posts.reverse
+ describe '.by_max_time' do
+ it 'respects time and order' do
+ end
+
+ it 'returns the posts ordered and limited by unix time' do
+ Post.for_a_stream(Time.now + 1, "created_at").should == @posts
+ Post.for_a_stream(Time.now + 1, "updated_at").should == @posts.reverse
+ end
end
- it 'includes everything in .includes_for_a_stream' do
- Post.should_receive(:includes_for_a_stream)
- Post.for_a_stream(Time.now + 1, "created_at")
+
+ describe '.for_visible_shareable_sql' do
+ it 'calls max_time' do
+ time = Time.now + 1
+ Post.should_receive(:by_max_time).with(time, 'created_at').and_return(Post)
+ Post.for_visible_shareable_sql(time, 'created_at')
+ end
+
+ it 'defaults to 15 posts' do
+ chain = stub.as_null_object
+
+ Post.stub(:by_max_time).and_return(chain)
+ chain.should_receive(:limit).with(15).and_return(Post)
+ Post.for_visible_shareable_sql(Time.now + 1, "created_at")
+ end
+
+ it 'respects the type option'
end
- it 'is limited to 15 posts' do
- Post.stub(:by_max_time).and_return(Post)
- Post.stub(:includes_for_a_stream).and_return(stub(:where => Post))
- Post.should_receive(:limit)
- Post.for_a_stream(Time.now + 1, "created_at")
+
+ describe 'includes for a stream' do
+ it 'inclues author profile and mentions'
+ it 'should include photos and root of reshares(but does not)'
end
- end
- describe 'includes for a stream' do
- it 'inclues author profile and mentions'
- it 'should include photos and root of reshares(but does not)'
end
-
end
diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb
index 849fa796b4..92c591f32c 100644
--- a/spec/models/status_message_spec.rb
+++ b/spec/models/status_message_spec.rb
@@ -26,38 +26,8 @@ describe StatusMessage do
Factory.create(:status_message, :text => @test_string )
Factory.create(:status_message, :text => @test_string )
Factory(:status_message)
-
- StatusMessage.where_person_is_mentioned(@bo).count.should == 2
- end
- end
-
- describe '.owned_or_visible_by_user' do
- before do
- @you = bob
- @public_post = Factory(:status_message, :public => true)
- @your_post = Factory(:status_message, :author => @you.person)
- @post_from_contact = eve.post(:status_message, :text => 'wooo', :to => eve.aspects.where(:name => 'generic').first)
- @post_from_stranger = Factory(:status_message, :public => false)
- end
-
- it 'returns post from your contacts' do
- StatusMessage.owned_or_visible_by_user(@you).should include(@post_from_contact)
- end
- it 'returns your posts' do
- StatusMessage.owned_or_visible_by_user(@you).should include(@your_post)
- end
-
- it 'returns public posts' do
- StatusMessage.owned_or_visible_by_user(@you).should include(@public_post)
- end
-
- it 'does not return non contacts, non-public post' do
- StatusMessage.owned_or_visible_by_user(@you).should_not include(@post_from_stranger)
- end
-
- it 'should return the three visible posts' do
- StatusMessage.owned_or_visible_by_user(@you).count.should == 3
+ StatusMessage.where_person_is_mentioned(@bo).count.should == 2
end
end
end
diff --git a/spec/models/user/querying_spec.rb b/spec/models/user/querying_spec.rb
index 90c94b2fd8..c028aa01d3 100644
--- a/spec/models/user/querying_spec.rb
+++ b/spec/models/user/querying_spec.rb
@@ -24,8 +24,10 @@ describe User do
end
it "contains public posts from people you're following" do
+ pending
dogs = bob.aspects.create(:name => "dogs")
- bobs_public_post = bob.post(:status_message, :text => "hello", :public => true, :to => dogs.id)
+ bobs_public_post = Factory(:status_message, :text => "hello", :public => true, :author => bob.person)
+
alice.visible_shareable_ids(Post).should include(bobs_public_post.id)
end
@@ -158,6 +160,10 @@ describe User do
end
describe "#visible_shareables" do
+ it 'never contains posts from people not in your aspects' do
+ Factory(:status_message, :public => true)
+ bob.visible_shareables(Post).count.should == 0
+ end
context 'with many posts' do
before do
bob.move_contact(eve.person, @bobs_aspect, bob.aspects.create(:name => 'new aspect'))
@@ -177,7 +183,7 @@ describe User do
it 'works' do # The set up takes a looong time, so to save time we do several tests in one
bob.visible_shareables(Post).length.should == 15 #it returns 15 by default
- bob.visible_shareables(Post).should == bob.visible_shareables(Post, :by_members_of => bob.aspects.map { |a| a.id }) # it is the same when joining through aspects
+ bob.visible_shareables(Post).map(&:id).should == bob.visible_shareables(Post, :by_members_of => bob.aspects.map { |a| a.id }).map(&:id) # it is the same when joining through aspects
# checks the default sort order
bob.visible_shareables(Post).sort_by { |p| p.created_at }.map { |p| p.id }.should == bob.visible_shareables(Post).map { |p| p.id }.reverse #it is sorted updated_at desc by default
@@ -193,7 +199,7 @@ describe User do
# It should respect the limit option
opts = {:limit => 40}
bob.visible_shareables(Post, opts).length.should == 40
- bob.visible_shareables(Post, opts).should == bob.visible_shareables(Post, opts.merge(:by_members_of => bob.aspects.map { |a| a.id }))
+ bob.visible_shareables(Post, opts).map(&:id).should == bob.visible_shareables(Post, opts.merge(:by_members_of => bob.aspects.map { |a| a.id })).map(&:id)
bob.visible_shareables(Post, opts).sort_by { |p| p.created_at }.map { |p| p.id }.should == bob.visible_shareables(Post, opts).map { |p| p.id }.reverse
# It should paginate using a datetime timestamp
--
GitLab