FIX: Ensure soft-deleted topics can be deleted (#19802)

* FIX: Ensure soft-deleted topics can be deleted

The topic was not found during the deletion process because it was
deleted and `@post.topic` was nil.

* DEV: Use @topic instead of finding the topic every time
This commit is contained in:
Bianca Nenciu 2023-01-27 16:15:33 +02:00 committed by GitHub
parent bffb15e13b
commit 8fc11215e1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 59 additions and 35 deletions

View File

@ -674,7 +674,10 @@ export default Controller.extend(bufferedProperty("model"), {
deletePost(post, opts) {
if (post.get("post_number") === 1) {
return this.deleteTopic(opts);
} else if (!post.can_delete) {
} else if (
(!opts?.force_destroy && !post.can_delete) ||
(opts?.force_destroy && !post.can_permanently_delete)
) {
return false;
}

View File

@ -468,14 +468,15 @@ const Topic = RestModel.extend({
this.siteSettings.can_permanently_delete && deleted_by.admin,
});
if (
!deleted_by.staff &&
!deleted_by.groups.some(
(group) => group.name === this.category.reviewable_by_group_name
) &&
!(
this.siteSettings.tl4_delete_posts_and_topics &&
deleted_by.trust_level >= 4
)
opts.force_destroy ||
(!deleted_by.staff &&
!deleted_by.groups.some(
(group) => group.name === this.category.reviewable_by_group_name
) &&
!(
this.siteSettings.tl4_delete_posts_and_topics &&
deleted_by.trust_level >= 4
))
) {
DiscourseURL.redirectTo("/");
}

View File

@ -649,7 +649,9 @@ class TopicsController < ApplicationController
force_destroy = ActiveModel::Type::Boolean.new.cast(params[:force_destroy])
if force_destroy
if !guardian.can_permanently_delete?(topic)
if !topic
raise Discourse::InvalidAccess
elsif !guardian.can_permanently_delete?(topic)
return render_json_error topic.cannot_permanently_delete_reason(current_user), status: 403
end
else

View File

@ -55,18 +55,17 @@ class PostDestroyer
def initialize(user, post, opts = {})
@user = user
@post = post
@topic = post.topic if post
@topic = post.topic || Topic.with_deleted.find_by(id: @post.topic_id)
@opts = opts
end
def destroy
payload = WebHook.generate_payload(:post, @post) if WebHook.active_web_hooks(:post).exists?
topic = Topic.with_deleted.find_by(id: @post.topic_id)
is_first_post = @post.is_first_post? && topic
is_first_post = @post.is_first_post? && @topic
has_topic_web_hooks = is_first_post && WebHook.active_web_hooks(:topic).exists?
if has_topic_web_hooks
topic_view = TopicView.new(topic.id, Discourse.system_user, skip_staff_action: true)
topic_view = TopicView.new(@topic.id, Discourse.system_user, skip_staff_action: true)
topic_payload = WebHook.generate_payload(:topic, topic_view, WebHookTopicViewSerializer)
end
@ -74,7 +73,7 @@ class PostDestroyer
@opts[:delete_removed_posts_after] || SiteSetting.delete_removed_posts_after
if delete_removed_posts_after < 1 || post_is_reviewable? ||
Guardian.new(@user).can_moderate_topic?(topic) || permanent?
Guardian.new(@user).can_moderate_topic?(@topic) || permanent?
perform_delete
elsif @user.id == @post.user_id
mark_for_deletion(delete_removed_posts_after)
@ -84,13 +83,13 @@ class PostDestroyer
DiscourseEvent.trigger(:post_destroyed, @post, @opts, @user)
WebHook.enqueue_post_hooks(:post_destroyed, @post, payload)
Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic
Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @topic.id) if @topic
if is_first_post
UserProfile.remove_featured_topic_from_all_profiles(topic)
UserActionManager.topic_destroyed(topic)
DiscourseEvent.trigger(:topic_destroyed, topic, @user)
WebHook.enqueue_topic_hooks(:topic_destroyed, topic, topic_payload) if has_topic_web_hooks
UserProfile.remove_featured_topic_from_all_profiles(@topic)
UserActionManager.topic_destroyed(@topic)
DiscourseEvent.trigger(:topic_destroyed, @topic, @user)
WebHook.enqueue_topic_hooks(:topic_destroyed, @topic, topic_payload) if has_topic_web_hooks
end
end
@ -101,23 +100,23 @@ class PostDestroyer
elsif @user.staff? || @user.id == @post.user_id
user_recovered
end
topic = Topic.with_deleted.find @post.topic_id
topic.update_column(:user_id, Discourse::SYSTEM_USER_ID) if !topic.user_id
topic.recover!(@user) if @post.is_first_post?
topic.update_statistics
Topic.publish_stats_to_clients!(topic.id, :recovered)
@topic.update_column(:user_id, Discourse::SYSTEM_USER_ID) if !@topic.user_id
@topic.recover!(@user) if @post.is_first_post?
@topic.update_statistics
Topic.publish_stats_to_clients!(@topic.id, :recovered)
UserActionManager.post_created(@post)
DiscourseEvent.trigger(:post_recovered, @post, @opts, @user)
Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic
Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @topic.id) if @topic
Jobs.enqueue(:notify_mailing_list_subscribers, post_id: @post.id)
if @post.is_first_post?
UserActionManager.topic_created(topic)
DiscourseEvent.trigger(:topic_recovered, topic, @user)
UserActionManager.topic_created(@topic)
DiscourseEvent.trigger(:topic_recovered, @topic, @user)
if @user.id != @post.user_id
StaffActionLogger.new(@user).log_topic_delete_recover(
topic,
@topic,
"recover_topic",
@opts.slice(:context),
)
@ -194,10 +193,11 @@ class PostDestroyer
end
end
if @post.topic && @post.is_first_post?
permanent? ? @post.topic.destroy! : @post.topic.trash!(@user)
PublishedPage.unpublish!(@user, @post.topic) if @post.topic.published_page
if @topic && @post.is_first_post?
permanent? ? @topic.destroy! : @topic.trash!(@user)
PublishedPage.unpublish!(@user, @topic) if @topic.published_page
end
TopicLink.where(link_post_id: @post.id).destroy_all
update_associated_category_latest_topic
update_user_counts if !permanent?
@ -281,8 +281,7 @@ class PostDestroyer
def post_is_reviewable?
return true if @user.staff?
topic = @post.topic || Topic.with_deleted.find(@post.topic_id)
Guardian.new(@user).can_review_topic?(topic) && Reviewable.exists?(target: @post)
Guardian.new(@user).can_review_topic?(@topic) && Reviewable.exists?(target: @post)
end
# we need topics to change if ever a post in them is deleted or created

View File

@ -1116,13 +1116,32 @@ RSpec.describe PostDestroyer do
expect(post_revision.reload.persisted?).to be true
end
it "always destroy the post when the force_destroy option is passed" do
it "destroys the post when force_destroy is true for soft deleted topics" do
post = Fabricate(:post)
topic = post.topic
PostDestroyer.new(moderator, post).destroy
post = Post.with_deleted.find_by(id: post.id)
expect(post).not_to eq(nil)
PostDestroyer.new(moderator, post, force_destroy: true).destroy
post = Post.with_deleted.find_by(id: post.id)
expect(post).to eq(nil)
topic = Topic.with_deleted.find_by(id: topic.id)
expect(topic).to eq(nil)
end
it "destroys the post when force_destroy is true for regular posts" do
PostDestroyer.new(moderator, reply, force_destroy: true).destroy
expect { reply.reload }.to raise_error(ActiveRecord::RecordNotFound)
regular_post = Fabricate(:post)
topic = regular_post.topic
PostDestroyer.new(moderator, regular_post, force_destroy: true).destroy
expect { regular_post.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { topic.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end