Collection hierarchy #3407
Collection hierarchy #3407
Conversation
…eQueryset, since it is not specific to pages and can be used for Collection navigation as well.
… out logic from `get_pages_with_direct_explore_permission` into a separate helper method, to be reused for collections.
…tion. Updated index view to display a collections children.
…he collection nesting.
…o be different from those on `GroupCollectionPermission`.
…management of collections can be controlled per group.
…issions a user has for a particular collection.
… Created hook for registering the collection listing buttons.
…ton templatetag. Also applied some styling/structural tweaks.
… any of its children collections contain any content.
… user pemissions for all collections.
… determining whether or not the user should be given access. Fixed issue where `get_collections_with_direct_explore_permission` could contain the same Collection multiple times.
…view. Still need to get search working.
|
Hey guys, So there is still some work that needs to be done on this, such as writing tests and fixing nested modal issues. Before I spend a lot more time working on this, is this something you guys would be willing to look at any time soon? |
|
+1 to have this in next reelase... |
|
@trumpet2012, I'm really sorry that it's taken us so long to respond to this impressive piece of work. I think we were all slightly terrified by the scale of it! We have just been discussing your PR in our core team meeting, and @chosak has kindly agreed to give it an initial review. We'd like to include this in the next release, if possible. Will you have time to write the new tests in the next few weeks? |
|
@tomdyson That is awesome to hear! Aside from creating new tests and fixing conflicts, the only other issue I know of is how to handle the nested modals, such as opening the collection chooser from the image/document chooser. I explain the problem in greater detail here: https://groups.google.com/forum/?pli=1#!topic/wagtail-developers/8wVNtiYnPF8. The next few weeks are gonna be a little crazy here with Thanksgiving and then Christmas around the corner. I'm talking with my company about the possibility of me being able to use my work time to work on this. This is a feature they want to see implemented, but we have other scheduled development ongoing. Regardless, I will try and make some time to work on this. What does the timeline look like for the 2.0 release? I assume you guys want to release around the same time as Django 2.0. |
There was a problem hiding this comment.
What does the timeline look like for the 2.0 release?
The current plan is to release 2.0 in early December, shortly after Django 2.0 is released.
@trumpet2012 this is a nice improvement, thank you for contributing!
I took a stab at squashing and rebasing to try to review this PR (my branch is here). There seems to be some overlap with how collection view restrictions were implemented in #3644 (View restrictions for documents). Do you have any thoughts as to how to best reconcile?
There are some front-end styling things that others might be better suited to review, e.g. this.
This PR is quite large, and it would be nice to somehow break it down somehow into a few smaller pieces. My first thought is perhaps it'd be best to defer the new collection chooser. This would eliminate the "move" functionality (which doesn't seem so critical) and simplify integration into the image/document chooser views. For those could we instead use a modified version of the current dropdown that visually indicates the nesting using e.g. Root, -- Child, ---- Grandchild?
At a high level integration of the collection chooser nested inside of the image/document chooser feels a bit messy. Are there other examples of this kind of nesting in Wagtail? The modifications this requires to e.g. the generic index view feel a bit overly complex.
| @@ -34,6 +34,10 @@ header { | |||
| .left { | |||
| float: left; | |||
|
|
|||
| @media screen and (max-width: $breakpoint-mobile) { | |||
There was a problem hiding this comment.
What's the motivation behind this specific change? This seems like it'd affect the header on all admin pages.
| @@ -84,6 +84,105 @@ def not_ancestor_of(self, other, inclusive=False): | |||
| """ | |||
| return self.exclude(self.ancestor_of_q(other, inclusive)) | |||
|
|
|||
| def first_common_ancestor(self, include_self=False, strict=False): | |||
There was a problem hiding this comment.
Does this change modify anything, or is it just moving the method from below? Can this change be removed?
There was a problem hiding this comment.
I moved it from PageQuerySet to TreeQuerySet since this logic is generic enough to be used for any treebeard tree. This way it can be used for collection tree navigation.
|
|
||
| return context | ||
|
|
||
| def get(self, request, root_id=None): |
There was a problem hiding this comment.
Having this endpoint handle both the browse and (nested) chooser feels a bit complicated, although unfortunately I don't have a cleaner approach off the top of my head. See other comment on wagtailadmin/views/generic.py.
| return context | ||
|
|
||
| def get(self, request): | ||
| context = self.get_context() |
There was a problem hiding this comment.
This change conflicts with (and breaks) the default list view handling introduced in #3625. See default Django behavior here that supports both self.template_name and self.get_template_names() as possible template sources.
|
Hello, and thank you! I was wondering if this implementation could somehow be used for other treebeard hierarchies, for example, on the modeladmin contrib. |
…eviously addressed issue with overlapping menu icon and header, which should have since been resolved in newer wagtail versions.
What is the best way to work off of your changes? I'm working on pulling in the latest Wagtail changes as well. I sent you an invite so that you could have write access to this branch if you need it.
Need to figure out what exactly the overlap between the two is. The collection permissions I added are intended to control which users can add/edit/delete collections from the admin.
Yeah I think that makes sense. In hindsight, I should have done it like that from the beginning. Using the dropdown box should work as a first out. I will work on taking out the chooser and doing the dropdown. Is the idea of having a collection chooser modal something that makes sense to you guys as a future implementation?
I have been unable to find anywhere this has been done before. I agree that it doesn't feel clean.
It looks like the implementation of the Wagtail generic index view has changed considerably since I last looked at this. I would be able to get the modal working without modifying the generic index view now. |
|
@rafaponieman This implementation is very collection centric. As it stands, it could not be used to handle other treebeard hierarchies. It is likely out of scope to try and make this generic. |
|
@trumpet2012 thanks for following up!
You could branch off of mine if you like, or, if you're amenable, I would probably recommend opening a brand new PR off of current master, and pull in relevant changes without the chooser (using a simplified dropdown as proposed). Optionally you could try to remove from this PR and merge in all the necessary changes, but it might be easier to start with a new branch.
Thanks, I saw that, but that's not necessary. It's probably best if you rescind that invitation, honestly, as there's no need for me to be part of your company's private organization.
Personally I think this could be useful if someone has a large number of collection. As there doesn't appear to be any other nested modals, it'd require some higher-level thinking about how best to implement. |
I work on migrating a website to wagtail that has been around since 2004. It's the website of a society and they do regular events every year. The website has about 100 photos per event and about 30 events per year. I would very much welcome a collection chooser that is not only a select box. Just as an idea, maybe something similar to the flyout menu for Pages in the admin might be easier to implement than nested modals. |
|
@trumpet2012 excellent, I'll close this PR and give the new one a review. |
This PR is for adding hierarchical support for Collections to the admin interface #3380.
This has quickly become a rather large PR so I wanted to go ahead and open it so you guys could give feedback as I go along.
Screenshots at current state (2/27/2017)
http://imgur.com/a/Q5DPK
Screenshots (3/2/2017)
http://imgur.com/a/BPnrf
TODO:
AdminCollectionChooserWidgetCollectionviewsCollectionchooserCollectionChooserPanelGroupsadmin pageDocumentsadmin to use the chooserImagesadmin to use the chooserGroupCollectionManagementPermissionto control which groups of users can make changes to which part of the collection tree. (Works like Pages)UserCollectionPermissionsProxyandCollectionPermissionTesterfor handling checking a users access level for collections (just like the Proxy and Tester for Page)CollectionChooserPanel