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

V15: Warn when content is unroutable #17837

Merged
merged 8 commits into from
Jan 8, 2025
Merged
Changes from 1 commit
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
Prev Previous commit
Dedicated configuration option to suppress unroutable content warnings
kjac committed Jan 7, 2025
commit d467086cd06c5652914748a7063d9f5cfb853ce0
7 changes: 7 additions & 0 deletions src/Umbraco.Core/Configuration/Models/ContentSettings.cs
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@ public class ContentSettings
internal const bool StaticDisableUnpublishWhenReferenced = false;
internal const bool StaticAllowEditInvariantFromNonDefault = false;
internal const bool StaticShowDomainWarnings = true;
internal const bool StaticShowUnroutableContentWarnings = true;

/// <summary>
/// Gets or sets a value for the content notification settings.
@@ -141,4 +142,10 @@ public class ContentSettings
/// </summary>
[DefaultValue(StaticShowDomainWarnings)]
public bool ShowDomainWarnings { get; set; } = StaticShowDomainWarnings;

/// <summary>
/// Gets or sets a value indicating whether to show unroutable content warnings.
/// </summary>
[DefaultValue(StaticShowUnroutableContentWarnings)]
public bool ShowUnroutableContentWarnings { get; set; } = StaticShowUnroutableContentWarnings;
}
Original file line number Diff line number Diff line change
@@ -29,90 +29,91 @@
private readonly IEventMessagesFactory _eventMessagesFactory;
private readonly ContentSettings _contentSettings;

public AddUnroutableContentWarningsWhenPublishingNotificationHandler(
IPublishedRouter publishedRouter,
IUmbracoContextAccessor umbracoContextAccessor,
ILanguageService languageService,
ILocalizedTextService localizedTextService,
IContentService contentService,
IVariationContextAccessor variationContextAccessor,
ILoggerFactory loggerFactory,
UriUtility uriUtility,
IPublishedUrlProvider publishedUrlProvider,
IPublishedContentCache publishedContentCache,
IDocumentNavigationQueryService navigationQueryService,
IEventMessagesFactory eventMessagesFactory,
IOptions<ContentSettings> contentSettings)
{
_publishedRouter = publishedRouter;
_umbracoContextAccessor = umbracoContextAccessor;
_languageService = languageService;
_localizedTextService = localizedTextService;
_contentService = contentService;
_variationContextAccessor = variationContextAccessor;
_loggerFactory = loggerFactory;
_uriUtility = uriUtility;
_publishedUrlProvider = publishedUrlProvider;
_publishedContentCache = publishedContentCache;
_navigationQueryService = navigationQueryService;
_eventMessagesFactory = eventMessagesFactory;
_contentSettings = contentSettings.Value;
}

Check warning on line 60 in src/Umbraco.Core/Events/AddUnroutableContentWarningsWhenPublishingNotificationHandler.cs

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

❌ New issue: Constructor Over-Injection

AddUnroutableContentWarningsWhenPublishingNotificationHandler has 13 arguments, threshold = 5. This constructor has too many arguments, indicating an object with low cohesion or missing function argument abstraction. Avoid adding more arguments.

public async Task HandleAsync(ContentPublishedNotification notification, CancellationToken cancellationToken)
{
if (_contentSettings.ShowDomainWarnings is false)
if (_contentSettings.ShowUnroutableContentWarnings is false)
{
return;
}

foreach (IContent content in notification.PublishedEntities)
{
string[]? successfulCultures;
if (content.ContentType.VariesByCulture() is false)
{
// successfulCultures will be null here - change it to a wildcard and utilize this below
successfulCultures = ["*"];
}
else
{
successfulCultures = content.PublishedCultures.ToArray();
}

if (successfulCultures?.Any() is not true)
{
return;
}

if (!_umbracoContextAccessor.TryGetUmbracoContext(out IUmbracoContext? umbracoContext))
{
return;
}

UrlInfo[] urls = (await content.GetContentUrlsAsync(
_publishedRouter,
umbracoContext,
_languageService,
_localizedTextService,
_contentService,
_variationContextAccessor,
_loggerFactory.CreateLogger<IContent>(),
_uriUtility,
_publishedUrlProvider,
_publishedContentCache,
_navigationQueryService)).ToArray();


EventMessages eventMessages = _eventMessagesFactory.Get();
foreach (var culture in successfulCultures)
{
if (urls.Where(u => u.Culture == culture || culture == "*").All(u => u.IsUrl is false))
{
eventMessages.Add(new EventMessage("Content published", "The document does not have a URL, possibly due to a naming collision with another document. More details can be found under Info.", EventMessageType.Warning));

// only add one warning here, even though there might actually be more
break;
}
}
}
}

Check warning on line 118 in src/Umbraco.Core/Events/AddUnroutableContentWarningsWhenPublishingNotificationHandler.cs

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

❌ New issue: Complex Method

HandleAsync has a cyclomatic complexity of 9, 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.

Check warning on line 118 in src/Umbraco.Core/Events/AddUnroutableContentWarningsWhenPublishingNotificationHandler.cs

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

❌ New issue: Bumpy Road Ahead

HandleAsync has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
}

Unchanged files with check annotations Beta

using System.Globalization;

Check notice on line 1 in src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs

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

ℹ Getting worse: Overall Code Complexity

The mean cyclomatic complexity increases from 5.57 to 6.14, threshold = 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 Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
// We have the published content now, so we can check if the culture is published, and thus avoid the DB hit.
string route;
var isDraft = _umbracoContextAccessor.GetRequiredUmbracoContext().InPreviewMode;
if(isDraft is false && string.IsNullOrWhiteSpace(culture) is false && content.Cultures.Any() && content.IsInvariantOrHasCulture(culture) is false)

Check warning on line 174 in src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs

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

❌ New issue: Complex Conditional

GetUrl has 1 complex conditionals with 3 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
{
route = "#";
}