Merge pull request activeadmin#2313 from Daxter/bugfix/namespace-pollution

seanlinsley · seanlinsley · commit 7f92ec353f0d · 2013-07-02T20:54:49.000-07:00
Bugfix: apps no longer use the same namespace hash
diff --git a/lib/active_admin/application.rb b/lib/active_admin/application.rb
@@ -4,18 +4,9 @@
 module ActiveAdmin
   class Application
     include Settings
+    include Settings::Inheritance
 
-    # Adds settings to both the Application and the Namespace instance
-    # so that they can be configured independantly.
-    def self.inheritable_setting(name, default)
-      Namespace.setting name, nil
-      setting name, default
-    end
-
-    def self.deprecated_inheritable_setting(name, default)
-      Namespace.deprecated_setting name, nil
-      deprecated_setting name, default
-    end
+    settings_inherited_by Namespace
 
     # The default namespace to put controllers and routes inside. Set this
     # in config/initializers/active_admin.rb using:
@@ -24,8 +15,10 @@ def self.deprecated_inheritable_setting(name, default)
     #
     setting :default_namespace, :admin
 
-    # A hash of all the registered namespaces
-    setting :namespaces, {}
+    attr_reader :namespaces
+    def initialize
+      @namespaces = {}
+    end
 
     # Load paths for admin configurations. Add folders to this load path
     # to load up other resources for administration. External gems can
diff --git a/lib/active_admin/deprecation.rb b/lib/active_admin/deprecation.rb
@@ -1,6 +1,6 @@
 module ActiveAdmin
   module Deprecation
-    extend self
+    module_function
 
     def warn(message, callstack = caller)
       ActiveSupport::Deprecation.warn "Active Admin: #{message}", callstack
@@ -23,8 +23,8 @@ def warn(message, callstack = caller)
     #     end
     #
     def deprecate(klass, method, message)
-      klass.class_eval <<-EOC, __FILE__, __LINE__
-        alias_method :"deprecated_#{method}", :#{method}
+      klass.class_eval <<-EOC, __FILE__, __LINE__ + 1
+        alias_method :deprecated_#{method}, :#{method}
         def #{method}(*args)
           ActiveAdmin::Deprecation.warn('#{message}', caller)
           send(:deprecated_#{method}, *args)
diff --git a/lib/active_admin/helpers/settings.rb b/lib/active_admin/helpers/settings.rb
@@ -1,5 +1,3 @@
-require 'active_support/concern'
-
 module ActiveAdmin
 
   # Adds a class method to a class to create settings with default values.
@@ -18,7 +16,10 @@ module ActiveAdmin
   #   conf.site_title #=> "Override Default"
   #
   module Settings
-    extend ActiveSupport::Concern
+
+    def self.included(base)
+      base.extend ClassMethods
+    end
 
     def read_default_setting(name)
       default_settings[name]
@@ -34,10 +35,9 @@ module ClassMethods
 
       def setting(name, default)
         default_settings[name] = default
-        attr_accessor(name)
+        attr_writer name
 
-        # Create an accessor that grabs from the defaults
-        # if @name has not been set yet
+        # Creates a reader that will grab the default if no value has been set.
         class_eval <<-EOC, __FILE__, __LINE__ + 1
           def #{name}
             if instance_variable_defined? :@#{name}
@@ -50,10 +50,10 @@ def #{name}
       end
 
       def deprecated_setting(name, default, message = nil)
-        message = message || "The #{name} setting is deprecated and will be removed."
         setting(name, default)
 
-        ActiveAdmin::Deprecation.deprecate self, name, message
+        message ||= "The #{name} setting is deprecated and will be removed."
+        ActiveAdmin::Deprecation.deprecate self,     name,    message
         ActiveAdmin::Deprecation.deprecate self, :"#{name}=", message
       end
 
@@ -62,5 +62,47 @@ def default_settings
       end
 
     end
+
+
+    # Allows you to define child classes that should receive the same
+    # settings, as well as the same default values.
+    #
+    # Example from the codebase:
+    #
+    #   class Application
+    #     include Settings
+    #     include Settings::Inheritance
+    #
+    #     settings_inherited_by :Namespace
+    #
+    #     inheritable_setting :root_to, 'dashboard#index'
+    #   end
+    #
+    module Inheritance
+
+      def self.included(base)
+        base.extend ClassMethods
+      end
+
+      module ClassMethods
+
+        def settings_inherited_by(heir)
+          (@setting_heirs ||= []) << heir
+          heir.send :include, ActiveAdmin::Settings
+        end
+
+        def inheritable_setting(name, default)
+          setting name, default
+          @setting_heirs.each{ |c| c.setting name, default }
+        end
+
+        def deprecated_inheritable_setting(name, default)
+          deprecated_setting name, default
+          @setting_heirs.each{ |c| c.deprecated_setting name, default }
+        end
+
+      end
+    end
+
   end
 end
diff --git a/lib/active_admin/namespace.rb b/lib/active_admin/namespace.rb
@@ -1,4 +1,3 @@
-require 'active_admin/helpers/settings'
 require 'active_admin/resource_collection'
 
 module ActiveAdmin
@@ -26,8 +25,6 @@ module ActiveAdmin
   # resource will be accessible from "/posts" and the controller will be PostsController.
   #
   class Namespace
-    include Settings
-
     RegisterEvent = 'active_admin.namespace.register'.freeze
 
     attr_reader :application, :resources, :name, :menus
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
@@ -110,6 +110,13 @@
         end
       }.to raise_error("found")
     end
+
+    it "should not pollute the global app" do
+      application.namespaces.keys.should be_empty
+      application.namespace(:brand_new_ns)
+      application.namespaces.keys.should eq [:brand_new_ns]
+      ActiveAdmin.application.namespaces.keys.should eq [:admin]
+    end
   end
 
   describe "#register_page" do
diff --git a/spec/unit/authorization/controller_authorization_spec.rb b/spec/unit/authorization/controller_authorization_spec.rb
@@ -24,14 +24,9 @@
   end
 
   it "should authorize the create action with the new resource" do
-    pending "This test redirects to / for some reason, when it should redirect to /admin/posts"
-
-    mock_post = mock("Post", :save => true, :errors => [])
-    Post.should_receive(:new).at_least(:once).and_return(mock_post)
-
-    authorization.should_receive(:authorized?).with(Auth::CREATE, mock_post).and_return true
+    authorization.should_receive(:authorized?).with(Auth::CREATE, an_instance_of(Post)).and_return true
     post :create
-    response.should redirect_to action: 'index'
+    response.should redirect_to action: 'show', id: Post.last.id
   end
 
   it "should redirect when the user isn't authorized" do
diff --git a/spec/unit/namespace/authorization_spec.rb b/spec/unit/namespace/authorization_spec.rb
@@ -9,23 +9,19 @@
   describe "authorization_adapter" do
 
     it "should return AuthorizationAdapter by default" do
-      namespace.authorization_adapter.should == ActiveAdmin::AuthorizationAdapter
+      app.authorization_adapter.should       eq ActiveAdmin::AuthorizationAdapter
+      namespace.authorization_adapter.should eq ActiveAdmin::AuthorizationAdapter
     end
 
     it "should be settable on the namespace" do
-      namespace.authorization_adapter.should == ActiveAdmin::AuthorizationAdapter
       namespace.authorization_adapter = mock_auth
-
-      namespace.authorization_adapter.should == mock_auth
+      namespace.authorization_adapter.should eq mock_auth
     end
 
     it "should be settable on the application" do
-      namespace.authorization_adapter.should == ActiveAdmin::AuthorizationAdapter
       app.authorization_adapter = mock_auth
-
-      namespace.authorization_adapter.should == mock_auth
+      app.authorization_adapter.should eq mock_auth
     end
 
   end
-
 end
diff --git a/spec/unit/resource/routes_spec.rb b/spec/unit/resource/routes_spec.rb
@@ -39,7 +39,7 @@ class ::News; def self.has_many(*); end end
         let!(:config) { ActiveAdmin.register News }
         before{ reload_routes! }
 
-        it "should return the plurali route with _index" do
+        it "should return the plural route with _index" do
           config.route_collection_path.should == "/admin/news"
         end
       end
diff --git a/spec/unit/resource_registration_spec.rb b/spec/unit/resource_registration_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe "Registering an object to administer" do
-  let(:application){ ActiveAdmin::Application.new }
+  application = ActiveAdmin::Application.new
 
   context "with no configuration" do
-    let(:namespace) { ActiveAdmin::Namespace.new(application, :admin) }
+    namespace = ActiveAdmin::Namespace.new(application, :admin)
     it "should call register on the namespace" do
       application.namespaces[namespace.name] = namespace
       namespace.should_receive(:register)
diff --git a/spec/unit/routing_spec.rb b/spec/unit/routing_spec.rb
@@ -9,8 +9,8 @@
     reload_routes!
   end
 
-  pending "should only have the (default) admin namespace registered" do
-    ActiveAdmin.application.namespaces.keys.should eq [:admin]
+  it "should only have the namespaces necessary for route testing" do
+    ActiveAdmin.application.namespaces.keys.should eq [:admin, :root]
   end
 
   it "should route to the admin dashboard" do
diff --git a/spec/unit/settings_spec.rb b/spec/unit/settings_spec.rb
@@ -0,0 +1,88 @@
+require 'spec_helper'
+
+describe ActiveAdmin::Settings do
+
+  subject{ Class.new{ include ActiveAdmin::Settings } }
+
+  it{ should respond_to :setting }
+  it{ should respond_to :deprecated_setting }
+  it{ should respond_to :default_settings }
+
+  describe "class API" do
+    it "should create settings" do
+      subject.setting :foo, 'bar'
+      subject.default_settings[:foo].should eq 'bar'
+    end
+
+    it "should create deprecated settings" do
+      ActiveAdmin::Deprecation.should_receive(:deprecate).twice
+      subject.deprecated_setting :baz, 32
+      subject.default_settings[:baz].should eq 32
+    end
+  end
+
+  describe "instance API" do
+
+    before do
+      subject.setting :foo, 'bar'
+      subject.deprecated_setting :baz, 32
+    end
+    let(:instance) { subject.new }
+
+    it "should have access to a default value" do
+      instance.foo.should eq 'bar'
+      instance.foo = 'qqq'
+      instance.foo.should eq 'qqq'
+    end
+
+    it "should have access to a deprecated value" do
+      ActiveAdmin::Deprecation.should_receive(:warn).exactly(3).times
+      instance.baz.should eq 32
+      instance.baz = [45]
+      instance.baz.should eq [45]
+    end
+  end
+
+end
+
+
+describe ActiveAdmin::Settings::Inheritance do
+
+  subject do
+    Class.new do
+      include ActiveAdmin::Settings
+      include ActiveAdmin::Settings::Inheritance
+    end
+  end
+
+  it{ should respond_to :settings_inherited_by }
+  it{ should respond_to :inheritable_setting }
+  it{ should respond_to :deprecated_inheritable_setting }
+
+  let(:heir) { Class.new }
+
+  before do
+    subject.settings_inherited_by heir
+  end
+
+  describe "class API" do
+    it "should add setting to an heir" do
+      subject.inheritable_setting :one, 2
+      heir.default_settings[:one].should eq 2
+    end
+
+    it "should add deprecated setting to an heir" do
+      ActiveAdmin::Deprecation.should_receive(:deprecate).exactly(4).times
+      subject.deprecated_inheritable_setting :three, 4
+      heir.default_settings[:three].should eq 4
+    end
+  end
+
+  describe "instance API" do
+    before{ subject.inheritable_setting :left, :right }
+    it "should work" do
+      heir.new.left.should eq :right
+    end
+  end
+
+end
