FIX: some posters were not getting added to topic_allowed_users when moving posts to a new PM

follow-up

#1

FIX: some posters were not getting added to topic_allowed_users when moving posts to a new PM

If a user posted twice in a topic then subsequent posters were not getting added as topic_allowed_users.

diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index 47b8f3c..99ee4b1 100644
--- a/app/models/post_mover.rb
+++ b/app/models/post_mover.rb
@@ -87,11 +87,10 @@ class PostMover
 
     posts.each do |post|
       post.is_first_post? ? create_first_post(post) : move(post)
-      if @move_to_pm
-        destination_topic.topic_allowed_users.build(user_id: post.user_id) unless destination_topic.topic_allowed_users.where(user_id: post.user_id).exists?
+      if @move_to_pm && destination_topic.topic_allowed_users.where(user_id: post.user_id).blank?
+        destination_topic.topic_allowed_users.create!(user_id: post.user_id)
       end
     end
-    destination_topic.save! if @move_to_pm
 
     PostReply.where("reply_id IN (:post_ids) OR post_id IN (:post_ids)", post_ids: post_ids).each do |post_reply|
       if post_reply.post && post_reply.reply && post_reply.reply.topic_id != post_reply.post.topic_id
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index b4c6095..3d7ec10 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -580,13 +580,15 @@ describe PostMover do
 
       context 'move to new message' do
         it "adds post users as topic allowed users" do
-          personal_message.move_posts(admin, [p2.id, p5.id], title: "new testing message name", tags: ["tag1", "tag2"], archetype: "private_message")
+          personal_message.move_posts(admin, [p2.id, p3.id, p4.id, p5.id], title: "new testing message name", tags: ["tag1", "tag2"], archetype: "private_message")
 
           p2.reload
-          expect(p2.topic.archetype).to eq(Archetype.private_message)
-          expect(p2.topic.topic_allowed_users.where(user_id: another_user.id).count).to eq(1)
-          expect(p2.topic.topic_allowed_users.where(user_id: evil_trout.id).count).to eq(1)
-          expect(p2.topic.tags.pluck(:name)).to eq([])
+          destination_topic = p2.topic
+          expect(destination_topic.archetype).to eq(Archetype.private_message)
+          expect(destination_topic.topic_allowed_users.where(user_id: user.id).count).to eq(1)
+          expect(destination_topic.topic_allowed_users.where(user_id: another_user.id).count).to eq(1)
+          expect(destination_topic.topic_allowed_users.where(user_id: evil_trout.id).count).to eq(1)
+          expect(destination_topic.tags.pluck(:name)).to eq([])
         end
 
         it "can add tags to new message when allow_staff_to_tag_pms is enabled" do

GitHub sha: dcd7b925


#2

This commit has been mentioned on Discourse Meta. There might be relevant details there:


#3

I think there is a regression here. exists? translates to an SQL query that does SELECT 1 whereas blank? loads all the records into an array and checks if the array is empty.


Follow Up #4