Skip to content

Commit 254400b

Browse files
MigaroezAndyButlandkjac
authored
Block level variance: fix values being polluted when changing variance before publish (#21121)
* TDD solution to Block level variance publishing changing to no variance retaining block level variance values and vica versa * Add similar tests for the other block editors * Apply suggestions from code review Co-authored-by: Andy Butland <abutland73@gmail.com> * Amend the test scenarios * Simplify the merge clean-up to remove all misaligned values. * Fix failing test (remove false assumptions) --------- Co-authored-by: Andy Butland <abutland73@gmail.com> Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
1 parent 3bcdcc5 commit 254400b

File tree

5 files changed

+951
-8
lines changed

5 files changed

+951
-8
lines changed

‎src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -675,7 +675,8 @@ private void MergePartialPropertyValueForCulture(List<BlockItemData> sourceBlock
675675
{
676676
Alias = sourceBlockPropertyValue.Alias,
677677
Culture = sourceBlockPropertyValue.Culture,
678-
Segment = sourceBlockPropertyValue.Segment
678+
Segment = sourceBlockPropertyValue.Segment,
679+
PropertyType = sourceBlockPropertyValue.PropertyType
679680
};
680681
targetBlockItem.Values.Add(targetBlockPropertyValue);
681682
}
@@ -686,5 +687,20 @@ private void MergePartialPropertyValueForCulture(List<BlockItemData> sourceBlock
686687
: mergingDataEditor!.MergePartialPropertyValueForCulture(sourceBlockPropertyValue.Value, targetBlockPropertyValue.Value, culture);
687688
}
688689
}
690+
691+
// After merging, remove stale values when property variation changed.
692+
foreach (BlockItemData targetBlockItem in targetBlockItems)
693+
{
694+
targetBlockItem.Values.RemoveAll(value =>
695+
{
696+
if (value.PropertyType is null)
697+
{
698+
throw new ArgumentException("One or more block item values did not have a resolved property type. Block item value property types must be resolved before attempting perform partial value merging.", nameof(targetBlockItem));
699+
}
700+
701+
var propertyValueIsCultureVariant = value.Culture is not null;
702+
return propertyValueIsCultureVariant != value.PropertyType.VariesByCulture();
703+
});
704+
}
689705
}
690706
}

‎tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockGridElementLevelVariationTests.cs‎

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,4 +530,253 @@ private IContent CreateContent(IContentType contentType, BlockGridValue blockGri
530530
ContentService.Save(content);
531531
return content;
532532
}
533+
534+
[Test]
535+
public async Task Publishing_After_Changing_Element_Property_From_Variant_To_Invariant_Does_Not_Keep_Old_Culture_Specific_Values()
536+
{
537+
// 1. Create element type WITH culture variation
538+
var elementType = CreateElementType(ContentVariation.Culture);
539+
var areaKey = Guid.NewGuid();
540+
var blockGridDataType = await CreateBlockGridDataType(elementType, areaKey);
541+
var contentType = CreateContentType(blockGridDataType);
542+
543+
// 2. Create a simple block grid value with a single block for clarity
544+
var contentElementKey = Guid.NewGuid();
545+
var settingsElementKey = Guid.NewGuid();
546+
var blockGridValue = new BlockGridValue([
547+
new BlockGridLayoutItem(contentElementKey, settingsElementKey) { ColumnSpan = 12, RowSpan = 1 }
548+
])
549+
{
550+
ContentData =
551+
[
552+
new(contentElementKey, elementType.Key, elementType.Alias)
553+
{
554+
Values =
555+
[
556+
new() { Alias = "invariantText", Value = "The invariant content value" },
557+
new() { Alias = "variantText", Value = "The content value in English", Culture = "en-US" },
558+
new() { Alias = "variantText", Value = "The content value in Danish", Culture = "da-DK" }
559+
]
560+
}
561+
],
562+
SettingsData =
563+
[
564+
new(settingsElementKey, elementType.Key, elementType.Alias)
565+
{
566+
Values =
567+
[
568+
new() { Alias = "invariantText", Value = "The invariant settings value" },
569+
new() { Alias = "variantText", Value = "The settings value in English", Culture = "en-US" },
570+
new() { Alias = "variantText", Value = "The settings value in Danish", Culture = "da-DK" }
571+
]
572+
}
573+
],
574+
Expose =
575+
[
576+
new(contentElementKey, "en-US", null),
577+
new(contentElementKey, "da-DK", null)
578+
]
579+
};
580+
581+
var content = CreateContent(contentType, blockGridValue);
582+
PublishContent(content, ["en-US", "da-DK"]);
583+
584+
// 3. Change element property type to invariant (remove culture variation)
585+
elementType.PropertyTypes.Single(pt => pt.Alias == "variantText").Variations = ContentVariation.Nothing;
586+
587+
ContentTypeService.Save(elementType);
588+
589+
// 4. Update the content values to be invariant
590+
blockGridValue = JsonSerializer.Deserialize<BlockGridValue>((string)content.Properties["blocks"]!.GetValue()!)!;
591+
592+
foreach (var blockPropertyValue in blockGridValue.ContentData[0].Values.Where(v => v.Alias == "variantText"))
593+
{
594+
blockPropertyValue.Value += " => to invariant";
595+
blockPropertyValue.Culture = null;
596+
}
597+
598+
foreach (var blockPropertyValue in blockGridValue.SettingsData[0].Values.Where(v => v.Alias == "variantText"))
599+
{
600+
blockPropertyValue.Value += " => to invariant";
601+
blockPropertyValue.Culture = null;
602+
}
603+
604+
blockGridValue.Expose = blockGridValue.Expose
605+
.Select(e => new BlockItemVariation(e.ContentKey, null, null))
606+
.DistinctBy(e => e.ContentKey)
607+
.ToList();
608+
609+
content.Properties["blocks"]!.SetValue(JsonSerializer.Serialize(blockGridValue));
610+
ContentService.Save(content);
611+
612+
// 5. Publish
613+
PublishContent(content, ["en-US", "da-DK"]);
614+
615+
// 6. Verify published JSON doesn't contain old culture-specific values
616+
content = ContentService.GetById(content.Key)!;
617+
var publishedValue = (string?)content.Properties["blocks"]!.GetValue(null, null, published: true);
618+
Assert.IsNotNull(publishedValue, "Published value should not be null");
619+
620+
var publishedBlockGridValue = JsonSerializer.Deserialize<BlockGridValue>(publishedValue);
621+
Assert.IsNotNull(publishedBlockGridValue);
622+
623+
foreach (var contentData in publishedBlockGridValue!.ContentData)
624+
{
625+
var aliasGroups = contentData.Values.GroupBy(v => v.Alias);
626+
foreach (var group in aliasGroups)
627+
{
628+
Assert.AreEqual(
629+
1,
630+
group.Count(),
631+
$"Property '{group.Key}' has multiple values. Values: {string.Join(", ", group.Select(v => $"Culture={v.Culture ?? "null"}:Value={v.Value}"))}");
632+
}
633+
}
634+
635+
foreach (var settingsData in publishedBlockGridValue.SettingsData)
636+
{
637+
var aliasGroups = settingsData.Values.GroupBy(v => v.Alias);
638+
foreach (var group in aliasGroups)
639+
{
640+
Assert.AreEqual(
641+
1,
642+
group.Count(),
643+
$"Property '{group.Key}' has multiple values. Values: {string.Join(", ", group.Select(v => $"Culture={v.Culture ?? "null"}:Value={v.Value}"))}");
644+
}
645+
}
646+
}
647+
648+
[TestCase(true, true)]
649+
[TestCase(true, false)]
650+
[TestCase(false, true)]
651+
public async Task Publishing_After_Changing_Element_Property_From_Invariant_To_Variant_Does_Not_Keep_Old_Invariant_Values(bool republishEnglish, bool republishDanish)
652+
{
653+
// 1. Create element type WITHOUT culture variation (invariant)
654+
var elementType = CreateElementType(ContentVariation.Nothing);
655+
var areaKey = Guid.NewGuid();
656+
var blockGridDataType = await CreateBlockGridDataType(elementType, areaKey);
657+
var contentType = CreateContentType(blockGridDataType);
658+
659+
// 2. Create a simple block grid value with a single block
660+
var contentElementKey = Guid.NewGuid();
661+
var settingsElementKey = Guid.NewGuid();
662+
var blockGridValue = new BlockGridValue([
663+
new BlockGridLayoutItem(contentElementKey, settingsElementKey) { ColumnSpan = 12, RowSpan = 1 }
664+
])
665+
{
666+
ContentData =
667+
[
668+
new(contentElementKey, elementType.Key, elementType.Alias)
669+
{
670+
Values =
671+
[
672+
new() { Alias = "invariantText", Value = "The invariant content value" },
673+
new() { Alias = "variantText", Value = "The original invariant value for content" }
674+
]
675+
}
676+
],
677+
SettingsData =
678+
[
679+
new(settingsElementKey, elementType.Key, elementType.Alias)
680+
{
681+
Values =
682+
[
683+
new() { Alias = "invariantText", Value = "The invariant settings value" },
684+
new() { Alias = "variantText", Value = "The original invariant value for settings" }
685+
]
686+
}
687+
],
688+
Expose =
689+
[
690+
new(contentElementKey, null, null)
691+
]
692+
};
693+
694+
var content = CreateContent(contentType, blockGridValue);
695+
PublishContent(content, ["en-US", "da-DK"]);
696+
697+
// 3. Change element type to variant (add culture variation)
698+
elementType.Variations = ContentVariation.Culture;
699+
elementType.PropertyTypes.Single(pt => pt.Alias == "variantText").Variations = ContentVariation.Culture;
700+
701+
ContentTypeService.Save(elementType);
702+
703+
// 4. Update the content values to have culture-specific values
704+
blockGridValue = JsonSerializer.Deserialize<BlockGridValue>((string)content.Properties["blocks"]!.GetValue()!)!;
705+
706+
// Add variant values for the property that is now variant
707+
blockGridValue.ContentData[0].Values.RemoveAll(value => value.Alias == "variantText");
708+
blockGridValue.ContentData[0].Values.Add(new BlockPropertyValue
709+
{
710+
Alias = "variantText",
711+
Value = "The content value in English",
712+
Culture = "en-US"
713+
});
714+
blockGridValue.ContentData[0].Values.Add(new BlockPropertyValue
715+
{
716+
Alias = "variantText",
717+
Value = "The content value in Danish",
718+
Culture = "da-DK"
719+
});
720+
721+
blockGridValue.SettingsData[0].Values.RemoveAll(value => value.Alias == "variantText");
722+
blockGridValue.SettingsData[0].Values.Add(new BlockPropertyValue
723+
{
724+
Alias = "variantText",
725+
Value = "The settings value in English",
726+
Culture = "en-US"
727+
});
728+
blockGridValue.SettingsData[0].Values.Add(new BlockPropertyValue
729+
{
730+
Alias = "variantText",
731+
Value = "The settings value in Danish",
732+
Culture = "da-DK"
733+
});
734+
735+
blockGridValue.Expose =
736+
[
737+
new BlockItemVariation(contentElementKey, "en-US", null),
738+
new BlockItemVariation(contentElementKey, "da-DK", null)
739+
];
740+
741+
content.Properties["blocks"]!.SetValue(JsonSerializer.Serialize(blockGridValue));
742+
ContentService.Save(content);
743+
744+
// 5. Publish selected cultures
745+
string[] culturesToPublish = republishEnglish && republishDanish
746+
? ["en-US", "da-DK"]
747+
: republishEnglish
748+
? ["en-US"]
749+
: republishDanish
750+
? ["da-DK"]
751+
: throw new ArgumentException("Can't proceed without republishing at least one culture");
752+
PublishContent(content, culturesToPublish);
753+
754+
// 6. Verify published JSON doesn't contain old invariant values for variantText
755+
content = ContentService.GetById(content.Key)!;
756+
var publishedValue = (string?)content.Properties["blocks"]!.GetValue(null, null, published: true);
757+
Assert.IsNotNull(publishedValue, "Published value should not be null");
758+
759+
var publishedBlockGridValue = JsonSerializer.Deserialize<BlockGridValue>(publishedValue);
760+
Assert.IsNotNull(publishedBlockGridValue);
761+
762+
// Verify ContentData entries are not duplicated
763+
Assert.AreEqual(1, publishedBlockGridValue!.ContentData.Count, "Should have exactly 1 content data entry");
764+
Assert.AreEqual(1, publishedBlockGridValue.SettingsData.Count, "Should have exactly 1 settings data entry");
765+
766+
var variantTextValues = publishedBlockGridValue.ContentData[0].Values.Where(v => v.Alias == "variantText").ToList();
767+
Assert.IsFalse(
768+
variantTextValues.Any(v => v.Culture is null),
769+
$"variantText property should not have invariant values after changing to variant. Values: {string.Join(", ", variantTextValues.Select(v => $"Culture={v.Culture ?? "null"}:Value={v.Value}"))}");
770+
771+
variantTextValues = publishedBlockGridValue.SettingsData[0].Values.Where(v => v.Alias == "variantText").ToList();
772+
Assert.IsFalse(
773+
variantTextValues.Any(v => v.Culture is null),
774+
$"variantText property should not have invariant values after changing to variant. Values: {string.Join(", ", variantTextValues.Select(v => $"Culture={v.Culture ?? "null"}:Value={v.Value}"))}");
775+
776+
// Verify Expose entries are not duplicated
777+
var exposeGroups = publishedBlockGridValue.Expose.GroupBy(e => (e.ContentKey, e.Culture, e.Segment));
778+
Assert.IsTrue(
779+
exposeGroups.All(g => g.Count() == 1),
780+
$"Duplicate Expose entries found. Expose: {string.Join(", ", publishedBlockGridValue.Expose.Select(e => $"{e.ContentKey}:{e.Culture}:{e.Segment}"))}");
781+
}
533782
}

‎tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockListElementLevelVariationTests.Parsing.cs‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -268,13 +268,6 @@ public async Task Can_Become_Variant_After_Publish(string culture, bool expectEx
268268

269269
RefreshContentTypeCache(elementType, contentType);
270270

271-
// to re-publish the content in both cultures we need to set the culture names
272-
content = ContentService.GetById(content.Key)!;
273-
content.SetCultureName("Home (en)", "en-US");
274-
content.SetCultureName("Home (da)", "da-DK");
275-
ContentService.Save(content);
276-
PublishContent(content, contentType);
277-
278271
var publishedContent = GetPublishedContent(content.Key);
279272

280273
SetVariationContext(culture, null);

0 commit comments

Comments
 (0)