Content Types: Fix MovePropertyType orphaning the property when moving it to no group (closes #23481)

src/Umbraco.Core/Models/ContentTypeBase.cs

@@ -16,7 +16,7 @@
 public abstract class ContentTypeBase : TreeEntityBase, IContentTypeBase
 {
     // Custom comparer for enumerable
-    private static readonly DelegateEqualityComparer<IEnumerable<ContentTypeSort>> ContentTypeSortComparer =
+    private static readonly DelegateEqualityComparer<IEnumerable<ContentTypeSort>> _contentTypeSortComparer =
         new(
             (sorts, enumerable) => sorts.UnsortedSequenceEqual(enumerable),
             sorts => sorts.GetHashCode());
@@ -228,7 +228,7 @@
     public IEnumerable<ContentTypeSort>? AllowedContentTypes
     {
         get => _allowedContentTypes;
-        set => SetPropertyValueAndDetectChanges(value, ref _allowedContentTypes, nameof(AllowedContentTypes), ContentTypeSortComparer);
+        set => SetPropertyValueAndDetectChanges(value, ref _allowedContentTypes, nameof(AllowedContentTypes), _contentTypeSortComparer);
     }
 
     /// <summary>
@@ -289,10 +289,7 @@
         get => PropertyTypeCollection;
         set
         {
-            if (PropertyTypeCollection != null)
-            {
-                PropertyTypeCollection.ClearCollectionChangedEvents();
-            }
+            PropertyTypeCollection?.ClearCollectionChangedEvents();
 
             PropertyTypeCollection = new PropertyTypeCollection(SupportsPublishing, value);
             PropertyTypeCollection.CollectionChanged += PropertyTypesChanged;
@@ -336,43 +333,59 @@
     /// </summary>
     /// <param name="propertyTypeAlias">Alias of the PropertyType to move</param>
     /// <param name="propertyGroupAlias">Alias of the PropertyGroup to move the PropertyType to</param>
-    /// <returns></returns>
+    /// <returns>
+    ///     Returns <c>True</c> if the PropertyType was moved, otherwise <c>False</c>.
+    /// </returns>
     /// <remarks>
     ///     If <paramref name="propertyGroupAlias" /> is null then the property is moved back to
     ///     "generic properties" ie does not have a tab anymore.
     /// </remarks>
-    public bool MovePropertyType(string propertyTypeAlias, string propertyGroupAlias)
+    public bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlias)
     {
         // get property, ensure it exists
         IPropertyType? propertyType = PropertyTypes.FirstOrDefault(x => x.Alias == propertyTypeAlias);
         if (propertyType == null)
         {
             return false;
         }
 
         // get new group, if required, and ensure it exists
         PropertyGroup? newPropertyGroup = null;
-        if (propertyGroupAlias != null)
+        if (propertyGroupAlias is not null)
         {
             var index = PropertyGroups.IndexOfKey(propertyGroupAlias);
             if (index == -1)
             {
                 return false;
             }
 
             newPropertyGroup = PropertyGroups[index];
         }
 
         // get old group
         PropertyGroup? oldPropertyGroup = PropertyGroups.FirstOrDefault(x => x.PropertyTypes?.Any(y => y.Alias == propertyTypeAlias) ?? false);
 
         // set new group
         propertyType.PropertyGroupId =
             newPropertyGroup == null ? null : new Lazy<int>(() => newPropertyGroup.Id, false);
 
-        // remove from old group, if any - add to new group, if any
+        // remove from the old group, if any, and add to the new group - or, when there's no new
+        // group, to the collection of properties that do not belong to a group
         oldPropertyGroup?.PropertyTypes?.RemoveItem(propertyTypeAlias);
-        newPropertyGroup?.PropertyTypes?.Add(propertyType);
+
+        if (newPropertyGroup is not null)
+        {
+            PropertyTypeCollection.RemoveItem(propertyTypeAlias);
+
+            // the group's collection is nullable, and a null-conditional add would silently drop
+            // the property instead of moving it, so ensure the collection exists first
+            newPropertyGroup.PropertyTypes ??= new PropertyTypeCollection(SupportsPublishing);
+            newPropertyGroup.PropertyTypes.Add(propertyType);
+        }
+        else if (PropertyTypeCollection.Contains(propertyTypeAlias) is false)
+        {
+            PropertyTypeCollection.Add(propertyType);
+        }
 
         return true;
     }

src/Umbraco.Core/Models/IContentTypeBase.cs

@@ -172,8 +172,14 @@ public interface IContentTypeBase : IUmbracoEntity, IRememberBeingDirty
     /// </summary>
     /// <param name="propertyTypeAlias">Alias of the PropertyType to move</param>
     /// <param name="propertyGroupAlias">Alias of the PropertyGroup to move the PropertyType to</param>
-    /// <returns></returns>
-    bool MovePropertyType(string propertyTypeAlias, string propertyGroupAlias);
+    /// <returns>
+    ///     Returns <c>True</c> if the PropertyType was moved, otherwise <c>False</c>.
+    /// </returns>
+    /// <remarks>
+    ///     If <paramref name="propertyGroupAlias" /> is null then the property is moved back to
+    ///     "generic properties" ie does not have a tab anymore.
+    /// </remarks>
+    bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlias);
 
     /// <summary>
     ///     Gets an <see cref="ISimpleContentType" /> corresponding to this content type.

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs

@@ -2087,6 +2087,112 @@ public void Can_Remove_PropertyGroup_Without_Removing_Property_Types()
         Assert.AreEqual(count, basePage.PropertyTypes.Count());
     }
 
+    [Test]
+    public void Can_Move_PropertyType_To_No_Group()
+    {
+        IContentType basePage = CreateContentTypeWithSingleGroupedProperty();
+
+        Assert.IsTrue(basePage.MovePropertyType("title", null));
+
+        Assert.Multiple(() =>
+        {
+            Assert.AreEqual(1, basePage.PropertyTypes.Count(), "the property type should not be orphaned in memory");
+            Assert.AreEqual("title", basePage.NoGroupPropertyTypes.SingleOrDefault()?.Alias);
+            Assert.IsEmpty(basePage.PropertyGroups["content"].PropertyTypes!);
+        });
+
+        ContentTypeService.Save(basePage);
+        basePage = ContentTypeService.Get(basePage.Id);
+
+        Assert.Multiple(() =>
+        {
+            Assert.AreEqual(1, basePage.PropertyTypes.Count(), "the property type should not be deleted on save");
+            Assert.AreEqual("title", basePage.NoGroupPropertyTypes.SingleOrDefault()?.Alias);
+            Assert.IsNull(basePage.NoGroupPropertyTypes.Single().PropertyGroupId);
+            Assert.IsEmpty(basePage.PropertyGroups["content"].PropertyTypes!);
+        });
+    }
+
+    [Test]
+    public void Can_Move_PropertyType_To_No_Group_Without_Losing_Content_Values()
+    {
+        IContentType basePage = CreateContentTypeWithSingleGroupedProperty();
+
+        IContent contentItem = ContentBuilder.CreateBasicContent(basePage);
+        contentItem.SetValue("title", "The title");
+        ContentService.Save(contentItem);
+
+        basePage.MovePropertyType("title", null);
+        ContentTypeService.Save(basePage);
+
+        contentItem = ContentService.GetById(contentItem.Id);
+
+        Assert.AreEqual("The title", contentItem.GetValue<string>("title"));
+    }
+
+    [Test]
+    public void Can_Move_PropertyType_From_No_Group_Into_Group()
+    {
+        IContentType basePage = CreateContentTypeWithSingleUngroupedProperty();
+        Assert.AreEqual("title", basePage.NoGroupPropertyTypes.SingleOrDefault()?.Alias, "the property type should start un-grouped");
+
+        Assert.IsTrue(basePage.MovePropertyType("title", "content"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.IsEmpty(basePage.NoGroupPropertyTypes, "the property type should no longer be un-grouped in memory");
+            Assert.AreEqual("title", basePage.PropertyGroups["content"].PropertyTypes!.SingleOrDefault()?.Alias);
+        });
+
+        ContentTypeService.Save(basePage);
+        basePage = ContentTypeService.Get(basePage.Id);
+
+        Assert.Multiple(() =>
+        {
+            Assert.AreEqual(1, basePage.PropertyTypes.Count());
+            Assert.IsEmpty(basePage.NoGroupPropertyTypes);
+            Assert.AreEqual("title", basePage.PropertyGroups["content"].PropertyTypes!.SingleOrDefault()?.Alias);
+        });
+    }
+
+    private IContentType CreateContentTypeWithSingleGroupedProperty()
+    {
+        ContentType basePage = ContentTypeBuilder.CreateBasicContentType();
+        basePage.AddPropertyGroup("content", "Content");
+        Assert.IsTrue(basePage.AddPropertyType(CreateTitlePropertyType(), "content", "Content"));
+
+        ContentTypeService.Save(basePage);
+
+        return ContentTypeService.Get(basePage.Id);
+    }
+
+    private IContentType CreateContentTypeWithSingleUngroupedProperty()
+    {
+        ContentType basePage = ContentTypeBuilder.CreateBasicContentType();
+        basePage.AddPropertyGroup("content", "Content");
+
+        // the single argument overload adds the property type without a group
+        Assert.IsTrue(basePage.AddPropertyType(CreateTitlePropertyType()));
+
+        ContentTypeService.Save(basePage);
+
+        return ContentTypeService.Get(basePage.Id);
+    }
+
+    private PropertyType CreateTitlePropertyType() =>
+        new(
+            ShortStringHelper,
+            Constants.PropertyEditors.Aliases.TextBox,
+            ValueStorageType.Nvarchar,
+            "title")
+        {
+            Name = "Title",
+            Description = string.Empty,
+            Mandatory = false,
+            SortOrder = 1,
+            DataTypeId = Constants.DataTypes.Textbox,
+        };
+
     [Test]
     public void Can_Add_PropertyGroup_With_Same_Name_On_Parent_and_Child()
     {

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

@@ -267,6 +267,169 @@
         Assert.That(property.IsDirty(), Is.False);
     }
 
+    [Test]
+    public void Can_Move_PropertyType_To_No_Group()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+
+        Assert.IsTrue(contentType.MovePropertyType("title", null));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.PropertyTypes.Select(x => x.Alias), Is.EquivalentTo(new[] { "title" }));
+            Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(new[] { "title" }));
+            Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty);
+        });
+    }
+
+    [Test]
+    public void Can_Move_PropertyType_From_No_Group_Into_Group()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+        contentType.AddPropertyType(new PropertyTypeBuilder().WithAlias("noGroup").WithName("No Group").Build());
+
+        Assert.IsTrue(contentType.MovePropertyType("noGroup", "content"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.NoGroupPropertyTypes, Is.Empty);
+            Assert.That(
+                contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title", "noGroup" }));
+        });
+    }
+
+    [Test]
+    public void Can_Move_PropertyType_Between_Groups()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+        contentType.AddPropertyGroup("meta", "Meta");
+
+        Assert.IsTrue(contentType.MovePropertyType("title", "meta"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.NoGroupPropertyTypes, Is.Empty);
+            Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty);
+            Assert.That(
+                contentType.PropertyGroups["meta"].PropertyTypes!.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title" }));
+        });
+    }
+
+    [Test]
+    public void Can_Move_PropertyType_Into_Group_With_Uninitialised_PropertyTypes()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+        contentType.AddPropertyGroup("meta", "Meta");
+        contentType.PropertyGroups["meta"].PropertyTypes = null;
+
+        Assert.IsTrue(contentType.MovePropertyType("title", "meta"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(
+                contentType.PropertyTypes.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title" }),
+                "the property type should not be orphaned");
+            Assert.That(
+                contentType.PropertyGroups["meta"].PropertyTypes?.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title" }));
+            Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty);
+        });
+    }
+
+    [Test]
+    public void Can_Move_Already_Ungrouped_PropertyType_To_No_Group()
+    {
+        var contentType = BuildContentTypeWithUngroupedProperty();
+        Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(new[] { "title" }));
+
+        // Moving a property that is already un-grouped is a no-op, and must not duplicate it.
+        Assert.IsTrue(contentType.MovePropertyType("title", null));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.PropertyTypes.Select(x => x.Alias), Is.EquivalentTo(new[] { "title" }));
+            Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(new[] { "title" }));
+            Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty);
+        });
+    }
+
+    [Test]
+    public void Cannot_Move_PropertyType_To_Unknown_Group()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+
+        Assert.IsFalse(contentType.MovePropertyType("title", "noSuchGroup"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.NoGroupPropertyTypes, Is.Empty);
+            Assert.That(
+                contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title" }));
+        });
+    }
+
+    [Test]
+    public void Cannot_Move_Unknown_PropertyType()
+    {
+        var contentType = BuildContentTypeWithSingleGroup();
+
+        Assert.Multiple(() =>
+        {
+            Assert.IsFalse(contentType.MovePropertyType("noSuchProperty", null));
+            Assert.IsFalse(contentType.MovePropertyType("noSuchProperty", "content"));
+        });
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(contentType.NoGroupPropertyTypes, Is.Empty);
+            Assert.That(
+                contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias),
+                Is.EquivalentTo(new[] { "title" }));
+        });
+    }
+
+    private static ContentType BuildContentTypeWithSingleGroup() =>
+        (ContentType)new ContentTypeBuilder()
+            .WithAlias("textPage")
+            .WithName("Text Page")
+            .WithPropertyTypeIdsIncrementingFrom(200)
+            .AddPropertyGroup()
+                .WithAlias("content")
+                .WithName("Content")
+                .WithSortOrder(1)
+                .AddPropertyType()
+                    .WithAlias("title")
+                    .WithName("Title")
+                    .WithSortOrder(1)
+                    .Done()
+                .Done()
+            .Build();
+
+    /// <remarks>
+    ///     A property type added at the builder root belongs to no group, so this yields an empty
+    ///     property group alongside an un-grouped property type.
+    /// </remarks>
+    private static ContentType BuildContentTypeWithUngroupedProperty() =>
+        (ContentType)new ContentTypeBuilder()
+            .WithAlias("textPage")
+            .WithName("Text Page")
+            .WithPropertyTypeIdsIncrementingFrom(200)
+            .AddPropertyGroup()
+                .WithAlias("content")
+                .WithName("Content")
+                .WithSortOrder(1)
+                .Done()
+            .AddPropertyType()
+                .WithAlias("title")
+                .WithName("Title")
+                .WithSortOrder(1)
+                .Done()
+            .Build();
+
     [Test]
     public void Can_Deep_Clone_Media_Type()
     {

tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj

@@ -26,6 +26,15 @@
     <WarningsNotAsErrors>$(WarningsNotAsErrors),SYSLIB0013,CS0618,CS1998,SA1117,CS0067,CA1822,CA1416,IDE0028,SA1401,SA1405,IDE0060,CS0114,CS0414,CS0252,CS0612,IDE1006</WarningsNotAsErrors>
   </PropertyGroup>
 
+  <PropertyGroup>
+    <!--
+      Deliberately suppressed, not pending a fix:
+      [CA1861] inline arrays of expected values keep assertions readable, and hoisting them into
+               static fields to avoid an allocation is not a worthwhile trade in tests
+    -->
+    <NoWarn>$(NoWarn),CA1861</NoWarn>
+  </PropertyGroup>
+
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.TimeProvider.Testing" />
     <PackageReference Include="Microsoft.NET.Test.Sdk" />