Element picker: Add allowed element types configuration#23026
Element picker: Add allowed element types configuration#23026NguyenThuyLan wants to merge 14 commits into
Conversation
|
Claude finished @NguyenThuyLan's task in 6m 43s —— View job PR ReviewTarget: Adds an "Allow items of type" configuration option to the Element Picker property editor, restricting which element types can be selected. Touches the C# value editor (new
Critical
Important
Suggestions
Request ChangesThe Critical issue (value converter not updated) must be addressed — it causes a complete regression for anyone using the Element Picker in templates or the Delivery API after this PR ships. The data migration concern is also worth resolving before merging since this is a release candidate. The feature itself is well-structured: the backend validator, configuration model, and frontend components are cleanly implemented, and the unit tests cover the key scenarios. |
…8/feature/element-picker-configuration-allowed-types
… of https://github.com/umbraco/Umbraco-CMS into v18/feature/element-picker-configuration-allowed-types
|
Claude finished @NguyenThuyLan's task in 5m 17s —— View job Claude finished @NguyenThuyLan's task in 6m 43s —— View job PR ReviewTarget: Adds "Allow items of type" configuration to the Element Picker property editor: a backend
Important
Suggestions
Approved with Suggestions for improvementThe previous Critical issue (value converter regression) has been resolved, and the overall structure is clean. Please address the dead parameter and review the silent-bypass behaviour before merging. |
…8/feature/element-picker-configuration-allowed-types
AndyButland
left a comment
There was a problem hiding this comment.
I had a quick look over @NguyenThuyLan. I haven't tested as I wanted to give you some feedback that you'd have in the morning. Looks great so far, but I found a few things to suggest inline.
Don't forget also that we discussed on the Azure board story that we wanted to align the document picker with this and the media picker for validation. It's also missing the ability to select allowed types, so we should introduce that. And I think it makes sense to do so in this PR, so we can see it's all aligned and guarantee that it's released together (in C# the document picker is called ``ContentPickerPropertyEditor(with nestedContentPickerPropertyValueEditor`).
In the end we'd like to get the document, media and element picker all as aligned as we can in terms of features, validation and how they are handled client and server-side.
| IElement[] elements = _elementService.GetByIds(value).ToArray(); | ||
| scope.Complete(); | ||
|
|
||
| if (elements.Length != value.Length) |
There was a problem hiding this comment.
I don't know if we need this check do we? The media picker equivalent doesn't do it. I would say a missing element would simply contribute no type alias to the check. Also the invalidObjectType response isn't really reflecting what's failed in the check.
There was a problem hiding this comment.
This is to solve a problem found by Claude: the key doesn't exist in the database; GetByIds silently omits it from the result array — its type is never checked.
I have changed invalidObjectType to missingContent.
There was a problem hiding this comment.
OK, I guess that's reasonable. If we don't know what the ID is, we don't know what type it is, and therefore we can't validate that it's of that type. But in that case we should look to add it to the media and document (content) picker too, so they are aligned.
| { | ||
| _jsonSerializer = jsonSerializer; | ||
| Validators.Add(new ElementPickerValidatorRunner( | ||
| new AllowedTypeValidator(localizedTextService, elementService, coreScopeProvider))); |
There was a problem hiding this comment.
The media picker equivalent also has MinMaxValidator and StartNodeValidator. If the element picker supports those configurations, then we should also server-side validate them.
There was a problem hiding this comment.
Currently, Element Picker doesn't have min/max configurations. Is it possible to add this option to Element Picker?
There was a problem hiding this comment.
Yes, I think they are worth adding. In general we should look to align the document (content), media and element pickers.
Prerequisites
(Element Picker: Add setting "Allow items of type" #22851)
Description
Adds an "Allow items of type" configuration option to the Element Picker property editor, allowing editors to restrict which element types can be selected.
How to test