Validating models on destruction

[Ryan Angilly]

Hey guys,

Have you ever wanted to validate a record was valid for destruction?  Does this make sense conceptually?  Let me paint a picture of something I just ran into that makes me think I should be able to do this.

I have a many to many polymorphic relationship between our Advertisement model and a bunch of other models in our app.  One of these models is the User model.  At any point I may want to find the ad for top_dashboard_ad_1 for a given user, but if that user doesnt have an ad specified I want to fall back to our defaults:

@user.ad_for('top_dashboard_ad_1')

class User < AR::B
  def ad_for(type)
    advertisements.find_by_location(type) || SiteSettings.instance.ad_for(type)
  end
end

SiteSettings is a singleton model that also has a polymorphic relationship with the Advertisement model.  This is all CMS'd.  A certain User can have special ads assigned, and the defaults can be changed at any time.  Here's the kicker: If a certain advertisement is associated with the SiteSettings instance (meaning it is a default), I dont want it to allow it to be destroyed.

I currently have this in my Advertisement model:

class Advertisement < AR::B
  before_destroy :make_sure_not_default

  private
  def make_sure_not_default
    !(ad_zone_placements.map{|a| a.placement.class}.include? SiteSettings
  end
end

This does prevent the record from being destroyed, but doesnt provide any feedback to the user as to why.

Seems like this should be a validation.  I want to be able to use AR::Errors to send a message back to host explaining why it cannot be destroyed, but that behavior kind of flies in the face of the conventional restful controller generated in Rails scaffold (ie: in the destroy action, there is no 'check for errors and render the edit page').  I feel like I should be able to do:

  validates_on_destroy :make_sure_not_default

  def make_sure_not_default
    errors.add_to_base('Cannot destroy this AdZone while it is a default') if ad_zone_placements.map{|a| a.placement.class}.include? SiteSettings
  end


Right?  Has anyone else ever ran into this?  Is there something built in I'm missing?  Am I have a braindead moment like that time I forgot about "has_many :through" ?

Thanks,
Ryan

[dasil003]

I understand where you're coming from here. I faced this issue just
last week. However I think it's a bit tricky to piggyback this onto
validations, because validations are conceptually designed around
record creation. It's all built to support the idea of showing errors
on specific fields in an edit form. When it comes to destroying
records I'm not sure it makes conceptual sense to flag individual
fields with errors. In some cases yes, but more often it seems like
it would be due to some combined state with other records and
associations and in general maps much less cleanly to fields.

I would definitely support some infrastructure improvement here, but I
think we'd need to take a deep look at AR::Errors and figure out what
would be the useful part for destroy validations, and make sure to
keep the terminology clear... you wouldn't want valid? to return false
because a destroy validation failed for instance.

[Ryan Angilly]

On Thu, Nov 12, 2009 at 2:10 PM, dasil003 <gabr...@gmail.com> wrote:

I understand where you're coming from here.  I faced this issue just
last week.  However I think it's a bit tricky to piggyback this onto
validations, because validations are conceptually designed around
record creation.  It's all built to support the idea of showing errors
on specific fields in an edit form.  When it comes to destroying
records I'm not sure it makes conceptual sense to flag individual
fields with errors.  In some cases yes, but more often it seems like
it would be due to some combined state with other records and
associations and in general maps much less cleanly to fields.

Right, which is why in this case I wanted to use add_to_base
 

I would definitely support some infrastructure improvement here, but I
think we'd need to take a deep look at AR::Errors and figure out what
would be the useful part for destroy validations, and make sure to
keep the terminology clear... you wouldn't want valid? to return false 

because a destroy validation failed for instance.

Agreed.

I'm not saying we should take all the current validations (uniqueness_of, presence_of, etc...) and give them the option to run on destruction.  Because you're right that doesnt make any sense.  But I can see adding two methods: AR::B#validate_on_destroy and AR::B#valid_for_destruction?.   Then I'd modify the default scaffolding to check the return value of #destroy and conditionally display any errors that come back.

Any other thoughts?  Want patch?