diff --git a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs index 4e7dc1e982..33dabe1b24 100644 --- a/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs +++ b/src/Umbraco.Core/Persistence/Factories/PropertyFactory.cs @@ -104,10 +104,9 @@ namespace Umbraco.Core.Persistence.Factories /// The properties to map /// /// out parameter indicating that one or more properties have been edited - /// out parameter containing a collection of edited cultures when the contentVariation varies by culture - /// - /// out parameter containing a collection of edited cultures that are currently persisted in the database which is used to maintain the edited state - /// of each culture value (in cases where variance is being switched) + /// + /// Out parameter containing a collection of edited cultures when the contentVariation varies by culture. + /// The value of this will be used to populate the edited cultures in the umbracoDocumentCultureVariation table. /// /// public static IEnumerable BuildDtos(ContentVariation contentVariation, int currentVersionId, int publishedVersionId, IEnumerable properties, @@ -146,14 +145,16 @@ namespace Umbraco.Core.Persistence.Factories if (propertyValue.EditedValue != null) propertyDataDtos.Add(BuildDto(currentVersionId, property, languageRepository.GetIdByIsoCode(propertyValue.Culture), propertyValue.Segment, propertyValue.EditedValue)); - //// property.Values will contain ALL of it's values, both variant and invariant which will be populated if the administrator has previously - //// changed the property type to be variant vs invariant. - //// We need to check for this scenario here because otherwise the editedCultures and edited flags - //// will end up incorrectly so here we need to only process edited cultures based on the - //// current value type and how the property varies. + // property.Values will contain ALL of it's values, both variant and invariant which will be populated if the + // administrator has previously changed the property type to be variant vs invariant. + // We need to check for this scenario here because otherwise the editedCultures and edited flags + // will end up incorrectly set in the umbracoDocumentCultureVariation table so here we need to + // only process edited cultures based on the current value type and how the property varies. + // The above logic will still persist the currently saved property value for each culture in case the admin + // decides to swap the property's variance again, in which case the edited flag will be recalculated. - //if (property.PropertyType.VariesByCulture() && isInvariantValue) continue; - //if (!property.PropertyType.VariesByCulture() && isCultureValue) continue; + if (property.PropertyType.VariesByCulture() && isInvariantValue || !property.PropertyType.VariesByCulture() && isCultureValue) + continue; // use explicit equals here, else object comparison fails at comparing eg strings var sameValues = propertyValue.PublishedValue == null ? propertyValue.EditedValue == null : propertyValue.PublishedValue.Equals(propertyValue.EditedValue); diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index 756435692a..b29565a35e 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -648,14 +648,14 @@ AND umbracoNode.id <> @id", case ContentVariation.Culture: CopyPropertyData(null, defaultLanguageId, propertyTypeIds, impactedL); CopyTagData(null, defaultLanguageId, propertyTypeIds, impactedL); - RenormalizeDocumentCultureVariations(propertyTypeIds, impactedL); + RenormalizeDocumentEditedFlags(propertyTypeIds, impactedL); //TODO: Here we need to normalize the umbracoDocumentCultureVariation table for it's edited flags which are calculated based //on changed property or name values break; case ContentVariation.Nothing: CopyPropertyData(defaultLanguageId, null, propertyTypeIds, impactedL); CopyTagData(defaultLanguageId, null, propertyTypeIds, impactedL); - RenormalizeDocumentCultureVariations(propertyTypeIds, impactedL); + RenormalizeDocumentEditedFlags(propertyTypeIds, impactedL); //TODO: Here we need to normalize the umbracoDocumentCultureVariation table for it's edited flags which are calculated based //on changed property or name values break; @@ -1024,19 +1024,17 @@ AND umbracoNode.id <> @id", } /// - /// Re-normalizes the edited value in the umbracoDocumentCultureVariation table when property variations are changed + /// Re-normalizes the edited value in the umbracoDocumentCultureVariation and umbracoDocument table when variations are changed /// /// /// /// /// If this is not done, then in some cases the "edited" value for a particular culture for a document will remain true when it should be false /// if the property was changed to invariant. In order to do this we need to recalculate this value based on the values stored for each - /// property, culture and current/published version. The end result is to update the edited value in the umbracoDocumentCultureVariation table so we - /// make sure to join this table with the lookups so that only relevant data is returned. + /// property, culture and current/published version. /// - private void RenormalizeDocumentCultureVariations(IReadOnlyCollection propertyTypeIds, IReadOnlyCollection contentTypeIds = null) + private void RenormalizeDocumentEditedFlags(IReadOnlyCollection propertyTypeIds, IReadOnlyCollection contentTypeIds = null) { - 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 @@ -1073,14 +1071,16 @@ AND umbracoNode.id <> @id", .OrderBy(x => x.NodeId) .OrderBy(x => x.PropertyTypeId, x => x.LanguageId, x => x.VersionId); - //keep track of this node/lang to mark or unmark as edited - var editedVersions = new Dictionary<(int nodeId, int? langId), bool>(); - + //keep track of this node/lang to mark or unmark a culture as edited + var editedLanguageVersions = new Dictionary<(int nodeId, int? langId), bool>(); + //keep track of which node to mark or unmark as edited + var editedDocument = new Dictionary(); var nodeId = -1; var propertyTypeId = -1; + PropertyValueVersionDto pubRow = null; - //This is a QUERY we are not fetching this all into memory so we cannot make any changes during this iteration, we are just collecting data. + //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. //Published data will always come before Current data based on the version id sort. //There will only be one published row (max) and one current row per property. foreach (var row in Database.Query(propertySql)) @@ -1105,19 +1105,24 @@ AND umbracoNode.id <> @id", || propVariations.VariesByCulture() && !row.LanguageId.HasValue) { //Flag this as not edited for this node/lang if the key doesn't exist - if (!editedVersions.TryGetValue((row.NodeId, row.LanguageId), out _)) - editedVersions.Add((row.NodeId, row.LanguageId), false); + if (!editedLanguageVersions.TryGetValue((row.NodeId, row.LanguageId), out _)) + editedLanguageVersions.Add((row.NodeId, row.LanguageId), false); + + //mark as false if the item doesn't exist, else coerce to true + editedDocument[row.NodeId] = editedDocument.TryGetValue(row.NodeId, out var edited) ? (edited |= false) : false; } else if (pubRow == null) { //this would mean that that this property is 'edited' since there is no published version - editedVersions.Add((row.NodeId, row.LanguageId), true); + editedLanguageVersions.Add((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 else if (IsPropertyValueChanged(pubRow, row)) { //Here we would check if the property is invariant, in which case the edited language should be indicated by the default lang - editedVersions[(row.NodeId, !propVariations.VariesByCulture() ? defaultLang : row.LanguageId)] = true; + editedLanguageVersions[(row.NodeId, !propVariations.VariesByCulture() ? defaultLang : row.LanguageId)] = true; + editedDocument[row.NodeId] = true; } //reset @@ -1125,32 +1130,6 @@ AND umbracoNode.id <> @id", } } - //lookup all matching rows in umbracoDocumentCultureVariation - var docCultureVariationsToUpdate = Database.Fetch( - Sql().Select().From() - .WhereIn(x => x.LanguageId, editedVersions.Keys.Select(x => x.langId).ToList()) - .WhereIn(x => x.NodeId, editedVersions.Keys.Select(x => x.nodeId))) - //convert to dictionary with the same key type - .ToDictionary(x => (x.NodeId, (int?)x.LanguageId), x => x); - - foreach (var ev in editedVersions) - { - if (docCultureVariationsToUpdate.TryGetValue(ev.Key, out var docVariations)) - { - //check if it needs updating - if (docVariations.Edited != ev.Value) - { - docVariations.Edited = ev.Value; - Database.Update(docVariations); - } - } - else - { - //the row doesn't exist but needs creating - //TODO: Does this ever happen?? Need to see if we can test this - } - } - ////Generate SQL to lookup the current name vs the publish name for each language //var nameSql = Sql() // .Select("cv1", x => x.NodeId, x => Alias(x.Id, "currentVersion")) @@ -1172,8 +1151,57 @@ AND umbracoNode.id <> @id", // .Where(x => x.Current, "cv1") // .OrderBy("cv1.nodeId, cvcv1.versionId, cvcv1.languageId"); - //var names = Database.Fetch(nameSql); + ////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. + //foreach (var name in Database.Query(nameSql)) + //{ + // if (name.CurrentName != name.PublishedName) + // { + // } + //} + + //lookup all matching rows in umbracoDocumentCultureVariation + var docCultureVariationsToUpdate = editedLanguageVersions.InGroupsOf(2000) + .SelectMany(_ => Database.Fetch( + Sql().Select().From() + .WhereIn(x => x.LanguageId, editedLanguageVersions.Keys.Select(x => x.langId).ToList()) + .WhereIn(x => x.NodeId, editedLanguageVersions.Keys.Select(x => x.nodeId)))) + //convert to dictionary with the same key type + .ToDictionary(x => (x.NodeId, (int?)x.LanguageId), x => x); + + var toUpdate = new List(); + foreach (var ev in editedLanguageVersions) + { + if (docCultureVariationsToUpdate.TryGetValue(ev.Key, out var docVariations)) + { + //check if it needs updating + if (docVariations.Edited != ev.Value) + { + docVariations.Edited = ev.Value; + toUpdate.Add(docVariations); + } + } + else + { + //the row doesn't exist but needs creating + //TODO: Does this ever happen?? Need to see if we can test this + throw new PanicException($"The existing DocumentCultureVariationDto was not found for node {ev.Key.nodeId} and language {ev.Key.langId}"); + } + } + + //Now bulk update the table DocumentCultureVariationDto, once for edited = true, another for edited = false + foreach (var editValue in toUpdate.GroupBy(x => x.Edited)) + { + Database.Execute(Sql().Update(u => u.Set(x => x.Edited, editValue.Key)) + .WhereIn(x => x.Id, editValue.Select(x => x.Id))); + } + + //Now bulk update the umbracoDocument table + 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))); + } } private static bool IsPropertyValueChanged(PropertyValueVersionDto pubRow, PropertyValueVersionDto row) diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs index 0dbfc61b8c..c18921b59e 100644 --- a/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/DocumentRepository.cs @@ -511,7 +511,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement entity.VersionId = documentVersionDto.Id = contentVersionDto.Id; // get the new id documentVersionDto.Published = false; // non-published version - Database.Insert(documentVersionDto); + Database.Insert(documentVersionDto); } // replace the property data (rather than updating) diff --git a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs index 53a39ffe40..2fed37c389 100644 --- a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs +++ b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs @@ -828,12 +828,14 @@ namespace Umbraco.Tests.Services Assert.AreEqual("doc1en", document.GetCultureName("en")); Assert.AreEqual("doc1fr", document.GetCultureName("fr")); Assert.AreEqual("v1en", document.GetValue("value1", "en")); + Assert.AreEqual("v1en-init", document.GetValue("value1", "en", published: true)); Assert.AreEqual("v1fr", document.GetValue("value1", "fr")); + Assert.AreEqual("v1fr-init", document.GetValue("value1", "fr", published: true)); Assert.IsTrue(document.IsCultureEdited("en")); //This will be true because the edited value isn't the same as the published value Assert.IsTrue(document.IsCultureEdited("fr")); //This will be true because the edited value isn't the same as the published value Assert.IsTrue(document.Edited); - // switch property type to Nothing + // switch property type to Invariant contentType.PropertyTypes.First(x => x.Alias == "value1").Variations = ContentVariation.Nothing; ServiceContext.ContentTypeService.Save(contentType); //This is going to have to re-normalize the "Edited" flag @@ -852,9 +854,12 @@ namespace Umbraco.Tests.Services Assert.AreEqual("doc1fr", document.GetCultureName("fr")); Assert.IsNull(document.GetValue("value1", "en")); //The values are there but the business logic returns null Assert.IsNull(document.GetValue("value1", "fr")); //The values are there but the business logic returns null + Assert.IsNull(document.GetValue("value1", "en", published: true)); //The values are there but the business logic returns null + Assert.IsNull(document.GetValue("value1", "fr", published: true)); //The values are there but the business logic returns null Assert.AreEqual("v1inv", document.GetValue("value1")); - Assert.IsFalse(document.IsCultureEdited("en")); //This returns false, everything is published - however in the DB this is not the case for the "en" version of the property - Assert.IsFalse(document.IsCultureEdited("fr")); //This returns false, everything is published + Assert.AreEqual("v1inv", document.GetValue("value1", published: true)); + Assert.IsFalse(document.IsCultureEdited("en")); //This returns false, everything is published + Assert.IsFalse(document.IsCultureEdited("fr")); //This will be false because nothing has changed for this culture and the property no longer reflects variant changes Assert.IsFalse(document.Edited); // switch property back to Culture @@ -863,14 +868,12 @@ namespace Umbraco.Tests.Services document = ServiceContext.ContentService.GetById(document.Id); Assert.AreEqual("v1inv", document.GetValue("value1", "en")); //The invariant property value gets copied over to the default language - Assert.AreEqual("v1fr", document.GetValue("value1", "fr")); + Assert.AreEqual("v1inv", document.GetValue("value1", "en", published: true)); + Assert.AreEqual("v1fr", document.GetValue("value1", "fr")); //values are still retained + Assert.AreEqual("v1fr-init", document.GetValue("value1", "fr", published: true)); //values are still retained Assert.IsFalse(document.IsCultureEdited("en")); //The invariant published AND edited values are copied over to the default language - //TODO: This fails - for some reason in the DB, the DocumentCultureVariationDto Edited flag is set to false for the FR culture - // I'm assuming this is somehow done during the ContentTypeService.Save method when changing variation. It should not be false but instead - // true because the values for it's edited vs published version are different. Perhaps it's false because that is the default boolean value and - // this row gets deleted somewhere along the way? Assert.IsTrue(document.IsCultureEdited("fr")); //The previously existing french values are there and there is no published value - Assert.IsTrue(document.Edited); + Assert.IsTrue(document.Edited); //Will be flagged edited again because the french culture had pending changes // publish again document.SetValue("value1", "v1en2", "en"); //update the value now that it's variant again