Validating models on destruction

rubyonrails-core
1

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

2

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.

3

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?