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/4] 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/4] 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] From af8c3459e35ba64db9ac84b322c8ef49030bf0a8 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 30 Sep 2019 15:59:45 +0200 Subject: [PATCH 3/4] Moves Assert4A/B methods to methods and renames some parameters ... need to figure out what the rest mean --- src/Umbraco.Tests/Models/VariationTests.cs | 25 +++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 62c4849855..6ead404a09 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -70,18 +70,6 @@ namespace Umbraco.Tests.Models [Test] public void ValidateVariationTests() { - void Assert4A(ContentVariation v, string c, string s, bool xx) - { - Assert4B(v, c, s, xx, xx, xx, xx); - } - - void Assert4B(ContentVariation v, string c, string s, bool ew, bool nn, bool en, bool nw) - { - Assert.AreEqual(ew, v.ValidateVariation(c, s, true, true, false)); - Assert.AreEqual(nn, v.ValidateVariation(c, s, false, false, false)); - Assert.AreEqual(en, v.ValidateVariation(c, s, true, false, false)); - Assert.AreEqual(nw, v.ValidateVariation(c, s, false, true, false)); - } // All tests: // 1. if exact is set to true: culture cannot be null when the ContentVariation.Culture flag is set @@ -174,6 +162,19 @@ namespace Umbraco.Tests.Models #endregion } + private static void Assert4B(ContentVariation v, string culture, string segment, bool ew, bool nn, bool en, bool nw) + { + Assert.AreEqual(ew, v.ValidateVariation(culture, segment, true, true, false)); + Assert.AreEqual(nn, v.ValidateVariation(culture, segment, false, false, false)); + Assert.AreEqual(en, v.ValidateVariation(culture, segment, true, false, false)); + Assert.AreEqual(nw, v.ValidateVariation(culture, segment, false, true, false)); + } + + private static void Assert4A(ContentVariation v, string culture, string segment, bool xx) + { + Assert4B(v, culture, segment, xx, xx, xx, xx); + } + [Test] public void PropertyTests() { From d97101a14bf7aca069d5e81e21494179ac3bf026 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 1 Oct 2019 09:37:43 +0200 Subject: [PATCH 4/4] Updates tests to rename assertion method parameters including code comments --- src/Umbraco.Tests/Models/VariationTests.cs | 33 +++++++++++++++++----- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 6ead404a09..ab5f726894 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -162,17 +162,36 @@ namespace Umbraco.Tests.Models #endregion } - private static void Assert4B(ContentVariation v, string culture, string segment, bool ew, bool nn, bool en, bool nw) + /// + /// Asserts the result of + /// + /// + /// + /// + /// Validate using Exact + Wildcards flags + /// Validate using non Exact + no Wildcard flags + /// Validate using Exact + no Wildcard flags + /// Validate using non Exact + Wildcard flags + private static void Assert4B(ContentVariation variation, string culture, string segment, + bool exactAndWildcards, bool nonExactAndNoWildcards, bool exactAndNoWildcards, bool nonExactAndWildcards) { - Assert.AreEqual(ew, v.ValidateVariation(culture, segment, true, true, false)); - Assert.AreEqual(nn, v.ValidateVariation(culture, segment, false, false, false)); - Assert.AreEqual(en, v.ValidateVariation(culture, segment, true, false, false)); - Assert.AreEqual(nw, v.ValidateVariation(culture, segment, false, true, false)); + Assert.AreEqual(exactAndWildcards, variation.ValidateVariation(culture, segment, true, true, false)); + Assert.AreEqual(nonExactAndNoWildcards, variation.ValidateVariation(culture, segment, false, false, false)); + Assert.AreEqual(exactAndNoWildcards, variation.ValidateVariation(culture, segment, true, false, false)); + Assert.AreEqual(nonExactAndWildcards, variation.ValidateVariation(culture, segment, false, true, false)); } - private static void Assert4A(ContentVariation v, string culture, string segment, bool xx) + /// + /// Asserts the result of + /// where expectedResult matches all combinations of Exact + Wildcard + /// + /// + /// + /// + /// + private static void Assert4A(ContentVariation variation, string culture, string segment, bool expectedResult) { - Assert4B(v, culture, segment, xx, xx, xx, xx); + Assert4B(variation, culture, segment, expectedResult, expectedResult, expectedResult, expectedResult); } [Test]