PostEnqueuer object to handle validation of enqueued posts

app/models/post.rb

@@ -369,7 +369,9 @@ class Post < ActiveRecord::Base
     problems
   end
 
-  def rebake!(opts={})
+  def rebake!(opts=nil)
+    opts ||= {}
+
     new_cooked = cook(
       raw,
       topic_id: topic_id,

app/models/queued_post.rb

@@ -2,8 +2,6 @@ class QueuedPost < ActiveRecord::Base
 
   class InvalidStateTransition < StandardError; end;
 
-  serialize :post_options, JSON
-
   belongs_to :user
   belongs_to :topic
   belongs_to :approved_by, class_name: "User"
@@ -36,6 +34,8 @@ class QueuedPost < ActiveRecord::Base
     opts = {raw: raw}
     post_attributes.each {|a| opts[a] = post_options[a.to_s] }
 
+    opts[:cooking_options].symbolize_keys! if opts[:cooking_options]
+
     opts[:topic_id] = topic_id if topic_id
     opts
   end
@@ -52,6 +52,7 @@ class QueuedPost < ActiveRecord::Base
   end
 
   private
+
     def post_attributes
       [QueuedPost.attributes_by_queue[:base], QueuedPost.attributes_by_queue[queue.to_sym]].flatten.compact
     end

db/migrate/20150318143915_create_directory_items.rb

@@ -1,6 +1,6 @@
 class CreateDirectoryItems < ActiveRecord::Migration
   def change
-    create_table :directory_items do |t|
+    create_table :directory_items, force: true do |t|
       t.integer :period_type, null: false
       t.references :user, null: false
       t.integer :likes_received, null: false

db/migrate/20150323234856_add_muted_users.rb

@@ -1,6 +1,6 @@
 class AddMutedUsers < ActiveRecord::Migration
   def change
-    create_table :muted_users do |t|
+    create_table :muted_users, force: true do |t|
       t.integer :user_id, null: false
       t.integer :muted_user_id, null: false
       t.timestamps

db/migrate/20150325190959_create_queued_posts.rb

@@ -1,11 +1,11 @@
 class CreateQueuedPosts < ActiveRecord::Migration
   def change
-    create_table :queued_posts do |t|
+    create_table :queued_posts, force: true do |t|
       t.string     :queue, null: false
       t.integer    :state, null: false
       t.integer    :user_id, null: false
       t.text       :raw, null: false
-      t.text       :post_options, null: false
+      t.json       :post_options, null: false
       t.integer    :topic_id
       t.integer    :approved_by_id
       t.timestamp  :approved_at
@@ -15,6 +15,6 @@ class CreateQueuedPosts < ActiveRecord::Migration
     end
 
     add_index :queued_posts, [:queue, :state, :created_at], name: 'by_queue_status'
-    add_index :queued_posts, [:topic, :queue, :state, :created_at], name: 'by_queue_status_topic'
+    add_index :queued_posts, [:topic_id, :queue, :state, :created_at], name: 'by_queue_status_topic'
   end
 end

lib/has_errors.rb

@@ -0,0 +1,31 @@
+# Helper functions for dealing with errors and objects that have
+# child objects with errors
+module HasErrors
+
+  def errors
+    @errors ||= ActiveModel::Errors.new(self)
+  end
+
+  def validate_child(obj)
+    return true if obj.valid?
+    add_errors_from(obj)
+    false
+  end
+
+  def rollback_with!(obj, error)
+    obj.errors[:base] << error
+    rollback_from_errors!(obj)
+  end
+
+  def rollback_from_errors!(obj)
+    add_errors_from(obj)
+    raise ActiveRecord::Rollback.new
+  end
+
+  def add_errors_from(obj)
+    obj.errors.full_messages.each do |msg|
+      errors[:base] << msg unless errors[:base].include?(msg)
+    end
+  end
+
+end

lib/post_creator.rb

@@ -4,10 +4,12 @@ require_dependency 'rate_limiter'
 require_dependency 'topic_creator'
 require_dependency 'post_jobs_enqueuer'
 require_dependency 'distributed_mutex'
+require_dependency 'has_errors'
 
 class PostCreator
+  include HasErrors
 
-  attr_reader :errors, :opts
+  attr_reader :opts
 
   # Acceptable options:
   #
@@ -66,21 +68,50 @@ class PostCreator
     @guardian ||= Guardian.new(@user)
   end
 
-  def create
+  def valid?
     @topic = nil
     @post = nil
 
     if @user.suspended? && !skip_validations?
-      @errors = Post.new.errors
-      @errors.add(:base, I18n.t(:user_is_suspended))
-      return
+      errors[:base] << I18n.t(:user_is_suspended)
+      return false
+    end
+
+    if new_topic?
+      topic_creator = TopicCreator.new(@user, guardian, @opts)
+      return false unless skip_validations? || validate_child(topic_creator)
+    else
+      @topic = Topic.find_by(id: @opts[:topic_id])
+      if (@topic.blank? || !guardian.can_create?(Post, @topic))
+        errors[:base] << I18n.t(:topic_not_found)
+        return false
+      end
     end
 
-    transaction do
-      setup_topic
     setup_post
-      rollback_if_host_spam_detected
-      plugin_callbacks
+
+    return true if skip_validations?
+    if @post.has_host_spam?
+      @spam = true
+      errors[:base] << I18n.t(:spamming_host)
+      return false
+    end
+
+    DiscourseEvent.trigger :before_create_post, @post
+    DiscourseEvent.trigger :validate_post, @post
+
+    post_validator = Validators::PostValidator.new(skip_topic: true)
+    post_validator.validate(@post)
+
+    valid = @post.errors.blank?
+    add_errors_from(@post) unless valid
+    valid
+  end
+
+  def create
+    if valid?
+      transaction do
+        create_topic
         save_post
         extract_links
         store_unique_post_key
@@ -94,8 +125,9 @@ class PostCreator
         @post.advance_draft_sequence
         @post.save_reply_relationships
       end
+    end
 
-    if @post && @post.errors.empty?
+    if @post && errors.blank?
       publish
       PostAlerter.post_created(@post) unless @opts[:import_mode]
 
@@ -164,16 +196,11 @@ class PostCreator
                            { user: @user,
                              limit_once_per: 24.hours,
                              message_params: {domains: @post.linked_hosts.keys.join(', ')} } )
-    elsif @post && !@post.errors.present? && !skip_validations?
+    elsif @post && errors.blank? && !skip_validations?
       SpamRulesEnforcer.enforce!(@post)
     end
   end
 
-  def plugin_callbacks
-    DiscourseEvent.trigger :before_create_post, @post
-    DiscourseEvent.trigger :validate_post, @post
-  end
-
   def track_latest_on_category
     return unless @post && @post.errors.count == 0 && @topic && @topic.category_id
 
@@ -191,27 +218,17 @@ class PostCreator
 
   private
 
-  def setup_topic
-    if new_topic?
-      topic_creator = TopicCreator.new(@user, guardian, @opts)
-
+  def create_topic
+    return if @topic
     begin
-        topic = topic_creator.create
-        @errors = topic_creator.errors
-      rescue ActiveRecord::Rollback => ex
-        # In the event of a rollback, grab the errors from the topic
-        @errors = topic_creator.errors
-        raise ex
+      topic_creator = TopicCreator.new(@user, guardian, @opts)
+      @topic = topic_creator.create
+    rescue ActiveRecord::Rollback
+      add_errors_from(topic_creator)
+      return
     end
-    else
-      topic = Topic.find_by(id: @opts[:topic_id])
-      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
+    @post.topic_id = @topic.id
+    @post.topic = @topic
   end
 
   def update_topic_stats
@@ -232,7 +249,8 @@ class PostCreator
   def setup_post
     @opts[:raw] = TextCleaner.normalize_whitespaces(@opts[:raw]).gsub(/\s+\z/, "")
 
-    post = @topic.posts.new(raw: @opts[:raw],
+    post = Post.new(raw: @opts[:raw],
+                    topic_id: @topic.try(:id),
                     user: @user,
                     reply_to_post_number: @opts[:reply_to_post_number])
 
@@ -251,21 +269,9 @@ class PostCreator
     @post = post
   end
 
-  def rollback_if_host_spam_detected
-    return if skip_validations?
-    if @post.has_host_spam?
-      @post.errors.add(:base, I18n.t(:spamming_host))
-      @errors = @post.errors
-      @spam = true
-      raise ActiveRecord::Rollback.new
-    end
-  end
-
   def save_post
-    unless @post.save(validate: !skip_validations?)
-      @errors = @post.errors
-      raise ActiveRecord::Rollback.new
-    end
+    saved = @post.save(validate: !skip_validations?)
+    rollback_from_errors!(@post) unless saved
   end
 
   def store_unique_post_key

lib/post_enqueuer.rb

@@ -0,0 +1,41 @@
+require_dependency 'topic_creator'
+require_dependency 'queued_post'
+require_dependency 'has_errors'
+
+class PostEnqueuer
+  include HasErrors
+
+  def initialize(user, queue)
+    @user = user
+    @queue = queue
+  end
+
+  def enqueue(args)
+
+    queued_post = QueuedPost.new(queue: @queue,
+                                 state: QueuedPost.states[:new],
+                                 user_id: @user.id,
+                                 topic_id: args[:topic_id],
+                                 raw: args[:raw],
+                                 post_options: args[:post_options] || {})
+
+    validate_method = :"validate_#{@queue}"
+    if respond_to?(validate_method)
+      return unless send(validate_method, queued_post.create_options)
+    end
+
+    add_errors_from(queued_post) unless queued_post.save
+    queued_post
+  end
+
+  def validate_new_topic(create_options)
+    topic_creator = TopicCreator.new(@user, Guardian.new(@user), create_options)
+    validate_child(topic_creator)
+  end
+
+  def validate_new_post(create_options)
+    post_creator = PostCreator.new(@user, create_options)
+    validate_child(post_creator)
+  end
+
+end

lib/topic_creator.rb

@@ -1,6 +1,7 @@
-class TopicCreator
+require_dependency 'has_errors'
 
-  attr_accessor :errors
+class TopicCreator
+  include HasErrors
 
   def self.create(user, guardian, opts)
     self.new(user, guardian, opts).create
@@ -13,55 +14,52 @@ class TopicCreator
     @added_users = []
   end
 
+  def valid?
+    topic = Topic.new(setup_topic_params)
+    validate_child(topic)
+  end
+
   def create
-    @topic = Topic.new(setup_topic_params)
+    topic = Topic.new(setup_topic_params)
 
-    setup_auto_close_time
-    process_private_message
-    save_topic
-    create_warning
-    watch_topic
+    setup_auto_close_time(topic)
+    process_private_message(topic)
+    save_topic(topic)
+    create_warning(topic)
+    watch_topic(topic)
 
-    @topic
+    topic
   end
 
   private
 
-  def create_warning
+  def create_warning(topic)
     return unless @opts[:is_warning]
 
     # We can only attach warnings to PMs
-    unless @topic.private_message?
-      @topic.errors.add(:base, :warning_requires_pm)
-      @errors = @topic.errors
-      raise ActiveRecord::Rollback.new
-    end
+    rollback_with!(topic, :warning_requires_pm) unless topic.private_message?
 
     # Don't create it if there is more than one user
-    if @added_users.size != 1
-      @topic.errors.add(:base, :too_many_users)
-      @errors = @topic.errors
-      raise ActiveRecord::Rollback.new
-    end
+    rollback_with!(topic, :too_many_users) if @added_users.size != 1
 
     # Create a warning record
-    Warning.create(topic: @topic, user: @added_users.first, created_by: @user)
+    Warning.create(topic: topic, user: @added_users.first, created_by: @user)
   end
 
-  def watch_topic
+  def watch_topic(topic)
     unless @opts[:auto_track] == false
-      @topic.notifier.watch_topic!(@topic.user_id)
+      topic.notifier.watch_topic!(topic.user_id)
     end
 
-    user_ids = @topic.topic_allowed_users(true).pluck(:user_id)
-    user_ids += @topic.topic_allowed_groups(true).map { |t| t.group.users.pluck(:id) }.flatten
+    user_ids = topic.topic_allowed_users(true).pluck(:user_id)
+    user_ids += topic.topic_allowed_groups(true).map { |t| t.group.users.pluck(:id) }.flatten
 
-    user_ids.uniq.reject{ |id| id == @topic.user_id }.each do |user_id|
-      @topic.notifier.watch_topic!(user_id, nil) unless user_id == -1
+    user_ids.uniq.reject{ |id| id == topic.user_id }.each do |user_id|
+      topic.notifier.watch_topic!(user_id, nil) unless user_id == -1
     end
 
-    CategoryUser.auto_watch_new_topic(@topic)
-    CategoryUser.auto_track_new_topic(@topic)
+    CategoryUser.auto_watch_new_topic(topic)
+    CategoryUser.auto_track_new_topic(topic)
   end
 
   def setup_topic_params
@@ -106,31 +104,28 @@ class TopicCreator
     end
   end
 
-  def setup_auto_close_time
+  def setup_auto_close_time(topic)
     return unless @opts[:auto_close_time].present?
-    return unless @guardian.can_moderate?(@topic)
-    @topic.set_auto_close(@opts[:auto_close_time], @user)
+    return unless @guardian.can_moderate?(topic)
+    topic.set_auto_close(@opts[:auto_close_time], @user)
   end
 
-  def process_private_message
+  def process_private_message(topic)
     return unless @opts[:archetype] == Archetype.private_message
-    @topic.subtype = TopicSubtype.user_to_user unless @topic.subtype
+    topic.subtype = TopicSubtype.user_to_user unless topic.subtype
 
     unless @opts[:target_usernames].present? || @opts[:target_group_names].present?
-      @topic.errors.add(:base, :no_user_selected)
-      @errors = @topic.errors
-      raise ActiveRecord::Rollback.new
+      rollback_with!(topic, :no_user_selected)
     end
 
-    add_users(@topic,@opts[:target_usernames])
-    add_groups(@topic,@opts[:target_group_names])
-    @topic.topic_allowed_users.build(user_id: @user.id)
+    add_users(topic,@opts[:target_usernames])
+    add_groups(topic,@opts[:target_group_names])
+    topic.topic_allowed_users.build(user_id: @user.id)
   end
 
-  def save_topic
-    unless @topic.save(validate: !@opts[:skip_validations])
-      @errors = @topic.errors
-      raise ActiveRecord::Rollback.new
+  def save_topic(topic)
+    unless topic.save(validate: !@opts[:skip_validations])
+      rollback_from_errors!(topic)
     end
   end
 
@@ -146,16 +141,12 @@ class TopicCreator
   def add_groups(topic, groups)
     return unless groups
     Group.where(name: groups.split(',')).each do |group|
-      check_can_send_permission!(topic,group)
+      check_can_send_permission!(topic, group)
       topic.topic_allowed_groups.build(group_id: group.id)
     end
   end
 
-  def check_can_send_permission!(topic,item)
-    unless @guardian.can_send_private_message?(item)
-      topic.errors.add(:base, :cant_send_pm)
-      @errors = topic.errors
-      raise ActiveRecord::Rollback.new
-    end
+  def check_can_send_permission!(topic, obj)
+    rollback_with!(topic, :cant_send_pm) unless @guardian.can_send_private_message?(obj)
   end
 end

lib/validators/post_validator.rb

@@ -1,6 +1,7 @@
 require_dependency 'validators/stripped_length_validator'
 module Validators; end
 class Validators::PostValidator < ActiveModel::Validator
+
   def validate(record)
     presence(record)
     unless Discourse.static_doc_topic_ids.include?(record.topic_id) && record.acting_user.try(:admin?)
@@ -16,9 +17,12 @@ class Validators::PostValidator < ActiveModel::Validator
   end
 
   def presence(post)
-    [:raw,:topic_id].each do |attr_name|
-       post.errors.add(attr_name, :blank, options) if post.send(attr_name).blank?
+
+    post.errors.add(:raw, :blank, options) if post.raw.blank?
+    unless options[:skip_topic]
+      post.errors.add(:topic_id, :blank, options) if post.topic_id.blank?
     end
+
     if post.new_record? and post.user_id.nil?
       post.errors.add(:user_id, :blank, options)
     end

spec/components/has_errors_spec.rb

@@ -0,0 +1,55 @@
+require 'spec_helper'
+require 'has_errors'
+
+describe HasErrors do
+
+  class ErrorTestClass
+    include HasErrors
+  end
+
+  let(:error_test) { ErrorTestClass.new }
+  let(:title_error) { "Title can't be blank" }
+
+  # No title is invalid
+  let(:invalid_topic) { Fabricate.build(:topic, title: '') }
+
+  it "has no errors by default" do
+    expect(error_test.errors).to be_blank
+  end
+
+  context "validate_child" do
+    it "adds the errors from invalid AR objects" do
+      expect(error_test.validate_child(invalid_topic)).to eq(false)
+      expect(error_test.errors).to be_present
+      expect(error_test.errors[:base]).to include(title_error)
+    end
+
+    it "doesn't add the errors from valid AR objects" do
+      topic = Fabricate.build(:topic)
+      expect(error_test.validate_child(topic)).to eq(true)
+      expect(error_test.errors).to be_blank
+    end
+  end
+
+  context "rollback_from_errors!" do
+    it "triggers a rollback" do
+      invalid_topic.valid?
+
+      expect(-> { error_test.rollback_from_errors!(invalid_topic) }).to raise_error(ActiveRecord::Rollback)
+      expect(error_test.errors).to be_present
+      expect(error_test.errors[:base]).to include(title_error)
+    end
+  end
+
+  context "rollback_with_error!" do
+    it "triggers a rollback" do
+
+      expect(-> {
+        error_test.rollback_with!(invalid_topic, :custom_error)
+      }).to raise_error(ActiveRecord::Rollback)
+      expect(error_test.errors).to be_present
+      expect(error_test.errors[:base]).to include(:custom_error)
+    end
+  end
+
+end

spec/components/post_creator_spec.rb

@@ -311,7 +311,8 @@ describe PostCreator do
 
     it "does not create the post" do
       GroupMessage.stubs(:create)
-      creator.create
+      post = creator.create
+
       expect(creator.errors).to be_present
       expect(creator.spam?).to eq(true)
     end
@@ -521,7 +522,7 @@ describe PostCreator do
     it 'can save a post' do
       creator = PostCreator.new(user, raw: 'q', title: 'q', skip_validations: true)
       creator.create
-      expect(creator.errors).to eq(nil)
+      expect(creator.errors).to be_blank
     end
   end
 
@@ -573,7 +574,8 @@ describe PostCreator do
   end
 
   it "doesn't strip starting whitespaces" do
-    post = PostCreator.new(user, { title: "testing whitespace stripping", raw: "    <-- whitespaces -->    " }).create
+    pc = PostCreator.new(user, { title: "testing whitespace stripping", raw: "    <-- whitespaces -->    " })
+    post = pc.create
     expect(post.raw).to eq("    <-- whitespaces -->")
   end
 

spec/components/post_enqueuer_spec.rb

@@ -0,0 +1,47 @@
+require 'spec_helper'
+require_dependency 'post_enqueuer'
+
+describe PostEnqueuer do
+
+  let(:user) { Fabricate(:user) }
+
+  context 'with valid arguments' do
+    let(:topic) { Fabricate(:topic) }
+    let(:enqueuer) { PostEnqueuer.new(user, 'new_post') }
+
+    it 'enqueues the post' do
+      qp = enqueuer.enqueue(raw: 'This should be enqueued',
+                            topic_id: topic.id,
+                            post_options: { reply_to_post_number: 1 })
+
+      expect(enqueuer.errors).to be_blank
+      expect(qp).to be_present
+      expect(qp.topic).to eq(topic)
+      expect(qp.user).to eq(user)
+    end
+  end
+
+  context "topic validations" do
+    let(:enqueuer) { PostEnqueuer.new(user, 'new_topic') }
+
+    it "doesn't enqueue the post" do
+      qp = enqueuer.enqueue(raw: 'This should not be enqueued',
+                            post_options: { title: 'too short' })
+
+      expect(enqueuer.errors).to be_present
+      expect(qp).to be_blank
+    end
+  end
+
+  context "post validations" do
+    let(:enqueuer) { PostEnqueuer.new(user, 'new_post') }
+
+    it "doesn't enqueue the post" do
+      qp = enqueuer.enqueue(raw: 'too short')
+
+      expect(enqueuer.errors).to be_present
+      expect(qp).to be_blank
+    end
+  end
+
+end

spec/models/queued_post_spec.rb

@@ -19,7 +19,7 @@ describe QueuedPost do
                                    auto_track: true,
                                    custom_fields: { hello: 'world' },
                                    cooking_options: { cat: 'hat' },
-                                   cook_method: 'regular',
+                                   cook_method: Post.cook_methods[:raw_html],
                                    not_create_option: true,
                                    image_sizes: {"http://foo.bar/image.png" => {"width" => 0, "height" => 222}}
                                  }) }
@@ -34,8 +34,8 @@ describe QueuedPost do
       expect(create_options[:raw_email]).to eq('store_me')
       expect(create_options[:auto_track]).to eq(true)
       expect(create_options[:custom_fields]).to eq('hello' => 'world')
-      expect(create_options[:cooking_options]).to eq('cat' => 'hat')
-      expect(create_options[:cook_method]).to eq('regular')
+      expect(create_options[:cooking_options]).to eq(cat: 'hat')
+      expect(create_options[:cook_method]).to eq(Post.cook_methods[:raw_html])
       expect(create_options[:not_create_option]).to eq(nil)
       expect(create_options[:image_sizes]).to eq("http://foo.bar/image.png" => {"width" => 0, "height" => 222})
     end
@@ -47,6 +47,7 @@ describe QueuedPost do
       expect(post).to be_present
       expect(post).to be_valid
       expect(post.topic).to eq(topic)
+      expect(post.custom_fields['hello']).to eq('world')
 
       # Updates the QP record
       expect(qp.approved_by).to eq(admin)
@@ -74,8 +75,9 @@ describe QueuedPost do
   context "creating a topic" do
     let(:user) { Fabricate(:user) }
     let(:admin) { Fabricate(:admin) }
-    let!(:category) { Fabricate(:category) }
 
+    context "with a valid topic" do
+      let!(:category) { Fabricate(:category) }
       let(:qp) { QueuedPost.create(queue: 'new_topic',
                                    state: QueuedPost.states[:new],
                                    user_id: user.id,
@@ -120,5 +122,6 @@ describe QueuedPost do
         expect(Post.count).to eq(post_count)
       end
     end
+  end
 
 end