Move 'Files' tab to position it under admin/content/media #221
Conversation
| function _remove_access_files_overview_permission() { | ||
| $roles = Role::loadMultiple(); | ||
| foreach ($roles as $role) { | ||
| if ($role->hasPermission('access files overview')) { | ||
| $role->revokePermission('access files overview'); | ||
| $role->save(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not so sure of this, if there are custom roles that have been assigned this then they will get deleted. Sometimes it is useful if there are files which for what ever reason are not yet in media. I'd limit this to localgov drupal provided roles, and I'd check if they have been altered from default. Noting the update hook also runs this.
|
As @andybroomfield points out - there are potential issues removing this permission for existing sites. Maybe just remove the update hook. We could always add a release note suggesting existing sites do this manually if it's important. |
|
Just discussing in Merge Tuesday. Generally happy to remove the permission for fresh installs. However, deploying an update hook that removes permissions from existing roles is perhaps a bit too far. Perhaps we should remove the permission in the default install, and add instructions for people to clarify that they can remove the permission for certain roles. |
|
Note: would like to check where the permission 'access files overview' is being set to see if we can remove it there rather than here. |
There was a problem hiding this comment.
There's been some discussion on #220 on how desirable this actually is. At least one person finds the files list useful, but it's also needed to ensure a file has been deleted.
If we go ahead and merge this we need to at least document how to get it back should it be needed.
|
Maybe we could have the best of both worlds.
That would mean people can still get to the files tab, but don't mistake if for the media tab which is what they usually want. It might make more sense to go to "Admin > Content > Media > Files" for content editors, as then it looks like files are a sub-item of media items (which they are). |
|
MT: moving the link sounds more promising than removing permission; Are there files being used that aren't in media? |
|
Note: @willguv is away for a few weeks, so might want to park this until he's back. |
|
@willguv likes the sound of @markconroy's suggestion! |
This reverts commit 596cddb.
|
I've rewritten this PR. New version:
Thanks to Big Blue Door for sponsoring my time to work on this. |
localgov_core.install
Outdated
There was a problem hiding this comment.
I don't think we should be moving the files tab for existing installs. The new location isn't very obvious and will be confusing to people who are using files already.
There was a problem hiding this comment.
I've removed the update hook now, however given that we are creating a localgov_core.links.task.yml file, we will now have a 'Files' tab on the admin/content page and also on the admin/content/media page.
Maybe that's okay, and existing sites can update the view manually if they want to remove the tab from the admin/content page.
There was a problem hiding this comment.
There is a little oddity here for existing installs as going to the files listing now looks like this
Let's discuss as a Merge Tuesday as to how much if an issue it is.
I also note that Files appears under Content in the menu and where perhaps it should under Content > Media. But if this is a problem then it could be another issue.
|
@willguv we're discussing this in Merge Tuesday and not sure of the motivation for moving the files tab. Could we discuss with content and product people? Files and media entities are different things, and there are use cases for accessing both. We could perhaps address this with a how-to or other documentation? |
|
@finnlewis Will stated at product group meeting today that we should just create an update hook to remove the tab from admin/content and write a release note to say where the files tab has been moved to. |
|
referencing #283 follow-up for why this is done, and potentially reviewing how this is done. |
… menu. Note: This is opinionated administration configuration that probably shouldn't be enforced on users just requiring core or media. It probably wants to live somewhere else.
The view is supplied by localgov_media so creating the link here too. Probably wants to be in some other LocalGov Admin configuration module really.
Closes #220
Removes the permission to access the files overview permission. It's probably only developers that need this for debugging things, so we can add that manually when needed, or add it to the admin role (when we have one).
===
Thanks to Big Blue Door for sponsoring my time to work on this.