From c8593c23e6a60dfac11114cd4db44bea0154d65c Mon Sep 17 00:00:00 2001 From: Mads Rasmussen Date: Wed, 12 Dec 2018 11:59:08 +0100 Subject: [PATCH 01/12] show correct createDate for variants, only show contextual variant status, only show context variant urls, add variant log colors --- .../content/umbcontentnodeinfo.directive.js | 89 ++++++++++--------- .../content/umb-content-node-info.html | 23 ++--- 2 files changed, 57 insertions(+), 55 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js index 31e847f0f6..232e31daee 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js @@ -10,6 +10,8 @@ var auditTrailLoaded = false; var labels = {}; scope.publishStatus = []; + scope.currentVariant = null; + scope.currentUrls = []; scope.disableTemplates = Umbraco.Sys.ServerVariables.features.disabledFeatures.disableTemplates; scope.allowChangeDocumentType = false; @@ -17,6 +19,18 @@ function onInit() { + // set currentVariant + scope.currentVariant = _.find(scope.node.variants, (v) => v.active); + + // find the urls for the currently selected language + if(scope.node.variants.length > 1) { + // nodes with variants + scope.currentUrls = _.filter(scope.node.urls, (url) => scope.currentVariant.language.culture === url.culture); + } else { + // invariant nodes + scope.currentUrls = scope.node.urls; + } + // if there are any infinite editors open we are in infinite editing scope.isInfiniteMode = editorService.getNumberOfEditors() > 0 ? true : false; @@ -50,7 +64,7 @@ labels.unsavedChanges = data[5]; labels.doctypeChangeWarning = data[6]; - setNodePublishStatus(scope.node); + setNodePublishStatus(); }); @@ -58,6 +72,9 @@ "id": scope.node.id }; + // make sure dates are formatted to the user's locale + formatDatesToLocal(); + // get available templates scope.availableTemplates = scope.node.allowedTemplates; @@ -221,12 +238,13 @@ function setAuditTrailLogTypeColor(auditTrail) { angular.forEach(auditTrail, function (item) { - switch (item.logType) { case "Publish": + case "PublishVariant": item.logTypeColor = "success"; break; case "Unpublish": + case "UnpublishVariant": case "Delete": item.logTypeColor = "danger"; break; @@ -236,51 +254,40 @@ }); } - function setNodePublishStatus(node) { + function setNodePublishStatus() { - scope.publishStatus = []; + scope.status = {}; // deleted node - if (node.trashed === true) { - scope.publishStatus.push({ - label: labels.deleted, - color: "danger" - }); + if (scope.node.trashed === true) { + scope.status.color = "danger"; return; } - if (node.variants) { - for (var i = 0; i < node.variants.length; i++) { - - var variant = node.variants[i]; - - var status = { - culture: variant.language ? variant.language.culture : null - }; - - if (variant.state === "NotCreated") { - status.label = labels.notCreated; - status.color = "gray"; - } - else if (variant.state === "Draft") { - // draft node - status.label = labels.unpublished; - status.color = "gray"; - } - else if (variant.state === "Published") { - // published node - status.label = labels.published; - status.color = "success"; - } - else if (variant.state === "PublishedPendingChanges") { - // published node with pending changes - status.label = labels.publishedPendingChanges; - status.color = "success"; - } - - scope.publishStatus.push(status); - } + // variant status + if (scope.currentVariant.state === "NotCreated") { + // not created + scope.status.color = "gray"; } + else if (scope.currentVariant.state === "Draft") { + // draft node + scope.status.color = "gray"; + } + else if (scope.currentVariant.state === "Published") { + // published node + scope.status.color = "success"; + } + else if (scope.currentVariant.state === "PublishedPendingChanges") { + // published node with pending changes + scope.status.color = "success"; + } + } + + function formatDatesToLocal() { + // get current backoffice user and format dates + userService.getCurrentUser().then(function (currentUser) { + scope.currentVariant.createDateFormatted = dateHelper.getLocalDate(scope.currentVariant.createDate, currentUser.locale, 'LLL'); + }); } // load audit trail and redirects when on the info tab @@ -306,7 +313,7 @@ auditTrailLoaded = false; loadAuditTrail(); loadRedirectUrls(); - setNodePublishStatus(scope.node); + setNodePublishStatus(); } }); diff --git a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html index 429cceb4d9..e880fc0f4e 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html @@ -2,21 +2,19 @@
- + @@ -62,8 +60,9 @@
- +
@@ -96,7 +95,6 @@ class="history-item__badge" size="xs" color="{{item.logTypeColor}}"> - {{ item.logType }} @@ -130,16 +128,13 @@ -
- {{status.culture}}: - - {{status.label}} - -
+ + + - {{node.createDateFormatted}} by {{ node.owner.name }} + {{currentVariant.createDateFormatted}} From 4f6f959d7fd9ba96bb2ea926ac02cb0fec62a401 Mon Sep 17 00:00:00 2001 From: Mads Rasmussen Date: Wed, 12 Dec 2018 13:12:13 +0100 Subject: [PATCH 02/12] Add missing translations --- src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml b/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml index 775b40fd0c..498744f5cf 100644 --- a/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml +++ b/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml @@ -147,8 +147,9 @@ Viewing for Content deleted Content unpublished - Content saved and Published - Content saved and published for languages: %0% + Content unpublished for languages: %0% + Content published + Content published for languages: %0% Content saved Content saved for languages: %0% Content moved @@ -165,6 +166,7 @@ Save Delete Unpublish + Unpublish Rollback Send To Publish Send To Publish From defa231c4bcdc54689386f98dd0270793476eee4 Mon Sep 17 00:00:00 2001 From: Mads Rasmussen Date: Wed, 12 Dec 2018 14:29:20 +0100 Subject: [PATCH 03/12] add label for history --- .../components/content/umbcontentnodeinfo.directive.js | 8 ++++++-- .../views/components/content/umb-content-node-info.html | 3 ++- src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml | 1 + 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js index 232e31daee..7b56bb2bd0 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js @@ -51,7 +51,9 @@ "content_publishedPendingChanges", "content_notCreated", "prompt_unsavedChanges", - "prompt_doctypeChangeWarning" + "prompt_doctypeChangeWarning", + "general_history", + "auditTrails_historyIncludingVariants" ]; localizationService.localizeMany(keys) @@ -63,7 +65,9 @@ labels.notCreated = data[4]; labels.unsavedChanges = data[5]; labels.doctypeChangeWarning = data[6]; - + + scope.historyLabel = scope.node.variants && scope.node.variants.length === 1 ? data[7] : data[8]; + setNodePublishStatus(); }); diff --git a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html index e880fc0f4e..b37e3c7ba4 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html @@ -41,7 +41,8 @@ + title="{{historyLabel}}"> + Send To Publish Send To Publish Sort + History (all variants) To change the document type for the selected content, first select from the list of valid types for this location. From a19a58796fb04f6aa62b308689f8726ebd6b232f Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 18 Dec 2018 22:02:39 +1100 Subject: [PATCH 04/12] Changes IUrlProvider to return UrlInfo --- .../Routing/UrlsProviderWithDomainsTests.cs | 4 +- .../TestControllerActivatorBase.cs | 2 +- .../Testing/TestingTests/MockTests.cs | 2 +- .../Web/TemplateUtilitiesTests.cs | 2 +- src/Umbraco.Web/Routing/AliasUrlProvider.cs | 29 ++++----- .../Routing/CustomRouteUrlProvider.cs | 16 +++-- src/Umbraco.Web/Routing/DefaultUrlProvider.cs | 27 ++++---- src/Umbraco.Web/Routing/IUrlProvider.cs | 4 +- src/Umbraco.Web/Routing/UrlInfo.cs | 65 ++++++++++++++++--- src/Umbraco.Web/Routing/UrlProvider.cs | 10 +-- .../Routing/UrlProviderExtensions.cs | 46 ++++--------- 11 files changed, 118 insertions(+), 89 deletions(-) diff --git a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs index c6bd0fdb88..2f1bef3b9a 100644 --- a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs +++ b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs @@ -418,8 +418,8 @@ namespace Umbraco.Tests.Routing foreach (var x in result) Console.WriteLine(x); Assert.AreEqual(2, result.Length); - Assert.IsTrue(result.Contains("http://domain1a.com/en/1001-1-1/")); - Assert.IsTrue(result.Contains("http://domain1b.com/en/1001-1-1/")); + Assert.AreEqual(result[0].Text, "http://domain1a.com/en/1001-1-1/"); + Assert.AreEqual(result[1].Text, "http://domain1b.com/en/1001-1-1/"); } } } diff --git a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs index 2cf64f04d1..7152622b07 100644 --- a/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs +++ b/src/Umbraco.Tests/TestHelpers/ControllerTesting/TestControllerActivatorBase.cs @@ -159,7 +159,7 @@ namespace Umbraco.Tests.TestHelpers.ControllerTesting var urlHelper = new Mock(); urlHelper.Setup(provider => provider.GetUrl(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns("/hello/world/1234"); + .Returns(UrlInfo.Url("/hello/world/1234")); var membershipHelper = new MembershipHelper(umbCtx, Mock.Of(), Mock.Of()); diff --git a/src/Umbraco.Tests/Testing/TestingTests/MockTests.cs b/src/Umbraco.Tests/Testing/TestingTests/MockTests.cs index 51855f7e19..5b93de0c09 100644 --- a/src/Umbraco.Tests/Testing/TestingTests/MockTests.cs +++ b/src/Umbraco.Tests/Testing/TestingTests/MockTests.cs @@ -75,7 +75,7 @@ namespace Umbraco.Tests.Testing.TestingTests var urlProviderMock = new Mock(); urlProviderMock.Setup(provider => provider.GetUrl(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns("/hello/world/1234"); + .Returns(UrlInfo.Url("/hello/world/1234")); var urlProvider = urlProviderMock.Object; var theUrlProvider = new UrlProvider(umbracoContext, new [] { urlProvider }, umbracoContext.VariationContextAccessor); diff --git a/src/Umbraco.Tests/Web/TemplateUtilitiesTests.cs b/src/Umbraco.Tests/Web/TemplateUtilitiesTests.cs index 1e6229fa4c..d98afa6bcc 100644 --- a/src/Umbraco.Tests/Web/TemplateUtilitiesTests.cs +++ b/src/Umbraco.Tests/Web/TemplateUtilitiesTests.cs @@ -79,7 +79,7 @@ namespace Umbraco.Tests.Web var testUrlProvider = new Mock(); testUrlProvider .Setup(x => x.GetUrl(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((UmbracoContext umbCtx, IPublishedContent content, UrlProviderMode mode, string culture, Uri url) => "/my-test-url"); + .Returns((UmbracoContext umbCtx, IPublishedContent content, UrlProviderMode mode, string culture, Uri url) => UrlInfo.Url("/my-test-url")); var globalSettings = SettingsForTests.GenerateMockGlobalSettings(); diff --git a/src/Umbraco.Web/Routing/AliasUrlProvider.cs b/src/Umbraco.Web/Routing/AliasUrlProvider.cs index 8c851d139f..4c879e931f 100644 --- a/src/Umbraco.Web/Routing/AliasUrlProvider.cs +++ b/src/Umbraco.Web/Routing/AliasUrlProvider.cs @@ -31,7 +31,7 @@ namespace Umbraco.Web.Routing #region GetUrl /// - public string GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) + public UrlInfo GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) { return null; // we have nothing to say } @@ -51,13 +51,14 @@ namespace Umbraco.Web.Routing /// Other urls are those that GetUrl would not return in the current context, but would be valid /// urls for the node in other contexts (different domain for current request, umbracoUrlAlias...). /// - public IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) + public IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) { var node = umbracoContext.ContentCache.GetById(id); - if (node == null) return Enumerable.Empty(); + if (node == null) + yield break; if (!node.HasProperty(Constants.Conventions.Content.UrlAlias)) - return Enumerable.Empty(); + yield break; var domainHelper = umbracoContext.GetDomainHelper(_siteDomainHelper); @@ -82,20 +83,19 @@ namespace Umbraco.Web.Routing // the content finder may work, depending on the 'current' culture, // but there's no way we can return something meaningful here if (varies) - return Enumerable.Empty(); + yield break; var umbracoUrlName = node.Value(Constants.Conventions.Content.UrlAlias); if (string.IsNullOrWhiteSpace(umbracoUrlName)) - return Enumerable.Empty(); + yield break; var path = "/" + umbracoUrlName; var uri = new Uri(path, UriKind.Relative); - return new[] { UriUtility.UriFromUmbraco(uri, _globalSettings, _requestConfig).ToString() }; + yield return UrlInfo.Url(UriUtility.UriFromUmbraco(uri, _globalSettings, _requestConfig).ToString()); } else { // some domains: one url per domain, which is "/" - var result = new List(); foreach(var domainUri in domainUris) { // if the property is invariant, get the invariant value, url is "/" @@ -108,14 +108,13 @@ namespace Umbraco.Web.Routing ? node.Value(Constants.Conventions.Content.UrlAlias, culture: domainUri.Culture.Name) : node.Value(Constants.Conventions.Content.UrlAlias); - if (!string.IsNullOrWhiteSpace(umbracoUrlName)) - { - var path = "/" + umbracoUrlName; - var uri = new Uri(CombinePaths(domainUri.Uri.GetLeftPart(UriPartial.Path), path)); - result.Add(UriUtility.UriFromUmbraco(uri, _globalSettings, _requestConfig).ToString()); - } + if (string.IsNullOrWhiteSpace(umbracoUrlName)) + continue; + + var path = "/" + umbracoUrlName; + var uri = new Uri(CombinePaths(domainUri.Uri.GetLeftPart(UriPartial.Path), path)); + yield return UrlInfo.Url(UriUtility.UriFromUmbraco(uri, _globalSettings, _requestConfig).ToString(), domainUri.Culture.Name); } - return result; } } diff --git a/src/Umbraco.Web/Routing/CustomRouteUrlProvider.cs b/src/Umbraco.Web/Routing/CustomRouteUrlProvider.cs index ce1fc1ffab..ae17ff6258 100644 --- a/src/Umbraco.Web/Routing/CustomRouteUrlProvider.cs +++ b/src/Umbraco.Web/Routing/CustomRouteUrlProvider.cs @@ -15,7 +15,7 @@ namespace Umbraco.Web.Routing /// /// This will return the URL that is returned by the assigned custom if this is a custom route /// - public string GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) + public UrlInfo GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) { if (umbracoContext?.PublishedRequest?.PublishedContent == null) return null; if (umbracoContext.HttpContext?.Request?.RequestContext?.RouteData?.DataTokens == null) return null; @@ -26,13 +26,15 @@ namespace Umbraco.Web.Routing //NOTE: This looks like it might cause an infinite loop because PublishedContentBase.Url calls into UmbracoContext.Current.UrlProvider.GetUrl which calls back into the IUrlProvider pipeline // but the specific purpose of this is that a developer is using their own IPublishedContent that returns a specific Url and doesn't go back into the UrlProvider pipeline. //TODO: We could put a try/catch here just in case, else we could do some reflection checking to see if the implementation is PublishedContentBase and the Url property is not overridden. - return content.Id == umbracoContext.PublishedRequest.PublishedContent.Id - ? umbracoContext.PublishedRequest.PublishedContent.GetUrl(culture) - : null; + return UrlInfo.Url( + content.Id == umbracoContext.PublishedRequest.PublishedContent.Id + ? umbracoContext.PublishedRequest.PublishedContent.GetUrl(culture) + : null, + culture); } /// - /// This always returns null because this url provider is used purely to deal with Umbraco custom routes with + /// This always returns an empty result because this url provider is used purely to deal with Umbraco custom routes with /// UmbracoVirtualNodeRouteHandler, we really only care about the normal URL so that RedirectToCurrentUmbracoPage() works /// with SurfaceControllers /// @@ -40,9 +42,9 @@ namespace Umbraco.Web.Routing /// /// /// - public IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) + public IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) { - return null; + yield break; } } } diff --git a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs index 61437b6640..5045b1af96 100644 --- a/src/Umbraco.Web/Routing/DefaultUrlProvider.cs +++ b/src/Umbraco.Web/Routing/DefaultUrlProvider.cs @@ -29,7 +29,7 @@ namespace Umbraco.Web.Routing #region GetUrl /// - public virtual string GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) + public virtual UrlInfo GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current) { if (!current.IsAbsoluteUri) throw new ArgumentException("Current url must be absolute.", nameof(current)); @@ -39,7 +39,7 @@ namespace Umbraco.Web.Routing return GetUrlFromRoute(route, umbracoContext, content.Id, current, mode, culture); } - internal string GetUrlFromRoute(string route, UmbracoContext umbracoContext, int id, Uri current, UrlProviderMode mode, string culture) + internal UrlInfo GetUrlFromRoute(string route, UmbracoContext umbracoContext, int id, Uri current, UrlProviderMode mode, string culture) { if (string.IsNullOrWhiteSpace(route)) { @@ -58,7 +58,9 @@ namespace Umbraco.Web.Routing : domainHelper.DomainForNode(int.Parse(route.Substring(0, pos)), current, culture); // assemble the url from domainUri (maybe null) and path - return AssembleUrl(domainUri, path, current, mode).ToString(); + var url = AssembleUrl(domainUri, path, current, mode).ToString(); + + return UrlInfo.Url(url, culture); } #endregion @@ -76,10 +78,11 @@ namespace Umbraco.Web.Routing /// Other urls are those that GetUrl would not return in the current context, but would be valid /// urls for the node in other contexts (different domain for current request, umbracoUrlAlias...). /// - public virtual IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) + public virtual IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current) { var node = umbracoContext.ContentCache.GetById(id); - if (node == null) return Enumerable.Empty(); + if (node == null) + yield break; var domainHelper = umbracoContext.GetDomainHelper(_siteDomainHelper); @@ -94,13 +97,14 @@ namespace Umbraco.Web.Routing // no domains = exit if (domainUris ==null) - return Enumerable.Empty(); + yield break; - var result = new List(); foreach (var d in domainUris) { + var culture = d?.Culture?.Name; + //although we are passing in culture here, if any node in this path is invariant, it ignores the culture anyways so this is ok - var route = umbracoContext.ContentCache.GetRouteById(id, d?.Culture?.Name); + var route = umbracoContext.ContentCache.GetRouteById(id, culture); if (route == null) continue; //need to strip off the leading ID for the route if it exists (occurs if the route is for a node with a domain assigned) @@ -109,9 +113,8 @@ namespace Umbraco.Web.Routing var uri = new Uri(CombinePaths(d.Uri.GetLeftPart(UriPartial.Path), path)); uri = UriUtility.UriFromUmbraco(uri, _globalSettings, _requestSettings); - result.Add(uri.ToString()); + yield return UrlInfo.Url(uri.ToString(), culture); } - return result; } #endregion @@ -146,7 +149,7 @@ namespace Umbraco.Web.Routing uri = new Uri(path, UriKind.Relative); break; default: - throw new ArgumentOutOfRangeException("mode"); + throw new ArgumentOutOfRangeException(nameof(mode)); } } else // a domain was found @@ -169,7 +172,7 @@ namespace Umbraco.Web.Routing uri = new Uri(CombinePaths(domainUri.Uri.AbsolutePath, path), UriKind.Relative); break; default: - throw new ArgumentOutOfRangeException("mode"); + throw new ArgumentOutOfRangeException(nameof(mode)); } } diff --git a/src/Umbraco.Web/Routing/IUrlProvider.cs b/src/Umbraco.Web/Routing/IUrlProvider.cs index 0f102dceb8..55d39880d6 100644 --- a/src/Umbraco.Web/Routing/IUrlProvider.cs +++ b/src/Umbraco.Web/Routing/IUrlProvider.cs @@ -24,7 +24,7 @@ namespace Umbraco.Web.Routing /// when no culture is specified, the current culture. /// If the provider is unable to provide a url, it should return null. /// - string GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current); + UrlInfo GetUrl(UmbracoContext umbracoContext, IPublishedContent content, UrlProviderMode mode, string culture, Uri current); /// /// Gets the other urls of a published content. @@ -37,6 +37,6 @@ namespace Umbraco.Web.Routing /// Other urls are those that GetUrl would not return in the current context, but would be valid /// urls for the node in other contexts (different domain for current request, umbracoUrlAlias...). /// - IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current); + IEnumerable GetOtherUrls(UmbracoContext umbracoContext, int id, Uri current); } } diff --git a/src/Umbraco.Web/Routing/UrlInfo.cs b/src/Umbraco.Web/Routing/UrlInfo.cs index 01dbe4a0e1..a795f1577b 100644 --- a/src/Umbraco.Web/Routing/UrlInfo.cs +++ b/src/Umbraco.Web/Routing/UrlInfo.cs @@ -1,4 +1,5 @@ -using System.Runtime.Serialization; +using System; +using System.Runtime.Serialization; namespace Umbraco.Web.Routing { @@ -6,13 +7,25 @@ namespace Umbraco.Web.Routing /// Represents infos for a url. /// [DataContract(Name = "urlInfo", Namespace = "")] - public class UrlInfo + public class UrlInfo : IEquatable { + + /// + /// Creates a instance representing a true url. + /// + public static UrlInfo Url(string text, string culture = null) => new UrlInfo(text, true, culture); + + /// + /// Creates a instance representing a message. + /// + public static UrlInfo Message(string text, string culture = null) => new UrlInfo(text, false, culture); + /// /// Initializes a new instance of the class. /// - public UrlInfo(string text, bool isUrl, string culture) + private UrlInfo(string text, bool isUrl, string culture) { + if (string.IsNullOrWhiteSpace(text)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(text)); IsUrl = isUrl; Text = text; Culture = culture; @@ -38,13 +51,47 @@ namespace Umbraco.Web.Routing public string Text { get; } /// - /// Creates a instance representing a true url. + /// Checks equality /// - public static UrlInfo Url(string text, string culture = null) => new UrlInfo(text, true, culture); + /// + /// + /// + /// Compare both culture and Text as invariant strings since URLs are not case sensitive, nor are culture names within Umbraco + /// + public bool Equals(UrlInfo other) + { + if (ReferenceEquals(null, other)) return false; + if (ReferenceEquals(this, other)) return true; + return string.Equals(Culture, other.Culture, StringComparison.InvariantCultureIgnoreCase) && IsUrl == other.IsUrl && string.Equals(Text, other.Text, StringComparison.InvariantCultureIgnoreCase); + } - /// - /// Creates a instance representing a message. - /// - public static UrlInfo Message(string text, string culture = null) => new UrlInfo(text, false, culture); + public override bool Equals(object obj) + { + if (ReferenceEquals(null, obj)) return false; + if (ReferenceEquals(this, obj)) return true; + if (obj.GetType() != this.GetType()) return false; + return Equals((UrlInfo) obj); + } + + public override int GetHashCode() + { + unchecked + { + var hashCode = (Culture != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(Culture) : 0); + hashCode = (hashCode * 397) ^ IsUrl.GetHashCode(); + hashCode = (hashCode * 397) ^ (Text != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(Text) : 0); + return hashCode; + } + } + + public static bool operator ==(UrlInfo left, UrlInfo right) + { + return Equals(left, right); + } + + public static bool operator !=(UrlInfo left, UrlInfo right) + { + return !Equals(left, right); + } } } diff --git a/src/Umbraco.Web/Routing/UrlProvider.cs b/src/Umbraco.Web/Routing/UrlProvider.cs index b265d48923..491deee9e3 100644 --- a/src/Umbraco.Web/Routing/UrlProvider.cs +++ b/src/Umbraco.Web/Routing/UrlProvider.cs @@ -202,7 +202,7 @@ namespace Umbraco.Web.Routing var url = _urlProviders.Select(provider => provider.GetUrl(_umbracoContext, content, mode, culture, current)) .FirstOrDefault(u => u != null); - return url ?? "#"; // legacy wants this + return url?.Text ?? "#"; // legacy wants this } internal string GetUrlFromRoute(int id, string route, string culture) @@ -210,7 +210,7 @@ namespace Umbraco.Web.Routing var provider = _urlProviders.OfType().FirstOrDefault(); var url = provider == null ? route // what else? - : provider.GetUrlFromRoute(route, UmbracoContext.Current, id, _umbracoContext.CleanedUmbracoUrl, Mode, culture); + : provider.GetUrlFromRoute(route, UmbracoContext.Current, id, _umbracoContext.CleanedUmbracoUrl, Mode, culture)?.Text; return url ?? "#"; } @@ -228,7 +228,7 @@ namespace Umbraco.Web.Routing /// urls for the node in other contexts (different domain for current request, umbracoUrlAlias...). /// The results depend on the current url. /// - public IEnumerable GetOtherUrls(int id) + public IEnumerable GetOtherUrls(int id) { return GetOtherUrls(id, _umbracoContext.CleanedUmbracoUrl); } @@ -243,10 +243,10 @@ namespace Umbraco.Web.Routing /// Other urls are those that GetUrl would not return in the current context, but would be valid /// urls for the node in other contexts (different domain for current request, umbracoUrlAlias...). /// - public IEnumerable GetOtherUrls(int id, Uri current) + public IEnumerable GetOtherUrls(int id, Uri current) { // providers can return null or an empty list or a non-empty list, be prepared - var urls = _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current) ?? Enumerable.Empty()); + var urls = _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current)); return urls; } diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index 14b9c0a465..3a0caeec78 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -34,14 +34,11 @@ namespace Umbraco.Web.Routing if (contentService == null) throw new ArgumentNullException(nameof(contentService)); if (logger == null) throw new ArgumentNullException(nameof(logger)); - var urls = new List(); - if (content.Published == false) - { - urls.Add(UrlInfo.Message(textService.Localize("content/itemNotPublished"))); - return urls; - } - + yield return UrlInfo.Message(textService.Localize("content/itemNotPublished")); + + var urls = new HashSet(); + // build a list of urls, for the back-office // which will contain // - the 'main' urls, which is what .Url would return, for each culture @@ -92,37 +89,18 @@ namespace Umbraco.Web.Routing } } - // prepare for de-duplication - var durl = new Dictionary>(); - var dmsg = new Dictionary>(); - foreach (var url in urls) - { - var d = url.IsUrl ? durl : dmsg; - if (!d.TryGetValue(url.Text, out var l)) - d[url.Text] = l = new List(); - l.Add(url); - } - - // deduplicate, order urls first then messages, concatenate cultures (hide if 'all') - var ret = new List(); - foreach (var (text, infos) in durl) - ret.Add(UrlInfo.Url(text, infos.Count == cultures.Count ? null : string.Join(", ", infos.Select(x => x.Culture)))); - foreach (var (text, infos) in dmsg) - ret.Add(UrlInfo.Message(text, infos.Count == cultures.Count ? null : string.Join(", ", infos.Select(x => x.Culture)))); + //return the real urls first, then the messages + foreach (var urlGroup in urls.GroupBy(x => x.IsUrl).OrderByDescending(x => x.Key)) + foreach (var url in urlGroup) + yield return url; // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. - // also, we are not dealing with cultures at all - that will have to wait - foreach(var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) - { - if (urls.Any(x => x.IsUrl && x.Text == otherUrl)) continue; - ret.Add(UrlInfo.Url(otherUrl)); - } - - return ret; + foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) + yield return otherUrl; } - private static void HandleCouldNotGetUrl(IContent content, string culture, List urls, IContentService contentService, ILocalizedTextService textService) + private static void HandleCouldNotGetUrl(IContent content, string culture, ICollection urls, IContentService contentService, ILocalizedTextService textService) { // document has a published version yet its url is "#" => a parent must be // unpublished, walk up the tree until we find it, and report. @@ -143,7 +121,7 @@ namespace Umbraco.Web.Routing urls.Add(UrlInfo.Message(textService.Localize("content/parentCultureNotPublished", new[] { parent.Name }), culture)); } - private static bool DetectCollision(IContent content, string url, List urls, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService) + private static bool DetectCollision(IContent content, string url, ICollection urls, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService) { // test for collisions on the 'main' url var uri = new Uri(url.TrimEnd('/'), UriKind.RelativeOrAbsolute); From 7047a07660546559088f207d2ff2f5b94b385433 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 19 Dec 2018 00:03:49 +1100 Subject: [PATCH 05/12] Shows the culture for URLs if the node is invariant but it has cultural URLs (it exists underneath a variant) --- .../Routing/UrlsProviderWithDomainsTests.cs | 4 +- src/Umbraco.Web/Routing/DomainHelper.cs | 8 +- src/Umbraco.Web/Routing/UrlInfo.cs | 2 +- src/Umbraco.Web/Routing/UrlInfoComparer.cs | 66 ++++++++++++++ .../Routing/UrlProviderExtensions.cs | 89 +++++++++++++------ src/Umbraco.Web/Umbraco.Web.csproj | 1 + 6 files changed, 141 insertions(+), 29 deletions(-) create mode 100644 src/Umbraco.Web/Routing/UrlInfoComparer.cs diff --git a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs index 2f1bef3b9a..c7ce42e4bc 100644 --- a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs +++ b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs @@ -418,8 +418,8 @@ namespace Umbraco.Tests.Routing foreach (var x in result) Console.WriteLine(x); Assert.AreEqual(2, result.Length); - Assert.AreEqual(result[0].Text, "http://domain1a.com/en/1001-1-1/"); - Assert.AreEqual(result[1].Text, "http://domain1b.com/en/1001-1-1/"); + Assert.AreEqual(result[0].Text, "http://domain1b.com/en/1001-1-1/"); + Assert.AreEqual(result[1].Text, "http://domain1a.com/en/1001-1-1/"); } } } diff --git a/src/Umbraco.Web/Routing/DomainHelper.cs b/src/Umbraco.Web/Routing/DomainHelper.cs index b6d79e788a..9b300009d0 100644 --- a/src/Umbraco.Web/Routing/DomainHelper.cs +++ b/src/Umbraco.Web/Routing/DomainHelper.cs @@ -207,8 +207,14 @@ namespace Umbraco.Web.Routing var cultureDomains = domainsAndUris.Where(x => x.Culture.Name.InvariantEquals(culture)).ToList(); if (cultureDomains.Count > 0) return cultureDomains; + // if a culture is supplied, we *want* a url for that culture, else fail - throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); + //throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); + //fixme: Review - throwing here causes a problem because the UrlProviderExtensions.GetContentUrls iterates through + // ALL cultures even if those cultures are not assigned for use within a branch. Returning null + // here fixes that problem and the URLs resolve correctly, however i don't know if this is causing other + // residual problems. It would also suggest that below in GetByCulture we don't throw either but instead return null?? + return null; } if (defaultCulture != null) // try the defaultCulture culture diff --git a/src/Umbraco.Web/Routing/UrlInfo.cs b/src/Umbraco.Web/Routing/UrlInfo.cs index a795f1577b..ae5c4b412c 100644 --- a/src/Umbraco.Web/Routing/UrlInfo.cs +++ b/src/Umbraco.Web/Routing/UrlInfo.cs @@ -23,7 +23,7 @@ namespace Umbraco.Web.Routing /// /// Initializes a new instance of the class. /// - private UrlInfo(string text, bool isUrl, string culture) + public UrlInfo(string text, bool isUrl, string culture) { if (string.IsNullOrWhiteSpace(text)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(text)); IsUrl = isUrl; diff --git a/src/Umbraco.Web/Routing/UrlInfoComparer.cs b/src/Umbraco.Web/Routing/UrlInfoComparer.cs new file mode 100644 index 0000000000..ff41dc6bc9 --- /dev/null +++ b/src/Umbraco.Web/Routing/UrlInfoComparer.cs @@ -0,0 +1,66 @@ +using System; +using System.Collections.Generic; + +namespace Umbraco.Web.Routing +{ + /// + /// Compares + /// + public class UrlInfoComparer : IEqualityComparer + { + private readonly bool _variesByCulture; + + public UrlInfoComparer(bool variesByCulture) + { + _variesByCulture = variesByCulture; + } + + /// + /// Determines equality between + /// + /// + /// + /// + /// + /// If variesByCulture is true, then culture is compared, otherwise culture is not compared. + /// Both culture and url are compared without case sensitivity. + /// + public bool Equals(UrlInfo x, UrlInfo y) + { + if (ReferenceEquals(null, y)) return false; + if (ReferenceEquals(null, x)) return false; + if (ReferenceEquals(x, y)) return true; + + if (_variesByCulture) + { + return string.Equals(x.Culture, y.Culture, StringComparison.InvariantCultureIgnoreCase) + && x.IsUrl == y.IsUrl + && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); + } + + return x.IsUrl == y.IsUrl + && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); + } + + /// + /// Calculates a hash code + /// + /// + /// + /// + /// If variesByCulture is true then culture is used in the calculation, otherwise it's not + /// + public int GetHashCode(UrlInfo obj) + { + unchecked + { + var hashCode = (obj.Text != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Text) : 0); + hashCode = (hashCode * 397) ^ obj.IsUrl.GetHashCode(); + if (_variesByCulture) + hashCode = (hashCode * 397) ^ (obj.Culture != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Culture) : 0); + return hashCode; + } + + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index 3a0caeec78..71a0c294ab 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -18,7 +18,7 @@ namespace Umbraco.Web.Routing /// Use when displaying Urls. If errors occur when generating the Urls, they will show in the list. /// Contains all the Urls that we can figure out (based upon domains, etc). /// - public static IEnumerable GetContentUrls(this IContent content, + public static IEnumerable GetContentUrls(this IContent content, PublishedRouter publishedRouter, UmbracoContext umbracoContext, ILocalizationService localizationService, @@ -44,12 +44,57 @@ namespace Umbraco.Web.Routing // - the 'main' urls, which is what .Url would return, for each culture // - the 'other' urls we know (based upon domains, etc) // - // need to work on each culture. - // on invariant trees, each culture return the same thing - // but, we don't know if the tree to this content is invariant + // need to work through each installed culture. + // fixme: Why not just work with each culture assigned to domains in the branch? + // on invariant nodes, each culture returns the same url segment + // but, we don't know if the branch to this content is invariant so we need to ask + // for URLs for all cultures. var cultures = localizationService.GetAllLanguages().Select(x => x.IsoCode).ToList(); + //get all URLs for all cultures + foreach (var cultureUrl in GetContentUrlsByCulture(content, cultures, publishedRouter, umbracoContext, contentService, textService, logger)) + { + urls.Add(cultureUrl); + } + + //return the real urls first, then the messages + foreach (var urlGroup in urls.GroupBy(x => x.IsUrl).OrderByDescending(x => x.Key)) + { + //in some cases there will be the same URL for multiple cultures: + // * The entire branch is invariant + // * If there are less domain/cultures assigned to the branch than the number of cultures/languages installed + + foreach (var dUrl in urlGroup.DistinctBy(x => x.Text.ToUpperInvariant())) + yield return dUrl; + } + + // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. + // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. + foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) + if (urls.Add(otherUrl)) //avoid duplicates + yield return otherUrl; + } + + /// + /// Tries to return a for each culture for the content while detecting collisions/errors + /// + /// + /// + /// + /// + /// + /// + /// + /// + private static IEnumerable GetContentUrlsByCulture(IContent content, + IEnumerable cultures, + PublishedRouter publishedRouter, + UmbracoContext umbracoContext, + IContentService contentService, + ILocalizedTextService textService, + ILogger logger) + { foreach (var culture in cultures) { // if content is variant, and culture is not published, skip @@ -73,34 +118,26 @@ namespace Umbraco.Web.Routing { // deal with 'could not get the url' case "#": - HandleCouldNotGetUrl(content, culture, urls, contentService, textService); + yield return HandleCouldNotGetUrl(content, culture, contentService, textService); break; // deal with exceptions case "#ex": - urls.Add(UrlInfo.Message(textService.Localize("content/getUrlException"), culture)); + yield return UrlInfo.Message(textService.Localize("content/getUrlException"), culture); break; // got a url, deal with collisions, add url default: - if (!DetectCollision(content, url, urls, culture, umbracoContext, publishedRouter, textService)) // detect collisions, etc - urls.Add(UrlInfo.Url(url, culture)); + if (DetectCollision(content, url, culture, umbracoContext, publishedRouter, textService, out var urlInfo)) // detect collisions, etc + yield return urlInfo; + else + yield return UrlInfo.Url(url, culture); break; } } - - //return the real urls first, then the messages - foreach (var urlGroup in urls.GroupBy(x => x.IsUrl).OrderByDescending(x => x.Key)) - foreach (var url in urlGroup) - yield return url; - - // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. - // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. - foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) - yield return otherUrl; } - private static void HandleCouldNotGetUrl(IContent content, string culture, ICollection urls, IContentService contentService, ILocalizedTextService textService) + private static UrlInfo HandleCouldNotGetUrl(IContent content, string culture, IContentService contentService, ILocalizedTextService textService) { // document has a published version yet its url is "#" => a parent must be // unpublished, walk up the tree until we find it, and report. @@ -112,16 +149,16 @@ namespace Umbraco.Web.Routing while (parent != null && parent.Published && (!parent.ContentType.VariesByCulture() || parent.IsCulturePublished(culture))); if (parent == null) // oops, internal error - urls.Add(UrlInfo.Message(textService.Localize("content/parentNotPublishedAnomaly"), culture)); + return UrlInfo.Message(textService.Localize("content/parentNotPublishedAnomaly"), culture); else if (!parent.Published) // totally not published - urls.Add(UrlInfo.Message(textService.Localize("content/parentNotPublished", new[] { parent.Name }), culture)); + return UrlInfo.Message(textService.Localize("content/parentNotPublished", new[] {parent.Name}), culture); else // culture not published - urls.Add(UrlInfo.Message(textService.Localize("content/parentCultureNotPublished", new[] { parent.Name }), culture)); + return UrlInfo.Message(textService.Localize("content/parentCultureNotPublished", new[] {parent.Name}), culture); } - private static bool DetectCollision(IContent content, string url, ICollection urls, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService) + private static bool DetectCollision(IContent content, string url, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService, out UrlInfo urlInfo) { // test for collisions on the 'main' url var uri = new Uri(url.TrimEnd('/'), UriKind.RelativeOrAbsolute); @@ -130,9 +167,11 @@ namespace Umbraco.Web.Routing var pcr = publishedRouter.CreateRequest(umbracoContext, uri); publishedRouter.TryRouteRequest(pcr); + urlInfo = null; + if (pcr.HasPublishedContent == false) { - urls.Add(UrlInfo.Message(textService.Localize("content/routeErrorCannotRoute"), culture)); + urlInfo = UrlInfo.Message(textService.Localize("content/routeErrorCannotRoute"), culture); return true; } @@ -151,7 +190,7 @@ namespace Umbraco.Web.Routing l.Reverse(); var s = "/" + string.Join("/", l) + " (id=" + pcr.PublishedContent.Id + ")"; - urls.Add(UrlInfo.Message(textService.Localize("content/routeError", new[] { s }), culture)); + urlInfo = UrlInfo.Message(textService.Localize("content/routeError", new[] { s }), culture); return true; } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 699c985af3..6700c0b182 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -167,6 +167,7 @@ + From 36fd32ee9a8a9613c173b5658be67687140e9f56 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 19 Dec 2018 00:04:35 +1100 Subject: [PATCH 06/12] remove unused comparer --- src/Umbraco.Web/Routing/UrlInfoComparer.cs | 66 ---------------------- src/Umbraco.Web/Umbraco.Web.csproj | 1 - 2 files changed, 67 deletions(-) delete mode 100644 src/Umbraco.Web/Routing/UrlInfoComparer.cs diff --git a/src/Umbraco.Web/Routing/UrlInfoComparer.cs b/src/Umbraco.Web/Routing/UrlInfoComparer.cs deleted file mode 100644 index ff41dc6bc9..0000000000 --- a/src/Umbraco.Web/Routing/UrlInfoComparer.cs +++ /dev/null @@ -1,66 +0,0 @@ -using System; -using System.Collections.Generic; - -namespace Umbraco.Web.Routing -{ - /// - /// Compares - /// - public class UrlInfoComparer : IEqualityComparer - { - private readonly bool _variesByCulture; - - public UrlInfoComparer(bool variesByCulture) - { - _variesByCulture = variesByCulture; - } - - /// - /// Determines equality between - /// - /// - /// - /// - /// - /// If variesByCulture is true, then culture is compared, otherwise culture is not compared. - /// Both culture and url are compared without case sensitivity. - /// - public bool Equals(UrlInfo x, UrlInfo y) - { - if (ReferenceEquals(null, y)) return false; - if (ReferenceEquals(null, x)) return false; - if (ReferenceEquals(x, y)) return true; - - if (_variesByCulture) - { - return string.Equals(x.Culture, y.Culture, StringComparison.InvariantCultureIgnoreCase) - && x.IsUrl == y.IsUrl - && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); - } - - return x.IsUrl == y.IsUrl - && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); - } - - /// - /// Calculates a hash code - /// - /// - /// - /// - /// If variesByCulture is true then culture is used in the calculation, otherwise it's not - /// - public int GetHashCode(UrlInfo obj) - { - unchecked - { - var hashCode = (obj.Text != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Text) : 0); - hashCode = (hashCode * 397) ^ obj.IsUrl.GetHashCode(); - if (_variesByCulture) - hashCode = (hashCode * 397) ^ (obj.Culture != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Culture) : 0); - return hashCode; - } - - } - } -} \ No newline at end of file diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 6700c0b182..699c985af3 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -167,7 +167,6 @@ - From 72d42e42281a76bbdedb5f8901c7abf88045883a Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 19 Dec 2018 15:58:18 +1100 Subject: [PATCH 07/12] updates the UI to show culture when the item is not variant and shows the unpublished status if the culture has no variants --- .../content/umbcontentnodeinfo.directive.js | 15 ++++++++++++++- .../components/content/umb-content-node-info.html | 8 +++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js index 7b56bb2bd0..4feea82d47 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbcontentnodeinfo.directive.js @@ -53,7 +53,8 @@ "prompt_unsavedChanges", "prompt_doctypeChangeWarning", "general_history", - "auditTrails_historyIncludingVariants" + "auditTrails_historyIncludingVariants", + "content_itemNotPublished" ]; localizationService.localizeMany(keys) @@ -65,11 +66,23 @@ labels.notCreated = data[4]; labels.unsavedChanges = data[5]; labels.doctypeChangeWarning = data[6]; + labels.notPublished = data[9]; scope.historyLabel = scope.node.variants && scope.node.variants.length === 1 ? data[7] : data[8]; setNodePublishStatus(); + if (scope.currentUrls.length === 0) { + if (scope.node.id > 0) { + //it's created but not published + scope.currentUrls.push({ text: labels.notPublished, isUrl: false }); + } + else { + //it's new + scope.currentUrls.push({ text: labels.notCreated, isUrl: false }) + } + } + }); scope.auditTrailOptions = { diff --git a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html index b37e3c7ba4..b1757d927f 100644 --- a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html +++ b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-content-node-info.html @@ -8,13 +8,19 @@
  • + + {{url.culture}} {{url.text}} +
    - {{url.text}} + + {{url.culture}} + {{url.text}} +
From 6a8c7d1a3bf1323323bcae8e0caefbe2bd56cd82 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 19 Dec 2018 17:13:46 +1100 Subject: [PATCH 08/12] Writes some tests, fixes a bug, orders results, writes notes --- .../Routing/GetContentUrlsTests.cs | 136 ++++++++++++++++++ src/Umbraco.Tests/Umbraco.Tests.csproj | 1 + .../Routing/UrlProviderExtensions.cs | 7 +- 3 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 src/Umbraco.Tests/Routing/GetContentUrlsTests.cs diff --git a/src/Umbraco.Tests/Routing/GetContentUrlsTests.cs b/src/Umbraco.Tests/Routing/GetContentUrlsTests.cs new file mode 100644 index 0000000000..a2a627227e --- /dev/null +++ b/src/Umbraco.Tests/Routing/GetContentUrlsTests.cs @@ -0,0 +1,136 @@ +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using Moq; +using NUnit.Framework; +using Umbraco.Core.Configuration.UmbracoSettings; +using Umbraco.Core.Models; +using Umbraco.Core.Services; +using Umbraco.Tests.TestHelpers; +using Umbraco.Tests.TestHelpers.Entities; +using Umbraco.Web.Routing; + +namespace Umbraco.Tests.Routing +{ + [TestFixture] + public class GetContentUrlsTests : UrlRoutingTestBase + { + private IUmbracoSettingsSection _umbracoSettings; + + public override void SetUp() + { + base.SetUp(); + + //generate new mock settings and assign so we can configure in individual tests + _umbracoSettings = SettingsForTests.GenerateMockUmbracoSettings(); + SettingsForTests.ConfigureSettings(_umbracoSettings); + } + + private ILocalizedTextService GetTextService() + { + var textService = Mock.Of( + x => x.Localize("content/itemNotPublished", + It.IsAny(), + It.IsAny>()) == "content/itemNotPublished"); + return textService; + } + + private ILocalizationService GetLangService(params string[] isoCodes) + { + var allLangs = isoCodes + .Select(CultureInfo.GetCultureInfo) + .Select(culture => new Language(culture.Name) + { + CultureName = culture.DisplayName, + IsDefault = true, + IsMandatory = true + }).ToArray(); + + var langService = Mock.Of(x => x.GetAllLanguages() == allLangs); + return langService; + } + + [Test] + public void Content_Not_Published() + { + var contentType = MockedContentTypes.CreateBasicContentType(); + var content = MockedContent.CreateBasicContent(contentType); + content.Id = 1046; //fixme: we are using this ID only because it's built into the test XML published cache + content.Path = "-1,1046"; + + var umbContext = GetUmbracoContext("http://localhost:8000"); + var publishedRouter = CreatePublishedRouter(Container, + contentFinders: new ContentFinderCollection(new[] { new ContentFinderByUrl(Logger) })); + var urls = content.GetContentUrls(publishedRouter, + umbContext, + GetLangService("en-US", "fr-FR"), GetTextService(), ServiceContext.ContentService, + Logger).ToList(); + + Assert.AreEqual(1, urls.Count); + Assert.AreEqual("content/itemNotPublished", urls[0].Text); + Assert.IsFalse(urls[0].IsUrl); + } + + [Test] + public void Invariant_Root_Content_Published_No_Domains() + { + var contentType = MockedContentTypes.CreateBasicContentType(); + var content = MockedContent.CreateBasicContent(contentType); + content.Id = 1046; //fixme: we are using this ID only because it's built into the test XML published cache + content.Path = "-1,1046"; + content.Published = true; + + var umbContext = GetUmbracoContext("http://localhost:8000", + urlProviders: new []{ new DefaultUrlProvider(_umbracoSettings.RequestHandler, Logger, TestObjects.GetGlobalSettings(), new SiteDomainHelper()) }); + var publishedRouter = CreatePublishedRouter(Container, + contentFinders:new ContentFinderCollection(new[]{new ContentFinderByUrl(Logger) })); + var urls = content.GetContentUrls(publishedRouter, + umbContext, + GetLangService("en-US", "fr-FR"), GetTextService(), ServiceContext.ContentService, + Logger).ToList(); + + Assert.AreEqual(1, urls.Count); + Assert.AreEqual("/home/", urls[0].Text); + Assert.AreEqual("en-US", urls[0].Culture); + Assert.IsTrue(urls[0].IsUrl); + } + + [Test] + public void Invariant_Child_Content_Published_No_Domains() + { + var contentType = MockedContentTypes.CreateBasicContentType(); + var parent = MockedContent.CreateBasicContent(contentType); + parent.Id = 1046; //fixme: we are using this ID only because it's built into the test XML published cache + parent.Name = "home"; + parent.Path = "-1,1046"; + parent.Published = true; + var child = MockedContent.CreateBasicContent(contentType); + child.Name = "sub1"; + child.Id = 1173; //fixme: we are using this ID only because it's built into the test XML published cache + child.Path = "-1,1046,1173"; + child.Published = true; + + var umbContext = GetUmbracoContext("http://localhost:8000", + urlProviders: new[] { new DefaultUrlProvider(_umbracoSettings.RequestHandler, Logger, TestObjects.GetGlobalSettings(), new SiteDomainHelper()) }); + var publishedRouter = CreatePublishedRouter(Container, + contentFinders: new ContentFinderCollection(new[] { new ContentFinderByUrl(Logger) })); + var urls = child.GetContentUrls(publishedRouter, + umbContext, + GetLangService("en-US", "fr-FR"), GetTextService(), ServiceContext.ContentService, + Logger).ToList(); + + Assert.AreEqual(1, urls.Count); + Assert.AreEqual("/home/sub1/", urls[0].Text); + Assert.AreEqual("en-US", urls[0].Culture); + Assert.IsTrue(urls[0].IsUrl); + } + + //TODO: We need a lot of tests here, the above was just to get started with being able to unit test this method + // * variant URLs without domains assigned, what happens? + // * variant URLs with domains assigned, but also having more languages installed than there are domains/cultures assigned + // * variant URLs with an ancestor culture unpublished + // * invariant URLs with ancestors as variants + // * ... probably a lot more + + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index baeb667fcc..ed2324e02c 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -132,6 +132,7 @@ + diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index 71a0c294ab..de9314704b 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -35,7 +35,10 @@ namespace Umbraco.Web.Routing if (logger == null) throw new ArgumentNullException(nameof(logger)); if (content.Published == false) + { yield return UrlInfo.Message(textService.Localize("content/itemNotPublished")); + yield break; + } var urls = new HashSet(); @@ -65,13 +68,13 @@ namespace Umbraco.Web.Routing // * The entire branch is invariant // * If there are less domain/cultures assigned to the branch than the number of cultures/languages installed - foreach (var dUrl in urlGroup.DistinctBy(x => x.Text.ToUpperInvariant())) + foreach (var dUrl in urlGroup.DistinctBy(x => x.Text.ToUpperInvariant()).OrderBy(x => x.Text).ThenBy(x => x.Culture)) yield return dUrl; } // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. - foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) + foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id).OrderBy(x => x.Text).ThenBy(x => x.Culture)) if (urls.Add(otherUrl)) //avoid duplicates yield return otherUrl; } From 8219e0ccdd82f1fefff1dd7551cc8250a0347c3c Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 20 Dec 2018 08:25:58 +0100 Subject: [PATCH 09/12] Fix failing test --- src/Umbraco.Tests/Composing/TypeLoaderTests.cs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Tests/Composing/TypeLoaderTests.cs b/src/Umbraco.Tests/Composing/TypeLoaderTests.cs index 7b2f9766f9..75edcd6404 100644 --- a/src/Umbraco.Tests/Composing/TypeLoaderTests.cs +++ b/src/Umbraco.Tests/Composing/TypeLoaderTests.cs @@ -58,18 +58,17 @@ namespace Umbraco.Tests.Composing // cleanup var assDir = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory; - foreach (var d in Directory.GetDirectories(Path.Combine(assDir.FullName, "TypeLoader"))) - Directory.Delete(d, true); + var tlDir = Path.Combine(assDir.FullName, "TypeLoader"); + if (!Directory.Exists(tlDir)) + return; + Directory.Delete(tlDir, true); } private DirectoryInfo PrepareFolder() { var assDir = new FileInfo(Assembly.GetExecutingAssembly().Location).Directory; - var dir = Directory.CreateDirectory(Path.Combine(assDir.FullName, "TypeLoader", Guid.NewGuid().ToString("N"))); - foreach (var f in dir.GetFiles()) - { - f.Delete(); - } + var tlDir = Path.Combine(assDir.FullName, "TypeLoader"); + var dir = Directory.CreateDirectory(Path.Combine(tlDir, Guid.NewGuid().ToString("N"))); return dir; } From 56c82d120b6cf837d1e2576772f5654cf75056db Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 20 Dec 2018 13:27:57 +0100 Subject: [PATCH 10/12] Bugfix keepalive route issue --- src/Umbraco.Web/Editors/KeepAliveController.cs | 7 ++++++- src/Umbraco.Web/Scheduling/KeepAlive.cs | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/Editors/KeepAliveController.cs b/src/Umbraco.Web/Editors/KeepAliveController.cs index 78b1289e38..b067a5b67e 100644 --- a/src/Umbraco.Web/Editors/KeepAliveController.cs +++ b/src/Umbraco.Web/Editors/KeepAliveController.cs @@ -4,6 +4,11 @@ using Umbraco.Web.WebApi; namespace Umbraco.Web.Editors { + // fixme + // this is not authenticated, and therefore public, and therefore reveals we + // are running Umbraco - but, all requests should come from localhost really, + // so there should be a way to 404 when the request comes from the outside. + public class KeepAliveController : UmbracoApiController { [HttpGet] @@ -14,7 +19,7 @@ namespace Umbraco.Web.Editors Success = true, Message = "I'm alive!" }; - } + } } public class KeepAlivePingResult diff --git a/src/Umbraco.Web/Scheduling/KeepAlive.cs b/src/Umbraco.Web/Scheduling/KeepAlive.cs index bf8610e005..8068e387f8 100644 --- a/src/Umbraco.Web/Scheduling/KeepAlive.cs +++ b/src/Umbraco.Web/Scheduling/KeepAlive.cs @@ -59,7 +59,7 @@ namespace Umbraco.Web.Scheduling return true; // repeat } - var url = umbracoAppUrl + "api/keepalive/ping"; + var url = umbracoAppUrl.TrimEnd('/') + "/api/keepalive/ping"; var request = new HttpRequestMessage(HttpMethod.Get, url); var result = await _httpClient.SendAsync(request, token); From e959605b39b3e9d2eb67ea526df91dab4b4bd27f Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 20 Dec 2018 14:31:46 +0100 Subject: [PATCH 11/12] Cleanup and deal with FIXMEs --- src/Umbraco.Web/Routing/DomainHelper.cs | 30 ++++++------------- src/Umbraco.Web/Routing/UrlProvider.cs | 5 +--- .../Routing/UrlProviderExtensions.cs | 16 +++++----- 3 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/Umbraco.Web/Routing/DomainHelper.cs b/src/Umbraco.Web/Routing/DomainHelper.cs index 9b300009d0..edfcb33aa5 100644 --- a/src/Umbraco.Web/Routing/DomainHelper.cs +++ b/src/Umbraco.Web/Routing/DomainHelper.cs @@ -30,10 +30,10 @@ namespace Umbraco.Web.Routing /// The culture, or null. /// The domain and its uri, if any, that best matches the specified uri and culture, else null. /// - /// f at least a domain is set on the node then the method returns the domain that + /// If at least a domain is set on the node then the method returns the domain that /// best matches the specified uri and culture, else it returns null. - /// If culture is null, uses the default culture for the installation instead. - /// fixme not exactly - if culture is !null, we MUST have a value for THAT culture, else we use default as hint + /// If culture is null, uses the default culture for the installation instead. Otherwise, + /// will try with the specified culture, else return null. /// internal DomainAndUri DomainForNode(int nodeId, Uri current, string culture = null) { @@ -49,13 +49,9 @@ namespace Umbraco.Web.Routing return null; // else filter - var domainAndUri = SelectDomain(domains, current, culture, _domainCache.DefaultCulture, + // it could be that none apply (due to culture) + return SelectDomain(domains, current, culture, _domainCache.DefaultCulture, (cdomainAndUris, ccurrent, cculture, cdefaultCulture) => _siteDomainHelper.MapDomain(cdomainAndUris, ccurrent, cculture, cdefaultCulture)); - - if (domainAndUri == null) - throw new Exception("DomainForUri returned null."); - - return domainAndUri; } /// @@ -202,19 +198,12 @@ namespace Umbraco.Web.Routing private static IReadOnlyCollection SelectByCulture(IReadOnlyCollection domainsAndUris, string culture, string defaultCulture) { + // we try our best to match cultures, but may end with a bogus domain + if (culture != null) // try the supplied culture { var cultureDomains = domainsAndUris.Where(x => x.Culture.Name.InvariantEquals(culture)).ToList(); if (cultureDomains.Count > 0) return cultureDomains; - - - // if a culture is supplied, we *want* a url for that culture, else fail - //throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); - //fixme: Review - throwing here causes a problem because the UrlProviderExtensions.GetContentUrls iterates through - // ALL cultures even if those cultures are not assigned for use within a branch. Returning null - // here fixes that problem and the URLs resolve correctly, however i don't know if this is causing other - // residual problems. It would also suggest that below in GetByCulture we don't throw either but instead return null?? - return null; } if (defaultCulture != null) // try the defaultCulture culture @@ -230,13 +219,12 @@ namespace Umbraco.Web.Routing { DomainAndUri domainAndUri; + // we try our best to match cultures, but may end with a bogus domain + if (culture != null) // try the supplied culture { domainAndUri = domainsAndUris.FirstOrDefault(x => x.Culture.Name.InvariantEquals(culture)); if (domainAndUri != null) return domainAndUri; - - // if a culture is supplied, we *want* a url for that culture, else fail - throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); } if (defaultCulture != null) // try the defaultCulture culture diff --git a/src/Umbraco.Web/Routing/UrlProvider.cs b/src/Umbraco.Web/Routing/UrlProvider.cs index 491deee9e3..7ed530093c 100644 --- a/src/Umbraco.Web/Routing/UrlProvider.cs +++ b/src/Umbraco.Web/Routing/UrlProvider.cs @@ -245,10 +245,7 @@ namespace Umbraco.Web.Routing /// public IEnumerable GetOtherUrls(int id, Uri current) { - // providers can return null or an empty list or a non-empty list, be prepared - var urls = _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current)); - - return urls; + return _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current)); } #endregion diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index de9314704b..8db657ed30 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -5,7 +5,6 @@ using Umbraco.Core.Models; using Umbraco.Core.Services; using Umbraco.Core; using Umbraco.Core.Logging; -using LightInject; namespace Umbraco.Web.Routing { @@ -38,24 +37,25 @@ namespace Umbraco.Web.Routing { yield return UrlInfo.Message(textService.Localize("content/itemNotPublished")); yield break; - } - - var urls = new HashSet(); + } // build a list of urls, for the back-office // which will contain // - the 'main' urls, which is what .Url would return, for each culture // - the 'other' urls we know (based upon domains, etc) // - // need to work through each installed culture. - // fixme: Why not just work with each culture assigned to domains in the branch? - // on invariant nodes, each culture returns the same url segment - // but, we don't know if the branch to this content is invariant so we need to ask + // need to work through each installed culture: + // on invariant nodes, each culture returns the same url segment but, + // we don't know if the branch to this content is invariant, so we need to ask // for URLs for all cultures. + // and, not only for those assigned to domains in the branch, because we want + // to show what GetUrl() would return, for every culture. + var urls = new HashSet(); var cultures = localizationService.GetAllLanguages().Select(x => x.IsoCode).ToList(); //get all URLs for all cultures + //in a HashSet, so de-duplicates too foreach (var cultureUrl in GetContentUrlsByCulture(content, cultures, publishedRouter, umbracoContext, contentService, textService, logger)) { urls.Add(cultureUrl); From b0eb0d120505142093b63f6b07c3964202f0dbb8 Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 20 Dec 2018 15:36:12 +0100 Subject: [PATCH 12/12] Fix tests --- src/Umbraco.Tests/Web/Controllers/ContentControllerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Tests/Web/Controllers/ContentControllerTests.cs b/src/Umbraco.Tests/Web/Controllers/ContentControllerTests.cs index e28d11dc4e..9bd00f3d96 100644 --- a/src/Umbraco.Tests/Web/Controllers/ContentControllerTests.cs +++ b/src/Umbraco.Tests/Web/Controllers/ContentControllerTests.cs @@ -79,7 +79,7 @@ namespace Umbraco.Tests.Web.Controllers }); var textService = new Mock(); - textService.Setup(x => x.Localize(It.IsAny(), It.IsAny(), It.IsAny>())).Returns(""); + textService.Setup(x => x.Localize(It.IsAny(), It.IsAny(), It.IsAny>())).Returns("text"); Container.RegisterSingleton(f => Mock.Of()); Container.RegisterSingleton(f => userServiceMock.Object);