Skip to content

Commit 2289493

Browse files
lauranetolauraneto
authored
Moving properties between groups sometimes clears their values (#19881)
* Fix moving properties between groups sometimes clearing their values * Small adjustment * Fix failing integration test The mapping method was only setting the property group when it was not null, but for orphaned properties we want to specifically set it to null. * Adjust 'Can_Move_Properties_To_Another_Container' integration test to check more scenarios and that values are kept * Adjust to add isElement variable in test (as previously)
1 parent 2def046 commit 2289493

File tree

3 files changed

+122
-43
lines changed

3 files changed

+122
-43
lines changed

‎src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingServiceBase.cs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,10 @@ private async Task UpdatePropertiesAsync(
520520
// the containers and their parent relationships in the model, so it's ok
521521
container => model.Containers.First(c => c.Key == container.ParentKey).Name);
522522

523+
// Store the existing property types in a list to reference when processing properties.
524+
// This ensures we correctly handle property types that may have been filtered out from groups.
525+
var existingPropertyTypes = contentType.PropertyTypes.ToList();
526+
523527
// handle properties in groups
524528
PropertyGroup[] propertyGroups = model.Containers.Select(container =>
525529
{
@@ -540,7 +544,7 @@ private async Task UpdatePropertiesAsync(
540544
IPropertyType[] properties = model
541545
.Properties
542546
.Where(property => property.ContainerKey == container.Key)
543-
.Select(property => MapProperty(contentType, property, propertyGroup, dataTypesByKey))
547+
.Select(property => MapProperty(contentType, property, propertyGroup, existingPropertyTypes, dataTypesByKey))
544548
.ToArray();
545549

546550
if (properties.Any() is false && parentContainerNamesById.ContainsKey(container.Key) is false)
@@ -565,8 +569,8 @@ private async Task UpdatePropertiesAsync(
565569
}
566570

567571
// handle orphaned properties
568-
IEnumerable<TPropertyTypeModel> orphanedPropertyTypeModels = model.Properties.Where (x => x.ContainerKey is null).ToArray();
569-
IPropertyType[] orphanedPropertyTypes = orphanedPropertyTypeModels.Select(property => MapProperty(contentType, property, null, dataTypesByKey)).ToArray();
572+
IEnumerable<TPropertyTypeModel> orphanedPropertyTypeModels = model.Properties.Where(x => x.ContainerKey is null).ToArray();
573+
IPropertyType[] orphanedPropertyTypes = orphanedPropertyTypeModels.Select(property => MapProperty(contentType, property, null, existingPropertyTypes, dataTypesByKey)).ToArray();
570574
if (contentType.NoGroupPropertyTypes.SequenceEqual(orphanedPropertyTypes) is false)
571575
{
572576
contentType.NoGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing, orphanedPropertyTypes);
@@ -588,6 +592,7 @@ private IPropertyType MapProperty(
588592
TContentType contentType,
589593
TPropertyTypeModel property,
590594
PropertyGroup? propertyGroup,
595+
IEnumerable<IPropertyType> existingPropertyTypes,
591596
IDictionary<Guid, IDataType> dataTypesByKey)
592597
{
593598
// get the selected data type
@@ -598,7 +603,7 @@ private IPropertyType MapProperty(
598603
}
599604

600605
// get the current property type (if it exists)
601-
IPropertyType propertyType = contentType.PropertyTypes.FirstOrDefault(pt => pt.Key == property.Key)
606+
IPropertyType propertyType = existingPropertyTypes.FirstOrDefault(pt => pt.Key == property.Key)
602607
?? new PropertyType(_shortStringHelper, dataType);
603608

604609
// We are demanding a property type key in the model, so we should probably ensure that it's the on that's actually used.
@@ -617,10 +622,9 @@ private IPropertyType MapProperty(
617622
propertyType.SortOrder = property.SortOrder;
618623
propertyType.LabelOnTop = property.Appearance.LabelOnTop;
619624

620-
if (propertyGroup is not null)
621-
{
622-
propertyType.PropertyGroupId = new Lazy<int>(() => propertyGroup.Id, false);
623-
}
625+
propertyType.PropertyGroupId = propertyGroup is null
626+
? null
627+
: new Lazy<int>(() => propertyGroup.Id, false);
624628

625629
return propertyType;
626630
}

‎tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTests.Update.cs

Lines changed: 108 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Umbraco.Cms.Core.Models;
44
using Umbraco.Cms.Core.Models.ContentTypeEditing;
55
using Umbraco.Cms.Core.Services.OperationStatus;
6+
using Umbraco.Cms.Tests.Common.Builders;
67

78
namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services;
89

@@ -305,46 +306,118 @@ public async Task Can_Edit_Properties(bool isElement)
305306
Assert.AreEqual(1, contentType.NoGroupPropertyTypes.Count());
306307
}
307308

308-
[TestCase(false)]
309-
[TestCase(true)]
310-
public async Task Can_Move_Properties_To_Another_Container(bool isElement)
309+
public enum PropertyMoveOperation
311310
{
312-
var createModel = ContentTypeCreateModel("Test", "test", isElement: isElement);
313-
var container1 = ContentTypePropertyContainerModel("One");
314-
createModel.Containers = new[] { container1 };
311+
ToEarlier,
312+
ToLater,
313+
}
315314

315+
[TestCase(GroupContainerType, PropertyMoveOperation.ToEarlier, false)]
316+
[TestCase(TabContainerType, PropertyMoveOperation.ToEarlier, false)]
317+
[TestCase(GroupContainerType, PropertyMoveOperation.ToLater, false)]
318+
[TestCase(TabContainerType, PropertyMoveOperation.ToLater, false)]
319+
[TestCase(GroupContainerType, PropertyMoveOperation.ToEarlier, true)]
320+
[TestCase(TabContainerType, PropertyMoveOperation.ToEarlier, true)]
321+
[TestCase(GroupContainerType, PropertyMoveOperation.ToLater, true)]
322+
[TestCase(TabContainerType, PropertyMoveOperation.ToLater, true)]
323+
public async Task Can_Move_Properties_To_Another_Container(string containerType, PropertyMoveOperation propertyMoveOperation, bool isElement)
324+
{
325+
var container1 = ContentTypePropertyContainerModel($"{containerType} 1", containerType);
326+
var container2 = ContentTypePropertyContainerModel($"{containerType} 2", containerType);
316327
var propertyType1 = ContentTypePropertyTypeModel("Test Property 1", "testProperty1", containerKey: container1.Key);
317328
var propertyType2 = ContentTypePropertyTypeModel("Test Property 2", "testProperty2", containerKey: container1.Key);
318-
createModel.Properties = new[] { propertyType1, propertyType2 };
319-
320-
var contentType = (await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!;
329+
var propertyType3 = ContentTypePropertyTypeModel("Test Property 3", "testProperty3", containerKey: container2.Key);
330+
var propertyType4 = ContentTypePropertyTypeModel("Test Property 4", "testProperty4", containerKey: container2.Key);
331+
List<ContentTypePropertyTypeModel> properties = [propertyType1, propertyType2, propertyType3, propertyType4];
332+
var createModel = ContentTypeCreateModel(
333+
"Test Content Type",
334+
containers: [container1, container2],
335+
propertyTypes: properties,
336+
isElement: isElement);
321337

322-
var updateModel = ContentTypeUpdateModel("Test", "test", isElement: isElement);
323-
var container2 = ContentTypePropertyContainerModel("Two");
324-
container2.SortOrder = 0;
325-
container1.SortOrder = 1;
326-
updateModel.Containers = new[] { container1, container2 };
327-
propertyType2.ContainerKey = container2.Key;
328-
updateModel.Properties = new[] { propertyType1, propertyType2 };
338+
var createAttempt = await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey);
339+
Assert.Multiple(() =>
340+
{
341+
Assert.IsTrue(createAttempt.Success);
342+
Assert.AreEqual(ContentTypeOperationStatus.Success, createAttempt.Status);
343+
Assert.IsNotNull(createAttempt.Result);
344+
345+
Assert.AreEqual(4, createAttempt.Result.PropertyTypes.Count());
346+
Assert.AreEqual(2, createAttempt.Result.PropertyGroups.Count);
347+
var createdContainer1 = createAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container1.Key);
348+
Assert.NotNull(createdContainer1);
349+
Assert.NotNull(createdContainer1.PropertyTypes);
350+
Assert.AreEqual(2, createdContainer1.PropertyTypes.Count);
351+
Assert.AreEqual("Test Property 1", createdContainer1.PropertyTypes[0].Name);
352+
Assert.AreEqual("Test Property 2", createdContainer1.PropertyTypes[1].Name);
353+
var createdContainer2 = createAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container2.Key);
354+
Assert.NotNull(createdContainer2);
355+
Assert.NotNull(createdContainer2.PropertyTypes);
356+
Assert.AreEqual(2, createdContainer2.PropertyTypes?.Count);
357+
Assert.AreEqual("Test Property 3", createdContainer2.PropertyTypes[0].Name);
358+
Assert.AreEqual("Test Property 4", createdContainer2.PropertyTypes[1].Name);
359+
});
329360

330-
var result = await ContentTypeEditingService.UpdateAsync(contentType, updateModel, Constants.Security.SuperUserKey);
331-
Assert.IsTrue(result.Success);
332-
Assert.IsNotNull(result.Result);
361+
Content content = ContentBuilder.CreateBasicContent(createAttempt.Result!);
362+
foreach (var property in properties)
363+
{
364+
content.Properties[property.Alias]!.SetValue(property.Name);
365+
}
333366

334-
// Ensure it's actually persisted
335-
contentType = await ContentTypeService.GetAsync(result.Result!.Key);
367+
ContentService.Save(content);
336368

337-
Assert.IsNotNull(contentType);
338-
Assert.AreEqual(isElement, contentType.IsElement);
339-
Assert.AreEqual(2, contentType.PropertyGroups.Count);
340-
Assert.AreEqual(2, contentType.PropertyTypes.Count());
341-
Assert.AreEqual(1, contentType.PropertyGroups.First().PropertyTypes!.Count);
342-
Assert.AreEqual(1, contentType.PropertyGroups.Last().PropertyTypes!.Count);
343-
Assert.IsEmpty(contentType.NoGroupPropertyTypes);
369+
if (propertyMoveOperation == PropertyMoveOperation.ToEarlier)
370+
{
371+
propertyType3.ContainerKey = container1.Key;
372+
}
373+
else
374+
{
375+
propertyType1.ContainerKey = container2.Key;
376+
}
344377

345-
var sortedPropertyGroups = contentType.PropertyGroups.OrderBy(g => g.SortOrder).ToArray();
346-
Assert.AreEqual("testProperty2", sortedPropertyGroups.First().PropertyTypes!.Single().Alias);
347-
Assert.AreEqual("testProperty1", sortedPropertyGroups.Last().PropertyTypes!.Single().Alias);
378+
var updateModel = ContentTypeUpdateModel(
379+
"Test Content Type",
380+
containers: [container1, container2],
381+
propertyTypes: [propertyType1, propertyType2, propertyType3, propertyType4]);
382+
var updateAttempt = await ContentTypeEditingService.UpdateAsync(createAttempt.Result, updateModel, Constants.Security.SuperUserKey);
383+
Assert.Multiple(() =>
384+
{
385+
Assert.IsTrue(updateAttempt.Success);
386+
Assert.AreEqual(ContentTypeOperationStatus.Success, updateAttempt.Status);
387+
Assert.AreEqual(4, updateAttempt.Result!.PropertyTypes.Count());
388+
Assert.AreEqual(2, updateAttempt.Result.PropertyGroups.Count);
389+
390+
var updatedContainer1 = updateAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container1.Key);
391+
Assert.NotNull(updatedContainer1?.PropertyTypes);
392+
var updatedContainer2 = updateAttempt.Result.PropertyGroups.SingleOrDefault(x => x.Key == container2.Key);
393+
Assert.NotNull(updatedContainer2?.PropertyTypes);
394+
if (propertyMoveOperation == PropertyMoveOperation.ToEarlier)
395+
{
396+
Assert.AreEqual(3, updatedContainer1.PropertyTypes.Count);
397+
Assert.AreEqual("Test Property 1", updatedContainer1.PropertyTypes[0].Name);
398+
Assert.AreEqual("Test Property 2", updatedContainer1.PropertyTypes[1].Name);
399+
Assert.AreEqual("Test Property 3", updatedContainer1.PropertyTypes[2].Name);
400+
Assert.AreEqual(1, updatedContainer2.PropertyTypes.Count);
401+
Assert.AreEqual("Test Property 4", updatedContainer2.PropertyTypes[0].Name);
402+
}
403+
else
404+
{
405+
Assert.AreEqual(1, updatedContainer1.PropertyTypes.Count);
406+
Assert.AreEqual("Test Property 2", updatedContainer1.PropertyTypes[0].Name);
407+
Assert.AreEqual(3, updatedContainer2.PropertyTypes.Count);
408+
Assert.AreEqual("Test Property 1", updatedContainer2.PropertyTypes[0].Name);
409+
Assert.AreEqual("Test Property 3", updatedContainer2.PropertyTypes[1].Name);
410+
Assert.AreEqual("Test Property 4", updatedContainer2.PropertyTypes[2].Name);
411+
}
412+
413+
Assert.AreEqual(0, updateAttempt.Result.NoGroupPropertyTypes.Count());
414+
415+
var updatedContent = ContentService.GetById(content.Id);
416+
foreach (var property in properties)
417+
{
418+
Assert.AreEqual(property.Name, updatedContent?.Properties[property.Alias]?.GetValue());
419+
}
420+
});
348421
}
349422

350423
[TestCase(false)]
@@ -393,18 +466,18 @@ public async Task Can_Make_Properties_Orphaned(bool isElement)
393466
var createModel = ContentTypeCreateModel("Test", "test", isElement: isElement);
394467
var container1 = ContentTypePropertyContainerModel("One");
395468
var container2 = ContentTypePropertyContainerModel("Two");
396-
createModel.Containers = new[] { container1, container2 };
469+
createModel.Containers = [container1, container2];
397470

398471
var propertyType1 = ContentTypePropertyTypeModel("Test Property 1", "testProperty1", containerKey: container1.Key);
399472
var propertyType2 = ContentTypePropertyTypeModel("Test Property 2", "testProperty2", containerKey: container2.Key);
400-
createModel.Properties = new[] { propertyType1, propertyType2 };
473+
createModel.Properties = [propertyType1, propertyType2];
401474

402475
var contentType = (await ContentTypeEditingService.CreateAsync(createModel, Constants.Security.SuperUserKey)).Result!;
403476

404477
var updateModel = ContentTypeUpdateModel("Test", "test", isElement: isElement);
405-
updateModel.Containers = new[] { container1 };
478+
updateModel.Containers = [container1];
406479
propertyType2.ContainerKey = null;
407-
updateModel.Properties = new[] { propertyType1, propertyType2 };
480+
updateModel.Properties = [propertyType1, propertyType2];
408481

409482
var result = await ContentTypeEditingService.UpdateAsync(contentType, updateModel, Constants.Security.SuperUserKey);
410483
Assert.IsTrue(result.Success);

‎tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTestsBase.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ internal abstract class ContentTypeEditingServiceTestsBase : UmbracoIntegrationT
2121

2222
protected IContentTypeService ContentTypeService => GetRequiredService<IContentTypeService>();
2323

24+
protected IContentService ContentService => GetRequiredService<IContentService>();
25+
2426
protected IMediaTypeService MediaTypeService => GetRequiredService<IMediaTypeService>();
2527

2628
protected const string TabContainerType = "Tab";

0 commit comments

Comments
 (0)