From 92b64b2ca612bd2b75549db80a21b194f8c5c56f Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 4 Apr 2019 21:57:49 +1100 Subject: [PATCH] Changes controller to be explicit about which culture will be affiliated with invariant property issues --- src/Umbraco.Core/Models/CultureImpact.cs | 26 +++++++++ .../Models/CultureImpactTests.cs | 26 ++++++++- .../Web/ModelStateExtensionsTests.cs | 6 +- src/Umbraco.Web/Editors/ContentController.cs | 56 +++++++++---------- src/Umbraco.Web/ModelStateExtensions.cs | 25 ++++++--- 5 files changed, 99 insertions(+), 40 deletions(-) diff --git a/src/Umbraco.Core/Models/CultureImpact.cs b/src/Umbraco.Core/Models/CultureImpact.cs index 4e6b6a1a36..0b035c1703 100644 --- a/src/Umbraco.Core/Models/CultureImpact.cs +++ b/src/Umbraco.Core/Models/CultureImpact.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; namespace Umbraco.Core.Models { @@ -11,6 +12,31 @@ namespace Umbraco.Core.Models /// internal class CultureImpact { + /// + /// Utility method to return the culture used for invariant property errors based on what cultures are being actively saved, + /// the default culture and the state of the current content item + /// + /// + /// + /// + /// + public static string GetCultureForInvariantErrors(IContent content, string[] savingCultures, string defaultCulture) + { + if (content == null) throw new ArgumentNullException(nameof(content)); + if (savingCultures == null) throw new ArgumentNullException(nameof(savingCultures)); + if (savingCultures.Length == 0) throw new ArgumentException(nameof(savingCultures)); + if (defaultCulture == null) throw new ArgumentNullException(nameof(defaultCulture)); + + var cultureForInvariantErrors = savingCultures.Any(x => x.InvariantEquals(defaultCulture)) + //the default culture is being flagged for saving so use it + ? defaultCulture + //If the content has no published version, we need to affiliate validation with the first variant being saved. + //If the content has a published version we will not affiliate the validation with any culture (null) + : !content.Published ? savingCultures[0] : null; + + return cultureForInvariantErrors; + } + /// /// Initializes a new instance of the class. /// diff --git a/src/Umbraco.Tests/Models/CultureImpactTests.cs b/src/Umbraco.Tests/Models/CultureImpactTests.cs index 9badbd6c34..6cad634a14 100644 --- a/src/Umbraco.Tests/Models/CultureImpactTests.cs +++ b/src/Umbraco.Tests/Models/CultureImpactTests.cs @@ -1,4 +1,5 @@ -using NUnit.Framework; +using Moq; +using NUnit.Framework; using Umbraco.Core.Models; namespace Umbraco.Tests.Models @@ -6,6 +7,29 @@ namespace Umbraco.Tests.Models [TestFixture] public class CultureImpactTests { + [Test] + public void Get_Culture_For_Invariant_Errors() + { + var result = CultureImpact.GetCultureForInvariantErrors( + Mock.Of(x => x.Published == true), + new[] { "en-US", "fr-FR" }, + "en-US"); + Assert.AreEqual("en-US", result); //default culture is being saved so use it + + result = CultureImpact.GetCultureForInvariantErrors( + Mock.Of(x => x.Published == false), + new[] { "fr-FR" }, + "en-US"); + Assert.AreEqual("fr-FR", result); //default culture not being saved with not published version, use the first culture being saved + + result = CultureImpact.GetCultureForInvariantErrors( + Mock.Of(x => x.Published == true), + new[] { "fr-FR" }, + "en-US"); + Assert.AreEqual(null, result); //default culture not being saved with published version, use null + + } + [Test] public void All_Cultures() { diff --git a/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs b/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs index 9fc3cee4ef..3ce43b5fc2 100644 --- a/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs +++ b/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs @@ -22,7 +22,7 @@ namespace Umbraco.Tests.Web ms.AddPropertyError(new ValidationResult("no header image"), "headerImage", null); //invariant property ms.AddPropertyError(new ValidationResult("title missing"), "title", "en-US"); //variant property - var result = ms.GetCulturesWithErrors(localizationService.Object); + var result = ms.GetCulturesWithErrors(localizationService.Object, "en-US"); //even though there are 2 errors, they are both for en-US since that is the default language and one of the errors is for an invariant property Assert.AreEqual(1, result.Count); @@ -31,7 +31,7 @@ namespace Umbraco.Tests.Web ms = new ModelStateDictionary(); ms.AddCultureValidationError("en-US", "generic culture error"); - result = ms.GetCulturesWithErrors(localizationService.Object); + result = ms.GetCulturesWithErrors(localizationService.Object, "en-US"); Assert.AreEqual(1, result.Count); Assert.AreEqual("en-US", result[0]); @@ -47,7 +47,7 @@ namespace Umbraco.Tests.Web ms.AddPropertyError(new ValidationResult("no header image"), "headerImage", null); //invariant property ms.AddPropertyError(new ValidationResult("title missing"), "title", "en-US"); //variant property - var result = ms.GetCulturesWithPropertyErrors(localizationService.Object); + var result = ms.GetCulturesWithPropertyErrors(localizationService.Object, "en-US"); //even though there are 2 errors, they are both for en-US since that is the default language and one of the errors is for an invariant property Assert.AreEqual(1, result.Count); diff --git a/src/Umbraco.Web/Editors/ContentController.cs b/src/Umbraco.Web/Editors/ContentController.cs index d62f8ee844..a3788e4590 100644 --- a/src/Umbraco.Web/Editors/ContentController.cs +++ b/src/Umbraco.Web/Editors/ContentController.cs @@ -664,11 +664,19 @@ namespace Umbraco.Web.Editors [string.Empty] = globalNotifications }; + //The default validation language will be either: The default languauge, else if the content is brand new and the default culture is + // not marked to be saved, it will be the first culture in the list marked for saving. + var defaultCulture = _allLangs.Value.First(x => x.Value.IsDefault).Key; + var cultureForInvariantErrors = CultureImpact.GetCultureForInvariantErrors( + contentItem.PersistedContent, + contentItem.Variants.Where(x => x.Save).Select(x => x.Culture).ToArray(), + defaultCulture); + switch (contentItem.Action) { case ContentSaveAction.Save: case ContentSaveAction.SaveNew: - SaveAndNotify(contentItem, saveMethod, variantCount, notifications, globalNotifications, "editContentSavedText", "editVariantSavedText", out wasCancelled); + SaveAndNotify(contentItem, saveMethod, variantCount, notifications, globalNotifications, "editContentSavedText", "editVariantSavedText", cultureForInvariantErrors, out wasCancelled); break; case ContentSaveAction.Schedule: case ContentSaveAction.ScheduleNew: @@ -678,7 +686,7 @@ namespace Umbraco.Web.Editors wasCancelled = false; break; } - SaveAndNotify(contentItem, saveMethod, variantCount, notifications, globalNotifications, "editContentScheduledSavedText", "editVariantSavedText", out wasCancelled); + SaveAndNotify(contentItem, saveMethod, variantCount, notifications, globalNotifications, "editContentScheduledSavedText", "editVariantSavedText", cultureForInvariantErrors, out wasCancelled); break; case ContentSaveAction.SendPublish: @@ -689,7 +697,7 @@ namespace Umbraco.Web.Editors { if (variantCount > 1) { - var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); foreach (var c in contentItem.Variants.Where(x => x.Save && !cultureErrors.Contains(x.Culture)).Select(x => x.Culture).ToArray()) { AddSuccessNotification(notifications, c, @@ -708,7 +716,7 @@ namespace Umbraco.Web.Editors case ContentSaveAction.Publish: case ContentSaveAction.PublishNew: { - var publishStatus = PublishInternal(contentItem, out wasCancelled, out var successfulCultures); + var publishStatus = PublishInternal(contentItem, defaultCulture, cultureForInvariantErrors, out wasCancelled, out var successfulCultures); //global notifications AddMessageForPublishStatus(new[] { publishStatus }, globalNotifications, successfulCultures); //variant specific notifications @@ -728,7 +736,7 @@ namespace Umbraco.Web.Editors break; } - var publishStatus = PublishBranchInternal(contentItem, false, out wasCancelled, out var successfulCultures).ToList(); + var publishStatus = PublishBranchInternal(contentItem, false, cultureForInvariantErrors, out wasCancelled, out var successfulCultures).ToList(); //global notifications AddMessageForPublishStatus(publishStatus, globalNotifications, successfulCultures); @@ -749,7 +757,7 @@ namespace Umbraco.Web.Editors break; } - var publishStatus = PublishBranchInternal(contentItem, true, out wasCancelled, out var successfulCultures).ToList(); + var publishStatus = PublishBranchInternal(contentItem, true, cultureForInvariantErrors, out wasCancelled, out var successfulCultures).ToList(); //global notifications AddMessageForPublishStatus(publishStatus, globalNotifications, successfulCultures); @@ -774,7 +782,7 @@ namespace Umbraco.Web.Editors } //lastly, if it is not valid, add the model state to the outgoing object and throw a 400 - HandleInvalidModelState(display); + HandleInvalidModelState(display, cultureForInvariantErrors); if (wasCancelled) { @@ -857,6 +865,8 @@ namespace Umbraco.Web.Editors return true; } + + /// /// Helper method to perform the saving of the content and add the notifications to the result /// @@ -873,7 +883,7 @@ namespace Umbraco.Web.Editors /// private void SaveAndNotify(ContentItemSave contentItem, Func saveMethod, int variantCount, Dictionary notifications, SimpleNotificationModel globalNotifications, - string invariantSavedLocalizationKey, string variantSavedLocalizationKey, + string invariantSavedLocalizationKey, string variantSavedLocalizationKey, string cultureForInvariantErrors, out bool wasCancelled) { var saveResult = saveMethod(contentItem.PersistedContent); @@ -882,7 +892,7 @@ namespace Umbraco.Web.Editors { if (variantCount > 1) { - var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); foreach (var c in contentItem.Variants.Where(x => x.Save && !cultureErrors.Contains(x.Culture)).Select(x => x.Culture).ToArray()) { AddSuccessNotification(notifications, c, @@ -1124,7 +1134,7 @@ namespace Umbraco.Web.Editors return denied.Count == 0; } - private IEnumerable PublishBranchInternal(ContentItemSave contentItem, bool force, + private IEnumerable PublishBranchInternal(ContentItemSave contentItem, bool force, string cultureForInvariantErrors, out bool wasCancelled, out string[] successfulCultures) { if (!contentItem.PersistedContent.ContentType.VariesByCulture()) @@ -1142,7 +1152,7 @@ namespace Umbraco.Web.Editors var mandatoryCultures = _allLangs.Value.Values.Where(x => x.IsMandatory).Select(x => x.IsoCode).ToList(); - var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); //validate if we can publish based on the mandatory language requirements var canPublish = ValidatePublishingMandatoryLanguages( @@ -1198,7 +1208,7 @@ namespace Umbraco.Web.Editors /// /// If this is a culture variant than we need to do some validation, if it's not we'll publish as normal /// - private PublishResult PublishInternal(ContentItemSave contentItem, out bool wasCancelled, out string[] successfulCultures) + private PublishResult PublishInternal(ContentItemSave contentItem, string defaultCulture, string cultureForInvariantErrors, out bool wasCancelled, out string[] successfulCultures) { if (!contentItem.PersistedContent.ContentType.VariesByCulture()) { @@ -1214,7 +1224,7 @@ namespace Umbraco.Web.Editors var mandatoryCultures = _allLangs.Value.Values.Where(x => x.IsMandatory).Select(x => x.IsoCode).ToList(); - var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); //validate if we can publish based on the mandatory languages selected var canPublish = ValidatePublishingMandatoryLanguages( @@ -1243,7 +1253,7 @@ namespace Umbraco.Web.Editors { //try to publish all the values on the model - this will generally only fail if someone is tampering with the request //since there's no reason variant rules would be violated in normal cases. - canPublish = PublishCulture(contentItem.PersistedContent, cultureVariants); + canPublish = PublishCulture(contentItem.PersistedContent, cultureVariants, defaultCulture); } if (canPublish) @@ -1336,12 +1346,12 @@ namespace Umbraco.Web.Editors /// /// This would generally never fail unless someone is tampering with the request /// - private bool PublishCulture(IContent persistentContent, IEnumerable cultureVariants) + private bool PublishCulture(IContent persistentContent, IEnumerable cultureVariants, string defaultCulture) { foreach (var variant in cultureVariants.Where(x => x.Publish)) { // publishing any culture, implies the invariant culture - var valid = persistentContent.PublishCulture(CultureImpact.Explicit(variant.Culture, IsDefaultCulture(variant.Culture))); + var valid = persistentContent.PublishCulture(CultureImpact.Explicit(variant.Culture, defaultCulture.InvariantEquals(variant.Culture))); if (!valid) { AddCultureValidationError(variant.Culture, "speechBubbles/contentCultureValidationError"); @@ -1786,12 +1796,12 @@ namespace Umbraco.Web.Editors /// /// This is required to wire up the validation in the save/publish dialog /// - private void HandleInvalidModelState(ContentItemDisplay display) + private void HandleInvalidModelState(ContentItemDisplay display, string cultureForInvariantErrors) { if (!ModelState.IsValid && display.Variants.Count() > 1) { //Add any culture specific errors here - var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); foreach (var cultureError in cultureErrors) { @@ -2125,16 +2135,6 @@ namespace Umbraco.Web.Editors } } - /// - /// Returns true if the culture specified is the default culture - /// - /// - /// - private bool IsDefaultCulture(string culture) - { - return _allLangs.Value.Any(x => x.Value.IsDefault && x.Key.InvariantEquals(culture)); - } - /// /// Used to map an instance to a and ensuring a language is present if required /// diff --git a/src/Umbraco.Web/ModelStateExtensions.cs b/src/Umbraco.Web/ModelStateExtensions.cs index 46be12cae8..3bbcdfb0ac 100644 --- a/src/Umbraco.Web/ModelStateExtensions.cs +++ b/src/Umbraco.Web/ModelStateExtensions.cs @@ -4,6 +4,7 @@ using System.ComponentModel.DataAnnotations; using System.Linq; using System.Web.Mvc; using Umbraco.Core; +using Umbraco.Core.Models; using Umbraco.Core.Services; namespace Umbraco.Web @@ -76,9 +77,12 @@ namespace Umbraco.Web /// /// /// - /// + /// The culture to affiliate invariant errors with + /// + /// A list of cultures that have property validation errors. The default culture will be returned for any invariant property errors. + /// internal static IReadOnlyList GetCulturesWithPropertyErrors(this System.Web.Http.ModelBinding.ModelStateDictionary modelState, - ILocalizationService localizationService) + ILocalizationService localizationService, string cultureForInvariantErrors) { //Add any culture specific errors here var cultureErrors = modelState.Keys @@ -88,7 +92,8 @@ namespace Umbraco.Web .Where(x => !x.IsNullOrWhiteSpace()) //if it has a value //if it's marked "invariant" than return the default language, this is because we can only edit invariant properties on the default language //so errors for those must show up under the default lang. - .Select(x => x == "invariant" ? localizationService.GetDefaultLanguageIsoCode() : x) + .Select(x => x == "invariant" ? cultureForInvariantErrors : x) + .WhereNotNull() .Distinct() .ToList(); @@ -100,11 +105,14 @@ namespace Umbraco.Web /// /// /// - /// + /// The culture to affiliate invariant errors with + /// + /// A list of cultures that have validation errors. The default culture will be returned for any invariant errors. + /// internal static IReadOnlyList GetCulturesWithErrors(this System.Web.Http.ModelBinding.ModelStateDictionary modelState, - ILocalizationService localizationService) + ILocalizationService localizationService, string cultureForInvariantErrors) { - var propertyCultureErrors = modelState.GetCulturesWithPropertyErrors(localizationService); + var propertyCultureErrors = modelState.GetCulturesWithPropertyErrors(localizationService, cultureForInvariantErrors); //now check the other special culture errors that are var genericCultureErrors = modelState.Keys @@ -113,12 +121,13 @@ namespace Umbraco.Web .Where(x => !x.IsNullOrWhiteSpace()) //if it's marked "invariant" than return the default language, this is because we can only edit invariant properties on the default language //so errors for those must show up under the default lang. - .Select(x => x == "invariant" ? localizationService.GetDefaultLanguageIsoCode() : x) + .Select(x => x == "invariant" ? cultureForInvariantErrors : x) + .WhereNotNull() .Distinct(); return propertyCultureErrors.Union(genericCultureErrors).ToList(); } - + /// /// Adds the error to model state correctly for a property so we can use it on the client side. ///