Skip to content

Commit e6c7ef8

Browse files
AndyButlandCopilotkjac
authored
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 <kja@umbraco.dk> * Relocated function following code review. * Reset unchanged files. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
1 parent effccef commit e6c7ef8

File tree

4 files changed

+288
-16
lines changed

4 files changed

+288
-16
lines changed

‎src/Umbraco.Core/Services/ContentValidationServiceBase.cs‎

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ protected async Task<ContentValidationResult> HandlePropertiesValidationAsync(
5050
cultures = await GetCultureCodes();
5151
}
5252

53-
// we don't have any managed segments, so we have to make do with the ones passed in the model
53+
// We don't have managed segments, so we have to make do with the ones passed in the model.
5454
var segments =
5555
new string?[] { null }
5656
.Union(contentEditingModelBase.Variants
@@ -105,21 +105,39 @@ protected async Task<ContentValidationResult> HandlePropertiesValidationAsync(
105105
}
106106
}
107107

108-
foreach (IPropertyType propertyType in cultureAndSegmentVariantPropertyTypes)
108+
if (cultureAndSegmentVariantPropertyTypes.Length > 0)
109109
{
110-
foreach (var culture in cultures)
110+
// Get a mapping of segments to their associated cultures based on the variants and properties provided in the model.
111+
// Without managed segments again we need to rely on the model data.
112+
Dictionary<string, HashSet<string>> segmentCultures = GetPopulatedSegmentCultures(contentEditingModelBase, cultures);
113+
114+
foreach (IPropertyType propertyType in cultureAndSegmentVariantPropertyTypes)
111115
{
112-
foreach (var segment in segments.DefaultIfEmpty(null))
116+
foreach (var culture in cultures)
113117
{
114-
var validationContext = new PropertyValidationContext
118+
foreach (var segment in segments.DefaultIfEmpty(null))
115119
{
116-
Culture = culture, Segment = segment, CulturesBeingValidated = cultures, SegmentsBeingValidated = segments
117-
};
118-
119-
PropertyValueModel? propertyValueModel = contentEditingModelBase
120-
.Properties
121-
.FirstOrDefault(propertyValue => propertyValue.Alias == propertyType.Alias && propertyValue.Culture.InvariantEquals(culture) && propertyValue.Segment.InvariantEquals(segment));
122-
validationErrors.AddRange(ValidateProperty(propertyType, propertyValueModel, validationContext));
120+
// Skip validation if the segment has cultures defined and the current culture is not included.
121+
if (segment is not null &&
122+
segmentCultures.TryGetValue(segment, out HashSet<string>? associatedCultures) &&
123+
associatedCultures.Contains(culture) is false)
124+
{
125+
continue;
126+
}
127+
128+
var validationContext = new PropertyValidationContext
129+
{
130+
Culture = culture,
131+
Segment = segment,
132+
CulturesBeingValidated = cultures,
133+
SegmentsBeingValidated = segments,
134+
};
135+
136+
PropertyValueModel? propertyValueModel = contentEditingModelBase
137+
.Properties
138+
.FirstOrDefault(propertyValue => propertyValue.Alias == propertyType.Alias && propertyValue.Culture.InvariantEquals(culture) && propertyValue.Segment.InvariantEquals(segment));
139+
validationErrors.AddRange(ValidateProperty(propertyType, propertyValueModel, validationContext));
140+
}
123141
}
124142
}
125143
}
@@ -140,6 +158,31 @@ public async Task<bool> ValidateCulturesAsync(ContentEditingModelBase contentEdi
140158

141159
private async Task<string[]> GetCultureCodes() => (await _languageService.GetAllIsoCodesAsync()).ToArray();
142160

161+
/// <summary>
162+
/// Gets a dictionary of segments along with the cultures they are associated with.
163+
/// </summary>
164+
/// <param name="contentEditingModel">The content editing model.</param>
165+
/// <param name="cultures">The cultures to consider when finding associated cultures for each segment.</param>
166+
/// <returns>
167+
/// A dictionary where the key is a unique segment from <see cref="ContentEditingModelBase.Variants"/> and the value is
168+
/// the set of cultures that have at least one property defined for that segment in <see cref="ContentEditingModelBase.Properties"/>.
169+
/// </returns>
170+
/// <remarks>
171+
/// Internal to support unit testing.
172+
/// </remarks>
173+
internal static Dictionary<string, HashSet<string>> GetPopulatedSegmentCultures(ContentEditingModelBase contentEditingModel, string[] cultures)
174+
{
175+
IEnumerable<string> uniqueSegments = contentEditingModel.Variants.Select(variant => variant.Segment).WhereNotNull().Distinct();
176+
177+
return uniqueSegments.ToDictionary(
178+
segment => segment,
179+
segment => contentEditingModel.Properties
180+
.Where(property => property.Segment.InvariantEquals(segment))
181+
.Where(property => property.Culture is not null && cultures.Contains(property.Culture))
182+
.Select(property => property.Culture!)
183+
.ToHashSet());
184+
}
185+
143186
private IEnumerable<PropertyValidationError> ValidateProperty(IPropertyType propertyType, PropertyValueModel? propertyValueModel, PropertyValidationContext validationContext)
144187
{
145188
ValidationResult[] validationResults = _propertyValidationService

‎tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<?xml version="1.0" encoding="utf-8"?>
1+
<?xml version="1.0" encoding="utf-8"?>
22
<!-- https://learn.microsoft.com/dotnet/fundamentals/package-validation/diagnostic-ids -->
33
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
44
<Suppression>
@@ -85,11 +85,25 @@
8585
<Right>lib/net10.0/Umbraco.Tests.Integration.dll</Right>
8686
<IsBaselineSuppression>true</IsBaselineSuppression>
8787
</Suppression>
88+
<Suppression>
89+
<DiagnosticId>CP0002</DiagnosticId>
90+
<Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Can_Validate_Valid_Variant_Content</Target>
91+
<Left>lib/net10.0/Umbraco.Tests.Integration.dll</Left>
92+
<Right>lib/net10.0/Umbraco.Tests.Integration.dll</Right>
93+
<IsBaselineSuppression>true</IsBaselineSuppression>
94+
</Suppression>
8895
<Suppression>
8996
<DiagnosticId>CP0002</DiagnosticId>
9097
<Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Relate(Umbraco.Cms.Core.Models.IContent,Umbraco.Cms.Core.Models.IContent)</Target>
9198
<Left>lib/net10.0/Umbraco.Tests.Integration.dll</Left>
9299
<Right>lib/net10.0/Umbraco.Tests.Integration.dll</Right>
93100
<IsBaselineSuppression>true</IsBaselineSuppression>
94101
</Suppression>
95-
</Suppressions>
102+
<Suppression>
103+
<DiagnosticId>CP0002</DiagnosticId>
104+
<Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.ContentEditingServiceTests.Will_Fail_Invalid_Variant_Content</Target>
105+
<Left>lib/net10.0/Umbraco.Tests.Integration.dll</Left>
106+
<Right>lib/net10.0/Umbraco.Tests.Integration.dll</Right>
107+
<IsBaselineSuppression>true</IsBaselineSuppression>
108+
</Suppression>
109+
</Suppressions>

‎tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentEditingServiceTests.Validate.cs‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
using Umbraco.Cms.Core.Models;
44
using Umbraco.Cms.Core.Models.ContentEditing;
55
using Umbraco.Cms.Core.Models.Membership;
6+
using Umbraco.Cms.Core.Services;
67
using Umbraco.Cms.Core.Services.OperationStatus;
78
using Umbraco.Cms.Tests.Common.Builders;
89
using Umbraco.Cms.Tests.Common.Builders.Extensions;
10+
using Umbraco.Cms.Tests.Integration.Attributes;
911

1012
namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services;
1113

@@ -60,7 +62,7 @@ public async Task Will_Fail_Invalid_Invariant_Content()
6062
}
6163

6264
[Test]
63-
public async Task Can_Validate_Valid_Variant_Content()
65+
public async Task Can_Validate_Valid_Culture_Variant_Content()
6466
{
6567
var content = await CreateCultureVariantContent();
6668

@@ -85,7 +87,7 @@ public async Task Can_Validate_Valid_Variant_Content()
8587
}
8688

8789
[Test]
88-
public async Task Will_Fail_Invalid_Variant_Content()
90+
public async Task Will_Fail_Invalid_Culture_Variant_Content()
8991
{
9092
var content = await CreateCultureVariantContent();
9193

@@ -111,6 +113,71 @@ public async Task Will_Fail_Invalid_Variant_Content()
111113
Assert.AreEqual("#validation_invalidNull", result.Result.ValidationErrors.Single(x => x.Alias == "variantTitle" && x.Culture == "da-DK").ErrorMessages[0]);
112114
}
113115

116+
[Test]
117+
public async Task Can_Validate_Valid_Culture_And_Segment_Variant_Content()
118+
{
119+
var content = await CreateCultureAndSegmentVariantContent(ContentVariation.Culture);
120+
121+
var validateContentUpdateModel = new ValidateContentUpdateModel
122+
{
123+
Properties =
124+
[
125+
new PropertyValueModel { Alias = "invariantTitle", Value = "The updated invariant title" },
126+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English default segment title", Culture = "en-US" },
127+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish default segment title", Culture = "da-DK" },
128+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 1 title", Culture = "en-US", Segment = "seg-1" },
129+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 1 title", Culture = "da-DK", Segment = "seg-1" },
130+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 2 title", Culture = "en-US", Segment = "seg-2" },
131+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 2 title", Culture = "da-DK", Segment = "seg-2" }
132+
],
133+
Variants =
134+
[
135+
new VariantModel { Culture = "en-US", Segment = "seg-1", Name = "Updated English segment 1 Name" },
136+
new VariantModel { Culture = "da-DK", Segment = "seg-1", Name = "Updated Danish segment 1 Name" },
137+
new VariantModel { Culture = "en-US", Segment = "seg-2", Name = "Updated English segment 2 Name" },
138+
new VariantModel { Culture = "da-DK", Segment = "seg-2", Name = "Updated Danish segment 2 Name" }
139+
140+
],
141+
};
142+
143+
Attempt<ContentValidationResult, ContentEditingOperationStatus> result = await ContentEditingService.ValidateUpdateAsync(content.Key, validateContentUpdateModel, Constants.Security.SuperUserKey);
144+
Assert.IsTrue(result.Success);
145+
Assert.AreEqual(ContentEditingOperationStatus.Success, result.Status);
146+
}
147+
148+
[Test]
149+
public async Task Will_Fail_Invalid_Culture_And_Segment_Variant_Content()
150+
{
151+
var content = await CreateCultureAndSegmentVariantContent(ContentVariation.Culture);
152+
153+
var validateContentUpdateModel = new ValidateContentUpdateModel
154+
{
155+
Properties =
156+
[
157+
new PropertyValueModel { Alias = "invariantTitle", Value = "The updated invariant title" },
158+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English default segment title", Culture = "en-US" },
159+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish default segment title", Culture = "da-DK" },
160+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 1 title", Culture = "en-US", Segment = "seg-1" },
161+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated Danish segment 1 title", Culture = "da-DK", Segment = "seg-1" },
162+
new PropertyValueModel { Alias = "variantTitle", Value = "The updated English segment 2 title", Culture = "en-US", Segment = "seg-2" },
163+
new PropertyValueModel { Alias = "variantTitle", Value = null, Culture = "da-DK", Segment = "seg-2" }
164+
],
165+
Variants =
166+
[
167+
new VariantModel { Culture = "en-US", Segment = "seg-1", Name = "Updated English segment 1 Name" },
168+
new VariantModel { Culture = "da-DK", Segment = "seg-1", Name = "Updated Danish segment 1 Name" },
169+
new VariantModel { Culture = "en-US", Segment = "seg-2", Name = "Updated English segment 2 Name" },
170+
new VariantModel { Culture = "da-DK", Segment = "seg-2", Name = "Updated Danish segment 2 Name" }
171+
],
172+
};
173+
174+
Attempt<ContentValidationResult, ContentEditingOperationStatus> result = await ContentEditingService.ValidateUpdateAsync(content.Key, validateContentUpdateModel, Constants.Security.SuperUserKey);
175+
Assert.IsFalse(result.Success);
176+
Assert.AreEqual(ContentEditingOperationStatus.PropertyValidationError, result.Status);
177+
Assert.AreEqual(1, result.Result.ValidationErrors.Count());
178+
Assert.AreEqual("#validation_invalidNull", result.Result.ValidationErrors.Single(x => x.Alias == "variantTitle" && x.Culture == "da-DK" && x.Segment == "seg-2").ErrorMessages[0]);
179+
}
180+
114181
[Test]
115182
public async Task Will_Succeed_For_Invalid_Variant_Content_Without_Access_To_Edited_Culture()
116183
{

‎tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentValidationServiceTests.cs‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
// Copyright (c) Umbraco.
2+
// See LICENSE for more details.
3+
4+
using NUnit.Framework;
5+
using Umbraco.Cms.Core.Models.ContentEditing;
6+
using Umbraco.Cms.Core.Services;
7+
8+
namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models.ContentEditing;
9+
10+
[TestFixture]
11+
public class ContentValidationServiceTests
12+
{
13+
// Concrete implementation for testing the abstract base class.
14+
private class TestContentEditingModel : ContentEditingModelBase
15+
{
16+
}
17+
18+
[Test]
19+
public void GetPopulatedSegmentCultures_ReturnsEmptyDictionary_WhenNoSegmentsInVariants()
20+
{
21+
var model = new TestContentEditingModel
22+
{
23+
Variants =
24+
[
25+
new VariantModel { Name = "English", Culture = "en-US", Segment = null },
26+
new VariantModel { Name = "Danish", Culture = "da-DK", Segment = null }
27+
],
28+
Properties =
29+
[
30+
new PropertyValueModel { Alias = "title", Value = "Hello", Culture = "en-US", Segment = null }
31+
],
32+
};
33+
34+
var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]);
35+
36+
Assert.That(result, Is.Empty);
37+
}
38+
39+
[Test]
40+
public void GetPopulatedSegmentCultures_ReturnsSegmentWithCultures_WhenPropertiesExistForSegment()
41+
{
42+
var model = new TestContentEditingModel
43+
{
44+
Variants =
45+
[
46+
new VariantModel { Name = "English Default", Culture = "en-US", Segment = null },
47+
new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" },
48+
new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" }
49+
],
50+
Properties =
51+
[
52+
new PropertyValueModel { Alias = "title", Value = "Hello", Culture = "en-US", Segment = null },
53+
new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" },
54+
new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" }
55+
],
56+
};
57+
58+
var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]);
59+
60+
Assert.That(result, Has.Count.EqualTo(1));
61+
Assert.That(result.ContainsKey("segment-1"), Is.True);
62+
Assert.That(result["segment-1"], Does.Contain("en-US"));
63+
Assert.That(result["segment-1"], Does.Contain("da-DK"));
64+
}
65+
66+
[Test]
67+
public void GetPopulatedSegmentCultures_ReturnsOnlyCulturesWithProperties_WhenSomeSegmentCultureCombinationsHaveNoProperties()
68+
{
69+
var model = new TestContentEditingModel
70+
{
71+
Variants =
72+
[
73+
new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" },
74+
new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" }
75+
],
76+
Properties =
77+
[
78+
// Only en-US has a property for the mobile segment
79+
new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" }
80+
],
81+
};
82+
83+
var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]);
84+
85+
Assert.That(result, Has.Count.EqualTo(1));
86+
Assert.That(result.ContainsKey("segment-1"), Is.True);
87+
Assert.That(result["segment-1"], Does.Contain("en-US"));
88+
Assert.That(result["segment-1"], Does.Not.Contain("da-DK"));
89+
}
90+
91+
[Test]
92+
public void GetPopulatedSegmentCultures_ExcludesCulturesNotInCulturesParameter()
93+
{
94+
var model = new TestContentEditingModel
95+
{
96+
Variants =
97+
[
98+
new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" },
99+
new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" },
100+
new VariantModel { Name = "German Segment 1", Culture = "de-DE", Segment = "segment-1" }
101+
],
102+
Properties =
103+
[
104+
new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" },
105+
new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" },
106+
new PropertyValueModel { Alias = "title", Value = "Hallo Segment 1", Culture = "de-DE", Segment = "segment-1" }
107+
],
108+
};
109+
110+
// Only validating en-US and da-DK, not de-DE
111+
var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]);
112+
113+
Assert.That(result, Has.Count.EqualTo(1));
114+
Assert.That(result["segment-1"], Does.Contain("en-US"));
115+
Assert.That(result["segment-1"], Does.Contain("da-DK"));
116+
Assert.That(result["segment-1"], Does.Not.Contain("de-DE"));
117+
}
118+
119+
[Test]
120+
public void GetPopulatedSegmentCultures_HandlesMultipleSegments()
121+
{
122+
var model = new TestContentEditingModel
123+
{
124+
Variants =
125+
[
126+
new VariantModel { Name = "English Segment 1", Culture = "en-US", Segment = "segment-1" },
127+
new VariantModel { Name = "English Segment 2", Culture = "en-US", Segment = "segment-2" },
128+
new VariantModel { Name = "Danish Segment 1", Culture = "da-DK", Segment = "segment-1" }
129+
],
130+
Properties =
131+
[
132+
new PropertyValueModel { Alias = "title", Value = "Hello Segment 1", Culture = "en-US", Segment = "segment-1" },
133+
new PropertyValueModel { Alias = "title", Value = "Hello Segment 2", Culture = "en-US", Segment = "segment-2" },
134+
new PropertyValueModel { Alias = "title", Value = "Hej Segment 1", Culture = "da-DK", Segment = "segment-1" }
135+
],
136+
};
137+
138+
var result = ContentValidationService.GetPopulatedSegmentCultures(model, ["en-US", "da-DK"]);
139+
140+
Assert.That(result, Has.Count.EqualTo(2));
141+
Assert.That(result.ContainsKey("segment-1"), Is.True);
142+
Assert.That(result.ContainsKey("segment-2"), Is.True);
143+
Assert.That(result["segment-1"], Does.Contain("en-US"));
144+
Assert.That(result["segment-1"], Does.Contain("da-DK"));
145+
Assert.That(result["segment-2"], Does.Contain("en-US"));
146+
Assert.That(result["segment-2"], Does.Not.Contain("da-DK"));
147+
}
148+
}

0 commit comments

Comments
 (0)