ActiveAdmin::Resource loads a resource for breadcrumbs #2315

amiel · 2013-07-02T18:36:48Z

Here is an proof-of-concept attempt at #2266.

A couple of notes/thoughts:

I think this is better than having the loading logic in the breadcrumbs helper, but it still doesn't seem idea. All of the resource loading logic is already in the controller (through inherited resources, so not easy to refactor out).

Another possible direction would be to use ActiveAdmin::Resource#controller to load the resource. I started trying this, but it gets ugly pretty quick. Here's what it would look like:

  def resource_from_param(id)
    c = controller.new
    c.params = { id: id }
    c.send(:instance_variable_set, "@_session", session)
    # I didn't actually get this far, so I'm not sure what other hacks we would need...
    c.send(:resource)
  end

So, yeah, not great, but at least the resource loading logic isn't duplicated.

@daxter, any thoughts?

amiel · 2013-07-02T18:37:27Z

Oh, by the way, I'm just spiking here. I can write tests once a direction is decided...

seanlinsley · 2013-07-04T01:21:08Z

This is the direction I was imagining, except you still need to add the decorator wrapping :)

Instead of resource_from_param, couldn't you name it find_resource? Or is that method name already taken?

amiel · 2013-07-04T16:44:41Z

The decorator wrapping is already in there, I just need to fix all the tests and add a test for the decorated breadcrumb case.

find_resource sounds pretty good to me.

seanlinsley · 2013-07-04T18:42:43Z

Ah, my apologies.

Cool. Any idea when you'll have time to update this?

amiel · 2013-07-04T18:45:22Z

I definitely won't have time before next week. I'll try to get it in on the early side of next week.

Thanks for your input :)

seanlinsley · 2013-07-04T19:10:57Z

Not at all; thank you for going to the trouble to do this!

amiel · 2013-07-11T18:44:00Z

@daxter I just rebased this branch onto master.

I'm still getting a lot of test failures on master. Should master be all green right now and I have an environment problem? Or are there known failures on master?

BTW, I made sure to rm -rf spec/rails to make sure my spec/rails was regenerated.

seanlinsley · 2013-07-11T19:48:43Z

No, master is passing just fine. I'll take a look into what's.causing the failures.

amiel · 2013-07-11T19:58:27Z

I've been looking into the failures. It looks like the database isn't set up (the failures are mostly Could not find table 'admin_users' or Could not find table 'posts'.

Here's what I've tried so far (to no avail):

cd spec/rails/rails-3.2.13
rake db:migrate
rake db:test:prepare
TEST_ENV_NUMBER=2 rake db:test:prepare
cd ../../..

rake # still fails

amiel · 2013-07-11T19:59:17Z

Anyway, it's helpful to know that it's working for you.

Given my last post, do you have any ideas?

seanlinsley · 2013-07-15T19:18:30Z

Are you not able to run even master? This should work just fine:

rm -rf spec/rails
rspec spec/unit

amiel · 2013-07-15T20:21:36Z

running

rm -rf spec/rails
rspec spec/unit

passes.

rm -rf spec/rails
rake

fails on spec/unit/views/tabbed_navigation_spec.rb

I'll start writing some tests using rspec instead of rake thanks.

seanlinsley · 2013-07-15T20:33:36Z

What about rake test? That should also work.

amiel · 2013-07-15T20:44:28Z

Well that's embarrassing, rake test is passing for me on master.

It looks like plain old rake default is failing because it runs rake parallel_tests instead of rake test (task :default => :parallel_tests).

Sorry to have bothered you with this, and thanks for helping troubleshoot.

seanlinsley · 2013-07-15T20:48:06Z

No problem. IMO that's probably something that should be changed. Or more, the parallel_tests rake task shouldn't try to spin up X number of runners that each try to build the very same Rails app to use for testing.

BTW there are a couple other useful things in the CONTRIBUTING file

amiel · 2013-07-15T20:49:40Z

Yeah, thanks. I actually did read CONTRIBUTING, but I sort of assumed that the default rake was rake test. I should have checked.

amiel · 2013-07-16T20:17:52Z

spec/unit/view_helpers/breadcrumbs_spec.rb

@@ -136,7 +139,7 @@ def link_to(name, url); {:name => name, :path => url}; end
        trail[1][:path].should == "/admin/posts"
      end
      it "should have a link to /admin/posts/1" do
-        trail[2][:name].should == "1"
+        trail[2][:name].should == "Hello World"


This is a slight change in functionality, but I think for the better.

Actually, I lied, it's just a change in the stub used by the whole test.

What part of your changes caused this?

Good question. It was https://github.com/gregbell/active_admin/pull/2315/files#L3R14

amiel · 2013-07-16T20:21:24Z

@daxter Ok, I think I'm all done here. Do you want me to squash this in to one commit? I've been asked to do that before in ActiveAdmin pull requests.

amiel · 2013-07-16T20:37:40Z

Hmmm, there are travis failures. I'm out of open-source time today. I can take a look in a couple of days if you haven't figured it out.

For reference, same failure is happening in #2350, and it is unrelated to this pull-request.

coveralls · 2013-07-16T21:22:32Z

Coverage decreased (-0%) when pulling c88fa7c671e4cd086f303789bfa3c5ced93ec60c on carnesmedia:config_loads_resource into ec99964 on gregbell:master.

seanlinsley · 2013-08-05T02:42:50Z

@amiel can you rebase this on master? Tests are passing as of #2385

amiel · 2013-08-05T18:45:20Z

Ok, rebased, and tests pass for me locally. Hopefully Travis is happy now...

amiel · 2013-08-05T19:37:45Z

@daxter this should be good to go

seanlinsley · 2013-08-05T19:48:56Z

lib/active_admin/view_helpers/breadcrumb_helper.rb

@@ -13,7 +13,7 @@ def breadcrumb_links(path = request.path)
          # 3. default to calling `titlecase` on the URL fragment
          if part =~ /\A(\d+|[a-f0-9]{24})\z/ && parts[index-1]
            config = active_admin_config.belongs_to_config.try(:target) || active_admin_config
-            obj    = config.resource_class.where( config.resource_class.primary_key => part ).first
+            obj    = config.find_resource(part)
            name   = display_name obj


Could you collapse the last two lines?

name = display_name config.find_resource(part)

Yep, will do

amiel · 2013-08-05T20:07:39Z

Would you like me to squash this in to one commit?

seanlinsley · 2013-08-05T20:15:48Z

Yes, please. Feel free to add as much as you want to the commit message when you squash them.

Closes #2266 Add `ActiveAdmin::Resource#find_resource`. This is now a centralized place to load a resource, with correct handling of primary key, decoration, etc. Then, use this new way of loading resources in the Breadcrumbs logic. This was the original impetus for for defining `ActiveAdmin::Resource#find_resource`. Previously, the `BreadcrumbsHelper` did a one-off load of the resource, which has historically been a source of bugs and/or inconsistencies. Please note that this is duplication with InheritedResources. Normally, the resource is loaded by InheritedResources, but there are situations in which ActiveAdmin needs to load resources as well (ie Breadcrumbs). It was not reasonable to use InheritedResources to load the resource (see the discussion at #2315).

coveralls · 2013-08-05T20:44:56Z

Coverage increased (+0%) when pulling 8552e2f on carnesmedia:config_loads_resource into 33429ed on gregbell:master.

ActiveAdmin::Resource loads a resource for breadcrumbs

seanlinsley · 2013-08-05T21:59:23Z

Thanks for the contribution! 🐙

Closes activeadmin#2266 Add `ActiveAdmin::Resource#find_resource`. This is now a centralized place to load a resource, with correct handling of primary key, decoration, etc. Then, use this new way of loading resources in the Breadcrumbs logic. This was the original impetus for for defining `ActiveAdmin::Resource#find_resource`. Previously, the `BreadcrumbsHelper` did a one-off load of the resource, which has historically been a source of bugs and/or inconsistencies. Please note that this is duplication with InheritedResources. Normally, the resource is loaded by InheritedResources, but there are situations in which ActiveAdmin needs to load resources as well (ie Breadcrumbs). It was not reasonable to use InheritedResources to load the resource (see the discussion at activeadmin#2315).

seanlinsley mentioned this pull request Jul 16, 2013

Default rake to rake test #2350

Merged

amiel reviewed Jul 16, 2013
View reviewed changes

seanlinsley reviewed Aug 5, 2013
View reviewed changes

seanlinsley added a commit that referenced this pull request Aug 5, 2013

Merge pull request #2315 from carnesmedia/config_loads_resource

02ff162

ActiveAdmin::Resource loads a resource for breadcrumbs

seanlinsley merged commit 02ff162 into activeadmin:master Aug 5, 2013

amiel deleted the config_loads_resource branch August 5, 2013 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActiveAdmin::Resource loads a resource for breadcrumbs #2315

ActiveAdmin::Resource loads a resource for breadcrumbs #2315

amiel commented Jul 2, 2013

amiel commented Jul 2, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 4, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 4, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 11, 2013

seanlinsley commented Jul 11, 2013

amiel commented Jul 11, 2013

amiel commented Jul 11, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

amiel Jul 16, 2013

amiel Jul 16, 2013

seanlinsley Jul 16, 2013

amiel Jul 16, 2013

amiel commented Jul 16, 2013

amiel commented Jul 16, 2013

coveralls commented Jul 16, 2013

seanlinsley commented Aug 5, 2013

amiel commented Aug 5, 2013

amiel commented Aug 5, 2013

seanlinsley Aug 5, 2013

amiel Aug 5, 2013

amiel commented Aug 5, 2013

seanlinsley commented Aug 5, 2013

coveralls commented Aug 5, 2013

seanlinsley commented Aug 5, 2013

ActiveAdmin::Resource loads a resource for breadcrumbs #2315

ActiveAdmin::Resource loads a resource for breadcrumbs #2315

Conversation

amiel commented Jul 2, 2013

amiel commented Jul 2, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 4, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 4, 2013

seanlinsley commented Jul 4, 2013

amiel commented Jul 11, 2013

seanlinsley commented Jul 11, 2013

amiel commented Jul 11, 2013

amiel commented Jul 11, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

seanlinsley commented Jul 15, 2013

amiel commented Jul 15, 2013

amiel Jul 16, 2013

Choose a reason for hiding this comment

amiel Jul 16, 2013

Choose a reason for hiding this comment

seanlinsley Jul 16, 2013

Choose a reason for hiding this comment

amiel Jul 16, 2013

Choose a reason for hiding this comment

amiel commented Jul 16, 2013

amiel commented Jul 16, 2013

coveralls commented Jul 16, 2013

seanlinsley commented Aug 5, 2013

amiel commented Aug 5, 2013

amiel commented Aug 5, 2013

seanlinsley Aug 5, 2013

Choose a reason for hiding this comment

amiel Aug 5, 2013

Choose a reason for hiding this comment

amiel commented Aug 5, 2013

seanlinsley commented Aug 5, 2013

coveralls commented Aug 5, 2013

seanlinsley commented Aug 5, 2013