From 9538981730d71698e90a00aba92fea700b587eef Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 1 Aug 2019 18:49:05 +1000 Subject: [PATCH] Adds more assertions and tests and validates variations --- src/Umbraco.Core/Models/IContentTypeBase.cs | 2 +- .../Implement/ContentTypeRepositoryBase.cs | 68 ++-- .../Implement/DocumentRepository.cs | 10 +- .../ContentTypeServiceVariantsTests.cs | 290 ++++++------------ 4 files changed, 133 insertions(+), 237 deletions(-) diff --git a/src/Umbraco.Core/Models/IContentTypeBase.cs b/src/Umbraco.Core/Models/IContentTypeBase.cs index 5f1fe6ed49..ed87c5f320 100644 --- a/src/Umbraco.Core/Models/IContentTypeBase.cs +++ b/src/Umbraco.Core/Models/IContentTypeBase.cs @@ -101,7 +101,7 @@ namespace Umbraco.Core.Models PropertyGroupCollection PropertyGroups { get; set; } /// - /// Gets all local property types belonging to a group, across all local property groups. + /// Gets all local property types all local property groups or ungrouped. /// IEnumerable PropertyTypes { get; } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index b29565a35e..8f88c71bf0 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -100,6 +100,8 @@ namespace Umbraco.Core.Persistence.Repositories.Implement protected void PersistNewBaseContentType(IContentTypeComposition entity) { + ValidateVariations(entity); + var dto = ContentTypeFactory.BuildContentTypeDto(entity); //Cannot add a duplicate content type @@ -165,11 +167,11 @@ AND umbracoNode.nodeObjectType = @objectType", foreach (var allowedContentType in entity.AllowedContentTypes) { Database.Insert(new ContentTypeAllowedContentTypeDto - { - Id = entity.Id, - AllowedId = allowedContentType.Id.Value, - SortOrder = allowedContentType.SortOrder - }); + { + Id = entity.Id, + AllowedId = allowedContentType.Id.Value, + SortOrder = allowedContentType.SortOrder + }); } @@ -216,6 +218,8 @@ AND umbracoNode.nodeObjectType = @objectType", protected void PersistUpdatedBaseContentType(IContentTypeComposition entity) { + ValidateVariations(entity); + var dto = ContentTypeFactory.BuildContentTypeDto(entity); // ensure the alias is not used already @@ -372,7 +376,7 @@ AND umbracoNode.id <> @id", foreach (var propertyGroup in entity.PropertyGroups) { // insert or update group - var groupDto = PropertyGroupFactory.BuildGroupDto(propertyGroup,entity.Id); + var groupDto = PropertyGroupFactory.BuildGroupDto(propertyGroup, entity.Id); var groupId = propertyGroup.HasIdentity ? Database.Update(groupDto) : Convert.ToInt32(Database.Insert(groupDto)); @@ -390,7 +394,7 @@ AND umbracoNode.id <> @id", //check if the content type variation has been changed var contentTypeVariationDirty = entity.IsPropertyDirty("Variations"); - var oldContentTypeVariation = (ContentVariation) dtoPk.Variations; + var oldContentTypeVariation = (ContentVariation)dtoPk.Variations; var newContentTypeVariation = entity.Variations; var contentTypeVariationChanging = contentTypeVariationDirty && oldContentTypeVariation != newContentTypeVariation; if (contentTypeVariationChanging) @@ -451,7 +455,7 @@ AND umbracoNode.id <> @id", // via composition, with their original variations (ie not filtered by this // content type variations - we need this true value to make decisions. - foreach (var propertyType in ((ContentTypeCompositionBase) entity).RawComposedPropertyTypes) + foreach (var propertyType in ((ContentTypeCompositionBase)entity).RawComposedPropertyTypes) { if (propertyType.VariesBySegment() || newContentTypeVariation.VariesBySegment()) throw new NotSupportedException(); // TODO: support this @@ -520,6 +524,19 @@ AND umbracoNode.id <> @id", CommonRepository.ClearCache(); // always } + /// + /// Ensures that no property types are flagged for a variance that is not supported by the content type itself + /// + /// + private void ValidateVariations(IContentTypeComposition entity) + { + //if the entity does not vary at all, then the property cannot have a variance value greater than it + if (entity.Variations == ContentVariation.Nothing) + foreach (var prop in entity.PropertyTypes) + if (prop.Variations > entity.Variations) + throw new InvalidOperationException($"The property {prop.Alias} cannot have variations of {prop.Variations} with the content type variations of {entity.Variations}"); + } + private IEnumerable GetImpactedContentTypes(IContentTypeComposition contentType, IEnumerable all) { var impact = new List(); @@ -527,12 +544,12 @@ AND umbracoNode.id <> @id", var tree = new Dictionary>(); foreach (var x in all) - foreach (var y in x.ContentTypeComposition) - { - if (!tree.TryGetValue(y.Id, out var list)) - list = tree[y.Id] = new List(); - list.Add(x); - } + foreach (var y in x.ContentTypeComposition) + { + if (!tree.TryGetValue(y.Id, out var list)) + list = tree[y.Id] = new List(); + list.Add(x); + } var nset = new List(); do @@ -574,7 +591,7 @@ AND umbracoNode.id <> @id", // new property type, ignore if (!oldVariations.TryGetValue(propertyType.Id, out var oldVariationB)) continue; - var oldVariation = (ContentVariation) oldVariationB; // NPoco cannot fetch directly + var oldVariation = (ContentVariation)oldVariationB; // NPoco cannot fetch directly // only those property types that *actually* changed var newVariation = propertyType.Variations; @@ -638,7 +655,7 @@ AND umbracoNode.id <> @id", var impactedL = impacted.Select(x => x.Id).ToList(); //Group by the "To" variation so we can bulk update in the correct batches - foreach(var grouping in propertyTypeChanges.GroupBy(x => x.Value.ToVariation)) + foreach (var grouping in propertyTypeChanges.GroupBy(x => x.Value.ToVariation)) { var propertyTypeIds = grouping.Select(x => x.Key).ToList(); var toVariation = grouping.Key; @@ -1037,8 +1054,8 @@ AND umbracoNode.id <> @id", { var defaultLang = LanguageRepository.GetDefaultId(); - //This will build up a query to get the property values of both the current and the published version so that we can check - //based on the current variance of each item to see if it's 'edited' value should be true/false. + //This will build up a query to get the property values of both the current and the published version so that we can check + //based on the current variance of each item to see if it's 'edited' value should be true/false. var whereInArgsCount = propertyTypeIds.Count + (contentTypeIds?.Count ?? 0); if (whereInArgsCount > 2000) @@ -1077,7 +1094,7 @@ AND umbracoNode.id <> @id", var editedDocument = new Dictionary(); var nodeId = -1; var propertyTypeId = -1; - + PropertyValueVersionDto pubRow = null; //This is a reader (Query), we are not fetching this all into memory so we cannot make any changes during this iteration, we are just collecting data. @@ -1087,7 +1104,7 @@ AND umbracoNode.id <> @id", { //make sure to reset on each node/property change if (nodeId != row.NodeId || propertyTypeId != row.PropertyTypeId) - { + { nodeId = row.NodeId; propertyTypeId = row.PropertyTypeId; pubRow = null; @@ -1114,7 +1131,7 @@ AND umbracoNode.id <> @id", else if (pubRow == null) { //this would mean that that this property is 'edited' since there is no published version - editedLanguageVersions.Add((row.NodeId, row.LanguageId), true); + editedLanguageVersions[(row.NodeId, row.LanguageId)] = true; editedDocument[row.NodeId] = true; } //compare the property values, if they differ from versions then flag the current version as edited @@ -1181,10 +1198,9 @@ AND umbracoNode.id <> @id", toUpdate.Add(docVariations); } } - else + else if (ev.Key.langId.HasValue) { - //the row doesn't exist but needs creating - //TODO: Does this ever happen?? Need to see if we can test this + //This should never happen! If a property culture is flagged as edited then the culture must exist at the document level throw new PanicException($"The existing DocumentCultureVariationDto was not found for node {ev.Key.nodeId} and language {ev.Key.langId}"); } } @@ -1197,7 +1213,7 @@ AND umbracoNode.id <> @id", } //Now bulk update the umbracoDocument table - foreach(var editValue in editedDocument.GroupBy(x => x.Value)) + foreach (var editValue in editedDocument.GroupBy(x => x.Value)) { Database.Execute(Sql().Update(u => u.Set(x => x.Edited, editValue.Key)) .WhereIn(x => x.NodeId, editValue.Select(x => x.Key))); @@ -1226,7 +1242,7 @@ AND umbracoNode.id <> @id", } private class PropertyValueVersionDto - { + { public int VersionId { get; set; } public int PropertyTypeId { get; set; } public int? LanguageId { get; set; } diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index c18921b59e..344557d815 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -1310,14 +1310,12 @@ namespace Umbraco.Core.Persistence.Repositories.Implement Name = content.GetCultureName(culture) ?? content.GetPublishName(culture), Available = content.IsCultureAvailable(culture), - Published = content.IsCulturePublished(culture) + Published = content.IsCulturePublished(culture), + // note: can't use IsCultureEdited at that point - hasn't been updated yet - see PersistUpdatedItem + Edited = content.IsCultureAvailable(culture) && + (!content.IsCulturePublished(culture) || (editedCultures != null && editedCultures.Contains(culture))) }; - // note: can't use IsCultureEdited at that point - hasn't been updated yet - see PersistUpdatedItem - - dto.Edited = content.IsCultureAvailable(culture) && - (!content.IsCulturePublished(culture) || (editedCultures != null && editedCultures.Contains(culture))); - yield return dto; } diff --git a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs index 0c54cf5975..4b8262d4a5 100644 --- a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs +++ b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs @@ -110,7 +110,6 @@ namespace Umbraco.Tests.Services [Test] public void Change_Content_Type_Variation_Clears_Redirects() { - //create content type with a property type that varies by culture var contentType = MockedContentTypes.CreateBasicContentType(); contentType.Variations = ContentVariation.Nothing; var contentCollection = new PropertyTypeCollection(true); @@ -154,8 +153,7 @@ namespace Umbraco.Tests.Services [Test] public void Change_Content_Type_From_Invariant_Variant() - { - //create content type with a property type that varies by culture + { var contentType = MockedContentTypes.CreateBasicContentType(); contentType.Variations = ContentVariation.Nothing; var contentCollection = new PropertyTypeCollection(true); @@ -205,7 +203,6 @@ namespace Umbraco.Tests.Services [Test] public void Change_Content_Type_From_Variant_Invariant() { - //create content type with a property type that varies by culture var contentType = MockedContentTypes.CreateBasicContentType(); contentType.Variations = ContentVariation.Culture; var contentCollection = new PropertyTypeCollection(true); @@ -264,39 +261,49 @@ namespace Umbraco.Tests.Services } [Test] - public void Change_Property_Type_From_Invariant_Variant() + public void Change_Property_Type_From_To_Variant_On_Invariant_Content_Type() { - //create content type with a property type that varies by culture var contentType = MockedContentTypes.CreateBasicContentType(); contentType.Variations = ContentVariation.Nothing; - var contentCollection = new PropertyTypeCollection(true); - contentCollection.Add(new PropertyType("test", ValueStorageType.Ntext) - { - Alias = "title", - Name = "Title", - Description = "", - Mandatory = false, - SortOrder = 1, - DataTypeId = -88, - Variations = ContentVariation.Nothing - }); - contentType.PropertyGroups.Add(new PropertyGroup(contentCollection) { Name = "Content", SortOrder = 1 }); + var properties = CreatePropertyCollection(("title", ContentVariation.Nothing)); + contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); + ServiceContext.ContentTypeService.Save(contentType); + + //change the property type to be variant + contentType.PropertyTypes.First().Variations = ContentVariation.Culture; + + //Cannot change a property type to be variant if the content type itself is not variant + Assert.Throws(() => ServiceContext.ContentTypeService.Save(contentType)); + } + + [Test] + public void Change_Property_Type_From_Invariant_Variant() + { + var contentType = MockedContentTypes.CreateBasicContentType(); + contentType.Variations = ContentVariation.Culture; + var properties = CreatePropertyCollection(("title", ContentVariation.Nothing)); + contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); ServiceContext.ContentTypeService.Save(contentType); //create some content of this content type IContent doc = MockedContent.CreateBasicContent(contentType); - doc.Name = "Home"; + doc.SetCultureName("Home", "en-US"); doc.SetValue("title", "hello world"); ServiceContext.ContentService.Save(doc); + doc = ServiceContext.ContentService.GetById(doc.Id); //re-get Assert.AreEqual("hello world", doc.GetValue("title")); - + Assert.IsTrue(doc.IsCultureEdited("en-US")); //invariant prop changes show up on default lang + Assert.IsTrue(doc.Edited); + //change the property type to be variant contentType.PropertyTypes.First().Variations = ContentVariation.Culture; ServiceContext.ContentTypeService.Save(contentType); doc = ServiceContext.ContentService.GetById(doc.Id); //re-get Assert.AreEqual("hello world", doc.GetValue("title", "en-US")); + Assert.IsTrue(doc.IsCultureEdited("en-US")); + Assert.IsTrue(doc.Edited); //change back property type to be invariant contentType.PropertyTypes.First().Variations = ContentVariation.Nothing; @@ -304,6 +311,8 @@ namespace Umbraco.Tests.Services doc = ServiceContext.ContentService.GetById(doc.Id); //re-get Assert.AreEqual("hello world", doc.GetValue("title")); + Assert.IsTrue(doc.IsCultureEdited("en-US")); //invariant prop changes show up on default lang + Assert.IsTrue(doc.Edited); } [Test] @@ -470,28 +479,11 @@ namespace Umbraco.Tests.Services CreateFrenchAndEnglishLangs(); - var contentType = new ContentType(-1) - { - Alias = "contentType", - Name = "contentType", - Variations = ContentVariation.Culture - }; + var contentType = CreateContentType(ContentVariation.Culture); - var properties = new PropertyTypeCollection(true) - { - new PropertyType("value1", ValueStorageType.Ntext) - { - Alias = "value1", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value2", ValueStorageType.Ntext) - { - Alias = "value2", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties = CreatePropertyCollection( + ("value1", ContentVariation.Culture), + ("value2", ContentVariation.Nothing)); contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); ServiceContext.ContentTypeService.Save(contentType); @@ -579,28 +571,11 @@ namespace Umbraco.Tests.Services var languageFr = new Language("fr"); ServiceContext.LocalizationService.Save(languageFr); - var contentType = new ContentType(-1) - { - Alias = "contentType", - Name = "contentType", - Variations = ContentVariation.Nothing - }; + var contentType = CreateContentType(ContentVariation.Nothing); - var properties = new PropertyTypeCollection(true) - { - new PropertyType("value1", ValueStorageType.Ntext) - { - Alias = "value1", - DataTypeId = -88, - Variations = ContentVariation.Nothing - }, - new PropertyType("value2", ValueStorageType.Ntext) - { - Alias = "value2", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties = CreatePropertyCollection( + ("value1", ContentVariation.Nothing), + ("value2", ContentVariation.Nothing)); contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); ServiceContext.ContentTypeService.Save(contentType); @@ -681,28 +656,11 @@ namespace Umbraco.Tests.Services CreateFrenchAndEnglishLangs(); - var contentType = new ContentType(-1) - { - Alias = "contentType", - Name = "contentType", - Variations = ContentVariation.Culture - }; + var contentType = CreateContentType(ContentVariation.Culture); - var properties = new PropertyTypeCollection(true) - { - new PropertyType("value1", ValueStorageType.Ntext) - { - Alias = "value1", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value2", ValueStorageType.Ntext) - { - Alias = "value2", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties = CreatePropertyCollection( + ("value1", ContentVariation.Culture), + ("value2", ContentVariation.Nothing)); contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); ServiceContext.ContentTypeService.Save(contentType); @@ -780,29 +738,16 @@ namespace Umbraco.Tests.Services } [Test] - public void Change_Variations_SimpleContentType_VariantPropertyToInvariantAndBack_While_Publishing() + public void Change_Property_Variations_From_Variant_To_Invariant_And_Ensure_Edited_Values_Are_Renormalized() { // one simple content type, variant, with both variant and invariant properties // can change an invariant property to variant and back CreateFrenchAndEnglishLangs(); - var contentType = new ContentType(-1) - { - Alias = "contentType", - Name = "contentType", - Variations = ContentVariation.Culture - }; + var contentType = CreateContentType(ContentVariation.Culture); - var properties = new PropertyTypeCollection(true) - { - new PropertyType("value1", ValueStorageType.Ntext) - { - Alias = "value1", - DataTypeId = -88, - Variations = ContentVariation.Culture - } - }; + var properties = CreatePropertyCollection(("value1", ContentVariation.Culture)); contentType.PropertyGroups.Add(new PropertyGroup(properties) { Name = "Content" }); ServiceContext.ContentTypeService.Save(contentType); @@ -902,54 +847,20 @@ namespace Umbraco.Tests.Services CreateFrenchAndEnglishLangs(); - var composing = new ContentType(-1) - { - Alias = "composing", - Name = "composing", - Variations = ContentVariation.Culture - }; + var composing = CreateContentType(ContentVariation.Culture, "composing"); - var properties1 = new PropertyTypeCollection(true) - { - new PropertyType("value11", ValueStorageType.Ntext) - { - Alias = "value11", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value12", ValueStorageType.Ntext) - { - Alias = "value12", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties1 = CreatePropertyCollection( + ("value11", ContentVariation.Culture), + ("value12", ContentVariation.Nothing)); composing.PropertyGroups.Add(new PropertyGroup(properties1) { Name = "Content" }); ServiceContext.ContentTypeService.Save(composing); - var composed = new ContentType(-1) - { - Alias = "composed", - Name = "composed", - Variations = ContentVariation.Culture - }; + var composed = CreateContentType(ContentVariation.Culture, "composed"); - var properties2 = new PropertyTypeCollection(true) - { - new PropertyType("value21", ValueStorageType.Ntext) - { - Alias = "value21", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value22", ValueStorageType.Ntext) - { - Alias = "value22", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties2 = CreatePropertyCollection( + ("value21", ContentVariation.Culture), + ("value22", ContentVariation.Nothing)); composed.PropertyGroups.Add(new PropertyGroup(properties2) { Name = "Content" }); composed.AddContentType(composing); @@ -1031,81 +942,30 @@ namespace Umbraco.Tests.Services CreateFrenchAndEnglishLangs(); - var composing = new ContentType(-1) - { - Alias = "composing", - Name = "composing", - Variations = ContentVariation.Culture - }; + var composing = CreateContentType(ContentVariation.Culture, "composing"); - var properties1 = new PropertyTypeCollection(true) - { - new PropertyType("value11", ValueStorageType.Ntext) - { - Alias = "value11", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value12", ValueStorageType.Ntext) - { - Alias = "value12", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties1 = CreatePropertyCollection( + ("value11", ContentVariation.Culture), + ("value12", ContentVariation.Nothing)); composing.PropertyGroups.Add(new PropertyGroup(properties1) { Name = "Content" }); ServiceContext.ContentTypeService.Save(composing); - var composed1 = new ContentType(-1) - { - Alias = "composed1", - Name = "composed1", - Variations = ContentVariation.Culture - }; + var composed1 = CreateContentType(ContentVariation.Culture, "composed1"); - var properties2 = new PropertyTypeCollection(true) - { - new PropertyType("value21", ValueStorageType.Ntext) - { - Alias = "value21", - DataTypeId = -88, - Variations = ContentVariation.Culture - }, - new PropertyType("value22", ValueStorageType.Ntext) - { - Alias = "value22", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties2 = CreatePropertyCollection( + ("value21", ContentVariation.Culture), + ("value22", ContentVariation.Nothing)); composed1.PropertyGroups.Add(new PropertyGroup(properties2) { Name = "Content" }); composed1.AddContentType(composing); ServiceContext.ContentTypeService.Save(composed1); - var composed2 = new ContentType(-1) - { - Alias = "composed2", - Name = "composed2", - Variations = ContentVariation.Nothing - }; + var composed2 = CreateContentType(ContentVariation.Nothing, "composed2"); - var properties3 = new PropertyTypeCollection(true) - { - new PropertyType("value31", ValueStorageType.Ntext) - { - Alias = "value31", - DataTypeId = -88, - Variations = ContentVariation.Nothing - }, - new PropertyType("value32", ValueStorageType.Ntext) - { - Alias = "value32", - DataTypeId = -88, - Variations = ContentVariation.Nothing - } - }; + var properties3 = CreatePropertyCollection( + ("value31", ContentVariation.Nothing), + ("value32", ContentVariation.Nothing)); composed2.PropertyGroups.Add(new PropertyGroup(properties3) { Name = "Content" }); composed2.AddContentType(composing); @@ -1219,5 +1079,27 @@ namespace Umbraco.Tests.Services var languageFr = new Language("fr"); ServiceContext.LocalizationService.Save(languageFr); } + + private IContentType CreateContentType(ContentVariation variance, string alias = "contentType") => new ContentType(-1) + { + Alias = alias, + Name = alias, + Variations = variance + }; + + private PropertyTypeCollection CreatePropertyCollection(params (string alias, ContentVariation variance)[] props) + { + var propertyCollection = new PropertyTypeCollection(true); + + foreach (var (alias, variance) in props) + propertyCollection.Add(new PropertyType(alias, ValueStorageType.Ntext) + { + Alias = alias, + DataTypeId = -88, + Variations = variance + }); + + return propertyCollection; + } } }