diff --git a/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs b/uSync.Core/Serialization/Serializers/ContentTypeBaseSerializer.cs index 2f4edd84..3ffd7f97 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,27 @@ 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); + 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"); + }); + } +}