wip added more tests and notes

src/Umbraco.Core/Models/ContentRepositoryExtensions.cs

@@ -222,7 +222,13 @@ namespace Umbraco.Core.Models
             return true;
         }
 
-        public static void UnpublishCulture(this IContent content, string culture = "*")
+        /// <summary>
+        /// Returns false if the culture is already unpublished
+        /// </summary>
+        /// <param name="content"></param>
+        /// <param name="culture"></param>
+        /// <returns></returns>
+        public static bool UnpublishCulture(this IContent content, string culture = "*")
         {
             culture = culture.NullOrWhiteSpaceAsNull();
 
@@ -230,16 +236,31 @@ namespace Umbraco.Core.Models
             if (!content.ContentType.SupportsPropertyVariation(culture, "*", true))
                 throw new NotSupportedException($"Culture \"{culture}\" is not supported by content type \"{content.ContentType.Alias}\" with variation \"{content.ContentType.Variations}\".");
 
-            if (culture == "*") // all cultures
+
+            var keepProcessing = true;
+
+            if (culture == "*")
+            {
+                // all cultures
                 content.ClearPublishInfos();
-            else // one single culture
-                content.ClearPublishInfo(culture);
+            }
+            else
+            {
+                // one single culture
+                keepProcessing = content.ClearPublishInfo(culture);
+            }
+            
 
-            // property.PublishValues only publishes what is valid, variation-wise
-            foreach (var property in content.Properties)
-                property.UnpublishValues(culture);
+            if (keepProcessing)
+            {
+                // property.PublishValues only publishes what is valid, variation-wise
+                foreach (var property in content.Properties)
+                    property.UnpublishValues(culture);
 
-            content.PublishedState = PublishedState.Publishing;
+                content.PublishedState = PublishedState.Publishing;
+            }
+
+            return keepProcessing;
         }
 
         public static void ClearPublishInfos(this IContent content)
@@ -247,15 +268,24 @@ namespace Umbraco.Core.Models
             content.PublishCultureInfos = null;
         }
 
-        public static void ClearPublishInfo(this IContent content, string culture)
+        /// <summary>
+        /// Returns false if the culture is already unpublished
+        /// </summary>
+        /// <param name="content"></param>
+        /// <param name="culture"></param>
+        /// <returns></returns>
+        public static bool ClearPublishInfo(this IContent content, string culture)
         {
             if (culture.IsNullOrWhiteSpace())
                 throw new ArgumentNullOrEmptyException(nameof(culture));
 
-            content.PublishCultureInfos.Remove(culture);
-
-            // set the culture to be dirty - it's been modified
-            content.TouchCulture(culture);
+            var removed = content.PublishCultureInfos.Remove(culture);
+            if (removed)
+            {
+                // set the culture to be dirty - it's been modified
+                content.TouchCulture(culture);
+            }
+            return removed;
         }
 
         /// <summary>

src/Umbraco.Core/Services/Implement/ContentService.cs

@@ -1012,7 +1012,9 @@ namespace Umbraco.Core.Services.Implement
                     // essentially be re-publishing the document with the requested culture removed.
                     // The call to CommitDocumentChangesInternal will perform all the checks like if this is a mandatory culture or the last culture being unpublished
                     // and will then unpublish the document accordingly.
-                    content.UnpublishCulture(culture);
+                    var removed = content.UnpublishCulture(culture);
+
+                    //TODO: if !removed then there is really nothing to process and we should exit here with SuccessUnpublishAlready.
 
 
                     //TODO: Move this logic into CommitDocumentChangesInternal, we are already looking up the item there
@@ -2613,8 +2615,14 @@ namespace Umbraco.Core.Services.Implement
                 if (culturesPublishing == null)
                     throw new InvalidOperationException("Internal error, variesByCulture but culturesPublishing is null.");
 
-                if (content.Published && culturesPublishing.Count == 0 && culturesUnpublishing.Count == 0) // no published cultures = cannot be published
+                if (content.Published && culturesPublishing.Count == 0 && culturesUnpublishing.Count == 0)
+                {
+                    // no published cultures = cannot be published
+                    // This will occur if for example, a culture that is already unpublished is sent to be unpublished again, or vice versa, in that case
+                    // there will be nothing to publish/unpublish.
                     return new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content);
+                }
+                
 
                 // missing mandatory culture = cannot be published
                 var mandatoryCultures = allLangs.Where(x => x.IsMandatory).Select(x => x.IsoCode);

src/Umbraco.Tests/Services/ContentServiceTests.cs

@@ -852,6 +852,48 @@ namespace Umbraco.Tests.Services
             Assert.AreEqual(PublishedState.Unpublished, content.PublishedState); 
         }
 
+        [Test]
+        public void Unpublishing_Already_Unpublished_Culture()
+        {
+            // Arrange
+
+            var langUk = new Language("en-GB") { IsDefault = true };
+            var langFr = new Language("fr-FR");
+
+            ServiceContext.LocalizationService.Save(langFr);
+            ServiceContext.LocalizationService.Save(langUk);
+
+            var contentType = MockedContentTypes.CreateBasicContentType();
+            contentType.Variations = ContentVariation.Culture;
+            ServiceContext.ContentTypeService.Save(contentType);
+
+            IContent content = new Content("content", Constants.System.Root, contentType);
+            content.SetCultureName("content-fr", langFr.IsoCode);
+            content.SetCultureName("content-en", langUk.IsoCode);
+
+            var published = ServiceContext.ContentService.SaveAndPublish(content, new[] { langFr.IsoCode, langUk.IsoCode });
+            Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode));
+            Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode));
+            Assert.IsTrue(published.Success);
+            Assert.AreEqual(PublishedState.Published, content.PublishedState);
+
+            //re-get
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            var unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); 
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishCulture, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+
+            content = ServiceContext.ContentService.GetById(content.Id);
+
+            unpublished = ServiceContext.ContentService.Unpublish(content, langUk.IsoCode); //unpublish again
+            Assert.IsTrue(unpublished.Success);
+            Assert.AreEqual(PublishResultType.SuccessUnpublishAlready, unpublished.Result);
+            Assert.IsFalse(content.IsCulturePublished(langUk.IsoCode));
+
+        }
+
         [Test]
         public void Pending_Invariant_Property_Changes_Affect_Default_Language_Edited_State()
         {