refactor Topic validation

introduce a couple of custom validators
fix minor discrepancies in tests
copy I18n error message keys to default location
clean up validation invocation
move some responsibilities out of validator into class

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
+  before_validation :sanitize_title
-  validates_presence_of :title
+
-  validate :title, -> { SiteSetting.topic_title_length.include? :length }
+  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: [])

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.

config/locales/server.cs.yml

@@ -104,6 +104,9 @@ cs:
       post:
         raw: "Tělo"
     errors:
+      messages:
+        has_already_been_used: "již bylo použito"
+        is_invalid: "je neplatné; zkuste se vyjádřit více popisně"
       models:
         topic:
           attributes:

config/locales/server.da.yml

@@ -69,6 +69,9 @@ da:
       post:
         raw: "Brødtekst"
     errors:
+      messages:
+        has_already_been_used: "er allerede i brug"
+        is_invalid: "er ugyldig; prøv at være mere beskrivende"
       models:
         topic:
           attributes:

config/locales/server.de.yml

@@ -80,6 +80,9 @@ de:
       post:
         raw: "Hauptteil"
     errors:
+      messages:
+        has_already_been_used: "wurde schon benutzt"
+        is_invalid: "ist ungültig; bitte ein wenig anschaulicher"
       models:
         topic:
           attributes:

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:

config/locales/server.es.yml

@@ -69,6 +69,9 @@ es:
       post:
         raw: "Body"
     errors:
+      messages:
+        has_already_been_used: "has already been used"
+        is_invalid: "is invalid; try to be a little more descriptive"
       models:
         topic:
           attributes:

config/locales/server.fr.yml

@@ -107,6 +107,9 @@ fr:
       post:
         raw: "Corps"
     errors:
+      messages:
+        has_already_been_used: "a déjà été utilisé"
+        is_invalid: "est invalide; essayez d'être plus précis"
       models:
         topic:
           attributes:

config/locales/server.id.yml

@@ -69,6 +69,9 @@ id:
       post:
         raw: "Body"
     errors:
+      messages:
+        has_already_been_used: "has already been used"
+        is_invalid: "is invalid; try to be a little more descriptive"
       models:
         topic:
           attributes:

config/locales/server.it.yml

@@ -100,6 +100,9 @@ it:
       post:
         raw: "Contenuto"
     errors:
+      messages:
+        has_already_been_used: "è già stato usato"
+        is_invalid: "non è valido; prova ad essere un po' più descrittivo"
       models:
         topic:
           attributes:

config/locales/server.nl.yml

@@ -103,6 +103,9 @@ nl:
       post:
         raw: Inhoud
     errors:
+      messages:
+        has_already_been_used: is al eens gebruikt
+        is_invalid: "is niet geldig; probeer het iets beter te omschrijven"
       models:
         topic:
           attributes:
@@ -433,8 +436,8 @@ nl:
     crawl_images: Zet het ophalen van afbeeldingen van externe bronnen aan
     ninja_edit_window: "Hoe snel je een aanpassing kan maken zonder dat er een nieuwe versie wordt opgeslagen, in seconden."
     enable_imgur: "Gebruik de imgur API voor uploads en sla afbeeldingen niet lokaal op"
-    imgur_client_id: "Je imgur.com client ID, nodig om afbeeldingen te kunnen uploaden naar imgur" 
+    imgur_client_id: "Je imgur.com client ID, nodig om afbeeldingen te kunnen uploaden naar imgur"
-    imgur_client_secret: "Je imgur.com client secret. Is nog niet nodig voor het uploaden van afbeeldingen, maar dat zou in de toekomst kunnen veranderen." 
+    imgur_client_secret: "Je imgur.com client secret. Is nog niet nodig voor het uploaden van afbeeldingen, maar dat zou in de toekomst kunnen veranderen."
     imgur_endpoint: "End point voor het uploaden van imgur.com-afbeeldingen"
     max_image_width: Maximale breedte voor een afbeelding in een bericht
     category_featured_topics: Aantal topics dat wordt weergegeven in de categorielijst

config/locales/server.pseudo.yml

@@ -83,6 +83,10 @@ pseudo:
       post:
         raw: '[[ Ɓóďý ]]'
     errors:
+      messages:
+        has_already_been_used: '[[ ĥáš áłřéáďý ƀééɳ ůšéď ]]'
+        is_invalid: '[[ íš íɳνáłíď; ťřý ťó ƀé á łíťťłé ɱóřé ďéščříƿťíνé ]]'
+
       models:
         topic:
           attributes:

config/locales/server.pt.yml

@@ -56,6 +56,9 @@ pt:
       post:
         raw: "Corpo"
     errors:
+      messages:
+        has_already_been_used: "já foi usado"
+        is_invalid: "é inválido; tenta ser um bocado mais descritivo"
       models:
         topic:
           attributes:

config/locales/server.sv.yml

@@ -83,6 +83,9 @@ sv:
       post:
         raw: "Inlägg"
     errors:
+      messages:
+        has_already_been_used: "har redan använts"
+        is_invalid: "är otillåtet; försök att vara lite mer beskrivande"
       models:
         topic:
           attributes:

config/locales/server.zh_CN.yml

@@ -89,6 +89,9 @@ zh_CN:
       post:
         raw: "内容"
     errors:
+      messages:
+        has_already_been_used: "已经被使用"
+        is_invalid: "无效，请描述得更清楚一点"
       models:
         topic:
           attributes:

config/locales/server.zh_TW.yml

@@ -88,6 +88,9 @@ zh_TW:
       post:
         raw: "內容"
     errors:
+      messages:
+        has_already_been_used: "已經被使用"
+        is_invalid: "無效，請描述得更清楚一點"
       models:
         topic:
           attributes:

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.to_s.strip.split('').uniq.size
-    @entropy ||= @text.strip.each_char.to_a.uniq.size
   end
 
   def valid?
     # Blank strings are not valid
-    return false if @text.blank? || @text.strip.blank?
+    @text.present? &&
 
-    # Entropy check if required
+    # Minimum entropy if entropy check required
-    return false if @opts[:min_entropy].present? && (entropy < @opts[:min_entropy])
+    (@opts[:min_entropy].blank? || (entropy >= @opts[:min_entropy])) &&
 
-    # We don't have a comprehensive list of symbols, but this will eliminate some noise
+    # At least some non-symbol characters
-    non_symbols = @text.gsub(TextSentinel.non_symbols_regexp, '').size
+    # (We don't have a comprehensive list of symbols, but this will eliminate some noise)
-    return false if non_symbols == 0
+    (@text.gsub(TextSentinel.non_symbols_regexp, '').size > 0) &&
 
-    # Don't allow super long strings without spaces
+    # Don't allow super long words if there is a word length maximum
-    return false if @opts[:max_word_length] && @text =~ /\w{#{@opts[:max_word_length]},}(\s|$)/
+    (@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
 

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

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

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
+

spec/models/topic_spec.rb

@@ -102,7 +102,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 +112,7 @@ describe Topic do
   context 'html in title' do
 
     def build_topic_with_title(title)
-      t = build(:topic, title: title)
+      build(:topic, title: title).tap{ |t| t.valid? }
-      t.sanitize_title
-      t.title_quality
-      t
     end
 
     let(:topic_bold) { build_topic_with_title("Topic with <b>bold</b> text in its title" ) }