From 254860bddc1f68f96430019f96d1b94e92977572 Mon Sep 17 00:00:00 2001 From: Maxwell Salzberg Date: Sat, 7 Jan 2012 17:52:35 -0800 Subject: [PATCH] SM MS; Read email sent to diaspora-dev for more information about this commit. Add migration and rake task to copy hidden information from share_visibilities to users. see: http://devblog.joindiaspora.com/?p=44 --- .../share_visibilities_controller.rb | 31 ++------ app/models/notification.rb | 18 ++--- app/models/post.rb | 27 +++++-- app/models/user.rb | 52 ++++++++++++++ ...0942_move_recently_hidden_posts_to_user.rb | 28 ++++++++ db/schema.rb | 1 + lib/diaspora/redis_cache.rb | 12 ++++ lib/share_visibility_converter.rb | 24 +++++++ lib/tasks/db.rake | 11 ++- lib/tasks/migrations.rake | 7 ++ .../share_visibilities_controller_spec.rb | 62 ++-------------- spec/factories.rb | 1 + spec/lib/diaspora/redis_cache_spec.rb | 26 +++++++ spec/models/notification_spec.rb | 12 ---- spec/models/post_spec.rb | 22 ++++++ spec/models/user_spec.rb | 71 +++++++++++++++++++ 16 files changed, 291 insertions(+), 114 deletions(-) create mode 100644 db/migrate/20120107220942_move_recently_hidden_posts_to_user.rb create mode 100644 lib/share_visibility_converter.rb diff --git a/app/controllers/share_visibilities_controller.rb b/app/controllers/share_visibilities_controller.rb index 1c6cb9a8e..49c7ead7c 100644 --- a/app/controllers/share_visibilities_controller.rb +++ b/app/controllers/share_visibilities_controller.rb @@ -11,37 +11,14 @@ class ShareVisibilitiesController < ApplicationController params[:shareable_id] ||= params[:post_id] params[:shareable_type] ||= 'Post' - @post = accessible_post - @contact = current_user.contact_for(@post.author) - - if @contact && @vis = ShareVisibility.where(:contact_id => @contact.id, - :shareable_id => params[:shareable_id], - :shareable_type => params[:shareable_type]).first - @vis.hidden = !@vis.hidden - if @vis.save - update_cache(@vis) - render :nothing => true, :status => 200 - return - end - end - render :nothing => true, :status => 403 + vis = current_user.toggle_hidden_shareable(accessible_post) + RedisCache.update_cache_for(current_user, accessible_post, vis) + render :nothing => true, :status => 200 end protected - def update_cache(visibility) - return unless RedisCache.configured? - - cache = RedisCache.new(current_user, 'created_at') - - if visibility.hidden? - cache.remove(accessible_post.id) - else - cache.add(accessible_post.created_at.to_i, accessible_post.id) - end - end - def accessible_post - @post ||= Post.where(:id => params[:post_id]).select("id, guid, author_id, created_at").first + @post ||= params[:shareable_type].constantize.where(:id => params[:post_id]).select("id, guid, author_id, created_at").first end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 547899379..a15711c31 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -22,6 +22,7 @@ class Notification < ActiveRecord::Base else n = note_type.make_notification(recipient, target, actor, note_type) end + if n n.email_the_user(target, actor) n @@ -43,7 +44,8 @@ class Notification < ActiveRecord::Base private def self.concatenate_or_create(recipient, target, actor, notification_type) - return nil if share_visiblity_is_hidden?(recipient, target) + return nil if suppress_notification?(recipient, target) + if n = notification_type.where(:target_id => target.id, :target_type => target.class.base_class, :recipient_id => recipient.id, @@ -64,22 +66,16 @@ private def self.make_notification(recipient, target, actor, notification_type) - return nil if share_visiblity_is_hidden?(recipient, target) + return nil if suppress_notification?(recipient, target) n = notification_type.new(:target => target, - :recipient_id => recipient.id) + :recipient_id => recipient.id) n.actors = n.actors | [actor] n.unread = false if target.is_a? Request n.save! n end - #horrible hack that should not be here! - def self.share_visiblity_is_hidden?(recipient, post) - return false unless post.is_a?(Post) - - contact = recipient.contact_for(post.author) - return false unless contact && recipient && post - pv = ShareVisibility.where(:contact_id => contact.id, :shareable_id => post.id, :shareable_type => post.class.base_class.to_s).first - pv.present? ? pv.hidden? : false + def self.suppress_notification?(recipient, post) + post.is_a?(Post) && recipient.is_shareable_hidden?(post) end end diff --git a/app/models/post.rb b/app/models/post.rb index 4e066b635..dc656e672 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -71,27 +71,40 @@ class Post < ActiveRecord::Base end def self.excluding_blocks(user) - people = user.blocks.includes(:person).map{|b| b.person} + people = user.blocks.map{|b| b.person_id} + scope = scoped - if people.present? - where("posts.author_id NOT IN (?)", people.map { |person| person.id }) - else - scoped + if people.any? + scope = scope.where("posts.author_id NOT IN (?)", people) end + + scope + end + + def self.excluding_hidden_shareables(user) + scope = scoped + if user.has_hidden_shareables_of_type? + scope = scope.where('posts.id NOT IN (?)', user.hidden_shareables["#{self.base_class}"]) + end + scope + end + + def self.excluding_hidden_content(user) + excluding_blocks(user).excluding_hidden_shareables(user) end def self.for_a_stream(max_time, order, user=nil) scope = self.for_visible_shareable_sql(max_time, order). includes_for_a_stream - scope = scope.excluding_blocks(user) if user.present? + scope = scope.excluding_hidden_content(user) if user.present? scope end ############# - def self.diaspora_initialize params + def self.diaspora_initialize(params) new_post = self.new params.to_hash new_post.author = params[:author] new_post.public = params[:public] if params[:public] diff --git a/app/models/user.rb b/app/models/user.rb index 49e681187..3ba7864fc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -30,6 +30,8 @@ class User < ActiveRecord::Base validates_associated :person validate :no_person_with_same_username + serialize :hidden_shareables, Hash + has_one :person, :foreign_key => :owner_id delegate :public_key, :posts, :photos, :owns?, :diaspora_handle, :name, :public_url, :profile, :first_name, :last_name, :to => :person @@ -99,6 +101,56 @@ class User < ActiveRecord::Base end end + def hidden_shareables + self[:hidden_shareables] ||= {} + end + + def add_hidden_shareable(key, share_id, opts={}) + if self.hidden_shareables.has_key?(key) + self.hidden_shareables[key] << share_id + else + self.hidden_shareables[key] = [share_id] + end + self.save unless opts[:batch] + self.hidden_shareables + end + + def remove_hidden_shareable(key, share_id) + if self.hidden_shareables.has_key?(key) + self.hidden_shareables[key].delete(share_id) + end + end + + def is_shareable_hidden?(shareable) + shareable_type = shareable.class.base_class.name + if self.hidden_shareables.has_key?(shareable_type) + self.hidden_shareables[shareable_type].include?(shareable.id.to_s) + else + false + end + end + + + def toggle_hidden_shareable(share) + share_id = share.id.to_s + key = share.class.base_class.to_s + if self.hidden_shareables.has_key?(key) && self.hidden_shareables[key].include?(share_id) + self.remove_hidden_shareable(key, share_id) + self.save + false + else + self.add_hidden_shareable(key, share_id) + self.save + true + end + end + + def has_hidden_shareables_of_type?(t = Post) + share_type = t.base_class.to_s + self.hidden_shareables[share_type].present? + end + + def self.create_from_invitation!(invitation) user = User.new user.generate_keys diff --git a/db/migrate/20120107220942_move_recently_hidden_posts_to_user.rb b/db/migrate/20120107220942_move_recently_hidden_posts_to_user.rb new file mode 100644 index 000000000..bac3a2e78 --- /dev/null +++ b/db/migrate/20120107220942_move_recently_hidden_posts_to_user.rb @@ -0,0 +1,28 @@ +class Person < ActiveRecord::Base + belongs_to :owner, :class_name => 'User' +end + +class User < ActiveRecord::Base + serialize :hidden_shareables, Hash +end + +class Contact < ActiveRecord::Base + belongs_to :user +end + +class ShareVisibility < ActiveRecord::Base + belongs_to :contact +end + +require File.join(File.dirname(__FILE__), '..', '..', 'lib', 'share_visibility_converter') + +class MoveRecentlyHiddenPostsToUser < ActiveRecord::Migration + def self.up + add_column :users, :hidden_shareables, :text + ShareVisibilityConverter.copy_hidden_share_visibilities_to_users(true) + end + + def self.down + remove_column :users, :hidden_shareables + end +end \ No newline at end of file diff --git a/db/schema.rb b/db/schema.rb index f41d0de14..62ec26489 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -463,6 +463,7 @@ ActiveRecord::Schema.define(:version => 20120114191018) do t.boolean "show_community_spotlight_in_stream", :default => true, :null => false t.boolean "auto_follow_back", :default => false t.integer "auto_follow_back_aspect_id" + t.text "hidden_shareables" end add_index "users", ["authentication_token"], :name => "index_users_on_authentication_token", :unique => true diff --git a/lib/diaspora/redis_cache.rb b/lib/diaspora/redis_cache.rb index 8f5ceb672..636c90e9d 100644 --- a/lib/diaspora/redis_cache.rb +++ b/lib/diaspora/redis_cache.rb @@ -19,6 +19,18 @@ class RedisCache AppConfig[:redis_cache].present? end + def self.update_cache_for(user, post, post_was_hidden) + return unless RedisCache.configured? + + cache = RedisCache.new(user, 'created_at') + + if post_was_hidden + cache.remove(post.id) + else + cache.add(post.created_at.to_i, post.id) + end + end + # @return [Boolean] def cache_exists? self.redis.exists(set_key) diff --git a/lib/share_visibility_converter.rb b/lib/share_visibility_converter.rb new file mode 100644 index 000000000..c8f6dff30 --- /dev/null +++ b/lib/share_visibility_converter.rb @@ -0,0 +1,24 @@ +class ShareVisibilityConverter + RECENT = 2 # number of weeks to do in the migration + def self.copy_hidden_share_visibilities_to_users(only_recent = false) + query = ShareVisibility.where(:hidden => true).includes(:contact => :user) + query = query.where('share_visibilities.updated_at > ?', RECENT.weeks.ago) if only_recent + count = query.count + puts "Updating #{count} records in batches of 1000..." + + batch_count = 1 + query.find_in_batches do |visibilities| + puts "Updating batch ##{batch_count} of #{(count/1000)+1}..." + batch_count += 1 + visibilities.each do |visibility| + type = visibility.shareable_type + id = visibility.shareable_id.to_s + u = visibility.contact.user + u.hidden_shareables ||= {} + u.hidden_shareables[type] ||= [] + u.hidden_shareables[type] << id unless u.hidden_shareables[type].include?(id) + u.save!(:validate => false) + end + end + end +end \ No newline at end of file diff --git a/lib/tasks/db.rake b/lib/tasks/db.rake index 15fd9fda2..9f9e069c9 100644 --- a/lib/tasks/db.rake +++ b/lib/tasks/db.rake @@ -4,7 +4,16 @@ namespace :db do desc "rebuild and prepare test db" - task :rebuild => [:drop, :drop_integration, :create, :migrate, :seed, 'db:test:prepare'] + task :rebuild do + Rake::Task['db:drop'].invoke + Rake::Task['db:drop_integration'].invoke + Rake::Task['db:create'].invoke + Rake::Task['db:migrate'].invoke + puts "seeding users, this will take awhile" + `rake db:seed` #ghetto hax as we have active record garbage in our models + puts "seeded!" + Rake::Task['db:test:prepare'].invoke + end namespace :integration do # desc 'Check for pending migrations and load the integration schema' diff --git a/lib/tasks/migrations.rake b/lib/tasks/migrations.rake index 4cd77c89f..ab445085d 100644 --- a/lib/tasks/migrations.rake +++ b/lib/tasks/migrations.rake @@ -3,6 +3,13 @@ # the COPYRIGHT file. namespace :migrations do + + desc 'copy all hidden share visibilities from share_visibilities to users. Can be run with the site still up.' + task :copy_hidden_share_visibilities_to_users => [:environment] do + require File.join(Rails.root, 'lib', 'share_visibility_converter') + ShareVisibilityConverter.copy_hidden_share_visibilities_to_users + end + desc 'absolutify all existing image references' task :absolutify_image_references do require File.join(File.dirname(__FILE__), '..', '..', 'config', 'environment') diff --git a/spec/controllers/share_visibilities_controller_spec.rb b/spec/controllers/share_visibilities_controller_spec.rb index a7634c37a..b22535e21 100644 --- a/spec/controllers/share_visibilities_controller_spec.rb +++ b/spec/controllers/share_visibilities_controller_spec.rb @@ -7,7 +7,6 @@ require 'spec_helper' describe ShareVisibilitiesController do before do @status = alice.post(:status_message, :text => "hello", :to => alice.aspects.first) - @vis = @status.share_visibilities.first sign_in :user, bob end @@ -23,72 +22,23 @@ describe ShareVisibilitiesController do end it 'calls #update_cache' do - @controller.should_receive(:update_cache).with(an_instance_of(ShareVisibility)) + RedisCache.should_receive(:update_cache_for).with(an_instance_of(User), an_instance_of(Post), true) put :update, :format => :js, :id => 42, :post_id => @status.id end - it 'marks hidden if visible' do + it 'it calls toggle_hidden_shareable' do + @controller.current_user.should_receive(:toggle_hidden_shareable).with(an_instance_of(Post)) put :update, :format => :js, :id => 42, :post_id => @status.id - @vis.reload.hidden.should be_true - end - - it 'marks visible if hidden' do - @vis.update_attributes(:hidden => true) - - put :update, :format => :js, :id => 42, :post_id => @status.id - @vis.reload.hidden.should be_false - end - end - - context "post you do not see" do - before do - sign_in :user, eve - end - - it 'does not let a user destroy a visibility that is not theirs' do - lambda { - put :update, :format => :js, :id => 42, :post_id => @status.id - }.should_not change(@vis.reload, :hidden).to(true) - end - - it 'does not succeed' do - put :update, :format => :js, :id => 42, :post_id => @status.id - response.should_not be_success end end end - - describe '#update_cache' do - before do - @controller.params[:post_id] = @status.id - @cache = RedisCache.new(bob, 'created_at') - RedisCache.stub(:new).and_return(@cache) - RedisCache.stub(:configured?).and_return(true) - end - - it 'does nothing if cache is not configured' do - RedisCache.stub(:configured?).and_return(false) - RedisCache.should_not_receive(:new) - @controller.send(:update_cache, @vis) - end - - it 'removes the post from the cache if visibility is marked as hidden' do - @vis.hidden = true - @cache.should_receive(:remove).with(@vis.shareable_id) - @controller.send(:update_cache, @vis) - end - - it 'adds the post from the cache if visibility is marked as hidden' do - @vis.hidden = false - @cache.should_receive(:add).with(@status.created_at.to_i, @vis.shareable_id) - @controller.send(:update_cache, @vis) - end - end - + describe "#accessible_post" do it "memoizes a query for a post given a post_id param" do id = 1 @controller.params[:post_id] = id + @controller.params[:shareable_type] = 'Post' + Post.should_receive(:where).with(hash_including(:id => id)).once.and_return(stub.as_null_object) 2.times do |n| @controller.send(:accessible_post) diff --git a/spec/factories.rb b/spec/factories.rb index 1a64e2b1c..e2cba1be7 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -100,6 +100,7 @@ end Factory.define(:photo) do |p| p.sequence(:random_string) {|n| ActiveSupport::SecureRandom.hex(10) } + p.association :author, :factory => :person p.after_build do |p| p.unprocessed_image.store! File.open(File.join(File.dirname(__FILE__), 'fixtures', 'button.png')) p.update_remote_path diff --git a/spec/lib/diaspora/redis_cache_spec.rb b/spec/lib/diaspora/redis_cache_spec.rb index 27dd9d6dd..da56a2354 100644 --- a/spec/lib/diaspora/redis_cache_spec.rb +++ b/spec/lib/diaspora/redis_cache_spec.rb @@ -220,4 +220,30 @@ describe RedisCache do RedisCache.acceptable_types.should =~ Stream::Base::TYPES_OF_POST_IN_STREAM end end + + describe '#update_cache' do + before do + @cache = RedisCache.new(bob, 'created_at') + RedisCache.stub(:new).and_return(@cache) + RedisCache.stub(:configured?).and_return(true) + @post = Factory(:status_message) + end + + it 'does nothing if cache is not configured' do + RedisCache.stub(:configured?).and_return(false) + RedisCache.should_not_receive(:new) + RedisCache.update_cache_for(bob, @post, true) + end + + it 'removes the post from the cache if visibility is marked as hidden' do + @cache.should_receive(:remove).with(@post.id) + RedisCache.update_cache_for(bob, @post, true) + end + + it 'adds the post from the cache if visibility is unmarked as hidden' do + @cache.should_receive(:add).with(@post.created_at.to_i, @post.id) + RedisCache.update_cache_for(bob, @post, false) + end + end + end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index a619bb7ef..b6b973124 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -50,18 +50,6 @@ describe Notification do end end describe '.notify' do - it 'does not call Notification.create if the object does not have a notification_type' do - Notification.should_not_receive(:make_notificatin) - Notification.notify(@user, @sm, @person) - end - - it 'does not create a notification if the post visibility is hidden' do - Notification.stub(:share_visiblity_is_hidden).and_return(true) - expect{ - Notification.notify(@user, @sm, @person) - }.to change(Notification, :count).by(0) - end - context 'with a request' do before do @request = Request.diaspora_initialize(:from => @user.person, :to => @user2.person, :into => @aspect) diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 4d6a2fdcc..a93e7d7d6 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -87,6 +87,28 @@ describe Post do end end + describe '.excluding_hidden_shareables' do + before do + @post = Factory(:status_message, :author => alice.person) + @other_post = Factory(:status_message, :author => eve.person) + bob.toggle_hidden_shareable(@post) + end + it 'excludes posts the user has hidden' do + Post.excluding_hidden_shareables(bob).should_not include(@post) + end + it 'includes posts the user has not hidden' do + Post.excluding_hidden_shareables(bob).should include(@other_post) + end + end + + describe '.excluding_hidden_content' do + it 'calls excluding_blocks and excluding_hidden_shareables' do + Post.should_receive(:excluding_blocks).and_return(Post) + Post.should_receive(:excluding_hidden_shareables) + Post.excluding_hidden_content(bob) + end + end + context 'having some posts' do before do time_interval = 1000 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a18974173..e0a3633c6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -58,6 +58,76 @@ describe User do end end + describe 'hidden_shareables' do + before do + @sm = Factory(:status_message) + @sm_id = @sm.id.to_s + @sm_class = @sm.class.base_class.to_s + end + + it 'is a hash' do + alice.hidden_shareables.should == {} + end + + describe '#add_hidden_shareable' do + it 'adds the share id to an array which is keyed by the objects class' do + alice.add_hidden_shareable(@sm_class, @sm_id) + alice.hidden_shareables['Post'].should == [@sm_id] + end + + it 'handles having multiple posts' do + sm2 = Factory(:status_message) + alice.add_hidden_shareable(@sm_class, @sm_id) + alice.add_hidden_shareable(sm2.class.base_class.to_s, sm2.id.to_s) + + alice.hidden_shareables['Post'].should =~ [@sm_id, sm2.id.to_s] + end + + it 'handles having multiple shareable types' do + photo = Factory(:photo) + alice.add_hidden_shareable(photo.class.base_class.to_s, photo.id.to_s) + alice.add_hidden_shareable(@sm_class, @sm_id) + + alice.hidden_shareables['Photo'].should == [photo.id.to_s] + end + end + + describe '#remove_hidden_shareable' do + it 'removes the id from the hash if it is there' do + alice.add_hidden_shareable(@sm_class, @sm_id) + alice.remove_hidden_shareable(@sm_class, @sm_id) + alice.hidden_shareables['Post'].should == [] + end + end + + describe 'toggle_hidden_shareable' do + it 'calls add_hidden_shareable if the key does not exist, and returns true' do + alice.should_receive(:add_hidden_shareable).with(@sm_class, @sm_id) + alice.toggle_hidden_shareable(@sm).should be_true + end + + it 'calls remove_hidden_shareable if the key exists' do + alice.should_receive(:remove_hidden_shareable).with(@sm_class, @sm_id) + alice.add_hidden_shareable(@sm_class, @sm_id) + alice.toggle_hidden_shareable(@sm).should be_false + end + end + + describe '#is_shareable_hidden?' do + it 'returns true if the shareable is hidden' do + post = Factory(:status_message) + bob.toggle_hidden_shareable(post) + bob.is_shareable_hidden?(post).should be_true + end + + it 'returns false if the shareable is not present' do + post = Factory(:status_message) + bob.is_shareable_hidden?(post).should be_false + end + end + end + + describe 'overwriting people' do it 'does not overwrite old users with factory' do lambda { @@ -1037,6 +1107,7 @@ describe User do current_sign_in_at last_sign_in_at current_sign_in_ip + hidden_shareables last_sign_in_ip invitation_service invitation_identifier