Skip to content
11 changes: 7 additions & 4 deletions src/Umbraco.Core/Extensions/PublishedContentExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Umbraco.

Check notice on line 1 in src/Umbraco.Core/Extensions/PublishedContentExtensions.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Primitive Obsession

The ratio of primitive types in function arguments decreases from 31.24% to 31.07%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.
// See LICENSE for more details.

using System.Data;
Expand Down Expand Up @@ -153,7 +153,9 @@

// parent key is null if content is at root
return parentKey.HasValue
? publishedStatusFilteringService.FilterAvailable([parentKey.Value], null).FirstOrDefault()
#pragma warning disable CS0618 // Type or member is obsolete (justification: temporary means to avoid breaking changes in the PublishedContentExtensions)
? publishedStatusFilteringService.Unfiltered([parentKey.Value]).FirstOrDefault()
#pragma warning restore CS0618 // Type or member is obsolete
: null;
}

Expand Down Expand Up @@ -2261,27 +2263,28 @@
INavigationQueryService navigationQueryService,
IPublishedStatusFilteringService publishedStatusFilteringService,
bool orSelf,
string? contentTypeAlias = null,
string? culture = null)
string? contentTypeAlias = null)
{
if (orSelf)
{
if (contentTypeAlias is null || content.ContentType.Alias == contentTypeAlias)
{
yield return content;
}
}

var nodeExists = contentTypeAlias is null
? navigationQueryService.TryGetAncestorsKeys(content.Key, out IEnumerable<Guid> ancestorsKeys)
: navigationQueryService.TryGetAncestorsKeysOfType(content.Key, contentTypeAlias, out ancestorsKeys);

if (nodeExists is false)
{
yield break;
}

IEnumerable<IPublishedContent> ancestors = publishedStatusFilteringService.FilterAvailable(ancestorsKeys, culture);
#pragma warning disable CS0618 // Type or member is obsolete (justification: temporary means to avoid breaking changes in the PublishedContentExtensions)
Comment thread
kjac marked this conversation as resolved.
IEnumerable<IPublishedContent> ancestors = publishedStatusFilteringService.Unfiltered(ancestorsKeys);
#pragma warning restore CS0618 // Type or member is obsolete

Check notice on line 2287 in src/Umbraco.Core/Extensions/PublishedContentExtensions.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Excess Number of Function Arguments

EnumerateAncestorsOrSelfInternal decreases from 6 to 5 arguments, max arguments = 4. This function has too many arguments, indicating a lack of encapsulation. Avoid adding more arguments.
foreach (IPublishedContent ancestor in ancestors)
{
yield return ancestor;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,14 @@ public interface IPublishStatusQueryService
/// <param name="documentKey">The document's key.</param>
/// <returns>True if document has a published ancestor path.</returns>
bool HasPublishedAncestorPath(Guid documentKey);

/// <summary>
/// Verifies if a document has a published ancestor path (i.e. all ancestors are themselves published in at specific culture).
/// </summary>
/// <param name="documentKey">The document's key.</param>
/// <param name="culture">The culture.</param>
/// <returns>True if document has a published ancestor path.</returns>
// TODO (V18): Remove the default implementation.
bool HasPublishedAncestorPath(Guid documentKey, string culture)
=> HasPublishedAncestorPath(documentKey);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,12 @@ public interface IPublishedStatusFilteringService
/// <param name="culture">The culture to filter by, or <c>null</c> to use the current culture context.</param>
/// <returns>A collection of <see cref="IPublishedContent"/> items that are available for display.</returns>
IEnumerable<IPublishedContent> FilterAvailable(IEnumerable<Guid> candidateKeys, string? culture);

/// <summary>
/// Returns content for a collection of candidate content keys.
/// </summary>
/// <param name="candidateKeys">The collection of content keys to return.</param>
/// <returns>A collection of <see cref="IPublishedContent"/> items that are available for display.</returns>
[Obsolete("This is an intermediate solution to avoid breaking changes. Use the IPublishedContentCache to get published content by key. Scheduled for removal in V19.")]
IEnumerable<IPublishedContent> Unfiltered(IEnumerable<Guid> candidateKeys) => throw new NotImplementedException();
}
12 changes: 11 additions & 1 deletion src/Umbraco.Core/Services/PublishStatus/PublishStatusService.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Concurrent;

Check warning on line 1 in src/Umbraco.Core/Services/PublishStatus/PublishStatusService.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Primitive Obsession

In this module, 55.0% of all function arguments are primitive types, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Persistence.Repositories;
using Umbraco.Cms.Core.Scoping;
Expand Down Expand Up @@ -107,6 +107,13 @@

/// <inheritdoc/>
public bool HasPublishedAncestorPath(Guid contentKey)
=> HasPublishedAncestorPathInternal(contentKey, null);

/// <inheritdoc/>
public bool HasPublishedAncestorPath(Guid contentKey, string culture)
Comment thread
kjac marked this conversation as resolved.
=> HasPublishedAncestorPathInternal(contentKey, culture);

private bool HasPublishedAncestorPathInternal(Guid contentKey, string? culture)
{
var success = _documentNavigationQueryService.TryGetAncestorsKeys(contentKey, out IEnumerable<Guid> keys);
if (success is false)
Expand All @@ -119,8 +126,11 @@

foreach (Guid key in keys)
{
var isPublished = culture is null
? IsDocumentPublishedInAnyCulture(key)
: IsDocumentPublished(key, culture);

if (IsDocumentPublishedInAnyCulture(key) is false)
if (isPublished is false)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,18 @@ public IEnumerable<IPublishedContent> FilterAvailable(IEnumerable<Guid> candidat
? candidateKeysAsArray
: candidateKeysAsArray.Where(key =>
_publishStatusQueryService.IsDocumentPublished(key, culture)
&& _publishStatusQueryService.HasPublishedAncestorPath(key));
&& _publishStatusQueryService.HasPublishedAncestorPath(key, culture));

return WhereIsInvariantOrHasCultureOrRequestedAllCultures(candidateKeys, culture, preview).ToArray();
}

/// <inheritdoc />
public IEnumerable<IPublishedContent> Unfiltered(IEnumerable<Guid> candidateKeys)
Comment thread
AndyButland marked this conversation as resolved.
{
var preview = _previewService.IsInPreview();
return candidateKeys.Select(key => _publishedContentCache.GetById(preview, key)).WhereNotNull().ToArray();
}

/// <summary>
/// Filters content items to include only those that are invariant, have the requested culture, or when all cultures are requested.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,8 @@ public PublishedMediaStatusFilteringService(IPublishedMediaCache publishedMediaC
/// <inheritdoc />
public IEnumerable<IPublishedContent> FilterAvailable(IEnumerable<Guid> candidateKeys, string? culture)
=> candidateKeys.Select(_publishedMediaCache.GetById).WhereNotNull().ToArray();

/// <inheritdoc />
public IEnumerable<IPublishedContent> Unfiltered(IEnumerable<Guid> candidateKeys)
=> candidateKeys.Select(_publishedMediaCache.GetById).WhereNotNull().ToArray();
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.Navigation;
using Umbraco.Cms.Tests.Common.Builders;
using Umbraco.Cms.Tests.Common.Builders.Extensions;

namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services;

Expand Down Expand Up @@ -126,4 +128,57 @@ public void Published_Document_With_UnPublished_Parent_Has_Unpublished_Path()
Assert.IsFalse(PublishStatusQueryService.HasPublishedAncestorPath(Subpage.Key));
});
}

[TestCase("en-US")]
[TestCase("da-DK")]
public async Task Unpublished_Document_Culture_Yields_Correct_Published_Ancestor_Path(string cultureToUnpublish)
{
await GetRequiredService<ILanguageService>()
.CreateAsync(new Language("da-DK", "Danish"), Constants.Security.SuperUserKey);

var contentTypeKey = Guid.NewGuid();
var contentType = new ContentTypeBuilder()
.WithKey(contentTypeKey)
.WithAlias("variant")
.WithContentVariation(ContentVariation.Culture)
.WithAllowAsRoot(true)
.Build();
await ContentTypeService.CreateAsync(contentType, Constants.Security.SuperUserKey);
contentType.AllowedContentTypes = [new ContentTypeSort(contentTypeKey, 1, "variant")];
await ContentTypeService.UpdateAsync(contentType, Constants.Security.SuperUserKey);

IContent root = new ContentBuilder()
.WithContentType(contentType)
.WithCultureName("en-US", "Root EN")
.WithCultureName("da-DK", "Root DA")
.Build();
ContentService.Save(root);

IContent child = new ContentBuilder()
.WithContentType(contentType)
.WithCultureName("en-US", "Child EN")
.WithCultureName("da-DK", "Child DA")
.WithParent(root)
.Build();
ContentService.Save(child);

IContent grandchild = new ContentBuilder()
.WithContentType(contentType)
.WithCultureName("en-US", "Grandchild EN")
.WithCultureName("da-DK", "Grandchild DA")
.WithParent(child)
.Build();
ContentService.Save(grandchild);

ContentService.PublishBranch(root, PublishBranchFilter.IncludeUnpublished, ["en-US", "da-DK"]);

// must refresh the child instance before unpublishing it, to reflect the state changes from the branch publish above
child = ContentService.GetById(child.Key)!;
ContentService.Unpublish(child, cultureToUnpublish);

var publishedCulture = cultureToUnpublish is "en-US" ? "da-DK" : "en-US";
Assert.IsTrue(PublishStatusQueryService.HasPublishedAncestorPath(grandchild.Key, publishedCulture));
Assert.IsFalse(PublishStatusQueryService.HasPublishedAncestorPath(grandchild.Key, cultureToUnpublish));
Assert.IsTrue(PublishStatusQueryService.HasPublishedAncestorPath(grandchild.Key, Constants.System.InvariantCulture));
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Moq;

Check warning on line 1 in tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringServiceTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Code Duplication

introduced similar code in: FilterAvailable_Variant_ForNonPreview_YieldsOnlyItemsWithPublishedAncestorPath,FilterAvailable_Variant_ForPreview_IgnoresMissingPublishedAncestorPath. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

Check notice on line 1 in tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringServiceTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Code Duplication

reduced similar code in: SetupInvariant. Avoid duplicated, aka copy-pasted, code inside the module. More duplication lowers the code health.

Check notice on line 1 in tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringServiceTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

✅ Getting better: Primitive Obsession

The ratio of primitive types in function arguments decreases from 80.00% to 74.07%, threshold = 30.0%. The functions in this file have too many primitive types (e.g. int, double, float) in their function argument lists. Using many primitive types lead to the code smell Primitive Obsession. Avoid adding more primitive arguments.
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Models;
Expand Down Expand Up @@ -127,6 +127,91 @@
}
}

[TestCase("da-DK", 1)]
[TestCase("en-US", 2)]
[TestCase("*", 3)]
public void FilterAvailable_Variant_ForNonPreview_YieldsOnlyItemsWithPublishedAncestorPath(string culture, int expectedNumberOfChildren)
{
var (sut, items) = SetupVariant(
false,
culture == Constants.System.InvariantCulture ? "en-US" : culture,
(key, _, allItems) => allItems.Keys.IndexOf(key) > 2);

var children = sut.FilterAvailable(items.Keys, culture).ToArray();
Assert.AreEqual(expectedNumberOfChildren, children.Length);

// IDs 0 through 3 exist in both en-US and da-DK, but none pass both the published and ancestor-path checks

// IDs 4 through 6 exist only in en-US - only even IDs are published
if (culture == "en-US")
{
Assert.AreEqual(4, children[0].Id);
Assert.AreEqual(6, children[1].Id);
}

// IDs 7 through 9 exist only in da-DK - only even IDs are published
if (culture == "da-DK")
{
Assert.AreEqual(8, children[0].Id);
}

if (culture == Constants.System.InvariantCulture)
{
Assert.AreEqual(4, children[0].Id);
Assert.AreEqual(6, children[1].Id);
Assert.AreEqual(8, children[2].Id);
}
}

[TestCase("da-DK", 7)]
[TestCase("en-US", 7)]
[TestCase("*", 10)]
public void FilterAvailable_Variant_ForPreview_IgnoresMissingPublishedAncestorPath(string culture, int expectedNumberOfChildren)
{
var (sut, items) = SetupVariant(
true,
culture == Constants.System.InvariantCulture ? "en-US" : culture,
(_, _, _) => false);

var children = sut.FilterAvailable(items.Keys, culture).ToArray();
Assert.AreEqual(expectedNumberOfChildren, children.Length);

// IDs 0 through 3 exist in both en-US and da-DK
Assert.Multiple(() =>
{
Assert.AreEqual(0, children[0].Id);
Assert.AreEqual(1, children[1].Id);
Assert.AreEqual(2, children[2].Id);
Assert.AreEqual(3, children[3].Id);
});

// IDs 4 through 6 exist only in en-US
if (culture == "en-US")
{
Assert.AreEqual(4, children[4].Id);
Assert.AreEqual(5, children[5].Id);
Assert.AreEqual(6, children[6].Id);
}

// IDs 7 through 9 exist only in da-DK
if (culture == "da-DK")
{
Assert.AreEqual(7, children[4].Id);
Assert.AreEqual(8, children[5].Id);
Assert.AreEqual(9, children[6].Id);
}

if (culture == Constants.System.InvariantCulture)
{
Assert.AreEqual(4, children[4].Id);
Assert.AreEqual(5, children[5].Id);
Assert.AreEqual(6, children[6].Id);
Assert.AreEqual(7, children[7].Id);
Assert.AreEqual(8, children[8].Id);
Assert.AreEqual(9, children[9].Id);
}
}

Check warning on line 213 in tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringServiceTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Large Assertion Blocks

The number of large assertion blocks increases from 5 to 7, threshold = 4. This test file has several blocks of large, consecutive assert statements. Avoid adding more.

Check warning on line 213 in tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/PublishStatus/PublishedContentStatusFilteringServiceTests.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ New issue: Duplicated Assertion Blocks

The test suite contains 3 functions with duplicated assertion blocks (FilterAvailable_MixedVariance_ForPreview_YieldsPublishedItemsInCultureOrInvariant,FilterAvailable_Variant_ForPreview_IgnoresMissingPublishedAncestorPath,FilterAvailable_Variant_ForPreview_YieldsUnpublishedItemsInCulture), threshold = 2. This test file has several blocks of duplicated assertion statements. Avoid adding more.

[TestCase("da-DK", 4)]
[TestCase("en-US", 4)]
[TestCase("*", 5)]
Expand Down Expand Up @@ -260,7 +345,7 @@
// - IDs 4 through 6 exist only in en-US
// - IDs 7 through 9 exist only in da-DK
// - even IDs (0, 2, ...) are published, odd are unpublished
private (PublishedContentStatusFilteringService PublishedContentStatusFilteringService, Dictionary<Guid, IPublishedContent> Items) SetupVariant(bool forPreview, string requestCulture)
private (PublishedContentStatusFilteringService PublishedContentStatusFilteringService, Dictionary<Guid, IPublishedContent> Items) SetupVariant(bool forPreview, string requestCulture, Func<Guid, string, Dictionary<Guid, IPublishedContent>, bool>? hasPublishedAncestorPath = null)
{
var contentType = new Mock<IPublishedContentType>();
contentType.SetupGet(c => c.Variations).Returns(ContentVariation.Culture);
Expand All @@ -287,7 +372,7 @@

var publishedContentCache = SetupPublishedContentCache(forPreview, items);
var previewService = SetupPreviewService(forPreview);
var publishStatusQueryService = SetupPublishStatusQueryService(items);
var publishStatusQueryService = SetupPublishStatusQueryService(items, hasPublishedAncestorPath);
var variationContextAccessor = SetupVariantContextAccessor(requestCulture);

return (
Expand Down Expand Up @@ -353,10 +438,10 @@
items);
}

private IPublishStatusQueryService SetupPublishStatusQueryService(Dictionary<Guid, IPublishedContent> items)
=> SetupPublishStatusQueryService(items, id => id % 2 == 0);
private IPublishStatusQueryService SetupPublishStatusQueryService(Dictionary<Guid, IPublishedContent> items, Func<Guid, string, Dictionary<Guid, IPublishedContent>, bool>? hasPublishedAncestorPath = null)
=> SetupPublishStatusQueryService(items, id => id % 2 == 0, hasPublishedAncestorPath);

private IPublishStatusQueryService SetupPublishStatusQueryService(Dictionary<Guid, IPublishedContent> items, Func<int, bool> idIsPublished)
private IPublishStatusQueryService SetupPublishStatusQueryService(Dictionary<Guid, IPublishedContent> items, Func<int, bool> idIsPublished, Func<Guid, string, Dictionary<Guid, IPublishedContent>, bool>? hasPublishedAncestorPath = null)
{
var publishStatusQueryService = new Mock<IPublishStatusQueryService>();
publishStatusQueryService
Expand All @@ -366,8 +451,8 @@
&& idIsPublished(item.Id)
&& (culture == Constants.System.InvariantCulture || item.ContentType.VariesByCulture() is false || item.Cultures.ContainsKey(culture)));
publishStatusQueryService
.Setup(s => s.HasPublishedAncestorPath(It.IsAny<Guid>()))
.Returns(true);
.Setup(s => s.HasPublishedAncestorPath(It.IsAny<Guid>(), It.IsAny<string>()))
.Returns((Guid key, string culture) => hasPublishedAncestorPath?.Invoke(key, culture, items) ?? true);
return publishStatusQueryService.Object;
}

Expand Down
Loading