Fix titles in breadcrumbs (particularly belongs_to)

[shekibobo]

Fixes #2132. For realz.

[shekibobo]

module ActiveAdmin
  module ViewHelpers
    module BreadcrumbHelper

      # Returns an array of links to use in a breadcrumb
      def breadcrumb_links(path = request.path)
        parts = path[1..-1].split('/') # remove leading "/" and split up the URL
        parts.pop                      # remove last since it's used as the page title

        parts.each_with_index.map do |part, index|
          # 1. try using `display_name` if we can locate a DB object
          # 2. try using the model name translation
          # 3. default to calling `titlecase` on the URL fragment
          if part =~ /\A(\d+|[a-f0-9]{24})\z/ && parts[index-1]

I'm confused about what's happening here. If we grab the belongs_to_config (if it exists), aren't we always going to get the parent resource? It seems like just grabbing active_admin_config would do what we want, but that's coming from the controller as far as I can tell.

            config = active_admin_config.belongs_to_config.try(:target) || active_admin_config
            name   = display_name config.find_resource(part)
          end
          name ||= I18n.t "activerecord.models.#{part.singularize}", :count => 1.1, :default => part.titlecase

          link_to name, '/' + parts[0..index].join('/')
        end
      end

    end
  end
end

[shekibobo]

Is there a way to check if the parts[index-1] string in some way matches the config we get?

[zorab47]

You may be able to compare it to the routes defined in resource/routes.rb, if it is a resource config object.

[shekibobo]

I've got this tested manually and it appears to work, but I can't get the specs to pass. I'm pretty new to using test doubles, and here's the error I get:

Failure/Error: let(:trail) { breadcrumb_links(path) }
       Double received unexpected message :find_resource with ("1")
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:19:in `block in breadcrumb_links'
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:10:in `each'
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:10:in `each_with_index'
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:10:in `each'
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:10:in `map'
     # ./lib/active_admin/view_helpers/breadcrumb_helper.rb:10:in `breadcrumb_links'
     # ./spec/unit/view_helpers/breadcrumbs_spec.rb:26:in `block (3 levels) in <top (required)>'
     # ./spec/unit/view_helpers/breadcrumbs_spec.rb:172:in `block (4 levels) in <top (required)>'

It's the same for each test lines 157-176.

[zorab47]

That error is being raised because the active admin config double does not respond to the #find_resource method.

FYI, naming doubles is helpful in finding these types of errors:

double("user", display_name: "User 1")

[zorab47]

I think the intent is for the active_admin_config to be an alias of post_config.

With this change all the tests pass:

let :active_admin_config do
  post_config
end

[shekibobo]

Thanks @zorab47, that seems to have fixed it.

[zorab47]

This line has become very complex. How would you describe its purpose and can it be factored out into a predicate method?

Is it checking for parent components of the URL?

[shekibobo]

Basically, if part is an id-like parameter, and the previous part exists (if so, it is the route key), then we have a chain we can use to figure out what the resource is.

I could just as easily not do the assignment in the conditional, changing it to

if part =~ /\A(\d+|[a-f0-9]{24})\z/ && parts[index-1]
  route_key = parts[index-1]
  # ...

or

config   = parent_config if parent_config && parent_config.resource_name.route_key == parts[index-1]

[seanlinsley]

There's a lot of lines changed in the tests, particularly switching from posts to users. What do those changes accomplish?

[shekibobo]

Mostly keeps the test running down the same chain of associations. Before the tests were only checking for the first set of associations in the breadcrumbs. The new ones assume that a chain like admin/users/1/posts/1/edit. I changed the tests to have a consistent context throughout. Before, it was just looking at posts, but I needed posts to belong to users. Since the resource itself is arbitrary for top-level routes, I switched it to users so posts could show up nested.

[jerodsanto]

Just want to add my +1 to this PR. I kept getting a nil resource when using belongs_to on an edit action and pulling in @shekibobo's changes fixed it for me.

I also agree that the if part line is quite complex and could use a little refactoring, but would like to see this merged regardless.

Thanks to all for your hard work!

[seanlinsley]

Anything need changing in this PR @shekibobo?

[shekibobo]

Well, I could move the variable assignment out of that first line, but aside from that coding style issue and a squash, I'm good to go on this one.

[seanlinsley]

I was considering changing it to:

if part =~ /\A(\d+|[a-f0-9]{24})\z/ && parts[index-1]
  parent = active_admin_config.belongs_to_config.try :target
  config = parent && parent.resource_name.route_key == parts[index-1] ? parent : active_admin_config
  name   = display_name config.find_resource part
end

But then you have one really long line in the middle :/

[shekibobo]

That was kind of the struggle I was having. It's either multiple lines or longer lines. Not sure we can shortcut the logic anywhere, really. Either way should work, though. Your call.

[seanlinsley]

Hmm, yeah let's go the longer line route. I think it more clearly describes what's happening, even if style guides might frown on it.

[shekibobo]

Travis sent me an email saying the build was broken before they all passed, so I rebased on master to be sure. If it passes, I'll squash and we should be good to go.

[shekibobo]

Squashed 💚

[coveralls]

Coverage decreased (-0.03%) when pulling 59fea68 on shekibobo:2132-fix-breadcrumbs into dea86ac on gregbell:master.