Name is not saved correctly when using variations by segment

[D-Inventor]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.8.2

Bug summary

When using custom variations using segments, it is noted that changes to the name of a specific variation is not saved and actually reverts back upon saving or saving and publishing. Changing the name of the bottom-most variation in the list seems to change the name for all segment variations.

Specifics

I followed the tutorial as closely as I could from here.
I had to make some changes to actually make it work, so I ended up with the following code to generate my segments:

internal class ContentSegmentRenderHandler : INotificationHandler<SendingContentNotification>
{
    private readonly ISegmentProvider _segmentProvider;
    private readonly IContentTypeService _contentTypeService;

    public ContentSegmentRenderHandler(ISegmentProvider segmentProvider, IContentTypeService contentTypeService)
    {
        _segmentProvider = segmentProvider;
        _contentTypeService = contentTypeService;
    }

    public void Handle(SendingContentNotification notification)
    {
        /* There are several rules for segmentation
         *  - A page has to be eligible for segmentation, which is decided by the website itself.
         *  - Any "original" version has no segment value
         *  - For each original version, we should have additional versions for each segment
         *  - A segment should be added if no version already exists on the content model
         */

        if (!_segmentProvider.IsEligible(notification.Content)) return;

        var segments = _segmentProvider.GetSegments();

        var contentType = _contentTypeService.Get(notification.Content.ContentTypeId.Value);
        if (contentType is null) return;

        var props = contentType.PropertyTypes.Where(p => p.VariesBySegment()).Select(p => p.Alias).ToHashSet();
        props.UnionWith(contentType.CompositionPropertyGroups.SelectMany(p => p.PropertyTypes).Where(p => p.VariesBySegment()).Select(p => p.Alias));

        if (!props.Any()) return;

        // The frontend depends on the order of the segments,
        //   therefore it is important to ensure a consistent order of all different variations.
        var newVariants = new List<ContentVariantDisplay>(notification.Content.Variants);
        newVariants = EnsureProperDisplayNames(newVariants, segments);
        newVariants = EnsureSegmentForEachVariant(newVariants, segments, props);
        notification.Content.Variants = newVariants
            .OrderBy(v => IsDefaultLanguage(v) ? 0 : 1)
            .ThenBy(v => IsDefaultSegment(v) ? 0 : 1)
            .ThenBy(v => v?.Language?.Name)
            .ThenBy(v => v.Segment)
            .ToList();
    }

    private static bool IsDefaultLanguage(ContentVariantDisplay variant) =>
        variant.Language == null || variant.Language.IsDefault;

    private static bool IsDefaultSegment(ContentVariantDisplay variant) => variant.Segment == null;

    private List<ContentVariantDisplay> EnsureSegmentForEachVariant(List<ContentVariantDisplay> newVariants, IEnumerable<Segment> segments, ISet<string> variantProps)
    {
        var additionalVariants = new List<ContentVariantDisplay>();

        // A variant is original if the value for segment is empty
        foreach (var variant in newVariants.Where(v => string.IsNullOrWhiteSpace(v.Segment)))
        {
            // A variant is the same if their language and segment are equal.
            foreach (var segment in segments.Where(s => !newVariants.Any(v => v.Language?.Id == variant.Language?.Id && v.Segment == s.Id)))
            {
                var newVariant = new ContentVariantDisplay
                {
                    AllowedActions = new List<string>(variant.AllowedActions),
                    DisplayName = segment.Name,
                    Language = variant.Language,
                    Segment = segment.Id,
                    State = ContentSavedState.NotCreated,
                    Tabs = variant.Tabs.Select(x => new Tab<ContentPropertyDisplay>
                    {
                        Alias = x.Alias,
                        IsActive = x.IsActive,
                        Expanded = x.Expanded,
                        Id = x.Id,
                        Key = x.Key,
                        Label = x.Label,
                        Type = x.Type,
                        Properties = x.Properties?.Select(p => new ContentPropertyDisplay
                        {
                            Alias = p.Alias,
                            Config = p.Config,
                            ConfigNullable = p.ConfigNullable,
                            Culture = p.Culture,
                            DataTypeKey = p.DataTypeKey,
                            Description = p.Description,
                            Editor = p.Editor,
                            HideLabel = p.HideLabel,
                            Id = p.Id,
                            IsSensitive = p.IsSensitive,
                            Label = p.Label,
                            LabelOnTop = p.LabelOnTop,
                            PropertyEditor = p.PropertyEditor,
                            Readonly = p.Readonly,
                            Segment = variantProps.Contains(p.Alias) ? segment.Id : null,
                            SupportsReadOnly = p.SupportsReadOnly,
                            Validation = p.Validation,
                            Value = variantProps.Contains(p.Alias) ? null : p.Value,
                            Variations = p.Variations,
                            View = p.View,
                        }),
                    })
                };

                additionalVariants.Add(newVariant);
            }
        }

        newVariants.AddRange(additionalVariants);
        return newVariants;
    }

    private List<ContentVariantDisplay> EnsureProperDisplayNames(List<ContentVariantDisplay> newVariants, IEnumerable<Segment> segments)
    {
        // Always assign the current name of the segment to the displayname to ensure that the frontend always shows an up-to-date name
        foreach(var variant in newVariants)
        {
            if (string.IsNullOrWhiteSpace(variant.Segment)) continue;
            var segment = segments.FirstOrDefault(s => s.Id == variant.Segment);
            if (segment is null) continue;

            variant.DisplayName = segment.Name;
        }

        return newVariants;
    }
}

Notice the ISegmentProvider, which is a simple service that returns a list of models like this:

public record Segment(string Name, string Id, bool Default = false);

It also decides which document types are elligible for segmentation. The following implementation of ISegmentProvider should suffice to reproduce the issue:

public interface ISegmentProvider
{
    IEnumerable<Segment> GetSegments();
    bool IsEligible(ContentItemDisplay contentItem);
}

public class TestSegmentProvider
    : ISegmentProvider
{
    public bool IsEligible(ContentItemDisplay contentItem) => true;

    public IEnumerable<Segment> GetSegments()
        => new []
        {
            new Segment("First Segment", "1111", true),
            new Segment("Second Segment", "2222", false),
            new Segment("Third Segment", "3333", false)
        }
}

A quick scan through the Umbraco source, revealed the following line of code:

contentSave.PersistedContent.SetCultureName(variant.Name, variant.Culture);

To be found here. This line indicates that segments are not considered while naming a content variation. Something worth looking into.

I have attempted to work around this issue by copying the name of the default variation to the new segments in the ContentSegmentRenderHandler, but doing so results in spotty behaviour in the frontend with segments.

Steps to reproduce

1. Create a document type and enable variation by segment.
   You need to make the checkbox visible for that first. I wrote some code for that in my blog. You can find a VariantsServerVariablesParser there. That code snippet should allow you to enable variation by segment on a document type
2. Implement segment generation. In the 'specifics' chapter of this issue, I provide code snippets to generate some segments.
3. Make an instance of the document type with variation by segment enabled, optionally enter some content and save and publish
4. Reload the page by pressing F5.
5. Check the variations in the top-right corner in the name input and create a new variation by segment. Also enter a name (preferably a different name than the default) and save and publish.
6. Notice that all variations always have the same name and notice that changing the name on some variations doesn't work and on other variations changes the name for all.

Expected result / actual result

I expect that each variation can have it's own name or that variations by segment have their name field replicated from the "default" and greyed out so I can't overwrite it. I expect the "default" variation to be leading when naming a document type.

---

This item has been added to our backlog AB#37563

[github-actions]

Hi there @D-Inventor!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[Migaroez]

Hey Dennis, this is an interesting one that will need some internal discussion. I will try to get back to you somewhere next week.

[nielslyngsoe]

Hi @D-Inventor

I would let you know that this problem has been resolved in newer versions, 16-17+

Segments is a slim feature in older versions and we are maturing on the ownership of the feature in latest versions which has resolved this problem. There is a few more things we like to improve and take care of in the Core. So I hope we will get your inputs when you start using the latest version.

Thanks in advance and since its resolved in a later version I will close this ticket.