From 3871cdf10c064916d13e99cbc09387a04e3b5e4a Mon Sep 17 00:00:00 2001 From: Anthony Date: Mon, 19 Oct 2020 20:01:55 +0100 Subject: [PATCH 1/3] Ignored test artifacts --- .gitignore | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.gitignore b/.gitignore index 8fb2789eea..dc6f431550 100644 --- a/.gitignore +++ b/.gitignore @@ -189,3 +189,9 @@ cypress.env.json /src/Umbraco.Web.UI.NetCore/App_Data/TEMP/* /src/Umbraco.Web.UI.NetCore/App_Data/Smidge/Cache/* /src/Umbraco.Web.UI.NetCore/umbraco/logs + +src/Umbraco.Tests.Integration/umbraco/logs/ + +src/Umbraco.Tests.Integration/Views/ + +src/Umbraco.Tests/TEMP/ From c1e59525a3aff39eca489e24f5a43278ba42b8ba Mon Sep 17 00:00:00 2001 From: Anthony Date: Mon, 19 Oct 2020 20:06:49 +0100 Subject: [PATCH 2/3] Refactored unique node name generation with new BDD tests --- .../Repositories/Implement/SimilarNodeName.cs | 325 ++++++++++++------ .../Repositories/SimilarNodeNameTests.cs | 205 ++++++++--- 2 files changed, 379 insertions(+), 151 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs index 99e824757d..73836ff69a 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs @@ -1,118 +1,249 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; +using static Umbraco.Core.Persistence.Repositories.Implement.SimilarNodeName; namespace Umbraco.Core.Persistence.Repositories.Implement { internal class SimilarNodeName { - private int _numPos = -2; + const string SPACE_CHARACTER = " "; public int Id { get; set; } public string Name { get; set; } - // cached - reused - public int NumPos - { - get - { - if (_numPos != -2) return _numPos; - - var name = Name; - - // cater nodes with no name. - if (string.IsNullOrWhiteSpace(name)) - return _numPos; - - if (name[name.Length - 1] != ')') - return _numPos = -1; - - var pos = name.LastIndexOf('('); - if (pos < 2 || pos == name.Length - 2) // < 2 and not < 0, because we want at least "x (" - return _numPos = -1; - - return _numPos = pos; - } - } - - // not cached - used only once - public int NumVal - { - get - { - if (NumPos < 0) - throw new InvalidOperationException(); - int num; - if (int.TryParse(Name.Substring(NumPos + 1, Name.Length - 2 - NumPos), out num)) - return num; - return 0; - } - } - - // compare without allocating, nor parsing integers - internal class Comparer : IComparer - { - public int Compare(SimilarNodeName x, SimilarNodeName y) - { - if (x == null) throw new ArgumentNullException("x"); - if (y == null) throw new ArgumentNullException("y"); - - var xpos = x.NumPos; - var ypos = y.NumPos; - - var xname = x.Name; - var yname = y.Name; - - if (xpos < 0 || ypos < 0 || xpos != ypos) - return string.Compare(xname, yname, StringComparison.Ordinal); - - // compare the part before (number) - var n = string.Compare(xname, 0, yname, 0, xpos, StringComparison.Ordinal); - if (n != 0) - return n; - - // compare (number) lengths - var diff = xname.Length - yname.Length; - if (diff != 0) return diff < 0 ? -1 : +1; - - // actually compare (number) - var i = xpos; - while (i < xname.Length - 1) - { - if (xname[i] != yname[i]) - return xname[i] < yname[i] ? -1 : +1; - i++; - } - return 0; - } - } - // gets a unique name public static string GetUniqueName(IEnumerable names, int nodeId, string nodeName) { - var uniqueNumber = 1; - var uniqueing = false; - foreach (var name in names.OrderBy(x => x, new Comparer())) - { - // ignore self - if (nodeId != 0 && name.Id == nodeId) continue; + var cleanName = string.IsNullOrWhiteSpace(nodeName) ? SPACE_CHARACTER : nodeName; - if (uniqueing) + var items = names + .Where(x => x.Id != nodeId) // ignore same node + .Select(x => x.Name); + + var uniqueName = GetUniqueName(items, cleanName); + + return uniqueName; + } + + + public static string GetUniqueName(IEnumerable names, string name) + { + var model = new StructuredName(name); + var items = names + .Where(x => x.InvariantStartsWith(model.Text)) // ignore non-matching names + .Select(x => new StructuredName(x)); + + // name is empty, and there are no other names with suffixes, so just return " (1)" + if (model.IsEmptyName() && !items.Any()) + { + model.SetSuffix(1); + + return model.FullName; + } + + // name is empty, and there are other names with suffixes + if (model.IsEmptyName() && items.SuffixedNameExists()) + { + var emptyNameSuffix = GetSuffixNumber(items); + + if (emptyNameSuffix > 0) { - if (name.NumPos > 0 && name.Name.InvariantStartsWith(nodeName) && name.NumVal == uniqueNumber) - uniqueNumber++; - else - break; - } - else if (name.Name.InvariantEquals(nodeName)) - { - uniqueing = true; + model.SetSuffix(emptyNameSuffix); + + return model.FullName; } } - return uniqueing || string.IsNullOrWhiteSpace(nodeName) - ? string.Concat(nodeName, " (", uniqueNumber.ToString(), ")") - : nodeName; + // no suffix - name without suffix does NOT exist, AND name with suffix does NOT exist + if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + { + model.SetNoSuffix(); + + return model.FullName; + } + + // no suffix - name without suffix exists, however name with suffix does NOT exist + if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + { + var firstSuffix = GetFirstSuffix(items); + model.SetSuffix(firstSuffix); + + return model.FullName; + } + + // no suffix - name without suffix exists, AND name with suffix does exist + if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + { + var nextSuffix = GetSuffixNumber(items); + model.SetSuffix(nextSuffix); + + return model.FullName; + } + + // no suffix - name without suffix does NOT exist, however name with suffix exists + if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + { + var nextSuffix = GetSuffixNumber(items); + model.SetSuffix(nextSuffix); + + return model.FullName; + } + + // has suffix - name without suffix exists + if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + { + var nextSuffix = GetSuffixNumber(items); + model.SetSuffix(nextSuffix); + + return model.FullName; + } + + // has suffix - name without suffix does NOT exist + // a case where the user added the suffix, so add a secondary suffix + if (model.HasSuffix() && !items.SimpleNameExists(model.Text)) + { + model.SetText(model.FullName); + model.SetNoSuffix(); + + // filter items based on full name with suffix + items = items.Where(x => x.Text.InvariantStartsWith(model.FullName)); + var secondarySuffix = GetFirstSuffix(items); + model.SetSuffix(secondarySuffix); + + return model.FullName; + } + + // has suffix - name without suffix also exists, therefore we simply increment + if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + { + var nextSuffix = GetSuffixNumber(items); + model.SetSuffix(nextSuffix); + + return model.FullName; + } + + return name; + } + + private static int GetFirstSuffix(IEnumerable items) + { + const int suffixStart = 1; + + if (!items.Any(x => x.Number == suffixStart)) + { + // none of the suffixes are the same as suffixStart, so we can use suffixStart! + return suffixStart; + } + + return GetSuffixNumber(items); + } + + private static int GetSuffixNumber(IEnumerable items) + { + int current = 1; + foreach (var item in items.OrderBy(x => x.Number)) + { + if (item.Number == current) + { + current++; + } + else if (item.Number > current) + { + // do nothing - we found our number! + // eg. when suffixes are 1 & 3, then this method is required to generate 2 + break; + } + } + + return current; + } + + internal class StructuredName + { + private const string Suffixed_Pattern = @"(.*)\s\((\d+)\)$"; + internal string Text { get; private set; } + internal int Number { get; private set; } + public string FullName + { + get + { + string text = string.IsNullOrWhiteSpace(Text) ? Text.Trim() : Text; + + return Number > 0 ? $"{text} ({Number})" : text; + } + } + + internal StructuredName(string name) + { + if (string.IsNullOrWhiteSpace(name)) + { + Text = SPACE_CHARACTER; + Number = 0; + + return; + } + + var rg = new Regex(Suffixed_Pattern); + var matches = rg.Matches(name); + if (matches.Count > 0) + { + var match = matches[0]; + Text = match.Groups[1].Value; + int number; + Number = int.TryParse(match.Groups[2].Value, out number) ? number : 0; + + return; + } + else + { + Text = name; + Number = 0; + } + } + + internal bool HasSuffix() + { + return Number > 0; + } + + internal void SetSuffix(int number) + { + Number = number; + } + + internal void SetNoSuffix() + { + Number = 0; + } + + internal void SetText(string text) + { + Text = text; + } + + internal bool IsEmptyName() + { + return string.IsNullOrWhiteSpace(Text); + } + } + } + + internal static class ListExtensions + { + internal static bool Contains(this IEnumerable items, StructuredName model) + { + return items.Any(x => x.FullName.InvariantEquals(model.FullName)); + } + + internal static bool SimpleNameExists(this IEnumerable items, string name) + { + return items.Any(x => x.FullName.InvariantEquals(name)); + } + + internal static bool SuffixedNameExists(this IEnumerable items) + { + return items.Any(x => x.HasSuffix()); } } } diff --git a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs index b60709c8bf..14746b919c 100644 --- a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs +++ b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs @@ -7,72 +7,169 @@ namespace Umbraco.Tests.Integration.Persistence.Repositories [TestFixture] public class SimilarNodeNameTests { - [TestCase("Alpha", "Alpha", 0)] - [TestCase("Alpha", "ALPHA", +1)] // case is important - [TestCase("Alpha", "Bravo", -1)] - [TestCase("Bravo", "Alpha", +1)] - [TestCase("Alpha (1)", "Alpha (1)", 0)] - [TestCase("Alpha", "Alpha (1)", -1)] - [TestCase("Alpha (1)", "Alpha", +1)] - [TestCase("Alpha (1)", "Alpha (2)", -1)] - [TestCase("Alpha (2)", "Alpha (1)", +1)] - [TestCase("Alpha (2)", "Alpha (10)", -1)] // this is the real stuff - [TestCase("Alpha (10)", "Alpha (2)", +1)] // this is the real stuff - [TestCase("Kilo", "Golf (2)", +1)] - [TestCase("Kilo (1)", "Golf (2)", +1)] - [TestCase("", "", 0)] - [TestCase(null, null, 0)] - public void ComparerTest(string name1, string name2, int expected) - { - var comparer = new SimilarNodeName.Comparer(); - - var result = comparer.Compare(new SimilarNodeName { Name = name1 }, new SimilarNodeName { Name = name2 }); - if (expected == 0) - Assert.AreEqual(0, result); - else if (expected < 0) - Assert.IsTrue(result < 0, "Expected <0 but was " + result); - else if (expected > 0) - Assert.IsTrue(result > 0, "Expected >0 but was " + result); - } - - [Test] - public void OrderByTest() + public void Name_Is_Suffixed() { var names = new[] { - new SimilarNodeName { Id = 1, Name = "Alpha (2)" }, - new SimilarNodeName { Id = 2, Name = "Alpha" }, - new SimilarNodeName { Id = 3, Name = "Golf" }, - new SimilarNodeName { Id = 4, Name = "Zulu" }, - new SimilarNodeName { Id = 5, Name = "Mike" }, - new SimilarNodeName { Id = 6, Name = "Kilo (1)" }, - new SimilarNodeName { Id = 7, Name = "Yankee" }, - new SimilarNodeName { Id = 8, Name = "Kilo" }, - new SimilarNodeName { Id = 9, Name = "Golf (2)" }, - new SimilarNodeName { Id = 10, Name = "Alpha (1)" }, + new SimilarNodeName { Id = 1, Name = "Zulu" } }; - var ordered = names.OrderBy(x => x, new SimilarNodeName.Comparer()).ToArray(); - - var i = 0; - Assert.AreEqual(2, ordered[i++].Id); - Assert.AreEqual(10, ordered[i++].Id); - Assert.AreEqual(1, ordered[i++].Id); - Assert.AreEqual(3, ordered[i++].Id); - Assert.AreEqual(9, ordered[i++].Id); - Assert.AreEqual(8, ordered[i++].Id); - Assert.AreEqual(6, ordered[i++].Id); - Assert.AreEqual(5, ordered[i++].Id); - Assert.AreEqual(7, ordered[i++].Id); - Assert.AreEqual(4, ordered[i++].Id); + var res = SimilarNodeName.GetUniqueName(names, 0, "Zulu"); + Assert.AreEqual("Zulu (1)", res); } + [Test] + public void Suffixed_Name_Is_Incremented() + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Zulu" }, + new SimilarNodeName { Id = 2, Name = "Kilo (1)" }, + new SimilarNodeName { Id = 3, Name = "Kilo" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Kilo (1)"); + Assert.AreEqual("Kilo (2)", res); + } + + [Test] + public void Lower_Number_Suffix_Is_Inserted() + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Golf" }, + new SimilarNodeName { Id = 2, Name = "Golf (2)" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Golf"); + Assert.AreEqual("Golf (1)", res); + } + + [Test] + [TestCase(0, "Alpha", "Alpha (3)")] + [TestCase(0, "alpha", "alpha (3)")] + public void Case_Is_Ignored(int nodeId, string nodeName, string expected) + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Alpha" }, + new SimilarNodeName { Id = 2, Name = "Alpha (1)" }, + new SimilarNodeName { Id = 3, Name = "Alpha (2)" }, + }; + var res = SimilarNodeName.GetUniqueName(names, nodeId, nodeName); + + Assert.AreEqual(expected, res); + } + + [Test] + public void Empty_List_Causes_Unchanged_Name() + { + var names = new SimilarNodeName[] { }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Charlie"); + + Assert.AreEqual("Charlie", res); + } + + [Test] + [TestCase(0, "", " (1)")] + [TestCase(0, null, " (1)")] + public void Empty_Name_Is_Suffixed(int nodeId, string nodeName, string expected) + { + var names = new SimilarNodeName[] { }; + + var res = SimilarNodeName.GetUniqueName(names, nodeId, nodeName); + + Assert.AreEqual(expected, res); + } + + [Test] + public void Matching_NoedId_Causes_No_Change() + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Kilo (1)" }, + new SimilarNodeName { Id = 2, Name = "Yankee" }, + new SimilarNodeName { Id = 3, Name = "Kilo" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 1, "Kilo (1)"); + + Assert.AreEqual("Kilo (1)", res); + } + + [Test] + public void Extra_MultiSuffixed_Name_Is_Ignored() + { + // Sequesnce is: Test, Test (1), Test (2) + // Ignore: Test (1) (1) + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Alpha (2)" }, + new SimilarNodeName { Id = 2, Name = "Test" }, + new SimilarNodeName { Id = 3, Name = "Test (1)" }, + new SimilarNodeName { Id = 4, Name = "Test (2)" }, + new SimilarNodeName { Id = 5, Name = "Test (1) (1)" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Test"); + + Assert.AreEqual("Test (3)", res); + } + + [Test] + public void Matched_Name_Is_Suffixed() + { + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Test" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Test"); + + Assert.AreEqual("Test (1)", res); + } + + [Test] + public void MultiSuffixed_Name_Is_Icremented() + { + // "Test (1)" is treated as the "original" version of the name. + // "Test (1) (1)" is the suffixed result of a copy, and therefore is incremented + // Hence this test result should be the same as Suffixed_Name_Is_Incremented + var names = new[] + { + new SimilarNodeName { Id = 1, Name = "Test (1)" }, + new SimilarNodeName { Id = 2, Name = "Test (1) (1)" }, + }; + + var res = SimilarNodeName.GetUniqueName(names, 0, "Test (1) (1)"); + + Assert.AreEqual("Test (1) (2)", res); + } + + [Test] + public void Suffixed_Name_Causes_Secondary_Suffix() + { + var names = new[] + { + new SimilarNodeName { Id = 6, Name = "Alpha (1)" } + }; + var res = SimilarNodeName.GetUniqueName(names, 0, "Alpha (1)"); + + Assert.AreEqual("Alpha (1) (1)", res); + } + + + + /* Original Tests - Can be deleted, as new tests cover all cases */ + [TestCase(0, "Charlie", "Charlie")] [TestCase(0, "Zulu", "Zulu (1)")] [TestCase(0, "Golf", "Golf (1)")] [TestCase(0, "Kilo", "Kilo (2)")] [TestCase(0, "Alpha", "Alpha (3)")] - [TestCase(0, "Kilo (1)", "Kilo (1) (1)")] // though... we might consider "Kilo (2)" + //[TestCase(0, "Kilo (1)", "Kilo (1) (1)")] // though... we might consider "Kilo (2)" + [TestCase(0, "Kilo (1)", "Kilo (2)")] // names[] contains "Kilo" AND "Kilo (1)", which implies that result should be "Kilo (2)" [TestCase(6, "Kilo (1)", "Kilo (1)")] // because of the id [TestCase(0, "alpha", "alpha (3)")] [TestCase(0, "", " (1)")] From 759eee042fd56f431487e07563e8860f92acc7ab Mon Sep 17 00:00:00 2001 From: Anthony Date: Wed, 21 Oct 2020 20:57:21 +0100 Subject: [PATCH 3/3] Refactored according to PR feedback. --- .../Repositories/Implement/SimilarNodeName.cs | 98 +++++++------------ .../Repositories/SimilarNodeNameTests.cs | 15 +++ 2 files changed, 52 insertions(+), 61 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs index 73836ff69a..b613ea84aa 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs @@ -7,26 +7,20 @@ namespace Umbraco.Core.Persistence.Repositories.Implement { internal class SimilarNodeName { - const string SPACE_CHARACTER = " "; - public int Id { get; set; } public string Name { get; set; } - // gets a unique name public static string GetUniqueName(IEnumerable names, int nodeId, string nodeName) { - var cleanName = string.IsNullOrWhiteSpace(nodeName) ? SPACE_CHARACTER : nodeName; - var items = names .Where(x => x.Id != nodeId) // ignore same node .Select(x => x.Name); - var uniqueName = GetUniqueName(items, cleanName); + var uniqueName = GetUniqueName(items, nodeName); return uniqueName; } - public static string GetUniqueName(IEnumerable names, string name) { var model = new StructuredName(name); @@ -37,7 +31,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // name is empty, and there are no other names with suffixes, so just return " (1)" if (model.IsEmptyName() && !items.Any()) { - model.SetSuffix(1); + model.Suffix = StructuredName.INITIAL_SUFFIX; return model.FullName; } @@ -49,76 +43,76 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (emptyNameSuffix > 0) { - model.SetSuffix(emptyNameSuffix); + model.Suffix = (uint?)emptyNameSuffix; return model.FullName; } } // no suffix - name without suffix does NOT exist, AND name with suffix does NOT exist - if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + if (!model.Suffix.HasValue && !items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) { - model.SetNoSuffix(); + model.Suffix = StructuredName.NO_SUFFIX; return model.FullName; } // no suffix - name without suffix exists, however name with suffix does NOT exist - if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + if (!model.Suffix.HasValue && items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) { var firstSuffix = GetFirstSuffix(items); - model.SetSuffix(firstSuffix); + model.Suffix = (uint?)firstSuffix; return model.FullName; } // no suffix - name without suffix exists, AND name with suffix does exist - if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + if (!model.Suffix.HasValue && items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // no suffix - name without suffix does NOT exist, however name with suffix exists - if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + if (!model.Suffix.HasValue && !items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // has suffix - name without suffix exists - if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && items.SimpleNameExists(model.Text)) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // has suffix - name without suffix does NOT exist // a case where the user added the suffix, so add a secondary suffix - if (model.HasSuffix() && !items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && !items.SimpleNameExists(model.Text)) { - model.SetText(model.FullName); - model.SetNoSuffix(); + model.Text = model.FullName; + model.Suffix = StructuredName.NO_SUFFIX; // filter items based on full name with suffix items = items.Where(x => x.Text.InvariantStartsWith(model.FullName)); var secondarySuffix = GetFirstSuffix(items); - model.SetSuffix(secondarySuffix); + model.Suffix = (uint?)secondarySuffix; return model.FullName; } // has suffix - name without suffix also exists, therefore we simply increment - if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && items.SimpleNameExists(model.Text)) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } @@ -130,7 +124,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement { const int suffixStart = 1; - if (!items.Any(x => x.Number == suffixStart)) + if (!items.Any(x => x.Suffix == suffixStart)) { // none of the suffixes are the same as suffixStart, so we can use suffixStart! return suffixStart; @@ -142,13 +136,13 @@ namespace Umbraco.Core.Persistence.Repositories.Implement private static int GetSuffixNumber(IEnumerable items) { int current = 1; - foreach (var item in items.OrderBy(x => x.Number)) + foreach (var item in items.OrderBy(x => x.Suffix)) { - if (item.Number == current) + if (item.Suffix == current) { current++; } - else if (item.Number > current) + else if (item.Suffix > current) { // do nothing - we found our number! // eg. when suffixes are 1 & 3, then this method is required to generate 2 @@ -161,16 +155,20 @@ namespace Umbraco.Core.Persistence.Repositories.Implement internal class StructuredName { - private const string Suffixed_Pattern = @"(.*)\s\((\d+)\)$"; - internal string Text { get; private set; } - internal int Number { get; private set; } + const string SPACE_CHARACTER = " "; + const string SUFFIXED_PATTERN = @"(.*) \(([1-9]\d*)\)$"; + internal const uint INITIAL_SUFFIX = 1; + internal static readonly uint? NO_SUFFIX = default; + + internal string Text { get; set; } + internal uint ? Suffix { get; set; } public string FullName { get { - string text = string.IsNullOrWhiteSpace(Text) ? Text.Trim() : Text; + string text = (Text == SPACE_CHARACTER) ? Text.Trim() : Text; - return Number > 0 ? $"{text} ({Number})" : text; + return Suffix > 0 ? $"{text} ({Suffix})" : text; } } @@ -179,48 +177,26 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (string.IsNullOrWhiteSpace(name)) { Text = SPACE_CHARACTER; - Number = 0; return; } - var rg = new Regex(Suffixed_Pattern); + var rg = new Regex(SUFFIXED_PATTERN); var matches = rg.Matches(name); if (matches.Count > 0) { var match = matches[0]; Text = match.Groups[1].Value; - int number; - Number = int.TryParse(match.Groups[2].Value, out number) ? number : 0; + int number = int.TryParse(match.Groups[2].Value, out number) ? number : 0; + Suffix = (uint?)(number); return; } else { Text = name; - Number = 0; } - } - - internal bool HasSuffix() - { - return Number > 0; - } - - internal void SetSuffix(int number) - { - Number = number; - } - - internal void SetNoSuffix() - { - Number = 0; - } - - internal void SetText(string text) - { - Text = text; - } + } internal bool IsEmptyName() { @@ -243,7 +219,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement internal static bool SuffixedNameExists(this IEnumerable items) { - return items.Any(x => x.HasSuffix()); + return items.Any(x => x.Suffix.HasValue); } } } diff --git a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs index 14746b919c..688baad540 100644 --- a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs +++ b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs @@ -160,6 +160,21 @@ namespace Umbraco.Tests.Integration.Persistence.Repositories } + [TestCase("Test (0)", "Test (0) (1)")] + [TestCase("Test (-1)", "Test (-1) (1)")] + [TestCase("Test (1) (-1)", "Test (1) (-1) (1)")] + public void NonPositive_Suffix_Is_Ignored(string suffix, string expected) + { + var names = new[] + { + new SimilarNodeName { Id = 6, Name = suffix } + }; + var res = SimilarNodeName.GetUniqueName(names, 0, suffix); + + Assert.AreEqual(expected, res); + } + + /* Original Tests - Can be deleted, as new tests cover all cases */