From a28704bcee45460b245f92ba59b05b4f25e595a1 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 6 Mar 2017 13:17:57 +0800 Subject: [PATCH 1/2] FIX: Can't recover a post when its user has been deleted. https://meta.discourse.org/t/moving-posts-to-new-topic/58436 --- lib/guardian/post_guardian.rb | 6 +++- lib/guardian/topic_guardian.rb | 2 +- spec/components/guardian_spec.rb | 54 +++++++++++++++++++++++++++++--- 3 files changed, 56 insertions(+), 6 deletions(-) diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index b29467ceede..ee72eaf465b 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -149,7 +149,11 @@ module PostGuardian # Recovery Method def can_recover_post?(post) - is_staff? || (is_my_own?(post) && post.user_deleted && !post.deleted_at) + if is_staff? + post.deleted_at && post.user + else + is_my_own?(post) && post.user_deleted && !post.deleted_at + end end def can_delete_post_action?(post_action) diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index ea086ef1997..add956351be 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -52,7 +52,7 @@ module TopicGuardian # Recovery Method def can_recover_topic?(topic) - is_staff? + topic && topic.deleted_at && topic.user && is_staff? end def can_delete_topic?(topic) diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index a140c3c0744..702344884ae 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -880,8 +880,30 @@ describe Guardian do expect(Guardian.new(user).can_recover_topic?(topic)).to be_falsey end - it "returns true for a moderator" do - expect(Guardian.new(moderator).can_recover_topic?(topic)).to be_truthy + context 'as a moderator' do + before do + topic.save! + post.save! + end + + describe 'when post has been deleted' do + it "should return the right value" do + expect(Guardian.new(moderator).can_recover_topic?(topic)).to be_falsey + + PostDestroyer.new(moderator, topic.first_post).destroy + + expect(Guardian.new(moderator).can_recover_topic?(topic.reload)).to be_truthy + end + end + + describe "when post's user has been deleted" do + it 'should return the right value' do + PostDestroyer.new(moderator, topic.first_post).destroy + topic.first_post.user.destroy! + + expect(Guardian.new(moderator).can_recover_topic?(topic.reload)).to be_falsey + end + end end end @@ -899,8 +921,32 @@ describe Guardian do expect(Guardian.new(user).can_recover_post?(post)).to be_falsey end - it "returns true for a moderator" do - expect(Guardian.new(moderator).can_recover_post?(post)).to be_truthy + context 'as a moderator' do + let(:other_post) { Fabricate(:post, topic: topic, user: topic.user) } + + before do + topic.save! + post.save! + end + + describe 'when post has been deleted' do + it "should return the right value" do + expect(Guardian.new(moderator).can_recover_post?(post)).to be_falsey + + PostDestroyer.new(moderator, post).destroy + + expect(Guardian.new(moderator).can_recover_post?(post.reload)).to be_truthy + end + + describe "when post's user has been deleted" do + it 'should return the right value' do + PostDestroyer.new(moderator, post).destroy + post.user.destroy! + + expect(Guardian.new(moderator).can_recover_post?(post.reload)).to be_falsey + end + end + end end end From 477eb0591e55a6d89d6b1928d6ddd8575a3ced73 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 6 Mar 2017 14:55:58 +0800 Subject: [PATCH 2/2] FIX: Posts in a deleted topic couldn't be moved. https://meta.discourse.org/t/moving-posts-to-new-topic/58436/4 --- app/controllers/topics_controller.rb | 2 +- spec/controllers/topics_controller_spec.rb | 48 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index f5bddc8981a..e3f70dc40b9 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -497,7 +497,7 @@ class TopicsController < ApplicationController params.require(:topic_id) params.permit(:category_id) - topic = Topic.find_by(id: params[:topic_id]) + topic = Topic.with_deleted.find_by(id: params[:topic_id]) guardian.ensure_can_move_posts!(topic) dest_topic = move_posts_to_destination(topic) diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 67342edd042..4a5885ff60d 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -64,7 +64,7 @@ describe TopicsController do end describe 'moving to a new topic' do - let!(:user) { log_in(:moderator) } + let(:user) { log_in(:moderator) } let(:p1) { Fabricate(:post, user: user) } let(:topic) { p1.topic } @@ -79,18 +79,49 @@ describe TopicsController do end context 'success' do - let(:p2) { Fabricate(:post, user: user) } - - before do - Topic.any_instance.expects(:move_posts).with(user, [p2.id], title: 'blah', category_id: 123).returns(topic) - xhr :post, :move_posts, topic_id: topic.id, title: 'blah', post_ids: [p2.id], category_id: 123 - end + let(:user) { log_in(:admin) } + let(:p2) { Fabricate(:post, user: user, topic: topic) } it "returns success" do + p2 + + expect do + xhr :post, :move_posts, + topic_id: topic.id, + title: 'Logan is a good movie', + post_ids: [p2.id], + category_id: 123 + end.to change { Topic.count }.by(1) + expect(response).to be_success + result = ::JSON.parse(response.body) + expect(result['success']).to eq(true) - expect(result['url']).to be_present + expect(result['url']).to eq(Topic.last.relative_url) + end + + describe 'when topic has been deleted' do + it 'should still be able to move posts' do + PostDestroyer.new(user, topic.first_post).destroy + + expect(topic.reload.deleted_at).to_not be_nil + + expect do + xhr :post, :move_posts, + topic_id: topic.id, + title: 'Logan is a good movie', + post_ids: [p2.id], + category_id: 123 + end.to change { Topic.count }.by(1) + + expect(response).to be_success + + result = JSON.parse(response.body) + + expect(result['success']).to eq(true) + expect(result['url']).to eq(Topic.last.relative_url) + end end end @@ -131,7 +162,6 @@ describe TopicsController do end - describe 'moving to an existing topic' do let!(:user) { log_in(:moderator) } let(:p1) { Fabricate(:post, user: user) }