Fixes: U4-2589 Save and Publish is not creating version of document and adds a few unit tests

src/Umbraco.Core/Models/ContentBase.cs

@@ -436,5 +436,20 @@ namespace Umbraco.Core.Models
         }
 
         public abstract void ChangeTrashedState(bool isTrashed, int parentId = -20);
+
+        /// <summary>
+        /// We will override this method to ensure that when we reset the dirty properties that we 
+        /// also reset the dirty changes made to the content's Properties (user defined)
+        /// </summary>
+        /// <param name="rememberPreviouslyChangedProperties"></param>
+        internal override void ResetDirtyProperties(bool rememberPreviouslyChangedProperties)
+        {
+            base.ResetDirtyProperties(rememberPreviouslyChangedProperties);
+
+            foreach (var prop in Properties)
+            {
+                prop.ResetDirtyProperties(rememberPreviouslyChangedProperties);
+            }
+        }
     }
 }

src/Umbraco.Core/Models/ContentExtensions.cs

@@ -12,6 +12,7 @@ using System.Xml.Linq;
 using Umbraco.Core.Configuration;
 using Umbraco.Core.IO;
 using Umbraco.Core.Media;
+using Umbraco.Core.Models.EntityBase;
 using Umbraco.Core.Models.Membership;
 using Umbraco.Core.Strings;
 using Umbraco.Core.Persistence;
@@ -23,6 +24,48 @@ namespace Umbraco.Core.Models
     public static class ContentExtensions
     {
         #region IContent
+
+        /// <summary>
+        /// Determines if a new version should be created
+        /// </summary>
+        /// <param name="entity"></param>
+        /// <returns></returns>
+        /// <remarks>
+        /// A new version needs to be created when:
+        /// * Any property value is changed (to enable a rollback)
+        /// * The publish status is changed
+        /// * The language is changed
+        /// </remarks>
+        internal static bool ShouldCreateNewVersion(this IContent entity)
+        {
+            var publishedState = ((Content)entity).PublishedState;
+            return ShouldCreateNewVersion(entity, publishedState);
+        }
+
+        /// <summary>
+        /// Determines if a new version should be created
+        /// </summary>
+        /// <param name="entity"></param>
+        /// <param name="publishedState"></param>
+        /// <returns></returns>
+        /// <remarks>
+        /// A new version needs to be created when:
+        /// * Any property value is changed (to enable a rollback)
+        /// * The publish status is changed
+        /// * The language is changed
+        /// </remarks>
+        internal static bool ShouldCreateNewVersion(this IContent entity, PublishedState publishedState)
+        {
+            var dirtyEntity = (ICanBeDirty)entity;
+            var contentChanged =
+                (dirtyEntity.IsPropertyDirty("Published") && publishedState != PublishedState.Unpublished)
+                || dirtyEntity.IsPropertyDirty("Language");
+
+            var propertyValueChanged = entity.Properties.Any(x => ((ICanBeDirty)x).IsDirty());
+
+            return contentChanged || propertyValueChanged;
+        }
+
         /// <summary>
         /// Returns a list of the current contents ancestors, not including the content itself.
         /// </summary>

src/Umbraco.Core/Persistence/Repositories/ContentRepository.cs

@@ -275,8 +275,9 @@ namespace Umbraco.Core.Persistence.Repositories
         protected override void PersistUpdatedItem(IContent entity)
         {
             var publishedState = ((Content) entity).PublishedState;
-            //A new version should only be created if published state (or language) has changed
-            bool shouldCreateNewVersion = (((ICanBeDirty)entity).IsPropertyDirty("Published") && publishedState != PublishedState.Unpublished) || ((ICanBeDirty)entity).IsPropertyDirty("Language");
+            
+            //check if we need to create a new version
+            bool shouldCreateNewVersion = entity.ShouldCreateNewVersion(publishedState);
             if (shouldCreateNewVersion)
             {
                 //Updates Modified date and Version Guid
@@ -338,22 +339,19 @@ namespace Umbraco.Core.Persistence.Repositories
                 }
             }
 
-            //Look up (newest) entries by id in cmsDocument table to set newest = false
-            //NOTE: This should only be done for all other versions then the current one, so we don't cause the same entry to be updated multiple times.
-            var documentDtos =
-                Database.Query<DocumentDto>(
-                    "WHERE nodeId = @Id AND newest = @IsNewest AND NOT(versionId = @VersionId)",
-                    new {Id = entity.Id, IsNewest = true, VersionId = dto.ContentVersionDto.VersionId});
-            foreach (var documentDto in documentDtos)
-            {
-                var docDto = documentDto;
-                docDto.Newest = false;
-                Database.Update(docDto);
-            }
-
             var contentVersionDto = dto.ContentVersionDto;
             if (shouldCreateNewVersion)
             {
+                //Look up (newest) entries by id in cmsDocument table to set newest = false
+                //NOTE: This is only relevant when a new version is created, which is why its done inside this if-statement.
+                var documentDtos = Database.Fetch<DocumentDto>("WHERE nodeId = @Id AND newest = @IsNewest", new { Id = entity.Id, IsNewest = true });
+                foreach (var documentDto in documentDtos)
+                {
+                    var docDto = documentDto;
+                    docDto.Newest = false;
+                    Database.Update(docDto);
+                }
+
                 //Create a new version - cmsContentVersion
                 //Assumes a new Version guid and Version date (modified date) has been set
                 Database.Insert(contentVersionDto);
@@ -484,7 +482,7 @@ namespace Umbraco.Core.Persistence.Repositories
         }
 
         #endregion
-
+        
         /// <summary>
         /// Private method to create a content object from a DocumentDto, which is used by Get and GetByVersion.
         /// </summary>

src/Umbraco.Tests/Models/ContentExtensionsTests.cs

@@ -0,0 +1,59 @@
+using System.Linq;
+using NUnit.Framework;
+using Umbraco.Core.Models;
+using Umbraco.Tests.TestHelpers.Entities;
+
+namespace Umbraco.Tests.Models
+{
+    [TestFixture]
+    public class ContentExtensionsTests
+    {
+        [Test]
+        public void Should_Create_New_Version_When_Publish_Status_Changed()
+        {
+            var contentType = MockedContentTypes.CreateTextpageContentType();
+            var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1);
+
+            content.ResetDirtyProperties(false);
+
+            content.Published = true;
+            Assert.IsTrue(content.ShouldCreateNewVersion(PublishedState.Published));
+        }
+
+        [Test]
+        public void Should_Create_New_Version_When_Language_Changed()
+        {
+            var contentType = MockedContentTypes.CreateTextpageContentType();
+            var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1);
+
+            content.ResetDirtyProperties(false);
+
+            content.Language = "en-AU";
+            Assert.IsTrue(content.ShouldCreateNewVersion(PublishedState.Unpublished));
+        }
+
+        [Test]
+        public void Should_Create_New_Version_When_Any_Property_Value_Changed()
+        {
+            var contentType = MockedContentTypes.CreateTextpageContentType();
+            var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1);
+
+            content.ResetDirtyProperties(false);
+
+            content.Properties.First().Value = "hello world";
+            Assert.IsTrue(content.ShouldCreateNewVersion(PublishedState.Unpublished));
+        }
+
+        [Test]
+        public void Should_Not_Create_New_Version_When_Anything_Other_Than_Published_Language_Or_Property_Vals_Changed()
+        {
+            var contentType = MockedContentTypes.CreateTextpageContentType();
+            var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1);
+
+            content.ResetDirtyProperties(false);
+
+            Assert.IsFalse(content.ShouldCreateNewVersion(PublishedState.Unpublished));
+        }
+
+    }
+}

src/Umbraco.Tests/Models/ContentTests.cs

@@ -13,6 +13,21 @@ namespace Umbraco.Tests.Models
     [TestFixture]
     public class ContentTests
     {
+        [Test]
+        public void All_Dirty_Properties_Get_Reset()
+        {
+            var contentType = MockedContentTypes.CreateTextpageContentType();
+            var content = MockedContent.CreateTextpageContent(contentType, "Textpage", -1);
+
+            content.ResetDirtyProperties(false);
+
+            Assert.IsFalse(content.IsDirty());
+            foreach (var prop in content.Properties)
+            {
+                Assert.IsFalse(prop.IsDirty());
+            }
+        }
+
         [Test]
         public void Can_Verify_Mocked_Content()
         {

src/Umbraco.Tests/Umbraco.Tests.csproj

@@ -185,6 +185,7 @@
     <Compile Include="Configurations\FileSystemProviderTests.cs" />
     <Compile Include="CoreXml\FrameworkXmlTests.cs" />
     <Compile Include="Integration\CreateContent.cs" />
+    <Compile Include="Models\ContentExtensionsTests.cs" />
     <Compile Include="Models\DataValueSetterTests.cs" />
     <Compile Include="Persistence\PetaPocoExtensionsTest.cs" />
     <Compile Include="Persistence\Repositories\UserRepositoryTest.cs" />