From a627930b581048f6ec3484b3d5aa895597ee8f9a Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Mon, 9 Dec 2024 11:07:32 +0100 Subject: [PATCH] Warn about un-routable content at publish time (#17705) (cherry picked from commit 2d9cfc880bed5398ccce374295f0f6e93d76949d) --- .../EmbeddedResources/Lang/da.xml | 1 + .../EmbeddedResources/Lang/en.xml | 1 + .../EmbeddedResources/Lang/en_us.xml | 1 + .../Controllers/ContentController.cs | 82 +++++++++++++++++-- .../Controllers/ContentControllerTests.cs | 40 +++++++-- 5 files changed, 110 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/da.xml b/src/Umbraco.Core/EmbeddedResources/Lang/da.xml index 15cbe5f30a..73ef388cb1 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/da.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/da.xml @@ -1469,6 +1469,7 @@ Mange hilsner fra Umbraco robotten Der er ikke noget domæne konfigureret for %0%, kontakt vensligst en administrator, se loggen for mere information + Dokumentet har ikke nogen URL, muligvis grundet en kollision med et andet dokuments navn. Flere detaljer kan ses under Info. Dit systems information er blevet kopieret til udklipsholderen Kunne desværre ikke kopiere dit systems information til udklipsholderen Webhook gemt diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml index 4995967993..2b8a4a4c2f 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en.xml @@ -1700,6 +1700,7 @@ To manage your website, simply open the Umbraco backoffice and start adding cont There is no domain configured for %0%, please contact an administrator, see log for more information + The document does not have a URL, possibly due to a naming collision with another document. More details can be found under Info. Your system information has successfully been copied to the clipboard Could not copy your system information to the clipboard Webhook saved diff --git a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml index 39b20009f4..472aa58c0f 100644 --- a/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml +++ b/src/Umbraco.Core/EmbeddedResources/Lang/en_us.xml @@ -1732,6 +1732,7 @@ To manage your website, simply open the Umbraco backoffice and start adding cont There is no domain configured for %0%, please contact an administrator, see log for more information + The document does not have a URL, possibly due to a naming collision with another document. More details can be found under Info. An error occurred while enabling version cleanup for %0% An error occurred while disabling version cleanup for %0% Your system information has successfully been copied to the clipboard diff --git a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs index 62cbae3012..c676cee2ca 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/ContentController.cs @@ -1134,8 +1134,12 @@ public class ContentController : ContentControllerBase { PublishResult publishStatus = PublishInternal(contentItem, defaultCulture, cultureForInvariantErrors, out wasCancelled, out var successfulCultures); // Add warnings if domains are not set up correctly - AddDomainWarnings(publishStatus.Content, successfulCultures, globalNotifications); + var addedDomainWarnings = AddDomainWarnings(publishStatus.Content, successfulCultures, globalNotifications, defaultCulture); AddPublishStatusNotifications(new[] { publishStatus }, globalNotifications, notifications, successfulCultures); + if (addedDomainWarnings is false) + { + AddPublishRoutableErrorNotifications(new[] { publishStatus }, globalNotifications, successfulCultures); + } } break; case ContentSaveAction.PublishWithDescendants: @@ -1151,8 +1155,12 @@ public class ContentController : ContentControllerBase } var publishStatus = PublishBranchInternal(contentItem, false, cultureForInvariantErrors, out wasCancelled, out var successfulCultures).ToList(); - AddDomainWarnings(publishStatus, successfulCultures, globalNotifications); + var addedDomainWarnings = AddDomainWarnings(publishStatus, successfulCultures, globalNotifications, defaultCulture); AddPublishStatusNotifications(publishStatus, globalNotifications, notifications, successfulCultures); + if (addedDomainWarnings is false) + { + AddPublishRoutableErrorNotifications(publishStatus, globalNotifications, successfulCultures); + } } break; case ContentSaveAction.PublishWithDescendantsForce: @@ -1235,6 +1243,48 @@ public class ContentController : ContentControllerBase } } + private void AddPublishRoutableErrorNotifications( + IReadOnlyCollection publishStatus, + SimpleNotificationModel globalNotifications, + string[]? successfulCultures) + { + IContent? content = publishStatus.FirstOrDefault()?.Content; + if (content is null) + { + return; + } + + if (content.ContentType.VariesByCulture() is false) + { + // successfulCultures will be null here - change it to a wildcard and utilize this below + successfulCultures = ["*"]; + } + + if (successfulCultures?.Any() is not true) + { + return; + } + + ContentItemDisplay? contentItemDisplay = _umbracoMapper.Map(publishStatus.FirstOrDefault()?.Content); + if (contentItemDisplay?.Urls is null) + { + return; + } + + foreach (var culture in successfulCultures) + { + if (contentItemDisplay.Urls.Where(u => u.Culture == culture || culture == "*").All(u => u.IsUrl is false)) + { + globalNotifications.AddWarningNotification( + _localizedTextService.Localize("auditTrails", "publish"), + _localizedTextService.Localize("speechBubbles", "publishWithNoUrl")); + + // only add one warning here, even though there might actually be more + break; + } + } + } + /// /// Validates critical data for persistence and updates the ModelState and result accordingly /// @@ -1770,12 +1820,15 @@ public class ContentController : ContentControllerBase } } - private void AddDomainWarnings(IEnumerable publishResults, string[]? culturesPublished, SimpleNotificationModel globalNotifications) + private bool AddDomainWarnings(IEnumerable publishResults, string[]? culturesPublished, SimpleNotificationModel globalNotifications, string? defaultCulture) { + var addedDomainWarnings = false; foreach (PublishResult publishResult in publishResults) { - AddDomainWarnings(publishResult.Content, culturesPublished, globalNotifications); + addedDomainWarnings &= AddDomainWarnings(publishResult.Content, culturesPublished, globalNotifications, defaultCulture); } + + return addedDomainWarnings; } /// @@ -1788,24 +1841,25 @@ public class ContentController : ContentControllerBase /// /// /// - internal void AddDomainWarnings(IContent? persistedContent, string[]? culturesPublished, SimpleNotificationModel globalNotifications) + /// + internal bool AddDomainWarnings(IContent? persistedContent, string[]? culturesPublished, SimpleNotificationModel globalNotifications, string? defaultCulture) { if (_contentSettings.ShowDomainWarnings is false) { - return; + return false; } // Don't try to verify if no cultures were published if (culturesPublished is null) { - return; + return false; } var publishedCultures = GetPublishedCulturesFromAncestors(persistedContent).ToList(); // If only a single culture is published we shouldn't have any routing issues if (publishedCultures.Count < 2) { - return; + return false; } // If more than a single culture is published we need to verify that there's a domain registered for each published culture @@ -1827,6 +1881,12 @@ public class ContentController : ContentControllerBase // No domains at all, add a warning, to add domains. if (assignedDomains is null || assignedDomains.Count == 0) { + // If only the default culture was published we shouldn't have any routing issues + if (culturesPublished.Length == 1 && culturesPublished[0].InvariantEquals(defaultCulture)) + { + return false; + } + globalNotifications.AddWarningNotification( _localizedTextService.Localize("auditTrails", "publish"), _localizedTextService.Localize("speechBubbles", "publishWithNoDomains")); @@ -1835,14 +1895,16 @@ public class ContentController : ContentControllerBase "The root node {RootNodeName} was published with multiple cultures, but no domains are configured, this will cause routing and caching issues, please register domains for: {Cultures}", persistedContent?.Name, string.Join(", ", publishedCultures)); - return; + return true; } // If there is some domains, verify that there's a domain for each of the published cultures + var addedDomainWarnings = false; foreach (var culture in culturesPublished .Where(culture => assignedDomains.Any(x => x.LanguageIsoCode?.Equals(culture, StringComparison.OrdinalIgnoreCase) ?? false) is false)) { + addedDomainWarnings = true; globalNotifications.AddWarningNotification( _localizedTextService.Localize("auditTrails", "publish"), _localizedTextService.Localize("speechBubbles", "publishWithMissingDomain", new[] { culture })); @@ -1852,6 +1914,8 @@ public class ContentController : ContentControllerBase persistedContent?.Name, culture); } + + return addedDomainWarnings; } /// diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/ContentControllerTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/ContentControllerTests.cs index b7b9b85c94..bd0f9e33d0 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/ContentControllerTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/ContentControllerTests.cs @@ -56,7 +56,7 @@ public class ContentControllerTests var notifications = new SimpleNotificationModel(); var contentController = CreateContentController(domainServiceMock.Object); - contentController.AddDomainWarnings(rootNode, culturesPublished, notifications); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); Assert.IsEmpty(notifications.Notifications); } @@ -82,7 +82,7 @@ public class ContentControllerTests var notifications = new SimpleNotificationModel(); var contentController = CreateContentController(domainServiceMock.Object); - contentController.AddDomainWarnings(rootNode, culturesPublished, notifications); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); Assert.IsEmpty(notifications.Notifications); } @@ -111,7 +111,7 @@ public class ContentControllerTests var notifications = new SimpleNotificationModel(); var contentController = CreateContentController(domainServiceMock.Object); - contentController.AddDomainWarnings(rootNode, culturesPublished, notifications); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); Assert.AreEqual(1, notifications.Notifications.Count(x => x.NotificationType == NotificationStyle.Warning)); } @@ -139,10 +139,38 @@ public class ContentControllerTests var notifications = new SimpleNotificationModel(); var contentController = CreateContentController(domainServiceMock.Object); - contentController.AddDomainWarnings(rootNode, culturesPublished, notifications); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); Assert.AreEqual(3, notifications.Notifications.Count(x => x.NotificationType == NotificationStyle.Warning)); } + [Test] + public void Does_Not_Warn_When_Only_Publishing_The_Default_Culture_With_No_Domains_Assigned() + { + var domainServiceMock = new Mock(); + domainServiceMock.Setup(x => x.GetAssignedDomains(It.IsAny(), It.IsAny())) + .Returns(Enumerable.Empty()); + + var rootNode = new ContentBuilder() + .WithContentType(CreateContentType()) + .WithId(1060) + .AddContentCultureInfosCollection() + .AddCultureInfos() + .WithCultureIso("da-dk") + .Done() + .AddCultureInfos() + .WithCultureIso("en-us") + .Done() + .Done() + .Build(); + + var culturesPublished = new[] { "en-us" }; + var notifications = new SimpleNotificationModel(); + + var contentController = CreateContentController(domainServiceMock.Object); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); + Assert.IsEmpty(notifications.Notifications); + } + [Test] public void Ancestor_Domains_Counts() { @@ -189,7 +217,7 @@ public class ContentControllerTests var contentController = CreateContentController(domainServiceMock.Object); var notifications = new SimpleNotificationModel(); - contentController.AddDomainWarnings(level3Node, culturesPublished, notifications); + contentController.AddDomainWarnings(level3Node, culturesPublished, notifications, "en-us"); // We expect one error because all domains except "de-de" is registered somewhere in the ancestor path Assert.AreEqual(1, notifications.Notifications.Count(x => x.NotificationType == NotificationStyle.Warning)); @@ -225,7 +253,7 @@ public class ContentControllerTests var notifications = new SimpleNotificationModel(); var contentController = CreateContentController(domainServiceMock.Object); - contentController.AddDomainWarnings(rootNode, culturesPublished, notifications); + contentController.AddDomainWarnings(rootNode, culturesPublished, notifications, "en-us"); // We only get two errors, one for each culture being published, so no errors from previously published cultures. Assert.AreEqual(2, notifications.Notifications.Count(x => x.NotificationType == NotificationStyle.Warning));