Merge pull request #333 from haines/method_missing

Refactor Decorator#method_missing

lib/draper/decorator.rb

@@ -157,21 +157,12 @@ module Draper
     end
 
     def method_missing(method, *args, &block)
-      super unless allow?(method)
-
-      if source.respond_to?(method)
-        self.class.send :define_method, method do |*args, &blokk|
-          source.send method, *args, &blokk
-        end
-
-        send method, *args, &block
+      if allow?(method) && source.respond_to?(method)
+        self.class.define_proxy(method)
+        send(method, *args, &block)
       else
         super
       end
-
-    rescue NoMethodError => no_method_error
-      super if no_method_error.name == method
-      raise no_method_error
     end
 
     # For ActiveModel compatibilty
@@ -184,7 +175,13 @@ module Draper
       source.to_param
     end
 
-  private
+    private
+
+    def self.define_proxy(method)
+      define_method(method) do |*args, &block|
+        source.send(method, *args, &block)
+      end
+    end
 
     def self.security
       @security ||= Security.new

spec/draper/collection_decorator_spec.rb

@@ -136,11 +136,11 @@ describe Draper::CollectionDecorator do
 
   describe ".helpers" do
     it "returns a HelperProxy" do
-      Decorator.helpers.should be_a Draper::HelperProxy
+      subject.class.helpers.should be_a Draper::HelperProxy
     end
 
     it "is aliased to .h" do
-      Decorator.h.should be Decorator.helpers
+      subject.class.h.should be subject.class.helpers
     end
   end
 

spec/draper/decorator_spec.rb

@@ -2,9 +2,9 @@ require 'spec_helper'
 
 describe Draper::Decorator do
   before { ApplicationController.new.view_context }
-  subject { Decorator.new(source) }
-  let(:source){ Product.new }
-  let(:non_active_model_source){ NonActiveModelProduct.new }
+  subject { decorator_class.new(source) }
+  let(:decorator_class) { Draper::Decorator }
+  let(:source) { Product.new }
 
   describe "#initialize" do
     it "sets the source" do
@@ -12,7 +12,7 @@ describe Draper::Decorator do
     end
 
     it "stores options" do
-      decorator = Decorator.new(source, some: "options")
+      decorator = decorator_class.new(source, some: "options")
       decorator.options.should == {some: "options"}
     end
 
@@ -106,7 +106,7 @@ describe Draper::Decorator do
   describe "#localize" do
     before { subject.helpers.should_receive(:localize).with(:an_object, {some: "options"}) }
 
-    it "delegates to helpers" do
+    it "delegates to #helpers" do
       subject.localize(:an_object, some: "options")
     end
 
@@ -117,16 +117,17 @@ describe Draper::Decorator do
 
   describe ".helpers" do
     it "returns a HelperProxy" do
-      Decorator.helpers.should be_a Draper::HelperProxy
+      subject.class.helpers.should be_a Draper::HelperProxy
     end
 
     it "is aliased to .h" do
-      Decorator.h.should be Decorator.helpers
+      subject.class.h.should be subject.class.helpers
     end
   end
 
   describe ".decorates_association" do
-    before { subject.class.decorates_association :similar_products, with: ProductDecorator }
+    let(:decorator_class) { Class.new(ProductDecorator) }
+    before { decorator_class.decorates_association :similar_products, with: ProductDecorator }
 
     describe "overridden association method" do
       let(:decorated_association) { ->{} }
@@ -151,7 +152,7 @@ describe Draper::Decorator do
   end
 
   describe ".decorates_associations" do
-    subject { Decorator }
+    subject { decorator_class }
 
     it "decorates each of the associations" do
       subject.should_receive(:decorates_association).with(:similar_products, {})
@@ -210,56 +211,11 @@ describe Draper::Decorator do
     end
   end
 
-  describe "method selection" do
-    it "echos the methods of the wrapped class" do
-      source.methods.each do |method|
-        subject.respond_to?(method.to_sym, true).should be_true
-      end
+  describe "#to_param" do
+    it "proxies to the source" do
+      source.stub(:to_param).and_return(42)
+      subject.to_param.should == 42
     end
-
-    it "not override a defined method with a source method" do
-      DecoratorWithApplicationHelper.new(source).length.should == "overridden"
-    end
-
-    it "not copy the .class, .inspect, or other existing methods" do
-      source.class.should_not == subject.class
-      source.inspect.should_not == subject.inspect
-      source.to_s.should_not == subject.to_s
-    end
-
-    context "when an ActiveModel descendant" do
-      it "always proxy to_param if it is not defined on the decorator itself" do
-        source.stub(:to_param).and_return(1)
-        Draper::Decorator.new(source).to_param.should == 1
-      end
-
-      it "always proxy id if it is not defined on the decorator itself" do
-        source.stub(:id).and_return(123456789)
-        Draper::Decorator.new(source).id.should == 123456789
-      end
-
-      it "always proxy errors if it is not defined on the decorator itself" do
-        Draper::Decorator.new(source).errors.should be_an_instance_of ActiveModel::Errors
-      end
-
-      it "never proxy to_param if it is defined on the decorator itself" do
-        source.stub(:to_param).and_return(1)
-        DecoratorWithSpecialMethods.new(source).to_param.should == "foo"
-      end
-
-      it "never proxy id if it is defined on the decorator itself" do
-        source.stub(:id).and_return(123456789)
-        DecoratorWithSpecialMethods.new(source).id.should == 1337
-      end
-
-      it "never proxy errors if it is defined on the decorator itself" do
-        DecoratorWithSpecialMethods.new(source).errors.should be_an_instance_of Array
-      end
-    end
-  end
-
-  it "wrap source methods so they still accept blocks" do
-    subject.block{"marker"}.should == "marker"
   end
 
   describe "#==" do
@@ -270,13 +226,104 @@ describe Draper::Decorator do
   end
 
   describe "#respond_to?" do
-    # respond_to? is called by some proxies (id, to_param, errors).
-    # This is, why I stub it this way.
-    it "delegates to the decorated model" do
-      other = Draper::Decorator.new(source)
-      source.stub(:respond_to?).and_return(false)
-      source.should_receive(:respond_to?).with(:whatever, true).once.and_return("mocked")
-      subject.respond_to?(:whatever, true).should == "mocked"
+    let(:decorator_class) { Class.new(ProductDecorator) }
+
+    it "returns true for its own methods" do
+      subject.should respond_to :awesome_title
+    end
+
+    it "returns true for the source's methods" do
+      subject.should respond_to :title
+    end
+
+    context "with include_private" do
+      it "returns true for its own private methods" do
+        subject.respond_to?(:awesome_private_title, true).should be_true
+      end
+
+      it "returns true for the source's private methods" do
+        subject.respond_to?(:private_title, true).should be_true
+      end
+    end
+
+    context "with method security" do
+      it "respects allows" do
+        subject.class.allows :hello_world
+
+        subject.should respond_to :hello_world
+        subject.should_not respond_to :goodnight_moon
+      end
+
+      it "respects denies" do
+        subject.class.denies :goodnight_moon
+
+        subject.should respond_to :hello_world
+        subject.should_not respond_to :goodnight_moon
+      end
+
+      it "respects denies_all" do
+        subject.class.denies_all
+
+        subject.should_not respond_to :hello_world
+        subject.should_not respond_to :goodnight_moon
+      end
+    end
+  end
+
+  describe "method proxying" do
+    let(:decorator_class) { Class.new(ProductDecorator) }
+
+    it "does not proxy methods that are defined on the decorator" do
+      subject.overridable.should be :overridden
+    end
+
+    it "does not proxy methods inherited from Object" do
+      subject.inspect.should_not be source.inspect
+    end
+
+    it "proxies missing methods that exist on the source" do
+      source.stub(:hello_world).and_return(:proxied)
+      subject.hello_world.should be :proxied
+    end
+
+    it "adds proxied methods to the decorator when they are used" do
+      subject.methods.should_not include :hello_world
+      subject.hello_world
+      subject.methods.should include :hello_world
+    end
+
+    it "passes blocks to proxied methods" do
+      subject.block{"marker"}.should == "marker"
+    end
+
+    it "does not confuse Kernel#Array" do
+      Array(subject).should be_a Array
+    end
+
+    context "with method security" do
+      it "respects allows" do
+        source.stub(:hello_world, :goodnight_moon).and_return(:proxied)
+        subject.class.allows :hello_world
+
+        subject.hello_world.should be :proxied
+        expect{subject.goodnight_moon}.to raise_error NameError
+      end
+
+      it "respects denies" do
+        source.stub(:hello_world, :goodnight_moon).and_return(:proxied)
+        subject.class.denies :goodnight_moon
+
+        subject.hello_world.should be :proxied
+        expect{subject.goodnight_moon}.to raise_error NameError
+      end
+
+      it "respects denies_all" do
+        source.stub(:hello_world, :goodnight_moon).and_return(:proxied)
+        subject.class.denies_all
+
+        expect{subject.hello_world}.to raise_error NameError
+        expect{subject.goodnight_moon}.to raise_error NameError
+      end
     end
   end
 
@@ -302,64 +349,26 @@ describe Draper::Decorator do
   end
 
   context "in a Rails application" do
-    let(:decorator){ DecoratorWithApplicationHelper.decorate(Object.new) }
+    let(:decorator_class) { DecoratorWithApplicationHelper }
 
     it "has access to ApplicationHelper helpers" do
-      decorator.uses_hello_world.should == "Hello, World!"
+      subject.uses_hello_world.should == "Hello, World!"
     end
 
     it "is able to use the content_tag helper" do
-      decorator.sample_content.to_s.should == "<span>Hello, World!</span>"
+      subject.sample_content.to_s.should == "<span>Hello, World!</span>"
     end
 
     it "is able to use the link_to helper" do
-      decorator.sample_link.should == "<a href=\"/World\">Hello</a>"
+      subject.sample_link.should == "<a href=\"/World\">Hello</a>"
     end
 
-    it "is able to use the pluralize helper" do
-      decorator.sample_truncate.should == "Once..."
+    it "is able to use the truncate helper" do
+      subject.sample_truncate.should == "Once..."
     end
 
     it "is able to access html_escape, a private method" do
-      decorator.sample_html_escaped_text.should == '&lt;script&gt;danger&lt;/script&gt;'
-    end
-  end
-
-  describe "#method_missing" do
-    context "with an isolated decorator class" do
-      let(:decorator_class) { Class.new(Decorator) }
-      subject{ decorator_class.new(source) }
-
-      context "when #hello_world is called again" do
-        it "proxies method directly after first hit" do
-          subject.methods.should_not include(:hello_world)
-          subject.hello_world
-          subject.methods.should include(:hello_world)
-        end
-      end
-
-      context "when #hello_world is called for the first time" do
-        it "hits method missing" do
-          subject.should_receive(:method_missing)
-          subject.hello_world
-        end
-      end
-    end
-
-    context "when the delegated method calls a non-existant method" do
-      it "should not try to delegate to non-existant methods to not confuse Kernel#Array" do
-        Array(subject).should be_kind_of(Array)
-      end
-
-      it "raises the correct NoMethodError" do
-        begin
-          subject.some_action
-        rescue NoMethodError => e
-          e.name.should_not == :some_action
-        else
-          fail("No exception raised")
-        end
-      end
+      subject.sample_html_escaped_text.should == '&lt;script&gt;danger&lt;/script&gt;'
     end
   end
 
@@ -390,24 +399,28 @@ describe Draper::Decorator do
       end
     end
 
-    context "with for: symbol" do
-      it "sets the finder class" do
-        FinderDecorator.has_finders for: :product
-        FinderDecorator.finder_class.should be Product
-      end
-    end
+    context "with :for option" do
+      subject { Class.new(Draper::Decorator) }
 
-    context "with for: string" do
-      it "sets the finder class" do
-        FinderDecorator.has_finders for: "some_thing"
-        FinderDecorator.finder_class.should be SomeThing
+      context "with a symbol" do
+        it "sets the finder class" do
+          subject.has_finders for: :product
+          subject.finder_class.should be Product
+        end
       end
-    end
 
-    context "with for: class" do
-      it "sets the finder_class" do
-        FinderDecorator.has_finders for: Namespace::Product
-        FinderDecorator.finder_class.should be Namespace::Product
+      context "with a string" do
+        it "sets the finder class" do
+          subject.has_finders for: "some_thing"
+          subject.finder_class.should be SomeThing
+        end
+      end
+
+      context "with a class" do
+        it "sets the finder_class" do
+          subject.has_finders for: Namespace::Product
+          subject.finder_class.should be Namespace::Product
+        end
       end
     end
   end

spec/spec_helper.rb

@@ -3,49 +3,29 @@ require 'ammeter/init'
 require 'rails'
 
 require 'action_view'
-Bundler.require
-
-require './spec/support/samples/active_record'
-require './spec/support/samples/decorator'
-require './spec/support/samples/decorator_with_application_helper'
-require './spec/support/samples/decorator_with_special_methods'
-require './spec/support/samples/finder_decorator'
-require './spec/support/samples/namespaced_product'
-require './spec/support/samples/namespaced_product_decorator'
-require './spec/support/samples/non_active_model_product'
-require './spec/support/samples/non_active_model_product_decorator'
-require './spec/support/samples/product'
-require './spec/support/samples/product_decorator'
-require './spec/support/samples/products_decorator'
-require './spec/support/samples/sequel_product'
-require './spec/support/samples/specific_product_decorator'
-require './spec/support/samples/some_thing'
-require './spec/support/samples/some_thing_decorator'
-require './spec/support/samples/uninferrable_decorator_model'
-require './spec/support/samples/widget'
-require './spec/support/samples/widget_decorator'
-
 require 'action_controller'
 require 'action_controller/test_case'
 
-module ActionController
-  class Base
-    Draper::System.setup_action_controller(self)
-  end
-end
+Bundler.require
 
-module ActiveRecord
-  class Relation
-  end
-end
+require './spec/support/active_record'
+require './spec/support/action_controller'
 
-class ApplicationController < ActionController::Base
-  def hello_world
-    "Hello, World!"
-  end
-  helper_method :hello_world
-end
+require './spec/support/models/product'
+require './spec/support/models/namespaced_product'
+require './spec/support/models/non_active_model_product'
+require './spec/support/models/widget'
+require './spec/support/models/some_thing'
+require './spec/support/models/uninferrable_decorator_model'
 
+require './spec/support/decorators/product_decorator'
+require './spec/support/decorators/namespaced_product_decorator'
+require './spec/support/decorators/non_active_model_product_decorator'
+require './spec/support/decorators/widget_decorator'
+require './spec/support/decorators/specific_product_decorator'
+require './spec/support/decorators/products_decorator'
+require './spec/support/decorators/some_thing_decorator'
+require './spec/support/decorators/decorator_with_application_helper'
 
 class << Rails
   undef application # Avoid silly Rails bug: https://github.com/rails/rails/pull/6429

spec/support/action_controller.rb

@@ -0,0 +1,12 @@
+module ActionController
+  class Base
+    Draper::System.setup_action_controller(self)
+  end
+end
+
+class ApplicationController < ActionController::Base
+  def hello_world
+    "Hello, World!"
+  end
+  helper_method :hello_world
+end

spec/support/active_record.rb

@@ -15,3 +15,8 @@ module ActiveRecord
 
   end
 end
+
+module ActiveRecord
+  class Relation
+  end
+end

spec/support/decorators/namespaced_product_decorator.rb

@@ -1,5 +1,3 @@
-require './spec/support/samples/namespaced_product'
-
 module Namespace
   class ProductDecorator < Draper::Decorator
     has_finders

spec/support/decorators/product_decorator.rb

@@ -5,6 +5,15 @@ class ProductDecorator < Draper::Decorator
     "Awesome Title"
   end
 
+  def overridable
+    :overridden
+  end
+
   def self.my_class_method
   end
+
+  private
+
+  def awesome_private_title
+  end
 end

spec/support/decorators/specific_product_decorator.rb

@@ -1,4 +1,2 @@
-require './spec/support/samples/product_decorator'
-
 class SpecificProductDecorator < ProductDecorator
 end

spec/support/models/namespaced_product.rb

@@ -1,5 +1,3 @@
-require './spec/support/samples/product'
-
 module Namespace
   class Product < ActiveRecord::Base
     include Draper::Decoratable

spec/support/models/product.rb

@@ -77,4 +77,12 @@ class Product < ActiveRecord::Base
     Product.new
   end
 
+  def overridable
+    :overridable
+  end
+
+  private
+
+  def private_title
+  end
 end

spec/support/samples/decorator.rb

@@ -1,5 +0,0 @@
-class Decorator < Draper::Decorator
-  def self.own_class_method
-    "own class method"
-  end
-end

spec/support/samples/decorator_with_special_methods.rb

@@ -1,13 +0,0 @@
-class DecoratorWithSpecialMethods < Draper::Decorator
-  def to_param
-    "foo"
-  end
-
-  def id
-    1337
-  end
-
-  def errors
-    ["omg errors!"]
-  end
-end

spec/support/samples/finder_decorator.rb

@@ -1,2 +0,0 @@
-class FinderDecorator < Draper::Decorator
-end

spec/support/samples/sequel_product.rb

@@ -1,13 +0,0 @@
-module Sequel
- class Model
-   def each
-   end
- end
-end
-
-class SequelProduct < Sequel::Model
-  def some_attribute
-    "hello"
-  end
-end
-