Skip to content

Elements: Cleanup element TODOs in infrastructure#22443

Merged
kjac merged 8 commits into
v18/devfrom
v18/task/clean-up-elements-todos-in-infrastructure
Apr 13, 2026
Merged

Elements: Cleanup element TODOs in infrastructure#22443
kjac merged 8 commits into
v18/devfrom
v18/task/clean-up-elements-todos-in-infrastructure

Conversation

@kjac

@kjac kjac commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Description

This is the infrastructure equivalent of #22399

No new added functionality to test here; visual inspection only.

@claude

claude Bot commented Apr 12, 2026

Copy link
Copy Markdown

PR Review

Target: origin/v18/dev - Based on commit: e1f88ce86e5fd77f7daaaa192ca914687bf77955

Resolves several TODO ELEMENTS markers: extracts CultureAndScheduleRequestModel / ScheduleRequestModel to ViewModels.Content, centralises publish-schedule validation into PublishableContentPresentationFactoryBase, and consolidates duplicated entity-builder code in ContentBaseFactory into a shared generic BuildPublishableEntity helper.

  • Modified public API: IElementPresentationFactory (new method); CultureAndScheduleRequestModel and ScheduleRequestModel namespace changed from ViewModels.Document to ViewModels.Content
  • Affected implementations (outside this PR): All external implementations of IElementPresentationFactory; external consumers referencing the old ViewModels.Document namespace for these two types
  • Other changes: Element entity dates (CreateDate, UpdateDate, PublishDate) now call .EnsureUtc() via the shared builder -- aligning element behaviour with documents (bug fix; no impact for UTC-stored dates)

Note

Complexity advisory -- This PR may benefit from splitting.

Layer spread: API Management (11 files) + Infrastructure (4 files) + Tests (1 file) = 3 layers, 16 files. The changes are tightly coupled so splitting may not be practical.

This is an observation, not a blocker. The full review follows below.


Important

  • src/Umbraco.Cms.Api.Management/Factories/IElementPresentationFactory.cs:52: CreateCulturePublishScheduleModels is added to a public interface without a default implementation. Any external implementation of IElementPresentationFactory will fail to compile. Per CLAUDE.md Pattern 3, a default implementation should be provided -- IDocumentPresentationFactory already sets this precedent with its equivalent method using StaticServiceProvider. See inline comment for a suggested default.

  • src/Umbraco.Cms.Api.Management/ViewModels/Document/PublishDocumentRequestModel.cs: CultureAndScheduleRequestModel and ScheduleRequestModel were public types in ViewModels.Document; removing them is a binary breaking change for consumers who import them from that namespace. If any v18 beta NuGet packages shipped these types there, keep [Obsolete] subclasses in the original namespace pointing to the new location. If v18 has had no public releases with these types, this can be dismissed.


Suggestions

  • src/Umbraco.Cms.Api.Management/Factories/PublishableContentPresentationFactoryBase.cs:131: New protected method CreateCulturePublishScheduleModels is missing an XML doc comment while all other members in the class have one.

Request Changes

The IElementPresentationFactory missing default is a concrete breaking-change violation per the project convention. Please address that and confirm (or address) the namespace move. Happy to take another look after!

@kjac

kjac commented Apr 12, 2026

Copy link
Copy Markdown
Contributor Author

The Claude review correctly points out breaking changes, and I have labelled the PR accordingly.

The breaking changes are OK here, though. We can make compile time breaking changes for a new major version, and I'd rather make the changes obvious now than try to hide them behind a default implementations and left-over shims.

@AndyButland AndyButland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again all looks good. I just found a few things to fix/consider.

Comment thread src/Umbraco.Cms.Api.Management/ViewModels/Content/ScheduleRequestModel.cs Outdated
Comment thread src/Umbraco.Cms.Api.Management/Factories/IElementPresentationFactory.cs Outdated
kjac and others added 2 commits April 13, 2026 07:21
…estModel.cs

Co-authored-by: Andy Butland <abutland73@gmail.com>
…actory.cs

Co-authored-by: Andy Butland <abutland73@gmail.com>
@kjac kjac merged commit 6c96ad1 into v18/dev Apr 13, 2026
31 of 32 checks passed
@kjac kjac deleted the v18/task/clean-up-elements-todos-in-infrastructure branch April 13, 2026 06:09
@kjac kjac added the release/no-notes Not directly part of the release (updating README, build scripts, tests, etc.) label Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/backend category/api category/breaking category/refactor release/no-notes Not directly part of the release (updating README, build scripts, tests, etc.) release/18.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants