From 82c3af51fea5e9da6f720c5b277236c1e635a50b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Knippers?= Date: Fri, 23 Aug 2019 13:19:33 +0200 Subject: [PATCH 1/2] Fixes writing / reading segment data combined with segment null --- .../ContentVariationExtensions.cs | 57 ++++++++++--------- .../Compose/NotificationsComponent.cs | 3 - 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/Umbraco.Core/ContentVariationExtensions.cs b/src/Umbraco.Core/ContentVariationExtensions.cs index 9fdc5f0b90..2c838eecbc 100644 --- a/src/Umbraco.Core/ContentVariationExtensions.cs +++ b/src/Umbraco.Core/ContentVariationExtensions.cs @@ -150,39 +150,44 @@ namespace Umbraco.Core culture = culture.NullOrWhiteSpaceAsNull(); segment = segment.NullOrWhiteSpaceAsNull(); - bool Validate(bool variesBy, string value) - { - if (variesBy) - { - // varies by - // in exact mode, the value cannot be null (but it can be a wildcard) - // in !wildcards mode, the value cannot be a wildcard (but it can be null) - if ((exact && value == null) || (!wildcards && value == "*")) - return false; - } - else - { - // does not vary by value - // the value cannot have a value - // unless wildcards and it's "*" - if (value != null && (!wildcards || value != "*")) - return false; - } - - return true; - } - - if (!Validate(variation.VariesByCulture(), culture)) + // if wildcards are disabled, do not allow "*" + if (!wildcards && (culture == "*" || segment == "*")) { if (throwIfInvalid) - throw new NotSupportedException($"Culture value \"{culture ?? ""}\" is invalid."); + throw new NotSupportedException($"Variation wildcards are not supported."); return false; } - if (!Validate(variation.VariesBySegment(), segment)) + if (variation.VariesByCulture()) + { + // varies by culture + // in exact mode, the culture cannot be null + if (exact && culture == null) + { + if (throwIfInvalid) + throw new NotSupportedException($"Culture may not be null because culture variation is enabled."); + return false; + } + } + else + { + // does not vary by culture + // the culture cannot have a value + // unless wildcards and it's "*" + if (culture != null && !(wildcards && culture == "*")) + { + if (throwIfInvalid) + throw new NotSupportedException($"Culture \"{culture}\" is invalid because culture variation is disabled."); + return false; + } + } + + // if it does not vary by segment + // the segment cannot have a value + if (!variation.VariesBySegment() && segment != null && !(wildcards && segment == "*")) { if (throwIfInvalid) - throw new NotSupportedException($"Segment value \"{segment ?? ""}\" is invalid."); + throw new NotSupportedException($"Segment \"{segment}\" is invalid because segment variation is disabled."); return false; } diff --git a/src/Umbraco.Web/Compose/NotificationsComponent.cs b/src/Umbraco.Web/Compose/NotificationsComponent.cs index db602b3db9..9b2609718e 100644 --- a/src/Umbraco.Web/Compose/NotificationsComponent.cs +++ b/src/Umbraco.Web/Compose/NotificationsComponent.cs @@ -209,9 +209,6 @@ namespace Umbraco.Web.Compose //group by the content type variation since the emails will be different foreach(var contentVariantGroup in entities.GroupBy(x => x.ContentType.Variations)) { - if (contentVariantGroup.Key == ContentVariation.CultureAndSegment || contentVariantGroup.Key == ContentVariation.Segment) - throw new NotSupportedException("Segments are not yet supported in Umbraco"); - _notificationService.SendNotifications( sender, contentVariantGroup, From aa4677e1ddf598dae8ddcd3b915f4c8ebe3b541c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Knippers?= Date: Thu, 19 Sep 2019 13:50:19 +0200 Subject: [PATCH 2/2] Added and updated unit tests for the ValidateVariation method --- .../ContentVariationExtensions.cs | 2 + src/Umbraco.Tests/Models/VariationTests.cs | 96 +++++++++++++++---- 2 files changed, 79 insertions(+), 19 deletions(-) diff --git a/src/Umbraco.Core/ContentVariationExtensions.cs b/src/Umbraco.Core/ContentVariationExtensions.cs index 2c838eecbc..5b157307ab 100644 --- a/src/Umbraco.Core/ContentVariationExtensions.cs +++ b/src/Umbraco.Core/ContentVariationExtensions.cs @@ -184,6 +184,8 @@ namespace Umbraco.Core // if it does not vary by segment // the segment cannot have a value + // segment may always be null, even when the ContentVariation.Segment flag is set for this variation, + // therefore the exact parameter is not used in segment validation. if (!variation.VariesBySegment() && segment != null && !(wildcards && segment == "*")) { if (throwIfInvalid) diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 8d64a6d28f..62c4849855 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -83,37 +83,95 @@ namespace Umbraco.Tests.Models Assert.AreEqual(nw, v.ValidateVariation(c, s, false, true, false)); } - // always support invariant,neutral - Assert4A(ContentVariation.Nothing, null, null, true); + // All tests: + // 1. if exact is set to true: culture cannot be null when the ContentVariation.Culture flag is set + // 2. if wildcards is set to false: fail when "*" is passed in as either culture or segment. + // 3. ContentVariation flag is ignored when wildcards are used. + // 4. Empty string is considered the same as null - // never support culture and/or segment - Assert4A(ContentVariation.Nothing, "culture", null, false); + #region Nothing + + Assert4A(ContentVariation.Nothing, null, null, true); + Assert4A(ContentVariation.Nothing, null, "", true); + Assert4B(ContentVariation.Nothing, null, "*", true, false, false, true); Assert4A(ContentVariation.Nothing, null, "segment", false); + Assert4A(ContentVariation.Nothing, "", null, true); + Assert4A(ContentVariation.Nothing, "", "", true); + Assert4B(ContentVariation.Nothing, "", "*", true, false, false, true); + Assert4A(ContentVariation.Nothing, "", "segment", false); + Assert4B(ContentVariation.Nothing, "*", null, true, false, false, true); + Assert4B(ContentVariation.Nothing, "*", "", true, false, false, true); + Assert4B(ContentVariation.Nothing, "*", "*", true, false, false, true); + Assert4A(ContentVariation.Nothing, "*", "segment", false); + Assert4A(ContentVariation.Nothing, "culture", null, false); + Assert4A(ContentVariation.Nothing, "culture", "", false); + Assert4A(ContentVariation.Nothing, "culture", "*", false); Assert4A(ContentVariation.Nothing, "culture", "segment", false); - // support '*' only when wildcards are supported - Assert4B(ContentVariation.Nothing, "*", null, true, false, false, true); - Assert4B(ContentVariation.Nothing, null, "*", true, false, false, true); - Assert4B(ContentVariation.Nothing, "*", "*", true, false, false, true); + #endregion + #region Culture - // support invariant if not exact Assert4B(ContentVariation.Culture, null, null, false, true, false, true); - - // support invariant if not exact, '*' when wildcards are supported - Assert4B(ContentVariation.Culture, "*", null, true, false, false, true); + Assert4B(ContentVariation.Culture, null, "", false, true, false, true); Assert4B(ContentVariation.Culture, null, "*", false, false, false, true); - Assert4B(ContentVariation.Culture, "*", "*", true, false, false, true); - - // never support segment Assert4A(ContentVariation.Culture, null, "segment", false); - Assert4A(ContentVariation.Culture, "culture", "segment", false); + Assert4B(ContentVariation.Culture, "", null, false, true, false, true); + Assert4B(ContentVariation.Culture, "", "", false, true, false, true); + Assert4B(ContentVariation.Culture, "", "*", false, false, false, true); + Assert4A(ContentVariation.Culture, "", "segment", false); + Assert4B(ContentVariation.Culture, "*", null, true, false, false, true); + Assert4B(ContentVariation.Culture, "*", "", true, false, false, true); + Assert4B(ContentVariation.Culture, "*", "*", true, false, false, true); Assert4A(ContentVariation.Culture, "*", "segment", false); - - Assert4B(ContentVariation.Culture, null, "*", false, false, false, true); + Assert4A(ContentVariation.Culture, "culture", null, true); + Assert4A(ContentVariation.Culture, "culture", "", true); Assert4B(ContentVariation.Culture, "culture", "*", true, false, false, true); + Assert4A(ContentVariation.Culture, "culture", "segment", false); - // could do the same with .Segment, and .CultureAndSegment + #endregion + + #region Segment + + Assert4B(ContentVariation.Segment, null, null, true, true, true, true); + Assert4B(ContentVariation.Segment, null, "", true, true, true, true); + Assert4B(ContentVariation.Segment, null, "*", true, false, false, true); + Assert4A(ContentVariation.Segment, null, "segment", true); + Assert4B(ContentVariation.Segment, "", null, true, true, true, true); + Assert4B(ContentVariation.Segment, "", "", true, true, true, true); + Assert4B(ContentVariation.Segment, "", "*", true, false, false, true); + Assert4A(ContentVariation.Segment, "", "segment", true); + Assert4B(ContentVariation.Segment, "*", null, true, false, false, true); + Assert4B(ContentVariation.Segment, "*", "", true, false, false, true); + Assert4B(ContentVariation.Segment, "*", "*", true, false, false, true); + Assert4B(ContentVariation.Segment, "*", "segment", true, false, false, true); + Assert4A(ContentVariation.Segment, "culture", null, false); + Assert4A(ContentVariation.Segment, "culture", "", false); + Assert4A(ContentVariation.Segment, "culture", "*", false); + Assert4A(ContentVariation.Segment, "culture", "segment", false); + + #endregion + + #region CultureAndSegment + + Assert4B(ContentVariation.CultureAndSegment, null, null, false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, null, "", false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, null, "*", false, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, null, "segment", false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, "", null, false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, "", "", false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, "", "*", false, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "", "segment", false, true, false, true); + Assert4B(ContentVariation.CultureAndSegment, "*", null, true, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "*", "", true, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "*", "*", true, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "*", "segment", true, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "culture", null, true, true, true, true); + Assert4B(ContentVariation.CultureAndSegment, "culture", "", true, true, true, true); + Assert4B(ContentVariation.CultureAndSegment, "culture", "*", true, false, false, true); + Assert4B(ContentVariation.CultureAndSegment, "culture", "segment", true, true, true, true); + + #endregion } [Test]