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

uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs

@@ -426,7 +426,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;
 
@@ -571,6 +572,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;
+                    }
+                }
             }
         }
 
@@ -1232,12 +1242,27 @@ private async Task<PropertyTypeResult> GetOrCreatePropertyAsync(TObject item,
     }
 
 
-    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");
+        });
+    }
+}