From eaed3505e4e221b0ff3e295e6381f14e8ee35f5d Mon Sep 17 00:00:00 2001 From: danielgrippi Date: Tue, 24 Jan 2012 16:28:54 -0800 Subject: [PATCH] Don't create share_visibilities for public posts, sans migration of deleting old data. --- app/models/share_visibility.rb | 15 ++++++++-- lib/diaspora/shareable.rb | 8 ------ lib/diaspora/user/connecting.rb | 6 ---- lib/diaspora/user/querying.rb | 25 ++++++++++++++--- spec/factories.rb | 18 ++++++------ spec/integration/receiving_spec.rb | 42 ++-------------------------- spec/models/share_visibility_spec.rb | 6 ++++ spec/models/user/connecting_spec.rb | 2 +- 8 files changed, 52 insertions(+), 70 deletions(-) diff --git a/app/models/share_visibility.rb b/app/models/share_visibility.rb index b46fb3342..8ca3ee8b6 100644 --- a/app/models/share_visibility.rb +++ b/app/models/share_visibility.rb @@ -6,19 +6,23 @@ class ShareVisibility < ActiveRecord::Base belongs_to :contact belongs_to :shareable, :polymorphic => :true - scope :for_a_users_contacts, lambda { |user| + scope :for_a_users_contacts, lambda { |user| where(:contact_id => user.contacts.map {|c| c.id}) } - scope :for_contacts_of_a_person, lambda { |person| + scope :for_contacts_of_a_person, lambda { |person| where(:contact_id => person.contacts.map {|c| c.id}) } + validate :not_public + # Perform a batch import, given a set of contacts and a shareable # @note performs a bulk insert in mySQL; performs linear insertions in postgres # @param contacts [Array] Recipients # @param share [Shareable] # @return [void] def self.batch_import(contact_ids, share) + return false unless ShareVisibility.new(:shareable_id => share.id, :shareable_type => share.class.to_s).valid? + if postgres? contact_ids.each do |contact_id| ShareVisibility.find_or_create_by_contact_id_and_shareable_id_and_shareable_type(contact_id, share.id, share.class.base_class.to_s) @@ -30,4 +34,11 @@ class ShareVisibility < ActiveRecord::Base ShareVisibility.import([:contact_id, :shareable_id, :shareable_type], new_share_visibilities_data) end end + + private + def not_public + if shareable.public? + errors[:base] << "Cannot create visibility for a public object" + end + end end diff --git a/lib/diaspora/shareable.rb b/lib/diaspora/shareable.rb index 44dce5891..fb7f3c12e 100644 --- a/lib/diaspora/shareable.rb +++ b/lib/diaspora/shareable.rb @@ -61,14 +61,6 @@ module Diaspora write_attribute(:diaspora_handle, nd) end - def user_refs - if AspectVisibility.exists?(:shareable_id => self.id, :shareable_type => self.class.base_class.to_s) - self.share_visibilities.count + 1 - else - self.share_visibilities.count - end - end - # @param [User] user The user that is receiving this shareable. # @param [Person] person The person who dispatched this shareable to the # @return [void] diff --git a/lib/diaspora/user/connecting.rb b/lib/diaspora/user/connecting.rb index 47ef5f07a..c52dea5f7 100644 --- a/lib/diaspora/user/connecting.rb +++ b/lib/diaspora/user/connecting.rb @@ -50,12 +50,6 @@ module Diaspora else contact.update_attributes(:receiving => false) end - - posts.each do |p| - if p.user_refs < 1 - p.destroy - end - end end def disconnect(bad_contact, opts={}) diff --git a/lib/diaspora/user/querying.rb b/lib/diaspora/user/querying.rb index fea5c6e4b..9e11314a0 100644 --- a/lib/diaspora/user/querying.rb +++ b/lib/diaspora/user/querying.rb @@ -47,7 +47,11 @@ module Diaspora # @return [Array] def visible_ids_from_sql(klass, opts={}) opts = prep_opts(klass, opts) - klass.connection.select_values(visible_shareable_sql(klass, opts)).map { |id| id.to_i } + opts[:klass] = klass + opts[:by_members_of] ||= self.aspects.map{|a| a.id} + + post_ids = klass.connection.select_values(visible_shareable_sql(klass, opts)).map { |id| id.to_i } + post_ids += klass.connection.select_values(construct_public_followings_sql(opts).to_sql).map {|id| id.to_i } end def visible_shareable_sql(klass, opts={}) @@ -80,6 +84,19 @@ module Diaspora ugly_select_clause(query, opts) end + def construct_public_followings_sql(opts) + aspects = Aspect.where(:id => opts[:by_members_of]) + person_ids = people_in_aspects(aspects).map{|p| p.id} + + query = opts[:klass].where(:author_id => person_ids, :public => true, :pending => false) + + unless(opts[:klass] == Photo) + query = query.where(:type => opts[:type]) + 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) @@ -131,9 +148,9 @@ module Diaspora end def contacts_in_aspects aspects - aspects.inject([]) do |contacts,aspect| - contacts | aspect.contacts - end + aspect_ids = aspects.map{|a| a.id} + Contact.joins(:aspect_memberships => :aspect). + where(Aspect.arel_table[:id].in(aspect_ids)) end def posts_from(person) diff --git a/spec/factories.rb b/spec/factories.rb index 25f8ab5aa..3526097cf 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -38,14 +38,14 @@ FactoryGirl.define do end end - factory :account_deletion do + factory :account_deletion do association :person after_build do |delete| delete.diaspora_handle = delete.person.diaspora_handle end end - factory :searchable_person, :parent => :person do + factory :searchable_person, :parent => :person do after_build do |person| person.profile = Factory.build(:profile, :person => person, :searchable => true) end @@ -92,7 +92,7 @@ FactoryGirl.define do end end - factory(:status_message_with_photo, :parent => :status_message) do + factory(:status_message_with_photo, :parent => :status_message) do sequence(:text) { |n| "There are #{n} ninjas in this photo." } after_build do |sm| Factory(:photo, :author => sm.author, :status_message => sm, :pending => false, :public => public) @@ -139,7 +139,7 @@ FactoryGirl.define do sequence(:access_secret) { |token| "98765#{token}" } end - factory :service_user do + factory :service_user do sequence(:uid) { |id| "a#{id}"} sequence(:name) { |num| "Rob Fergus the #{num.ordinalize}" } association :service @@ -162,7 +162,7 @@ FactoryGirl.define do end end - factory(:activity_streams_photo, :class => ActivityStreams::Photo) do + factory(:activity_streams_photo, :class => ActivityStreams::Photo) do association(:author, :factory => :person) image_url "#{AppConfig[:pod_url]}/images/asterisk.png" image_height 154 @@ -174,7 +174,7 @@ FactoryGirl.define do public true end - factory(:app, :class => OAuth2::Provider.client_class) do + factory(:app, :class => OAuth2::Provider.client_class) do sequence(:name) { |token| "Chubbies#{token}" } sequence(:application_base_url) { |token| "http://chubbi#{token}.es/" } @@ -189,7 +189,7 @@ FactoryGirl.define do association(:resource_owner, :factory => :user) end - factory(:oauth_access_token, :class => OAuth2::Provider.access_token_class) do + factory(:oauth_access_token, :class => OAuth2::Provider.access_token_class) do association(:authorization, :factory => :oauth_authorization) end @@ -197,7 +197,7 @@ FactoryGirl.define do name "partytimeexcellent" end - factory(:tag_following) do + factory(:tag_following) do association(:tag, :factory => :tag) association(:user, :factory => :user) end @@ -211,4 +211,4 @@ FactoryGirl.define do association(:person, :factory => :person) association(:post, :factory => :status_message) end -end \ No newline at end of file +end diff --git a/spec/integration/receiving_spec.rb b/spec/integration/receiving_spec.rb index 662d58f15..0645f41b8 100644 --- a/spec/integration/receiving_spec.rb +++ b/spec/integration/receiving_spec.rb @@ -149,8 +149,8 @@ describe 'a user receives a post' do alice.visible_shareables(Post).should_not include @status_message end - context 'dependant delete' do - before do + context 'dependent delete' do + it 'deletes share_visibilities on disconnected by' do @person = Factory(:person) alice.contacts.create(:person => @person, :aspects => [@alices_aspect]) @@ -161,50 +161,12 @@ describe 'a user receives a post' do @contact.share_visibilities.reset @contact.posts(true).should include(@post) @post.share_visibilities.reset - end - it 'deletes a post if the no one links to it' do - lambda { - alice.disconnected_by(@person) - }.should change(Post, :count).by(-1) - end - - it 'deletes share_visibilities on disconnected by' do lambda { alice.disconnected_by(@person) }.should change{@post.share_visibilities(true).count}.by(-1) end end - - it 'should keep track of user references for one person ' do - @status_message.reload - @status_message.user_refs.should == 3 - @status_message.contacts(true).should include(@contact) - - alice.remove_contact(@contact, :force => true) - @status_message.reload - @status_message.contacts(true).should_not include(@contact) - @status_message.share_visibilities.reset - @status_message.user_refs.should == 2 - end - - it 'should not override userrefs on receive by another person' do - new_user = Factory(:user_with_aspect) - @status_message.share_visibilities.reset - @status_message.user_refs.should == 3 - - new_user.contacts.create(:person => bob.person, :aspects => [new_user.aspects.first]) - xml = @status_message.to_diaspora_xml - - receive_with_zord(new_user, bob.person, xml) - - @status_message.share_visibilities.reset - @status_message.user_refs.should == 4 - - alice.remove_contact(@contact, :force => true) - @status_message.share_visibilities.reset - @status_message.user_refs.should == 3 - end end describe 'comments' do diff --git a/spec/models/share_visibility_spec.rb b/spec/models/share_visibility_spec.rb index 4c72adf21..5c7c9c56f 100644 --- a/spec/models/share_visibility_spec.rb +++ b/spec/models/share_visibility_spec.rb @@ -11,6 +11,12 @@ describe ShareVisibility do @contact = bob.contact_for(alice.person) end + it 'returns false if share is public' do + @post.public = true + @post.save + ShareVisibility.batch_import([@contact.id], @post).should be_false + end + it 'creates a visibility for each user' do lambda { ShareVisibility.batch_import([@contact.id], @post) diff --git a/spec/models/user/connecting_spec.rb b/spec/models/user/connecting_spec.rb index 2b5770698..1329120cf 100644 --- a/spec/models/user/connecting_spec.rb +++ b/spec/models/user/connecting_spec.rb @@ -78,7 +78,7 @@ describe Diaspora::UserModules::Connecting do describe '#register_share_visibilities' do it 'creates post visibilites for up to 100 posts' do - Post.stub_chain(:where, :limit).and_return([Factory(:status_message, :public => true)]) + Post.stub_chain(:where, :limit).and_return([Factory(:status_message)]) c = Contact.create!(:user_id => alice.id, :person_id => eve.person.id) expect{ alice.register_share_visibilities(c)