From ce9e6a66692dce77492fbd963cefcf74ad9eddae Mon Sep 17 00:00:00 2001 From: Anthony Smith Date: Wed, 26 Aug 2015 16:45:34 +0100 Subject: [PATCH 1/2] Do not observe the same model more than once. Fixes #35. * This was happening when a subclass of an observed class was created. * The subclass was being observed twice. * I've changed one of the tests to catch this. --- lib/rails/observers/active_model/observing.rb | 1 + test/observing_test.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/rails/observers/active_model/observing.rb b/lib/rails/observers/active_model/observing.rb index 3f01a90..387070e 100644 --- a/lib/rails/observers/active_model/observing.rb +++ b/lib/rails/observers/active_model/observing.rb @@ -307,6 +307,7 @@ class << self def observe(*models) models.flatten! models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model } + models.uniq! singleton_class.redefine_method(:observed_classes) { models } end diff --git a/test/observing_test.rb b/test/observing_test.rb index 120d531..10386ff 100644 --- a/test/observing_test.rb +++ b/test/observing_test.rb @@ -100,7 +100,7 @@ def teardown test "passes observers to subclasses" do FooObserver.instance bar = Class.new(Foo) - assert_equal Foo.observers_count, bar.observers_count + assert_equal [Foo, bar], FooObserver.observed_classes end end From 8080eea68af707541aa5d462ff08b12fe0ddf2c3 Mon Sep 17 00:00:00 2001 From: Anthony Smith Date: Thu, 27 Aug 2015 11:20:45 +0100 Subject: [PATCH 2/2] Add subclasses to observed_classes when observer is instantiated. For #35. * Subclasses were being observed (via add_observer) but not added to observed_classes * Added call to observe() in initialize to update observed_classes * Moved searching of descendants to initialize, so the comment "Start observing the declared classes and their subclasses." will be correct for all subclasses of ActiveModel::Observer (not just ActiveRecord::Observer) * Removed ActiveRecord::Observer#observed_classes, as descendants are now searched on initialize (this was the main cause of #35) * No need for models.uniq! in #observe (undoing previous commit) * Improved tests to better cover uniqueness of observed_classes * Tests don't work with minitest 5.8.0; require < 5 --- lib/rails/observers/active_model/observing.rb | 6 ++++-- lib/rails/observers/activerecord/observer.rb | 5 ----- rails-observers.gemspec | 2 +- test/lifecycle_test.rb | 8 +++++++- test/observing_test.rb | 16 ++++++++-------- 5 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lib/rails/observers/active_model/observing.rb b/lib/rails/observers/active_model/observing.rb index 387070e..8206b4b 100644 --- a/lib/rails/observers/active_model/observing.rb +++ b/lib/rails/observers/active_model/observing.rb @@ -307,7 +307,6 @@ class << self def observe(*models) models.flatten! models.collect! { |model| model.respond_to?(:to_sym) ? model.to_s.camelize.constantize : model } - models.uniq! singleton_class.redefine_method(:observed_classes) { models } end @@ -339,7 +338,10 @@ def observed_class # Start observing the declared classes and their subclasses. # Called automatically by the instance method. def initialize #:nodoc: - observed_classes.each { |klass| add_observer!(klass) } + klasses = observed_classes + klasses = (klasses + klasses.map(&:descendants).flatten).uniq + self.class.observe(klasses) + klasses.each { |klass| add_observer!(klass) } end def observed_classes #:nodoc: diff --git a/lib/rails/observers/activerecord/observer.rb b/lib/rails/observers/activerecord/observer.rb index 5218008..760c87a 100644 --- a/lib/rails/observers/activerecord/observer.rb +++ b/lib/rails/observers/activerecord/observer.rb @@ -96,11 +96,6 @@ class Observer < ActiveModel::Observer protected - def observed_classes - klasses = super - klasses + klasses.map { |klass| klass.descendants }.flatten - end - def add_observer!(klass) super define_callbacks klass diff --git a/rails-observers.gemspec b/rails-observers.gemspec index 52526ad..415a562 100644 --- a/rails-observers.gemspec +++ b/rails-observers.gemspec @@ -18,7 +18,7 @@ Gem::Specification.new do |s| s.add_dependency 'activemodel', '>= 4.0' - s.add_development_dependency 'minitest', '>= 3' + s.add_development_dependency 'minitest', '>= 3', '< 5' s.add_development_dependency 'railties', '>= 4.0' s.add_development_dependency 'activerecord', '>= 4.0' s.add_development_dependency 'actionmailer', '>= 4.0' diff --git a/test/lifecycle_test.rb b/test/lifecycle_test.rb index fb55206..ca5a3b4 100644 --- a/test/lifecycle_test.rb +++ b/test/lifecycle_test.rb @@ -131,7 +131,7 @@ def test_before_destroy def test_auto_observer topic_observer = TopicaAuditor.instance assert_nil TopicaAuditor.observed_class - assert_equal [Topic], TopicaAuditor.observed_classes.to_a + assert_equal [Topic, Reply, AroundTopic], TopicaAuditor.observed_classes.to_a topic = Topic.find(1) assert_equal topic.title, topic_observer.topic.title @@ -161,9 +161,15 @@ def test_observing_subclasses developer = SpecialDeveloper.find(1) assert_equal developer.name, multi_observer.record.name + # SpecialDeveloper was observed when DeveloperObserver initialized + assert_equal [Developer, SpecialDeveloper], DeveloperObserver.observed_classes + klass = Class.new(Developer) assert_equal klass, multi_observer.last_inherited + # Subclassing Developer adds new subclass to observed_classes + assert_equal [Developer, SpecialDeveloper, klass], DeveloperObserver.observed_classes + developer = klass.find(1) assert_equal developer.name, multi_observer.record.name end diff --git a/test/observing_test.rb b/test/observing_test.rb index 10386ff..c7c0304 100644 --- a/test/observing_test.rb +++ b/test/observing_test.rb @@ -124,26 +124,26 @@ def teardown end test "tracks implicit observable models" do - instance = FooObserver.new - assert_equal [Foo], instance.observed_classes + FooObserver.instance + assert_equal [Foo], FooObserver.observed_classes end test "tracks explicit observed model class" do FooObserver.observe ObservedModel - instance = FooObserver.new - assert_equal [ObservedModel], instance.observed_classes + FooObserver.instance + assert_equal [ObservedModel], FooObserver.observed_classes end test "tracks explicit observed model as string" do FooObserver.observe 'observed_model' - instance = FooObserver.new - assert_equal [ObservedModel], instance.observed_classes + FooObserver.instance + assert_equal [ObservedModel], FooObserver.observed_classes end test "tracks explicit observed model as symbol" do FooObserver.observe :observed_model - instance = FooObserver.new - assert_equal [ObservedModel], instance.observed_classes + FooObserver.instance + assert_equal [ObservedModel], FooObserver.observed_classes end test "calls existing observer event" do