From a48f7ba61ce85c1fb98653c15b7a00da2aed3e39 Mon Sep 17 00:00:00 2001
From: Bianca Nenciu <nbianca@users.noreply.github.com>
Date: Wed, 11 Nov 2020 15:11:36 +0200
Subject: [PATCH] FEATURE: Improve errors when title is invalid (#11149)

It used to simply say "title is invalid" without giving any hint what
the problem could be. This commit adds different errors messages for
all caps titles, low entropy titles or titles with very long words.
---
 config/locales/server.en.yml                  |  3 ++
 lib/text_sentinel.rb                          | 20 ++++++-------
 lib/validators/quality_title_validator.rb     | 13 ++++++++-
 .../quality_title_validator_spec.rb           | 29 ++++++++++++++-----
 4 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 66bfadabde3..3b47da5ff74 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -149,6 +149,9 @@ en:
       inclusion: is not included in the list
       invalid: is invalid
       is_invalid: "seems unclear, is it a complete sentence?"
+      is_invalid_meaningful: "seems unclear, most of the words contain the same letters over and over?"
+      is_invalid_unpretentious: "seems unclear, one or more words is very long?"
+      is_invalid_quiet: "seems unclear, did you mean to enter it in ALL CAPS?"
       invalid_timezone: "'%{tz}' is not a valid timezone"
       contains_censored_words: "contains the following censored words: %{censored_words}"
       less_than: must be less than %{count}
diff --git a/lib/text_sentinel.rb b/lib/text_sentinel.rb
index b3d9abb4316..72053574fb6 100644
--- a/lib/text_sentinel.rb
+++ b/lib/text_sentinel.rb
@@ -54,12 +54,6 @@ class TextSentinel
     seems_quiet?
   end
 
-  private
-
-  def symbols_regex
-    /[\ -\/\[-\`\:-\@\{-\~]/m
-  end
-
   def seems_meaningful?
     # Minimum entropy if entropy check required
     @opts[:min_entropy].blank? || (entropy >= @opts[:min_entropy])
@@ -71,10 +65,6 @@ class TextSentinel
     @text.gsub(symbols_regex, '').size > 0
   end
 
-  def skipped_locale
-    %w(zh_CN zh_TW ko ja).freeze
-  end
-
   def seems_unpretentious?
     return true if skipped_locale.include?(SiteSetting.default_locale)
     # Don't allow super long words if there is a word length maximum
@@ -87,4 +77,14 @@ class TextSentinel
     SiteSetting.allow_uppercase_posts || @text == @text.mb_chars.downcase.to_s || @text != @text.mb_chars.upcase.to_s
   end
 
+  private
+
+  def symbols_regex
+    /[\ -\/\[-\`\:-\@\{-\~]/m
+  end
+
+  def skipped_locale
+    %w(zh_CN zh_TW ko ja).freeze
+  end
+
 end
diff --git a/lib/validators/quality_title_validator.rb b/lib/validators/quality_title_validator.rb
index d7c8ebbf5bb..c05ae07f9de 100644
--- a/lib/validators/quality_title_validator.rb
+++ b/lib/validators/quality_title_validator.rb
@@ -6,6 +6,17 @@ 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?
+
+    if !sentinel.valid?
+      if !sentinel.seems_meaningful?
+        record.errors.add(attribute, :is_invalid_meaningful)
+      elsif !sentinel.seems_unpretentious?
+        record.errors.add(attribute, :is_invalid_unpretentious)
+      elsif !sentinel.seems_quiet?
+        record.errors.add(attribute, :is_invalid_quiet)
+      else
+        record.errors.add(attribute, :is_invalid)
+      end
+    end
   end
 end
diff --git a/spec/components/validators/quality_title_validator_spec.rb b/spec/components/validators/quality_title_validator_spec.rb
index 121a18e98c1..509a893204f 100644
--- a/spec/components/validators/quality_title_validator_spec.rb
+++ b/spec/components/validators/quality_title_validator_spec.rb
@@ -18,6 +18,10 @@ describe "A record validated with QualityTitleValidator" do
   let(:long_title) { valid_title.center(SiteSetting.max_topic_title_length + 1, 'x') }
   let(:xxxxx_title) { valid_title.gsub(/./, 'x') }
 
+  let(:meaningless_title) { "asdf asdf asdf" }
+  let(:loud_title) { "ALL CAPS INVALID TITLE" }
+  let(:pretentious_title) { "superverylongwordintitlefornoparticularreason" }
+
   subject(:topic) { QualityTitleValidatorSpec::Validatable.new }
 
   before(:each) do
@@ -67,12 +71,21 @@ describe "A record validated with QualityTitleValidator" do
     expect(topic).not_to 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
+  it "doesn't allow a meaningless title" do
+    topic.title = meaningless_title
+    expect(topic).not_to be_valid
+    expect(topic.errors.full_messages.first).to include(I18n.t("errors.messages.is_invalid_meaningful"))
+  end
+
+  it "doesn't allow a pretentious title" do
+    topic.title = pretentious_title
+    expect(topic).not_to be_valid
+    expect(topic.errors.full_messages.first).to include(I18n.t("errors.messages.is_invalid_unpretentious"))
+  end
+
+  it "doesn't allow a loud title" do
+    topic.title = loud_title
+    expect(topic).not_to be_valid
+    expect(topic.errors.full_messages.first).to include(I18n.t("errors.messages.is_invalid_quiet"))
+  end
 end