diff --git a/app/models/post.rb b/app/models/post.rb index 9e07e94f9..7e0e5e455 100644 --- a/app/models/post.rb +++ b/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, diff --git a/app/models/queued_post.rb b/app/models/queued_post.rb index a7b4dcec4..2b1cdacaf 100644 --- a/app/models/queued_post.rb +++ b/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 diff --git a/db/migrate/20150318143915_create_directory_items.rb b/db/migrate/20150318143915_create_directory_items.rb index 37f08336a..088bfba0d 100644 --- a/db/migrate/20150318143915_create_directory_items.rb +++ b/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 diff --git a/db/migrate/20150323234856_add_muted_users.rb b/db/migrate/20150323234856_add_muted_users.rb index 2e7c5048f..8192d3828 100644 --- a/db/migrate/20150323234856_add_muted_users.rb +++ b/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 diff --git a/db/migrate/20150325160959_create_queued_posts.rb b/db/migrate/20150325190959_create_queued_posts.rb similarity index 71% rename from db/migrate/20150325160959_create_queued_posts.rb rename to db/migrate/20150325190959_create_queued_posts.rb index 4941753ee..9946a642b 100644 --- a/db/migrate/20150325160959_create_queued_posts.rb +++ b/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 diff --git a/lib/has_errors.rb b/lib/has_errors.rb new file mode 100644 index 000000000..9aae07e05 --- /dev/null +++ b/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 diff --git a/lib/post_creator.rb b/lib/post_creator.rb index d4627ca2e..9660a96b9 100644 --- a/lib/post_creator.rb +++ b/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,36 +68,66 @@ 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 - transaction do - setup_topic - setup_post - rollback_if_host_spam_detected - plugin_callbacks - save_post - extract_links - store_unique_post_key - track_topic - update_topic_stats - update_topic_auto_close - update_user_counts - create_embedded_topic - - ensure_in_allowed_users if guardian.is_staff? - @post.advance_draft_sequence - @post.save_reply_relationships + 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 - if @post && @post.errors.empty? + setup_post + + 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 + track_topic + update_topic_stats + update_topic_auto_close + update_user_counts + create_embedded_topic + + ensure_in_allowed_users if guardian.is_staff? + @post.advance_draft_sequence + @post.save_reply_relationships + end + end + + 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? + def create_topic + return if @topic + begin topic_creator = TopicCreator.new(@user, guardian, @opts) - - 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 - 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 + @topic = topic_creator.create + rescue ActiveRecord::Rollback + add_errors_from(topic_creator) + return end - @topic = topic + @post.topic_id = @topic.id + @post.topic = @topic end def update_topic_stats @@ -232,9 +249,10 @@ class PostCreator def setup_post @opts[:raw] = TextCleaner.normalize_whitespaces(@opts[:raw]).gsub(/\s+\z/, "") - post = @topic.posts.new(raw: @opts[:raw], - user: @user, - reply_to_post_number: @opts[:reply_to_post_number]) + post = Post.new(raw: @opts[:raw], + topic_id: @topic.try(:id), + user: @user, + reply_to_post_number: @opts[:reply_to_post_number]) # Attributes we pass through to the post instance if present [:post_type, :no_bump, :cooking_options, :image_sizes, :acting_user, :invalidate_oneboxes, :cook_method, :via_email, :raw_email].each do |a| @@ -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 diff --git a/lib/post_enqueuer.rb b/lib/post_enqueuer.rb new file mode 100644 index 000000000..e6ba63f55 --- /dev/null +++ b/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 diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 40b6aee9d..790da802e 100644 --- a/lib/topic_creator.rb +++ b/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 diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb index a5751ccad..fc1657147 100644 --- a/lib/validators/post_validator.rb +++ b/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 diff --git a/spec/components/has_errors_spec.rb b/spec/components/has_errors_spec.rb new file mode 100644 index 000000000..9ea4d1a06 --- /dev/null +++ b/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 diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 77d658f08..7db6eb829 100644 --- a/spec/components/post_creator_spec.rb +++ b/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 diff --git a/spec/components/post_enqueuer_spec.rb b/spec/components/post_enqueuer_spec.rb new file mode 100644 index 000000000..0c9d5a1de --- /dev/null +++ b/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 diff --git a/spec/models/queued_post_spec.rb b/spec/models/queued_post_spec.rb index 7fa07b199..521819fef 100644 --- a/spec/models/queued_post_spec.rb +++ b/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,50 +75,52 @@ describe QueuedPost do context "creating a topic" do let(:user) { Fabricate(:user) } let(:admin) { Fabricate(:admin) } - let!(:category) { Fabricate(:category) } - let(:qp) { QueuedPost.create(queue: 'new_topic', - state: QueuedPost.states[:new], - user_id: user.id, - raw: 'This post should be queued up', - post_options: { - title: 'This is the topic title to queue up', - archetype: 'regular', - category: category.id, - meta_data: {evil: 'trout'} - }) } + 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, + raw: 'This post should be queued up', + post_options: { + title: 'This is the topic title to queue up', + archetype: 'regular', + category: category.id, + meta_data: {evil: 'trout'} + }) } - it "returns the appropriate options for creating a topic" do - create_options = qp.create_options + it "returns the appropriate options for creating a topic" do + create_options = qp.create_options - expect(create_options[:category]).to eq(category.id) - expect(create_options[:archetype]).to eq('regular') - expect(create_options[:meta_data]).to eq('evil' => 'trout') - end + expect(create_options[:category]).to eq(category.id) + expect(create_options[:archetype]).to eq('regular') + expect(create_options[:meta_data]).to eq('evil' => 'trout') + end - it "creates the post and topic" do - topic_count, post_count = Topic.count, Post.count - post = qp.approve!(admin) + it "creates the post and topic" do + topic_count, post_count = Topic.count, Post.count + post = qp.approve!(admin) - expect(Topic.count).to eq(topic_count + 1) - expect(Post.count).to eq(post_count + 1) + expect(Topic.count).to eq(topic_count + 1) + expect(Post.count).to eq(post_count + 1) - expect(post).to be_present - expect(post).to be_valid + expect(post).to be_present + expect(post).to be_valid - topic = post.topic - expect(topic).to be_present - expect(topic.category).to eq(category) - end + topic = post.topic + expect(topic).to be_present + expect(topic.category).to eq(category) + end - it "doesn't create the post and topic" do - topic_count, post_count = Topic.count, Post.count + it "doesn't create the post and topic" do + topic_count, post_count = Topic.count, Post.count - qp.reject!(admin) + qp.reject!(admin) - expect(Topic.count).to eq(topic_count) - expect(Post.count).to eq(post_count) + expect(Topic.count).to eq(topic_count) + expect(Post.count).to eq(post_count) + end end end