Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix route_instance_path #2099

Merged
merged 9 commits into from
Apr 16, 2013
Merged

Fix route_instance_path #2099

merged 9 commits into from
Apr 16, 2013

Conversation

pcreux
Copy link
Contributor

@pcreux pcreux commented Apr 12, 2013

route_instance_path was not working with nested resources (the one that use belongs_to). It's now fixed.

route_instance_path was returning a symbol like :admin_post_path while route_collection_path was returning the path like /admin/posts. route_instance_path now returns the path.

I also refactored the route generation into RouteBuilder.

🍰

@seanlinsley
Copy link
Contributor

Cool, thanks. Are there any Github Issues that are associated with this?

Why is all this needed in the first place? Why can't we build our objects so they act like the objects that the Rails router expects, so we don't have to do any routing at all?

@seanlinsley
Copy link
Contributor

BTW, I won't be able to do an in-depth review of this until Sunday at least.

@pcreux
Copy link
Contributor Author

pcreux commented Apr 12, 2013

What do you mean by building objects like the ones the Rails router expects?

@seanlinsley
Copy link
Contributor

I mean that in vanilla Rails you can just link_to the_resource_obj_instance and you're done.

@pcreux
Copy link
Contributor Author

pcreux commented Apr 12, 2013

Right. This only works when Rails can guess the route from the model name: User.find(1) => /users/1. So it only works if your resource is registered in the :root namespace.

The following would not work:

  • ActiveAdmin.register User, as: "Person"
  • or belongs_to: :manager
  • or namespace: :admin.

This was needed to fix auto_link for nested associations. I also also use it to build links in email notifications.

There is no Github issue.


def route_collection_params(params)
if nested?
params[:"#{belongs_to_name}_id"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to symbolize the key before lookup. The params hash is a HashWithIndifferentAccess which means that the keys are stored as strings, but if you look up a symbol it will convert it to a string before doing a search.

@seanlinsley
Copy link
Contributor

@pcreux what does it mean for a route to be "uncountable"? I've never understood that.

@seanlinsley
Copy link
Contributor

Nevermind, I figured it out myself. For posterity, here's a useful link, and my findings.

# our code
def route_uncountable?
  config = controller.resources_configuration[:self]
  config[:route_collection_name] == config[:route_instance_name]
end

# on the console
AdminUsersController.resources_configuration[:self]
 => {:route_collection_name=>:admin_users, :route_instance_name=>:admin_user, etc.}

# if we mark "admin_user" as an uncountable word with Rails,
ActiveSupport::Inflector.inflections do |inflect|
  inflect.uncountable %w(admin_user)
end

# then the configuration changes as such (after server restart)
AdminUserController.resources_configuration[:self]
 => {:route_collection_name=>:admin_user, :route_instance_name=>:admin_user, etc.}

# Since Rails (for some unknown reason) adds _index to the named route for the index page
# of an uncountable resource, this bit in our route lookup is necessary
resource.route_uncountable? ? 'index_path' : 'path'

macfanatic added a commit that referenced this pull request Apr 16, 2013
@macfanatic macfanatic merged commit d472b2a into master Apr 16, 2013
@macfanatic macfanatic deleted the fix-instance-route branch April 16, 2013 10:43
@seanlinsley
Copy link
Contributor

... @macfanatic what's the point of even doing a PR if you're going to accept code and not have concerns addressed before it's pulled in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants