Content Types: Page through all allowed content types rather than stopping at the first 100 (closes #23512) - #23522
Conversation
The Create dialogs, collection create actions, media dropzone and media picker only ever requested one page of allowed content types, so anything past the server's default take of 100 was silently unavailable. Page through the full set with the existing fetchAllPages helper, via a new additive requestAllAllowedChildrenOf. The existing requestAllowedChildrenOf gains an optional paging argument so a single page can still be requested. GetAllowedChildrenAsync applied Take before Skip, so any page after the first came back empty while Total reported the true count. This never surfaced while every caller requested only skip=0. Also page requestMediaTypesOfFolders and requestMediaTypesOf, which the dropzone and media picker intersect with allowed children, and which build the folder-type set that decides whether an item renders as a folder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @AndyButland's task in 10m 5s —— View job Code Review Complete
Summary: No critical issues. One important gap (no TS test for |
|
PR Review Target: origin/v17/dev | Based on commit: 4cc8d53 Fixes two independent bugs causing the Create dialog to silently truncate at 100 allowed types: an inverted Skip/Take in ContentTypeServiceBase.GetAllowedChildrenAsync (zero items returned for any skip > 0), and client-side repository methods that never sent paging parameters. Both sides are corrected with automated tests. Modified public API:
Affected implementations (outside this PR): Third-party classes implementing UmbContentTypeStructureRepository or UmbContentTypeStructureDataSource (both exported from @umbraco-cms/backoffice/content-type). Changes are additive so no compile-time break. Other changes: MSW mock handlers now respect skip/take query params - any Storybook story relying on >100 items without explicit pagination will now only receive the first page. Critical None. Important media-type-structure.repository.ts:63 - No TypeScript test for the requestMediaTypesOf/requestMediaTypesOfFolders conditional paging logic. Both are publicly exported from @umbraco-cms/backoffice/media-type and their new fetch-all-vs-single-page branching is untested. document-type-structure.repository.test.ts covers only the base-class path. A test (no args two paged requests for 150 items; explicit skip/take one request) would give the same fail-before guarantee the C# tests have. Suggestions ContentTypeServiceTests.cs:2723 - Can_Get_First_Page_Of_Allowed_Children (skip=0) passes regardless of the bug since Take().Skip() and Skip().Take() are equivalent at skip=0. A short inline comment would preserve this rationale in the test file without requiring a PR description lookup. mocks/utils.ts:67 - The || operator in Number(url.searchParams.get('skip')) || 0 treats numeric 0 as falsy, so the default applies by coincidence for skip=0. Null-coalescing before Number() communicates intent more clearly. Same for the take line. Approved with Suggestions for improvement Solid, well-scoped fix on both sides. The C# one-liner is exactly right, the TypeScript paging approach follows the established fetchAllPages pattern, and the test suite directly verifies the specific failure modes described. The only meaningful gap is TS-side coverage for UmbMediaTypeStructureRepository's conditional paging logic. |
The "explicit skip/take means one page, otherwise fetch all" branch in requestMediaTypesOf and requestMediaTypesOfFolders was the novel logic in this PR and had no test. Also honour an explicit skip=0/take=0 in the mock paging helper rather than relying on 0 being falsy, while still falling back on an unparseable value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The paged mock endpoints keep their paging in the db/manager layer and hand skip/take down from the handler, with pagedResult doing the slice. The allowed-children and allowed-at-root db methods now do the same, rather than collecting everything and re-slicing it in the handler through a helper that duplicated pagedResult. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
nikolajlauridsen
left a comment
There was a problem hiding this comment.
Backend changes looks good.
Also did all the manual tests, and they all looked great 👍



Description
A Document Type that allows more than 100 child Document Types only offered the first 100 in the Content section's Create dialog. Perhaps an edge case setup, but we should support retrieving all, similar to what we did recently for languages in #22765.
Fixes #23512
There were three separate issues to resolve.
1. The client never paged (the user-visible cause)
requestAllowedChildrenOfhad no paging parameters anywhere in the chain — data source, interface or repository. It fired a single request with noskip/take, so the server applied its defaulttakeof 100, and every one of the 8 consumers readdata.itemswhile discarding thetotalthe server correctly reported. Two of them carried a// TODO: implement pagination, or get 1000?.Fixed by paging through the full set with the
fetchAllPageshelper added in #22765 for the >100 languages case. This is additive, mirroring that PR'srequestCollection/requestAllItemssplit:requestAllAllowedChildrenOf(new) pages until every item is retrieved — all in-repo callers now use it.requestAllowedChildrenOfgains an optionalpagingargument, so a single page can be requested. Its existing behaviour is unchanged for external consumers.The
paging?: UmbOffsetPaginationRequestModelshape matchesUmbTreeChildrenOfRequestArgsand the search args rather than reintroducing bareskip/take, which were deprecated in 16.3.This affects allowed at root as much as allowed children, and needs resolving for every content type with an allowed-children/allowed-at-root endpoint — document, media and member types. Members need no configuration to hit it: every member type is a root candidate.
2. The server paged incorrectly (a latent prerequisite)
ContentTypeServiceBase.GetAllowedChildrenAsyncapplied.Take(take).Skip(skip)— inverted. Atskip=0that is harmless, which is why it went unnoticed; atskip=100, take=100it returned zero items whileTotalstill reported the true count. Client paging could not work until this was corrected.3. Media-type siblings, same failure class
requestMediaTypesOfFoldersandrequestMediaTypesOfhardcodedtake = 100against paged endpoints. The dropzone and the media picker's folder path intersect those lists with allowed children, so fixing only the allowed-children side would still have dropped valid options.Testing
Automated
Automated coverage added client and server; solution should build and CI checks pass.
Manual
The page size is a constant, so temporarily lowering it means we need 6 Document Types rather than 101.
Setup — temporarily set the page size to 5:
src/Umbraco.Web.UI.Client/src/packages/content/content-type/repository/structure/content-type-structure-repository-base.ts, change:src/Umbraco.Web.UI.Client/src/packages/media/media-types/repository/structure/media-type-structure.repository.ts:allowed.While the page size is lowered, four of the client unit tests fail (
expected [ Array(30) ] to deeply equal [ Array(2) ]) — they assert two requests at the real page size of 100, and 150 mock items at a page size of 5 is 30 requests. Expected, and resolved by the teardown step.Server-side check (needs no Document Types at all beyond a parent with 6 children). In Swagger, call
GET /umbraco/management/api/v1/document-type/{id}/allowed-children?skip=5&take=5. It returns the remaining item(s) with the fulltotal. Onv17/devtoday the same call returns an emptyitemsarray with a non-zerototal— that is defect 2.Allowed children:
skip=0&take=5thenskip=5&take=5.Allowed at root:
allowed-at-rootrequests.Members (no configuration needed — every member type is a root candidate):
Media:
Collection views: with the parent type allowing 6 children, open its collection (list view) and confirm the Create dropdown offers all 6.
Teardown: revert both page-size constants to
100.