From eca51631b79d87c76113e89d531cbecb0c33ff50 Mon Sep 17 00:00:00 2001 From: Stephan Date: Sat, 2 Dec 2017 13:13:24 +0100 Subject: [PATCH] Refactor validation and publishing --- src/Umbraco.Core/Models/Content.cs | 4 +- src/Umbraco.Core/Models/Property.cs | 49 +++++++++----------- src/Umbraco.Core/Services/ContentService.cs | 22 +++++---- src/Umbraco.Core/Services/IContentService.cs | 4 +- src/Umbraco.Tests/Models/VariationTests.cs | 4 +- 5 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/Umbraco.Core/Models/Content.cs b/src/Umbraco.Core/Models/Content.cs index fdfd962759..b1c3a00734 100644 --- a/src/Umbraco.Core/Models/Content.cs +++ b/src/Umbraco.Core/Models/Content.cs @@ -291,7 +291,7 @@ namespace Umbraco.Core.Models _publishedState = PublishedState.Publishing; } - private bool IsCopyFromSelf(IContent other) + private bool CopyingFromSelf(IContent other) { // copying from the same Id and VersionPk return Id == other.Id && VersionId == other.VersionId; @@ -306,7 +306,7 @@ namespace Umbraco.Core.Models // we could copy from another document entirely, // or from another version of the same document, // in which case there is a special case. - var published = IsCopyFromSelf(other); + var published = CopyingFromSelf(other); // note: use property.SetValue(), don't assign pvalue.EditValue, else change tracking fails diff --git a/src/Umbraco.Core/Models/Property.cs b/src/Umbraco.Core/Models/Property.cs index f6173db0d6..1fbcd3826a 100644 --- a/src/Umbraco.Core/Models/Property.cs +++ b/src/Umbraco.Core/Models/Property.cs @@ -154,11 +154,9 @@ namespace Umbraco.Core.Models } // internal - must be invoked by the content item + // does *not* validate the value - content item must validate first internal void PublishAllValues() { - // throw if some values are not valid - if (!IsAllValid()) throw new InvalidOperationException("Some values are not valid."); - // if invariant-neutral is supported, publish invariant-neutral if (_propertyType.ValidateVariation(null, null, false)) PublishPropertyValue(_pvalue); @@ -175,10 +173,9 @@ namespace Umbraco.Core.Models } // internal - must be invoked by the content item + // does *not* validate the value - content item must validate first internal void PublishValue(int? languageId = null, string segment = null) { - // throw if value is not valid, or if variation is not supported - if (!IsValid(languageId, segment)) throw new InvalidOperationException("Value is not valid."); _propertyType.ValidateVariation(languageId, segment, true); (var pvalue, _) = GetPValue(languageId, segment, false); @@ -187,11 +184,9 @@ namespace Umbraco.Core.Models } // internal - must be invoked by the content item + // does *not* validate the value - content item must validate first internal void PublishCultureValues(int? languageId = null) { - // throw if some values are not valid - if (!IsCultureValid(languageId)) throw new InvalidOperationException("Some values are not valid."); - // if invariant and invariant-neutral is supported, publish invariant-neutral if (!languageId.HasValue && _propertyType.ValidateVariation(null, null, false)) PublishPropertyValue(_pvalue); @@ -393,22 +388,22 @@ namespace Umbraco.Core.Models /// public bool IsAllValid() { - // if invariant-neutral is supported, validate invariant-neutral - if (_propertyType.ValidateVariation(null, null, false)) - if (!IsValidValue(_pvalue)) return false; + // invariant-neutral is supported, validate invariant-neutral + // includes mandatory validation + if (_propertyType.ValidateVariation(null, null, false) && !IsValidValue(_pvalue)) return false; - if (_vvalues == null) return IsValidValue(null); + // either invariant-neutral is not supported, or it is valid + // for anything else, validate the existing values (including mandatory), + // but we cannot validate mandatory globally (we don't know the possible cultures and segments) - // validate everything not invariant-neutral that is supported - // fixme - broken - how can we figure out what is mandatory here? + if (_vvalues == null) return true; var pvalues = _vvalues .Where(x => _propertyType.ValidateVariation(x.Value.LanguageId, x.Value.Segment, false)) .Select(x => x.Value) .ToArray(); - return pvalues.Length == 0 - ? IsValidValue(null) - : pvalues.All(x => IsValidValue(x.EditedValue)); + + return pvalues.Length == 0 || pvalues.All(x => IsValidValue(x.EditedValue)); } /// @@ -417,23 +412,24 @@ namespace Umbraco.Core.Models /// An invalid value can be saved, but only valid values can be published. public bool IsCultureValid(int? languageId) { - // if invariant and invariant-neutral is supported, validate invariant-neutral - if (!languageId.HasValue && _propertyType.ValidateVariation(null, null, false)) - if (!IsValidValue(_pvalue)) return false; + // culture-neutral is supported, validate culture-neutral + // includes mandatory validation + if (_propertyType.ValidateVariation(languageId, null, false) && !IsValidValue(GetValue(languageId))) + return false; - // validate everything not invariant-neutral that matches the culture and is supported - // fixme - broken - how can we figure out what is mandatory here? + // either culture-neutral is not supported, or it is valid + // for anything non-neutral, validate the existing values (including mandatory), + // but we cannot validate mandatory globally (we don't know the possible segments) - if (_vvalues == null) return IsValidValue(null); + if (_vvalues == null) return true; var pvalues = _vvalues .Where(x => x.Value.LanguageId == languageId) .Where(x => _propertyType.ValidateVariation(languageId, x.Value.Segment, false)) .Select(x => x.Value) .ToArray(); - return pvalues.Length == 0 - ? IsValidValue(null) - : pvalues.All(x => IsValidValue(x.EditedValue)); + + return pvalues.Length == 0 || pvalues.All(x => IsValidValue(x.EditedValue)); } /// @@ -442,6 +438,7 @@ namespace Umbraco.Core.Models /// An invalid value can be saved, but only valid values can be published. public bool IsValid(int? languageId = null, string segment = null) { + // single value -> validates mandatory return IsValidValue(GetValue(languageId, segment)); } diff --git a/src/Umbraco.Core/Services/ContentService.cs b/src/Umbraco.Core/Services/ContentService.cs index 1e56ded70f..6d92cce235 100644 --- a/src/Umbraco.Core/Services/ContentService.cs +++ b/src/Umbraco.Core/Services/ContentService.cs @@ -1216,11 +1216,15 @@ namespace Umbraco.Core.Services /// public IEnumerable SaveAndPublishBranch(IContent content, bool force, int? languageId = null, string segment = null, int userId = 0) { - return SaveAndPublishBranch(content, force, new[] { (languageId, segment) }, userId); + bool IsEditing(IContent c, int? l, string s) + => c.Properties.Any(x => x.Values.Where(y => y.LanguageId == l && y.Segment == s).Any(y => y.EditedValue != y.PublishedValue)); + + return SaveAndPublishBranch(content, force, document => IsEditing(document, languageId, segment), document => document.PublishValues(languageId, segment), userId); } /// - public IEnumerable SaveAndPublishBranch(IContent document, bool force, ValueTuple[] variations, int userId = 0) + public IEnumerable SaveAndPublishBranch(IContent document, bool force, + Func editing, Func publishValues, int userId = 0) { var evtMsgs = EventMessagesFactory.Get(); var results = new List(); @@ -1241,7 +1245,7 @@ namespace Umbraco.Core.Services throw new InvalidOperationException("Do not publish values when publishing branches."); // deal with the branch root - if it fails, abort - var result = SaveAndPublishBranchOne(document, repository, uow, variations, true, publishedDocuments, evtMsgs, userId); + var result = SaveAndPublishBranchOne(document, repository, uow, editing, publishValues, true, publishedDocuments, evtMsgs, userId); results.Add(result); if (!result.Success) return results; @@ -1261,7 +1265,7 @@ namespace Umbraco.Core.Services // no need to check path here, // 1. because we know the parent is path-published (we just published it) // 2. because it would not work as nothing's been written out to the db until the uow completes - result = SaveAndPublishBranchOne(d, repository, uow, variations, false, publishedDocuments, evtMsgs, userId); + result = SaveAndPublishBranchOne(d, repository, uow, editing, publishValues, false, publishedDocuments, evtMsgs, userId); results.Add(result); if (result.Success) continue; @@ -1281,20 +1285,18 @@ namespace Umbraco.Core.Services private PublishResult SaveAndPublishBranchOne(IContent document, IContentRepository repository, IScopeUnitOfWork uow, - ValueTuple[] variations, bool checkPath, + Func editing, Func publishValues, + bool checkPath, List publishedDocuments, EventMessages evtMsgs, int userId) { - bool IsEditingContent(IContent content, int? languageId, string segment) - => content.Properties.Any(x => x.Values.Where(y => y.LanguageId == languageId && y.Segment == segment).Any(y => y.EditedValue != y.PublishedValue)); - // if already published, and values haven't changed - i.e. not changing anything // nothing to do - fixme - unless we *want* to bump dates? - if (document.Published && variations.All(x => !IsEditingContent(document, x.Item1, x.Item2))) + if (document.Published && (editing == null || !editing(document))) return new PublishResult(PublishResultType.SuccessAlready, evtMsgs, document); // publish & check if values are valid - if (!variations.All(x => document.PublishValues(x.Item1, x.Item2))) + if (publishValues != null && !publishValues(document)) return new PublishResult(PublishResultType.FailedContentInvalid, evtMsgs, document); // check if we can publish diff --git a/src/Umbraco.Core/Services/IContentService.cs b/src/Umbraco.Core/Services/IContentService.cs index 4cf2bf8569..c8d35104a1 100644 --- a/src/Umbraco.Core/Services/IContentService.cs +++ b/src/Umbraco.Core/Services/IContentService.cs @@ -339,12 +339,12 @@ namespace Umbraco.Core.Services /// /// Saves and publishes a document branch. /// - IEnumerable SaveAndPublishBranch(IContent content, bool force, ValueTuple[] variations, int userId = 0); + IEnumerable SaveAndPublishBranch(IContent content, bool force, int? languageId = null, string segment = null, int userId = 0); /// /// Saves and publishes a document branch. /// - IEnumerable SaveAndPublishBranch(IContent content, bool force, int? languageId = null, string segment = null, int userId = 0); + IEnumerable SaveAndPublishBranch(IContent content, bool force, Func editing, Func publishValues, int userId = 0); /// /// Unpublishes a document. diff --git a/src/Umbraco.Tests/Models/VariationTests.cs b/src/Umbraco.Tests/Models/VariationTests.cs index c8111d4326..a8d6494136 100644 --- a/src/Umbraco.Tests/Models/VariationTests.cs +++ b/src/Umbraco.Tests/Models/VariationTests.cs @@ -259,8 +259,8 @@ namespace Umbraco.Tests.Models prop.SetValue(null); Assert.IsFalse(prop.IsValid()); - // cannot publish, because invalid - Assert.Throws(() => prop.PublishValue()); + // can publish, even though invalid + prop.PublishValue(); } } }