From a586dc1dae2d5178e69ac1719d027cf4a72b9068 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 28 Jul 2026 12:15:26 +0200 Subject: [PATCH 1/6] Fix move of property type from group to no group and vice versa. --- src/Umbraco.Core/Models/ContentTypeBase.cs | 25 ++++- src/Umbraco.Core/Models/IContentTypeBase.cs | 10 +- .../Services/ContentTypeServiceTests.cs | 95 +++++++++++++++++++ .../Umbraco.Core/Models/ContentTypeTests.cs | 65 +++++++++++++ 4 files changed, 188 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 0af3b9f7fbfa..06b822ed73f5 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -336,12 +336,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 +354,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 +372,22 @@ 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 null) + { + if (PropertyTypeCollection.Contains(propertyTypeAlias) == false) + { + PropertyTypeCollection.Add(propertyType); + } + } + else + { + PropertyTypeCollection.RemoveItem(propertyTypeAlias); + newPropertyGroup.PropertyTypes?.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..e02901624391 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs @@ -2087,6 +2087,101 @@ 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 = CreateContentTypeWithSingleGroupedProperty(); + + basePage.MovePropertyType("title", null); + ContentTypeService.Save(basePage); + basePage = ContentTypeService.Get(basePage.Id); + + 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() + { + var basePage = (IContentType)ContentTypeBuilder.CreateBasicContentType(); + basePage.AddPropertyGroup("content", "Content"); + + var titlePropertyType = new PropertyType( + ShortStringHelper, + Constants.PropertyEditors.Aliases.TextBox, + ValueStorageType.Nvarchar, + "title") + { + Name = "Title", + Description = string.Empty, + Mandatory = false, + SortOrder = 1, + DataTypeId = Constants.DataTypes.Textbox, + }; + Assert.IsTrue(basePage.AddPropertyType(titlePropertyType, "content", "Content")); + + ContentTypeService.Save(basePage); + + return ContentTypeService.Get(basePage.Id); + } + [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..8cdc816756e8 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -267,6 +267,71 @@ 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 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" })); + }); + } + + 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(); + [Test] public void Can_Deep_Clone_Media_Type() { From 484f2e674ba385c777edeb0a210becbb3f7bc061 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 28 Jul 2026 12:58:11 +0200 Subject: [PATCH 2/6] test: address SonarQube findings on the new tests Hoist the constant expected-alias arrays into static readonly fields (CA1861) and drop the redundant interface cast on the content type builder result (CA1859). Co-Authored-By: Claude Opus 5 (1M context) --- .../Services/ContentTypeServiceTests.cs | 2 +- .../Umbraco.Core/Models/ContentTypeTests.cs | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs index e02901624391..1a3ea0ab9557 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs @@ -2160,7 +2160,7 @@ public void Can_Move_PropertyType_From_No_Group_Into_Group() private IContentType CreateContentTypeWithSingleGroupedProperty() { - var basePage = (IContentType)ContentTypeBuilder.CreateBasicContentType(); + ContentType basePage = ContentTypeBuilder.CreateBasicContentType(); basePage.AddPropertyGroup("content", "Content"); var titlePropertyType = new PropertyType( diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs index 8cdc816756e8..643c6ef8b24a 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -14,6 +14,9 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models; [TestFixture] public class ContentTypeTests { + private static readonly string[] _titleAlias = ["title"]; + private static readonly string[] _titleAndNoGroupAliases = ["title", "noGroup"]; + [Test] [Ignore("Ignoring this test until we actually enforce this, see comments in ContentTypeBase.PropertyTypesChanged")] public void Cannot_Add_Duplicate_Property_Aliases() @@ -276,8 +279,8 @@ public void Can_Move_PropertyType_To_No_Group() 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.PropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); + Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty); }); } @@ -295,7 +298,7 @@ public void Can_Move_PropertyType_From_No_Group_Into_Group() Assert.That(contentType.NoGroupPropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(new[] { "title", "noGroup" })); + Is.EquivalentTo(_titleAndNoGroupAliases)); }); } @@ -311,7 +314,7 @@ public void Cannot_Move_PropertyType_To_Unknown_Group() Assert.That(contentType.NoGroupPropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(new[] { "title" })); + Is.EquivalentTo(_titleAlias)); }); } From bd750e165332da2b715e91d1102bf0b29f2587b8 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 28 Jul 2026 14:26:51 +0200 Subject: [PATCH 3/6] refactor: flatten the nested branch in MovePropertyType Express the un-grouped case as an else-if rather than a nested conditional, so the assignment reads as a single two-way choice. Behaviour is unchanged. Also adds unit tests for the previously uncovered paths - unknown property type, a move between two groups, and a no-op move of an already un-grouped property - so the restructure is covered before and after. Co-Authored-By: Claude Opus 5 (1M context) --- src/Umbraco.Core/Models/ContentTypeBase.cs | 22 +++----- .../Umbraco.Core/Models/ContentTypeTests.cs | 55 +++++++++++++++++++ 2 files changed, 63 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 06b822ed73f5..202ef96e6188 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; @@ -376,18 +373,15 @@ public bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlia // group, to the collection of properties that do not belong to a group oldPropertyGroup?.PropertyTypes?.RemoveItem(propertyTypeAlias); - if (newPropertyGroup is null) - { - if (PropertyTypeCollection.Contains(propertyTypeAlias) == false) - { - PropertyTypeCollection.Add(propertyType); - } - } - else + if (newPropertyGroup is not null) { PropertyTypeCollection.RemoveItem(propertyTypeAlias); newPropertyGroup.PropertyTypes?.Add(propertyType); } + else if (PropertyTypeCollection.Contains(propertyTypeAlias) is false) + { + PropertyTypeCollection.Add(propertyType); + } return true; } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs index 643c6ef8b24a..9be2e4b0e1d6 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -302,6 +302,41 @@ public void Can_Move_PropertyType_From_No_Group_Into_Group() }); } + [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(_titleAlias)); + }); + } + + [Test] + public void Can_Move_Already_Ungrouped_PropertyType_To_No_Group() + { + var contentType = BuildContentTypeWithSingleGroup(); + contentType.MovePropertyType("title", null); + + // 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(_titleAlias)); + Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); + Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty); + }); + } + [Test] public void Cannot_Move_PropertyType_To_Unknown_Group() { @@ -318,6 +353,26 @@ public void Cannot_Move_PropertyType_To_Unknown_Group() }); } + [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(_titleAlias)); + }); + } + private static ContentType BuildContentTypeWithSingleGroup() => (ContentType)new ContentTypeBuilder() .WithAlias("textPage") From edc6e9daef28d2fbdae912f1913761ea70f453cc Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 31 Jul 2026 07:27:10 +0100 Subject: [PATCH 4/6] fix: don't silently orphan the property when the target group has no collection PropertyGroup.PropertyTypes is nullable with a public setter that accepts null, so the previous null-conditional Add could no-op while MovePropertyType still returned true - removing the property from its old home and adding it nowhere, which is the same orphaning that then deletes it (and its content values) on save. Co-Authored-By: Claude Opus 5 (1M context) --- src/Umbraco.Core/Models/ContentTypeBase.cs | 6 ++++- .../Umbraco.Core/Models/ContentTypeTests.cs | 22 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 202ef96e6188..6b6d10925d6d 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -376,7 +376,11 @@ public bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlia if (newPropertyGroup is not null) { PropertyTypeCollection.RemoveItem(propertyTypeAlias); - newPropertyGroup.PropertyTypes?.Add(propertyType); + + // the group's collection is nullable, and adding to a null one would silently orphan + // the property, so ensure it exists rather than no-op away the move + newPropertyGroup.PropertyTypes ??= new PropertyTypeCollection(SupportsPublishing); + newPropertyGroup.PropertyTypes.Add(propertyType); } else if (PropertyTypeCollection.Contains(propertyTypeAlias) is false) { diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs index 9be2e4b0e1d6..a779f3fb0869 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -320,6 +320,28 @@ public void Can_Move_PropertyType_Between_Groups() }); } + [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(_titleAlias), + "the property type should not be orphaned"); + Assert.That( + contentType.PropertyGroups["meta"].PropertyTypes?.Select(x => x.Alias), + Is.EquivalentTo(_titleAlias)); + Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty); + }); + } + [Test] public void Can_Move_Already_Ungrouped_PropertyType_To_No_Group() { From 1725a30ccdf6c64cb5816afdb9fe13b296a79c16 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 31 Jul 2026 07:54:02 +0100 Subject: [PATCH 5/6] Supress CA1861. --- .../Umbraco.Core/Models/ContentTypeTests.cs | 23 ++++++++----------- .../Umbraco.Tests.UnitTests.csproj | 9 ++++++++ 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs index a779f3fb0869..2dd7e1e7efe7 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -14,9 +14,6 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Models; [TestFixture] public class ContentTypeTests { - private static readonly string[] _titleAlias = ["title"]; - private static readonly string[] _titleAndNoGroupAliases = ["title", "noGroup"]; - [Test] [Ignore("Ignoring this test until we actually enforce this, see comments in ContentTypeBase.PropertyTypesChanged")] public void Cannot_Add_Duplicate_Property_Aliases() @@ -279,8 +276,8 @@ public void Can_Move_PropertyType_To_No_Group() Assert.Multiple(() => { - Assert.That(contentType.PropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); - Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); + 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); }); } @@ -298,7 +295,7 @@ public void Can_Move_PropertyType_From_No_Group_Into_Group() Assert.That(contentType.NoGroupPropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(_titleAndNoGroupAliases)); + Is.EquivalentTo(new[] { "title", "noGroup" })); }); } @@ -316,7 +313,7 @@ public void Can_Move_PropertyType_Between_Groups() Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["meta"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(_titleAlias)); + Is.EquivalentTo(new[] { "title" })); }); } @@ -333,11 +330,11 @@ public void Can_Move_PropertyType_Into_Group_With_Uninitialised_PropertyTypes() { Assert.That( contentType.PropertyTypes.Select(x => x.Alias), - Is.EquivalentTo(_titleAlias), + Is.EquivalentTo(new[] { "title" }), "the property type should not be orphaned"); Assert.That( contentType.PropertyGroups["meta"].PropertyTypes?.Select(x => x.Alias), - Is.EquivalentTo(_titleAlias)); + Is.EquivalentTo(new[] { "title" })); Assert.That(contentType.PropertyGroups["content"].PropertyTypes, Is.Empty); }); } @@ -353,8 +350,8 @@ public void Can_Move_Already_Ungrouped_PropertyType_To_No_Group() Assert.Multiple(() => { - Assert.That(contentType.PropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); - Assert.That(contentType.NoGroupPropertyTypes.Select(x => x.Alias), Is.EquivalentTo(_titleAlias)); + 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); }); } @@ -371,7 +368,7 @@ public void Cannot_Move_PropertyType_To_Unknown_Group() Assert.That(contentType.NoGroupPropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(_titleAlias)); + Is.EquivalentTo(new[] { "title" })); }); } @@ -391,7 +388,7 @@ public void Cannot_Move_Unknown_PropertyType() Assert.That(contentType.NoGroupPropertyTypes, Is.Empty); Assert.That( contentType.PropertyGroups["content"].PropertyTypes!.Select(x => x.Alias), - Is.EquivalentTo(_titleAlias)); + Is.EquivalentTo(new[] { "title" })); }); } 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 + + From 15e67c3beceef60ac62e7ca919e0c40f887a804e Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 31 Jul 2026 08:11:12 +0100 Subject: [PATCH 6/6] Address test setup feedback. --- src/Umbraco.Core/Models/ContentTypeBase.cs | 4 +-- .../Services/ContentTypeServiceTests.cs | 35 ++++++++++++------- .../Umbraco.Core/Models/ContentTypeTests.cs | 25 +++++++++++-- 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 6b6d10925d6d..d6cd5c13141a 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -377,8 +377,8 @@ public bool MovePropertyType(string propertyTypeAlias, string? propertyGroupAlia { PropertyTypeCollection.RemoveItem(propertyTypeAlias); - // the group's collection is nullable, and adding to a null one would silently orphan - // the property, so ensure it exists rather than no-op away the move + // 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); } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs index 1a3ea0ab9557..8a79f772ff0b 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs @@ -2133,11 +2133,8 @@ public void Can_Move_PropertyType_To_No_Group_Without_Losing_Content_Values() [Test] public void Can_Move_PropertyType_From_No_Group_Into_Group() { - IContentType basePage = CreateContentTypeWithSingleGroupedProperty(); - - basePage.MovePropertyType("title", null); - ContentTypeService.Save(basePage); - basePage = ContentTypeService.Get(basePage.Id); + IContentType basePage = CreateContentTypeWithSingleUngroupedProperty(); + Assert.AreEqual("title", basePage.NoGroupPropertyTypes.SingleOrDefault()?.Alias, "the property type should start un-grouped"); Assert.IsTrue(basePage.MovePropertyType("title", "content")); @@ -2162,8 +2159,28 @@ private IContentType CreateContentTypeWithSingleGroupedProperty() { ContentType basePage = ContentTypeBuilder.CreateBasicContentType(); basePage.AddPropertyGroup("content", "Content"); + Assert.IsTrue(basePage.AddPropertyType(CreateTitlePropertyType(), "content", "Content")); + + ContentTypeService.Save(basePage); - var titlePropertyType = new PropertyType( + 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, @@ -2175,12 +2192,6 @@ private IContentType CreateContentTypeWithSingleGroupedProperty() SortOrder = 1, DataTypeId = Constants.DataTypes.Textbox, }; - Assert.IsTrue(basePage.AddPropertyType(titlePropertyType, "content", "Content")); - - ContentTypeService.Save(basePage); - - return ContentTypeService.Get(basePage.Id); - } [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 2dd7e1e7efe7..b07e138779e5 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Models/ContentTypeTests.cs @@ -342,8 +342,8 @@ public void Can_Move_PropertyType_Into_Group_With_Uninitialised_PropertyTypes() [Test] public void Can_Move_Already_Ungrouped_PropertyType_To_No_Group() { - var contentType = BuildContentTypeWithSingleGroup(); - contentType.MovePropertyType("title", null); + 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)); @@ -409,6 +409,27 @@ private static ContentType BuildContentTypeWithSingleGroup() => .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() {