Skip to content

Segments: Fix processing data for segments variants#21018

Merged
nielslyngsoe merged 7 commits intorelease/17.0from
v17/hotfix/presets-for-segments
Dec 2, 2025
Merged

Segments: Fix processing data for segments variants#21018
nielslyngsoe merged 7 commits intorelease/17.0from
v17/hotfix/presets-for-segments

Conversation

@nielslyngsoe
Copy link
Member

@nielslyngsoe nielslyngsoe commented Dec 1, 2025

Fixes an unreported issue regarding the data of Segmented Variants, where saved data was not loaded for Segment-variants.
(Internal reference: https://umbraco.slack.com/archives/C02JJQU9B/p1764248526131199 )

Replicate by saving a Segment-Variant, and reloading the document.

This PR stops loading Segments based on the Unique observable, to instead load and await the Segments as part of the data-processing.
This means we load the Segments before setting the data, having the Segments available for the data processing. aka. the value presets logic.

As part of that _loadSegmentsFor is introduced, deprecating the loadSegments. And stop using that by any of our code — leaving it just in case someone was using that based on the early v.17 releases. Only depreciating it for one major as it was never part of v.16. (introduced as part of #20340)

As well it surfaced that some places both called _scaffoldProcessData & _processIncomingData, one calling the other. so that got cleaned up as well, making it backwards compatible as the intention was for v.18.

Copilot AI review requested due to automatic review settings December 1, 2025 14:52
Copy link
Contributor

Copilot AI left a comment

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 refactors the data processing pipeline for workspace contexts to fix segment variant data loading. The main change consolidates how _processIncomingData is called through _scaffoldProcessData, ensuring segments are loaded when processing incoming data rather than reactively through observables. However, the implementation contains a critical bug where the wrong identifier is passed to the segment loading method.

Key Changes

  • Consolidated the data processing pipeline so both load() and createScaffold() operations consistently call _processIncomingData through _scaffoldProcessData
  • Moved segment loading from reactive observable-based approach to eagerly loading segments during data processing
  • Fixed a bug where getVariesByCulture() was incorrectly called instead of getVariesBySegment()

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
document-workspace.context.ts Refactored loadSegments() to _loadSegmentsFor() to accept entity unique as parameter instead of using reactive observable pattern
entity-detail-workspace-base.ts Modified _scaffoldProcessData() to call _processIncomingData(), consolidating the data processing pipeline and removing duplicate processing
content-detail-workspace-base.ts Added new _loadSegmentsFor() method, deprecated old loadSegments(), integrated segment loading into _processIncomingData(), and fixed getVariesBySegment() call
content-type-workspace-context-base.ts Removed duplicate _processIncomingData() call now that base class handles it through _scaffoldProcessData()

@nielslyngsoe nielslyngsoe enabled auto-merge (squash) December 1, 2025 15:00
@AndyButland AndyButland changed the title Segments: Fix for processing data for Segments-variants Segments: Fix processing data for segments variants Dec 2, 2025
@McGloud74
Copy link

This also seems to be an issue in 16.4.2, where I discovered this issue using Umbraco Engage, trying to implement an AB test.
I reported the issue to the Engage developers: umbraco/Umbraco.Engage.Issues#44, but they can't fix this and refer to the core team about this issue. Will this issue also be fixed in Umbraco 16?

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