Error: Display Name is invalid #157

Open
simon04 opened this Issue Nov 12, 2012 · 5 comments

Comments

Projects
None yet
2 participants
Member

simon04 commented Nov 12, 2012

The error "Display Name is invalid" while registering might not be too helpful. Instead, display which characters are not allowed (I'm not completely sure about the handling of \/):

diff --git a/app/models/user.rb b/app/models/user.rb
index 3b43130..27662f1 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -36,7 +36,7 @@ class User < ActiveRecord::Base
   validates_length_of :display_name, :within => 3..255, :allow_nil => true
   validates_email_format_of :email, :if => Proc.new { |u| u.email_changed? }
   validates_email_format_of :new_email, :allow_blank => true, :if => Proc.new { |u| u.new_email_changed? }
-  validates_format_of :display_name, :with => /^[^\/;.,?%#]*$/, :if => Proc.new { |u| u.display_name_changed? }
+  validates_format_of :display_name, :with => /^[^\/;.,?%#]*$/, :message => "contains invalid characters (\/;.,?%#)" :if => Proc.new { |u| u.display_name_changed? }
   validates_format_of :display_name, :with => /^\S/, :message => "has leading whitespace", :if => Proc.new { |u| u.display_name_changed? }
   validates_format_of :display_name, :with => /\S$/, :message => "has trailing whitespace", :if => Proc.new { |u| u.display_name_changed? }
   validates_numericality_of :home_lat, :allow_nil => true
Owner

tomhughes commented Nov 12, 2012

Do you really think a heiroglyphic message like that is going to mean anything to a non-programmer?

I can also guarantee that the two lists of invalid characters will never stay in sync...

Member

simon04 commented Nov 13, 2012

The wording of the message could/should be improved, yes. However, for me, it is not obvious at all that a period is not allowed, but a space and ☃ is (almost the opposite of the constraints in e-mail addresses).

Ad sync: I guess, now so many people are editing this regexp. A short code comment could be added …

Owner

tomhughes commented Nov 13, 2012

Well that brings me to another point that I realised after my previous comment - that message should be localised.

Member

simon04 commented Nov 13, 2012

So should "has leading whitespace"?

Owner

tomhughes commented Nov 13, 2012

Sure - any message in the code that is presented to the user should be localised. That one obviously got missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment