Skip to content

Content Editing: Fix save composition values on invariant content and save of default segment (closes #22800, #22865)#22846

Merged
AndyButland merged 6 commits into
v17/devfrom
v17/bugfix/22800-save-value-from-composition
May 20, 2026
Merged

Content Editing: Fix save composition values on invariant content and save of default segment (closes #22800, #22865)#22846
AndyButland merged 6 commits into
v17/devfrom
v17/bugfix/22800-save-value-from-composition

Conversation

@AndyButland

@AndyButland AndyButland commented May 14, 2026

Copy link
Copy Markdown
Contributor

Description

This PR combines two fixes that are related as coming from #22621 (Document Workspace: Add CRUD and property value tests for document workspace context, merged 2026-05-07). That PR added a guard in UmbContentDetailWorkspaceContextBase.setPropertyValue (src/Umbraco.Web.UI.Client/src/packages/content/content/workspace/content-detail-workspace-base.ts) which throws when a caller tries to write a value for a culture- or segment-variant property without supplying a corresponding variantId. The guard was too broad and caused two distinct user-visible regressions.

Fixes #22800 and fixes #22865.

#22800 — invariant content type with a variant composition

When an invariant content type uses a composition whose property type is culture- or segment-variant, typing a value into that field and clicking Save silently discarded the value and left the previous value in place — with an unhandled promise rejection in the browser console. The guard inspected only the property type's variance and ignored the host content type's variance.

Fix: intersect the property's variance with the content type's variance. A variant property on an invariant content type is treated as invariant, so the guard no longer rejects the write.

#22865 — default segment values for segment-variant properties

For a property that varies by segment, values entered for the default segment were never saved (omitted from the save/validate payload). Values for other segments saved fine. The guard rejected the default segment because UmbVariantId.isSegmentInvariant() returns true when segment === null — but unlike culture, a null segment is the default segment, a legitimate bucket, not a missing value.

Fix: drop the segment branch of the guard entirely. A null segment is always a valid value for a segment-variant property, so there is no useful safety check to express there. The culture branch is retained because a null culture genuinely is invalid on a culture-variant content type.

The first bug was reported as 18.1+ but both are present on v17/dev, and so on release/17.5.0, release/18.0, and main as well. Hence, when approved, this should be cherry-picked or merged into those branches.

Testing

Automated

The unit test suite added in #22621 is extended with:

All tests have been verified to fail before and pass after the fix is in place.

Manual

For #22800:

  1. Create a Document Type Composition that varies by culture, with one textstring property.
  2. Create a Document Type Host that does not vary by culture, with Composition added as a composition.
  3. Create a content item of type Host, type a value into the inherited property, click Save.

Before the fix: value reverts; no toast; an unhandled rejection appears in the browser console:

content-detail-workspace-base.ts:693 Uncaught (in promise) Error: Property alias "message" requires a variantId.
    at setPropertyValue (content-detail-workspace-base.ts:693:11)
    at async setPropertyValue (element-property-dataset.context.ts:213:4)

After the fix: value is persisted.

For #22865:

  1. Register a custom ISegmentService exposing one or more segments (see the reproduction code in #22865) and set SegmentSettings.Enabled = true.
  2. Create a Document Type with a property that varies by segment.
  3. Create a content item, type a value into the default segment field, click Save.

Before the fix: the default-segment value is omitted from the save/validate network payload and not persisted.

After the fix: the default-segment value is included in the payload and persisted alongside any named-segment values.

Copilot AI review requested due to automatic review settings May 14, 2026 11:09
@claude

claude Bot commented May 14, 2026

Copy link
Copy Markdown

Claude finished @AndyButland's task in 3m 24s —— View job


PR Review

Target: origin/v17/dev · Based on commit: 1786310d37d045d9105c3277799c86da04bebf08

Targeted bugfix that corrects the setPropertyValue guard in UmbContentDetailWorkspaceContextBase to check the host content type's effective variance, not just the property type's. Fixes silent value loss when an invariant document inherits a culture-variant property via composition.


Suggestions

  • content-detail-workspace-base.ts:689-691: The 3-line block comment violates the project's one-line-max policy for inline comments (CLAUDE.md: "Never write multi-line comment blocks — one short line max"). The "why" can be preserved in a single compact line, e.g.:

    // Effective variance is the intersection: a variant property on an invariant content type is treated as invariant.
  • document-workspace-property-values.context.test.ts: No unit test is added for the specific regression scenario — invariant document type with a property that variesByCulture: true (via composition) should succeed in setPropertyValue with an invariant variantId. The existing acceptance test can save property value from variant composition in invariant content in ContentWithVariantComposition.spec.ts covers the happy path end-to-end, but a companion unit test alongside the existing does not create a phantom invariant entry… test (line 137) would give faster, focused regression coverage. Fix this →


Approved with Suggestions for improvement

The logic is correct — intersecting the property's variance with the content type's variance is the right model, and the ?? false fallback is safe given the workspace loading sequence. The existing guard for genuine variant documents (variant doc + culture-variant property + no culture supplied) still fires correctly. Good fix.

Copilot AI 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.

Pull request overview

This PR fixes saving property values for invariant content types that inherit variant properties through compositions, aligning workspace validation with the effective variance of the host content type.

Changes:

  • Updates setPropertyValue to require a variant ID only when both the property and host content type vary by culture or segment.
  • Documents the effective-variance behavior for variant composition properties on invariant content.

@claude claude Bot added the area/frontend label May 14, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndyButland AndyButland changed the title Content: Save composition values on invariant content (closes #22800) Content Editing: Save composition values on invariant content (closes #22800) May 14, 2026
AndyButland and others added 3 commits May 14, 2026 13:19
…omposition property.

Adds a third mock document/document-type pair representing an invariant
content type whose flattened property list contains a culture-variant
property (the runtime shape produced when a variant composition is applied
to an invariant content type) and a setPropertyValue test asserting the
value is stored as a culture/segment-invariant entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndyButland AndyButland changed the title Content Editing: Save composition values on invariant content (closes #22800) Content Editing: Fix save composition values on invariant content and save of default segment (closes #22800, 22865) May 18, 2026
@AndyButland AndyButland changed the title Content Editing: Fix save composition values on invariant content and save of default segment (closes #22800, 22865) Content Editing: Fix save composition values on invariant content and save of default segment (closes #22800, #22865) May 18, 2026
@madsrasmussen madsrasmussen self-assigned this May 19, 2026
@madsrasmussen

madsrasmussen commented May 20, 2026

Copy link
Copy Markdown
Member

The fix looks good 👍

I noticed that the mock data was set up incorrectly. It didn’t use compositions; it just named the Doc Types something with “Composition”. I have pushed an update to the PR that updates the mock data with invariant Doc Types that compose properties from culture and segment variant Doc
Types.

This is how the mock data look in the UI.

You can visually test the mock data by running npm run dev:mock and then choose a mock set in the top right corner.

The new Doc Types are within the "Composed"-folder:
Screenshot 2026-05-20 at 10 04 20

I have added two new Documents that are based on these Doc Types and are used in the tests:
Screenshot 2026-05-20 at 10 04 48

If I revert your bug fix, I now see five failing tests that describe the two bugs you fixed.

I have approved the PR, but can I get you to sanity check the tests?

@AndyButland

Copy link
Copy Markdown
Contributor Author

Thanks @madsrasmussen (and for the tips on visualising the mocked data). What you've set up looks correct to me, and as you say, work as verification of the fix.

@AndyButland AndyButland merged commit 8d5826c into v17/dev May 20, 2026
30 checks passed
@AndyButland AndyButland deleted the v17/bugfix/22800-save-value-from-composition branch May 20, 2026 08:43
AndyButland added a commit that referenced this pull request May 20, 2026
… save of default segment (closes #22800, #22865) (#22846)

* Fix edit of a variant property composed to an invariant document.

* Collapse multi-line guard comment to a single line per project policy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Add unit test coverage for invariant content with a culture-variant composition property.

Adds a third mock document/document-type pair representing an invariant
content type whose flattened property list contains a culture-variant
property (the runtime shape produced when a variant composition is applied
to an invariant content type) and a setPropertyValue test asserting the
value is stored as a culture/segment-invariant entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Remove null guard for segment variant documents, as null segment is the default segment.

* update mock data and tests to include real compositions

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Mads Rasmussen <madsr@hey.com>
@AndyButland

AndyButland commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Cherry picked to release/17.5.0 and release/18.0.

AndyButland added a commit that referenced this pull request May 20, 2026
… save of default segment (closes #22800, #22865) (#22846)

* Fix edit of a variant property composed to an invariant document.

* Collapse multi-line guard comment to a single line per project policy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Add unit test coverage for invariant content with a culture-variant composition property.

Adds a third mock document/document-type pair representing an invariant
content type whose flattened property list contains a culture-variant
property (the runtime shape produced when a variant composition is applied
to an invariant content type) and a setPropertyValue test asserting the
value is stored as a culture/segment-invariant entry.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Remove null guard for segment variant documents, as null segment is the default segment.

* update mock data and tests to include real compositions

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Mads Rasmussen <madsr@hey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants