Moving properties between groups sometimes clears their values (#19881)

* Fix moving properties between groups sometimes clearing their values

* Small adjustment

* Fix failing integration test

The mapping method was only setting the property group when it was not null, but for orphaned properties we want to specifically set it to null.

* Adjust 'Can_Move_Properties_To_Another_Container' integration test to check more scenarios and that values are kept

* Adjust to add isElement variable in test (as previously)

src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingServiceBase.cs

@@ -520,6 +520,10 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
                 //       the containers and their parent relationships in the model, so it's ok
                 container => model.Containers.First(c => c.Key == container.ParentKey).Name);
 
+        // Store the existing property types in a list to reference when processing properties.
+        // This ensures we correctly handle property types that may have been filtered out from groups.
+        var existingPropertyTypes = contentType.PropertyTypes.ToList();
+
         // handle properties in groups
         PropertyGroup[] propertyGroups = model.Containers.Select(container =>
             {
@@ -540,7 +544,7 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
                 IPropertyType[] properties = model
                     .Properties
                     .Where(property => property.ContainerKey == container.Key)
-                    .Select(property => MapProperty(contentType, property, propertyGroup, dataTypesByKey))
+                    .Select(property => MapProperty(contentType, property, propertyGroup, existingPropertyTypes, dataTypesByKey))
                     .ToArray();
 
                 if (properties.Any() is false && parentContainerNamesById.ContainsKey(container.Key) is false)
@@ -565,8 +569,8 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
         }
 
         // handle orphaned properties
-        IEnumerable<TPropertyTypeModel> orphanedPropertyTypeModels = model.Properties.Where (x => x.ContainerKey is null).ToArray();
+        IEnumerable<TPropertyTypeModel> orphanedPropertyTypeModels = model.Properties.Where(x => x.ContainerKey is null).ToArray();
-        IPropertyType[] orphanedPropertyTypes = orphanedPropertyTypeModels.Select(property => MapProperty(contentType, property, null, dataTypesByKey)).ToArray();
+        IPropertyType[] orphanedPropertyTypes = orphanedPropertyTypeModels.Select(property => MapProperty(contentType, property, null, existingPropertyTypes, dataTypesByKey)).ToArray();
         if (contentType.NoGroupPropertyTypes.SequenceEqual(orphanedPropertyTypes) is false)
         {
             contentType.NoGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing, orphanedPropertyTypes);
@@ -588,6 +592,7 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
         TContentType contentType,
         TPropertyTypeModel property,
         PropertyGroup? propertyGroup,
+        IEnumerable<IPropertyType> existingPropertyTypes,
         IDictionary<Guid, IDataType> dataTypesByKey)
     {
         // get the selected data type
@@ -598,7 +603,7 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
         }
 
         // get the current property type (if it exists)
-        IPropertyType propertyType = contentType.PropertyTypes.FirstOrDefault(pt => pt.Key == property.Key)
+        IPropertyType propertyType = existingPropertyTypes.FirstOrDefault(pt => pt.Key == property.Key)
                                      ?? new PropertyType(_shortStringHelper, dataType);
 
         // We are demanding a property type key in the model, so we should probably ensure that it's the on that's actually used.
@@ -617,10 +622,9 @@ internal abstract class ContentTypeEditingServiceBase<TContentType, TContentType
         propertyType.SortOrder = property.SortOrder;
         propertyType.LabelOnTop = property.Appearance.LabelOnTop;
 
-        if (propertyGroup is not null)
+        propertyType.PropertyGroupId = propertyGroup is null
-        {
+            ? null
-            propertyType.PropertyGroupId = new Lazy<int>(() => propertyGroup.Id, false);
+            : new Lazy<int>(() => propertyGroup.Id, false);
-        }
 
         return propertyType;
     }

tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTests.Update.cs

@@ -3,6 +3,7 @@ using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.ContentTypeEditing;
 using Umbraco.Cms.Core.Services.OperationStatus;
+using Umbraco.Cms.Tests.Common.Builders;
 
 namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services;
 
@@ -305,46 +306,118 @@ internal sealed partial class ContentTypeEditingServiceTests
         Assert.AreEqual(1, contentType.NoGroupPropertyTypes.Count());
     }
 
-    [TestCase(false)]
+    public enum PropertyMoveOperation
-    [TestCase(true)]
-    public async Task Can_Move_Properties_To_Another_Container(bool isElement)
     {
-        var createModel = ContentTypeCreateModel("Test", "test", isElement: isElement);
+        ToEarlier,
-        var container1 = ContentTypePropertyContainerModel("One");
+        ToLater,
-        createModel.Containers = new[] { container1 };
+    }
 
+    [TestCase(GroupContainerType, PropertyMoveOperation.ToEarlier, false)]
+    [TestCase(TabContainerType, PropertyMoveOperation.ToEarlier, false)]
+    [TestCase(GroupContainerType, PropertyMoveOperation.ToLater, false)]
+    [TestCase(TabContainerType, PropertyMoveOperation.ToLater, false)]
+    [TestCase(GroupContainerType, PropertyMoveOperation.ToEarlier, true)]
+    [TestCase(TabContainerType, PropertyMoveOperation.ToEarlier, true)]
+    [TestCase(GroupContainerType, PropertyMoveOperation.ToLater, true)]
+    [TestCase(TabContainerType, PropertyMoveOperation.ToLater, true)]
+    public async Task Can_Move_Properties_To_Another_Container(string containerType, PropertyMoveOperation propertyMoveOperation, bool isElement)
+    {
+        var container1 = ContentTypePropertyContainerModel($"{containerType} 1", containerType);
+        var container2 = ContentTypePropertyContainerModel($"{containerType} 2", containerType);
         var propertyType1 = ContentTypePropertyTypeModel("Test Property 1", "testProperty1", containerKey: container1.Key);
         var propertyType2 = ContentTypePropertyTypeModel("Test Property 2", "testProperty2", containerKey: container1.Key);
-        createModel.Properties = new[] { propertyType1, propertyType2 };
+        var propertyType3 = ContentTypePropertyTypeModel("Test Property 3", "testProperty3", containerKey: container2.Key);
+        var propertyType4 = ContentTypePropertyTypeModel("Test Property 4", "testProperty4", containerKey: container2.Key);
+        List<ContentTypePropertyTypeModel> properties = [propertyType1, propertyType2, propertyType3, propertyType4];
+        var createModel = ContentTypeCreateModel(
+            "Test Content Type",
+            containers: [container1, container2],
+            propertyTypes: properties,
+            isElement: isElement);
 
-        var contentType = (await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!;
+        var createAttempt = await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey);
+        Assert.Multiple(() =>
+        {
+            Assert.IsTrue(createAttempt.Success);
+            Assert.AreEqual(ContentTypeOperationStatus.Success, createAttempt.Status);
+            Assert.IsNotNull(createAttempt.Result);
 
-        var updateModel = ContentTypeUpdateModel("Test", "test", isElement: isElement);
+            Assert.AreEqual(4, createAttempt.Result.PropertyTypes.Count());
-        var container2 = ContentTypePropertyContainerModel("Two");
+            Assert.AreEqual(2, createAttempt.Result.PropertyGroups.Count);
-        container2.SortOrder = 0;
+            var createdContainer1 = createAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container1.Key);
-        container1.SortOrder = 1;
+            Assert.NotNull(createdContainer1);
-        updateModel.Containers = new[] { container1, container2 };
+            Assert.NotNull(createdContainer1.PropertyTypes);
-        propertyType2.ContainerKey = container2.Key;
+            Assert.AreEqual(2, createdContainer1.PropertyTypes.Count);
-        updateModel.Properties = new[] { propertyType1, propertyType2 };
+            Assert.AreEqual("Test Property 1", createdContainer1.PropertyTypes[0].Name);
+            Assert.AreEqual("Test Property 2", createdContainer1.PropertyTypes[1].Name);
+            var createdContainer2 = createAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container2.Key);
+            Assert.NotNull(createdContainer2);
+            Assert.NotNull(createdContainer2.PropertyTypes);
+            Assert.AreEqual(2, createdContainer2.PropertyTypes?.Count);
+            Assert.AreEqual("Test Property 3", createdContainer2.PropertyTypes[0].Name);
+            Assert.AreEqual("Test Property 4", createdContainer2.PropertyTypes[1].Name);
+        });
 
-        var result = await ContentTypeEditingService.UpdateAsync(contentType, updateModel, Constants.Security.SuperUserKey);
+        Content content = ContentBuilder.CreateBasicContent(createAttempt.Result!);
-        Assert.IsTrue(result.Success);
+        foreach (var property in properties)
-        Assert.IsNotNull(result.Result);
+        {
+            content.Properties[property.Alias]!.SetValue(property.Name);
+        }
 
-        // Ensure it's actually persisted
+        ContentService.Save(content);
-        contentType = await ContentTypeService.GetAsync(result.Result!.Key);
 
-        Assert.IsNotNull(contentType);
+        if (propertyMoveOperation == PropertyMoveOperation.ToEarlier)
-        Assert.AreEqual(isElement, contentType.IsElement);
+        {
-        Assert.AreEqual(2, contentType.PropertyGroups.Count);
+            propertyType3.ContainerKey = container1.Key;
-        Assert.AreEqual(2, contentType.PropertyTypes.Count());
+        }
-        Assert.AreEqual(1, contentType.PropertyGroups.First().PropertyTypes!.Count);
+        else
-        Assert.AreEqual(1, contentType.PropertyGroups.Last().PropertyTypes!.Count);
+        {
-        Assert.IsEmpty(contentType.NoGroupPropertyTypes);
+            propertyType1.ContainerKey = container2.Key;
+        }
 
-        var sortedPropertyGroups = contentType.PropertyGroups.OrderBy(g => g.SortOrder).ToArray();
+        var updateModel = ContentTypeUpdateModel(
-        Assert.AreEqual("testProperty2", sortedPropertyGroups.First().PropertyTypes!.Single().Alias);
+            "Test Content Type",
-        Assert.AreEqual("testProperty1", sortedPropertyGroups.Last().PropertyTypes!.Single().Alias);
+            containers: [container1, container2],
+            propertyTypes: [propertyType1, propertyType2, propertyType3, propertyType4]);
+        var updateAttempt = await ContentTypeEditingService.UpdateAsync(createAttempt.Result, updateModel, Constants.Security.SuperUserKey);
+        Assert.Multiple(() =>
+        {
+            Assert.IsTrue(updateAttempt.Success);
+            Assert.AreEqual(ContentTypeOperationStatus.Success, updateAttempt.Status);
+            Assert.AreEqual(4, updateAttempt.Result!.PropertyTypes.Count());
+            Assert.AreEqual(2, updateAttempt.Result.PropertyGroups.Count);
+
+            var updatedContainer1 = updateAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container1.Key);
+            Assert.NotNull(updatedContainer1?.PropertyTypes);
+            var updatedContainer2 = updateAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container2.Key);
+            Assert.NotNull(updatedContainer2?.PropertyTypes);
+            if (propertyMoveOperation == PropertyMoveOperation.ToEarlier)
+            {
+                Assert.AreEqual(3, updatedContainer1.PropertyTypes.Count);
+                Assert.AreEqual("Test Property 1", updatedContainer1.PropertyTypes[0].Name);
+                Assert.AreEqual("Test Property 2", updatedContainer1.PropertyTypes[1].Name);
+                Assert.AreEqual("Test Property 3", updatedContainer1.PropertyTypes[2].Name);
+                Assert.AreEqual(1, updatedContainer2.PropertyTypes.Count);
+                Assert.AreEqual("Test Property 4", updatedContainer2.PropertyTypes[0].Name);
+            }
+            else
+            {
+                Assert.AreEqual(1, updatedContainer1.PropertyTypes.Count);
+                Assert.AreEqual("Test Property 2", updatedContainer1.PropertyTypes[0].Name);
+                Assert.AreEqual(3, updatedContainer2.PropertyTypes.Count);
+                Assert.AreEqual("Test Property 1", updatedContainer2.PropertyTypes[0].Name);
+                Assert.AreEqual("Test Property 3", updatedContainer2.PropertyTypes[1].Name);
+                Assert.AreEqual("Test Property 4", updatedContainer2.PropertyTypes[2].Name);
+            }
+
+            Assert.AreEqual(0, updateAttempt.Result.NoGroupPropertyTypes.Count());
+
+            var updatedContent = ContentService.GetById(content.Id);
+            foreach (var property in properties)
+            {
+                Assert.AreEqual(property.Name, updatedContent?.Properties[property.Alias]?.GetValue());
+            }
+        });
     }
 
     [TestCase(false)]
@@ -393,18 +466,18 @@ internal sealed partial class ContentTypeEditingServiceTests
         var createModel = ContentTypeCreateModel("Test", "test", isElement: isElement);
         var container1 = ContentTypePropertyContainerModel("One");
         var container2 = ContentTypePropertyContainerModel("Two");
-        createModel.Containers = new[] { container1, container2 };
+        createModel.Containers = [container1, container2];
 
         var propertyType1 = ContentTypePropertyTypeModel("Test Property 1", "testProperty1", containerKey: container1.Key);
         var propertyType2 = ContentTypePropertyTypeModel("Test Property 2", "testProperty2", containerKey: container2.Key);
-        createModel.Properties = new[] { propertyType1, propertyType2 };
+        createModel.Properties = [propertyType1, propertyType2];
 
         var contentType = (await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!;
 
         var updateModel = ContentTypeUpdateModel("Test", "test", isElement: isElement);
-        updateModel.Containers = new[] { container1 };
+        updateModel.Containers = [container1];
         propertyType2.ContainerKey = null;
-        updateModel.Properties = new[] { propertyType1, propertyType2 };
+        updateModel.Properties = [propertyType1, propertyType2];
 
         var result = await ContentTypeEditingService.UpdateAsync(contentType, updateModel, Constants.Security.SuperUserKey);
         Assert.IsTrue(result.Success);

tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs

@@ -21,6 +21,8 @@ internal abstract class ContentTypeEditingServiceTestsBase : UmbracoIntegrationT
 
     protected IContentTypeService ContentTypeService => GetRequiredService<IContentTypeService>();
 
+    protected IContentService ContentService => GetRequiredService<IContentService>();
+
     protected IMediaTypeService MediaTypeService => GetRequiredService<IMediaTypeService>();
 
     protected const string TabContainerType = "Tab";