From 3ed14ae7823cc2ec58aa8359781d35ab208fc11b Mon Sep 17 00:00:00 2001 From: evazion Date: Fri, 21 Jul 2017 00:10:07 -0500 Subject: [PATCH 1/6] Post#expunge!: wrap in transaction. --- app/models/post.rb | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 0c99784c9..8dfe1e177 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1379,17 +1379,20 @@ class Post < ApplicationRecord return false end - ModAction.log("permanently deleted post ##{id}") - delete!("Permanently deleted post ##{id}", :without_mod_action => true) - Post.without_timeout do - give_favorites_to_parent - update_children_on_destroy - decrement_tag_post_counts - remove_from_all_pools - remove_from_fav_groups - remove_from_favorites - destroy - update_parent_on_destroy + transaction do + Post.without_timeout do + ModAction.log("permanently deleted post ##{id}") + delete!("Permanently deleted post ##{id}", :without_mod_action => true) + + give_favorites_to_parent + update_children_on_destroy + decrement_tag_post_counts + remove_from_all_pools + remove_from_fav_groups + remove_from_favorites + destroy + update_parent_on_destroy + end end end From bac8ff4de0eadb86943efd046f566914fb8c0ffe Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 19 Jul 2017 22:58:59 -0500 Subject: [PATCH 2/6] Post#expunge!: destroy post replacements on expunge. --- app/models/post.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/post.rb b/app/models/post.rb index 8dfe1e177..bbbd4cca5 100644 --- a/app/models/post.rb +++ b/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 From 1b310dcc0b06894d119f67eabed801140036a2eb Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 20 Jul 2017 21:00:13 -0500 Subject: [PATCH 3/6] Post#expunge!: fix remove_from_all_pools to clear deleted pools. * Change Post#pools to return all pools, including deleted pools. This fixes remove_all_from_pools to remove the post from deleted pools too. * Change other users of Post#pools to explicitly select undeleted pools. --- app/models/post.rb | 6 +++--- app/presenters/post_presenter.rb | 6 +++--- app/views/moderator/post/queues/_post.html.erb | 2 +- app/views/posts/partials/show/_nav_links.html.erb | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index bbbd4cca5..f095f3092 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -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) @@ -1101,7 +1101,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 diff --git a/app/presenters/post_presenter.rb b/app/presenters/post_presenter.rb index 9e3af7ee1..5c3b8c347 100644 --- a/app/presenters/post_presenter.rb +++ b/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) diff --git a/app/views/moderator/post/queues/_post.html.erb b/app/views/moderator/post/queues/_post.html.erb index 91826bcc8..3369e8045 100644 --- a/app/views/moderator/post/queues/_post.html.erb +++ b/app/views/moderator/post/queues/_post.html.erb @@ -35,7 +35,7 @@
  • Source: <%= post.source %>
    • <% if post.has_active_pools? %> -
  • Pools: <%= render "pools/inline_list", pools: post.pools %>
    • +
  • Pools: <%= render "pools/inline_list", pools: post.pools.undeleted %>
    • <% end %>
  • Tags: <%= post.presenter.inline_tag_list_html(self) %>
    • diff --git a/app/views/posts/partials/show/_nav_links.html.erb b/app/views/posts/partials/show/_nav_links.html.erb index acefd87aa..306e4194f 100644 --- a/app/views/posts/partials/show/_nav_links.html.erb +++ b/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 %> From fbee7f6912080fab47221d4668a6adc73e3e74b2 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 20 Jul 2017 21:06:37 -0500 Subject: [PATCH 4/6] Post#expunge!: fix remove_pool! to remove posts from deleted pools. Don't silently ignore attempts to remove posts from deleted pools. Remove the restriction on removing posts from deleted pools instead (ref: #1109). Fixes failure to remove posts from deleted pools during expungement. --- app/models/pool.rb | 5 ++--- app/models/post.rb | 3 +-- test/unit/post_test.rb | 13 +++++++++++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/models/pool.rb b/app/models/pool.rb index eaae96384..42a84c5b2 100644 --- a/app/models/pool.rb +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index f095f3092..04f974c82 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -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 diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index a1d122492..d0b760fb2 100644 --- a/test/unit/post_test.rb +++ b/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) From 44f6673d941ca528eca5a085eac521752716ac75 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 20 Jul 2017 21:53:53 -0500 Subject: [PATCH 5/6] Post#expunge!: run callbacks when reparenting children. * Set parent IDs with `update` instead of `update_column` / `update_all` when reparenting children. This fixes it so that new post versions are saved and the has_children flag is set on the new parent. * Slightly simplify logic of update_children_on_destroy: the single child case is subsumed by the multi-child case. --- app/models/post.rb | 19 ++++++++----------- test/unit/post_test.rb | 43 +++++++++++++++++++++++++++++++----------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 04f974c82..6c82a89ce 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1314,17 +1314,14 @@ class Post < ApplicationRecord end def update_children_on_destroy - if children.size == 0 - # do nothing - elsif children.size == 1 - children.first.update_column(:parent_id, nil) - else - cached_children = children - eldest = cached_children[0] - siblings = cached_children[1..-1] - eldest.update_column(:parent_id, nil) - Post.where(:id => siblings.map(&:id)).update_all(:parent_id => eldest.id) - end + return unless children.present? + + eldest = children[0] + siblings = children[1..-1] + + eldest.update(parent_id: nil) + Post.where(id: siblings).find_each { |p| p.update(parent_id: eldest.id) } + # Post.where(id: siblings).update(parent_id: eldest.id) # XXX rails 5 end def update_parent_on_save diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index d0b760fb2..4b5496136 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -254,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) - c1 = FactoryGirl.create(:post, :parent_id => p1.id) - c2 = FactoryGirl.create(:post, :parent_id => p1.id) - c3 = FactoryGirl.create(:post, :parent_id => p1.id) - p1.expunge! - c1.reload - c2.reload - c3.reload - assert_nil(c1.parent_id) - assert_equal(c1.id, c2.parent_id) - assert_equal(c1.id, c3.parent_id) + @p1.expunge! + @c1.reload + @c2.reload + @c3.reload + + assert_nil(@c1.parent_id) + assert_equal(@c1.id, @c2.parent_id) + assert_equal(@c1.id, @c3.parent_id) + end + + 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 From bda285d97f8e349d8822172aaffb1d518ef80a3e Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 20 Jul 2017 22:11:43 -0500 Subject: [PATCH 6/6] Post#expunge!: simplify has_children flag logic. --- app/models/post.rb | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 6c82a89ce..34c769f39 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -1283,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_for(post_id) - 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) + def update_has_children_flag + update({has_children: children.exists?, has_active_children: children.undeleted.exists?}, without_protection: true) end def blank_out_nonexistent_parents @@ -1310,7 +1301,7 @@ 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 @@ -1325,14 +1316,10 @@ class Post < ApplicationRecord end def update_parent_on_save - if parent_id == parent_id_was - Post.update_has_children_flag_for(parent_id) - elsif !parent_id_was.nil? - Post.update_has_children_flag_for(parent_id) - Post.update_has_children_flag_for(parent_id_was) - else - Post.update_has_children_flag_for(parent_id) - end + return unless parent_id_changed? || is_deleted_changed? + + parent.update_has_children_flag if parent.present? + Post.find(parent_id_was).update_has_children_flag if parent_id_was.present? end def give_favorites_to_parent