diff --git a/app/models/topic.rb b/app/models/topic.rb index d5bc6f426aa..fdf606336cb 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -36,15 +36,24 @@ class Topic < ActiveRecord::Base rate_limit :limit_topics_per_day rate_limit :limit_private_messages_per_day - validate :title_quality - validates_presence_of :title - validate :title, -> { SiteSetting.topic_title_length.include? :length } + before_validation :sanitize_title + + validates :title, :presence => true, + :length => { :in => SiteSetting.topic_title_length, + :allow_blank => true }, + :quality_title => { :unless => :private_message? }, + :unique_among => { :unless => Proc.new { |t| (SiteSetting.allow_duplicate_topic_titles? || t.private_message?) }, + :message => :has_already_been_used, + :allow_blank => true, + :case_sensitive => false, + :collection => Proc.new{ Topic.listable_topics } } + + after_validation do + self.title = TextCleaner.clean_title(TextSentinel.title_sentinel(title).text) if errors[:title].empty? + end serialize :meta_data, ActiveRecord::Coders::Hstore - before_validation :sanitize_title - validate :unique_title - belongs_to :category has_many :posts has_many :topic_allowed_users @@ -142,22 +151,6 @@ class Topic < ActiveRecord::Base RateLimiter.new(user, "pms-per-day:#{Date.today.to_s}", SiteSetting.max_private_messages_per_day, 1.day.to_i) end - # Validate unique titles if a site setting is set - def unique_title - return if SiteSetting.allow_duplicate_topic_titles? - - # Let presence validation catch it if it's blank - return if title.blank? - - # Private messages can be called whatever they want - return if private_message? - - finder = Topic.listable_topics.where("lower(title) = ?", title.downcase) - finder = finder.where("id != ?", self.id) if self.id.present? - - errors.add(:title, I18n.t(:has_already_been_used)) if finder.exists? - end - def fancy_title return title unless SiteSetting.title_fancy_entities? @@ -168,19 +161,6 @@ class Topic < ActiveRecord::Base Redcarpet::Render::SmartyPants.render(title) end - def title_quality - # We don't care about quality on private messages - return if private_message? - - sentinel = TextSentinel.title_sentinel(title) - if sentinel.valid? - # clean up the title - self.title = TextCleaner.clean_title(sentinel.text) - else - errors.add(:title, I18n.t(:is_invalid)) - end - end - def sanitize_title if self.title.present? self.title = sanitize(title, tags: [], attributes: []) diff --git a/config/application.rb b/config/application.rb index bcc6f311ad4..a647f4f9147 100644 --- a/config/application.rb +++ b/config/application.rb @@ -31,7 +31,8 @@ module Discourse end # Custom directories with classes and modules you want to be autoloadable. - config.autoload_paths += %W(#{config.root}/app/serializers) + config.autoload_paths += Dir["#{config.root}/app/serializers"] + config.autoload_paths += Dir["#{config.root}/lib/validators/"] # Only load the plugins named here, in the order given (default is alphabetical). # :all can be used as a placeholder for all plugins not explicitly named. diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index dc053849c69..7c34f9e4e79 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -100,6 +100,9 @@ en: post: raw: "Body" errors: + messages: + is_invalid: "is invalid; try to be a little more descriptive" + has_already_been_used: "has already been used" models: topic: attributes: diff --git a/lib/text_sentinel.rb b/lib/text_sentinel.rb index 4a04edb9a49..439781b71d5 100644 --- a/lib/text_sentinel.rb +++ b/lib/text_sentinel.rb @@ -7,7 +7,7 @@ class TextSentinel def initialize(text, opts=nil) @opts = opts || {} - @text = text.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') if text.present? + @text = text.to_s.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') end def self.non_symbols_regexp @@ -26,28 +26,26 @@ class TextSentinel # Entropy is a number of how many unique characters the string needs. def entropy - return 0 if @text.blank? - @entropy ||= @text.strip.each_char.to_a.uniq.size + @entropy ||= @text.to_s.strip.split('').uniq.size end def valid? # Blank strings are not valid - return false if @text.blank? || @text.strip.blank? + @text.present? && - # Entropy check if required - return false if @opts[:min_entropy].present? && (entropy < @opts[:min_entropy]) + # Minimum entropy if entropy check required + (@opts[:min_entropy].blank? || (entropy >= @opts[:min_entropy])) && - # We don't have a comprehensive list of symbols, but this will eliminate some noise - non_symbols = @text.gsub(TextSentinel.non_symbols_regexp, '').size - return false if non_symbols == 0 + # At least some non-symbol characters + # (We don't have a comprehensive list of symbols, but this will eliminate some noise) + (@text.gsub(TextSentinel.non_symbols_regexp, '').size > 0) && - # Don't allow super long strings without spaces - return false if @opts[:max_word_length] && @text =~ /\w{#{@opts[:max_word_length]},}(\s|$)/ + # Don't allow super long words if there is a word length maximum + (@opts[:max_word_length].blank? || @text.split(/\W/).map(&:size).max <= @opts[:max_word_length] ) && # We don't allow all upper case content in english - return false if (@text =~ /[A-Z]+/) && (@text == @text.upcase) + not((@text =~ /[A-Z]+/) && (@text == @text.upcase)) && - # It is valid true end diff --git a/lib/validators/quality_title_validator.rb b/lib/validators/quality_title_validator.rb new file mode 100644 index 00000000000..598e78782f8 --- /dev/null +++ b/lib/validators/quality_title_validator.rb @@ -0,0 +1,9 @@ +require 'text_sentinel' +require 'text_cleaner' + +class QualityTitleValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + sentinel = TextSentinel.title_sentinel(value) + record.errors.add(attribute, :is_invalid) unless sentinel.valid? + end +end diff --git a/lib/validators/unique_among_validator.rb b/lib/validators/unique_among_validator.rb new file mode 100644 index 00000000000..3aada1bd0c9 --- /dev/null +++ b/lib/validators/unique_among_validator.rb @@ -0,0 +1,21 @@ +class UniqueAmongValidator < ActiveRecord::Validations::UniquenessValidator + def validate_each(record, attribute, value) + old_errors = record.errors[attribute].size + + # look for any duplicates at all + super + + new_errors = record.errors[attribute].size - old_errors + + # do nothing further unless there were some duplicates. + unless new_errors == 0 + # now look only in the collection we care about. + dupes = options[:collection].call.where("lower(#{attribute}) = ?", value.downcase) + dupes = dupes.where("id != ?", record.id) if record.persisted? + + # pop off the error, if it was a false positive + record.errors[attribute].pop(new_errors) unless dupes.exists? + end + end + +end diff --git a/spec/components/validators/quality_title_validator_spec.rb b/spec/components/validators/quality_title_validator_spec.rb new file mode 100644 index 00000000000..0eb22006caa --- /dev/null +++ b/spec/components/validators/quality_title_validator_spec.rb @@ -0,0 +1,73 @@ +# encoding: UTF-8 + +require 'spec_helper' +require 'validators/quality_title_validator' +require 'ostruct' + +module QualityTitleValidatorSpec + class Validatable < OpenStruct + include ActiveModel::Validations + validates :title, :quality_title => { :unless => :private_message? } + end +end + +describe "A record validated with QualityTitleValidator" do + let(:valid_title){ "hello this is my cool topic! welcome: all;" } + let(:short_title){ valid_title.slice(0, SiteSetting.min_topic_title_length - 1) } + let(:long_title ){ valid_title.center(SiteSetting.max_topic_title_length + 1, 'x') } + let(:xxxxx_title){ valid_title.gsub(/./,'x')} + + subject(:topic){ QualityTitleValidatorSpec::Validatable.new } + + before(:each) do + topic.stubs(:private_message? => false) + end + + it "allows a regular title with a few ascii characters" do + topic.title = valid_title + topic.should be_valid + end + + it "allows non ascii" do + topic.title = "Iñtërnâtiônàlizætiøn" + topic.should be_valid + end + + it "allows anything in a private message" do + topic.stubs(:private_message? => true) + [short_title, long_title, xxxxx_title].each do |bad_title| + topic.title = bad_title + topic.should be_valid + end + end + + it "strips a title when identifying length" do + topic.title = short_title.center(SiteSetting.min_topic_title_length + 1, ' ') + topic.should_not be_valid + end + + it "doesn't allow a long title" do + topic.title = long_title + topic.should_not be_valid + end + + it "doesn't allow a short title" do + topic.title = short_title + topic.should_not be_valid + end + + it "doesn't allow a title of one repeated character" do + topic.title = xxxxx_title + topic.should_not be_valid + end + + # describe "with a name" do + # it "is not valid without a name" do + # subject.stub(:name => nil) + # subject.should_not be_valid + # subject.should have(1).error_on(:name) + # subject.errors[:name].first.should == "can't be blank" + # end + # end +end + diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index c0075f8ca29..27e9e127a79 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -26,30 +26,6 @@ describe Topic do it_behaves_like "a versioned model" - context '.title_quality' do - - it "strips a title when identifying length" do - Fabricate.build(:topic, title: (" " * SiteSetting.min_topic_title_length) + "x").should_not be_valid - end - - it "doesn't allow a long title" do - Fabricate.build(:topic, title: "x" * (SiteSetting.max_topic_title_length + 1)).should_not be_valid - end - - it "doesn't allow a short title" do - Fabricate.build(:topic, title: "x" * (SiteSetting.min_topic_title_length + 1)).should_not be_valid - end - - it "allows a regular title with a few ascii characters" do - Fabricate.build(:topic, title: "hello this is my cool topic! welcome: all;").should be_valid - end - - it "allows non ascii" do - Fabricate.build(:topic, title: "Iñtërnâtiônàlizætiøn").should be_valid - end - - end - context 'slug' do let(:title) { "hello world topic" } @@ -102,7 +78,7 @@ describe Topic do SiteSetting.expects(:allow_duplicate_topic_titles?).returns(true) end - it "won't allow another topic to be created with the same name" do + it "will allow another topic to be created with the same name" do new_topic.should be_valid end end @@ -112,10 +88,7 @@ describe Topic do context 'html in title' do def build_topic_with_title(title) - t = build(:topic, title: title) - t.sanitize_title - t.title_quality - t + build(:topic, title: title).tap{ |t| t.valid? } end let(:topic_bold) { build_topic_with_title("Topic with bold text in its title" ) }