diff --git a/src/Umbraco.Core/ContentExtensions.cs b/src/Umbraco.Core/ContentExtensions.cs index 9fcc12bc10..44e9907210 100644 --- a/src/Umbraco.Core/ContentExtensions.cs +++ b/src/Umbraco.Core/ContentExtensions.cs @@ -56,15 +56,21 @@ namespace Umbraco.Core /// Gets the cultures that have been flagged for unpublishing. /// /// Gets cultures for which content.UnpublishCulture() has been invoked. - internal static IReadOnlyList GetCulturesUnpublishing(this IContent content) + internal static IReadOnlyList GetCulturesUnpublishing(this IContent content, IContent persisted) { + //TODO: The reason we need a ref to the original persisted IContent is to check if it is published + // however, for performance reasons we could pass in a ContentCultureInfosCollection which could be + // resolved from the database much more quickly than resolving an entire IContent object. + // That said, the GetById on the IContentService will return from cache so might not be something to worry about. + + if (!content.Published || !content.ContentType.VariesByCulture() || !content.IsPropertyDirty("PublishCultureInfos")) return Array.Empty(); var culturesChanging = content.CultureInfos.Where(x => x.Value.IsDirty()).Select(x => x.Key); return culturesChanging .Where(x => !content.IsCulturePublished(x) && // is not published anymore - content.WasCulturePublished(x)) // but was published before + persisted != null && persisted.IsCulturePublished(x)) // but was published before .ToList(); } diff --git a/src/Umbraco.Core/Models/Content.cs b/src/Umbraco.Core/Models/Content.cs index 84ccc42a77..63ab65b8fe 100644 --- a/src/Umbraco.Core/Models/Content.cs +++ b/src/Umbraco.Core/Models/Content.cs @@ -19,7 +19,6 @@ namespace Umbraco.Core.Models private bool _published; private PublishedState _publishedState; private ContentCultureInfosCollection _publishInfos; - private ContentCultureInfosCollection _publishInfosOrig; private HashSet _editedCultures; /// @@ -201,11 +200,6 @@ namespace Umbraco.Core.Models // a non-available culture could not become published anyways => _publishInfos != null && _publishInfos.ContainsKey(culture); - /// - public bool WasCulturePublished(string culture) - // just check _publishInfosOrig - a copy of _publishInfos - // a non-available culture could not become published anyways - => _publishInfosOrig != null && _publishInfosOrig.ContainsKey(culture); // adjust dates to sync between version, cultures etc // used by the repo when persisting @@ -216,9 +210,8 @@ namespace Umbraco.Core.Models if (_publishInfos == null || !_publishInfos.TryGetValue(culture, out var publishInfos)) continue; - if (_publishInfosOrig != null && _publishInfosOrig.TryGetValue(culture, out var publishInfosOrig) - && publishInfosOrig.Date == publishInfos.Date) - continue; + //fixme: Removing the logic here for the old WasCulturePublished and the _publishInfosOrig has broken + // the test Can_Rollback_Version_On_Multilingual, but we need to understand what it's doing since I don't _publishInfos.AddOrUpdate(culture, publishInfos.Name, date); @@ -449,12 +442,6 @@ namespace Umbraco.Core.Models // take care of the published state _publishedState = _published ? PublishedState.Published : PublishedState.Unpublished; - // Make a copy of the _publishInfos, this is purely so that we can detect - // if this entity's previous culture publish state (regardless of the rememberDirty flag) - _publishInfosOrig = _publishInfos == null - ? null - : new ContentCultureInfosCollection(_publishInfos); - if (_publishInfos == null) return; foreach (var infos in _publishInfos) diff --git a/src/Umbraco.Core/Models/IContent.cs b/src/Umbraco.Core/Models/IContent.cs index 1d85a735b5..f468df59ab 100644 --- a/src/Umbraco.Core/Models/IContent.cs +++ b/src/Umbraco.Core/Models/IContent.cs @@ -84,14 +84,6 @@ namespace Umbraco.Core.Models /// bool IsCulturePublished(string culture); - /// - /// Gets a value indicating whether a culture was published. - /// - /// - /// Mirrors whenever the content item is saved. - /// - bool WasCulturePublished(string culture); - /// /// Gets the date a culture was published. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs index fc5382499f..d49c92f1bf 100644 --- a/src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/IDocumentRepository.cs @@ -7,6 +7,8 @@ namespace Umbraco.Core.Persistence.Repositories { public interface IDocumentRepository : IContentRepository, IReadRepository { + + /// /// Clears the publishing schedule for all entries having an a date before (lower than, or equal to) a specified date. /// diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index 6af7031883..9b4175b63f 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -1127,7 +1127,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // TODO: shall we get published properties or not? //var publishedVersionId = dto.Published ? dto.PublishedVersionDto.Id : 0; - var publishedVersionId = dto.PublishedVersionDto != null ? dto.PublishedVersionDto.Id : 0; + var publishedVersionId = dto.PublishedVersionDto?.Id ?? 0; var temp = new TempContent(dto.NodeId, versionId, publishedVersionId, contentType); var ltemp = new List> { temp }; @@ -1424,6 +1424,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement } } + // ReSharper disable once ClassNeverInstantiated.Local private class CultureNodeName { public int Id { get; set; } diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index adc6c919f1..9c2721cbb4 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -921,6 +921,8 @@ namespace Umbraco.Core.Services.Implement /// public PublishResult Unpublish(IContent content, string culture = "*", int userId = 0) { + if (content == null) throw new ArgumentNullException(nameof(content)); + var evtMsgs = EventMessagesFactory.Get(); culture = culture.NullOrWhiteSpaceAsNull(); @@ -949,12 +951,16 @@ namespace Umbraco.Core.Services.Implement // all cultures = unpublish whole if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) { + //TODO: Stop casting https://github.com/umbraco/Umbraco-CMS/issues/4234 ((Content)content).PublishedState = PublishedState.Unpublishing; } else { - // if the culture we want to unpublish was already unpublished, nothing to do - if (!content.WasCulturePublished(culture)) + // If the culture we want to unpublish was already unpublished, nothing to do. + // To check for that we need to lookup the persisted content item + var persisted = content.HasIdentity ? GetById(content.Id) : null; + + if (persisted != null && !persisted.IsCulturePublished(culture)) return new PublishResult(PublishResultType.SuccessUnpublishAlready, evtMsgs, content); // unpublish the culture @@ -1025,7 +1031,10 @@ namespace Umbraco.Core.Services.Implement if (publishing) { - culturesUnpublishing = content.GetCulturesUnpublishing(); + //to continue, we need to have a reference to the original IContent item that is currently persisted + var persisted = content.HasIdentity ? GetById(content.Id) : null; + + culturesUnpublishing = content.GetCulturesUnpublishing(persisted); culturesPublishing = variesByCulture ? content.PublishCultureInfos.Where(x => x.Value.IsDirty()).Select(x => x.Key).ToList() : null; diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index 7dd6c7ba03..5bd37c0385 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -763,36 +763,29 @@ namespace Umbraco.Tests.Services content.PublishCulture(langFr.IsoCode); content.PublishCulture(langUk.IsoCode); Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode)); - Assert.IsFalse(content.WasCulturePublished(langFr.IsoCode)); //not persisted yet, will be false Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); - Assert.IsFalse(content.WasCulturePublished(langUk.IsoCode)); //not persisted yet, will be false var published = ServiceContext.ContentService.SaveAndPublish(content, new[]{ langFr.IsoCode , langUk.IsoCode }); + Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode)); + Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); + //re-get content = ServiceContext.ContentService.GetById(content.Id); Assert.IsTrue(published.Success); Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode)); - Assert.IsTrue(content.WasCulturePublished(langFr.IsoCode)); Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); - Assert.IsTrue(content.WasCulturePublished(langUk.IsoCode)); var unpublished = ServiceContext.ContentService.Unpublish(content, langFr.IsoCode); Assert.IsTrue(unpublished.Success); Assert.AreEqual(PublishResultType.SuccessUnpublishCulture, unpublished.Result); - Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode)); - //this is slightly confusing but this will be false because this method is used for checking the state of the current model, - //but the state on the model has changed with the above Unpublish call - Assert.IsFalse(content.WasCulturePublished(langFr.IsoCode)); + Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); //re-get content = ServiceContext.ContentService.GetById(content.Id); Assert.IsFalse(content.IsCulturePublished(langFr.IsoCode)); - //this is slightly confusing but this will be false because this method is used for checking the state of a current model, - //but we've re-fetched from the database - Assert.IsFalse(content.WasCulturePublished(langFr.IsoCode)); Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); - Assert.IsTrue(content.WasCulturePublished(langUk.IsoCode)); + }