From 806255b3c49da12e4af36ede2fba34023278aa7b Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Wed, 22 May 2013 21:52:12 -0700 Subject: [PATCH] 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 | 50 ++++--------- config/application.rb | 3 +- config/locales/server.cs.yml | 3 + config/locales/server.da.yml | 3 + config/locales/server.de.yml | 3 + config/locales/server.en.yml | 3 + config/locales/server.es.yml | 3 + config/locales/server.fr.yml | 3 + config/locales/server.id.yml | 3 + config/locales/server.it.yml | 3 + config/locales/server.nl.yml | 7 +- config/locales/server.pseudo.yml | 4 + config/locales/server.pt.yml | 3 + config/locales/server.sv.yml | 3 + config/locales/server.zh_CN.yml | 3 + config/locales/server.zh_TW.yml | 3 + lib/text_sentinel.rb | 24 +++--- lib/validators/quality_title_validator.rb | 9 +++ lib/validators/unique_among_validator.rb | 21 ++++++ .../quality_title_validator_spec.rb | 73 +++++++++++++++++++ spec/models/topic_spec.rb | 7 +- 21 files changed, 178 insertions(+), 56 deletions(-) create mode 100644 lib/validators/quality_title_validator.rb create mode 100644 lib/validators/unique_among_validator.rb create mode 100644 spec/components/validators/quality_title_validator_spec.rb 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.cs.yml b/config/locales/server.cs.yml index 108237ca0b1..991a11665ea 100644 --- a/config/locales/server.cs.yml +++ b/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: diff --git a/config/locales/server.da.yml b/config/locales/server.da.yml index 1c8a8ca8ea8..ecf3d97bb6d 100644 --- a/config/locales/server.da.yml +++ b/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: diff --git a/config/locales/server.de.yml b/config/locales/server.de.yml index f0852e53bd5..98f5a5c89ed 100644 --- a/config/locales/server.de.yml +++ b/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: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 1d4faa1d0e7..631cc88284e 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/config/locales/server.es.yml b/config/locales/server.es.yml index be883eb7135..2d1cfc42c4b 100644 --- a/config/locales/server.es.yml +++ b/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: diff --git a/config/locales/server.fr.yml b/config/locales/server.fr.yml index 833f9b7f7c7..e15c2f757b3 100644 --- a/config/locales/server.fr.yml +++ b/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: diff --git a/config/locales/server.id.yml b/config/locales/server.id.yml index 2dc03c92c8c..9246146f497 100644 --- a/config/locales/server.id.yml +++ b/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: diff --git a/config/locales/server.it.yml b/config/locales/server.it.yml index 280b3e9d4c2..344799d7c98 100644 --- a/config/locales/server.it.yml +++ b/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: diff --git a/config/locales/server.nl.yml b/config/locales/server.nl.yml index 335d2d8c538..48a3ed31832 100644 --- a/config/locales/server.nl.yml +++ b/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_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_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_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 diff --git a/config/locales/server.pseudo.yml b/config/locales/server.pseudo.yml index bf22177dff0..ddd24ba8a9f 100644 --- a/config/locales/server.pseudo.yml +++ b/config/locales/server.pseudo.yml @@ -83,6 +83,10 @@ pseudo: post: raw: '[[ Ɓóďý ]]' errors: + messages: + has_already_been_used: '[[ ĥáš áłřéáďý ƀééɳ ůšéď ]]' + is_invalid: '[[ íš íɳνáłíď; ťřý ťó ƀé á łíťťłé ɱóřé ďéščříƿťíνé ]]' + models: topic: attributes: diff --git a/config/locales/server.pt.yml b/config/locales/server.pt.yml index 72258944af3..38b514b43a2 100644 --- a/config/locales/server.pt.yml +++ b/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: diff --git a/config/locales/server.sv.yml b/config/locales/server.sv.yml index 507ed3f37c9..4bbe0aebca5 100644 --- a/config/locales/server.sv.yml +++ b/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: diff --git a/config/locales/server.zh_CN.yml b/config/locales/server.zh_CN.yml index 46434ab43ed..30c603733a7 100644 --- a/config/locales/server.zh_CN.yml +++ b/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: diff --git a/config/locales/server.zh_TW.yml b/config/locales/server.zh_TW.yml index ee154a2d3e4..7e9bd1aeca1 100644 --- a/config/locales/server.zh_TW.yml +++ b/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: 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..e2486c753d0 100644 --- a/spec/models/topic_spec.rb +++ b/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) - 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" ) }