varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

Nested resources #189

Closed jensljungblad closed 7 years ago

jensljungblad commented 8 years ago

This PR is an attempt at adding support for nested resources. Fixes #85

Some things to do:

This is how you would currently implement a nested resource:

# routes.rb
resources :blogs do
  resources :blog_posts
end

# blogs_controller.rb
class BlogsController < ApplicationController
  include Godmin::Resources::ResourceController
end

# blog_posts_controller.rb
class BlogPostsController < ApplicationController
  include Godmin::Resources::ResourceController
end

# blog_service.rb
class BlogService
  include Godmin::Resources::ResourceService
  has_many :blog_posts
end

# blog_post_service.rb
class BlogPostService
  include Godmin::Resources::ResourceService
end

Modules

My first idea was to break the nesting out into its own modules, e.g. Godmin::Resources::ResourceController::Nesting and Godmin::Resources::ResourceService::Nesting that you can include if you want the feature.

However, things are all tangled up in the view layer anyways so I'm thinking it's perhaps a better idea to just skip the modules and put the code in Resources::Controller and Resources::Service.

Update: Decided to skip the modules.

Shallow routes

I'm also thinking that perhaps nested resources should only be supported for shallow routes. That will keep things a bit simpler in the view, since we don't need to check for parents for show, edit, update and destroy. That would mean you would need to do:

# routes.rb
resources :blogs do
  resources :blog_posts, shallow: true
end

Update: Currently not possible since we need the parent id in the route to identify which resource the child is nested under, in order to print the breadcrumb. An alternative would be only allowing being nested under one resource and introducing a belongs_to macro on the service object.

Navigation

The other big thing remaining is how to actually navigate to the nested resources. I was thinking maybe a dropdown button next to the show/edit/destroy links perhaps. And also something on the show page. The problem is, how do we specify that a resource has children? I guess we need to add something new to the service object.

Update: Decided to use a has_many macro and list all associations in a dropdown in the breadcrumb.

benjamin-thomas commented 8 years ago

Sorry for the late feedback.

I tested your branch to have a feel for it, looks great :).

I'm not sure (since I encountered a few bugs) if your intention is to have one controller and one service for one resource (nested and non-nested) ?

If that's the case, I think I prefer the solution of creating a separate controller and service for the nested resource better.

Even if it means more boilerplate, the code remains simpler and more flexible that way.

After some trial and error, I came up with this solution. Then only downside is that I need to override a lot of templates.

I only came up with this earlier today, so I haven't tested it fully but I can't see any other downside at the moment.

# routes.rb
resources :blogs do
  resources :blog_posts, as: 'posts', path: 'posts', only: [:index, :new, :create] # BlogPostsController/BlogPostService
end
resources :posts # PostsController/PostService

# app/controllers/admin/blog_posts_controller.rb
require_dependency "admin/application_controller"
module Admin
  class BlogPostsController < ApplicationController
    include Godmin::Resources::ResourceController
  end
end

# app/services/admin/blog_post_service.rb
module Admin
  class BlogPostService
    include Godmin::Resources::ResourceService

    attrs_for_index :col1, :col2

    filter :my_filter

    def resource_class
      Post
    end

    def resources(params)
      super(params).where(blog_id: params.fetch(:blog_id))
    end

    def filter_my_filter(rel, val)
      rel.something
    end
  end
end
jensljungblad commented 8 years ago

Hey @benjamin-thomas, I realize I was a bit vague on how you would actually implement a nested resource, so I've updated the description with how the PR currently works.

benjamin-thomas commented 8 years ago

Hi @jensljungblad, if you're interested in feedback I'd give a vote for shallow routes only.

Regarding navigation, I think an extra button could work but it would need to be a dropdown since one parent resource could have many nested resources.

If you wanted to detect the nested resources links automatically, I guess you could parse the routes file, however I think it'd be better the let the programmer at least override the links via an instance variable in the controller or something similar.

This could be used for the show/edit pages as well.

Something like this.

def show
    _super = super
    @nested_links = [
         'Blog Posts', blog_posts_path(@resource),
    ]
    _super
end

What do you think?

jensljungblad commented 8 years ago

I'm also leaning towards shallow routes only.

Yup, I was thinking dropdown as well.

Yeah I don't think we should parse the routes file. I'm thinking we could add something to the service object rather than setting an instance variable though.

# blog_service.rb
class BlogService
  include Godmin::Resources::ResourceService

  attrs_for_index :title
  attrs_for_form :title

  nested_resources :blog_posts
end

@Linuus do you have any thoughts on this?

jensljungblad commented 7 years ago

@Linuus Any input on the shallow route thing? Would it make more sense to identify parent resources through the service objects instead of the URL?

(Although I'm thinking maybe we merge this now and revisit if we come up with a better way)

Linuus commented 7 years ago

Yeah, I think it's fine for now.

rubendinho commented 7 years ago

Hey guys i'm enjoying the nested routes features, but am having a problem getting it to work with polymorphic resources. It looks like @resource_service in godmin (1.5.0) app/views/godmin/resource/_scopes.html.erb, line 8 is trying to use the associated model name instead of of the polymorphic association:

<span class="text-muted"> (<%= @resource_service.scope_count(name) %>)</span>

Do you have any plans to add support nested routes for polymorphic associations?

jensljungblad commented 7 years ago

@rubendinho Hi, sorry for the late reply. If this is still an issue please feel free to open a proper issue! And if you have time to investigate either post your findings on the issue or send us a PR. Otherwise I'll take a look when I get a chance.