From 19a9a8b4088b56ff6996c0508ee7571d25c06c4e Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 31 Mar 2015 12:58:56 -0400 Subject: [PATCH] `NewPostManager` determines whether to queue a post or not --- .travis.yml | 3 + app/controllers/application_controller.rb | 13 +-- app/controllers/posts_controller.rb | 83 ++++++++----------- app/serializers/new_post_result_serializer.rb | 39 +++++++++ lib/discourse.rb | 2 - lib/discourse_event.rb | 4 + lib/guardian/post_guardian.rb | 1 + lib/new_post_manager.rb | 66 +++++++++++++++ lib/new_post_result.rb | 30 +++++++ lib/post_creator.rb | 7 ++ lib/post_enqueuer.rb | 1 - spec/components/new_post_manager_spec.rb | 83 +++++++++++++++++++ spec/components/new_post_result_spec.rb | 12 +++ spec/components/post_creator_spec.rb | 33 ++++++++ spec/controllers/posts_controller_spec.rb | 60 ++++++++------ 15 files changed, 354 insertions(+), 83 deletions(-) create mode 100644 app/serializers/new_post_result_serializer.rb create mode 100644 lib/new_post_manager.rb create mode 100644 lib/new_post_result.rb create mode 100644 spec/components/new_post_manager_spec.rb create mode 100644 spec/components/new_post_result_spec.rb diff --git a/.travis.yml b/.travis.yml index cf7aa1b1be3..64d25f1d16f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,6 +8,9 @@ env: - "RAILS_MASTER=1" - "RAILS_MASTER=0" +addons: + postgresql: 9.3 + matrix: allow_failures: - rvm: 2.0.0 diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index afaf3ef11e2..3b76392c287 100644 --- a/app/controllers/application_controller.rb +++ b/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? diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 0ebb3a766c8..e632b8f277f 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/app/serializers/new_post_result_serializer.rb b/app/serializers/new_post_result_serializer.rb new file mode 100644 index 00000000000..ff7330cdd47 --- /dev/null +++ b/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 diff --git a/lib/discourse.rb b/lib/discourse.rb index 9c37ed3505a..1bafaea8476 100644 --- a/lib/discourse.rb +++ b/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 diff --git a/lib/discourse_event.rb b/lib/discourse_event.rb index c0ec064d3d0..2c1110a632a 100644 --- a/lib/discourse_event.rb +++ b/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 diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 2e8031be78d..4c0292048b1 100644 --- a/lib/guardian/post_guardian.rb +++ b/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={}) diff --git a/lib/new_post_manager.rb b/lib/new_post_manager.rb new file mode 100644 index 00000000000..323acaa7ed8 --- /dev/null +++ b/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 diff --git a/lib/new_post_result.rb b/lib/new_post_result.rb new file mode 100644 index 00000000000..1e8bdffb04d --- /dev/null +++ b/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 diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 9660a96b9d3..71837937944 100644 --- a/lib/post_creator.rb +++ b/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? diff --git a/lib/post_enqueuer.rb b/lib/post_enqueuer.rb index e6ba63f554d..78242d405d1 100644 --- a/lib/post_enqueuer.rb +++ b/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, diff --git a/spec/components/new_post_manager_spec.rb b/spec/components/new_post_manager_spec.rb new file mode 100644 index 00000000000..523d8652a03 --- /dev/null +++ b/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 diff --git a/spec/components/new_post_result_spec.rb b/spec/components/new_post_result_spec.rb new file mode 100644 index 00000000000..3c555e79315 --- /dev/null +++ b/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 diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 7db6eb829e8..950af818854 100644 --- a/spec/components/post_creator_spec.rb +++ b/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 diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb index 3faf22d2aeb..41c114ab551 100644 --- a/spec/controllers/posts_controller_spec.rb +++ b/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