Skip to content

Loading…

Signing out redirects incorrectly when activeadmin route is scoped #1791

Merged
merged 1 commit into from

4 participants

@quark-zju

If active admin route is scoped, like

scope :path => '/foo' do
  ActiveAdmin.routes(self)
end

Then devise sign out action will redirect to '/admin', where it should be '/foo/admin'.

This is because in lib/active_admin/devise.rb, there are some hardcoded urls:

def root_path
  if ActiveAdmin.application.default_namespace
    "/#{ActiveAdmin.application.default_namespace}"
  else
    "/"
  end
end

It may be fixed by change these lines to

def root_path
  root_path_method = [ActiveAdmin.application.default_namespace, :root_path].join('_')
  respond_to?(root_path_method) ? send(root_path_method) : '/'
end

I am somehow busy these days. I will write test for this and commit fix later.

@pcreux

Awesome! Thanks!

@teaforthecat

bump. Thanks for the fix.
This is better solution than the one that exists which requires you set relative_url_root. (which then mangles the assets.prefix)

class Application < Rails::Application
  config.relative_url_root = "scoped_uri" # for activeadmin/devise redirect
@seanlinsley seanlinsley was assigned
@seanlinsley
Active Admin member

I get a stack level too deep error with your example on a root namespace AA application after this update:

# default_namespace is false if AA is using root namespace, so reject any blank entries
[ActiveAdmin.application.default_namespace, :root_path].reject(&:blank?).join('_')

Note that the method has changed a bit as of 977a511. I'm not sure what relative_url_root is even for, so...

      def root_path
        (Rails.configuration.action_controller[:relative_url_root] || '') +
        if ActiveAdmin.application.default_namespace
          "/#{ActiveAdmin.application.default_namespace}"
        else
          "/"
        end
      end
    end
@teaforthecat

@Daxter That's really weird. It seams like the stack level error would be coming from somewhere else.

  • And just to be clear I was recommending @quark-zju method over the relative_url_root config option.
@teaforthecat

@quark-zju you should make a pull request.

@quark-zju
@seanlinsley
Active Admin member

@teaforthecat well I'm not sure where it's coming from. Given my patch above, this works. But somewhere after root_path is called, something gets stuck in a loop

@quark-zju

@teaforthecat config.relative_url_root you suggested is applied to the whole rails app, not the same thing here.

It seems relative_url_root is deprecated in rails 3, according to http://stackoverflow.com/questions/3181746/what-is-the-replacement-for-actioncontrollerbase-relative-url-root. And I am unable to make relative_url_root work with a blank rails 3.2.12 app on my laptop. If rails drops the support, I think aa can drop it as well.

@quark-zju

@Daxter your stack level too deep error may be caused by ActiveAdmin.application.default_namespace is nil. I was assuming ActiveAdmin.application.default_namespace to be admin (default value) or some non-blank string. I will consider this case in the fix.

@quark-zju

Code attached. With mocha (0.12.7), rake test passed in my laptop. Travis-CI uses mocha (0.13.3) which seems to be the reason test breaks. mocha is required by bourne, which is in turn required by shoulda-matchers. After explicitly specifing shoulda-matchers to 1.5.0, all tests passed.

@seanlinsley seanlinsley commented on an outdated diff
lib/active_admin/devise.rb
@@ -35,12 +35,19 @@ module Controller
# Redirect to the default namespace on logout
def root_path
- (Rails.configuration.action_controller[:relative_url_root] || '') +
- if ActiveAdmin.application.default_namespace
- "/#{ActiveAdmin.application.default_namespace}"
- else
- "/"
- end
+ root_path_method = [ActiveAdmin.application.default_namespace, :root_path].join('_')
@seanlinsley Active Admin member

The line above will return false_root_path if you're using the root namespace for Active Admin.

Hence why I used reject(&:blank?) earlier in this PR

Sorry, I forget ActiveAdmin.application.default_namespace can be false. I thought it can be nil. I will fix it in next commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanlinsley seanlinsley commented on the diff
spec/unit/devise_spec.rb
@@ -42,6 +42,35 @@ def self.helper(*); end
end
end
@seanlinsley Active Admin member

Shouldn't this be in routing_spec instead of devise_spec?

I think it is better in devise_spec. It is mainly related to root_path method in devise.rb and there are other root_path related tests in devise_spec already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@seanlinsley
Active Admin member

@quark-zju default_namespace should never be nil. It should either be false, or a non-empty symbol or string that the user sets.

@seanlinsley
Active Admin member

So what exactly was the reason you downgraded shoulda-matchers? How was it the thing that broke tests?

@quark-zju

@daxter See my previous comment and Travis logs. In short, downgrading shoulda-matchers makes tests passed. Some gems updated today breaks the test. I diffed Gemfile.lock in my laptop and find mocha may be the reason. To get test passed, I need mocha downgraded, and that in turns requires shoulda-matchers to be downgraded.

@quark-zju

I added .presence to ActiveAdmin.application.default_namespace. This is equivalent to reject(&:blank) but more elegant I think.

@quark-zju

@Daxter No, _root_path is impossible. "".present is nil. .present is provided by ActiveSupport, documented in http://guides.rubyonrails.org/active_support_core_extensions.html#presence

@seanlinsley
Active Admin member

... try it on the console.

[nil,'foo'].join('_') # => "_foo"
@quark-zju

Ahh... Added a * and problem should be solved :)

@seanlinsley
Active Admin member

I fail to see how that's more elegant... seems more convoluted to me

@quark-zju

It is not that elegant but a little faster:

require 'benchmark'
require 'active_support/core_ext'

puts Benchmark.measure { 100000.times { [*false.presence, 'a'].join } }
  # => 0.650000   0.000000   0.650000 (  0.653403)

puts Benchmark.measure { 100000.times { [false, 'a'].reject(&:blank?).join } }
  # => 1.030000   0.000000   1.030000 (  1.030428)

I searched in my gems folder and found * in array is used widely. Some of them are intended to get rid of nil. I thought it is a practice pattern.

@quark-zju

With commit messages squashed and a misspelled word corrected, things are cleaner.

@seanlinsley
Active Admin member

@quark-zju could you clean up the commit history?

git reset --hard HEAD~            # removes the most recent commit (the merge commit)
git pull --rebase upstream master # syncs your branch with gregbell's master branch
git rebase -i HEAD~3              # now squash your two commits into one, and give it a descriptive name
@quark-zju

Done.

@seanlinsley
Active Admin member

Did you use the steps I gave? Because b51f87e and 3481027 shouldn't be showing up as part of your PR.

@quark-zju

Yes. But I have used rebase -i before. Maybe that makes something wrong. Now I am trying to rebase again to eliminate these commits.

Or, maybe it's a github's bug ?

@seanlinsley
Active Admin member

How are you pulling in changes from upstream? Are you using the command that I listed?

git pull --rebase upstream master
@quark-zju

Yes. But I am using git push -f, otherwise I got an error.

@quark-zju

Seems this time it works. I did 2 more steps:

git checkout master
git pull upstream master
@seanlinsley
Active Admin member

Manually tested this as well, and everything looks good. Pulling this in now. Thanks for the contribution!

@seanlinsley seanlinsley merged commit 9aa5b65 into activeadmin:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 27, 2013
  1. @quark-zju
Showing with 44 additions and 6 deletions.
  1. +15 −6 lib/active_admin/devise.rb
  2. +29 −0 spec/unit/devise_spec.rb
View
21 lib/active_admin/devise.rb
@@ -35,12 +35,21 @@ module Controller
# Redirect to the default namespace on logout
def root_path
- (Rails.configuration.action_controller[:relative_url_root] || '') +
- if ActiveAdmin.application.default_namespace
- "/#{ActiveAdmin.application.default_namespace}"
- else
- "/"
- end
+ namespace = ActiveAdmin.application.default_namespace.presence
+ root_path_method = [*namespace, :root_path].join('_')
+ url_helpers = Rails.application.routes.url_helpers
+
+ path = if url_helpers.respond_to? root_path_method
+ url_helpers.send root_path_method
+ else
+ # Guess a root_path when url_helpers not helpful
+ "/#{namespace}"
+ end
+
+ # NOTE: `relative_url_root` is deprecated by rails.
+ # Remove prefix here if it is removed completely.
+ prefix = Rails.configuration.action_controller[:relative_url_root] || ''
+ prefix + path
end
end
View
29 spec/unit/devise_spec.rb
@@ -42,6 +42,35 @@ def self.helper(*); end
end
end
@seanlinsley Active Admin member

Shouldn't this be in routing_spec instead of devise_spec?

I think it is better in devise_spec. It is mainly related to root_path method in devise.rb and there are other root_path related tests in devise_spec already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ context "within a scoped route" do
+
+ SCOPE = '/aa_scoped'
+
+ before do
+ # Remove existing routes
+ routes = Rails.application.routes
+ routes.clear!
+
+ # Add scoped routes
+ routes.draw do
+ scope :path => SCOPE do
+ ActiveAdmin.routes(self)
+ devise_for :admin_users, ActiveAdmin::Devise.config
+ end
+ end
+ end
+
+ after do
+ # Resume default routes
+ reload_routes!
+ end
+
+ it "should include scope path in root_path" do
+ controller.root_path.should == "#{SCOPE}/admin"
+ end
+
+ end
describe "#config" do
let(:config) { ActiveAdmin::Devise.config }
Something went wrong with that request. Please try again.