-
-
Notifications
You must be signed in to change notification settings - Fork 599
[5.x] Ability to opt into v6 asset folder permissions #12060
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Removed unused allow_downloading, allow_moving, allow_renaming - Removed unused folder_permissions_enabled - allow_uploads and create_folders use the respective policies - Adjusted the test to exclude the removed items - Adjusted the test so that allow_uploads and create_folders are false. This is because it'll now actually read from the permission. - Browser component doesn't need to check the permissions explicitly since it'll now be passed down from the server.
Member
|
We agree this is a good move for v6. The existing container level "permissions" (like allow moving, allow uploading, etc) should've always been actual permissions. I've updated this PR so that it's a more generic "Do it the v6 way" that you can opt into. In a separate PR I'll remove the setting, deprecated methods, and make things the new default behavior. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Edit by Jason:
This PR adds an option to opt into the v6 asset folder permissions.
This setting will be removed in v6 and will become the standard behavior.
This adds a new
edit {container} folderspermission that will be paired with the corresponding asset permissions.e.g.
edit {container} foldersedit {container} foldersandmove {container} assets.edit {container} foldersanddelete {container} assets.There's just one folder permission which acts as both edit/delete. This is because if you were to move a folder from
heretothere, you are technically creatingthereand deletinghere.Also in v6, the container level permission options will be removed in favor of these actual permissions. They are staying for v5 though.
Original PR description:
This PR adds dedicated permissions for creating, move, renaming and deleting asset folders. So far they are simply tied to the asset permissions themselves, but we need to be more specific in our current project.
As this would change the current behaviour of these permissions, I moved the changes behind a config toggle (with default
false). I feel like this could be sensible to enable by default in v6. I'm happy to create a PR for this as well, either with the default set totrueor without any config toggle at all. Let me know what you think 😃On a side note, I aligned the label of the RenameAssetFolder action with the other respective action labels. All other ones simply had "Delete" or "Move", just this one had "Rename folder", which looked a bit out of place. With this PR it's aligned to also just show "Rename".