Umbraco-CMS
Umbraco-CMS copied to clipboard
Refactor GetChildren method on Content Controller and introduce GetChildrenForSort
I've just pushed some updates that hopefully tackle the issue discussed here https://github.com/umbraco/Umbraco-CMS/issues/15608
This is a draft PR, so consider this a potential option for a fix. There maybe other options we can explore, so any feedback would be welcome :)
Introduced the GetChildrenFilter
class for parameter encapsulation to enable a DRY approach.
This change facilitates the introduction of a new GetChildrenForSort
method, allowing precise control over the ActionLetter in the FilterAllowedOutgoingContent
.
Added the GetChildrenFilter
to 'Umbraco.Cms.Core.Models.ContentEditing' as there seemed to be president with GetAvailableCompositionsFilter
Prerequisites
- [ x] I have added steps to test this contribution in the description below
Description
For reference: please see videos of the original bug here: https://github.com/umbraco/Umbraco-CMS/issues/15608
On a fresh install setup a new editor user and create a simple multi root node structure:
For the created user setup a new group:
- Content start node to be on of the root nodes
- Restrict the permission so only Sort is enabled in the Structure settings.
Set 'Granular permissions' for the start node as:
Assign this group to the created editor user.
Sign in as the editor user.
The Content should be restricted as expected.
The user should now be able to sort the nodes
Hi there @AdamPrendergast, thank you for this contribution! 👍
While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
- It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
- The automated tests all pass (see "Checks" tab on this PR)
- The level of security for this contribution is the same or improved
- The level of performance for this contribution is the same or improved
- Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
- If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
- [x] 💡 The contribution looks original and the contributor is presumably allowed to share it
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.
If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.
Thanks, from your friendly Umbraco GitHub bot 🤖 🙂
Hi @AdamPrendergast,
Thanks for your PR and for the detailed explanations on how to test this 👍 Someone from the core collaborators team will have a look at it soon.
Great, thanks @mikecp
Happy to implement any feedback or take a different approach if desired.
Thanks for the feedback @nul800sebastiaan
I had approached this thinking that that Content Controller was only used by the Backoffice code itself. Thanks for pointing out that it is indeed a public API - which does change matters.
I might have to go back to the drawing board with this one to work out if there is an 'elegant-ish' way of handling the permission value that is set in the FilterAllowedOutgoingContent
attribute.
If it is not possible in a way the is maintainable/sensible, I wonder if we just need to stipulate that in order to 'sort' nodes, a user requires 'browse' permission (which makes sense to me).
This could then be a documentation update, or even a message in the permission management section - just a thought.