Fix: move property out of group on import (#1009) (#1010) (#1043)

* Fix: move property out of group on import (#1009)

Importing a content type where a property has been moved out of all
groups (empty <Tab> and no matching <Tabs> entry) did nothing - the
property stayed in its original group.

DeserializePropertiesAsync only handled properties moving *into* a tab.
When the tab alias resolved to empty for an existing property, the else
branch fell through without action.

Now, when an existing property has no tab in the config but is still in a
group, it is queued to move to "no group" via MovePropertyType(alias,
null), which Umbraco treats as removing it from its current group.



* Fix: re-home property into no-group so move persists in one import (#1009)

Moving a property out of all groups still needed two imports. Root cause:
Umbraco's MovePropertyType(alias, null) removes the property from its
group but does NOT add it to the 'no group' collection - it orphans the
property, so the first import's save doesn't persist the move.

MoveProperties now re-adds the property with AddPropertyType after the
move to null, which lands it in NoGroupPropertyTypes. Added unit tests
documenting the orphaning behaviour and verifying the workaround.



---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: KevinJump

uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs

@@ -422,7 +422,8 @@ protected async Task<IEnumerable<uSyncChange>> DeserializePropertiesAsync(TObjec
         /// so we store them and do them once we've put 
         /// things in. 
         List<uSyncChange> changes = [];
-        Dictionary<string, string> propertiesToMove = [];
+        // value can be null - when the property is being moved out of all groups.
+        Dictionary<string, string?> propertiesToMove = [];
 
         List<string>? compositeProperties = default;
 
@@ -567,6 +568,15 @@ protected async Task<IEnumerable<uSyncChange>> DeserializePropertiesAsync(TObjec
                         changes.AddWarning(alias, name, $"Unable to find tab {tabAlias} to add property too");
                     }
                 }
+                else
+                {
+                    // no tab in the config - the property has been moved out of
+                    // any group. if it is currently in one, move it out (issue #1009)
+                    if (item.PropertyGroups.Any(x => x.PropertyTypes?.Contains(result.Property.Alias) is true))
+                    {
+                        propertiesToMove[result.Property.Alias] = null;
+                    }
+                }
             }
         }
 
@@ -1216,12 +1226,27 @@ private async Task<PropertyTypeResult> GetOrCreatePropertyAsync(TObject item,
         return result;
     }
 
-    private static IEnumerable<uSyncChange> MoveProperties(IContentTypeBase item, IDictionary<string, string> moves)
+    private static IEnumerable<uSyncChange> MoveProperties(IContentTypeBase item, IDictionary<string, string?> moves)
     {
         foreach (var move in moves)
         {
-            item.MovePropertyType(move.Key, move.Value);
-            yield return uSyncChange.Update($"{move.Key}/Tab/{move.Value}", move.Key, "", move.Value);
+            if (move.Value is null)
+            {
+                // moving the property out of all groups. MovePropertyType(alias, null)
+                // removes it from its current group but does *not* re-add it to the
+                // 'no group' collection, leaving it orphaned - so the change does not
+                // stick until a second import. We re-home it explicitly. (issue #1009)
+                var property = item.PropertyTypes.FirstOrDefault(x => x.Alias.InvariantEquals(move.Key));
+                item.MovePropertyType(move.Key, null!);
+                if (property is not null && item.PropertyTypes.Any(x => x.Alias.InvariantEquals(move.Key)) is false)
+                    item.AddPropertyType(property);
+            }
+            else
+            {
+                item.MovePropertyType(move.Key, move.Value);
+            }
+
+            yield return uSyncChange.Update($"{move.Key}/Tab/{move.Value}", move.Key, "", move.Value ?? "(No group)");
         }
     }
 

uSync.Tests/Serializers/MovePropertyTypeTests.cs

@@ -0,0 +1,80 @@
+using System.Linq;
+
+using NUnit.Framework;
+
+using Umbraco.Cms.Core.Models;
+using Umbraco.Cms.Core.Strings;
+
+namespace uSync.Tests.Serializers;
+
+/// <summary>
+///  guards the behaviour the content type serializer relies on when a property
+///  is moved out of all groups (#1009).
+/// </summary>
+/// <remarks>
+///  Umbraco's <c>MovePropertyType(alias, null)</c> removes the property from its
+///  group but does NOT re-add it to the 'no group' collection - it orphans it
+///  (see <see cref="MoveToNull_OrphansTheProperty"/>). The serializer works
+///  around this by re-adding the property with <c>AddPropertyType</c>.
+/// </remarks>
+[TestFixture]
+public class MovePropertyTypeTests
+{
+    private static IShortStringHelper ShortStringHelper
+        => new DefaultShortStringHelper(new DefaultShortStringHelperConfig());
+
+    private static IContentType BuildContentTypeWithGroupedProperty()
+    {
+        var contentType = new ContentType(ShortStringHelper, -1)
+        {
+            Alias = "test",
+            Name = "Test"
+        };
+
+        var propertyType = new PropertyType(ShortStringHelper, "Umbraco.TextBox", ValueStorageType.Nvarchar)
+        {
+            Alias = "prop1",
+            Name = "Prop1"
+        };
+
+        contentType.AddPropertyGroup("content", "Content");
+        contentType.AddPropertyType(propertyType, "content", "Content");
+
+        return contentType;
+    }
+
+    [Test]
+    public void MoveToNull_OrphansTheProperty()
+    {
+        // documents the Umbraco behaviour the fix works around: moving a property
+        // to a null group removes it from the group but loses it entirely.
+        var item = BuildContentTypeWithGroupedProperty();
+
+        item.MovePropertyType("prop1", null);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(item.PropertyGroups["content"].PropertyTypes.Any(x => x.Alias == "prop1"), Is.False, "removed from group");
+            Assert.That(item.PropertyTypes.Any(x => x.Alias == "prop1"), Is.False, "but also orphaned from the content type");
+        });
+    }
+
+    [Test]
+    public void MoveToNull_ThenReAdd_LandsInNoGroup()
+    {
+        // the approach the serializer uses: move out, then re-home into no-group.
+        var item = BuildContentTypeWithGroupedProperty();
+
+        var property = item.PropertyTypes.FirstOrDefault(x => x.Alias == "prop1");
+        item.MovePropertyType("prop1", null);
+        if (property is not null && item.PropertyTypes.Any(x => x.Alias == "prop1") is false)
+            item.AddPropertyType(property);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(item.PropertyGroups["content"].PropertyTypes.Any(x => x.Alias == "prop1"), Is.False, "prop1 no longer in the group");
+            Assert.That(item.PropertyTypes.Any(x => x.Alias == "prop1"), Is.True, "prop1 still exists on the content type");
+            Assert.That(item.NoGroupPropertyTypes.Any(x => x.Alias == "prop1"), Is.True, "prop1 is in NoGroupPropertyTypes");
+        });
+    }
+}