Merge pull request #12087 from umbraco/v9/bugfix/history-cleanup-make-contenttype-dirty

V9: Fix history cleanup not making content type dirty

src/Umbraco.Core/Models/ContentEditing/HistoryCleanup.cs

@@ -1,17 +1,34 @@
 using System.Runtime.Serialization;
+using Umbraco.Cms.Core.Models.Entities;
 
 namespace Umbraco.Cms.Core.Models.ContentEditing
 {
     [DataContract(Name = "historyCleanup", Namespace = "")]
-    public class HistoryCleanup
+    public class HistoryCleanup : BeingDirtyBase
     {
+        private bool _preventCleanup;
+        private int? _keepAllVersionsNewerThanDays;
+        private int? _keepLatestVersionPerDayForDays;
+
         [DataMember(Name = "preventCleanup")]
-        public bool PreventCleanup { get; set; }
+        public bool PreventCleanup
+        {
+            get => _preventCleanup;
+            set => SetPropertyValueAndDetectChanges(value, ref _preventCleanup, nameof(PreventCleanup));
+        }
 
         [DataMember(Name = "keepAllVersionsNewerThanDays")]
-        public int? KeepAllVersionsNewerThanDays { get; set; }
+        public int? KeepAllVersionsNewerThanDays
+        {
+            get => _keepAllVersionsNewerThanDays;
+            set => SetPropertyValueAndDetectChanges(value, ref _keepAllVersionsNewerThanDays, nameof(KeepAllVersionsNewerThanDays));
+        }
 
         [DataMember(Name = "keepLatestVersionPerDayForDays")]
-        public int? KeepLatestVersionPerDayForDays { get; set; }
+        public int? KeepLatestVersionPerDayForDays
+        {
+            get => _keepLatestVersionPerDayForDays;
+            set => SetPropertyValueAndDetectChanges(value, ref _keepLatestVersionPerDayForDays, nameof(KeepLatestVersionPerDayForDays));
+        }
     }
 }

src/Umbraco.Core/Models/ContentType.cs

@@ -96,7 +96,13 @@ namespace Umbraco.Cms.Core.Models
             }
         }
 
-        public HistoryCleanup HistoryCleanup { get; set; }
+        private HistoryCleanup _historyCleanup;
+
+        public HistoryCleanup HistoryCleanup
+        {
+            get => _historyCleanup;
+            set => SetPropertyValueAndDetectChanges(value, ref _historyCleanup, nameof(HistoryCleanup));
+        }
 
         /// <summary>
         ///     Determines if AllowedTemplates contains templateId
@@ -162,5 +168,8 @@ namespace Umbraco.Cms.Core.Models
         /// <inheritdoc />
         IContentType IContentType.DeepCloneWithResetIdentities(string newAlias) =>
             (IContentType)DeepCloneWithResetIdentities(newAlias);
+
+        /// <inheritdoc/>
+        public override bool IsDirty() => base.IsDirty() || HistoryCleanup.IsDirty();
     }
 }

src/Umbraco.Core/Models/Mapping/ContentTypeMapDefinition.cs

@@ -133,7 +133,7 @@ namespace Umbraco.Cms.Core.Models.Mapping
 
             if (target is IContentTypeWithHistoryCleanup targetWithHistoryCleanup)
             {
-                targetWithHistoryCleanup.HistoryCleanup = source.HistoryCleanup;
+                MapHistoryCleanup(source, targetWithHistoryCleanup);
             }
 
             target.AllowedTemplates = source.AllowedTemplates
@@ -147,6 +147,34 @@ namespace Umbraco.Cms.Core.Models.Mapping
                 : _fileService.GetTemplate(source.DefaultTemplate));
         }
 
+        private static void MapHistoryCleanup(DocumentTypeSave source, IContentTypeWithHistoryCleanup target)
+        {
+            // If source history cleanup is null we don't have to map all properties
+            if (source.HistoryCleanup is null)
+            {
+                target.HistoryCleanup = null;
+                return;
+            }
+
+            // We need to reset the dirty properties, because it is otherwise true, just because the json serializer has set properties
+            target.HistoryCleanup.ResetDirtyProperties(false);
+            if (target.HistoryCleanup.PreventCleanup != source.HistoryCleanup.PreventCleanup)
+            {
+                target.HistoryCleanup.PreventCleanup = source.HistoryCleanup.PreventCleanup;
+            }
+
+            if (target.HistoryCleanup.KeepAllVersionsNewerThanDays != source.HistoryCleanup.KeepAllVersionsNewerThanDays)
+            {
+                target.HistoryCleanup.KeepAllVersionsNewerThanDays = source.HistoryCleanup.KeepAllVersionsNewerThanDays;
+            }
+
+            if (target.HistoryCleanup.KeepLatestVersionPerDayForDays !=
+                source.HistoryCleanup.KeepLatestVersionPerDayForDays)
+            {
+                target.HistoryCleanup.KeepLatestVersionPerDayForDays = source.HistoryCleanup.KeepLatestVersionPerDayForDays;
+            }
+        }
+
         // no MapAll - take care
         private void Map(MediaTypeSave source, IMediaType target, MapperContext context)
         {
@@ -328,7 +356,10 @@ namespace Umbraco.Cms.Core.Models.Mapping
 
             if (source.GroupId > 0)
             {
-                target.PropertyGroupId = new Lazy<int>(() => source.GroupId, false);
+                if (target.PropertyGroupId?.Value != source.GroupId)
+                {
+                    target.PropertyGroupId = new Lazy<int>(() => source.GroupId, false);
+                }
             }
 
             target.Alias = source.Alias;
@@ -523,7 +554,15 @@ namespace Umbraco.Cms.Core.Models.Mapping
             target.Thumbnail = source.Thumbnail;
 
             target.AllowedAsRoot = source.AllowAsRoot;
-            target.AllowedContentTypes = source.AllowedContentTypes.Select((t, i) => new ContentTypeSort(t, i));
+
+            bool allowedContentTypesUnchanged = target.AllowedContentTypes.Select(x => x.Id.Value)
+                .SequenceEqual(source.AllowedContentTypes);
+
+            if (allowedContentTypesUnchanged is false)
+            {
+                target.AllowedContentTypes = source.AllowedContentTypes.Select((t, i) => new ContentTypeSort(t, i));
+            }
+
 
             if (!(target is IMemberType))
             {
@@ -574,13 +613,21 @@ namespace Umbraco.Cms.Core.Models.Mapping
 
                 // ensure no duplicate alias, then assign the group properties collection
                 EnsureUniqueAliases(destProperties);
-                destGroup.PropertyTypes = new PropertyTypeCollection(isPublishing, destProperties);
+                if (destGroup.PropertyTypes.SupportsPublishing != isPublishing || destGroup.PropertyTypes.SequenceEqual(destProperties) is false)
+                {
+                    destGroup.PropertyTypes = new PropertyTypeCollection(isPublishing, destProperties);
+                }
+
                 destGroups.Add(destGroup);
             }
 
             // ensure no duplicate name, then assign the groups collection
             EnsureUniqueAliases(destGroups);
-            target.PropertyGroups = new PropertyGroupCollection(destGroups);
+
+            if (target.PropertyGroups.SequenceEqual(destGroups) is false)
+            {
+                target.PropertyGroups = new PropertyGroupCollection(destGroups);
+            }
 
             // because the property groups collection was rebuilt, there is no need to remove
             // the old groups - they are just gone and will be cleared by the repository

tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeHistoryCleanupTests.cs

@@ -0,0 +1,105 @@
+using System.Linq;
+using NUnit.Framework;
+using Umbraco.Cms.Core.Models.ContentEditing;
+using Umbraco.Cms.Tests.Common.Builders;
+
+namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models
+{
+    [TestFixture]
+    public class ContentTypeHistoryCleanupTests
+    {
+        [Test]
+        public void Changing_Keep_all_Makes_ContentType_Dirty()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+
+            Assert.IsFalse(contentType.IsDirty());
+
+            var newValue = 2;
+            contentType.HistoryCleanup.KeepAllVersionsNewerThanDays = newValue;
+            Assert.IsTrue(contentType.IsDirty());
+            Assert.AreEqual(newValue, contentType.HistoryCleanup.KeepAllVersionsNewerThanDays);
+        }
+
+        [Test]
+        public void Changing_Keep_latest_Makes_ContentType_Dirty()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+
+            Assert.IsFalse(contentType.IsDirty());
+
+            var newValue = 2;
+            contentType.HistoryCleanup.KeepLatestVersionPerDayForDays = newValue;
+            Assert.IsTrue(contentType.IsDirty());
+            Assert.AreEqual(newValue, contentType.HistoryCleanup.KeepLatestVersionPerDayForDays);
+        }
+
+        [Test]
+        public void Changing_Prevent_Cleanup_Makes_ContentType_Dirty()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+
+            Assert.IsFalse(contentType.IsDirty());
+
+            var newValue = true;
+            contentType.HistoryCleanup.PreventCleanup = newValue;
+            Assert.IsTrue(contentType.IsDirty());
+            Assert.AreEqual(newValue, contentType.HistoryCleanup.PreventCleanup);
+        }
+
+        [Test]
+        public void Replacing_History_Cleanup_Registers_As_Dirty()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+            Assert.IsFalse(contentType.IsDirty());
+
+            contentType.HistoryCleanup = new HistoryCleanup();
+
+            Assert.IsTrue(contentType.IsDirty());
+            Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.HistoryCleanup)));
+        }
+
+        [Test]
+        public void Replacing_History_Cleanup_Removes_Old_Dirty_History_Properties()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+
+            contentType.Alias = "NewValue";
+            contentType.HistoryCleanup.KeepAllVersionsNewerThanDays = 2;
+
+            contentType.PropertyChanged += (sender, args) =>
+            {
+                // Ensure that property changed is only invoked for history cleanup
+                Assert.AreEqual(nameof(contentType.HistoryCleanup), args.PropertyName);
+            };
+
+            // Since we're replacing the entire HistoryCleanup the changed property is no longer dirty, the entire HistoryCleanup is
+            contentType.HistoryCleanup = new HistoryCleanup();
+
+            Assert.Multiple(() =>
+            {
+                Assert.IsTrue(contentType.IsDirty());
+                Assert.IsFalse(contentType.WasDirty());
+                Assert.AreEqual(2, contentType.GetDirtyProperties().Count());
+                Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.HistoryCleanup)));
+                Assert.IsTrue(contentType.IsPropertyDirty(nameof(contentType.Alias)));
+            });
+        }
+
+        [Test]
+        public void Old_History_Cleanup_Reference_Doesnt_Make_Content_Type_Dirty()
+        {
+            var contentType = ContentTypeBuilder.CreateBasicContentType();
+            var oldHistoryCleanup = contentType.HistoryCleanup;
+
+            contentType.HistoryCleanup = new HistoryCleanup();
+            contentType.ResetDirtyProperties();
+            contentType.ResetWereDirtyProperties();
+
+            oldHistoryCleanup.KeepAllVersionsNewerThanDays = 2;
+
+            Assert.IsFalse(contentType.IsDirty());
+            Assert.IsFalse(contentType.WasDirty());
+        }
+    }
+}