From e2e60bd30461bcb21d5b22313c8637e51f6fa7df Mon Sep 17 00:00:00 2001 From: Kevin Jump Date: Mon, 27 Jul 2026 10:38:26 +0100 Subject: [PATCH 1/2] Fix: move property out of group on import (#1009) Importing a content type where a property has been moved out of all groups (empty and no matching 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. Co-Authored-By: Claude Opus 4.8 --- .../Serializers/ContentTypeBaseSerializer.cs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs b/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs index 2f4edd84..75733e90 100644 --- a/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs +++ b/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs @@ -426,7 +426,8 @@ protected async Task> DeserializePropertiesAsync(TObjec /// so we store them and do them once we've put /// things in. List changes = []; - Dictionary propertiesToMove = []; + // value can be null - when the property is being moved out of all groups. + Dictionary propertiesToMove = []; List? compositeProperties = default; @@ -571,6 +572,15 @@ protected async Task> 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,13 @@ private async Task GetOrCreatePropertyAsync(TObject item, } - private static IEnumerable MoveProperties(IContentTypeBase item, IDictionary moves) + private static IEnumerable MoveProperties(IContentTypeBase item, IDictionary 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); + // a null target moves the property out of all groups (no group). + item.MovePropertyType(move.Key, move.Value!); + yield return uSyncChange.Update($"{move.Key}/Tab/{move.Value}", move.Key, "", move.Value ?? "(No group)"); } } From cd335cd8a031e87d7225dfe3c4eb855a90840670 Mon Sep 17 00:00:00 2001 From: Kevin Jump Date: Mon, 27 Jul 2026 11:03:36 +0100 Subject: [PATCH 2/2] 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 --- .../Serializers/ContentTypeBaseSerializer.cs | 18 ++++- .../Serializers/MovePropertyTypeTests.cs | 80 +++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 uSync.Tests/Serializers/MovePropertyTypeTests.cs diff --git a/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs b/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs index 75733e90..3ffd7f97 100644 --- a/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs +++ b/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs @@ -1246,8 +1246,22 @@ private static IEnumerable MoveProperties(IContentTypeBase item, ID { foreach (var move in moves) { - // a null target moves the property out of all groups (no group). - item.MovePropertyType(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)"); } } diff --git a/uSync.Tests/Serializers/MovePropertyTypeTests.cs b/uSync.Tests/Serializers/MovePropertyTypeTests.cs new file mode 100644 index 00000000..3c0483e4 --- /dev/null +++ b/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; + +/// +/// guards the behaviour the content type serializer relies on when a property +/// is moved out of all groups (#1009). +/// +/// +/// Umbraco's MovePropertyType(alias, null) removes the property from its +/// group but does NOT re-add it to the 'no group' collection - it orphans it +/// (see ). The serializer works +/// around this by re-adding the property with AddPropertyType. +/// +[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"); + }); + } +}