diff --git a/app/models/share_visibility.rb b/app/models/share_visibility.rb index 4dbe526bf..bffdf91cc 100644 --- a/app/models/share_visibility.rb +++ b/app/models/share_visibility.rb @@ -10,6 +10,10 @@ class ShareVisibility < ActiveRecord::Base where(user_id: user.id) } + scope :for_shareable, ->(shareable) { + where(shareable_id: shareable.id, shareable_type: shareable.class.base_class.to_s) + } + validate :not_public # Perform a batch import, given a set of users and a shareable @@ -18,8 +22,17 @@ class ShareVisibility < ActiveRecord::Base # @param share [Shareable] # @return [void] def self.batch_import(user_ids, share) - return false unless ShareVisibility.new(shareable_id: share.id, shareable_type: share.class.to_s).valid? + return false unless ShareVisibility.new(shareable_id: share.id, shareable_type: share.class.base_class.to_s).valid? + user_ids -= ShareVisibility.for_shareable(share).where(user_id: user_ids).pluck(:user_id) + return false if user_ids.empty? + + create_visilities(user_ids, share) + end + + private + + private_class_method def self.create_visilities(user_ids, share) if AppConfig.postgres? user_ids.each do |user_id| ShareVisibility.find_or_create_by( @@ -36,8 +49,6 @@ class ShareVisibility < ActiveRecord::Base end end - private - def not_public errors[:base] << "Cannot create visibility for a public object" if shareable.public? end diff --git a/app/models/status_message.rb b/app/models/status_message.rb index de858152f..6ee7d388e 100644 --- a/app/models/status_message.rb +++ b/app/models/status_message.rb @@ -120,6 +120,12 @@ class StatusMessage < Post } end + def receive(recipient_user_ids) + super(recipient_user_ids) + + photos.each {|photo| photo.receive(recipient_user_ids) } + end + private def presence_of_content diff --git a/db/migrate/20160509232726_cleanup_duplicates_and_add_unique_indexes.rb b/db/migrate/20160509232726_cleanup_duplicates_and_add_unique_indexes.rb index 4f5750a3e..26b79067b 100644 --- a/db/migrate/20160509232726_cleanup_duplicates_and_add_unique_indexes.rb +++ b/db/migrate/20160509232726_cleanup_duplicates_and_add_unique_indexes.rb @@ -1,17 +1,4 @@ class CleanupDuplicatesAndAddUniqueIndexes < ActiveRecord::Migration - class Post < ActiveRecord::Base - end - - class StatusMessage < Post - end - - class Photo < ActiveRecord::Base - belongs_to :status_message, foreign_key: :status_message_guid, primary_key: :guid - end - - class ShareVisibility < ActiveRecord::Base - end - def up # temporary index to speed up the migration add_index :photos, :guid, length: 191 @@ -34,12 +21,6 @@ class CleanupDuplicatesAndAddUniqueIndexes < ActiveRecord::Migration %i(conversations messages photos polls poll_answers poll_participations).each do |table| delete_duplicates_and_create_unique_index(table) end - - # fix photo public flag again ... - Photo.joins(:status_message).where(posts: {public: true}).update_all(public: true) - - ShareVisibility.joins("INNER JOIN photos ON photos.id = share_visibilities.shareable_id") - .where(shareable_type: "Photo", photos: {public: true}).delete_all end def down diff --git a/db/migrate/20160906225138_fix_photos_share_visibilities.rb b/db/migrate/20160906225138_fix_photos_share_visibilities.rb new file mode 100644 index 000000000..d14ca502c --- /dev/null +++ b/db/migrate/20160906225138_fix_photos_share_visibilities.rb @@ -0,0 +1,42 @@ +class FixPhotosShareVisibilities < ActiveRecord::Migration + class Photo < ActiveRecord::Base + end + + class ShareVisibility < ActiveRecord::Base + end + + def up + Photo.joins("INNER JOIN posts ON posts.guid = photos.status_message_guid") + .where(posts: {type: "StatusMessage", public: true}).update_all(public: true) + + ShareVisibility.joins("INNER JOIN photos ON photos.id = share_visibilities.shareable_id") + .where(shareable_type: "Photo", photos: {public: true}).delete_all + + remove_duplicates + remove_index :share_visibilities, name: :shareable_and_user_id + add_index :share_visibilities, %i(shareable_id shareable_type user_id), name: :shareable_and_user_id, unique: true + + execute "INSERT INTO share_visibilities (user_id, shareable_id, shareable_type) " \ + "SELECT post_visibility.user_id, photos.id, 'Photo' FROM photos " \ + "INNER JOIN posts ON posts.guid = photos.status_message_guid AND posts.type = 'StatusMessage' " \ + "LEFT OUTER JOIN share_visibilities ON share_visibilities.shareable_id = photos.id " \ + "INNER JOIN share_visibilities AS post_visibility ON post_visibility.shareable_id = posts.id " \ + "WHERE photos.public = 0 AND share_visibilities.shareable_id IS NULL " \ + "AND post_visibility.shareable_type = 'Post'" + end + + def down + remove_index :share_visibilities, name: :shareable_and_user_id + add_index :share_visibilities, %i(shareable_id shareable_type user_id), name: :shareable_and_user_id + end + + def remove_duplicates + where = "WHERE s1.user_id = s2.user_id AND s1.shareable_id = s2.shareable_id AND "\ + "s1.shareable_type = s2.shareable_type AND s1.id > s2.id" + if AppConfig.postgres? + execute("DELETE FROM share_visibilities AS s1 USING share_visibilities AS s2 #{where}") + else + execute("DELETE s1 FROM share_visibilities s1, share_visibilities s2 #{where}") + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 9a0326974..d18815df9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160902180630) do +ActiveRecord::Schema.define(version: 20160906225138) do create_table "account_deletions", force: :cascade do |t| t.string "diaspora_handle", limit: 255 @@ -538,7 +538,7 @@ ActiveRecord::Schema.define(version: 20160902180630) do end add_index "share_visibilities", ["shareable_id", "shareable_type", "hidden", "user_id"], name: "shareable_and_hidden_and_user_id", using: :btree - add_index "share_visibilities", ["shareable_id", "shareable_type", "user_id"], name: "shareable_and_user_id", using: :btree + add_index "share_visibilities", ["shareable_id", "shareable_type", "user_id"], name: "shareable_and_user_id", unique: true, using: :btree add_index "share_visibilities", ["shareable_id"], name: "index_post_visibilities_on_post_id", using: :btree add_index "share_visibilities", ["user_id"], name: "index_share_visibilities_on_user_id", using: :btree diff --git a/spec/models/share_visibility_spec.rb b/spec/models/share_visibility_spec.rb index 39698feab..cd691679e 100644 --- a/spec/models/share_visibility_spec.rb +++ b/spec/models/share_visibility_spec.rb @@ -4,7 +4,7 @@ require "spec_helper" -describe ShareVisibility, :type => :model do +describe ShareVisibility, type: :model do describe ".batch_import" do let(:post) { FactoryGirl.create(:status_message, author: alice.person) } @@ -29,6 +29,13 @@ describe ShareVisibility, :type => :model do }.not_to raise_error end + it "does not create visibilities for a public shareable" do + public_post = FactoryGirl.create(:status_message, author: alice.person, public: true) + + ShareVisibility.batch_import([bob.id], public_post) + expect(ShareVisibility.where(user_id: bob.id, shareable_id: post.id, shareable_type: "Post")).not_to exist + end + context "scopes" do before do alice.post(:status_message, text: "Hey", to: alice.aspects.first) diff --git a/spec/models/status_message_spec.rb b/spec/models/status_message_spec.rb index 31901380b..674229a91 100644 --- a/spec/models/status_message_spec.rb +++ b/spec/models/status_message_spec.rb @@ -302,4 +302,33 @@ describe StatusMessage, type: :model do end end end + + describe "#receive" do + let(:post) { FactoryGirl.create(:status_message, author: alice.person) } + + it "receives attached photos" do + photo = FactoryGirl.create(:photo, status_message: post) + + post.receive([bob.id]) + + expect(ShareVisibility.where(user_id: bob.id, shareable_id: post.id, shareable_type: "Post").count).to eq(1) + expect(ShareVisibility.where(user_id: bob.id, shareable_id: photo.id, shareable_type: "Photo").count).to eq(1) + end + + it "works without attached photos" do + post.receive([bob.id]) + + expect(ShareVisibility.where(user_id: bob.id, shareable_id: post.id, shareable_type: "Post").count).to eq(1) + end + + it "works with already received attached photos" do + photo = FactoryGirl.create(:photo, status_message: post) + + photo.receive([bob.id]) + post.receive([bob.id]) + + expect(ShareVisibility.where(user_id: bob.id, shareable_id: post.id, shareable_type: "Post").count).to eq(1) + expect(ShareVisibility.where(user_id: bob.id, shareable_id: photo.id, shareable_type: "Photo").count).to eq(1) + end + end end