Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle "expose" for variant elements with all invariant properties #17621

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using Microsoft.Extensions.DependencyInjection;

Check warning on line 1 in src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ New issue: Overall Code Complexity

This module has a mean cyclomatic complexity of 4.71 across 7 functions. The mean complexity threshold is 4. This file has many conditional statements (e.g. if, for, while) across its implementation, leading to lower code health. Avoid adding more conditionals.
using Umbraco.Cms.Core.DependencyInjection;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.Blocks;
using Umbraco.Cms.Core.Models.PublishedContent;
Expand All @@ -9,10 +11,29 @@
public sealed class BlockEditorVarianceHandler
{
private readonly ILanguageService _languageService;
private readonly IContentTypeService _contentTypeService;

[Obsolete("Please use the constructor that accepts IContentTypeService. Will be removed in V16.")]
public BlockEditorVarianceHandler(ILanguageService languageService)
=> _languageService = languageService;
: this(languageService, StaticServiceProvider.Instance.GetRequiredService<IContentTypeService>())
{
}

public BlockEditorVarianceHandler(ILanguageService languageService, IContentTypeService contentTypeService)
{
_languageService = languageService;
_contentTypeService = contentTypeService;
}

/// <summary>
/// Aligns a block property value for variance changes.
/// </summary>
/// <param name="blockPropertyValue">The block property value to align.</param>
/// <param name="propertyType">The underlying property type.</param>
/// <param name="culture">The culture being handled (null if invariant).</param>
/// <remarks>
/// Used for aligning variance changes when editing content.
/// </remarks>
public async Task AlignPropertyVarianceAsync(BlockPropertyValue blockPropertyValue, IPropertyType propertyType, string? culture)
{
culture ??= await _languageService.GetDefaultIsoCodeAsync();
Expand All @@ -24,6 +45,15 @@
}
}

/// <summary>
/// Aligns a block property value for variance changes.
/// </summary>
/// <param name="blockPropertyValue">The block property value to align.</param>
/// <param name="propertyType">The underlying property type.</param>
/// <param name="owner">The containing block element.</param>
/// <remarks>
/// Used for aligning variance changes when rendering content.
/// </remarks>
public async Task<BlockPropertyValue?> AlignedPropertyVarianceAsync(BlockPropertyValue blockPropertyValue, IPublishedPropertyType propertyType, IPublishedElement owner)
{
ContentVariation propertyTypeVariation = owner.ContentType.Variations & propertyType.Variations;
Expand Down Expand Up @@ -65,6 +95,15 @@
return null;
}

/// <summary>
/// Aligns a block value for variance changes.
/// </summary>
/// <param name="blockValue">The block property value to align.</param>
/// <param name="owner">The owner element (the content for block properties at content level, or the parent element for nested block properties).</param>
/// <param name="element">The containing block element.</param>
/// <remarks>
/// Used for aligning variance changes when rendering content.
/// </remarks>
public async Task<IEnumerable<BlockItemVariation>> AlignedExposeVarianceAsync(BlockValue blockValue, IPublishedElement owner, IPublishedElement element)
{
BlockItemVariation[] blockVariations = blockValue.Expose.Where(v => v.ContentKey == element.Key).ToArray();
Expand Down Expand Up @@ -96,17 +135,47 @@
return blockVariations;
}

/// <summary>
/// Aligns block value expose for variance changes.
/// </summary>
/// <param name="blockValue">The block value to align.</param>
/// <remarks>
/// <para>
/// Used for aligning variance changes when editing content.
/// </para>
/// <para>
/// This is expected to be invoked after all block values have been aligned for variance changes by <see cref="AlignPropertyVarianceAsync"/>.
/// </para>
/// </remarks>
public void AlignExposeVariance(BlockValue blockValue)
{
var contentDataToAlign = new List<BlockItemData>();
var elementTypesByKey = blockValue
.ContentData
.Select(cd => cd.ContentTypeKey)
.Distinct()
.Select(_contentTypeService.Get)
.WhereNotNull()
.ToDictionary(c => c.Key);

foreach (BlockItemVariation variation in blockValue.Expose)
{
BlockItemData? contentData = blockValue.ContentData.FirstOrDefault(cd => cd.Key == variation.ContentKey);
if (contentData is null)
{
continue;
}

if (elementTypesByKey.TryGetValue(contentData.ContentTypeKey, out IContentType? elementType) is false)
{
continue;
}

if (variation.Culture is not null == elementType.VariesByCulture())
{
continue;
}

Check notice on line 178 in src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

ℹ Getting worse: Complex Method

AlignExposeVariance increases in cyclomatic complexity from 12 to 16, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.
if((variation.Culture is null && contentData.Values.Any(v => v.Culture is not null))
|| (variation.Culture is not null && contentData.Values.All(v => v.Culture is null)))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public void Ignores_Other_Layouts()
private BlockGridPropertyValueConverter CreateConverter()
{
var publishedModelFactory = new NoopPublishedModelFactory();
var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>());
var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>(), Mock.Of<IContentTypeService>());
var editor = new BlockGridPropertyValueConverter(
Mock.Of<IProfilingLogger>(),
new BlockEditorConverter(GetPublishedContentTypeCache(), Mock.Of<ICacheManager>(), publishedModelFactory, Mock.Of<IVariationContextAccessor>(), blockVarianceHandler),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public class BlockListPropertyValueConverterTests : BlockPropertyValueConverterT
private BlockListPropertyValueConverter CreateConverter()
{
var publishedModelFactory = new NoopPublishedModelFactory();
var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>());
var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>(), Mock.Of<IContentTypeService>());
var editor = new BlockListPropertyValueConverter(
Mock.Of<IProfilingLogger>(),
new BlockEditorConverter(GetPublishedContentTypeCache(), Mock.Of<ICacheManager>(), publishedModelFactory, Mock.Of<IVariationContextAccessor>(), blockVarianceHandler),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public void SetUp()
_propertyEditorCollection = new PropertyEditorCollection(new DataEditorCollection(Enumerable.Empty<IDataEditor>));
_dataValueReferenceFactories = new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>);

var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>());
var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>(), Mock.Of<IContentTypeService>());
_dataValueEditorFactoryMock
.Setup(m =>
m.Create<BlockListPropertyEditorBase.BlockListEditorPropertyValueEditor>(It.IsAny<DataEditorAttribute>(), It.IsAny<BlockEditorDataConverter<BlockListValue, BlockListLayoutItem>>()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
public async Task Assigns_Default_Culture_When_Culture_Variance_Is_Enabled()
{
var propertyValue = new BlockPropertyValue { Culture = null };
var subject = BlockEditorVarianceHandler("da-DK");
var owner = PublishedElement(ContentVariation.Culture);
var subject = BlockEditorVarianceHandler("da-DK", owner);
var result = await subject.AlignedPropertyVarianceAsync(
propertyValue,
PublishedPropertyType(ContentVariation.Culture),
PublishedElement(ContentVariation.Culture));
owner);

Check warning on line 24 in tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/PropertyEditors/BlockEditorVarianceHandlerTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (v15/dev)

❌ New issue: Code Duplication

The module contains 6 functions with similar structure: AlignExpose_Can_Align_Invariance,AlignExpose_Can_Align_Variance,AlignExpose_Can_Handle_Variant_Element_Type_With_All_Invariant_Block_Values,Assigns_Default_Culture_When_Culture_Variance_Is_Enabled and 2 more functions. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.
Assert.IsNotNull(result);
Assert.AreEqual("da-DK", result.Culture);
}
Expand All @@ -29,11 +30,12 @@
public async Task Removes_Default_Culture_When_Culture_Variance_Is_Disabled()
{
var propertyValue = new BlockPropertyValue { Culture = "da-DK" };
var subject = BlockEditorVarianceHandler("da-DK");
var owner = PublishedElement(ContentVariation.Nothing);
var subject = BlockEditorVarianceHandler("da-DK", owner);
var result = await subject.AlignedPropertyVarianceAsync(
propertyValue,
PublishedPropertyType(ContentVariation.Nothing),
PublishedElement(ContentVariation.Nothing));
owner);
Assert.IsNotNull(result);
Assert.AreEqual(null, result.Culture);
}
Expand All @@ -42,14 +44,104 @@
public async Task Ignores_NonDefault_Culture_When_Culture_Variance_Is_Disabled()
{
var propertyValue = new BlockPropertyValue { Culture = "en-US" };
var subject = BlockEditorVarianceHandler("da-DK");
var owner = PublishedElement(ContentVariation.Nothing);
var subject = BlockEditorVarianceHandler("da-DK", owner);
var result = await subject.AlignedPropertyVarianceAsync(
propertyValue,
PublishedPropertyType(ContentVariation.Nothing),
PublishedElement(ContentVariation.Nothing));
owner);
Assert.IsNull(result);
}

[Test]
public void AlignExpose_Can_Align_Invariance()
{
var owner = PublishedElement(ContentVariation.Nothing);
var contentDataKey = Guid.NewGuid();
var blockValue = new BlockListValue
{
ContentData =
[
new()
{
Key = contentDataKey,
ContentTypeKey = owner.ContentType.Key,
Values =
[
new BlockPropertyValue { Alias = "one", Culture = null, Segment = null, Value = "Value one" }
]
}
],
Expose = [new() { ContentKey = contentDataKey, Culture = "da-DK" }]
};

var subject = BlockEditorVarianceHandler("da-DK", owner);
subject.AlignExposeVariance(blockValue);

Assert.AreEqual(null, blockValue.Expose.First().Culture);
}

[Test]
public void AlignExpose_Can_Align_Variance()
{
var owner = PublishedElement(ContentVariation.CultureAndSegment);
var contentDataKey = Guid.NewGuid();
var blockValue = new BlockListValue
{
ContentData =
[
new()
{
Key = contentDataKey,
ContentTypeKey = owner.ContentType.Key,
Values =
[
new BlockPropertyValue { Alias = "one", Culture = "en-US", Segment = "segment-one", Value = "Value one" }
]
}
],
Expose = [new() { ContentKey = contentDataKey, Culture = null, Segment = null }]
};

var subject = BlockEditorVarianceHandler("da-DK", owner);
subject.AlignExposeVariance(blockValue);

Assert.Multiple(() =>
{
var alignedExpose = blockValue.Expose.First();
Assert.AreEqual("en-US", alignedExpose.Culture);
Assert.AreEqual("segment-one", alignedExpose.Segment);
});
}

[Test]
public void AlignExpose_Can_Handle_Variant_Element_Type_With_All_Invariant_Block_Values()
{
var owner = PublishedElement(ContentVariation.Culture);
var contentDataKey = Guid.NewGuid();
var blockValue = new BlockListValue
{
ContentData =
[
new()
{
Key = contentDataKey,
ContentTypeKey = owner.ContentType.Key,
Values =
[
new BlockPropertyValue { Alias = "one", Culture = null, Segment = null, Value = "Value one" }
]
}
],
Expose = [new() { ContentKey = contentDataKey, Culture = "da-DK" }]
};

var subject = BlockEditorVarianceHandler("da-DK", owner);
subject.AlignExposeVariance(blockValue);

Assert.AreEqual("da-DK", blockValue.Expose.First().Culture);
}

private static IPublishedPropertyType PublishedPropertyType(ContentVariation variation)
{
var propertyTypeMock = new Mock<IPublishedPropertyType>();
Expand All @@ -61,15 +153,21 @@
{
var contentTypeMock = new Mock<IPublishedContentType>();
contentTypeMock.SetupGet(m => m.Variations).Returns(variation);
contentTypeMock.SetupGet(m => m.Key).Returns(Guid.NewGuid());
var elementMock = new Mock<IPublishedElement>();
elementMock.SetupGet(m => m.ContentType).Returns(contentTypeMock.Object);
return elementMock.Object;
}

private static BlockEditorVarianceHandler BlockEditorVarianceHandler(string defaultLanguageIsoCode)
private static BlockEditorVarianceHandler BlockEditorVarianceHandler(string defaultLanguageIsoCode, IPublishedElement element)
{
var languageServiceMock = new Mock<ILanguageService>();
languageServiceMock.Setup(m => m.GetDefaultIsoCodeAsync()).ReturnsAsync(defaultLanguageIsoCode);
return new BlockEditorVarianceHandler(languageServiceMock.Object);
var contentTypeServiceMock = new Mock<IContentTypeService>();
var elementType = new Mock<IContentType>();
elementType.SetupGet(e => e.Key).Returns(element.ContentType.Key);
elementType.SetupGet(e => e.Variations).Returns(element.ContentType.Variations);
contentTypeServiceMock.Setup(c => c.Get(element.ContentType.Key)).Returns(elementType.Object);
return new BlockEditorVarianceHandler(languageServiceMock.Object, contentTypeServiceMock.Object);
}
}
Loading