zorab47 / active_admin-sortable_tree

Show ActiveAdmin index as a nested tree with drag'n'drop
MIT License
162 stars 127 forks source link

Scopes problem #17

Closed ghost closed 9 years ago

ghost commented 10 years ago

Have a good day! I've found that active admin scopes are ignored in sortable tree mode. Am I doing something wrong or is it a bug?

zorab47 commented 10 years ago

It is behaving as expected, which skips the normal ActiveAdmin scopes and filters. The tree is rendered by finding the roots and then iterating through all of its children recursively. If any of the children (or roots) were filtered out then those branches would be excluded entirely using the recursive display approach.

In my experimental branch, I'm assuming the incoming collection only contains root tree nodes, which can then be filtered or scoped. However, those scopes are only applied to the root nodes.

How do you foresee the tree being displayed when a scope or filter is applied?

ghost commented 10 years ago

How do you foresee the tree being displayed when a scope or filter is applied?

I've got many trees in table so I need implement user filter for displaying required tree.

zorab47 commented 10 years ago

In your case does each user have his own tree?

ghost commented 10 years ago

Each user has different tree structures under management so I need to limit visibility

zorab47 commented 10 years ago

In that case, allowing the roots to be scoped would be way to go, and I think that would be a good enhancement. Perhaps the call to sortable could take an option identifying a scope/context proc. Currently, the context is always resource_class, which is always Node.roots (in the case where Node is the resource).

ActiveAdmin.register Node do
  # Proposed scope/context option (and the default)
  sortable tree: true, scope: proc { current_admin_user.nodes } # proc { resource_class }

  # Sortable would then find the roots by calling:
  # current_admin_user.nodes.roots => [#<Node id:1>, #<Node id:5>]
end

Thoughts @nebirhos ?

zorab47 commented 10 years ago

After more consideration, defining a roots_collection option might be a more explicit approach. If the roots_collection is a callable object, then it would be evaluated within the context of the controller. Otherwise it would fallback to calling roots_method on the resource_class.

ActiveAdmin.register Node do
  sortable tree: true, 
           roots_collection: proc { special_roots_collection }

  controller do
    # roots_collection would be evaluated within the controller's context.

    def special_roots_collection
      scoped_collection.roots
    end
  end
end
nebirhos commented 10 years ago

@zorab47 I think having a root_collection option is the right way, it should be easy to implement in https://github.com/nebirhos/activeadmin-sortable-tree/blob/master/lib/active_admin/views/index_as_sortable.rb

        @collection = if tree?
                        @options[:roots_collection] || resource_class.send(@options[:roots_method])
                      else
                        collection
                      end

If one of you could provide a pull request that would be great! Sooner or later I'll write a test suite to make the merge a bit safer :smile:

zorab47 commented 10 years ago

I created a pull request, #20.

zorab47 commented 10 years ago

@nebirhos I'm happy to help maintain the project as it has been very useful.

nebirhos commented 10 years ago

@zorab47 #20 merged, great job! sorry for the delay, but I'm using my free time since my company isn't interested anymore on the development of this project. As for the maintainance, I'd be very happy to be helped, but I have to ask the company first

zorab47 commented 10 years ago

@dshumilov do the latest updates fix your initial issue?

zorab47 commented 10 years ago

This issue can probably be closed.

@nebirhos what's your thought on help with project maintenance?

raels commented 9 years ago

Just found this project and thread. This is a very useful gem!

I was thinking about how search and scope might work..

Would it not make sense for the filters to apply as they normally do, which would yield a list of records The tree that encompasses those records would be displayed. That tree is the composition of all the nodes for the given ids and the ancestry of those nodes, which is pretty easy to compute using the tools you have provided in the gem. It would be awesome if there were a way to differentiate visually between the selected nodes and those included to keep the tree whole, but that sounds like extra credit to me.

Make sense?

zorab47 commented 9 years ago

@raels, agreed I think a highlighting search would be a great addition. However, it might prove difficult depending on which gem is used to manage the hierarchy (like Ancestry or others).