views:

301

answers:

2

Hi,

I'm having a problem with validation in my RoR Model:

def save
  self.accessed = Time.now.to_s
  self.modified = accessed
  validate_username
  super
end
def validate_username
  if User.find(:first, :select => :id, :conditions => ["userid = '#{self.userid}'"])
    self.errors.add(:userid, "already exists")
  end
end

As you can see, I've replaced the Model's save method with my own, calling validate_username before I call the parent .save method. My Problem is, that, even though the error is being added, Rails still tries to insert the new row into the database, even if the user name is a duplicate. What am I doing wrong here?

PS: I'm not using validate_uniqueness_of because of the following issue with case sensitivity: https://rails.lighthouseapp.com/projects/8994/tickets/2503-validates%5Funiqueness%5Fof-is-horribly-inefficient-in-mysql

Update: I tried weppos solution, and it works, but not quite as I'd like it to. Now, the field gets marked as incorrect, but only if all other fields are correct. What I mean is, if I enter a wrong E-Mail address for example, the email field is marked es faulty, the userid field is not. When I submit a correct email address then, the userid fields gets marked as incorrect. Hope you guys understand what I mean :D

Update2: The data should be validated in a way, that it should not be possible to insert duplicate user ids into the database, case insensitive. The user ids have the format "user-domain", eg. "test-something.net". Unfortunately, validates_uniqueness_of :userid does not work, it tries to insert "test-something.net" into the database even though there already is an "Test-something.net". validate_username was supposed to be my (quick) workaround for this problem, but it didn't work. weppos solution did work, but not quite as I want it to (as explained in my first update).

Haven't figured this out yet... anyone?

Best regards, x3ro

A: 

How about calling super only if validate_username returns true or something similar?

def save
  self.accessed = Time.now.to_s
  self.modified = accessed
  super if validate_username
end
def validate_username
  if User.find(:first, :select => :id, :conditions => ["userid = '#{self.userid}'"])
    self.errors.add(:userid, "already exists")
    return false
  end
end

... I think that you could also remove totally the super call. Not sure, but you could test it out.

marcgg
+5  A: 

Why don't you use a callback and leave the save method untouched? Also, avoid direct SQL value interpolation.

class ... < ActiveRecord::Base

  before_save :set_defaults
  before_create :validate_username

  protected

  def set_defaults
    self.accessed = Time.now.to_s
    self.modified = accessed
  end

  def validate_username
    errors.add(:userid, "already exists") if User.exists?(:userid => self.userid)
    errors.empty?
  end

end
Simone Carletti
before_create instead of before_save. Otherwise validate_username will fail on all saves. Otherwise great answer.
EmFi
Good point EmFi!
Simone Carletti