Permalink
Browse files

ActiveModel::Observing: stop using Observable Ruby module, re-impleme…

…nt `notify_observers`

`Observable#notify_observers` from Ruby always returns false (which halts ActiveRecord
callback chains) and has extra features (like `changed`) that were never used.

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
1 parent cf616e4 commit c2ca73c9ee5fc3dadf69cf565bd5e2bb30c82c50 @mislav mislav committed with jeremy Apr 16, 2010
Showing with 36 additions and 7 deletions.
  1. +16 −7 activemodel/lib/active_model/observing.rb
  2. +20 −0 activerecord/test/cases/lifecycle_test.rb
@@ -1,4 +1,3 @@
-require 'observer'
require 'singleton'
require 'active_support/core_ext/array/wrap'
require 'active_support/core_ext/module/aliasing'
@@ -8,10 +7,6 @@ module ActiveModel
module Observing
extend ActiveSupport::Concern
- included do
- extend Observable
- end
-
module ClassMethods
# Activates the observers assigned. Examples:
#
@@ -41,6 +36,22 @@ def instantiate_observers
observers.each { |o| instantiate_observer(o) }
end
+ def add_observer(observer)
@robertothais

robertothais Jun 25, 2010

What about delete_observers? Dynamically removing observers is useful in rake tasks and other contexts in which you don't want every observer firing.

@mislav

mislav Jun 25, 2010

Member

I wasn't sure that it was ever used. It wasn't covered by tests, for instance. Feel free to make a patch to add delete_observers method, but first think about other ways to prevent observers from loading, such as creating a special environment for rake tasks and disabling observers in that environment (by means of environment configuration).

@robertothais

robertothais Jun 25, 2010

Hi Mislav, thanks for the reply.
I stumbled upon a few articles on the web explaining how to disable observers, so it seems to me that quite a few people found use in this feature. In my particular case I want to disable observers only for one rake task, and not for the environment in general. I'll try coming up with a patch and some tests to cover this situation.

@spovich

spovich Jun 25, 2010

Here is a use case that is real world: We disabled observers on a model with an observer that would send emails based on a model add/update. We didn't want observers when seeding the db in development or running tests.

+1 for adding back in an easy way to delete/disable observers.

+ unless observer.respond_to? :update
+ raise ArgumentError, "observer needs to respond to `update'"
+ end
+ @observer_instances ||= []
+ @observer_instances << observer
+ end
+
+ def notify_observers(*arg)
+ if defined? @observer_instances
+ for observer in @observer_instances
+ observer.update(*arg)
+ end
+ end
+ end
+
protected
def instantiate_observer(observer) #:nodoc:
# string/symbol
@@ -56,7 +67,6 @@ def instantiate_observer(observer) #:nodoc:
# Notify observers when the observed class is subclassed.
def inherited(subclass)
super
- changed
notify_observers :observed_class_inherited, subclass
end
end
@@ -70,7 +80,6 @@ def inherited(subclass)
# notify_observers(:after_save)
# end
def notify_observers(method)
- self.class.changed
self.class.notify_observers(method, self)
end
end
@@ -7,6 +7,14 @@
class SpecialDeveloper < Developer; end
+class SalaryChecker < ActiveRecord::Observer
+ observe :special_developer
+
+ def before_save(developer)
+ return developer.salary > 80000
+ end
+end
+
class TopicaAuditor < ActiveRecord::Observer
observe :topic
@@ -159,4 +167,16 @@ def test_invalid_observer
assert_equal [ValidatedComment, ValidatedComment, ValidatedCommentObserver], callers,
"model callbacks did not fire before observers were notified"
end
+
+ test "able to save developer" do
+ SalaryChecker.instance # activate
+ developer = SpecialDeveloper.new :name => 'Roger', :salary => 100000
+ assert developer.save, "developer with normal salary failed to save"
+ end
+
+ test "unable to save developer with low salary" do
+ SalaryChecker.instance # activate
+ developer = SpecialDeveloper.new :name => 'Rookie', :salary => 50000
+ assert !developer.save, "allowed to save a developer with too low salary"
+ end
end

0 comments on commit c2ca73c

Please sign in to comment.