From b80415b34e5b63b52ac151a9181beefced183ca7 Mon Sep 17 00:00:00 2001 From: Shannon Deminick Date: Sat, 9 Feb 2013 06:05:35 +0600 Subject: [PATCH] Fixes PublishingStrategy - It will still check if it's expired even if the event isn't cancelled. It now adheres to some rules about publishing the children if a parent's publishing has failed or was cancelled. Have written unit tests for these too. --- .../Publishing/BasePublishingStrategy.cs | 6 +- .../Publishing/PublishingStrategy.cs | 185 ++++++++++++++---- src/Umbraco.Core/Services/ContentService.cs | 69 +++---- .../Publishing/PublishingStrategyTests.cs | 43 ++++ .../WebServices/SaveFileController.cs | 10 +- src/umbraco.cms/businesslogic/web/Document.cs | 7 +- 6 files changed, 236 insertions(+), 84 deletions(-) diff --git a/src/Umbraco.Core/Publishing/BasePublishingStrategy.cs b/src/Umbraco.Core/Publishing/BasePublishingStrategy.cs index 9ce2032b57..0ea65415c5 100644 --- a/src/Umbraco.Core/Publishing/BasePublishingStrategy.cs +++ b/src/Umbraco.Core/Publishing/BasePublishingStrategy.cs @@ -10,7 +10,7 @@ namespace Umbraco.Core.Publishing { protected internal abstract Attempt PublishInternal(IContent content, int userId); - + /// /// Publishes a list of content items /// @@ -21,8 +21,10 @@ namespace Umbraco.Core.Publishing /// not visible on the front-end. If set to false, this will only publish content that is live on the front-end but has new versions /// that have yet to be published. /// + /// If true this will validate each content item before trying to publish it, if validation fails it will not be published. /// - protected internal abstract IEnumerable> PublishWithChildrenInternal(IEnumerable content, int userId, bool includeUnpublishedDocuments = true); + protected internal abstract IEnumerable> PublishWithChildrenInternal( + IEnumerable content, int userId, bool includeUnpublishedDocuments = true, bool validateContent = false); protected internal abstract IEnumerable> UnPublishInternal(IEnumerable content, int userId); diff --git a/src/Umbraco.Core/Publishing/PublishingStrategy.cs b/src/Umbraco.Core/Publishing/PublishingStrategy.cs index 21b0d5a866..85d1c56745 100644 --- a/src/Umbraco.Core/Publishing/PublishingStrategy.cs +++ b/src/Umbraco.Core/Publishing/PublishingStrategy.cs @@ -22,7 +22,12 @@ namespace Umbraco.Core.Publishing protected internal override Attempt PublishInternal(IContent content, int userId) { if (Publishing.IsRaisedEventCancelled(new PublishEventArgs(content), this)) + { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' will not be published, the event was cancelled.", content.Name, content.Id)); return new Attempt(false, new PublishStatus(content, PublishStatusType.FailedCancelledByEvent)); + } + //Check if the Content is Expired to verify that it can in fact be published if (content.Status == ContentStatus.Expired) @@ -81,26 +86,74 @@ namespace Umbraco.Core.Publishing /// not visible on the front-end. If set to false, this will only publish content that is live on the front-end but has new versions /// that have yet to be published. /// + /// If true this will validate each content item before trying to publish it, if validation fails it will not be published. /// + /// + /// This method becomes complex once we start to be able to cancel events or stop publishing a content item in any way because if a + /// content item is not published then it's children shouldn't be published either. This rule will apply for the following conditions: + /// * If a document fails to be published, do not proceed to publish it's children if: + /// ** The document does not have a publish version + /// ** The document does have a published version but the includeUnpublishedDocuments = false + /// + /// In order to do this, we will order the content by level and begin by publishing each item at that level, then proceed to the next + /// level and so on. If we detect that the above rule applies when the document publishing is cancelled we'll add it to the list of + /// parentsIdsCancelled so that it's children don't get published. + /// protected internal override IEnumerable> PublishWithChildrenInternal( - IEnumerable content, int userId, bool includeUnpublishedDocuments = true) + IEnumerable content, int userId, bool includeUnpublishedDocuments = true, bool validateContent = false) { var statuses = new List>(); - /* Only update content thats not already been published - we want to loop through - * all unpublished content to write skipped content (expired and awaiting release) to log. - */ - foreach (var item in content.Where(x => x.Published == false)) - { - //Check if this item has never been published - if (!includeUnpublishedDocuments && !item.HasPublishedVersion()) - { - //this item does not have a published version and the flag is set to not include them - continue; - } + //a list of all document ids that had their publishing cancelled during these iterations. + //this helps us apply the rule listed in the notes above by checking if a document's parent id + //matches one in this list. + var parentsIdsCancelled = new List(); - //Fire Publishing event - if (Publishing.IsRaisedEventCancelled(new PublishEventArgs(item), this)) + //group by levels and iterate over the sorted ascending level. + //TODO: This will cause all queries to execute, they will not be lazy but I'm not really sure being lazy actually made + // much difference because we iterate over them all anyways?? Morten? + // Because we're grouping I think this will execute all the queries anyways so need to fetch it all first. + var fetchedContent = content.ToArray(); + var levelGroups = fetchedContent.GroupBy(x => x.Level); + foreach (var level in levelGroups.OrderBy(x => x.Key)) + { + + /* Only update content thats not already been published - we want to loop through + * all unpublished content to write skipped content (expired and awaiting release) to log. + */ + foreach (var item in level.Where(x => x.Published == false)) + { + //Check if this item should be excluded because it's parent's publishing has failed/cancelled + if (parentsIdsCancelled.Contains(item.ParentId)) + { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' will not be published because it's parent's publishing action failed or was cancelled.", item.Name, item.Id)); + //if this cannot be published, ensure that it's children can definitely not either! + parentsIdsCancelled.Add(item.Id); + continue; + } + + //Check if this item has never been published + if (!includeUnpublishedDocuments && !item.HasPublishedVersion()) + { + //this item does not have a published version and the flag is set to not include them + parentsIdsCancelled.Add(item.Id); + continue; + } + + //Fire Publishing event + if (Publishing.IsRaisedEventCancelled(new PublishEventArgs(item), this)) + { + //the publishing has been cancelled. + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' will not be published, the event was cancelled.", item.Name, item.Id)); + statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedCancelledByEvent))); + + //Does this document apply to our rule to cancel it's children being published? + CheckCancellingOfChildPublishing(item, parentsIdsCancelled, includeUnpublishedDocuments); + + continue; + } //Check if the Content is Expired to verify that it can in fact be published if (item.Status == ContentStatus.Expired) @@ -109,41 +162,84 @@ namespace Umbraco.Core.Publishing string.Format("Content '{0}' with Id '{1}' has expired and could not be published.", item.Name, item.Id)); statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedHasExpired))); + + //Does this document apply to our rule to cancel it's children being published? + CheckCancellingOfChildPublishing(item, parentsIdsCancelled, includeUnpublishedDocuments); + continue; } - //Check if the Content is Awaiting Release to verify that it can in fact be published - if (item.Status == ContentStatus.AwaitingRelease) - { + //Check if the Content is Awaiting Release to verify that it can in fact be published + if (item.Status == ContentStatus.AwaitingRelease) + { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' is awaiting release and could not be published.", + item.Name, item.Id)); + statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedAwaitingRelease))); + + //Does this document apply to our rule to cancel it's children being published? + CheckCancellingOfChildPublishing(item, parentsIdsCancelled, includeUnpublishedDocuments); + + continue; + } + + //Check if the Content is Trashed to verify that it can in fact be published + if (item.Status == ContentStatus.Trashed) + { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' is trashed and could not be published.", + item.Name, item.Id)); + statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedIsTrashed))); + + //Does this document apply to our rule to cancel it's children being published? + CheckCancellingOfChildPublishing(item, parentsIdsCancelled, includeUnpublishedDocuments); + + continue; + } + + item.ChangePublishedState(PublishedState.Published); + LogHelper.Info( - string.Format("Content '{0}' with Id '{1}' is awaiting release and could not be published.", + string.Format("Content '{0}' with Id '{1}' has been published.", item.Name, item.Id)); - statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedAwaitingRelease))); - continue; + + statuses.Add(new Attempt(true, new PublishStatus(item))); } - - //Check if the Content is Trashed to verify that it can in fact be published - if (item.Status == ContentStatus.Trashed) - { - LogHelper.Info( - string.Format("Content '{0}' with Id '{1}' is trashed and could not be published.", - item.Name, item.Id)); - statuses.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedIsTrashed))); - continue; - } - - item.ChangePublishedState(PublishedState.Published); - - LogHelper.Info( - string.Format("Content '{0}' with Id '{1}' has been published.", - item.Name, item.Id)); - - statuses.Add(new Attempt(true, new PublishStatus(item))); + } return statuses; } + /// + /// Based on the information provider we'll check if we should cancel the publishing of this document's children + /// + /// + /// + /// + /// + /// See remarks on method: PublishWithChildrenInternal + /// + private void CheckCancellingOfChildPublishing(IContent content, List parentsIdsCancelled, bool includeUnpublishedDocuments) + { + //Does this document apply to our rule to cancel it's children being published? + //TODO: We're going back to the service layer here... not sure how to avoid this? And this will add extra overhead to + // any document that fails to publish... + var hasPublishedVersion = ApplicationContext.Current.Services.ContentService.HasPublishedVersion(content.Id); + + if (hasPublishedVersion && !includeUnpublishedDocuments) + { + //it has a published version but our flag tells us to not include un-published documents and therefore we should + // not be forcing decendant/child documents to be published if their parent fails. + parentsIdsCancelled.Add(content.Id); + } + else if (!hasPublishedVersion) + { + //it doesn't have a published version so we certainly cannot publish it's children. + parentsIdsCancelled.Add(content.Id); + } + } + /// /// Publishes a list of Content /// @@ -153,7 +249,7 @@ namespace Umbraco.Core.Publishing public override bool PublishWithChildren(IEnumerable content, int userId) { var result = PublishWithChildrenInternal(content, userId); - + //NOTE: This previously always returned true so I've left it that way. It returned true because (from Morten)... // ... if one item couldn't be published it wouldn't be correct to return false. // in retrospect it should have returned a list of with Ids and Publish Status @@ -172,7 +268,12 @@ namespace Umbraco.Core.Publishing public override bool UnPublish(IContent content, int userId) { if (UnPublishing.IsRaisedEventCancelled(new PublishEventArgs(content), this)) + { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' will not be un-published, the event was cancelled.", content.Name, content.Id)); return false; + } + //If Content has a release date set to before now, it should be removed so it doesn't interrupt an unpublish //Otherwise it would remain released == published @@ -185,7 +286,7 @@ namespace Umbraco.Core.Publishing "Content '{0}' with Id '{1}' had its release date removed, because it was unpublished.", content.Name, content.Id)); } - + content.ChangePublishedState(PublishedState.Unpublished); LogHelper.Info( @@ -210,6 +311,8 @@ namespace Umbraco.Core.Publishing //Fire UnPublishing event if (UnPublishing.IsRaisedEventCancelled(new PublishEventArgs(item), this)) { + LogHelper.Info( + string.Format("Content '{0}' with Id '{1}' will not be published, the event was cancelled.", item.Name, item.Id)); result.Add(new Attempt(false, new PublishStatus(item, PublishStatusType.FailedCancelledByEvent))); continue; } @@ -307,7 +410,7 @@ namespace Umbraco.Core.Publishing /// Occurs after publish /// public static event TypedEventHandler> Published; - + /// /// Occurs before unpublish /// diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index 21e0f59143..e75ee525a0 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -1039,13 +1039,9 @@ namespace Umbraco.Core.Services /// Optional Id of the User issueing the publishing /// If set to true, this will also publish descendants that are completely unpublished, normally this will only publish children that have previously been published /// True if publishing succeeded, otherwise False - internal bool PublishWithChildren(IContent content, bool omitCacheRefresh = true, int userId = 0, bool includeUnpublished = false) + internal IEnumerable> PublishWithChildren(IContent content, bool omitCacheRefresh = true, int userId = 0, bool includeUnpublished = false) { - var result = PublishWithChildrenDo(content, omitCacheRefresh, userId, includeUnpublished); - - //This used to just return false only when the parent content failed, otherwise would always return true so we'll - // do the same thing for the moment - return result.Single(x => x.Result.ContentItem.Id == content.Id).Success; + return PublishWithChildrenDo(content, omitCacheRefresh, userId, includeUnpublished); } /// @@ -1201,40 +1197,39 @@ namespace Umbraco.Core.Services list.AddRange(GetDescendants(content)); //Publish and then update the database with new status - var publishedOutcome = _publishingStrategy.PublishWithChildrenInternal(list, userId).ToArray(); - //if (published) - //{ - var uow = _uowProvider.GetUnitOfWork(); - using (var repository = _repositoryFactory.CreateContentRepository(uow)) + var publishedOutcome = _publishingStrategy.PublishWithChildrenInternal(list, userId, includeUnpublished).ToArray(); + + var uow = _uowProvider.GetUnitOfWork(); + using (var repository = _repositoryFactory.CreateContentRepository(uow)) + { + //Only loop through content where the Published property has been updated + //foreach (var item in list.Where(x => ((ICanBeDirty)x).IsPropertyDirty("Published"))) + foreach (var item in publishedOutcome.Where(x => x.Success)) { - //Only loop through content where the Published property has been updated - //foreach (var item in list.Where(x => ((ICanBeDirty)x).IsPropertyDirty("Published"))) - foreach (var item in publishedOutcome.Where(x => x.Success)) - { - item.Result.ContentItem.WriterId = userId; - repository.AddOrUpdate(item.Result.ContentItem); - updated.Add(item.Result.ContentItem); - } - - uow.Commit(); - - foreach (var c in updated) - { - var xml = c.ToXml(); - var poco = new ContentXmlDto { NodeId = c.Id, Xml = xml.ToString(SaveOptions.None) }; - var exists = uow.Database.FirstOrDefault("WHERE nodeId = @Id", new { Id = c.Id }) != - null; - var r = exists - ? uow.Database.Update(poco) - : Convert.ToInt32(uow.Database.Insert(poco)); - } + item.Result.ContentItem.WriterId = userId; + repository.AddOrUpdate(item.Result.ContentItem); + updated.Add(item.Result.ContentItem); } - //Save xml to db and call following method to fire event: - if (omitCacheRefresh == false) - _publishingStrategy.PublishingFinalized(updated, false); - Audit.Add(AuditTypes.Publish, "Publish with Children performed by user", userId, content.Id); - //} + uow.Commit(); + + foreach (var c in updated) + { + var xml = c.ToXml(); + var poco = new ContentXmlDto { NodeId = c.Id, Xml = xml.ToString(SaveOptions.None) }; + var exists = uow.Database.FirstOrDefault("WHERE nodeId = @Id", new { Id = c.Id }) != + null; + var r = exists + ? uow.Database.Update(poco) + : Convert.ToInt32(uow.Database.Insert(poco)); + } + } + //Save xml to db and call following method to fire event: + if (omitCacheRefresh == false) + _publishingStrategy.PublishingFinalized(updated, false); + + Audit.Add(AuditTypes.Publish, "Publish with Children performed by user", userId, content.Id); + return publishedOutcome; } diff --git a/src/Umbraco.Tests/Publishing/PublishingStrategyTests.cs b/src/Umbraco.Tests/Publishing/PublishingStrategyTests.cs index 83a11814e1..7454861256 100644 --- a/src/Umbraco.Tests/Publishing/PublishingStrategyTests.cs +++ b/src/Umbraco.Tests/Publishing/PublishingStrategyTests.cs @@ -3,6 +3,7 @@ using System.IO; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Configuration; +using Umbraco.Core.Events; using Umbraco.Core.IO; using Umbraco.Core.Models; using Umbraco.Core.ObjectResolution; @@ -47,6 +48,9 @@ namespace Umbraco.Tests.Publishing { base.TearDown(); + //ensure event handler is gone + PublishingStrategy.Publishing -= PublishingStrategyPublishing; + //TestHelper.ClearDatabase(); //reset the app context @@ -62,6 +66,45 @@ namespace Umbraco.Tests.Publishing private IContent _homePage; + /// + /// in these tests we have a heirarchy of + /// - home + /// -- text page 1 + /// -- text page 2 + /// --- text page 3 + /// + /// For this test, none of them are published, then we bulk publish them all, however we cancel the publishing for + /// "text page 2". This internally will ensure that text page 3 doesn't get published either because text page 2 has + /// never been published. + /// + [Test] + public void Publishes_Many_Ignores_Cancelled_Items_And_Children() + { + var strategy = new PublishingStrategy(); + + + PublishingStrategy.Publishing +=PublishingStrategyPublishing; + + //publish root and nodes at it's children level + var listToPublish = ServiceContext.ContentService.GetDescendants(_homePage.Id).Concat(new[] {_homePage}); + var result = strategy.PublishWithChildrenInternal(listToPublish, 0); + + Assert.AreEqual(listToPublish.Count() - 2, result.Count(x => x.Success)); + Assert.IsTrue(result.Where(x => x.Success).Select(x => x.Result.ContentItem.Id) + .ContainsAll(listToPublish.Where(x => x.Name != "Text Page 2" && x.Name != "Text Page 3").Select(x => x.Id))); + } + + static void PublishingStrategyPublishing(IPublishingStrategy sender, PublishEventArgs e) + { + foreach (var i in e.PublishedEntities) + { + if (i.Name == "Text Page 2") + { + e.Cancel = true; + } + } + } + [Test] public void Publishes_Many_Ignores_Unpublished_Items() { diff --git a/src/Umbraco.Web/WebServices/SaveFileController.cs b/src/Umbraco.Web/WebServices/SaveFileController.cs index 2371aa001c..8ecb0a9f11 100644 --- a/src/Umbraco.Web/WebServices/SaveFileController.cs +++ b/src/Umbraco.Web/WebServices/SaveFileController.cs @@ -6,6 +6,7 @@ using System.Text; using System.Web.Mvc; using Umbraco.Core.IO; using Umbraco.Core.Logging; +using Umbraco.Core.Services; using Umbraco.Web.Cache; using Umbraco.Web.Macros; using Umbraco.Web.Mvc; @@ -32,8 +33,13 @@ namespace Umbraco.Web.WebServices [HttpPost] public JsonResult PublishDocument(int documentId, bool publishChildren) { - //var doc = Services.ContentService.GetById(documentId); - //if (Services.ContentService.PublishWithChildren(doc, UmbracoUser.Id)) + var doc = Services.ContentService.GetById(documentId); + var result = ((ContentService)Services.ContentService).PublishWithChildren(doc, true, UmbracoUser.Id); + + if (Services.ContentService.PublishWithChildren(doc, UmbracoUser.Id)) + { + + } return null; } } diff --git a/src/umbraco.cms/businesslogic/web/Document.cs b/src/umbraco.cms/businesslogic/web/Document.cs index 690b3a0750..91d630bb9d 100644 --- a/src/umbraco.cms/businesslogic/web/Document.cs +++ b/src/umbraco.cms/businesslogic/web/Document.cs @@ -817,7 +817,10 @@ namespace umbraco.cms.businesslogic.web [Obsolete("Obsolete, Use Umbraco.Core.Services.ContentService.PublishWithChildren()", false)] public bool PublishWithChildrenWithResult(User u) { - return ((ContentService)ApplicationContext.Current.Services.ContentService).PublishWithChildren(Content, true, u.Id); + var result = ((ContentService)ApplicationContext.Current.Services.ContentService).PublishWithChildren(Content, true, u.Id); + //This used to just return false only when the parent content failed, otherwise would always return true so we'll + // do the same thing for the moment + return result.Single(x => x.Result.ContentItem.Id == Id).Success; } /// @@ -854,7 +857,7 @@ namespace umbraco.cms.businesslogic.web if (!e.Cancel) { - var published = ((ContentService)ApplicationContext.Current.Services.ContentService).PublishWithChildren(Content, true, u.Id); + var publishedResults = ((ContentService)ApplicationContext.Current.Services.ContentService).PublishWithChildren(Content, true, u.Id); FireAfterPublish(e); }