Inherit from Admin::ApplicationController, instead of Main App ApplicationController #1935

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants

Continuation from the original pull request - gregbell#1934

Tests should now be passing, BaseController now inherits from Admin::ApplicationController, and admin/application_controller.rb added to install generator.

Owner

seanlinsley commented Feb 22, 2013

You know you could have just as easily overwritten the commits from the previous PR?

To be consistent, I think we should put all the resource controllers under the Admin:: namespace instead of dynamically changing it based on the URL namespace.

I did not know that, haha. So you're talking about removing the option to dynamically namespace all together?

It's been a habit for a long time. Most notably useful in situations where the parent module may not have been defined previously.

Owner

seanlinsley commented Feb 22, 2013

Yeah, unless there's a reason why it's there in the first place, it seems unnecessarily complicated.

One general feature that devs might appreciate is being able to choose the global namespace that Active Admin instantiates. I recall an Issue where people couldn't run AA because Admin:: was already being used.

This was the original reasoning behind using ActiveAdmin originally. I think that the general convention would be to use ActiveAdmin namespace, as it both matches the naming convention of the engine in use, and is explicit as to what that code is for. I could change the code to use ActiveAdmin:: instead. Thoughts?

Owner

seanlinsley commented Feb 22, 2013

I went googling and found this crazy weird ruby meta-programming example:

require "alice_bad_code"
class AliceBadCode
  ProgressBar = ::ProgressBar
end
Object.send(:remove_const, "ProgressBar")
Owner

seanlinsley commented Feb 22, 2013

A potential use case of dynamic namespacing is if... you want multiple AA instances running?

The problem is the master ApplicationController...

Yeah, I'm not sure what the best option is. I'll let it steep a while. Also, maybe @gregbell can chime in?

Sounds good, thanks for the feedback.

On another note, I'm not sure why the tests failed in this instance. Everything is passing on my end.

Owner

seanlinsley commented Feb 22, 2013

The REE and 1.9.2 tests aren't your fault. I opened an issue for that: #1927

Start reading here to see the tests are likely are your fault ;]

Owner

seanlinsley commented Feb 27, 2013

So yeah, it seems like we should go the mountable engine approach with:

controllers/admin/application_controller.rb # defining Admin::ApplicationController
controllers/admin/* # holding optional additions, e.g. Admin::UsersController

If people want to run multiple instances of AA in the same app, we can implement that when we get there.

Owner

seanlinsley commented Mar 15, 2013

Our Travis builds on master are now green (as of 166bc46). Please rebase your PR on the current master:

# Expectations: "upstream" is gregbell's GitHub repo and "origin" is your fork

# Rebase your fork's master branch with the latest upstream changes
git checkout master
git pull --rebase upstream master
git push origin master

# Rebase your feature branch with the latest upstream changes
git checkout your_feature_branch
git pull --rebase upstream master
git push origin your_feature_branch # note that you may need to use -f

@daxter I'm not sure why this failed. It says it's failing regarding devise routes, and I cannot seem to locate why. Would you mind taking a look and seeing if you can spot it? Thanks.

Owner

seanlinsley commented Mar 19, 2013

Sure, I'll take a look. Just to confirm, do the tests pass when you run them on your machine?

Yes, they pass on my end.

Owner

seanlinsley commented Mar 19, 2013

I'm getting the same failures locally.

Don't forget to delete the rails environment that AA generates, since your PR modifies what's created.

rm -rf spec/rails/*

Note that you can fully utilize your CPU to run the tests faster:

parallel_rspec spec/
parallel_cucumber features/
Owner

seanlinsley commented Mar 20, 2013

By the way, I just merged in more fixes for our test suite: c200735. You should probably rebase once more, and clean out your test environment like so:

rm Gemfile.lock .rails-version
bundle
rm -rf spec/rails/
rspec spec
Owner

seanlinsley commented Apr 16, 2013

Pinging @gitt :]

Yeah, sorry about that. Picked up a new job, and have just been bust acclimating. I will give this some love this weekend for sure.

Owner

seanlinsley commented Apr 16, 2013

No worries, and glad to hear it. This is open source, so improvements move at the speed of convenience.

hmm, now its passing, but only for the latest rails.

Owner

seanlinsley commented Apr 20, 2013

Specifically:

uninitialized constant Admin::DashboardController (ActionController::RoutingError)

(eval):2:in `click_button'
./features/step_definitions/user_steps.rb:28:in `/^I am logged in with capybara$/'
features/development_reloading.feature:13:in `And I am logged in with capybara'

cucumber -p class-reloading features/development_reloading.feature:8 # Scenario: Reloading an updated model that a resource points to

@daxter This is caused by the Admin namespace. I switched it to ActiveAdmin and the error is now gone. I know we discussed which namespace to use earlier in this PR. Is there anything wrong with app/controller/active_admin/application_controller.rb?

Further discussion about having things in app/controller/admin and app/views/admin here

Owner

seanlinsley commented Apr 21, 2013

Could you elaborate on how exactly the Admin namespace caused the error?

Owner

seanlinsley commented Apr 21, 2013

We currently have this scheme:

app/admin/*
app/assets/javascripts/active_admin.js
app/assets/stylesheets/active_admin.css.scss
app/controller/application_controller.rb

It almost seems like AA should have its own folder.

app/admin/pages/*
app/admin/assets/javascripts/application.js
app/admin/assets/stylesheets/application.css.scss
app/admin/controllers/application_controller.rb

I know when I first started using AA, it was really confusing that it threw its files inside the folders that I thought were only for my application.

Maybe it would be reasonable to go one more step:

admin/pages/*
admin/assets/javascripts/application.js
admin/assets/stylesheets/application.css.scss
admin/controllers/application_controller.rb

And of course either of the two options above could use active_admin instead.

I'm not sure I entirely understand it myself. However, based on the error, it was trying to load Admin::DashboardController. I couldn't find any reference to this in the code anywhere, so I started looking through issues for a similar error. I found it in two locations. Once is in side the README.md file, and the other was the issue I linked in my last comment. Taking a leap, I changed the namespace, ran the tests, and to my surprise they passed.

I would think moving the admin namespace up would go against standard conventions. I would really think the clearest thing you could do is change the namespace to be active_admin, to match the gem name. This removes any confusion on what the namespace is for, and what belongs within it. That being said, I could be completely oblivious as to the reasoning behind now using that particular namespace to begin with.

Owner

seanlinsley commented Apr 21, 2013

Oh, well so you're aware, AA automatically namespaces the controllers based off of the URL namespace you use.

# (default)
Admin::UsersController
# false
UsersController
# :foo_bar
FooBar::UsersController

I agree that active_admin is more clear, but as a typical rubyist I'm trying to avoid those extra characters.

On the subject of namespaces, something that AA really hasn't dealt with is: if my AA app has multiple namespaces, how the crap do I manage everything?

Even now, I'm not sure what the answer is. Because you would have different controllers and different AA pages, and perhaps even different CSS & JS.

Owner

seanlinsley commented Apr 21, 2013

@gregbell, @macfanatic thoughts on this?

@seanlinsley seanlinsley referenced this pull request Apr 22, 2013

Merged

Security Fixes #1926

Owner

seanlinsley commented Apr 22, 2013

It seems to me that this would be best merged in as part of 0.6.2. I'm going to set that as the milestone, but feel free to chime in @gregbell & @macfanatic.

Owner

seanlinsley commented Apr 22, 2013

So @gitt I have a question for you. What happens for unsuspecting current users that upgrade? Do they get an unhelpful uninitialized constant error? Maybe if that constant hasn't been defined, we should do this:

unless defined? ::ActiveAdmin::ApplicationController
  raise "helpful message here"
end

Which raises another question; can you just re-run the active_admin:install rake task to have it build the file?

@daxter I think that's probably a good idea. I'm not sure on the re-run. I would think yeah, the rest it would ask if you want to keep/replace any existing files.

Owner

seanlinsley commented May 2, 2013

To explain this a little bit better:

UsersController.superclass                 # ActiveAdmin::ResourceController
ActiveAdmin::ResourceController.superclass # ActiveAdmin::BaseController
ActiveAdmin::BaseController.superclass     # InheritedResources::Base
InheritedResources::Base.superclass        # ApplicationController

After looking through the IR source, I still have no idea where the ApplicationController reference comes from.

Also, from the Devise source:

module Devise
  # The parent controller all Devise controllers inherits from.
  # Defaults to ApplicationController. This should be set early
  # in the initialization process and should be set to a string.
  mattr_accessor :parent_controller
  @@parent_controller = "ApplicationController"
end

We should probably be setting this to Active Admin's base controller. We're not currently:

ActiveAdmin::Devise::SessionsController.superclass # Devise::SessionsController
Devise::SessionsController.superclass              # DeviseController
DeviseController.superclass                        # ApplicationController
Owner

seanlinsley commented May 2, 2013

This is somewhat related to #2126. Thoughts on the direction of this PR, @jherdman?

Contributor

jherdman commented May 2, 2013

Sort of related, but I'm very much in favour of this. I'm having to look into engines for #2126 anyways, something I'll be submitting a separate pull request for later tonight.

Owner

seanlinsley commented May 3, 2013

Ah... this PR won't work for existing AA installs.

$> rails g active_admin:install
lib/active_admin/base_controller/authorization.rb:21:in `<module:ActiveAdmin>': uninitialized constant ActiveAdmin::ApplicationController (NameError)
    from lib/active_admin/base_controller/authorization.rb:1:in `<top (required)>'
    from lib/active_admin/base_controller.rb:3:in `<top (required)>'
    from lib/active_admin/resource_controller/actions.rb:2:in `<module:ActiveAdmin>'
    from lib/active_admin/resource_controller/actions.rb:1:in `<top (required)>'
    from lib/active_admin/resource_controller.rb:2:in `<top (required)>'
    from lib/active_admin/batch_actions.rb:7:in `block in <top (required)>'
    from lib/active_admin/event.rb:25:in `call'
    from lib/active_admin/event.rb:25:in `block in dispatch'
    from lib/active_admin/event.rb:24:in `each'
    from lib/active_admin/event.rb:24:in `dispatch'
    from lib/active_admin/application.rb:174:in `load!'
    from lib/active_admin/application.rb:193:in `routes'
    from lib/active_admin.rb:84:in `routes'

codezomb commented May 4, 2013

hmm, I might have some time to try a fix this weekend.

Owner

seanlinsley commented Oct 4, 2013

I'm going to close this PR since it hasn't been worked on in so long. Please let me know if you'd like to start again.

@seanlinsley seanlinsley closed this Oct 4, 2013

Contributor

dmitry commented Mar 5, 2014

Just to be clear - there are no way for now, to inherit from the other controller, it always inherits from the ApplicationController?

Owner

seanlinsley commented Mar 5, 2014

Unfortunately, that's correct @dmitry :-(

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