Merge pull request #4739 from tgxworld/fix_cant_recover_a_topic_that_belongs_to_a_deleted_user

Fix cant recover a topic that belongs to a deleted user

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)

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)

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)

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
+    context 'as a moderator' do
-      expect(Guardian.new(moderator).can_recover_topic?(topic)).to be_truthy
+      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
+    context 'as a moderator' do
-      expect(Guardian.new(moderator).can_recover_post?(post)).to be_truthy
+      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

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) }
+        let(:user) { log_in(:admin) }
-
+        let(:p2) { Fabricate(:post, user: user, topic: topic) }
-        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
 
         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) }