Merge pull request #3277 from techAPJ/patch-1

FIX: return proper error message when replying to a closed or deleted topic

config/locales/server.en.yml

@@ -151,6 +151,7 @@ en:
     other: "Sorry, new users can only put %{count} links in a post."
   spamming_host: "Sorry you cannot post a link to that host."
   user_is_suspended: "Suspended users are not allowed to post."
+  topic_not_found: "Something has gone wrong. Perhaps this topic was closed or deleted while you were looking at it?"
 
   just_posted_that: "is too similar to what you recently posted"
   has_already_been_used: "has already been used"
@@ -2322,4 +2323,3 @@ en:
         Flagging is critical to the health of your community. If you notice any posts that require moderator attention please
         do not hesitate to flag. You may also use the flagging dialog to send <b>private messages</b> to users once you reach
         trust level 1.
-

lib/post_creator.rb

@@ -205,7 +205,11 @@ class PostCreator
       end
     else
       topic = Topic.find_by(id: @opts[:topic_id])
-      guardian.ensure_can_create!(Post, topic)
+      if (topic.blank? || !guardian.can_create?(Post, topic))
+        @errors = Post.new.errors
+        @errors.add(:base, I18n.t(:topic_not_found))
+        raise ActiveRecord::Rollback.new
+      end
     end
     @topic = topic
   end

spec/components/post_creator_spec.rb

@@ -332,7 +332,10 @@ describe PostCreator do
 
     it 'ensures the user can create the post' do
       Guardian.any_instance.expects(:can_create?).with(Post, topic).returns(false)
-      expect { creator.create }.to raise_error(Discourse::InvalidAccess)
+      post = creator.create
+      expect(post).to be_blank
+      expect(creator.errors.count).to eq 1
+      expect(creator.errors.messages[:base][0]).to match I18n.t(:topic_not_found)
     end
 
     context 'success' do
@@ -347,6 +350,30 @@ describe PostCreator do
 
   end
 
+  context 'closed topic' do
+    let!(:topic) { Fabricate(:topic, user: user, closed: true) }
+    let(:creator) { PostCreator.new(user, raw: 'test reply', topic_id: topic.id, reply_to_post_number: 4) }
+
+    it 'responds with an error message' do
+      post = creator.create
+      expect(post).to be_blank
+      expect(creator.errors.count).to eq 1
+      expect(creator.errors.messages[:base][0]).to match I18n.t(:topic_not_found)
+    end
+  end
+
+  context 'missing topic' do
+    let!(:topic) { Fabricate(:topic, user: user, deleted_at: 5.minutes.ago) }
+    let(:creator) { PostCreator.new(user, raw: 'test reply', topic_id: topic.id, reply_to_post_number: 4) }
+
+    it 'responds with an error message' do
+      post = creator.create
+      expect(post).to be_blank
+      expect(creator.errors.count).to eq 1
+      expect(creator.errors.messages[:base][0]).to match I18n.t(:topic_not_found)
+    end
+  end
+
   context "cooking options" do
     let(:raw) { "this is my awesome message body hello world" }
 

spec/integration/spam_rules_spec.rb

@@ -41,7 +41,7 @@ describe SpamRulesEnforcer do
 
           Invariant { expect(Guardian.new(spammer).can_create_topic?(nil)).to be false }
           Invariant { expect{PostCreator.create(spammer, {title: 'limited time offer for you', raw: 'better buy this stuff ok', archetype_id: 1})}.to raise_error(Discourse::InvalidAccess) }
-          Invariant { expect{PostCreator.create(spammer, {topic_id: another_topic.id, raw: 'my reply is spam in your topic', archetype_id: 1})}.to raise_error(Discourse::InvalidAccess) }
+          Invariant { PostCreator.create(spammer, {topic_id: another_topic.id, raw: 'my reply is spam in your topic', archetype_id: 1}).should == nil }
 
           Then { expect(spammer.reload).to be_blocked }
           And  { expect(spam_post.reload).to be_hidden }