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 <mail@bergmania.dk>

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,
+                // Unpublish the culture, this will change the document state to Publishing! ... which is expected because this will
-                // we are just unpublishing the whole document but leaving all of the culture's as-is. This is expected
+                // essentially be re-publishing the document with the requested culture removed
-                // because we don't want to actually unpublish every culture and then the document, we just want everything
+                // We are however unpublishing all cultures, so we will set this to unpublishing.
-                // to be non-routable so that when it's re-published all variants were as they were.
+                content.UnpublishCulture(culture);
                 content.PublishedState = PublishedState.Unpublishing;
                 PublishResult result = CommitDocumentChangesInternal(scope, content, evtMsgs, allLangs, userId, out _);
                 scope.Complete();

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;
+                if (dto.Published)
-                content.PublishName = publishedVersionDto.ContentVersionDto.Text;
+                {
-                content.PublisherId = publishedVersionDto.ContentVersionDto.UserId;
+                    content.PublishDate = publishedVersionDto.ContentVersionDto.VersionDate;
+                    content.PublishName = publishedVersionDto.ContentVersionDto.Text;
+                    content.PublisherId = publishedVersionDto.ContentVersionDto.UserId;
+                }
             }
 
             // templates = ignored, managed by the repository

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DocumentRepository.cs

@@ -399,8 +399,7 @@ public class DocumentRepository : ContentRepositoryBase<int, IContent, DocumentR
             }
         }
 
-        if (content.PublishedVersionId > 0 &&
+        if (content.PublishedState is PublishedState.Published && content.PublishedVersionId > 0 && contentVariations.TryGetValue(content.PublishedVersionId, out contentVariation))
-            contentVariations.TryGetValue(content.PublishedVersionId, out contentVariation))
         {
             foreach (ContentVariation v in contentVariation)
             {

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
+        // Everything should be unpublished
-        AssertPerCulture(content, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true),
+        AssertPerCulture(content, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, false), (langDe, false));
-            (langDe, false));
+        AssertPerCulture(content2, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, false), (langDe, false));
-        AssertPerCulture(content2, (x, c) => x.IsCulturePublished(c), (langFr, false), (langUk, true),
-            (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,
+        AssertPerCulture(content, (x, c) => x.GetPublishDate(c) == DateTime.MinValue, (langUk, false)); // FR, DE would throw
-            (langUk, false)); // FR, DE would throw
+        AssertPerCulture(content2, (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
 
         // Act
         ContentService.Publish(content, new[] { langUk.IsoCode });

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(1, content.PublishedCultures.Count());
-        Assert.AreEqual(2, 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);
 

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(0, content.PublishedCultures.Count());
-        Assert.AreEqual(2, 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]