NewPostManager determines whether to queue a post or not

.travis.yml

@@ -8,6 +8,9 @@ env:
     - "RAILS_MASTER=1"
     - "RAILS_MASTER=0"
 
+addons:
+  postgresql: 9.3
+
 matrix:
   allow_failures:
     - rvm: 2.0.0

app/controllers/application_controller.rb

@@ -198,9 +198,9 @@ class ApplicationController < ActionController::Base
     @guardian ||= Guardian.new(current_user)
   end
 
-  def serialize_data(obj, serializer, opts={})
+  def serialize_data(obj, serializer, opts=nil)
     # If it's an array, apply the serializer as an each_serializer to the elements
-    serializer_opts = {scope: guardian}.merge!(opts)
+    serializer_opts = {scope: guardian}.merge!(opts || {})
     if obj.respond_to?(:to_ary)
       serializer_opts[:each_serializer] = serializer
       ActiveModel::ArraySerializer.new(obj.to_ary, serializer_opts).as_json
@@ -213,12 +213,13 @@ class ApplicationController < ActionController::Base
   # 20% slower than calling MultiJSON.dump ourselves. I'm not sure why
   # Rails doesn't call MultiJson.dump when you pass it json: obj but
   # it seems we don't need whatever Rails is doing.
-  def render_serialized(obj, serializer, opts={})
-    render_json_dump(serialize_data(obj, serializer, opts))
+  def render_serialized(obj, serializer, opts=nil)
+    render_json_dump(serialize_data(obj, serializer, opts), opts)
   end
 
-  def render_json_dump(obj)
-    render json: MultiJson.dump(obj)
+  def render_json_dump(obj, opts=nil)
+    opts ||= {}
+    render json: MultiJson.dump(obj), status: opts[:status] || 200
   end
 
   def can_cache_content?

app/controllers/posts_controller.rb

@@ -1,6 +1,8 @@
+require_dependency 'new_post_manager'
 require_dependency 'post_creator'
 require_dependency 'post_destroyer'
 require_dependency 'distributed_memoizer'
+require_dependency 'new_post_result_serializer'
 
 class PostsController < ApplicationController
 
@@ -76,49 +78,21 @@ class PostsController < ApplicationController
   end
 
   def create
-    params = create_params
+    @manager_params = create_params
+    manager = NewPostManager.new(current_user, @manager_params)
 
-    key = params_key(params)
-    error_json = nil
-
-    if (is_api?)
-      payload = DistributedMemoizer.memoize(key, 120) do
-        success, json = create_post(params)
-        unless success
-          error_json = json
-          raise Discourse::InvalidPost
-        end
-        json
+    if is_api?
+      memoized_payload = DistributedMemoizer.memoize(signature_for(@manager_params), 120) do
+        result = manager.perform
+        MultiJson.dump(serialize_data(result, NewPostResultSerializer, root: false))
       end
+
+      parsed_payload = JSON.parse(memoized_payload)
+      backwards_compatible_json(parsed_payload, parsed_payload['success'])
     else
-      success, payload = create_post(params)
-      unless success
-        error_json = payload
-        raise Discourse::InvalidPost
-      end
-    end
-
-    render json: payload
-
-  rescue Discourse::InvalidPost
-    render json: error_json, status: 422
-  end
-
-  def create_post(params)
-    post_creator = PostCreator.new(current_user, params)
-    post = post_creator.create
-
-    if post_creator.errors.present?
-      # If the post was spam, flag all the user's posts as spam
-      current_user.flag_linked_posts_as_spam if post_creator.spam?
-      [false, MultiJson.dump(errors: post_creator.errors.full_messages)]
-
-    else
-      DiscourseEvent.trigger(:topic_created, post.topic, params, current_user) unless params[:topic_id]
-      DiscourseEvent.trigger(:post_created, post, params, current_user)
-      post_serializer = PostSerializer.new(post, scope: guardian, root: false)
-      post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key)
-      [true, MultiJson.dump(post_serializer)]
+      result = manager.perform
+      json = serialize_data(result, NewPostResultSerializer, root: false)
+      backwards_compatible_json(json, result.success?)
     end
   end
 
@@ -358,6 +332,15 @@ class PostsController < ApplicationController
 
   protected
 
+  # We can't break the API for making posts. The new, queue supporting API
+  # doesn't return the post as the root JSON object, but as a nested object.
+  # If a param is present it uses that result structure.
+  def backwards_compatible_json(json_obj, success)
+    json_obj = json_obj[:post] || json_obj['post'] unless params[:nested_post]
+    render json: json_obj, status: (!!success) ? 200 : 422
+  end
+
+
   def find_post_revision_from_params
     post_id = params[:id] || params[:post_id]
     revision = params[:revision].to_i
@@ -411,15 +394,6 @@ class PostsController < ApplicationController
          .limit(opts[:limit])
   end
 
-  def params_key(params)
-    "post##" << Digest::SHA1.hexdigest(params
-      .to_a
-      .concat([["user", current_user.id]])
-      .sort{|x,y| x[0] <=> y[0]}.join do |x,y|
-        "#{x}:#{y}"
-      end)
-  end
-
   def create_params
     permitted = [
       :raw,
@@ -454,6 +428,8 @@ class PostsController < ApplicationController
     if current_user.staff?
       params.permit(:is_warning)
       result[:is_warning] = (params[:is_warning] == "true")
+    else
+      result[:is_warning] = false
     end
 
     PostRevisor.tracked_topic_fields.keys.each do |f|
@@ -469,6 +445,15 @@ class PostsController < ApplicationController
     result
   end
 
+  def signature_for(args)
+    "post##" << Digest::SHA1.hexdigest(args
+      .to_a
+      .concat([["user", current_user.id]])
+      .sort{|x,y| x[0] <=> y[0]}.join do |x,y|
+        "#{x}:#{y}"
+      end)
+  end
+
   def too_late_to(action, post)
     !guardian.send("can_#{action}?", post) && post.user_id == current_user.id && post.edit_time_limit_expired?
   end

app/serializers/new_post_result_serializer.rb

@@ -0,0 +1,39 @@
+require_dependency 'application_serializer'
+
+class NewPostResultSerializer < ApplicationSerializer
+  attributes :action,
+             :post,
+             :errors,
+             :success
+
+  def post
+    post_serializer = PostSerializer.new(object.post, scope: scope, root: false)
+    post_serializer.draft_sequence = DraftSequence.current(scope.user, object.post.topic.draft_key)
+    post_serializer.as_json
+  end
+
+  def include_post?
+    object.post.present?
+  end
+
+  def success
+    true
+  end
+
+  def include_success?
+    @object.success?
+  end
+
+  def errors
+    object.errors.full_messages
+  end
+
+  def include_errors?
+    !object.errors.empty?
+  end
+
+  def action
+    object.action
+  end
+
+end

lib/discourse.rb

@@ -47,8 +47,6 @@ module Discourse
   # When ImageMagick is missing
   class ImageMagickMissing < StandardError; end
 
-  class InvalidPost < StandardError; end
-
   # When read-only mode is enabled
   class ReadOnly < StandardError; end
 

lib/discourse_event.rb

@@ -17,6 +17,10 @@ module DiscourseEvent
     events[event_name] << block
   end
 
+  def self.off(event_name, &block)
+    events[event_name].delete(block)
+  end
+
   def self.clear
     @events = nil
   end

lib/guardian/post_guardian.rb

@@ -1,5 +1,6 @@
 #mixin for all guardian methods dealing with post permissions
 module PostGuardian
+
   # Can the user act on the post in a particular way.
   #  taken_actions = the list of actions the user has already taken
   def post_can_act?(post, action_key, opts={})

lib/new_post_manager.rb

@@ -0,0 +1,66 @@
+require_dependency 'post_creator'
+require_dependency 'new_post_result'
+require_dependency 'post_enqueuer'
+
+# Determines what actions should be taken with new posts.
+#
+# The default action is to create the post, but this can be extended
+# with `NewPostManager.add_handler` to take other approaches depending
+# on the user or input.
+class NewPostManager
+
+  attr_reader :user, :args
+
+  def self.handlers
+    @handlers ||= Set.new
+  end
+
+  def self.add_handler(&block)
+    handlers << block
+  end
+
+  def initialize(user, args)
+    @user = user
+    @args = args
+  end
+
+  def perform
+
+    # Perform handlers until one returns a result
+    handled = NewPostManager.handlers.any? do |handler|
+      result = handler.call(self)
+      return result if result
+
+      false
+    end
+
+    perform_create_post unless handled
+  end
+
+  # Enqueue this post in a queue
+  def enqueue(queue)
+    result = NewPostResult.new(:enqueued)
+    enqueuer = PostEnqueuer.new(@user, queue)
+    post = enqueuer.enqueue(@args)
+
+    result.check_errors_from(enqueuer)
+    result
+  end
+
+  def perform_create_post
+    result = NewPostResult.new(:create_post)
+
+    creator = PostCreator.new(@user, @args)
+    post = creator.create
+    result.check_errors_from(creator)
+
+    if result.success?
+      result.post = post
+    else
+      @user.flag_linked_posts_as_spam if creator.spam?
+    end
+
+    result
+  end
+
+end

lib/new_post_result.rb

@@ -0,0 +1,30 @@
+require_dependency 'has_errors'
+
+class NewPostResult
+  include HasErrors
+
+  attr_reader :action
+  attr_accessor :post
+
+  def initialize(action, success=false)
+    @action = action
+    @success = success
+  end
+
+  def check_errors_from(obj)
+    if obj.errors.empty?
+      @success = true
+    else
+      add_errors_from(obj)
+    end
+  end
+
+  def success?
+    @success
+  end
+
+  def failed?
+    !@success
+  end
+
+end

lib/post_creator.rb

@@ -134,6 +134,8 @@ class PostCreator
       track_latest_on_category
       enqueue_jobs
       BadgeGranter.queue_badge_grant(Badge::Trigger::PostRevision, post: @post)
+
+      trigger_after_events(@post)
     end
 
     if @post || @spam
@@ -169,6 +171,11 @@ class PostCreator
 
   protected
 
+  def trigger_after_events(post)
+    DiscourseEvent.trigger(:topic_created, post.topic, @opts, @user) unless @opts[:topic_id]
+    DiscourseEvent.trigger(:post_created, post, @opts, @user)
+  end
+
   def transaction(&blk)
     Post.transaction do
       if new_topic?

lib/post_enqueuer.rb

@@ -11,7 +11,6 @@ class PostEnqueuer
   end
 
   def enqueue(args)
-
     queued_post = QueuedPost.new(queue: @queue,
                                  state: QueuedPost.states[:new],
                                  user_id: @user.id,

spec/components/new_post_manager_spec.rb

@@ -0,0 +1,83 @@
+require 'spec_helper'
+require 'new_post_manager'
+
+describe NewPostManager do
+
+  let(:topic) { Fabricate(:topic) }
+
+  context "default action" do
+    it "creates the post by default" do
+      manager = NewPostManager.new(topic.user, raw: 'this is a new post', topic_id: topic.id)
+      result = manager.perform
+
+      expect(result.action).to eq(:create_post)
+      expect(result).to be_success
+      expect(result.post).to be_present
+      expect(result.post).to be_a(Post)
+    end
+  end
+
+  context "extensibility" do
+
+    before do
+      @counter = 0
+
+      @counter_handler = lambda do |manager|
+        result = nil
+        if manager.args[:raw] == 'this post increases counter'
+          @counter += 1
+          result = NewPostResult.new(:counter, true)
+        end
+
+        result
+      end
+
+      @queue_handler = -> (manager) { manager.args[:raw] =~ /queue me/ ? manager.enqueue('test') : nil }
+
+      NewPostManager.add_handler(&@counter_handler)
+      NewPostManager.add_handler(&@queue_handler)
+    end
+
+    after do
+      NewPostManager.handlers.delete(@counter_handler)
+      NewPostManager.handlers.delete(@queue_handler)
+    end
+
+    it "calls custom handlers" do
+      manager = NewPostManager.new(topic.user, raw: 'this post increases counter', topic_id: topic.id)
+
+      result = manager.perform
+
+      expect(result.action).to eq(:counter)
+      expect(result).to be_success
+      expect(result.post).to be_blank
+      expect(@counter).to be(1)
+      expect(QueuedPost.count).to be(0)
+    end
+
+    it "calls custom enqueuing handlers" do
+      manager = NewPostManager.new(topic.user, raw: 'to the handler I say enqueue me!', topic_id: topic.id)
+
+      result = manager.perform
+
+      expect(result.action).to eq(:enqueued)
+      expect(result).to be_success
+      expect(result.post).to be_blank
+      expect(QueuedPost.count).to be(1)
+      expect(@counter).to be(0)
+    end
+
+    it "if nothing returns a result it creates a post" do
+      manager = NewPostManager.new(topic.user, raw: 'this is a new post', topic_id: topic.id)
+
+      result = manager.perform
+
+      expect(result.action).to eq(:create_post)
+      expect(result).to be_success
+      expect(result.post).to be_present
+      expect(@counter).to be(0)
+    end
+
+  end
+
+end

spec/components/new_post_result_spec.rb

@@ -0,0 +1,12 @@
+require 'spec_helper'
+require 'new_post_result'
+
+describe NewPostResult do
+
+  it "fails by default" do
+    result = NewPostResult.new(:eviltrout)
+    expect(result.failed?).to eq(true)
+    expect(result.success?).to eq(false)
+  end
+
+end

spec/components/post_creator_spec.rb

@@ -579,4 +579,37 @@ describe PostCreator do
     expect(post.raw).to eq("    <-- whitespaces -->")
   end
 
+  context "events" do
+    let(:topic) { Fabricate(:topic, user: user) }
+
+    before do
+      @posts_created = 0
+      @topics_created = 0
+
+      @increase_posts = -> (post, opts, user) { @posts_created += 1 }
+      @increase_topics = -> (topic, opts, user) { @topics_created += 1 }
+      DiscourseEvent.on(:post_created, &@increase_posts)
+      DiscourseEvent.on(:topic_created, &@increase_topics)
+    end
+
+    after do
+      DiscourseEvent.off(:post_created, &@increase_posts)
+      DiscourseEvent.off(:topic_created, &@increase_topics)
+    end
+
+    it "fires boths event when creating a topic" do
+      pc = PostCreator.new(user, raw: 'this is the new content for my topic', title: 'this is my new topic title')
+      post = pc.create
+      expect(@posts_created).to eq(1)
+      expect(@topics_created).to eq(1)
+    end
+
+    it "fires only the post event when creating a post" do
+      pc = PostCreator.new(user, topic_id: topic.id, raw: 'this is the new content for my post')
+      post = pc.create
+      expect(@posts_created).to eq(1)
+      expect(@topics_created).to eq(0)
+    end
+  end
+
 end

spec/controllers/posts_controller_spec.rb

@@ -449,7 +449,7 @@ describe PostsController do
     include_examples 'action requires login', :post, :create
 
     context 'api' do
-      it 'allows dupes through' do
+      it 'memoizes duplicate requests' do
         raw = "this is a test post 123 #{SecureRandom.hash}"
         title = "this is a title #{SecureRandom.hash}"
 
@@ -478,16 +478,25 @@ describe PostsController do
       end
 
       it 'creates the post' do
-        PostCreator.any_instance.expects(:create).returns(new_post)
-
-        # Make sure our extensibility points are triggered
-        DiscourseEvent.expects(:trigger).with(:topic_created, new_post.topic, anything, user).once
-        DiscourseEvent.expects(:trigger).with(:post_created, new_post, anything, user).once
-
-        xhr :post, :create, {raw: 'test'}
+        xhr :post, :create, {raw: 'this is the test content', title: 'this is the test title for the topic'}
 
         expect(response).to be_success
-        expect(::JSON.parse(response.body)).to be_present
+        parsed = ::JSON.parse(response.body)
+
+        # Deprecated structure
+        expect(parsed['post']).to be_blank
+        expect(parsed['cooked']).to be_present
+      end
+
+      it "returns the nested post with a param" do
+        xhr :post, :create, {raw: 'this is the test content',
+                             title: 'this is the test title for the topic',
+                             nested_post: true}
+
+        expect(response).to be_success
+        parsed = ::JSON.parse(response.body)
+        expect(parsed['post']).to be_present
+        expect(parsed['post']['cooked']).to be_present
       end
 
       it 'protects against dupes' do
@@ -528,72 +537,73 @@ describe PostsController do
 
       context "parameters" do
 
-        let(:post_creator) { mock }
-
         before do
-          post_creator.expects(:create).returns(new_post)
-          post_creator.stubs(:errors).returns(nil)
+          # Just for performance, no reason to actually perform for these
+          # tests.
+          NewPostManager.stubs(:perform).returns(NewPostResult)
         end
 
         it "passes raw through" do
-          PostCreator.expects(:new).with(user, has_entries('raw' => 'hello')).returns(post_creator)
           xhr :post, :create, {raw: 'hello'}
+          expect(assigns(:manager_params)['raw']).to eq('hello')
         end
 
         it "passes title through" do
-          PostCreator.expects(:new).with(user, has_entries('title' => 'new topic title')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', title: 'new topic title'}
+          expect(assigns(:manager_params)['title']).to eq('new topic title')
         end
 
         it "passes topic_id through" do
-          PostCreator.expects(:new).with(user, has_entries('topic_id' => '1234')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', topic_id: 1234}
+          expect(assigns(:manager_params)['topic_id']).to eq('1234')
         end
 
         it "passes archetype through" do
-          PostCreator.expects(:new).with(user, has_entries('archetype' => 'private_message')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', archetype: 'private_message'}
+          expect(assigns(:manager_params)['archetype']).to eq('private_message')
         end
 
         it "passes category through" do
-          PostCreator.expects(:new).with(user, has_entries('category' => 'cool')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', category: 'cool'}
+          expect(assigns(:manager_params)['category']).to eq('cool')
         end
 
         it "passes target_usernames through" do
-          PostCreator.expects(:new).with(user, has_entries('target_usernames' => 'evil,trout')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', target_usernames: 'evil,trout'}
+          expect(assigns(:manager_params)['target_usernames']).to eq('evil,trout')
         end
 
         it "passes reply_to_post_number through" do
-          PostCreator.expects(:new).with(user, has_entries('reply_to_post_number' => '6789')).returns(post_creator)
           xhr :post, :create, {raw: 'hello', reply_to_post_number: 6789}
+          expect(assigns(:manager_params)['reply_to_post_number']).to eq('6789')
         end
 
         it "passes image_sizes through" do
-          PostCreator.expects(:new).with(user, has_entries('image_sizes' => {'width' => '100', 'height' => '200'})).returns(post_creator)
           xhr :post, :create, {raw: 'hello', image_sizes: {width: '100', height: '200'}}
+          expect(assigns(:manager_params)['image_sizes']['width']).to eq('100')
+          expect(assigns(:manager_params)['image_sizes']['height']).to eq('200')
         end
 
         it "passes meta_data through" do
-          PostCreator.expects(:new).with(user, has_entries('meta_data' => {'xyz' => 'abc'})).returns(post_creator)
           xhr :post, :create, {raw: 'hello', meta_data: {xyz: 'abc'}}
+          expect(assigns(:manager_params)['meta_data']['xyz']).to eq('abc')
         end
 
         context "is_warning" do
           it "doesn't pass `is_warning` through if you're not staff" do
-            PostCreator.expects(:new).with(user, Not(has_entries('is_warning' => true))).returns(post_creator)
             xhr :post, :create, {raw: 'hello', archetype: 'private_message', is_warning: 'true'}
+            expect(assigns(:manager_params)['is_warning']).to eq(false)
           end
 
           it "passes `is_warning` through if you're staff" do
-            PostCreator.expects(:new).with(moderator, has_entries('is_warning' => true)).returns(post_creator)
+            log_in(:moderator)
             xhr :post, :create, {raw: 'hello', archetype: 'private_message', is_warning: 'true'}
+            expect(assigns(:manager_params)['is_warning']).to eq(true)
           end
 
           it "passes `is_warning` as false through if you're staff" do
-            PostCreator.expects(:new).with(moderator, has_entries('is_warning' => false)).returns(post_creator)
             xhr :post, :create, {raw: 'hello', archetype: 'private_message', is_warning: 'false'}
+            expect(assigns(:manager_params)['is_warning']).to eq(false)
           end
 
         end