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 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
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
@@ -0,0 +1,119 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Core.PublishedCache;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.Navigation;
using Umbraco.Cms.Core.Web;
using Umbraco.Extensions;

namespace Umbraco.Cms.Core.Events;

public class AddUnroutableContentWarningsWhenPublishingNotificationHandler : INotificationAsyncHandler<ContentPublishedNotification>
{
private readonly IPublishedRouter _publishedRouter;
private readonly IUmbracoContextAccessor _umbracoContextAccessor;
private readonly ILanguageService _languageService;
private readonly ILocalizedTextService _localizedTextService;
private readonly IContentService _contentService;
private readonly IVariationContextAccessor _variationContextAccessor;
private readonly ILoggerFactory _loggerFactory;
private readonly UriUtility _uriUtility;
private readonly IPublishedUrlProvider _publishedUrlProvider;
private readonly IPublishedContentCache _publishedContentCache;
private readonly IDocumentNavigationQueryService _navigationQueryService;
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.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.
}
19 changes: 17 additions & 2 deletions src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
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;
@@ -145,8 +145,10 @@

private string GetLegacyRouteFormatById(Guid key, string? culture)
{
var isDraft = _umbracoContextAccessor.GetRequiredUmbracoContext().InPreviewMode;

return _documentUrlService.GetLegacyRouteFormat(key, culture, _umbracoContextAccessor.GetRequiredUmbracoContext().InPreviewMode);

return _documentUrlService.GetLegacyRouteFormat(key, culture, isDraft);


}
@@ -163,9 +165,22 @@
throw new ArgumentException("Current URL must be absolute.", nameof(current));
}

// This might seem to be some code duplication, as we do the same check in GetLegacyRouteFormat
// but this is strictly neccesary, as if we're coming from a published notification
// this document will still not always be in the memory cache. And thus we have to hit the DB
// 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 = "#";
}
else
{
route = GetLegacyRouteFormatById(content.Key, culture);
}

// will not use cache if previewing
var route = GetLegacyRouteFormatById(content.Key, culture);

return GetUrlFromRoute(route, content.Id, current, mode, culture);
}
Original file line number Diff line number Diff line change
@@ -411,8 +411,8 @@ public static IUmbracoBuilder AddCoreNotifications(this IUmbracoBuilder builder)
.AddNotificationHandler<AssignedUserGroupPermissionsNotification, AuditNotificationsHandler>();

// Handlers for publish warnings
builder
.AddNotificationHandler<ContentPublishedNotification, AddDomainWarningsWhenPublishingNotificationHandler>();
builder.AddNotificationHandler<ContentPublishedNotification, AddDomainWarningsWhenPublishingNotificationHandler>();
builder.AddNotificationAsyncHandler<ContentPublishedNotification, AddUnroutableContentWarningsWhenPublishingNotificationHandler>();

// Handlers for save warnings
builder
Loading