Don't create share_visibilities for public posts, sans migration of deleting old data.

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<Contact>] 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

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]

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={})

lib/diaspora/user/querying.rb

@@ -47,7 +47,11 @@ module Diaspora
       # @return [Array<Integer>]
       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|
+        aspect_ids = aspects.map{|a| a.id}
-          contacts | aspect.contacts
+        Contact.joins(:aspect_memberships => :aspect).
-        end
+                where(Aspect.arel_table[:id].in(aspect_ids))
       end
 
       def posts_from(person)

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
+end

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
+    context 'dependent delete' do
-      before 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

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)

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)