@@ -1,5 +1,8 @@ This will only document major changes. Please see the commit log for minor changes. +-2009-09-25 +* fixing bug with has_many :through not respecting is_paranoid conditions (thanks, Ben Johnson) + -2009-09-18 * making is_paranoid play nice with habtm relationships CHANGELOG @@ -2,7 +2,11 @@ h1. is_paranoid ( same as it ever was ) h3. advice and disclaimer -You should always declare is_paranoid before any associations in your model unless you have a good reason for doing otherwise. +You should always declare is_paranoid before any associations in your model unless you have a good reason for doing otherwise. Some relationships might not behave properly if you fail to do so. If you know what you're doing (and have written tests) and want to supress the warning then you can pass :suppress_load_order_warning => true as an option. + +<pre> + is_paranoid :suppress_load_order_warning => true +</pre> You should never expect _any_ library to work or behave exactly how you want it to: test, test, test and file an issue if you have any problems. Bonus points if you include sample failing code. Extra bonus points if you send a pull request that implements a feature/fixes a bug. README.textile @@ -15,6 +15,10 @@ module IsParanoid class_inheritable_accessor :destroyed_field, :field_destroyed, :field_not_destroyed self.destroyed_field, self.field_destroyed, self.field_not_destroyed = opts[:field] + if self.reflect_on_all_associations.size > 0 && ! opts[:suppress_load_order_warning] + warn "is_paranoid warning in class #{self}: You should declare is_paranoid before your associations" + end + # This is the real magic. All calls made to this model will append # the conditions deleted_at => nil (or whatever your destroyed_field # and field_not_destroyed are). All exceptions require using @@ -27,6 +31,25 @@ module IsParanoid end module ClassMethods + def is_or_equals_not_destroyed + if [nil, 'NULL'].include?(field_not_destroyed) + 'IS NULL' + else + "= #{field_not_destroyed}" + end + end + + # ensure that we respect the is_paranoid conditions when being loaded as a has_many :through + # NOTE: this only works if is_paranoid is declared before has_many relationships. + def has_many(association_id, options = {}, &extension) + if options.key?(:through) + conditions = "#{options[:through].to_s.pluralize}.#{destroyed_field} #{is_or_equals_not_destroyed}" + options[:conditions] = "(" + [options[:conditions], conditions].compact.join(") AND (") + ")" + end + super + end + + # Actually delete the model, bypassing the safety net. Because # this method is called internally by Model.delete(id) and on the # delete method in each instance, we don't need to specify those lib/is_paranoid.rb @@ -72,6 +72,24 @@ describe IsParanoid do Android.count_with_destroyed.should == 2 end + it "shouldn't have problems with has_many :through relationships" do + # TODO: this spec can be cleaner and more specific, replace it later + # Dings use a boolean non-standard is_paranoid field + # Scratch uses the defaults. Testing both ensures compatibility + [[:dings, Ding], [:scratches, Scratch]].each do |method, klass| + @r2d2.dings.should == [] + + dent = Dent.create(:description => 'really terrible', :android_id => @r2d2.id) + item = klass.create(:description => 'quite nasty', :dent_id => dent.id) + @r2d2.reload + @r2d2.send(method).should == [item] + + dent.destroy + @r2d2.reload + @r2d2.send(method).should == [] + end + end + it "should not choke has_and_belongs_to_many relationships" do @r2d2.places.should include(@tatooine) @tatooine.destroy @@ -298,5 +316,4 @@ describe IsParanoid do uuid.destroy.should be_true end end - end spec/is_paranoid_spec.rb @@ -4,14 +4,16 @@ class Person < ActiveRecord::Base #:nodoc: end class Android < ActiveRecord::Base #:nodoc: + is_paranoid validates_uniqueness_of :name has_many :components, :dependent => :destroy has_one :sticker has_many :memories, :foreign_key => 'parent_id' + has_many :dents + has_many :dings, :through => :dents + has_many :scratches, :through => :dents has_and_belongs_to_many :places - is_paranoid - # this code is to ensure that our destroy and restore methods # work without triggering before/after_update callbacks before_update :raise_hell @@ -20,6 +22,23 @@ class Android < ActiveRecord::Base #:nodoc: end end +class Dent < ActiveRecord::Base #:nodoc: + is_paranoid + belongs_to :android + has_many :dings + has_many :scratches +end + +class Ding < ActiveRecord::Base #:nodoc: + is_paranoid :field => [:not_deleted, true, false] + belongs_to :dent +end + +class Scratch < ActiveRecord::Base #:nodoc: + is_paranoid + belongs_to :dent +end + class Component < ActiveRecord::Base #:nodoc: is_paranoid belongs_to :android, :dependent => :destroy spec/models.rb @@ -7,6 +7,24 @@ ActiveRecord::Schema.define(:version => 20090317164830) do t.datetime "updated_at" end + create_table "dents", :force => true do |t| + t.integer "android_id" + t.string "description" + t.datetime "deleted_at" + end + + create_table "dings", :force => true do |t| + t.integer "dent_id" + t.string "description" + t.boolean "not_deleted" + end + + create_table "scratches", :force => true do |t| + t.integer "dent_id" + t.string "description" + t.datetime "deleted_at" + end + create_table "androids_places", :force => true, :id => false do |t| t.integer "android_id" t.integer "place_id" spec/schema.rb 0bc96a8ffd960e428f0ee6b7610d921b300995b9 Jeffrey Chupp jeff@semanticart.com http://github.com/semanticart/is_paranoid/commit/fae6f0d3b056f7ba290d71fb6907d7cd6b933023 fae6f0d3b056f7ba290d71fb6907d7cd6b933023 2009-09-26T08:11:16-07:00 2009-09-26T07:57:25-07:00 fixing bug with has_many :through not respecting is_paranoid conditions (thanks, Ben Johnson) 31ea77a0e9f1d504c98a35d39c90af05875ba132 Jeffrey Chupp jeff@semanticart.com