New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inherit from ActiveAdmin::ApplicationController, instead of Main App ApplicationController #1934

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@codezomb
Copy link

codezomb commented Feb 21, 2013

This allows users to use active_admin without worrying about what's in their existing application_controller.rb. This allows the front end and the back end to remain segregated. This also solves a problem with cancan interfering with admin resources if you don't want it to.

This can also be overridden in the main app by creating an controllers/acitve_admin/application_controller.rb.

@seanlinsley

This comment has been minimized.

Copy link
Member

seanlinsley commented Feb 22, 2013

So problems from first glance:

  • this broke tests. You'll need to run those down.
  • you need to add a newline to the application_controller.rb file
  • you should add to the existing documentation, explaining how this can be used

A style issue is that if I want to customize the application controller for AA, this means I have to create a whole new folder in my controllers directory... for one file. Even more, I won't be able to put my FoosController in that folder because the namespace won't be the same. By default, it's Admin::FoosController but depending on my namespace it could be Anything::FoosController.

It seems like the best solution would be to have a app/controllers/admin folder just like we have app/admin, which itself ignores the normal Rails folder namespacing. Any admin-related controller additions could be optionally added to this folder, so they don't inherently have to be in app/admin files.

Moreover, instead of embedding the ActiveAdmin::ApplicationController into the AA source, it should be added to the rails generator tasks so that app/controllers/admin/application_controller.rb is auto-created.

We might as well discuss the whole extent of this problem now, because no matter what we do this is going to break some existing AA installations because people (including myself) expected ApplicationController to be included in AA.

@codezomb

This comment has been minimized.

Copy link

codezomb commented Feb 22, 2013

Indeed, I agree w/ the above. I threw this together quickly for my own project, and should have waited a bit longer. I'll take a look at the above, and see what I can throw together this weekend. Thanks for the feedback.

@seanlinsley

This comment has been minimized.

Copy link
Member

seanlinsley commented Feb 22, 2013

That's what's great about pull requests; getting feedback early :]

@seanlinsley

This comment has been minimized.

Copy link
Member

seanlinsley commented Feb 22, 2013

An issue with my idea is the AA load order. Anything in app/controllers/admin would have to be loaded first...

Which is actually the exact opposite of what we want e.g. if a dev adds FoosController to that folder. Those changes should be loaded last so you can override methods.

The other issue is class naming inside of the app/controllers/admin folder

class ActiveAdmin::ApplicationController # makes sense, but
class Admin::FoosController              # what about this?
class Anything::FoosController           # ...or this?
@codezomb

This comment has been minimized.

Copy link

codezomb commented Feb 22, 2013

This is definitely a tricky issue. Especially taking into consideration the dynamic nature of the namespace feature. My main hangup is BaseController needing a predefined namespace and class to inherit from. One possible solution here, is to change the class definition to something like the following:

BaseController = Class.new <namespace>::ApplicationController do
end

This would allow an argument to be passed to the class instantiation, though I feel like this would be a bit much, and quite possibly very error prone. Using the existing default namespace (admin or active_admin) would most likely be a better solution.

Another issue I ran into, and this is more of my lack of experience with rspec, but the generators don't run until after BaseController testing is executed. After adding the application_controller.rb to the generator caused the tests to fail. I can fix this, by generating a static active_admin/application_controller.rb, but that's duplication that doesn't ned to exist.

@seanlinsley

This comment has been minimized.

Copy link
Member

seanlinsley commented Feb 22, 2013

Being the usual pedant, your example wouldn't work unless you did this:

BaseController = Class.new "#{namespace}::ApplicationController".constantize do
end

I'm not sure what utility we gain from having/keeping dynamic namespaces... @gregbell, your thoughts?

I personally would like to see everything either under the Admin or ActiveAdmin namespace.

On the rspec issue, should we really be running any tests before the test app is generated? I suggest that be changed. @gitt, do you by chance know what part of the code is setting this order?

added admin/application_controller.rb to the install generator, chang…
…ed ActiveAdmin::BaseController to inherit from Admin::ApplicationController
@codezomb

This comment has been minimized.

Copy link

codezomb commented Feb 22, 2013

Right, I didn't like the proposed dynamic name spacing. I just thought it was somewhat of a requirement, if we didn't use the root application_controller via inherited resources. I've got code working well under the Admin namespace, I was just waiting to take another stab at fixing the tests.

I actually took some time this morning to take a look at this, and it was caused by the point in which the generator was run. Tests weren't actually running yet, however Admin::ApplicationController was undefined. I moved the install command up a few lines, and the tests are now running and passing.

I'll submit another request in just a few minutes. I stuck w/ admin as the namespace, since it's the default namespace of ActiveAdmin on the main app side. This made more sense to me than using ActiveAdmin, though this is just depends on how you look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment