Fix linking to ActiveAdmin actions for models that define to_param

[mhuggins]

If a model defines to_param in order to display pretty SEO-friendly URL's, ActiveAdmin links don't work. For example:

class Post < ActiveRecord::Base
  attr_accessible :title, :permalink

  before_create :generate_permalink

  def to_param
    permalink
  end

protected

  def generate_permalink
    permalink ||= title.parameterize
  end
end

In ActiveAdmin, all the links are called in a manner such as admin_post_path(post), which will become /admin/posts/hello-world for example. When ActiveAdmin attempts to load that page, RecordNotFound is raised since it's performing a find on the id column, not the permalink column.

The solution is to build all links with the record's id, e.g.: admin_post_path(post.id).

This pull request implements the above fix. It also includes an updated test that, if tested on the code prior to this pull request, will demonstrate the error.

[gregbell]

Hi Matt,

Thanks for this patch! This is great and definitely something we need merged in.

I'm getting failing specs from the patch though. Could you please get them passing, then I'll merge it in. Looks like it's the spec/integration/belongs_to specs that are failing.

Finished in 17.2 seconds
9 examples, 4 failures, 2 pending

Failed examples:

rspec ./spec/integration/belongs_to_spec.rb:20 # Belongs To the index page the main content should display the default table
rspec ./spec/integration/belongs_to_spec.rb:26 # Belongs To the index page the breadcrumb should have a link to the parent's index
rspec ./spec/integration/belongs_to_spec.rb:29 # Belongs To the index page the breadcrumb should have a link to the parent
rspec ./spec/integration/belongs_to_spec.rb:35 # Belongs To the index page the view links should take you to the sub resource
rake aborted!

[mhuggins]

Whoops, I see that now too...sorry about that, I thought I had gotten the spec working. Let me take a crack at that and ping you when I push that fix!

[mtylty]

@mhuggins do you need a hand with those tests?

[mhuggins]

@mtylty I've just been really busy all of a sudden, unfortunately. If you care to take a look at it, you're more than welcome! If not, it's still on my to-do list. :)

[mtylty]

I'm already deep in code :P

[pwightman]

I'm having a related problem where I have a parent:

ActiveAdmin.register FaqCategory do
  controller do
    defaults :finder => :find_by_url
  end
end

And child:

ActiveAdmin.register Faq do
  belongs_to :faq_category
end

But when I try to access the child, it is not using its parent's correct finder, it's trying to find id=some_url . I'm thinking this will fix it (unless there's something in the DSL I'm missing that lets me hook into the parent finder within the child). Just a bump for this issue :-)

[mtylty]

I've been messing around trying to fix the problem, but it seems that polymorphic paths don't go well with ids...

The polymorphic "problem" should be whenever resource_path is used with .id.

Anybody have suggestions?

[mtylty]

@mhuggins you should have a pull request waiting in your inbox, can you please update this pull request so that everybody can have a look and check if everything works? ;)

[mhuggins]

@gregbell Thanks to @mtylty's help, the tests now pass, and the links appear to render properly when I use his changes in my own project. Could you look into accepting this pull request again? Thanks!

[cmbankester]

Any idea when this will be merged?

[groe]

Currently having the same problem - would be nice if someone could merge this.

btw great gem :)

[lleger]

This is also a problem for me, but this patch fixed the issue. The tests pass for me and it doesn't break in situations where to_param isn't overridden. @gregbell, please merge. Thanks!

[mtylty]

Considering the last commit I am quite sure that @gregbell is on christmas vacation... :D

[mperham]

We, too, have had to pull in monkeypatches to fix this issue against master. I'd love to see it merged.

[aagrawal2001]

+1
Please merge this. I'm running into the same problem.

[kennyadsl]

Hi @gregbell,
is there any problem with this solution? It seems to be working and all tests pass. Please, just tell us what's wrong with it.

Thanks

[gregbell]

Hey guys, sorry for the delay on some of these pull requests. Christmas and then new company really limited my time for Active Admin.

Anyway, this looks good. I'm just running the tests on my machine and I'll get this merged in this morning for 0.4.0.beta

[gregbell]

When I run this pull request locally with models that implement to_param, the to_param is still used. It doesn't seem to be using the id instead. Can anyone confirm that they are getting correctly rendered paths using only ids?

[mtylty]

Yes I can confirm that... I'm working on a solution, I'll pull request @mhuggins as soon as I can.

[mtylty]

Ok, the pull request is https://github.com/mhuggins/active_admin/pull/2

Tests and webapp now work but I'm beginning to wonder if we need more thorough tests about nested resources.
I say this because, even though the tests now pass, I highly doubt the code will work when using nested resources
and index_as_blog (since the code is only tested on index_as_table). @gregbell am I right?

Anyone care to write some tests? :)

[mtylty]

It seems that, even with this pull request, the action_items links, with the same exact syntax used in index_as_blog or index_as_table, do not generate links with the numeric id...

At this point I'm lost :(

[aagrawal2001]

I ran into the same problem. Just submitted:

https://github.com/mhuggins/active_admin/pull/3

This fixed it for me.

[mtylty]

@aagrawal2001 I tried that, but it seemed integration tests fail. Are you sure all tests pass?

[mattvague]

@mhuggins any movement on this?

[dnordstrom]

I've fixed the to_param issues with an around filter but when adding comments I'm still redirected to the wrong URL. This patch would fix it:

https://github.com/mhuggins/active_admin/commit/f68223bd9884cd894c19ba88995a1fedd383c0f8

Any idea on when this may be pulled in?

[HamptonMakes]

+++10

[felipecsl]

+1 this is broken for me with activeadmin 0.4.3

[fallanic]

Had the same problem with 0.4.3 and had to use this workaround to make it work

#279 (comment)

[mergulhao]

+1 for this.

[iloveitaly]

+1

[xymbol]

Please note that, if using something like friendly_id, you already get this. The friendly_id gem overrides both the to_param and find methods accordingly so that regular finders just work.

[groe]

Any news on this?

[seanlinsley]

This PR is stale, so I'm closing it. If this functionality is still desired, please open a new pull request.

[santry]

@pwightman It's been 2 years so perhaps you found a solution or workaround already, but for posterity:

If you have a parent resource for which you have a custom to_param and therefore need to specify a finder:

ActiveAdmin.register FaqCategory do
  controller do
    defaults :finder => :find_by_url
  end
end

You can get a child resource to use the proper finder by specifying:

ActiveAdmin.register Faq do
  belongs_to :faq_category, :finder => :find_by_url
end

More detailed information on how to configure these sorts of things is available from the inherited_resources gem, which is what provides this functionality to ActiveAdmin.

[fakefarm]

Thanks, @santry! 🍻

[pwightman]

Yes, thanks!