From 74db02c444be0a9ca4e9f7590b23ed16e59cda97 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Thu, 1 Feb 2024 08:33:14 +0100 Subject: [PATCH] V14: Don't add published culture infos if not published (#15370) * Don't add published infos if not published * Unpublish all cultures as a whole * Added tests --------- Co-authored-by: Bjarke Berg --- src/Umbraco.Core/Services/ContentService.cs | 8 +-- .../Factories/ContentBaseFactory.cs | 13 ++-- .../Implement/DocumentRepository.cs | 3 +- .../Services/ContentServiceTests.cs | 68 +++---------------- .../ContentPublishingServiceTests.Publish.cs | 7 +- ...ContentPublishingServiceTests.Unpublish.cs | 37 +++++++++- 6 files changed, 58 insertions(+), 78 deletions(-) diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index 3c351a658b..b0b8d164e8 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -1257,10 +1257,10 @@ public class ContentService : RepositoryService, IContentService // all cultures = unpublish whole if (culture == "*" || (!content.ContentType.VariesByCulture() && culture == null)) { - // It's important to understand that when the document varies by culture but the "*" is used, - // we are just unpublishing the whole document but leaving all of the culture's as-is. This is expected - // because we don't want to actually unpublish every culture and then the document, we just want everything - // to be non-routable so that when it's re-published all variants were as they were. + // Unpublish the culture, this will change the document state to Publishing! ... which is expected because this will + // essentially be re-publishing the document with the requested culture removed + // We are however unpublishing all cultures, so we will set this to unpublishing. + content.UnpublishCulture(culture); content.PublishedState = PublishedState.Unpublishing; PublishResult result = CommitDocumentChangesInternal(scope, content, evtMsgs, allLangs, userId, out _); scope.Complete(); diff --git a/src/Umbraco.Infrastructure/Persistence/Factories/ContentBaseFactory.cs b/src/Umbraco.Infrastructure/Persistence/Factories/ContentBaseFactory.cs index ad295505c4..fed80e753f 100644 --- a/src/Umbraco.Infrastructure/Persistence/Factories/ContentBaseFactory.cs +++ b/src/Umbraco.Infrastructure/Persistence/Factories/ContentBaseFactory.cs @@ -45,14 +45,17 @@ internal class ContentBaseFactory content.Published = dto.Published; content.Edited = dto.Edited; - // TODO: shall we get published infos or not? - // if (dto.Published) if (publishedVersionDto != null) { + // We need this to get the proper versionId to match to unpublished values. + // This is only needed if the content has been published before. content.PublishedVersionId = publishedVersionDto.Id; - content.PublishDate = publishedVersionDto.ContentVersionDto.VersionDate; - content.PublishName = publishedVersionDto.ContentVersionDto.Text; - content.PublisherId = publishedVersionDto.ContentVersionDto.UserId; + if (dto.Published) + { + content.PublishDate = publishedVersionDto.ContentVersionDto.VersionDate; + content.PublishName = publishedVersionDto.ContentVersionDto.Text; + content.PublisherId = publishedVersionDto.ContentVersionDto.UserId; + } } // templates = ignored, managed by the repository diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs index 3951ffba69..210f5def2f 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs @@ -399,8 +399,7 @@ public class DocumentRepository : ContentRepositoryBase 0 && - contentVariations.TryGetValue(content.PublishedVersionId, out contentVariation)) + if (content.PublishedState is PublishedState.Published && content.PublishedVersionId > 0 && contentVariations.TryGetValue(content.PublishedVersionId, out contentVariation)) { foreach (ContentVariation v in contentVariation) { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs index e1edbd6179..cb42dfd3b5 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs @@ -2877,7 +2877,7 @@ public class ContentServiceTests : UmbracoIntegrationTestWithContent Assert.IsFalse(content.Published); Assert.IsTrue(content.Edited); - Assert.AreEqual("foo", content.GetValue("title", published: true)); + Assert.IsNull(content.GetValue("title", published: true)); Assert.AreEqual("foo", content.GetValue("title")); var vpk = ((Content)content).VersionId; @@ -3410,9 +3410,9 @@ public class ContentServiceTests : UmbracoIntegrationTestWithContent Assert.AreEqual("name-fr2", content2.GetCultureName(langFr.IsoCode)); Assert.AreEqual("name-uk2", content2.GetCultureName(langUk.IsoCode)); - Assert.AreEqual("name-fr2", content2.PublishName); // not null, see note above + Assert.IsNull(content2.PublishName); Assert.IsNull(content2.GetPublishName(langFr.IsoCode)); - Assert.AreEqual("name-uk", content2.GetPublishName(langUk.IsoCode)); // not null, see note above + Assert.IsNull(content2.GetPublishName(langUk.IsoCode)); Assert.AreEqual("value-fr2", content2.GetValue("prop", langFr.IsoCode)); Assert.AreEqual("value-uk2", content2.GetValue("prop", langUk.IsoCode)); @@ -3426,68 +3426,16 @@ public class ContentServiceTests : UmbracoIntegrationTestWithContent AssertPerCulture(content2, (x, c) => x.IsCultureAvailable(c), (langFr, true), (langUk, true), (langDe, false)); - // fr is not published anymore - uk still is, see note above - AssertPerCulture(content, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true), - (langDe, false)); - AssertPerCulture(content2, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true), - (langDe, false)); + // Everything should be unpublished + AssertPerCulture(content, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, false), (langDe, false)); + AssertPerCulture(content2, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, false), (langDe, false)); // and so, fr has to be edited - uk still is AssertPerCulture(content, (x, c) => x.IsCultureEdited(c), (langFr, true), (langUk, true), (langDe, false)); AssertPerCulture(content2, (x, c) => x.IsCultureEdited(c), (langFr, true), (langUk, true), (langDe, false)); - AssertPerCulture(content, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, - (langUk, false)); // FR, DE would throw - AssertPerCulture(content2, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, - (langUk, false)); // FR, DE would throw - - // Act - - // that HAS to be SavePublishing, because SaveAndPublish would just republish everything! - // TODO: This is using an internal API - the test can't pass without this but we want to keep the test here - // will need stephane to have a look at this test at some stage since there is a lot of logic here that we - // want to keep on testing but don't need the public API to do these more complicated things. - ContentService.CommitDocumentChanges(content); - - // content has been re-published, - // everything is back to what it was before being unpublished - content2 = ContentService.GetById(content.Id); - - Assert.IsTrue(content2.Published); - - Assert.AreEqual("name-fr2", content2.Name); // got the default culture name when saved - Assert.AreEqual("name-fr2", content2.GetCultureName(langFr.IsoCode)); - Assert.AreEqual("name-uk2", content2.GetCultureName(langUk.IsoCode)); - - Assert.AreEqual("name-fr2", content2.PublishName); - Assert.IsNull(content2.GetPublishName(langFr.IsoCode)); - Assert.AreEqual("name-uk", content2.GetPublishName(langUk.IsoCode)); - - Assert.AreEqual("value-fr2", content2.GetValue("prop", langFr.IsoCode)); - Assert.AreEqual("value-uk2", content2.GetValue("prop", langUk.IsoCode)); - Assert.IsNull(content2.GetValue("prop", langFr.IsoCode, published: true)); - Assert.AreEqual("value-uk1", content2.GetValue("prop", langUk.IsoCode, published: true)); - - // no change - AssertPerCulture(content, (x, c) => x.IsCultureAvailable(c), (langFr, true), (langUk, true), - (langDe, false)); - AssertPerCulture(content2, (x, c) => x.IsCultureAvailable(c), (langFr, true), (langUk, true), - (langDe, false)); - - // no change, back to published - AssertPerCulture(content, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true), - (langDe, false)); - AssertPerCulture(content2, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true), - (langDe, false)); - - // no change, back to published - AssertPerCulture(content, (x, c) => x.IsCultureEdited(c), (langFr, true), (langUk, true), (langDe, false)); - AssertPerCulture(content2, (x, c) => x.IsCultureEdited(c), (langFr, true), (langUk, true), (langDe, false)); - - AssertPerCulture(content, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, - (langUk, false)); // FR, DE would throw - AssertPerCulture(content2, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, - (langUk, false)); // FR, DE would throw + AssertPerCulture(content, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, (langUk, false)); // FR, DE would throw + AssertPerCulture(content2, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, (langUk, false)); // FR, DE would throw // Act ContentService.Publish(content, new[] { langUk.IsoCode }); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Publish.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Publish.cs index 05ce78e831..ce85bd7131 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Publish.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Publish.cs @@ -151,7 +151,7 @@ public partial class ContentPublishingServiceTests Assert.AreEqual(ContentPublishingOperationStatus.Success, unpublishResult.Result); content = ContentService.GetById(content.Key)!; - Assert.AreEqual(2, content.PublishedCultures.Count()); + Assert.AreEqual(0, content.PublishedCultures.Count()); publishResult = await ContentPublishingService.PublishAsync(content.Key, new[] { langDa.IsoCode }, Constants.Security.SuperUserKey); @@ -159,8 +159,7 @@ public partial class ContentPublishingServiceTests Assert.AreEqual(ContentPublishingOperationStatus.Success, publishResult.Status); content = ContentService.GetById(content.Key)!; - // FIXME: when work item 32809 has been fixed, this should assert for 1 expected published cultures - Assert.AreEqual(2, content.PublishedCultures.Count()); + Assert.AreEqual(1, content.PublishedCultures.Count()); Assert.IsTrue(content.PublishedCultures.InvariantContains(langDa.IsoCode)); } @@ -303,7 +302,7 @@ public partial class ContentPublishingServiceTests } [Test] - public async Task Cannot_Publish_Variant_Content_With_Mandatory_Culture() + public async Task Can_Publish_Variant_Content_With_Mandatory_Culture() { var (langEn, langDa, contentType) = await SetupVariantTest(true); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Unpublish.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Unpublish.cs index f8e3f9a6ae..7b6b8a1c92 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Unpublish.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentPublishingServiceTests.Unpublish.cs @@ -139,8 +139,7 @@ public partial class ContentPublishingServiceTests VerifyIsNotPublished(content.Key); content = ContentService.GetById(content.Key)!; - // FIXME: when work item 32809 has been fixed, this should assert for 0 expected published cultures - Assert.AreEqual(2, content.PublishedCultures.Count()); + Assert.AreEqual(0, content.PublishedCultures.Count()); } [Test] @@ -159,14 +158,46 @@ public partial class ContentPublishingServiceTests await ContentPublishingService.PublishAsync(content.Key, new[] { langEn.IsoCode, langDa.IsoCode }, Constants.Security.SuperUserKey); VerifyIsPublished(content.Key); + content = ContentService.GetById(content.Key)!; + Assert.AreEqual(2, content.PublishedCultures.Count()); + var result = await ContentPublishingService.UnpublishAsync(content.Key, langEn.IsoCode, Constants.Security.SuperUserKey); Assert.IsTrue(result.Success); Assert.AreEqual(ContentPublishingOperationStatus.Success, result.Result); VerifyIsNotPublished(content.Key); content = ContentService.GetById(content.Key)!; - // FIXME: when work item 32809 has been fixed, this should assert for 0 expected published cultures + Assert.AreEqual(0, content.PublishedCultures.Count()); + } + + + + [Test] + public async Task Can_Unpublish_Non_Mandatory_Cultures() + { + var (langEn, langDa, contentType) = await SetupVariantTest(true); + + IContent content = new ContentBuilder() + .WithContentType(contentType) + .WithCultureName(langEn.IsoCode, "EN root") + .WithCultureName(langDa.IsoCode, "DA root") + .Build(); + content.SetValue("title", "EN title", culture: langEn.IsoCode); + content.SetValue("title", "DA title", culture: langDa.IsoCode); + ContentService.Save(content); + await ContentPublishingService.PublishAsync(content.Key, new[] { langEn.IsoCode, langDa.IsoCode }, Constants.Security.SuperUserKey); + VerifyIsPublished(content.Key); + + content = ContentService.GetById(content.Key)!; Assert.AreEqual(2, content.PublishedCultures.Count()); + + var result = await ContentPublishingService.UnpublishAsync(content.Key, langDa.IsoCode, Constants.Security.SuperUserKey); + Assert.IsTrue(result.Success); + Assert.AreEqual(ContentPublishingOperationStatus.Success, result.Result); + VerifyIsPublished(content.Key); + + content = ContentService.GetById(content.Key)!; + Assert.AreEqual(1, content.PublishedCultures.Count()); } [Test]