-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Management API: Add endpoints, service and repository methods for retrieving the allowed parents for content types #21586
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
Conversation
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @NillasKA, I think this will work just fine. I had a few comments though, so will leave those, and when you've considered/updated I'll bring it down for some testing.
src/Umbraco.Cms.Api.Management/Controllers/DocumentType/AllowedParentsDocumentTypeController.cs
Outdated
Show resolved
Hide resolved
...mbraco.Cms.Api.Management/ViewModels/DocumentType/DocumentTypeAllowedParentsResponseModel.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Services/ContentTypeServiceBase{TRepository,TItem}.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs
Outdated
Show resolved
Hide resolved
|
@AndyButland I have implemented your requested changes. Please take note of the joined sql statement, my experience with joining is limited so there may a more effective way. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @NillasKA. I found a few other areas where I think we could improve further, so please see what you think on these comments.
I'd also add the following two general comments:
- When you have finished, please go to the Swagger UI and follow the link to the
swagger.jsonfile. Save the results over theOpenApi.jsonfile we keep in the solution, to ensure we have the latest representation of the API stored (this is used for generating the typed client used from the backoffice). - Look at the existing authorization integration tests we have for the management API endpoints, and add a couple more for the two endpoints you have added.
src/Umbraco.Core/Services/ContentTypeServiceBase{TRepository,TItem}.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Persistence/Repositories/IContentTypeRepositoryBase.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs
Outdated
Show resolved
Hide resolved
|
@AndyButland I have gone through the requested changes again, and improved upon them. The database query was a bit quirky, as i did not quite see a way to do this, without using aliases in the query. I instead made a sub query, which i spotted in other repositories, so i figured that was the way to go. In regards to the pagination, let me know if you'd like me to introduce it anyway, i decided not to, until you return with your response - My key point was that a content type realistically would not have enough parents, that pagination would be any concern. But let me know, and I'll add it regardless! |
…y and tests. Add a new status for the default implementation (NotImplemented felt more correct than NotFound). Updated inheritance in service layer so we maintain the NotFound behaviour for member types.
This is fine I would say. There is probably a way to do it with JOINs, that might be more performant with many rows - but for the amount of records we'll be working with here it's not going to make any difference. Even then it may make no difference, as the database optimizer will likely generate similar execution plans. And what you have is more readable.
I agree that realistically there's not a great deal of point in having paging on this endpoint, as you will always want everything. I was thinking really just for consistency we might have it - given e.g. GetAllowedChildrenAsync supports it. However that is returning full entities and not just keys, so I think you are right that we don't need to add it here. |
AndyButland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me now @NillasKA. My only doubt is on API alignment - in that we have:
GET /umbraco/management/api/v1/document-type/{id}/allowed-parents
That returns IDs only without paging.
And the very similar looking:
GET /umbraco/management/api/v1/document-type/{id}/allowed-children
That returns objects (i.e. not just ID, also name, description and icon), and does support paging.
For what we need this endpoint for, non-paging and just keys is sufficient and more performant, so I think we should go with what you have. But as I say, just a nagging doubt.
I suggest let's approve this from a BE perspective, but not merge it until the FE parts are also done - just to be sure that what is in place is right for what the backoffice implementation needs. The FE work can be done in this branch too, reviewed here, and then all merged together.
Description
This PR introduces two new endpoints, one for Media Types and another for Document Types.
Each of these endpoints returns a list of Document/Media type keys, these keys are the keys of the Document/Media types that allow the selected Document/Media type as a child. To use it, you input the key of a content type, that you wish to know the allowed parents of, then the endpoint returns the keys for the content types that allow your inputted key as a child.
Tests
I have made integration tests for this PR as well, to ensure the service keeps working. Please review them as well.