From e6c7ef8904cf2133699373606c00a17c6d6a42e1 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 4 Dec 2025 12:54:44 +0100 Subject: [PATCH] Segments: Only validate segment values for cultures they are defined for (closes #21029) (#21033) * Only validate segment values for cultures they are defined for. * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Integration test suppressions. * Remove previous implementation using ISegmentService and rely on values provided in the model to determine segments with cultures. * Omit null culture and remove passing but unrealistic tests. * Fixed nullability error. * Apply suggestions from code review Co-authored-by: Kenn Jacobsen * Relocated function following code review. * Reset unchanged files. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Kenn Jacobsen --- .../Services/ContentValidationServiceBase.cs | 71 +++++++-- .../CompatibilitySuppressions.xml | 18 ++- .../ContentEditingServiceTests.Validate.cs | 71 ++++++++- .../Services/ContentValidationServiceTests.cs | 148 ++++++++++++++++++ 4 files changed, 290 insertions(+), 18 deletions(-) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentValidationServiceTests.cs diff --git a/src/Umbraco.Core/Services/ContentValidationServiceBase.cs b/src/Umbraco.Core/Services/ContentValidationServiceBase.cs index 137962b84c..ba25c75530 100644 --- a/src/Umbraco.Core/Services/ContentValidationServiceBase.cs +++ b/src/Umbraco.Core/Services/ContentValidationServiceBase.cs @@ -50,7 +50,7 @@ internal abstract class ContentValidationServiceBase cultures = await GetCultureCodes(); } - // we don't have any managed segments, so we have to make do with the ones passed in the model + // We don't have managed segments, so we have to make do with the ones passed in the model. var segments = new string?[] { null } .Union(contentEditingModelBase.Variants @@ -105,21 +105,39 @@ internal abstract class ContentValidationServiceBase } } - foreach (IPropertyType propertyType in cultureAndSegmentVariantPropertyTypes) + if (cultureAndSegmentVariantPropertyTypes.Length > 0) { - foreach (var culture in cultures) - { - foreach (var segment in segments.DefaultIfEmpty(null)) - { - var validationContext = new PropertyValidationContext - { - Culture = culture, Segment = segment, CulturesBeingValidated = cultures, SegmentsBeingValidated = segments - }; + // Get a mapping of segments to their associated cultures based on the variants and properties provided in the model. + // Without managed segments again we need to rely on the model data. + Dictionary> segmentCultures = GetPopulatedSegmentCultures(contentEditingModelBase, cultures); - PropertyValueModel? propertyValueModel = contentEditingModelBase - .Properties - .FirstOrDefault(propertyValue => propertyValue.Alias == propertyType.Alias && propertyValue.Culture.InvariantEquals(culture) && propertyValue.Segment.InvariantEquals(segment)); - validationErrors.AddRange(ValidateProperty(propertyType, propertyValueModel, validationContext)); + foreach (IPropertyType propertyType in cultureAndSegmentVariantPropertyTypes) + { + foreach (var culture in cultures) + { + foreach (var segment in segments.DefaultIfEmpty(null)) + { + // Skip validation if the segment has cultures defined and the current culture is not included. + if (segment is not null && + segmentCultures.TryGetValue(segment, out HashSet? associatedCultures) && + associatedCultures.Contains(culture) is false) + { + continue; + } + + var validationContext = new PropertyValidationContext + { + Culture = culture, + Segment = segment, + CulturesBeingValidated = cultures, + SegmentsBeingValidated = segments, + }; + + PropertyValueModel? propertyValueModel = contentEditingModelBase + .Properties + .FirstOrDefault(propertyValue => propertyValue.Alias == propertyType.Alias && propertyValue.Culture.InvariantEquals(culture) && propertyValue.Segment.InvariantEquals(segment)); + validationErrors.AddRange(ValidateProperty(propertyType, propertyValueModel, validationContext)); + } } } } @@ -140,6 +158,31 @@ internal abstract class ContentValidationServiceBase private async Task GetCultureCodes() => (await _languageService.GetAllIsoCodesAsync()).ToArray(); + /// + /// Gets a dictionary of segments along with the cultures they are associated with. + /// + /// The content editing model. + /// The cultures to consider when finding associated cultures for each segment. + /// + /// A dictionary where the key is a unique segment from and the value is + /// the set of cultures that have at least one property defined for that segment in . + /// + /// + /// Internal to support unit testing. + /// + internal static Dictionary> GetPopulatedSegmentCultures(ContentEditingModelBase contentEditingModel, string[] cultures) + { + IEnumerable uniqueSegments = contentEditingModel.Variants.Select(variant => variant.Segment).WhereNotNull().Distinct(); + + return uniqueSegments.ToDictionary( + segment => segment, + segment => contentEditingModel.Properties + .Where(property => property.Segment.InvariantEquals(segment)) + .Where(property => property.Culture is not null && cultures.Contains(property.Culture)) + .Select(property => property.Culture!) + .ToHashSet()); + } + private IEnumerable ValidateProperty(IPropertyType propertyType, PropertyValueModel? propertyValueModel, PropertyValidationContext validationContext) { ValidationResult[] validationResults = _propertyValidationService diff --git a/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml b/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml index 3bf6ed53c9..d0c83d36f6 100644 --- a/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml +++ b/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml @@ -1,4 +1,4 @@ - + @@ -85,6 +85,13 @@ lib/net10.0/Umbraco.Tests.Integration.dll true + + CP0002 + M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Can_Validate_Valid_Variant_Content + lib/net10.0/Umbraco.Tests.Integration.dll + lib/net10.0/Umbraco.Tests.Integration.dll + true + CP0002 M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Relate(Umbraco.Cms.Core.Models.IContent,Umbraco.Cms.Core.Models.IContent) @@ -92,4 +99,11 @@ lib/net10.0/Umbraco.Tests.Integration.dll true - + + CP0002 + M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Will_Fail_Invalid_Variant_Content + lib/net10.0/Umbraco.Tests.Integration.dll + lib/net10.0/Umbraco.Tests.Integration.dll + true + + \ No newline at end of file diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentEditingServiceTests.Validate.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentEditingServiceTests.Validate.cs index 26ddddc089..ee7af0b3f4 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentEditingServiceTests.Validate.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentEditingServiceTests.Validate.cs @@ -3,9 +3,11 @@ using Umbraco.Cms.Core; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Services.OperationStatus; using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Builders.Extensions; +using Umbraco.Cms.Tests.Integration.Attributes; namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; @@ -60,7 +62,7 @@ public partial class ContentEditingServiceTests } [Test] - public async Task Can_Validate_Valid_Variant_Content() + public async Task Can_Validate_Valid_Culture_Variant_Content() { var content = await CreateCultureVariantContent(); @@ -85,7 +87,7 @@ public partial class ContentEditingServiceTests } [Test] - public async Task Will_Fail_Invalid_Variant_Content() + public async Task Will_Fail_Invalid_Culture_Variant_Content() { var content = await CreateCultureVariantContent(); @@ -111,6 +113,71 @@ public partial class ContentEditingServiceTests Assert.AreEqual("#validation_invalidNull", result.Result.ValidationErrors.Single(x => x.Alias == "variantTitle" && x.Culture == "da-DK").ErrorMessages[0]); } + [Test] + public async Task Can_Validate_Valid_Culture_And_Segment_Variant_Content() + { + var content = await CreateCultureAndSegmentVariantContent(ContentVariation.Culture); + + var validateContentUpdateModel = new ValidateContentUpdateModel + { + Properties = + [ + new PropertyValueModel { Alias = "invariantTitle", Value = "The updated invariant title" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English default segment title", Culture = "en-US" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish default segment title", Culture = "da-DK" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 1 title", Culture = "en-US", Segment = "seg-1" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 1 title", Culture = "da-DK", Segment = "seg-1" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 2 title", Culture = "en-US", Segment = "seg-2" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 2 title", Culture = "da-DK", Segment = "seg-2" } + ], + Variants = + [ + new VariantModel { Culture = "en-US", Segment = "seg-1", Name = "Updated English segment 1 Name" }, + new VariantModel { Culture = "da-DK", Segment = "seg-1", Name = "Updated Danish segment 1 Name" }, + new VariantModel { Culture = "en-US", Segment = "seg-2", Name = "Updated English segment 2 Name" }, + new VariantModel { Culture = "da-DK", Segment = "seg-2", Name = "Updated Danish segment 2 Name" } + + ], + }; + + Attempt result = await ContentEditingService.ValidateUpdateAsync(content.Key, validateContentUpdateModel, Constants.Security.SuperUserKey); + Assert.IsTrue(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.Success, result.Status); + } + + [Test] + public async Task Will_Fail_Invalid_Culture_And_Segment_Variant_Content() + { + var content = await CreateCultureAndSegmentVariantContent(ContentVariation.Culture); + + var validateContentUpdateModel = new ValidateContentUpdateModel + { + Properties = + [ + new PropertyValueModel { Alias = "invariantTitle", Value = "The updated invariant title" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English default segment title", Culture = "en-US" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish default segment title", Culture = "da-DK" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 1 title", Culture = "en-US", Segment = "seg-1" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 1 title", Culture = "da-DK", Segment = "seg-1" }, + new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 2 title", Culture = "en-US", Segment = "seg-2" }, + new PropertyValueModel { Alias = "variantTitle", Value = null, Culture = "da-DK", Segment = "seg-2" } + ], + Variants = + [ + new VariantModel { Culture = "en-US", Segment = "seg-1", Name = "Updated English segment 1 Name" }, + new VariantModel { Culture = "da-DK", Segment = "seg-1", Name = "Updated Danish segment 1 Name" }, + new VariantModel { Culture = "en-US", Segment = "seg-2", Name = "Updated English segment 2 Name" }, + new VariantModel { Culture = "da-DK", Segment = "seg-2", Name = "Updated Danish segment 2 Name" } + ], + }; + + Attempt result = await ContentEditingService.ValidateUpdateAsync(content.Key, validateContentUpdateModel, Constants.Security.SuperUserKey); + Assert.IsFalse(result.Success); + Assert.AreEqual(ContentEditingOperationStatus.PropertyValidationError, result.Status); + Assert.AreEqual(1, result.Result.ValidationErrors.Count()); + Assert.AreEqual("#validation_invalidNull", result.Result.ValidationErrors.Single(x => x.Alias == "variantTitle" && x.Culture == "da-DK" && x.Segment == "seg-2").ErrorMessages[0]); + } + [Test] public async Task Will_Succeed_For_Invalid_Variant_Content_Without_Access_To_Edited_Culture() { diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentValidationServiceTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentValidationServiceTests.cs new file mode 100644 index 0000000000..a521418d0b --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentValidationServiceTests.cs @@ -0,0 +1,148 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using NUnit.Framework; +using Umbraco.Cms.Core.Models.ContentEditing; +using Umbraco.Cms.Core.Services; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models.ContentEditing; + +[TestFixture] +public class ContentValidationServiceTests +{ + // Concrete implementation for testing the abstract base class. + private class TestContentEditingModel : ContentEditingModelBase + { + } + + [Test] + public void GetPopulatedSegmentCultures_ReturnsEmptyDictionary_WhenNoSegmentsInVariants() + { + var model = new TestContentEditingModel + { + Variants = + [ + new VariantModel { Name = "English", Culture = "en-US", Segment = null }, + new VariantModel { Name = "Danish", Culture = "da-DK", Segment = null } + ], + Properties = + [ + new PropertyValueModel { Alias = "title", Value = "Hello", Culture = "en-US", Segment = null } + ], + }; + + var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]); + + Assert.That(result, Is.Empty); + } + + [Test] + public void GetPopulatedSegmentCultures_ReturnsSegmentWithCultures_WhenPropertiesExistForSegment() + { + var model = new TestContentEditingModel + { + Variants = + [ + new VariantModel { Name = "English Default", Culture = "en-US", Segment = null }, + new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" }, + new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" } + ], + Properties = + [ + new PropertyValueModel { Alias = "title", Value = "Hello", Culture = "en-US", Segment = null }, + new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" }, + new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" } + ], + }; + + var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]); + + Assert.That(result, Has.Count.EqualTo(1)); + Assert.That(result.ContainsKey("segment-1"), Is.True); + Assert.That(result["segment-1"], Does.Contain("en-US")); + Assert.That(result["segment-1"], Does.Contain("da-DK")); + } + + [Test] + public void GetPopulatedSegmentCultures_ReturnsOnlyCulturesWithProperties_WhenSomeSegmentCultureCombinationsHaveNoProperties() + { + var model = new TestContentEditingModel + { + Variants = + [ + new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" }, + new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" } + ], + Properties = + [ + // Only en-US has a property for the mobile segment + new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" } + ], + }; + + var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]); + + Assert.That(result, Has.Count.EqualTo(1)); + Assert.That(result.ContainsKey("segment-1"), Is.True); + Assert.That(result["segment-1"], Does.Contain("en-US")); + Assert.That(result["segment-1"], Does.Not.Contain("da-DK")); + } + + [Test] + public void GetPopulatedSegmentCultures_ExcludesCulturesNotInCulturesParameter() + { + var model = new TestContentEditingModel + { + Variants = + [ + new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" }, + new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" }, + new VariantModel { Name = "German Segment 1", Culture = "de-DE", Segment = "segment-1" } + ], + Properties = + [ + new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" }, + new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" }, + new PropertyValueModel { Alias = "title", Value = "Hallo Segment 1", Culture = "de-DE", Segment = "segment-1" } + ], + }; + + // Only validating en-US and da-DK, not de-DE + var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]); + + Assert.That(result, Has.Count.EqualTo(1)); + Assert.That(result["segment-1"], Does.Contain("en-US")); + Assert.That(result["segment-1"], Does.Contain("da-DK")); + Assert.That(result["segment-1"], Does.Not.Contain("de-DE")); + } + + [Test] + public void GetPopulatedSegmentCultures_HandlesMultipleSegments() + { + var model = new TestContentEditingModel + { + Variants = + [ + new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" }, + new VariantModel { Name = "English Segment 2", Culture = "en-US", Segment = "segment-2" }, + new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" } + ], + Properties = + [ + new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" }, + new PropertyValueModel { Alias = "title", Value = "Hello Segment 2", Culture = "en-US", Segment = "segment-2" }, + new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" } + ], + }; + + var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]); + + Assert.That(result, Has.Count.EqualTo(2)); + Assert.That(result.ContainsKey("segment-1"), Is.True); + Assert.That(result.ContainsKey("segment-2"), Is.True); + Assert.That(result["segment-1"], Does.Contain("en-US")); + Assert.That(result["segment-1"], Does.Contain("da-DK")); + Assert.That(result["segment-2"], Does.Contain("en-US")); + Assert.That(result["segment-2"], Does.Not.Contain("da-DK")); + } +}