diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 0af3b9f7fbfa..d6cd5c13141a 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -16,7 +16,7 @@ namespace Umbraco.Cms.Core.Models; public abstract class ContentTypeBase : TreeEntityBase, IContentTypeBase { // Custom comparer for enumerable - private static readonly DelegateEqualityComparer> ContentTypeSortComparer = + private static readonly DelegateEqualityComparer> _contentTypeSortComparer = new( (sorts, enumerable) => sorts.UnsortedSequenceEqual(enumerable), sorts => sorts.GetHashCode()); @@ -228,7 +228,7 @@ public bool IsElement public IEnumerable? AllowedContentTypes { get => _allowedContentTypes; - set => SetPropertyValueAndDetectChanges(value, ref _allowedContentTypes, nameof(AllowedContentTypes), ContentTypeSortComparer); + set => SetPropertyValueAndDetectChanges(value, ref _allowedContentTypes, nameof(AllowedContentTypes), _contentTypeSortComparer); } /// @@ -289,10 +289,7 @@ public IEnumerable NoGroupPropertyTypes get => PropertyTypeCollection; set { - if (PropertyTypeCollection != null) - { - PropertyTypeCollection.ClearCollectionChangedEvents(); - } + PropertyTypeCollection?.ClearCollectionChangedEvents(); PropertyTypeCollection = new PropertyTypeCollection(SupportsPublishing, value); PropertyTypeCollection.CollectionChanged += PropertyTypesChanged; @@ -336,12 +333,14 @@ public bool AddPropertyType(IPropertyType propertyType) /// /// Alias of the PropertyType to move /// Alias of the PropertyGroup to move the PropertyType to - /// + /// + /// Returns True if the PropertyType was moved, otherwise False. + /// /// /// If is null then the property is moved back to /// "generic properties" ie does not have a tab anymore. /// - 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); @@ -352,7 +351,7 @@ public bool MovePropertyType(string propertyTypeAlias, string propertyGroupAlias // 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) @@ -370,9 +369,23 @@ public bool MovePropertyType(string propertyTypeAlias, string propertyGroupAlias propertyType.PropertyGroupId = newPropertyGroup == null ? null : new Lazy(() => 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; } diff --git a/src/Umbraco.Core/Models/IContentTypeBase.cs b/src/Umbraco.Core/Models/IContentTypeBase.cs index 3e93e9c7b25e..9df8a83b97f7 100644 --- a/src/Umbraco.Core/Models/IContentTypeBase.cs +++ b/src/Umbraco.Core/Models/IContentTypeBase.cs @@ -172,8 +172,14 @@ public interface IContentTypeBase : IUmbracoEntity, IRememberBeingDirty /// /// Alias of the PropertyType to move /// Alias of the PropertyGroup to move the PropertyType to - /// - bool MovePropertyType(string propertyTypeAlias, string propertyGroupAlias); + /// + /// Returns True if the PropertyType was moved, otherwise False. + /// + /// + /// If is null then the property is moved back to + /// "generic properties" ie does not have a tab anymore. + /// + bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlias); /// /// Gets an corresponding to this content type. diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs index ec35da90a66b..8a79f772ff0b 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs +++ b/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("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() { diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs index abc810f435ad..b07e138779e5 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -267,6 +267,169 @@ public void Can_Reset_Dirty_Properties_Cascades_Via_Parameterless_Overload() 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(); + + /// + /// 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. + /// + 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() { diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj b/tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj index 496515eaefd4..83f2ed42075c 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Tests.UnitTests.csproj @@ -26,6 +26,15 @@ $(WarningsNotAsErrors),SYSLIB0013,CS0618,CS1998,SA1117,CS0067,CA1822,CA1416,IDE0028,SA1401,SA1405,IDE0060,CS0114,CS0414,CS0252,CS0612,IDE1006 + + + $(NoWarn),CA1861 + +