Merge pull request #3231 from evazion/fix-3229

Fix #3229: Post expungement bugs

app/models/pool.rb

@@ -239,11 +239,10 @@ class Pool < ApplicationRecord
   def remove!(post)
     return unless contains?(post.id)
     return unless CurrentUser.user.can_remove_from_pools?
-    return if is_deleted?
 
     with_lock do
       update_attributes(:post_ids => remove_number_from_string(post.id, post_ids), :post_count => post_count - 1)
-      post.remove_pool!(self, true)
+      post.remove_pool!(self)
       clear_post_id_array
     end
   end
@@ -284,7 +283,7 @@ class Pool < ApplicationRecord
 
     removed.each do |post_id|
       post = Post.find(post_id)
-      post.remove_pool!(self, true)
+      post.remove_pool!(self)
     end
 
     normalize_post_ids

app/models/post.rb

@@ -50,7 +50,7 @@ class Post < ApplicationRecord
   has_many :approvals, :class_name => "PostApproval", :dependent => :destroy
   has_many :disapprovals, :class_name => "PostDisapproval", :dependent => :destroy
   has_many :favorites
-  has_many :replacements, class_name: "PostReplacement"
+  has_many :replacements, class_name: "PostReplacement", :dependent => :destroy
 
   if PostArchive.enabled?
     has_many :versions, lambda {order("post_versions.updated_at ASC")}, :class_name => "PostArchive", :dependent => :destroy
@@ -1051,12 +1051,12 @@ class Post < ApplicationRecord
       @pools ||= begin
         return Pool.none if pool_string.blank?
         pool_ids = pool_string.scan(/\d+/)
-        Pool.undeleted.where(id: pool_ids).series_first
+        Pool.where(id: pool_ids).series_first
       end
     end
 
     def has_active_pools?
-      pools.length > 0
+      pools.undeleted.length > 0
     end
 
     def belongs_to_pool?(pool)
@@ -1079,10 +1079,9 @@ class Post < ApplicationRecord
       end
     end
 
-    def remove_pool!(pool, force = false)
+    def remove_pool!(pool)
       return unless belongs_to_pool?(pool)
       return unless CurrentUser.user.can_remove_from_pools?
-      return if pool.is_deleted? && !force
 
       with_lock do
         self.pool_string = pool_string.gsub(/(?:\A| )pool:#{pool.id}(?:\Z| )/, " ").strip
@@ -1101,7 +1100,7 @@ class Post < ApplicationRecord
     def set_pool_category_pseudo_tags
       self.pool_string = (pool_string.scan(/\S+/) - ["pool:series", "pool:collection"]).join(" ")
 
-      pool_categories = pools.select("category").map(&:category)
+      pool_categories = pools.undeleted.pluck(:category)
       if pool_categories.include?("series")
         self.pool_string = "#{pool_string} pool:series".strip
       end
@@ -1284,17 +1283,8 @@ class Post < ApplicationRecord
     # - Move favorites to the first child.
     # - Reparent all children to the first child.
 
-    module ClassMethods
+    def update_has_children_flag
-      def update_has_children_flag_for(post_id)
+      update({has_children: children.exists?, has_active_children: children.undeleted.exists?}, without_protection: true)
-        return if post_id.nil?
-        has_children = Post.where("parent_id = ?", post_id).exists?
-        has_active_children = Post.where("parent_id = ? and is_deleted = ?", post_id, false).exists?
-        execute_sql("UPDATE posts SET has_children = ?, has_active_children = ? WHERE id = ?", has_children, has_active_children, post_id)
-      end
-    end
-
-    def self.included(m)
-      m.extend(ClassMethods)
     end
 
     def blank_out_nonexistent_parents
@@ -1311,32 +1301,25 @@ class Post < ApplicationRecord
     end
 
     def update_parent_on_destroy
-      Post.update_has_children_flag_for(parent_id) if parent_id
+      parent.update_has_children_flag if parent
     end
 
     def update_children_on_destroy
-      if children.size == 0
+      return unless children.present?
-        # do nothing
+
-      elsif children.size == 1
+      eldest = children[0]
-        children.first.update_column(:parent_id, nil)
+      siblings = children[1..-1]
-      else
+
-        cached_children = children
+      eldest.update(parent_id: nil)
-        eldest = cached_children[0]
+      Post.where(id: siblings).find_each { |p| p.update(parent_id: eldest.id) }
-        siblings = cached_children[1..-1]
+      # Post.where(id: siblings).update(parent_id: eldest.id) # XXX rails 5
-        eldest.update_column(:parent_id, nil)
-        Post.where(:id => siblings.map(&:id)).update_all(:parent_id => eldest.id)
-      end
     end
 
     def update_parent_on_save
-      if parent_id == parent_id_was
+      return unless parent_id_changed? || is_deleted_changed?
-        Post.update_has_children_flag_for(parent_id)
+
-      elsif !parent_id_was.nil?
+      parent.update_has_children_flag if parent.present?
-        Post.update_has_children_flag_for(parent_id)
+      Post.find(parent_id_was).update_has_children_flag if parent_id_was.present?
-        Post.update_has_children_flag_for(parent_id_was)
-      else
-        Post.update_has_children_flag_for(parent_id)
-      end
     end
 
     def give_favorites_to_parent
@@ -1379,17 +1362,20 @@ class Post < ApplicationRecord
         return false
       end
 
-      ModAction.log("permanently deleted post ##{id}")
+      transaction do
-      delete!("Permanently deleted post ##{id}", :without_mod_action => true)
+        Post.without_timeout do
-      Post.without_timeout do
+          ModAction.log("permanently deleted post ##{id}")
-        give_favorites_to_parent
+          delete!("Permanently deleted post ##{id}", :without_mod_action => true)
-        update_children_on_destroy
+
-        decrement_tag_post_counts
+          give_favorites_to_parent
-        remove_from_all_pools
+          update_children_on_destroy
-        remove_from_fav_groups
+          decrement_tag_post_counts
-        remove_from_favorites
+          remove_from_all_pools
-        destroy
+          remove_from_fav_groups
-        update_parent_on_destroy
+          remove_from_favorites
+          destroy
+          update_parent_on_destroy
+        end
       end
     end
 

app/presenters/post_presenter.rb

@@ -189,7 +189,7 @@ class PostPresenter < Presenter
   end
 
   def has_nav_links?(template)
-    (CurrentUser.user.enable_sequential_post_navigation && template.params[:tags].present? && template.params[:tags] !~ /(?:^|\s)(?:order|ordfav|ordpool):/) || @post.pools.any? || @post.favorite_groups(active_id=template.params[:favgroup_id]).any?
+    (CurrentUser.user.enable_sequential_post_navigation && template.params[:tags].present? && template.params[:tags] !~ /(?:^|\s)(?:order|ordfav|ordpool):/) || @post.pools.undeleted.any? || @post.favorite_groups(active_id=template.params[:favgroup_id]).any?
   end
 
   def post_footer_for_pool_html(template)
@@ -211,13 +211,13 @@ class PostPresenter < Presenter
       return if pool.nil?
       html += pool_link_html(template, pool, :include_rel => true)
 
-      other_pools = @post.pools.where("id <> ?", template.params[:pool_id]).series_first
+      other_pools = @post.pools.undeleted.where("id <> ?", template.params[:pool_id]).series_first
       other_pools.each do |other_pool|
         html += pool_link_html(template, other_pool)
       end
     else
       first = true
-      pools = @post.pools.series_first
+      pools = @post.pools.undeleted
       pools.each do |pool|
         if first && template.params[:tags].blank? && template.params[:favgroup_id].blank?
           html += pool_link_html(template, pool, :include_rel => true)

app/views/moderator/post/queues/_post.html.erb

@@ -35,7 +35,7 @@
       </li>
       <li><strong>Source</strong>: <%= post.source %></li>
       <% if post.has_active_pools? %>
-        <li><strong>Pools</strong>: <%= render "pools/inline_list", pools: post.pools %></li>
+        <li><strong>Pools</strong>: <%= render "pools/inline_list", pools: post.pools.undeleted %></li>
       <% end %>
       <li><strong>Tags</strong>: <%= post.presenter.inline_tag_list_html(self) %></li>
     </ul>

app/views/posts/partials/show/_nav_links.html.erb

@@ -4,7 +4,7 @@
       <%= render "posts/partials/show/search_seq", :post => post %>
     <% end %>
 
-    <% if post.pools.any? %>
+    <% if post.pools.undeleted.any? %>
       <%= render "posts/partials/show/pools", :post => post %>
     <% end %>
 

test/unit/post_test.rb

@@ -86,15 +86,24 @@ class PostTest < ActiveSupport::TestCase
           SqsService.any_instance.stubs(:send_message)
           @pool = FactoryGirl.create(:pool)
           @pool.add!(@post)
-          @post.reload
+
+          @deleted_pool = FactoryGirl.create(:pool)
+          @deleted_pool.add!(@post)
+          @deleted_pool.update_columns(is_deleted: true)
+
           @post.expunge!
+          @pool.reload
+          @deleted_pool.reload
         end
 
         should "remove the post from all pools" do
-          @pool.reload
           assert_equal("", @pool.post_ids)
         end
 
+        should "remove the post from deleted pools" do
+          assert_equal("", @deleted_pool.post_ids)
+        end
+
         should "destroy the record" do
           assert_equal([], @post.errors.full_messages)
           assert_equal(0, Post.where("id = ?", @post.id).count)
@@ -245,18 +254,39 @@ class PostTest < ActiveSupport::TestCase
       end
 
       context "two or more children" do
+        setup do
+          # ensure initial post versions won't be merged.
+          travel_to(1.day.ago) do
+            @p1 = FactoryGirl.create(:post)
+            @c1 = FactoryGirl.create(:post, :parent_id => @p1.id)
+            @c2 = FactoryGirl.create(:post, :parent_id => @p1.id)
+            @c3 = FactoryGirl.create(:post, :parent_id => @p1.id)
+          end
+        end
+
         should "reparent all children to the first child" do
-          p1 = FactoryGirl.create(:post)
+          @p1.expunge!
-          c1 = FactoryGirl.create(:post, :parent_id => p1.id)
+          @c1.reload
-          c2 = FactoryGirl.create(:post, :parent_id => p1.id)
+          @c2.reload
-          c3 = FactoryGirl.create(:post, :parent_id => p1.id)
+          @c3.reload
-          p1.expunge!
+
-          c1.reload
+          assert_nil(@c1.parent_id)
-          c2.reload
+          assert_equal(@c1.id, @c2.parent_id)
-          c3.reload
+          assert_equal(@c1.id, @c3.parent_id)
-          assert_nil(c1.parent_id)
+        end
-          assert_equal(c1.id, c2.parent_id)
+
-          assert_equal(c1.id, c3.parent_id)
+        should "save a post version record for each child" do
+          assert_difference(["@c1.versions.count", "@c2.versions.count", "@c3.versions.count"]) do
+            @p1.expunge!
+            @c1.reload
+            @c2.reload
+            @c3.reload
+          end
+        end
+
+        should "set the has_children flag on the new parent" do
+          @p1.expunge!
+          assert_equal(true, @c1.reload.has_children?)
         end
       end
     end