diff --git a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs index c0e4cd70321f..bf7228ca4795 100644 --- a/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs +++ b/src/Umbraco.Core/Models/ContentRepositoryExtensions.cs @@ -1,11 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; -using Umbraco.Core.Collections; -using Umbraco.Core.Composing; using Umbraco.Core.Exceptions; -using Umbraco.Core.PropertyEditors; -using Umbraco.Core.Services; namespace Umbraco.Core.Models { @@ -170,23 +166,23 @@ public static void SetCultureInfo(this IContentBase content, string culture, str /// Sets the publishing values for names and properties. /// /// - /// + /// /// A value indicating whether it was possible to publish the names and values for the specified /// culture(s). The method may fail if required names are not set, but it does NOT validate property data - public static bool PublishCulture(this IContent content, string culture = "*") + public static bool PublishCulture(this IContent content, CultureImpact impact) { - culture = culture.NullOrWhiteSpaceAsNull(); + if (impact == null) throw new ArgumentNullException(nameof(impact)); // the variation should be supported by the content type properties // if the content type is invariant, only '*' and 'null' is ok // if the content type varies, everything is ok because some properties may be invariant - if (!content.ContentType.SupportsPropertyVariation(culture, "*", true)) - throw new NotSupportedException($"Culture \"{culture}\" is not supported by content type \"{content.ContentType.Alias}\" with variation \"{content.ContentType.Variations}\"."); + if (!content.ContentType.SupportsPropertyVariation(impact.Culture, "*", true)) + throw new NotSupportedException($"Culture \"{impact.Culture}\" is not supported by content type \"{content.ContentType.Alias}\" with variation \"{content.ContentType.Variations}\"."); - var alsoInvariant = false; - if (culture == "*") // all cultures + // set names + if (impact.ImpactsAllCultures) { - foreach (var c in content.AvailableCultures) + foreach (var c in content.AvailableCultures) // does NOT contain the invariant culture { var name = content.GetCultureName(c); if (string.IsNullOrWhiteSpace(name)) @@ -194,26 +190,31 @@ public static bool PublishCulture(this IContent content, string culture = "*") content.SetPublishInfo(c, name, DateTime.Now); } } - else if (culture == null) // invariant culture + else if (impact.ImpactsOnlyInvariantCulture) { if (string.IsNullOrWhiteSpace(content.Name)) return false; // PublishName set by repository - nothing to do here } - else // one single culture + else if (impact.ImpactsExplicitCulture) { - var name = content.GetCultureName(culture); + var name = content.GetCultureName(impact.Culture); if (string.IsNullOrWhiteSpace(name)) return false; - content.SetPublishInfo(culture, name, DateTime.Now); - alsoInvariant = true; // we also want to publish invariant values + content.SetPublishInfo(impact.Culture, name, DateTime.Now); } - // property.PublishValues only publishes what is valid, variation-wise + // set values + // property.PublishValues only publishes what is valid, variation-wise, + // but accepts any culture arg: null, all, specific foreach (var property in content.Properties) { - property.PublishValues(culture); - if (alsoInvariant) + // for the specified culture (null or all or specific) + property.PublishValues(impact.Culture); + + // maybe the specified culture did not impact the invariant culture, so PublishValues + // above would skip it, yet it *also* impacts invariant properties + if (impact.ImpactsAlsoInvariantProperties) property.PublishValues(null); } diff --git a/src/Umbraco.Core/Models/CultureImpact.cs b/src/Umbraco.Core/Models/CultureImpact.cs new file mode 100644 index 000000000000..0b035c170343 --- /dev/null +++ b/src/Umbraco.Core/Models/CultureImpact.cs @@ -0,0 +1,258 @@ +using System; +using System.Linq; + +namespace Umbraco.Core.Models +{ + /// + /// Represents the impact of a culture set. + /// + /// + /// A set of cultures can be either all cultures (including the invariant culture), or + /// the invariant culture, or a specific culture. + /// + 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. + /// + /// The culture code. + /// A value indicating whether the culture is the default culture. + private CultureImpact(string culture, bool isDefault = false) + { + if (culture != null && culture.IsNullOrWhiteSpace()) + throw new ArgumentException("Culture \"\" is not valid here."); + + Culture = culture; + + if ((culture == null || culture == "*") && isDefault) + throw new ArgumentException("The invariant or 'all' culture can not be the default culture."); + + ImpactsOnlyDefaultCulture = isDefault; + } + + /// + /// Gets the impact of 'all' cultures (including the invariant culture). + /// + public static CultureImpact All { get; } = new CultureImpact("*"); + + /// + /// Gets the impact of the invariant culture. + /// + public static CultureImpact Invariant { get; } = new CultureImpact(null); + + /// + /// Creates an impact instance representing the impact of a specific culture. + /// + /// The culture code. + /// A value indicating whether the culture is the default culture. + public static CultureImpact Explicit(string culture, bool isDefault) + { + if (culture == null) + throw new ArgumentException("Culture is not explicit."); + if (culture.IsNullOrWhiteSpace()) + throw new ArgumentException("Culture \"\" is not explicit."); + if (culture == "*") + throw new ArgumentException("Culture \"*\" is not explicit."); + + return new CultureImpact(culture, isDefault); + } + + /// + /// Creates an impact instance representing the impact of a culture set, + /// in the context of a content item variation. + /// + /// The culture code. + /// A value indicating whether the culture is the default culture. + /// The content item. + /// + /// Validates that the culture is compatible with the variation. + /// + public static CultureImpact Create(string culture, bool isDefault, IContent content) + { + // throws if not successful + TryCreate(culture, isDefault, content.ContentType.Variations, true, out var impact); + return impact; + } + + /// + /// Tries to create an impact instance representing the impact of a culture set, + /// in the context of a content item variation. + /// + /// The culture code. + /// A value indicating whether the culture is the default culture. + /// A content variation. + /// A value indicating whether to throw if the impact cannot be created. + /// The impact if it could be created, otherwise null. + /// A value indicating whether the impact could be created. + /// + /// Validates that the culture is compatible with the variation. + /// + internal static bool TryCreate(string culture, bool isDefault, ContentVariation variation, bool throwOnFail, out CultureImpact impact) + { + impact = null; + + // if culture is invariant... + if (culture == null) + { + // ... then variation must not vary by culture ... + if (variation.VariesByCulture()) + { + if (throwOnFail) + throw new InvalidOperationException("The invariant culture is not compatible with a varying variation."); + return false; + } + + // ... and it cannot be default + if (isDefault) + { + if (throwOnFail) + throw new InvalidOperationException("The invariant culture can not be the default culture."); + return false; + } + + impact = Invariant; + return true; + } + + // if culture is 'all'... + if (culture == "*") + { + // ... it cannot be default + if (isDefault) + { + if (throwOnFail) + throw new InvalidOperationException("The 'all' culture can not be the default culture."); + return false; + } + + // if variation does not vary by culture, then impact is invariant + impact = variation.VariesByCulture() ? All : Invariant; + return true; + } + + // neither null nor "*" - cannot be the empty string + if (culture.IsNullOrWhiteSpace()) + { + if (throwOnFail) + throw new ArgumentException("Cannot be the empty string.", nameof(culture)); + return false; + } + + // if culture is specific, then variation must vary + if (!variation.VariesByCulture()) + { + if (throwOnFail) + throw new InvalidOperationException($"The variant culture {culture} is not compatible with an invariant variation."); + return false; + } + + // return specific impact + impact = new CultureImpact(culture, isDefault); + return true; + } + + /// + /// Gets the culture code. + /// + /// + /// Can be null (invariant) or * (all cultures) or a specific culture code. + /// + public string Culture { get; } + + /// + /// Gets a value indicating whether this impact impacts all cultures, including, + /// indirectly, the invariant culture. + /// + public bool ImpactsAllCultures => Culture == "*"; + + /// + /// Gets a value indicating whether this impact impacts only the invariant culture, + /// directly, not because all cultures are impacted. + /// + public bool ImpactsOnlyInvariantCulture => Culture == null; + + /// + /// Gets a value indicating whether this impact impacts an implicit culture. + /// + /// And then it does not impact the invariant culture. The impacted + /// explicit culture could be the default culture. + public bool ImpactsExplicitCulture => Culture != null && Culture != "*"; + + /// + /// Gets a value indicating whether this impact impacts the default culture, directly, + /// not because all cultures are impacted. + /// + public bool ImpactsOnlyDefaultCulture {get; } + + /// + /// Gets a value indicating whether this impact impacts the invariant properties, either + /// directly, or because all cultures are impacted, or because the default culture is impacted. + /// + public bool ImpactsInvariantProperties => Culture == null || Culture == "*" || ImpactsOnlyDefaultCulture; + + /// + /// Gets a value indicating whether this also impact impacts the invariant properties, + /// even though it does not impact the invariant culture, neither directly (ImpactsInvariantCulture) + /// nor indirectly (ImpactsAllCultures). + /// + public bool ImpactsAlsoInvariantProperties => !ImpactsOnlyInvariantCulture && + !ImpactsAllCultures && + ImpactsOnlyDefaultCulture; + + public Behavior CultureBehavior + { + get + { + //null can only be invariant + if (Culture == null) return Behavior.InvariantCulture | Behavior.InvariantProperties; + + // * is All which means its also invariant properties since this will include the default language + if (Culture == "*") return (Behavior.AllCultures | Behavior.InvariantProperties); + + //else it's explicit + var result = Behavior.ExplicitCulture; + + //if the explicit culture is the default, then the behavior is also InvariantProperties + if (ImpactsOnlyDefaultCulture) + result |= Behavior.InvariantProperties; + + return result; + } + } + + + [Flags] + public enum Behavior : byte + { + AllCultures = 1, + InvariantCulture = 2, + ExplicitCulture = 4, + InvariantProperties = 8 + } + } +} diff --git a/src/Umbraco.Core/Persistence/Repositories/Implement/LanguageRepositoryExtensions.cs b/src/Umbraco.Core/Persistence/Repositories/Implement/LanguageRepositoryExtensions.cs new file mode 100644 index 000000000000..b48b5588de43 --- /dev/null +++ b/src/Umbraco.Core/Persistence/Repositories/Implement/LanguageRepositoryExtensions.cs @@ -0,0 +1,11 @@ +namespace Umbraco.Core.Persistence.Repositories.Implement +{ + internal static class LanguageRepositoryExtensions + { + public static bool IsDefault(this ILanguageRepository repo, string culture) + { + if (culture == null || culture == "*") return false; + return repo.GetDefaultIsoCode().InvariantEquals(culture); + } + } +} diff --git a/src/Umbraco.Core/Services/Implement/ContentService.cs b/src/Umbraco.Core/Services/Implement/ContentService.cs index 5e1e0529b697..a4def1d209d7 100644 --- a/src/Umbraco.Core/Services/Implement/ContentService.cs +++ b/src/Umbraco.Core/Services/Implement/ContentService.cs @@ -10,7 +10,7 @@ using Umbraco.Core.Persistence.DatabaseModelDefinitions; using Umbraco.Core.Persistence.Querying; using Umbraco.Core.Persistence.Repositories; -using Umbraco.Core.PropertyEditors; +using Umbraco.Core.Persistence.Repositories.Implement; using Umbraco.Core.Scoping; using Umbraco.Core.Services.Changes; @@ -505,7 +505,7 @@ public IEnumerable GetVersionsSlim(int id, int skip, int take) /// public IEnumerable GetVersionIds(int id, int maxRows) { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + using (ScopeProvider.CreateScope(autoComplete: true)) { return _documentRepository.GetVersionIds(id, maxRows); } @@ -877,38 +877,15 @@ public PublishResult SaveAndPublish(IContent content, string culture = "*", int if (raiseEvents && scope.Events.DispatchCancelable(Saving, this, saveEventArgs, nameof(Saving))) return new PublishResult(PublishResultType.FailedPublishCancelledByEvent, evtMsgs, content); - Property[] invalidProperties; - // if culture is specific, first publish the invariant values, then publish the culture itself. // if culture is '*', then publish them all (including variants) - // explicitly SaveAndPublish a specific culture also publishes invariant values - if (!culture.IsNullOrWhiteSpace() && culture != "*") - { - // publish the invariant values - var publishInvariant = content.PublishCulture(null); - if (!publishInvariant) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); - - //validate the property values - if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) - { - InvalidProperties = invalidProperties - }; - } + //this will create the correct culture impact even if culture is * or null + var impact = CultureImpact.Create(culture, _languageRepository.IsDefault(culture), content); // publish the culture(s) - var publishCulture = content.PublishCulture(culture); - if (!publishCulture) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); - - //validate the property values - if (!_propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) - { - InvalidProperties = invalidProperties - }; + // we don't care about the response here, this response will be rechecked below but we need to set the culture info values now. + content.PublishCulture(impact); var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); scope.Complete(); @@ -941,16 +918,15 @@ public PublishResult SaveAndPublish(IContent content, string[] cultures, int use : new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); } + if (cultures.Any(x => x == null || x == "*")) + throw new InvalidOperationException("Only valid cultures are allowed to be used in this method, wildcards or nulls are not allowed"); - if (cultures.Select(content.PublishCulture).Any(isValid => !isValid)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + var impacts = cultures.Select(x => CultureImpact.Explicit(x, _languageRepository.IsDefault(x))); - //validate the property values - if (!_propertyValidationService.Value.IsPropertyDataValid(content, out var invalidProperties)) - return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) - { - InvalidProperties = invalidProperties - }; + // publish the culture(s) + // we don't care about the response here, this response will be rechecked below but we need to set the culture info values now. + foreach (var impact in impacts) + content.PublishCulture(impact); var result = CommitDocumentChangesInternal(scope, content, saveEventArgs, userId, raiseEvents); scope.Complete(); @@ -1096,6 +1072,7 @@ private PublishResult CommitDocumentChangesInternal(IScope scope, IContent conte if (publishing) { + //determine cultures publishing/unpublishing which will be based on previous calls to content.PublishCulture and ClearPublishInfo culturesUnpublishing = content.GetCulturesUnpublishing(); culturesPublishing = variesByCulture ? content.PublishCultureInfos.Values.Where(x => x.IsDirty()).Select(x => x.Culture).ToList() @@ -1147,7 +1124,7 @@ private PublishResult CommitDocumentChangesInternal(IScope scope, IContent conte // note: This unpublishes the entire document (not different variants) unpublishResult = StrategyCanUnpublish(scope, content, evtMsgs); if (unpublishResult.Success) - unpublishResult = StrategyUnpublish(scope, content, userId, evtMsgs); + unpublishResult = StrategyUnpublish(content, evtMsgs); else { // reset published state from temp values (publishing, unpublishing) to original value @@ -1330,7 +1307,8 @@ private IEnumerable PerformScheduledPublishInternal(DateTime date //publish the culture values and validate the property values, if validation fails, log the invalid properties so the develeper has an idea of what has failed Property[] invalidProperties = null; - var tryPublish = d.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(d, out invalidProperties); + var impact = CultureImpact.Explicit(culture, _languageRepository.IsDefault(culture)); + var tryPublish = d.PublishCulture(impact) && _propertyValidationService.Value.IsPropertyDataValid(d, out invalidProperties, impact); if (invalidProperties != null && invalidProperties.Length > 0) Logger.Warn("Scheduled publishing will fail for document {DocumentId} and culture {Culture} because of invalid properties {InvalidProperties}", d.Id, culture, string.Join(",", invalidProperties.Select(x => x.Alias))); @@ -1426,9 +1404,17 @@ private bool SaveAndPublishBranch_PublishCultures(IContent content, HashSet content.PublishCulture(culture) && _propertyValidationService.Value.IsPropertyDataValid(content, out _)) - : content.PublishCulture() && _propertyValidationService.Value.IsPropertyDataValid(content, out _); + if (content.ContentType.VariesByCulture()) + { + return culturesToPublish.All(culture => + { + var impact = CultureImpact.Create(culture, _languageRepository.IsDefault(culture), content); + return content.PublishCulture(impact) && _propertyValidationService.Value.IsPropertyDataValid(content, out _, impact); + }); + } + + return content.PublishCulture(CultureImpact.Invariant) + && _propertyValidationService.Value.IsPropertyDataValid(content, out _, CultureImpact.Invariant); } // utility 'ShouldPublish' func used by SaveAndPublishBranch @@ -2500,10 +2486,29 @@ private PublishResult StrategyCanPublish(IScope scope, IContent content, bool ch var variesByCulture = content.ContentType.VariesByCulture(); - //First check if mandatory languages fails, if this fails it will mean anything that the published flag on the document will + var impactsToPublish = culturesPublishing == null + ? new[] {CultureImpact.Invariant} //if it's null it's invariant + : culturesPublishing.Select(x => CultureImpact.Explicit(x, _languageRepository.IsDefault(x))).ToArray(); + + // publish the culture(s) + if (!impactsToPublish.All(content.PublishCulture)) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content); + + //validate the property values + Property[] invalidProperties = null; + if (!impactsToPublish.All(x => _propertyValidationService.Value.IsPropertyDataValid(content, out invalidProperties, x))) + return new PublishResult(PublishResultType.FailedPublishContentInvalid, evtMsgs, content) + { + InvalidProperties = invalidProperties + }; + + //Check if mandatory languages fails, if this fails it will mean anything that the published flag on the document will // be changed to Unpublished and any culture currently published will not be visible. if (variesByCulture) { + if (culturesPublishing == null) + throw new InvalidOperationException("Internal error, variesByCulture but culturesPublishing is null."); + if (content.Published && culturesPublishing.Count == 0 && culturesUnpublishing.Count == 0) // no published cultures = cannot be published return new PublishResult(PublishResultType.FailedPublishNothingToPublish, evtMsgs, content); @@ -2639,15 +2644,13 @@ private PublishResult StrategyCanUnpublish(IScope scope, IContent content, Event /// /// Unpublishes a document /// - /// /// - /// /// /// /// /// It is assumed that all unpublishing checks have passed before calling this method like /// - private PublishResult StrategyUnpublish(IScope scope, IContent content, int userId, EventMessages evtMsgs) + private PublishResult StrategyUnpublish(IContent content, EventMessages evtMsgs) { var attempt = new PublishResult(PublishResultType.SuccessUnpublish, evtMsgs, content); @@ -2899,7 +2902,7 @@ public IContent CreateContentFromBlueprint(IContent blueprint, string name, int public IEnumerable GetBlueprintsForContentTypes(params int[] contentTypeId) { - using (var scope = ScopeProvider.CreateScope(autoComplete: true)) + using (ScopeProvider.CreateScope(autoComplete: true)) { var query = Query(); if (contentTypeId.Length > 0) diff --git a/src/Umbraco.Core/Services/PropertyValidationService.cs b/src/Umbraco.Core/Services/PropertyValidationService.cs index 146173dd5c20..b846095bd1f8 100644 --- a/src/Umbraco.Core/Services/PropertyValidationService.cs +++ b/src/Umbraco.Core/Services/PropertyValidationService.cs @@ -31,30 +31,34 @@ public PropertyValidationService() /// /// Validates the content item's properties pass validation rules /// - /// If the content type is variant, then culture can be either '*' or an actual culture, but neither 'null' nor - /// 'empty'. If the content type is invariant, then culture can be either '*' or null or empty. - public bool IsPropertyDataValid(IContentBase content, out Property[] invalidProperties, string culture = "*") + public bool IsPropertyDataValid(IContent content, out Property[] invalidProperties, CultureImpact impact) { // select invalid properties invalidProperties = content.Properties.Where(x => { - // if culture is null, we validate invariant properties only - // if culture is '*' we validate both variant and invariant properties, automatically - // if culture is specific eg 'en-US' we both too, but explicitly + var propertyTypeVaries = x.PropertyType.VariesByCulture(); - var varies = x.PropertyType.VariesByCulture(); + // impacts invariant = validate invariant property, invariant culture + if (impact.ImpactsOnlyInvariantCulture) + return !(propertyTypeVaries || IsPropertyValid(x, null)); - if (culture == null) - return !(varies || IsPropertyValid(x, null)); // validate invariant property, invariant culture + // impacts all = validate property, all cultures (incl. invariant) + if (impact.ImpactsAllCultures) + return !IsPropertyValid(x); - if (culture == "*") - return !IsPropertyValid(x, culture); // validate property, all cultures + // impacts explicit culture = validate variant property, explicit culture + if (propertyTypeVaries) + return !IsPropertyValid(x, impact.Culture); - return varies - ? !IsPropertyValid(x, culture) // validate variant property, explicit culture - : !IsPropertyValid(x, null); // validate invariant property, explicit culture - }) - .ToArray(); + // and, for explicit culture, we may also have to validate invariant property, invariant culture + // if either + // - it is impacted (default culture), or + // - there is no published version of the content - maybe non-default culture, but no published version + + var alsoInvariant = impact.ImpactsAlsoInvariantProperties || !content.Published; + return alsoInvariant && !IsPropertyValid(x, null); + + }).ToArray(); return invalidProperties.Length == 0; } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index c4f5df9d5a39..a29cc3647a75 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -210,7 +210,9 @@ + + diff --git a/src/Umbraco.Tests/Models/ContentTests.cs b/src/Umbraco.Tests/Models/ContentTests.cs index 05f726893e6f..311608766975 100644 --- a/src/Umbraco.Tests/Models/ContentTests.cs +++ b/src/Umbraco.Tests/Models/ContentTests.cs @@ -104,7 +104,7 @@ public void Variant_Published_Culture_Names_Track_Dirty_Changes() Thread.Sleep(500); //The "Date" wont be dirty if the test runs too fast since it will be the same date content.SetCultureName("name-fr", langFr); - content.PublishCulture(langFr); //we've set the name, now we're publishing it + content.PublishCulture(CultureImpact.Explicit(langFr, false)); //we've set the name, now we're publishing it Assert.IsTrue(content.IsPropertyDirty("PublishCultureInfos")); //now it will be changed since the collection has changed var frCultureName = content.PublishCultureInfos[langFr]; Assert.IsTrue(frCultureName.IsPropertyDirty("Date")); @@ -116,7 +116,7 @@ public void Variant_Published_Culture_Names_Track_Dirty_Changes() Thread.Sleep(500); //The "Date" wont be dirty if the test runs too fast since it will be the same date content.SetCultureName("name-fr", langFr); - content.PublishCulture(langFr); //we've set the name, now we're publishing it + content.PublishCulture(CultureImpact.Explicit(langFr, false)); //we've set the name, now we're publishing it Assert.IsTrue(frCultureName.IsPropertyDirty("Date")); Assert.IsTrue(content.IsPropertyDirty("PublishCultureInfos")); //it's true now since we've updated a name } @@ -303,7 +303,7 @@ public void Can_Deep_Clone() content.SetCultureName("Hello", "en-US"); content.SetCultureName("World", "es-ES"); - content.PublishCulture("en-US"); + content.PublishCulture(CultureImpact.All); // should not try to clone something that's not Published or Unpublished // (and in fact it will not work) @@ -414,7 +414,7 @@ public void Remember_Dirty_Properties() content.SetCultureName("Hello", "en-US"); content.SetCultureName("World", "es-ES"); - content.PublishCulture("en-US"); + content.PublishCulture(CultureImpact.All); var i = 200; foreach (var property in content.Properties) diff --git a/src/Umbraco.Tests/Models/CultureImpactTests.cs b/src/Umbraco.Tests/Models/CultureImpactTests.cs new file mode 100644 index 000000000000..6cad634a1472 --- /dev/null +++ b/src/Umbraco.Tests/Models/CultureImpactTests.cs @@ -0,0 +1,163 @@ +using Moq; +using NUnit.Framework; +using Umbraco.Core.Models; + +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() + { + var impact = CultureImpact.All; + + Assert.AreEqual(impact.Culture, "*"); + + Assert.IsTrue(impact.ImpactsInvariantProperties); + Assert.IsFalse(impact.ImpactsAlsoInvariantProperties); + Assert.IsFalse(impact.ImpactsOnlyInvariantCulture); + Assert.IsFalse(impact.ImpactsExplicitCulture); + Assert.IsTrue(impact.ImpactsAllCultures); + Assert.IsFalse(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void Invariant_Culture() + { + var impact = CultureImpact.Invariant; + + Assert.AreEqual(impact.Culture, null); + + Assert.IsTrue(impact.ImpactsInvariantProperties); + Assert.IsFalse(impact.ImpactsAlsoInvariantProperties); + Assert.IsTrue(impact.ImpactsOnlyInvariantCulture); + Assert.IsFalse(impact.ImpactsExplicitCulture); + Assert.IsFalse(impact.ImpactsAllCultures); + Assert.IsFalse(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void Explicit_Default_Culture() + { + var impact = CultureImpact.Explicit("en-US", true); + + Assert.AreEqual(impact.Culture, "en-US"); + + Assert.IsTrue(impact.ImpactsInvariantProperties); + Assert.IsTrue(impact.ImpactsAlsoInvariantProperties); + Assert.IsFalse(impact.ImpactsOnlyInvariantCulture); + Assert.IsTrue(impact.ImpactsExplicitCulture); + Assert.IsFalse(impact.ImpactsAllCultures); + Assert.IsTrue(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void Explicit_NonDefault_Culture() + { + var impact = CultureImpact.Explicit("en-US", false); + + Assert.AreEqual(impact.Culture, "en-US"); + + Assert.IsFalse(impact.ImpactsInvariantProperties); + Assert.IsFalse(impact.ImpactsAlsoInvariantProperties); + Assert.IsFalse(impact.ImpactsOnlyInvariantCulture); + Assert.IsTrue(impact.ImpactsExplicitCulture); + Assert.IsFalse(impact.ImpactsAllCultures); + Assert.IsFalse(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void TryCreate_Explicit_Default_Culture() + { + var success = CultureImpact.TryCreate("en-US", true, ContentVariation.Culture, false, out var impact); + Assert.IsTrue(success); + + Assert.AreEqual(impact.Culture, "en-US"); + + Assert.IsTrue(impact.ImpactsInvariantProperties); + Assert.IsTrue(impact.ImpactsAlsoInvariantProperties); + Assert.IsFalse(impact.ImpactsOnlyInvariantCulture); + Assert.IsTrue(impact.ImpactsExplicitCulture); + Assert.IsFalse(impact.ImpactsAllCultures); + Assert.IsTrue(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void TryCreate_Explicit_NonDefault_Culture() + { + var success = CultureImpact.TryCreate("en-US", false, ContentVariation.Culture, false, out var impact); + Assert.IsTrue(success); + + Assert.AreEqual(impact.Culture, "en-US"); + + Assert.IsFalse(impact.ImpactsInvariantProperties); + Assert.IsFalse(impact.ImpactsAlsoInvariantProperties); + Assert.IsFalse(impact.ImpactsOnlyInvariantCulture); + Assert.IsTrue(impact.ImpactsExplicitCulture); + Assert.IsFalse(impact.ImpactsAllCultures); + Assert.IsFalse(impact.ImpactsOnlyDefaultCulture); + } + + [Test] + public void TryCreate_AllCultures_For_Invariant() + { + var success = CultureImpact.TryCreate("*", false, ContentVariation.Nothing, false, out var impact); + Assert.IsTrue(success); + + Assert.AreEqual(impact.Culture, null); + + Assert.AreSame(CultureImpact.Invariant, impact); + } + + [Test] + public void TryCreate_AllCultures_For_Variant() + { + var success = CultureImpact.TryCreate("*", false, ContentVariation.Culture, false, out var impact); + Assert.IsTrue(success); + + Assert.AreEqual(impact.Culture, "*"); + + Assert.AreSame(CultureImpact.All, impact); + } + + [Test] + public void TryCreate_Invariant_For_Variant() + { + var success = CultureImpact.TryCreate(null, false, ContentVariation.Culture, false, out var impact); + Assert.IsFalse(success); + } + + [Test] + public void TryCreate_Invariant_For_Invariant() + { + var success = CultureImpact.TryCreate(null, false, ContentVariation.Nothing, false, out var impact); + Assert.IsTrue(success); + + Assert.AreSame(CultureImpact.Invariant, impact); + } + } +} diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index 5569ecfa6002..8d64a6d28f2b 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -275,7 +275,7 @@ public void ContentPublishValues() // can publish value // and get edited and published values - Assert.IsTrue(content.PublishCulture()); + Assert.IsTrue(content.PublishCulture(CultureImpact.All)); Assert.AreEqual("a", content.GetValue("prop")); Assert.AreEqual("a", content.GetValue("prop", published: true)); @@ -305,9 +305,9 @@ public void ContentPublishValues() // can publish value // and get edited and published values - Assert.IsFalse(content.PublishCulture(langFr)); // no name + Assert.IsFalse(content.PublishCulture(CultureImpact.Explicit(langFr, false))); // no name content.SetCultureName("name-fr", langFr); - Assert.IsTrue(content.PublishCulture(langFr)); + Assert.IsTrue(content.PublishCulture(CultureImpact.Explicit(langFr, false))); Assert.IsNull(content.GetValue("prop")); Assert.IsNull(content.GetValue("prop", published: true)); Assert.AreEqual("c", content.GetValue("prop", langFr)); @@ -321,7 +321,7 @@ public void ContentPublishValues() Assert.IsNull(content.GetValue("prop", langFr, published: true)); // can publish all - Assert.IsTrue(content.PublishCulture("*")); + Assert.IsTrue(content.PublishCulture(CultureImpact.All)); Assert.IsNull(content.GetValue("prop")); Assert.IsNull(content.GetValue("prop", published: true)); Assert.AreEqual("c", content.GetValue("prop", langFr)); @@ -331,14 +331,14 @@ public void ContentPublishValues() content.UnpublishCulture(langFr); Assert.AreEqual("c", content.GetValue("prop", langFr)); Assert.IsNull(content.GetValue("prop", langFr, published: true)); - content.PublishCulture(langFr); + Assert.IsTrue(content.PublishCulture(CultureImpact.Explicit(langFr, false))); Assert.AreEqual("c", content.GetValue("prop", langFr)); Assert.AreEqual("c", content.GetValue("prop", langFr, published: true)); content.UnpublishCulture(); // clears invariant props if any Assert.IsNull(content.GetValue("prop")); Assert.IsNull(content.GetValue("prop", published: true)); - content.PublishCulture(); // publishes invariant props if any + Assert.IsTrue(content.PublishCulture(CultureImpact.All)); // publishes invariant props if any Assert.IsNull(content.GetValue("prop")); Assert.IsNull(content.GetValue("prop", published: true)); @@ -384,15 +384,20 @@ public void ContentPublishValuesWithMixedPropertyTypeVariations() content.SetCultureName("hello", langFr); - Assert.IsTrue(content.PublishCulture(langFr)); // succeeds because names are ok (not validating properties here) - Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// fails because prop1 is mandatory + //for this test we'll make the french culture the default one - this is needed for publishing invariant property values + var langFrImpact = CultureImpact.Explicit(langFr, true); + + Assert.IsTrue(content.PublishCulture(langFrImpact)); // succeeds because names are ok (not validating properties here) + Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFrImpact));// fails because prop1 is mandatory content.SetValue("prop1", "a", langFr); - Assert.IsTrue(content.PublishCulture(langFr)); // succeeds because names are ok (not validating properties here) - Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// fails because prop2 is mandatory and invariant + Assert.IsTrue(content.PublishCulture(langFrImpact)); // succeeds because names are ok (not validating properties here) + // fails because prop2 is mandatory and invariant and the item isn't published. + // Invariant is validated against the default language except when there isn't a published version, in that case it's always validated. + Assert.IsFalse(propertyValidationService.IsPropertyDataValid(content, out _, langFrImpact)); content.SetValue("prop2", "x"); - Assert.IsTrue(content.PublishCulture(langFr)); // still ok... - Assert.IsTrue(propertyValidationService.IsPropertyDataValid(content, out _, langFr));// now it's ok + Assert.IsTrue(content.PublishCulture(langFrImpact)); // still ok... + Assert.IsTrue(propertyValidationService.IsPropertyDataValid(content, out _, langFrImpact));// now it's ok Assert.AreEqual("a", content.GetValue("prop1", langFr, published: true)); Assert.AreEqual("x", content.GetValue("prop2", published: true)); @@ -423,12 +428,12 @@ public void ContentPublishVariations() content.SetValue("prop", "a-es", langEs); // cannot publish without a name - Assert.IsFalse(content.PublishCulture(langFr)); + Assert.IsFalse(content.PublishCulture(CultureImpact.Explicit(langFr, false))); // works with a name // and then FR is available, and published content.SetCultureName("name-fr", langFr); - Assert.IsTrue(content.PublishCulture(langFr)); + Assert.IsTrue(content.PublishCulture(CultureImpact.Explicit(langFr, false))); // now UK is available too content.SetCultureName("name-uk", langUk); diff --git a/src/Umbraco.Tests/Persistence/NPocoTests/PetaPocoCachesTest.cs b/src/Umbraco.Tests/Persistence/NPocoTests/PetaPocoCachesTest.cs index f558a6449908..5372a12ac211 100644 --- a/src/Umbraco.Tests/Persistence/NPocoTests/PetaPocoCachesTest.cs +++ b/src/Umbraco.Tests/Persistence/NPocoTests/PetaPocoCachesTest.cs @@ -181,13 +181,13 @@ private void CreateStuff(out int id1, out int id2, out int id3, out string alias contentTypeService.Save(contentType); var content1 = MockedContent.CreateSimpleContent(contentType, "Tagged content 1", -1); content1.AssignTags("tags", new[] { "hello", "world", "some", "tags" }); - content1.PublishCulture(); + content1.PublishCulture(CultureImpact.Invariant); contentService.SaveAndPublish(content1); id2 = content1.Id; var content2 = MockedContent.CreateSimpleContent(contentType, "Tagged content 2", -1); content2.AssignTags("tags", new[] { "hello", "world", "some", "tags" }); - content2.PublishCulture(); + content2.PublishCulture(CultureImpact.Invariant); contentService.SaveAndPublish(content2); id3 = content2.Id; diff --git a/src/Umbraco.Tests/Persistence/Repositories/DocumentRepositoryTest.cs b/src/Umbraco.Tests/Persistence/Repositories/DocumentRepositoryTest.cs index 2c4f3f190800..c7a83717a851 100644 --- a/src/Umbraco.Tests/Persistence/Repositories/DocumentRepositoryTest.cs +++ b/src/Umbraco.Tests/Persistence/Repositories/DocumentRepositoryTest.cs @@ -141,8 +141,8 @@ public void CreateVersions() // publish = new edit version content1.SetValue("title", "title"); - ((Content)content1).PublishCulture(); - ((Content)content1).PublishedState = PublishedState.Publishing; + content1.PublishCulture(CultureImpact.Invariant); + content1.PublishedState = PublishedState.Publishing; repository.Save(content1); versions.Add(content1.VersionId); // NEW VERSION @@ -203,8 +203,8 @@ public void CreateVersions() Assert.AreEqual(false, scope.Database.ExecuteScalar($"SELECT published FROM {Constants.DatabaseSchema.Tables.Document} WHERE nodeId=@id", new { id = content1.Id })); // publish = version - ((Content)content1).PublishCulture(); - ((Content)content1).PublishedState = PublishedState.Publishing; + content1.PublishCulture(CultureImpact.Invariant); + content1.PublishedState = PublishedState.Publishing; repository.Save(content1); versions.Add(content1.VersionId); // NEW VERSION @@ -239,8 +239,8 @@ public void CreateVersions() // publish = new version content1.Name = "name-4"; content1.SetValue("title", "title-4"); - ((Content)content1).PublishCulture(); - ((Content)content1).PublishedState = PublishedState.Publishing; + content1.PublishCulture(CultureImpact.Invariant); + content1.PublishedState = PublishedState.Publishing; repository.Save(content1); versions.Add(content1.VersionId); // NEW VERSION @@ -654,7 +654,7 @@ public void GetAllContentManyVersions() // publish them all foreach (var content in result) { - content.PublishCulture(); + content.PublishCulture(CultureImpact.Invariant); repository.Save(content); } diff --git a/src/Umbraco.Tests/Services/ContentServiceTagsTests.cs b/src/Umbraco.Tests/Services/ContentServiceTagsTests.cs index 5e97bea2c10c..6b4f2942f759 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTagsTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTagsTests.cs @@ -517,7 +517,7 @@ public void TagsAreUpdatedWhenContentIsTrashedAndUnTrashed_Tree() allTags = tagService.GetAllContentTags(); Assert.AreEqual(0, allTags.Count()); - content1.PublishCulture(); + content1.PublishCulture(CultureImpact.Invariant); contentService.SaveAndPublish(content1); Assert.IsTrue(content1.Published); @@ -601,7 +601,7 @@ public void TagsAreUpdatedWhenContentIsUnpublishedAndRePublished_Tree() var allTags = tagService.GetAllContentTags(); Assert.AreEqual(0, allTags.Count()); - content1.PublishCulture(); + content1.PublishCulture(CultureImpact.Invariant); contentService.SaveAndPublish(content1); tags = tagService.GetTagsForEntity(content2.Id); diff --git a/src/Umbraco.Tests/Services/ContentServiceTests.cs b/src/Umbraco.Tests/Services/ContentServiceTests.cs index 04cdc2aab708..7d8e000879db 100644 --- a/src/Umbraco.Tests/Services/ContentServiceTests.cs +++ b/src/Umbraco.Tests/Services/ContentServiceTests.cs @@ -732,8 +732,8 @@ public void Can_Unpublish_Content_Variation() IContent content = new Content("content", Constants.System.Root, contentType); content.SetCultureName("content-fr", langFr.IsoCode); content.SetCultureName("content-en", langUk.IsoCode); - content.PublishCulture(langFr.IsoCode); - content.PublishCulture(langUk.IsoCode); + content.PublishCulture(CultureImpact.Explicit(langFr.IsoCode, langFr.IsDefault)); + content.PublishCulture(CultureImpact.Explicit(langUk.IsoCode, langUk.IsDefault)); Assert.IsTrue(content.IsCulturePublished(langFr.IsoCode)); Assert.IsTrue(content.IsCulturePublished(langUk.IsoCode)); @@ -963,6 +963,18 @@ private void ContentServiceOnPublishing(IContentService sender, PublishEventArgs Assert.AreEqual("Home", e.Name); } + [Test] + public void Can_Not_Publish_Invalid_Cultures() + { + var contentService = ServiceContext.ContentService; + var content = Mock.Of(c => c.ContentType == Mock.Of(s => s.Variations == ContentVariation.Culture)); + + Assert.Throws(() => contentService.SaveAndPublish(content, new[] {"*"})); + Assert.Throws(() => contentService.SaveAndPublish(content, new string[] { null })); + Assert.Throws(() => contentService.SaveAndPublish(content, new[] { "*", null })); + Assert.Throws(() => contentService.SaveAndPublish(content, new[] { "en-US", "*", "es-ES" })); + } + [Test] public void Can_Publish_Only_Valid_Content() { @@ -973,10 +985,7 @@ public void Can_Publish_Only_Valid_Content() const int parentId = NodeDto.NodeIdSeed + 2; var contentService = ServiceContext.ContentService; - var content = MockedContent.CreateSimpleContent(contentType, "Invalid Content", parentId); - content.SetValue("author", string.Empty); - contentService.Save(content); - + var parent = contentService.GetById(parentId); var parentPublished = contentService.SaveAndPublish(parent); @@ -986,9 +995,13 @@ public void Can_Publish_Only_Valid_Content() Assert.IsTrue(parentPublished.Success); Assert.IsTrue(parent.Published); + var content = MockedContent.CreateSimpleContent(contentType, "Invalid Content", parentId); + content.SetValue("author", string.Empty); + Assert.IsFalse(content.HasIdentity); + // content cannot publish values because they are invalid var propertyValidationService = new PropertyValidationService(Factory.GetInstance(), ServiceContext.DataTypeService); - var isValid = propertyValidationService.IsPropertyDataValid(content, out var invalidProperties); + var isValid = propertyValidationService.IsPropertyDataValid(content, out var invalidProperties, CultureImpact.Invariant); Assert.IsFalse(isValid); Assert.IsNotEmpty(invalidProperties); @@ -998,7 +1011,12 @@ public void Can_Publish_Only_Valid_Content() Assert.IsFalse(contentPublished.Success); Assert.AreEqual(PublishResultType.FailedPublishContentInvalid, contentPublished.Result); Assert.IsFalse(content.Published); + + //Ensure it saved though + Assert.Greater(content.Id, 0); + Assert.IsTrue(content.HasIdentity); } + [Test] public void Can_Publish_And_Unpublish_Cultures_In_Single_Operation() @@ -1018,7 +1036,7 @@ public void Can_Publish_And_Unpublish_Cultures_In_Single_Operation() content.SetCultureName("name-fr", langFr.IsoCode); content.SetCultureName("name-da", langDa.IsoCode); - content.PublishCulture(langFr.IsoCode); + content.PublishCulture(CultureImpact.Explicit(langFr.IsoCode, langFr.IsDefault)); var result = ((ContentService)ServiceContext.ContentService).CommitDocumentChanges(content); Assert.IsTrue(result.Success); content = ServiceContext.ContentService.GetById(content.Id); @@ -1026,7 +1044,7 @@ public void Can_Publish_And_Unpublish_Cultures_In_Single_Operation() Assert.IsFalse(content.IsCulturePublished(langDa.IsoCode)); content.UnpublishCulture(langFr.IsoCode); - content.PublishCulture(langDa.IsoCode); + content.PublishCulture(CultureImpact.Explicit(langDa.IsoCode, langDa.IsDefault)); result = ((ContentService)ServiceContext.ContentService).CommitDocumentChanges(content); Assert.IsTrue(result.Success); diff --git a/src/Umbraco.Tests/Services/PerformanceTests.cs b/src/Umbraco.Tests/Services/PerformanceTests.cs index 449e933c24b0..9cf38e17891f 100644 --- a/src/Umbraco.Tests/Services/PerformanceTests.cs +++ b/src/Umbraco.Tests/Services/PerformanceTests.cs @@ -216,7 +216,7 @@ private IEnumerable PrimeDbWithLotsOfContent() var result = new List(); ServiceContext.ContentTypeService.Save(contentType1); IContent lastParent = MockedContent.CreateSimpleContent(contentType1); - lastParent.PublishCulture(); + lastParent.PublishCulture(CultureImpact.Invariant); ServiceContext.ContentService.SaveAndPublish(lastParent); result.Add(lastParent); //create 20 deep @@ -230,7 +230,7 @@ private IEnumerable PrimeDbWithLotsOfContent() //only publish evens if (j % 2 == 0) { - content.PublishCulture(); + content.PublishCulture(CultureImpact.Invariant); ServiceContext.ContentService.SaveAndPublish(content); } else diff --git a/src/Umbraco.Tests/Services/PropertyValidationServiceTests.cs b/src/Umbraco.Tests/Services/PropertyValidationServiceTests.cs new file mode 100644 index 000000000000..e1e19918ceaa --- /dev/null +++ b/src/Umbraco.Tests/Services/PropertyValidationServiceTests.cs @@ -0,0 +1,177 @@ +using System.Threading; +using Moq; +using NUnit.Framework; +using Umbraco.Core; +using Umbraco.Core.Models; +using Umbraco.Core.PropertyEditors; +using Umbraco.Core.PropertyEditors.Validators; +using Umbraco.Core.Services; +using Umbraco.Tests.Testing; +using Umbraco.Web.PropertyEditors; + +namespace Umbraco.Tests.Services +{ + [TestFixture] + public class PropertyValidationServiceTests : UmbracoTestBase + { + private void MockObjects(out PropertyValidationService validationService, out IDataType dt) + { + var textService = new Mock(); + textService.Setup(x => x.Localize(It.IsAny(), Thread.CurrentThread.CurrentCulture, null)).Returns("Localized text"); + + var dataTypeService = new Mock(); + var dataType = Mock.Of( + x => x.Configuration == (object)string.Empty //irrelevant but needs a value + && x.DatabaseType == ValueStorageType.Nvarchar + && x.EditorAlias == Constants.PropertyEditors.Aliases.TextBox); + dataTypeService.Setup(x => x.GetDataType(It.IsAny())).Returns(() => dataType); + dt = dataType; + + //new data editor that returns a TextOnlyValueEditor which will do the validation for the properties + var dataEditor = Mock.Of( + x => x.Type == EditorType.PropertyValue + && x.Alias == Constants.PropertyEditors.Aliases.TextBox); + Mock.Get(dataEditor).Setup(x => x.GetValueEditor(It.IsAny())) + .Returns(new CustomTextOnlyValueEditor(new DataEditorAttribute(Constants.PropertyEditors.Aliases.TextBox, "Test Textbox", "textbox"), textService.Object)); + + var propEditors = new PropertyEditorCollection(new DataEditorCollection(new[] { dataEditor })); + + validationService = new PropertyValidationService(propEditors, dataTypeService.Object); + } + + [Test] + public void Validate_Invariant_Properties_On_Variant_Default_Culture() + { + MockObjects(out var validationService, out var dataType); + + var p1 = new Property(new PropertyType(dataType, "test1") { Mandatory = true, Variations = ContentVariation.Culture}); + p1.SetValue("Hello", "en-US"); + var p2 = new Property(new PropertyType(dataType, "test2") { Mandatory = true, Variations = ContentVariation.Nothing }); + p2.SetValue("Hello", null); + var p3 = new Property(new PropertyType(dataType, "test3") { Mandatory = true, Variations = ContentVariation.Culture }); + p3.SetValue(null, "en-US"); //invalid + var p4 = new Property(new PropertyType(dataType, "test4") { Mandatory = true, Variations = ContentVariation.Nothing }); + p4.SetValue(null, null); //invalid + + var content = Mock.Of( + x => x.Published == true //set to published, the default culture will validate invariant anyways + && x.Properties == new PropertyCollection(new[] { p1, p2, p3, p4 })); + + var result = validationService.IsPropertyDataValid(content, out var invalid, CultureImpact.Explicit("en-US", true)); + + Assert.IsFalse(result); + Assert.AreEqual(2, invalid.Length); + } + + [Test] + public void Validate_Invariant_Properties_On_Variant_Non_Default_Culture() + { + MockObjects(out var validationService, out var dataType); + + var p1 = new Property(new PropertyType(dataType, "test1") { Mandatory = true, Variations = ContentVariation.Culture }); + p1.SetValue("Hello", "en-US"); + var p2 = new Property(new PropertyType(dataType, "test2") { Mandatory = true, Variations = ContentVariation.Nothing }); + p2.SetValue("Hello", null); + var p3 = new Property(new PropertyType(dataType, "test3") { Mandatory = true, Variations = ContentVariation.Culture }); + p3.SetValue(null, "en-US"); //invalid + var p4 = new Property(new PropertyType(dataType, "test4") { Mandatory = true, Variations = ContentVariation.Nothing }); + p4.SetValue(null, null); //invalid + + var content = Mock.Of( + x => x.Published == false //set to not published, the non default culture will need to validate invariant too + && x.Properties == new PropertyCollection(new[] { p1, p2, p3, p4 })); + + var result = validationService.IsPropertyDataValid(content, out var invalid, CultureImpact.Explicit("en-US", false)); + + Assert.IsFalse(result); + Assert.AreEqual(2, invalid.Length); + } + + [Test] + public void Validate_Variant_Properties_On_Variant() + { + MockObjects(out var validationService, out var dataType); + + var p1 = new Property(new PropertyType(dataType, "test1") { Mandatory = true, Variations = ContentVariation.Culture }); + p1.SetValue(null, "en-US"); //invalid + var p2 = new Property(new PropertyType(dataType, "test2") { Mandatory = true, Variations = ContentVariation.Nothing }); + p2.SetValue(null, null); //invalid + var p3 = new Property(new PropertyType(dataType, "test3") { Mandatory = true, Variations = ContentVariation.Culture }); + p3.SetValue(null, "en-US"); //ignored because the impact isn't the default lang + the content is published + var p4 = new Property(new PropertyType(dataType, "test4") { Mandatory = true, Variations = ContentVariation.Nothing }); + p4.SetValue(null, null); //ignored because the impact isn't the default lang + the content is published + + var content = Mock.Of( + x => x.Published == true //set to published + && x.Properties == new PropertyCollection(new[] { p1, p2, p3, p4 })); + + var result = validationService.IsPropertyDataValid(content, out var invalid, CultureImpact.Explicit("en-US", false)); + + Assert.IsFalse(result); + Assert.AreEqual(2, invalid.Length); + } + + [Test] + public void Validate_Invariant_Properties_On_Invariant() + { + MockObjects(out var validationService, out var dataType); + + var p1 = new Property(new PropertyType(dataType, "test1") { Mandatory = true, Variations = ContentVariation.Culture }); + p1.SetValue(null, "en-US"); //ignored since this is variant + var p2 = new Property(new PropertyType(dataType, "test2") { Mandatory = true, Variations = ContentVariation.Nothing }); + p2.SetValue(null, null); //invalid + var p3 = new Property(new PropertyType(dataType, "test3") { Mandatory = true, Variations = ContentVariation.Culture }); + p3.SetValue("Hello", "en-US"); //ignored since this is variant + var p4 = new Property(new PropertyType(dataType, "test4") { Mandatory = true, Variations = ContentVariation.Nothing }); + p4.SetValue(null, null); //invalid + + var content = Mock.Of( + x => x.Properties == new PropertyCollection(new[] { p1, p2, p3, p4 })); + + var result = validationService.IsPropertyDataValid(content, out var invalid, CultureImpact.Invariant); + + Assert.IsFalse(result); + Assert.AreEqual(2, invalid.Length); + } + + [Test] + public void Validate_Properties_On_All() + { + MockObjects(out var validationService, out var dataType); + + var p1 = new Property(new PropertyType(dataType, "test1") { Mandatory = true, Variations = ContentVariation.Culture }); + p1.SetValue(null, "en-US"); //invalid + var p2 = new Property(new PropertyType(dataType, "test2") { Mandatory = true, Variations = ContentVariation.Nothing }); + p2.SetValue(null, null); //invalid + var p3 = new Property(new PropertyType(dataType, "test3") { Mandatory = true, Variations = ContentVariation.Culture }); + p3.SetValue(null, "en-US"); //invalid + var p4 = new Property(new PropertyType(dataType, "test4") { Mandatory = true, Variations = ContentVariation.Nothing }); + p4.SetValue(null, null); //invalid + + var content = Mock.Of( + x => x.Properties == new PropertyCollection(new[] { p1, p2, p3, p4 })); + + var result = validationService.IsPropertyDataValid(content, out var invalid, CultureImpact.All); + + Assert.IsFalse(result); + Assert.AreEqual(4, invalid.Length); + } + + + //used so we can inject a mock - we should fix the base class DataValueEditor to be able to have the ILocalizedTextField passed + // in to create the Requried and Regex validators so we aren't using singletons + private class CustomTextOnlyValueEditor : TextOnlyValueEditor + { + private readonly ILocalizedTextService _textService; + + public CustomTextOnlyValueEditor(DataEditorAttribute attribute, ILocalizedTextService textService) : base(attribute) + { + _textService = textService; + } + + public override IValueRequiredValidator RequiredValidator => new RequiredValidator(_textService); + + public override IValueFormatValidator FormatValidator => new RegexValidator(_textService, null); + } + } +} diff --git a/src/Umbraco.Tests/Umbraco.Tests.csproj b/src/Umbraco.Tests/Umbraco.Tests.csproj index 85f4b8d5e096..fb3c8ccea20b 100644 --- a/src/Umbraco.Tests/Umbraco.Tests.csproj +++ b/src/Umbraco.Tests/Umbraco.Tests.csproj @@ -134,6 +134,7 @@ + @@ -153,6 +154,7 @@ + @@ -248,6 +250,7 @@ + diff --git a/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs b/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs new file mode 100644 index 000000000000..3ce43b5fc221 --- /dev/null +++ b/src/Umbraco.Tests/Web/ModelStateExtensionsTests.cs @@ -0,0 +1,81 @@ +using System.ComponentModel.DataAnnotations; +using System.Linq; +using System.Web.Http.ModelBinding; +using Moq; +using NUnit.Framework; +using Umbraco.Core.Services; +using Umbraco.Web; + +namespace Umbraco.Tests.Web +{ + [TestFixture] + public class ModelStateExtensionsTests + { + + [Test] + public void Get_Cultures_With_Errors() + { + var ms = new ModelStateDictionary(); + var localizationService = new Mock(); + localizationService.Setup(x => x.GetDefaultLanguageIsoCode()).Returns("en-US"); + + 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, "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); + Assert.AreEqual("en-US", result[0]); + + ms = new ModelStateDictionary(); + ms.AddCultureValidationError("en-US", "generic culture error"); + + result = ms.GetCulturesWithErrors(localizationService.Object, "en-US"); + + Assert.AreEqual(1, result.Count); + Assert.AreEqual("en-US", result[0]); + } + + [Test] + public void Get_Cultures_With_Property_Errors() + { + var ms = new ModelStateDictionary(); + var localizationService = new Mock(); + localizationService.Setup(x => x.GetDefaultLanguageIsoCode()).Returns("en-US"); + + 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, "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); + Assert.AreEqual("en-US", result[0]); + } + + [Test] + public void Add_Invariant_Property_Error() + { + var ms = new ModelStateDictionary(); + var localizationService = new Mock(); + localizationService.Setup(x => x.GetDefaultLanguageIsoCode()).Returns("en-US"); + + ms.AddPropertyError(new ValidationResult("no header image"), "headerImage", null); //invariant property + + Assert.AreEqual("_Properties.headerImage.invariant", ms.Keys.First()); + } + + [Test] + public void Add_Variant_Property_Error() + { + var ms = new ModelStateDictionary(); + var localizationService = new Mock(); + localizationService.Setup(x => x.GetDefaultLanguageIsoCode()).Returns("en-US"); + + ms.AddPropertyError(new ValidationResult("no header image"), "headerImage", "en-US"); //invariant property + + Assert.AreEqual("_Properties.headerImage.en-US", ms.Keys.First()); + } + } +} diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js index a7ce013bc359..67bb5a64a55f 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/edit.controller.js @@ -336,34 +336,6 @@ // This is a helper method to reduce the amount of code repitition for actions: Save, Publish, SendToPublish function performSave(args) { - // Check that all variants for publishing have a name. - if (args.action === "publish" || args.action === "sendToPublish") { - - if ($scope.content.variants) { - var iVariant; - for (var i = 0; i < $scope.content.variants.length; i++) { - iVariant = $scope.content.variants[i]; - - iVariant.notifications = [];// maybe not needed, need to investigate. - - if(iVariant.publish === true) { - if (iVariant.name == null) { - - var tokens = [iVariant.language.name]; - - localizationService.localize("publish_contentPublishedFailedByMissingName", tokens).then(function (value) { - iVariant.notifications.push({"message": value, "type": 1}); - }); - - return $q.reject(); - } - } - } - } - - } - - //Used to check validility of nested form - coming from Content Apps mostly //Set them all to be invalid var fieldsToRollback = checkValidility(); diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbvariantnotificationlist.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbvariantnotificationlist.directive.js new file mode 100644 index 000000000000..dca7cdbb18e4 --- /dev/null +++ b/src/Umbraco.Web.UI.Client/src/common/directives/components/content/umbvariantnotificationlist.directive.js @@ -0,0 +1,22 @@ +(function () { + 'use strict'; + + function umbNotificationList() { + + var vm = this; + + } + + var umbNotificationListComponent = { + templateUrl: 'views/components/content/umb-variant-notification-list.html', + bindings: { + notifications: "<" + }, + controllerAs: 'vm', + controller: umbNotificationList + }; + + angular.module("umbraco.directives") + .component('umbVariantNotificationList', umbNotificationListComponent); + +})(); diff --git a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js index c027e0778ee6..61f7f039d9de 100644 --- a/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js +++ b/src/Umbraco.Web.UI.Client/src/common/directives/validation/valpropertymsg.directive.js @@ -27,7 +27,10 @@ function valPropertyMsg(serverValidationManager) { var umbPropCtrl = ctrl[2]; scope.currentProperty = umbPropCtrl.property; - var currentCulture = scope.currentProperty.culture; + + //if the property is invariant (no culture), then we will explicitly set it to the string 'invariant' + //since this matches the value that the server will return for an invariant property. + var currentCulture = scope.currentProperty.culture || "invariant"; var watcher = null; diff --git a/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js b/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js index cbe9b561a072..a03a71febe04 100644 --- a/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js +++ b/src/Umbraco.Web.UI.Client/src/common/services/umbdataformatter.service.js @@ -365,7 +365,7 @@ action: action, variants: _.map(displayModel.variants, function(v) { return { - name: v.name, + name: v.name || "", //if its null/empty,we must pass up an empty string else we get json converter errors properties: getContentProperties(v.tabs), culture: v.language ? v.language.culture : null, publish: v.publish, diff --git a/src/Umbraco.Web.UI.Client/src/views/components/content/umb-variant-notification-list.html b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-variant-notification-list.html new file mode 100644 index 000000000000..739aab07304a --- /dev/null +++ b/src/Umbraco.Web.UI.Client/src/views/components/content/umb-variant-notification-list.html @@ -0,0 +1,8 @@ + + + + + {{notification.message}} + + diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js index d81b7e29e33c..8caa38ea17b6 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.controller.js @@ -72,10 +72,11 @@ } function hasAnyData(variant) { - - if(variant.name == null || variant.name.length === 0) { + + if (variant.name == null || variant.name.length === 0) { return false; } + var result = variant.isDirty != null; if(result) return true; diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.html b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.html index e92c16f0ef5c..4ae09049e08e 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.html +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publish.html @@ -10,17 +10,17 @@
-
+
-
- +
+ +
diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publishdescendants.html b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publishdescendants.html index 1c7f21222510..2ec74bcf70aa 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/publishdescendants.html +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/publishdescendants.html @@ -39,9 +39,10 @@
-
+
-
+
{{publishVariantSelectorForm.publishVariantSelector.errorMsg}} - - {{notification.message}} - + +
diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.html b/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.html index a61b1ac95ee6..6116867adb10 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.html +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/save.html @@ -16,9 +16,10 @@
-
+
-
+
{{saveVariantSelectorForm.saveVariantSelector.errorMsg}} - - {{notification.message}} - + +
diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/schedule.html b/src/Umbraco.Web.UI.Client/src/views/content/overlays/schedule.html index 8cdc4a0abc59..e993cd2cf398 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/schedule.html +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/schedule.html @@ -86,7 +86,7 @@
-
+
{{scheduleSelectorForm.saveVariantReleaseDate.errorMsg}}
-
-
{{notification.message}}
-
+
diff --git a/src/Umbraco.Web.UI.Client/src/views/content/overlays/sendtopublish.html b/src/Umbraco.Web.UI.Client/src/views/content/overlays/sendtopublish.html index cb7bcc79afd5..6ae777ec83f1 100644 --- a/src/Umbraco.Web.UI.Client/src/views/content/overlays/sendtopublish.html +++ b/src/Umbraco.Web.UI.Client/src/views/content/overlays/sendtopublish.html @@ -12,7 +12,7 @@
-
+
{{publishVariantSelectorForm.publishVariantSelector.errorMsg}} - - {{notification.message}} - + +
diff --git a/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml b/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml index 507dde4d7c22..d2daea059d28 100644 --- a/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml +++ b/src/Umbraco.Web.UI/Umbraco/config/lang/en_us.xml @@ -1241,6 +1241,7 @@ To manage your website, simply open the Umbraco back office and start adding con %0% can not be published, because a parent page is not published. ]]> + Validation failed for required language '%0%'. This language was saved but not published. Publishing in progress - please wait... %0% out of %1% pages have been published... %0% has been published @@ -1408,7 +1409,7 @@ To manage your website, simply open the Umbraco back office and start adding con User %0% was deleted Invite user Invitation has been re-sent to %0% - Cannot publish the document since the required '%0%' is not published + Cannot publish the document since the required '%0%' is not published Validation failed for language '%0%' Document type was exported to file An error occurred while exporting the document type diff --git a/src/Umbraco.Web/Editors/ContentController.cs b/src/Umbraco.Web/Editors/ContentController.cs index 12047b628b28..a3788e459081 100644 --- a/src/Umbraco.Web/Editors/ContentController.cs +++ b/src/Umbraco.Web/Editors/ContentController.cs @@ -359,7 +359,7 @@ public ContentItemDisplay GetEmpty(string contentTypeAlias, int parentId) // translate the content type name if applicable mapped.ContentTypeName = Services.TextService.UmbracoDictionaryTranslate(mapped.ContentTypeName); // if your user type doesn't have access to the Settings section it would not get this property mapped - if(mapped.DocumentType != null) + if (mapped.DocumentType != null) mapped.DocumentType.Name = Services.TextService.UmbracoDictionaryTranslate(mapped.DocumentType.Name); //remove the listview app if it exists @@ -616,56 +616,19 @@ private ContentItemDisplay PostSaveInternal(ContentItemSave contentItem, Func(); - foreach (var variant in contentItem.Variants) - { - var msKey = $"Variants[{variantCount}].Name"; - if (ModelState.ContainsKey(msKey)) - { - if (!variant.Save || IsCreatingAction(contentItem.Action)) - ModelState.Remove(msKey); - else - variantNameErrors.Add(variant.Culture); - } - variantCount++; - } + var passesCriticalValidationRules = ValidateCriticalData(contentItem, out var variantCount); - //We need to manually check the validation results here because: - // * We still need to save the entity even if there are validation value errors - // * Depending on if the entity is new, and if there are non property validation errors (i.e. the name is null) - // then we cannot continue saving, we can only display errors - // * If there are validation errors and they were attempting to publish, we can only save, NOT publish and display - // a message indicating this - if (ModelState.IsValid == false) + //we will continue to save if model state is invalid, however we cannot save if critical data is missing. + if (!ModelState.IsValid) { - //another special case, if there's more than 1 variant, then we need to add the culture specific error - //messages based on the variants in error so that the messages show in the publish/save dialog - if (variantCount > 1) + //check for critical data validation issues, we can't continue saving if this data is invalid + if (!passesCriticalValidationRules) { - foreach (var c in variantNameErrors) - { - AddCultureValidationError(c, "speechBubbles/contentCultureValidationError"); - } - } - - if (IsCreatingAction(contentItem.Action)) - { - if (!RequiredForPersistenceAttribute.HasRequiredValuesForPersistence(contentItem) - || contentItem.Variants - .Where(x => x.Save) - .Select(RequiredForPersistenceAttribute.HasRequiredValuesForPersistence) - .Any(x => x == false)) - { - //ok, so the absolute mandatory data is invalid and it's new, we cannot actually continue! - // add the model state to the outgoing object and throw a validation message - var forDisplay = MapToDisplay(contentItem.PersistedContent); - forDisplay.Errors = ModelState.ToErrorDictionary(); - throw new HttpResponseException(Request.CreateValidationErrorResponse(forDisplay)); - } + //ok, so the absolute mandatory data is invalid and it's new, we cannot actually continue! + // add the model state to the outgoing object and throw a validation message + var forDisplay = MapToDisplay(contentItem.PersistedContent); + forDisplay.Errors = ModelState.ToErrorDictionary(); + throw new HttpResponseException(Request.CreateValidationErrorResponse(forDisplay)); } //if there's only one variant and the model state is not valid we cannot publish so change it to save @@ -689,7 +652,6 @@ private ContentItemDisplay PostSaveInternal(ContentItemSave contentItem, Func 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: @@ -716,7 +686,7 @@ private ContentItemDisplay PostSaveInternal(ContentItemSave contentItem, Func 1) { - var cultureErrors = ModelState.GetCulturesWithPropertyErrors(); + 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, @@ -746,7 +716,7 @@ private ContentItemDisplay PostSaveInternal(ContentItemSave contentItem, Func + /// Validates critical data for persistence and updates the ModelState and result accordingly + /// + /// + /// Returns the total number of variants (will be one if it's an invariant content item) + /// + /// + /// For invariant, the variants collection count will be 1 and this will check if that invariant item has the critical values for persistence (i.e. Name) + /// + /// For variant, each variant will be checked for critical data for persistence and if it's not there then it's flags will be reset and it will not + /// be persisted. However, we also need to deal with the case where all variants don't pass this check and then there is nothing to save. This also deals + /// with removing the Name validation keys based on data annotations validation for items that haven't been marked to be saved. + /// + /// + /// returns false if persistence cannot take place, returns true if persistence can take place even if there are validation errors + /// + private bool ValidateCriticalData(ContentItemSave contentItem, out int variantCount) + { + var variants = contentItem.Variants.ToList(); + variantCount = variants.Count; + var savedCount = 0; + var variantCriticalValidationErrors = new List(); + for (var i = 0; i < variants.Count; i++) + { + var variant = variants[i]; + if (variant.Save) + { + //ensure the variant has all critical required data to be persisted + if (!RequiredForPersistenceAttribute.HasRequiredValuesForPersistence(variant)) + { + variantCriticalValidationErrors.Add(variant.Culture); + //if there's no Name, it cannot be persisted at all reset the flags, this cannot be saved or published + variant.Save = variant.Publish = false; + + //if there's more than 1 variant, then we need to add the culture specific error + //messages based on the variants in error so that the messages show in the publish/save dialog + if (variants.Count > 1) + AddCultureValidationError(variant.Culture, "publish/contentPublishedFailedByMissingName"); + else + return false; //It's invariant and is missing critical data, it cannot be saved + } + + savedCount++; + } + else + { + var msKey = $"Variants[{i}].Name"; + if (ModelState.ContainsKey(msKey)) + { + //if it's not being saved, remove the validation key + if (!variant.Save) ModelState.Remove(msKey); + } + } + } + + if (savedCount == variantCriticalValidationErrors.Count) + { + //in this case there can be nothing saved since all variants marked to be saved haven't passed critical validation rules + return false; + } + + return true; + } + + + /// /// Helper method to perform the saving of the content and add the notifications to the result /// @@ -847,7 +883,7 @@ private ContentItemDisplay PostSaveInternal(ContentItemSave contentItem, Func 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); @@ -856,7 +892,7 @@ private void SaveAndNotify(ContentItemSave contentItem, Func 1) { - var cultureErrors = ModelState.GetCulturesWithPropertyErrors(); + 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, @@ -1098,7 +1134,7 @@ private bool ValidatePublishBranchPermissions(ContentItemSave contentItem, out I 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()) @@ -1116,16 +1152,19 @@ private IEnumerable PublishBranchInternal(ContentItemSave content var mandatoryCultures = _allLangs.Value.Values.Where(x => x.IsMandatory).Select(x => x.IsoCode).ToList(); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); + //validate if we can publish based on the mandatory language requirements var canPublish = ValidatePublishingMandatoryLanguages( - contentItem, cultureVariants, mandatoryCultures, "speechBubbles/contentReqCulturePublishError", - mandatoryVariant => mandatoryVariant.Publish, out var _); + cultureErrors, + contentItem, cultureVariants, mandatoryCultures, + mandatoryVariant => mandatoryVariant.Publish); //Now check if there are validation errors on each variant. //If validation errors are detected on a variant and it's state is set to 'publish', then we //need to change it to 'save'. //It is a requirement that this is performed AFTER ValidatePublishingMandatoryLanguages. - var cultureErrors = ModelState.GetCulturesWithPropertyErrors(); + foreach (var variant in contentItem.Variants) { if (cultureErrors.Contains(variant.Culture)) @@ -1169,7 +1208,7 @@ private IEnumerable PublishBranchInternal(ContentItemSave content /// /// 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()) { @@ -1185,16 +1224,21 @@ private PublishResult PublishInternal(ContentItemSave contentItem, out bool wasC var mandatoryCultures = _allLangs.Value.Values.Where(x => x.IsMandatory).Select(x => x.IsoCode).ToList(); - //validate if we can publish based on the mandatory language requirements + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); + + //validate if we can publish based on the mandatory languages selected var canPublish = ValidatePublishingMandatoryLanguages( - contentItem, cultureVariants, mandatoryCultures, "speechBubbles/contentReqCulturePublishError", - mandatoryVariant => mandatoryVariant.Publish, out var _); + cultureErrors, + contentItem, cultureVariants, mandatoryCultures, + mandatoryVariant => mandatoryVariant.Publish); + + //if none are published and there are validation errors for mandatory cultures, then we can't publish anything + //Now check if there are validation errors on each variant. //If validation errors are detected on a variant and it's state is set to 'publish', then we //need to change it to 'save'. - //It is a requirement that this is performed AFTER ValidatePublishingMandatoryLanguages. - var cultureErrors = ModelState.GetCulturesWithPropertyErrors(); + //It is a requirement that this is performed AFTER ValidatePublishingMandatoryLanguages. foreach (var variant in contentItem.Variants) { if (cultureErrors.Contains(variant.Culture)) @@ -1209,7 +1253,7 @@ private PublishResult PublishInternal(ContentItemSave contentItem, out bool wasC { //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) @@ -1234,23 +1278,21 @@ private PublishResult PublishInternal(ContentItemSave contentItem, out bool wasC /// /// Validate if publishing is possible based on the mandatory language requirements /// + /// /// /// /// - /// /// - /// /// private bool ValidatePublishingMandatoryLanguages( + IReadOnlyCollection culturesWithValidationErrors, ContentItemSave contentItem, IReadOnlyCollection cultureVariants, IReadOnlyList mandatoryCultures, - string localizationKey, - Func publishingCheck, - out IReadOnlyList<(ContentVariantSave mandatoryVariant, bool isPublished)> mandatoryVariants) + Func publishingCheck) { var canPublish = true; - var result = new List<(ContentVariantSave, bool)>(); + var result = new List<(ContentVariantSave model, bool publishing, bool isValid)>(); foreach (var culture in mandatoryCultures) { @@ -1259,18 +1301,39 @@ private bool ValidatePublishingMandatoryLanguages( var mandatoryVariant = cultureVariants.First(x => x.Culture.InvariantEquals(culture)); var isPublished = contentItem.PersistedContent.Published && contentItem.PersistedContent.IsCulturePublished(culture); - result.Add((mandatoryVariant, isPublished)); - var isPublishing = isPublished || publishingCheck(mandatoryVariant); + var isValid = !culturesWithValidationErrors.InvariantContains(culture); - if (isPublished || isPublishing) continue; + result.Add((mandatoryVariant, isPublished || isPublishing, isValid)); + } + + //iterate over the results by invalid first + string firstInvalidMandatoryCulture = null; + foreach (var r in result.OrderBy(x => x.isValid)) + { + if (!r.isValid) + firstInvalidMandatoryCulture = r.model.Culture; - //cannot continue publishing since a required language that is not currently being published isn't published - AddCultureValidationError(culture, localizationKey); - canPublish = false; + if (r.publishing && !r.isValid) + { + //flagged for publishing but the mandatory culture is invalid + AddCultureValidationError(r.model.Culture, "publish/contentPublishedFailedReqCultureValidationError"); + canPublish = false; + } + else if (r.publishing && r.isValid && firstInvalidMandatoryCulture != null) + { + //in this case this culture also cannot be published because another mandatory culture is invalid + AddCultureValidationError(r.model.Culture, "publish/contentPublishedFailedReqCultureValidationError", firstInvalidMandatoryCulture); + canPublish = false; + } + else if (!r.publishing) + { + //cannot continue publishing since a required culture that is not currently being published isn't published + AddCultureValidationError(r.model.Culture, "speechBubbles/contentReqCulturePublishError"); + canPublish = false; + } } - mandatoryVariants = result; return canPublish; } @@ -1283,12 +1346,12 @@ private bool ValidatePublishingMandatoryLanguages( /// /// 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(variant.Culture); + var valid = persistentContent.PublishCulture(CultureImpact.Explicit(variant.Culture, defaultCulture.InvariantEquals(variant.Culture))); if (!valid) { AddCultureValidationError(variant.Culture, "speechBubbles/contentCultureValidationError"); @@ -1302,14 +1365,15 @@ private bool PublishCulture(IContent persistentContent, IEnumerable /// Adds a generic culture error for use in displaying the culture validation error in the save/publish/etc... dialogs /// - /// + /// Culture to assign the error to /// - private void AddCultureValidationError(string culture, string localizationKey) + /// + /// The culture used in the localization message, null by default which means will be used. + /// + private void AddCultureValidationError(string culture, string localizationKey, string cultureToken = null) { - var key = "_content_variant_" + culture + "_"; - if (ModelState.ContainsKey(key)) return; - var errMsg = Services.TextService.Localize(localizationKey, new[] { _allLangs.Value[culture].CultureName }); - ModelState.AddModelError(key, errMsg); + var errMsg = Services.TextService.Localize(localizationKey, new[] { cultureToken == null ? _allLangs.Value[culture].CultureName : _allLangs.Value[cultureToken].CultureName }); + ModelState.AddCultureValidationError(culture, errMsg); } /// @@ -1336,7 +1400,7 @@ public HttpResponseMessage PostPublishById(int id) if (publishResult.Success == false) { var notificationModel = new SimpleNotificationModel(); - AddMessageForPublishStatus(new [] { publishResult }, notificationModel); + AddMessageForPublishStatus(new[] { publishResult }, notificationModel); return Request.CreateValidationErrorResponse(notificationModel); } @@ -1725,18 +1789,19 @@ public DomainSave PostSaveLanguageAndDomains(DomainSave model) } /// - /// Override to ensure there is culture specific errors in the result if any errors are for culture properties + /// Ensure there is culture specific errors in the result if any errors are for culture properties + /// and we're dealing with variant content, then call the base class HandleInvalidModelState /// /// /// /// This is required to wire up the validation in the save/publish dialog /// - protected override void HandleInvalidModelState(IErrorModel display) + private void HandleInvalidModelState(ContentItemDisplay display, string cultureForInvariantErrors) { - if (!ModelState.IsValid) + if (!ModelState.IsValid && display.Variants.Count() > 1) { //Add any culture specific errors here - var cultureErrors = ModelState.GetCulturesWithPropertyErrors(); + var cultureErrors = ModelState.GetCulturesWithErrors(Services.LocalizationService, cultureForInvariantErrors); foreach (var cultureError in cultureErrors) { @@ -1745,8 +1810,9 @@ protected override void HandleInvalidModelState(IErrorModel display) } base.HandleInvalidModelState(display); - } + } + /// /// Maps the dto property values and names to the persisted model /// @@ -1886,12 +1952,12 @@ private IContent ValidateMoveOrCopy(MoveOrCopy model) /// /// Adds notification messages to the outbound display model for a given published status /// - /// + /// /// /// /// This is null when dealing with invariant content, else it's the cultures that were successfully published /// - private void AddMessageForPublishStatus(IEnumerable statuses, INotificationModel display, string[] successfulCultures = null) + private void AddMessageForPublishStatus(IReadOnlyCollection statuses, INotificationModel display, string[] successfulCultures = null) { var totalStatusCount = statuses.Count(); @@ -1990,7 +2056,8 @@ private void AddMessageForPublishStatus(IEnumerable statuses, INo break; case PublishResultType.FailedPublishPathNotPublished: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); + //TODO: This doesn't take into account variations with the successfulCultures param + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); display.AddWarningNotification( Services.TextService.Localize("publish"), Services.TextService.Localize("publish/contentPublishedFailedByParent", @@ -1999,13 +2066,15 @@ private void AddMessageForPublishStatus(IEnumerable statuses, INo break; case PublishResultType.FailedPublishCancelledByEvent: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); + //TODO: This doesn't take into account variations with the successfulCultures param + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); AddCancelMessage(display, message: "publish/contentPublishedFailedByEvent", messageParams: new[] { names }); } break; case PublishResultType.FailedPublishAwaitingRelease: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); + //TODO: This doesn't take into account variations with the successfulCultures param + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); display.AddWarningNotification( Services.TextService.Localize("publish"), Services.TextService.Localize("publish/contentPublishedFailedAwaitingRelease", @@ -2014,7 +2083,8 @@ private void AddMessageForPublishStatus(IEnumerable statuses, INo break; case PublishResultType.FailedPublishHasExpired: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); + //TODO: This doesn't take into account variations with the successfulCultures param + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); display.AddWarningNotification( Services.TextService.Localize("publish"), Services.TextService.Localize("publish/contentPublishedFailedExpired", @@ -2023,7 +2093,8 @@ private void AddMessageForPublishStatus(IEnumerable statuses, INo break; case PublishResultType.FailedPublishIsTrashed: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); + //TODO: This doesn't take into account variations with the successfulCultures param + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); display.AddWarningNotification( Services.TextService.Localize("publish"), Services.TextService.Localize("publish/contentPublishedFailedIsTrashed", @@ -2032,11 +2103,25 @@ private void AddMessageForPublishStatus(IEnumerable statuses, INo break; case PublishResultType.FailedPublishContentInvalid: { - var names = string.Join(", ", status.Select(x => $"{x.Content.Name} ({x.Content.Id})")); - display.AddWarningNotification( - Services.TextService.Localize("publish"), - Services.TextService.Localize("publish/contentPublishedFailedInvalid", - new[] { names }).Trim()); + if (successfulCultures == null) + { + var names = string.Join(", ", status.Select(x => $"'{x.Content.Name}'")); + display.AddWarningNotification( + Services.TextService.Localize("publish"), + Services.TextService.Localize("publish/contentPublishedFailedInvalid", + new[] { names }).Trim()); + } + else + { + foreach (var c in successfulCultures) + { + var names = string.Join(", ", status.Select(x => $"'{x.Content.GetCultureName(c)}'")); + display.AddWarningNotification( + Services.TextService.Localize("publish"), + Services.TextService.Localize("publish/contentPublishedFailedInvalid", + new[] { names }).Trim()); + } + } } break; case PublishResultType.FailedPublishMandatoryCultureMissing: diff --git a/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs b/src/Umbraco.Web/Editors/Filters/ContentModelValidator.cs similarity index 71% rename from src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs rename to src/Umbraco.Web/Editors/Filters/ContentModelValidator.cs index eacc904d9bb3..4acf0c948e4b 100644 --- a/src/Umbraco.Web/Editors/Filters/ContentItemValidationHelper.cs +++ b/src/Umbraco.Web/Editors/Filters/ContentModelValidator.cs @@ -8,6 +8,7 @@ using Umbraco.Core; using Umbraco.Core.Logging; using Umbraco.Core.Models; +using Umbraco.Core.PropertyEditors; using Umbraco.Web.Models.ContentEditing; namespace Umbraco.Web.Editors.Filters @@ -15,12 +16,12 @@ namespace Umbraco.Web.Editors.Filters /// /// A base class purely used for logging without generics /// - internal class ContentItemValidationHelper + internal abstract class ContentModelValidator { protected IUmbracoContextAccessor UmbracoContextAccessor { get; } protected ILogger Logger { get; } - public ContentItemValidationHelper(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) + protected ContentModelValidator(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) { Logger = logger ?? throw new ArgumentNullException(nameof(logger)); UmbracoContextAccessor = umbracoContextAccessor ?? throw new ArgumentNullException(nameof(umbracoContextAccessor)); @@ -32,26 +33,20 @@ public ContentItemValidationHelper(ILogger logger, IUmbracoContextAccessor umbra /// /// /// + /// /// /// If any severe errors occur then the response gets set to an error and execution will not continue. Property validation /// errors will just be added to the ModelState. /// - internal class ContentItemValidationHelper: ContentItemValidationHelper + internal abstract class ContentModelValidator: ContentModelValidator where TPersisted : class, IContentBase where TModelSave: IContentSave + where TModelWithProperties : IContentProperties { - public ContentItemValidationHelper(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) : base(logger, umbracoContextAccessor) + protected ContentModelValidator(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) : base(logger, umbracoContextAccessor) { } - - //public void ValidateItem(HttpActionContext actionContext, TModelSave model, IContentProperties modelWithProperties, ContentPropertyCollectionDto dto) - //{ - // //now do each validation step - // if (ValidateExistingContent(model, actionContext) == false) return; - // if (ValidateProperties(model, modelWithProperties, actionContext) == false) return; - // if (ValidatePropertyData(model, modelWithProperties, dto, actionContext.ModelState) == false) return; - //} - + /// /// Ensure the content exists /// @@ -119,13 +114,13 @@ protected bool ValidateProperties(List postedProperties, L /// /// All property data validation goes into the model state with a prefix of "Properties" /// - public virtual bool ValidatePropertyData( + public virtual bool ValidatePropertiesData( TModelSave model, - IContentProperties modelWithProperties, + TModelWithProperties modelWithProperties, ContentPropertyCollectionDto dto, ModelStateDictionary modelState) { - var properties = modelWithProperties.Properties.ToList(); + var properties = modelWithProperties.Properties.ToDictionary(x => x.Alias, x => x); foreach (var p in dto.Properties) { @@ -135,35 +130,49 @@ public virtual bool ValidatePropertyData( { var message = $"Could not find property editor \"{p.DataType.EditorAlias}\" for property with id {p.Id}."; - Logger.Warn(message); + Logger.Warn(message); continue; } //get the posted value for this property, this may be null in cases where the property was marked as readonly which means //the angular app will not post that value. - var postedProp = properties.FirstOrDefault(x => x.Alias == p.Alias); - if (postedProp == null) continue; + if (!properties.TryGetValue(p.Alias, out var postedProp)) + continue; var postedValue = postedProp.Value; - // validate - var valueEditor = editor.GetValueEditor(p.DataType.Configuration); - foreach (var r in valueEditor.Validate(postedValue, p.IsRequired, p.ValidationRegExp)) - { - //this could be a thing, but it does make the errors seem very verbose - ////update the error message to include the property name and culture if available - //r.ErrorMessage = p.Culture.IsNullOrWhiteSpace() - // ? $"'{p.Label}' - {r.ErrorMessage}" - // : $"'{p.Label}' ({p.Culture}) - {r.ErrorMessage}"; - - modelState.AddPropertyError(r, p.Alias, p.Culture); - } - + ValidatePropertyValue(model, modelWithProperties, editor, p, postedValue, modelState); + } return modelState.IsValid; } + /// + /// Validates a property's value and adds the error to model state if found + /// + /// + /// + /// + /// + /// + /// + protected virtual void ValidatePropertyValue( + TModelSave model, + TModelWithProperties modelWithProperties, + IDataEditor editor, + ContentPropertyDto property, + object postedValue, + ModelStateDictionary modelState) + { + // validate + var valueEditor = editor.GetValueEditor(property.DataType.Configuration); + foreach (var r in valueEditor.Validate(postedValue, property.IsRequired, property.ValidationRegExp)) + { + modelState.AddPropertyError(r, property.Alias, property.Culture); + } + } + } } diff --git a/src/Umbraco.Web/Editors/Filters/ContentSaveModelValidator.cs b/src/Umbraco.Web/Editors/Filters/ContentSaveModelValidator.cs new file mode 100644 index 000000000000..b74cd71f1114 --- /dev/null +++ b/src/Umbraco.Web/Editors/Filters/ContentSaveModelValidator.cs @@ -0,0 +1,19 @@ +using System.Web.Http.ModelBinding; +using Umbraco.Core; +using Umbraco.Core.Logging; +using Umbraco.Core.Models; +using Umbraco.Core.PropertyEditors; +using Umbraco.Web.Models.ContentEditing; + +namespace Umbraco.Web.Editors.Filters +{ + /// + /// Validator for + /// + internal class ContentSaveModelValidator : ContentModelValidator + { + public ContentSaveModelValidator(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) : base(logger, umbracoContextAccessor) + { + } + } +} diff --git a/src/Umbraco.Web/Editors/Filters/ContentSaveValidationAttribute.cs b/src/Umbraco.Web/Editors/Filters/ContentSaveValidationAttribute.cs index 6286572946c0..18fd334b1c23 100644 --- a/src/Umbraco.Web/Editors/Filters/ContentSaveValidationAttribute.cs +++ b/src/Umbraco.Web/Editors/Filters/ContentSaveValidationAttribute.cs @@ -44,7 +44,7 @@ public ContentSaveValidationAttribute(ILogger logger, IUmbracoContextAccessor um public override void OnActionExecuting(HttpActionContext actionContext) { var model = (ContentItemSave)actionContext.ActionArguments["contentItem"]; - var contentItemValidator = new ContentItemValidationHelper(_logger, _umbracoContextAccessor); + var contentItemValidator = new ContentSaveModelValidator(_logger, _umbracoContextAccessor); if (!ValidateAtLeastOneVariantIsBeingSaved(model, actionContext)) return; if (!contentItemValidator.ValidateExistingContent(model, actionContext)) return; @@ -54,7 +54,7 @@ public override void OnActionExecuting(HttpActionContext actionContext) foreach (var variant in model.Variants.Where(x => x.Save)) { if (contentItemValidator.ValidateProperties(model, variant, actionContext)) - contentItemValidator.ValidatePropertyData(model, variant, variant.PropertyCollectionDto, actionContext.ModelState); + contentItemValidator.ValidatePropertiesData(model, variant, variant.PropertyCollectionDto, actionContext.ModelState); } } diff --git a/src/Umbraco.Web/Editors/Filters/MediaItemSaveValidationAttribute.cs b/src/Umbraco.Web/Editors/Filters/MediaItemSaveValidationAttribute.cs index eda2bb8d53b5..7351c476e48b 100644 --- a/src/Umbraco.Web/Editors/Filters/MediaItemSaveValidationAttribute.cs +++ b/src/Umbraco.Web/Editors/Filters/MediaItemSaveValidationAttribute.cs @@ -39,14 +39,14 @@ public MediaItemSaveValidationAttribute(ILogger logger, IUmbracoContextAccessor public override void OnActionExecuting(HttpActionContext actionContext) { var model = (MediaItemSave)actionContext.ActionArguments["contentItem"]; - var contentItemValidator = new ContentItemValidationHelper(_logger, _umbracoContextAccessor); + var contentItemValidator = new MediaSaveModelValidator(_logger, _umbracoContextAccessor); if (ValidateUserAccess(model, actionContext)) { //now do each validation step if (contentItemValidator.ValidateExistingContent(model, actionContext)) if (contentItemValidator.ValidateProperties(model, model, actionContext)) - contentItemValidator.ValidatePropertyData(model, model, model.PropertyCollectionDto, actionContext.ModelState); + contentItemValidator.ValidatePropertiesData(model, model, model.PropertyCollectionDto, actionContext.ModelState); } } diff --git a/src/Umbraco.Web/Editors/Filters/MediaSaveModelValidator.cs b/src/Umbraco.Web/Editors/Filters/MediaSaveModelValidator.cs new file mode 100644 index 000000000000..83c0885d6221 --- /dev/null +++ b/src/Umbraco.Web/Editors/Filters/MediaSaveModelValidator.cs @@ -0,0 +1,16 @@ +using Umbraco.Core.Logging; +using Umbraco.Core.Models; +using Umbraco.Web.Models.ContentEditing; + +namespace Umbraco.Web.Editors.Filters +{ + /// + /// Validator for + /// + internal class MediaSaveModelValidator : ContentModelValidator> + { + public MediaSaveModelValidator(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor) : base(logger, umbracoContextAccessor) + { + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Web/Editors/Filters/MemberValidationHelper.cs b/src/Umbraco.Web/Editors/Filters/MemberSaveModelValidator.cs similarity index 94% rename from src/Umbraco.Web/Editors/Filters/MemberValidationHelper.cs rename to src/Umbraco.Web/Editors/Filters/MemberSaveModelValidator.cs index ac72019cdfa6..7fc2b1e6487e 100644 --- a/src/Umbraco.Web/Editors/Filters/MemberValidationHelper.cs +++ b/src/Umbraco.Web/Editors/Filters/MemberSaveModelValidator.cs @@ -14,15 +14,14 @@ namespace Umbraco.Web.Editors.Filters { - /// /// Custom validation helper so that we can exclude the Member.StandardPropertyTypeStubs from being validating for existence /// - internal class MemberValidationHelper : ContentItemValidationHelper + internal class MemberSaveModelValidator : ContentModelValidator> { private readonly IMemberTypeService _memberTypeService; - public MemberValidationHelper(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor, IMemberTypeService memberTypeService) + public MemberSaveModelValidator(ILogger logger, IUmbracoContextAccessor umbracoContextAccessor, IMemberTypeService memberTypeService) : base(logger, umbracoContextAccessor) { _memberTypeService = memberTypeService; @@ -36,7 +35,7 @@ public MemberValidationHelper(ILogger logger, IUmbracoContextAccessor umbracoCon /// /// /// - public override bool ValidatePropertyData(MemberSave model, IContentProperties modelWithProperties, ContentPropertyCollectionDto dto, ModelStateDictionary modelState) + public override bool ValidatePropertiesData(MemberSave model, IContentProperties modelWithProperties, ContentPropertyCollectionDto dto, ModelStateDictionary modelState) { if (model.Username.IsNullOrWhiteSpace()) { @@ -71,7 +70,7 @@ public override bool ValidatePropertyData(MemberSave model, IContentProperties diff --git a/src/Umbraco.Web/Editors/Filters/MemberSaveValidationAttribute.cs b/src/Umbraco.Web/Editors/Filters/MemberSaveValidationAttribute.cs index c99bc64e23b3..04b0112d56f3 100644 --- a/src/Umbraco.Web/Editors/Filters/MemberSaveValidationAttribute.cs +++ b/src/Umbraco.Web/Editors/Filters/MemberSaveValidationAttribute.cs @@ -30,11 +30,11 @@ public MemberSaveValidationAttribute(ILogger logger, IUmbracoContextAccessor umb public override void OnActionExecuting(HttpActionContext actionContext) { var model = (MemberSave)actionContext.ActionArguments["contentItem"]; - var contentItemValidator = new MemberValidationHelper(_logger, _umbracoContextAccessor, _memberTypeService); + var contentItemValidator = new MemberSaveModelValidator(_logger, _umbracoContextAccessor, _memberTypeService); //now do each validation step if (contentItemValidator.ValidateExistingContent(model, actionContext)) if (contentItemValidator.ValidateProperties(model, model, actionContext)) - contentItemValidator.ValidatePropertyData(model, model, model.PropertyCollectionDto, actionContext.ModelState); + contentItemValidator.ValidatePropertiesData(model, model, model.PropertyCollectionDto, actionContext.ModelState); } } } diff --git a/src/Umbraco.Web/Editors/MediaController.cs b/src/Umbraco.Web/Editors/MediaController.cs index 1411e6aca487..845d9711504b 100644 --- a/src/Umbraco.Web/Editors/MediaController.cs +++ b/src/Umbraco.Web/Editors/MediaController.cs @@ -489,16 +489,12 @@ public MediaItemDisplay PostSave( (save, property, v) => property.SetValue(v), //set prop val null); // media are all invariant - //We need to manually check the validation results here because: - // * We still need to save the entity even if there are validation value errors - // * Depending on if the entity is new, and if there are non property validation errors (i.e. the name is null) - // then we cannot continue saving, we can only display errors - // * If there are validation errors and they were attempting to publish, we can only save, NOT publish and display - // a message indicating this - if (ModelState.IsValid == false) - { - if (!RequiredForPersistenceAttribute.HasRequiredValuesForPersistence(contentItem) - && (contentItem.Action == ContentSaveAction.SaveNew)) + //we will continue to save if model state is invalid, however we cannot save if critical data is missing. + //TODO: Allowing media to be saved when it is invalid is odd - media doesn't have a publish phase so suddenly invalid data is allowed to be 'live' + if (!ModelState.IsValid) + { + //check for critical data validation issues, we can't continue saving if this data is invalid + if (!RequiredForPersistenceAttribute.HasRequiredValuesForPersistence(contentItem)) { //ok, so the absolute mandatory data is invalid and it's new, we cannot actually continue! // add the model state to the outgoing object and throw validation response diff --git a/src/Umbraco.Web/ModelStateExtensions.cs b/src/Umbraco.Web/ModelStateExtensions.cs index a01958be0184..3bbcdfb0ac76 100644 --- a/src/Umbraco.Web/ModelStateExtensions.cs +++ b/src/Umbraco.Web/ModelStateExtensions.cs @@ -4,6 +4,8 @@ using System.Linq; using System.Web.Mvc; using Umbraco.Core; +using Umbraco.Core.Models; +using Umbraco.Core.Services; namespace Umbraco.Web { @@ -51,15 +53,36 @@ internal static void AddPropertyError(this System.Web.Http.ModelBinding.ModelSta { if (culture == null) culture = ""; - modelState.AddValidationError(result, "_Properties", propertyAlias, culture); + modelState.AddValidationError(result, "_Properties", propertyAlias, + //if the culture is null, we'll add the term 'invariant' as part of the key + culture.IsNullOrWhiteSpace() ? "invariant" : culture); } /// - /// Returns a list of cultures that have property errors + /// Adds a generic culture error for use in displaying the culture validation error in the save/publish/etc... dialogs /// /// - /// - internal static IReadOnlyList GetCulturesWithPropertyErrors(this System.Web.Http.ModelBinding.ModelStateDictionary modelState) + /// + /// + internal static void AddCultureValidationError(this System.Web.Http.ModelBinding.ModelStateDictionary modelState, + string culture, string errMsg) + { + var key = "_content_variant_" + culture + "_"; + if (modelState.ContainsKey(key)) return; + modelState.AddModelError(key, errMsg); + } + + /// + /// Returns a list of cultures that have property validation errors errors + /// + /// + /// + /// 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, string cultureForInvariantErrors) { //Add any culture specific errors here var cultureErrors = modelState.Keys @@ -67,12 +90,44 @@ internal static IReadOnlyList GetCulturesWithPropertyErrors(this System. .Where(x => x.Length >= 3 && x[0] == "_Properties") //only choose _Properties errors .Select(x => x[2]) //select the culture part .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" ? cultureForInvariantErrors : x) + .WhereNotNull() .Distinct() .ToList(); return cultureErrors; } + /// + /// Returns a list of cultures that have any validation errors + /// + /// + /// + /// 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, string cultureForInvariantErrors) + { + var propertyCultureErrors = modelState.GetCulturesWithPropertyErrors(localizationService, cultureForInvariantErrors); + + //now check the other special culture errors that are + var genericCultureErrors = modelState.Keys + .Where(x => x.StartsWith("_content_variant_") && x.EndsWith("_")) + .Select(x => x.TrimStart("_content_variant_").TrimEnd("_")) + .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" ? 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. /// diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index d0185c9d8d64..4fe10f65b0ce 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -134,6 +134,8 @@ + + @@ -307,7 +309,7 @@ - + @@ -1062,7 +1064,7 @@ - +